Index | Thread | Search

From:
Alexandre Ratchov <alex@caoua.org>
Subject:
Re: uaudio: Handle certain devices with multiple clock domains, v2
To:
tech@openbsd.org
Date:
Fri, 14 Nov 2025 17:20:25 +0100

Download raw body.

Thread
On Thu, Nov 13, 2025 at 02:11:18PM +0100, Alexandre Ratchov wrote:
> uaudio(4) doesn't support devices with multiple "clock domains". They
> would be almost multiple independent devices in a single piece of
> hardware.
> 
> Physical clocks cost money, so I suspect that most devices have one
> physical clock only, i.e. that the different clocks the device exposes
> are synchronous. If so, uaudio(4) could handle them with the diff
> below.
> 
> To test this, first use a know working uaudio(4) device to validate
> your setup (usb host controller, hub etc). Once you feel audio is
> stable, try the device with the multiple clock domains on the same
> port.
> 
> I'm insisting on this because there are still usb-related bugs, that
> would prevent understanding the cause of a possible failure.
> 

Thanks for all the feedback.

Here's a better version of the diff. It assumes that the clocks are
equivalent if they have the same attributes and at least one common
rate.

This version addresses devices where certain rates are disabled in one
direction i.e. ones that failed with "clock rates differ" with the
previous version of the diff. If your devices just worked with the
previous diff, no need to test this one.

Index: uaudio.c
===================================================================
RCS file: /cvs/src/sys/dev/usb/uaudio.c,v
diff -u -p -u -p -r1.180 uaudio.c
--- uaudio.c	2 Nov 2025 14:35:20 -0000	1.180
+++ uaudio.c	14 Nov 2025 15:54:47 -0000
@@ -263,7 +263,7 @@ struct uaudio_softc {
 	/*
 	 * Current clock, UAC v2.0 only
 	 */
-	struct uaudio_unit *clock;
+	struct uaudio_unit *pclock, *rclock;
 
 	/*
 	 * Number of input and output terminals
@@ -1023,7 +1023,7 @@ uaudio_alt_getrates(struct uaudio_softc 
 	case UAUDIO_V1:
 		return p->v1_rates;
 	case UAUDIO_V2:
-		u = sc->clock;
+		u = p->mode == AUMODE_PLAY ? sc->pclock : sc->rclock;
 		while (1) {
 			switch (u->type) {
 			case UAUDIO_AC_CLKSRC:
@@ -1050,11 +1050,8 @@ uaudio_alt_getrates(struct uaudio_softc 
  * return the clock source unit
  */
 struct uaudio_unit *
-uaudio_clock(struct uaudio_softc *sc)
+uaudio_clock(struct uaudio_unit *u)
 {
-	struct uaudio_unit *u;
-
-	u = sc->clock;
 	while (1) {
 		if (u == NULL) {
 			DPRINTF("%s: NULL clock pointer\n", __func__);
@@ -1081,13 +1078,20 @@ uaudio_clock(struct uaudio_softc *sc)
  * Return the rates bitmap of the given parameters setting
  */
 int
-uaudio_getrates(struct uaudio_softc *sc, struct uaudio_params *p)
+uaudio_getrates(struct uaudio_softc *sc, int mode, struct uaudio_params *p)
 {
+	int rates;
+
 	switch (sc->version) {
 	case UAUDIO_V1:
 		return p->v1_rates;
 	case UAUDIO_V2:
-		return uaudio_alt_getrates(sc, p->palt ? p->palt : p->ralt);
+		rates = ~0;
+		if (p->palt != NULL && (mode & AUMODE_PLAY))
+			rates &= uaudio_alt_getrates(sc, p->palt);
+		if (p->ralt != NULL && (mode & AUMODE_RECORD))
+			rates &= uaudio_alt_getrates(sc, p->ralt);
+		return rates;
 	}
 	return 0;
 }
@@ -1181,6 +1185,38 @@ uaudio_feature_addent(struct uaudio_soft
 }
 
 /*
+ * Compare two clock source units. According to UAC2 each clock
+ * source defines its own clock domain. However there is hardware
+ * exposing multiple clock units that are clocked by the same source
+ * and consequently are equivalent to single domain.
+ */
+int
+uaudio_clock_equiv(struct uaudio_softc *sc,
+	struct uaudio_unit *u, struct uaudio_unit *v)
+{
+	if (u == NULL || v == NULL)
+		return 1;
+
+	u = uaudio_clock(u);
+	if (u == NULL)
+		return 0;
+
+	v = uaudio_clock(v);
+	if (v == NULL)
+		return 0;
+
+	if (u->term != v->term) {
+		printf("%s: clock attributes differ\n", DEVNAME(sc));
+		return 0;
+	}
+
+	if (u != v)
+		printf("%s: warning: assuming common clock\n", DEVNAME(sc));
+
+	return 1;
+}
+
+/*
  * For the given unit, parse the list of its sources and recursively
  * call uaudio_process_unit() for each.
  */
@@ -1887,7 +1923,7 @@ uaudio_conf_print(struct uaudio_softc *s
 			rates = p->v1_rates;
 			break;
 		case UAUDIO_V2:
-			rates = uaudio_getrates(sc, p);
+			rates = uaudio_getrates(sc, AUMODE_PLAY | AUMODE_RECORD, p);
 			break;
 		}
 		printf("pchan = %d, s%dle%d, rchan = %d, s%dle%d, rates:",
@@ -2264,34 +2300,39 @@ uaudio_process_ac(struct uaudio_softc *s
 	}
 
 	if (sc->version == UAUDIO_V2) {
+
 		/*
-		 * Find common clock unit. We assume all terminals
-		 * belong to the same clock domain (ie are connected
-		 * to the same source)
+		 * Find the clocks for the usb-streaming terminal units
 		 */
-		sc->clock = NULL;
+
 		for (u = sc->unit_list; u != NULL; u = u->unit_next) {
-			if (u->type != UAUDIO_AC_INPUT &&
-			    u->type != UAUDIO_AC_OUTPUT)
-				continue;
-			if (sc->clock == NULL) {
+			if (u->type == UAUDIO_AC_INPUT && (u->term >> 8) == 1) {
 				if (u->clock == NULL) {
-					printf("%s: terminal with no clock\n",
-					    DEVNAME(sc));
+					printf("%s: no play clock\n", DEVNAME(sc));
 					return 0;
 				}
-				sc->clock = u->clock;
-			} else if (u->clock != sc->clock) {
-				printf("%s: only one clock domain supported\n",
-				    DEVNAME(sc));
-				return 0;
+				sc->pclock = u->clock;
+				break;
 			}
 		}
 
-		if (sc->clock == NULL) {
-			printf("%s: no clock found\n", DEVNAME(sc));
-			return 0;
+		for (u = sc->unit_list; u != NULL; u = u->unit_next) {
+			if (u->type == UAUDIO_AC_OUTPUT && (u->term >> 8) == 1) {
+				if (u->clock == NULL) {
+					printf("%s: no rec clock\n", DEVNAME(sc));
+					return 0;
+				}
+				sc->rclock = u->clock;
+				break;
+			}
 		}
+
+		DPRINTF("%s: pclock = %d, rclock = %d\n", __func__,
+		    sc->pclock ? sc->pclock->id : -1,
+		    sc->rclock ? sc->rclock->id : -1);
+
+		if (!uaudio_clock_equiv(sc, sc->pclock, sc->rclock))
+			return 0;
 	}
 	return 1;
 }
@@ -3118,7 +3159,7 @@ uaudio_stream_open(struct uaudio_softc *
 		req_buf[1] = sc->rate >> 8;
 		req_buf[2] = sc->rate >> 16;
 		req_buf[3] = sc->rate >> 24;
-		clock = uaudio_clock(sc);
+		clock = uaudio_clock(dir == AUMODE_PLAY ? sc->pclock : sc->rclock);
 		if (clock == NULL) {
 			printf("%s: can't get clock\n", DEVNAME(sc));
 			goto failed;
@@ -3879,7 +3920,8 @@ uaudio_attach(struct device *parent, str
 	sc->names = NULL;
 	sc->alts = NULL;
 	sc->params_list = NULL;
-	sc->clock = NULL;
+	sc->rclock = NULL;
+	sc->pclock = NULL;
 	sc->params = NULL;
 	sc->rate = 0;
 	sc->mode = 0;
@@ -4059,7 +4101,7 @@ uaudio_set_params(void *self, int setmod
 			best_mode = p;
 
 		/* test if params match the requested rate */
-		if ((uaudio_getrates(sc, p) & (1 << rateindex)) == 0)
+		if ((uaudio_getrates(sc, setmode, p) & (1 << rateindex)) == 0)
 			continue;
 		if (best_rate == NULL)
 			best_rate = p;
@@ -4105,7 +4147,7 @@ uaudio_set_params(void *self, int setmod
 	 * Recalculate rate index, because the chosen parameters
 	 * may not support the requested one
 	 */
-	rateindex = uaudio_rates_indexof(uaudio_getrates(sc, p), rate);
+	rateindex = uaudio_rates_indexof(uaudio_getrates(sc, setmode, p), rate);
 	if (rateindex < 0)
 		return ENOTTY;