Re: [PATCH] usb-audio: Fix boot-time crackling for Generic USB Audio device

[Takashi Iwai <tiwai@suse.de>]

On Tue, 14 Jul 2026 03:58:08 +0200,
Zhang Heng wrote:
> 
> > On Mon, Jul 13, 2026 at 04:10:48PM +0800, Zhang Heng wrote:
> >> +		/* Generic USB Audio (0x1e0b:d01e): use SIA for consistent scheduling */
> >> +		if (ep->chip->usb_id == USB_ID(0x1e0b, 0xd01e))
> >> +			u->urb->transfer_flags |= URB_ISO_ASAP;
> > Not a maintainer, just a bystander who was Cc'd -- two small notes on the
> > form of the patch, plus one question I can't answer myself.
> > 
> > A per-device usb_id comparison in the endpoint.c fast path seems like the
> > kind of thing the quirk_flags_table in quirks.c exists to avoid. Would a
> > QUIRK_FLAG_ISO_ASAP (set in the table, tested here as
> > chip->quirk_flags & QUIRK_FLAG_ISO_ASAP) work for you? It keeps endpoint.c
> > device-agnostic, and the next device with the same symptom becomes a
> > one-line table entry rather than another if.
> First of all, let me clarify: this issue occurs when there is startup music,
> but it plays normally after entering the system. There is a heavy creaking
> sound when the system is turned on here. Based on the information from dmesg
> and syslog, there are a large number of xhci hcd frame synchronization
> failures. I tried adding URB_ISO_ASAP here, and it will be much better,
> with only a little noise.

OK, but it still makes sense to deal URB_ISO_ASAP workaround more
generically.

> >> +	if (ep->chip->usb_id == USB_ID(0x1e0b, 0xd01e) &&
> >> +	    ep->type == SND_USB_ENDPOINT_TYPE_SYNC)
> >> +		ep->skip_packets = 4;
> > The block immediately above this one already does exactly
> > "type == SND_USB_ENDPOINT_TYPE_SYNC -> skip_packets = 4"; adding the ID to
> > that condition would avoid the duplicate if. Also, the changelog doesn't
> > say what this hunk contributes on its own -- is URB_ISO_ASAP alone
> > insufficient, and if so, what does skipping the first 4 sync packets fix
> > that ASAP doesn't? Right now the two changes are indistinguishable in the
> > commit message, and skip_packets = 4 reads as belt-and-braces.
> As mentioned above, there is still a bit of noise when only adding
> URB_ISO_ASAP,
> 
> but skip-packets=4 can solve this problem.

Well, one missing thing is to understand why this fixes.
Originally, the skip_packets=4 for Playback Design devices was
introduced for bogus feedback packets at the start of the stream long
time ago.  But that's the only known device that needs it.  Does your
device send also 4 bogus packets?

The skip_packets=16 for M-Audio devices are rather for avoiding the
latency.  And, speaking of latency, I have a patch for the lowlatency
support of implicit fb mode, but never had a test environment.
Could you check the patch below and see the patch below has any
positive/negative influence?  Just to be sure.


thanks,

Takashi

-- 8< --
index 682b6c1fe76b..6d144e39a849 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -265,6 +265,7 @@ int snd_usb_init_pitch(struct snd_usb_audio *chip,
 	return 0;
 }
 
+/* stop both data and sync endpoints */
 static bool stop_endpoints(struct snd_usb_substream *subs, bool keep_pending)
 {
 	bool stopped = 0;
@@ -280,6 +281,24 @@ static bool stop_endpoints(struct snd_usb_substream *subs, bool keep_pending)
 	return stopped;
 }
 
+/* only start sync endpoint */
+static int start_sync_endpoint(struct snd_usb_substream *subs)
+{
+	int err;
+
+	if (subs->sync_endpoint &&
+	    !test_and_set_bit(SUBSTREAM_FLAG_SYNC_EP_STARTED, &subs->flags)) {
+		err = snd_usb_endpoint_start(subs->sync_endpoint);
+		if (err < 0) {
+			clear_bit(SUBSTREAM_FLAG_SYNC_EP_STARTED, &subs->flags);
+			return err;
+		}
+	}
+
+	return 0;
+}
+
+/* start both data and sync endpoints */
 static int start_endpoints(struct snd_usb_substream *subs)
 {
 	int err;
@@ -295,14 +314,9 @@ static int start_endpoints(struct snd_usb_substream *subs)
 		}
 	}
 
-	if (subs->sync_endpoint &&
-	    !test_and_set_bit(SUBSTREAM_FLAG_SYNC_EP_STARTED, &subs->flags)) {
-		err = snd_usb_endpoint_start(subs->sync_endpoint);
-		if (err < 0) {
-			clear_bit(SUBSTREAM_FLAG_SYNC_EP_STARTED, &subs->flags);
-			goto error;
-		}
-	}
+	err = start_sync_endpoint(subs);
+	if (err < 0)
+		goto error;
 
 	return 0;
 
@@ -656,12 +670,16 @@ static int lowlatency_playback_available(struct snd_pcm_runtime *runtime,
 		return false;
 	if (in_free_wheeling_mode(runtime))
 		return false;
-	/* implicit feedback mode has own operation mode */
-	if (snd_usb_endpoint_implicit_feedback_sink(subs->data_endpoint))
-		return false;
 	return true;
 }
 
+/* return true if it's a normal playback (not in implicit fb) */
+static bool is_normal_lowlatency_playback(struct snd_usb_substream *subs)
+{
+	return subs->lowlatency_playback &&
+		!snd_usb_endpoint_implicit_feedback_sink(subs->data_endpoint);
+}
+
 /*
  * prepare callback
  *
@@ -709,9 +727,11 @@ static int snd_usb_pcm_prepare(struct snd_pcm_substream *substream)
 	runtime->delay = 0;
 
 	subs->lowlatency_playback = lowlatency_playback_available(runtime, subs);
-	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK &&
-	    !subs->lowlatency_playback) {
-		ret = start_endpoints(subs);
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+		if (!subs->lowlatency_playback)
+			ret = start_endpoints(subs);
+		else if (snd_usb_endpoint_implicit_feedback_sink(subs->data_endpoint))
+			ret = start_sync_endpoint(subs);
 		/* if XRUN happens at starting streams (possibly with implicit
 		 * fb case), restart again, but only try once.
 		 */
@@ -1539,7 +1559,7 @@ static int prepare_playback_urb(struct snd_usb_substream *subs,
 		frame_limit = subs->frame_limit + ep->max_urb_frames;
 		transfer_done = subs->transfer_done;
 
-		if (subs->lowlatency_playback &&
+		if (is_normal_lowlatency_playback(subs) &&
 		    runtime->state != SNDRV_PCM_STATE_DRAINING) {
 			unsigned int hwptr = subs->hwptr_done / stride;
 
@@ -1625,7 +1645,8 @@ static int prepare_playback_urb(struct snd_usb_substream *subs,
 			subs->trigger_tstamp_pending_update = false;
 		}
 
-		if (period_elapsed && !subs->running && subs->lowlatency_playback) {
+		if (period_elapsed && !subs->running &&
+		    is_normal_lowlatency_playback(subs)) {
 			subs->period_elapsed_pending = 1;
 			period_elapsed = 0;
 		}
@@ -1677,7 +1698,7 @@ static int snd_usb_pcm_playback_ack(struct snd_pcm_substream *substream)
 	struct snd_usb_substream *subs = substream->runtime->private_data;
 	struct snd_usb_endpoint *ep;
 
-	if (!subs->lowlatency_playback || !subs->running)
+	if (!is_normal_lowlatency_playback(subs) || !subs->running)
 		return 0;
 	ep = subs->data_endpoint;
 	if (!ep)
@@ -1705,6 +1726,7 @@ static int snd_usb_substream_playback_trigger(struct snd_pcm_substream *substrea
 					      prepare_playback_urb,
 					      retire_playback_urb,
 					      subs);
+		/* start EPs for both normal and implicit-fb modes */
 		if (subs->lowlatency_playback &&
 		    cmd == SNDRV_PCM_TRIGGER_START) {
 			if (in_free_wheeling_mode(substream->runtime))