On Thu, 01 Sep 2022 07:50:40 +0200, chihhao chen wrote: > > Hi Takashi, > > The patch fixes this problem. > I tried it with "Product: Samsung USB C Earphone" and missing sound > problem cannot be reproduced. OK, it's a good news. I'm going to add more information to the patch description about the regression and submit the patch. thanks, Takashi > > Thanks > > On Wed, 2022-08-31 at 15:40 +0200, Takashi Iwai wrote: > > On Wed, 31 Aug 2022 15:16:39 +0200, > > chihhao chen wrote: > > > > > > Hi Takashi, > > > > > > Yes no error reported and data on USB bus is also complete. (Use > > > USB > > > analyzer to collect packets on bus and check these data.) > > > > Hm, then it has something to do with the device firmware side... > > > > > I added delay right after find_substream_format() in > > > snd_usb_hw_params() as follows > > > 1. first time call snd_usb_hw_params(), do nothing > > > 2. second time call snd_usb_hw_params(), delay 150ms after > > > find_substream_format() > > > > > > I tried to set snd_usb_use_vmalloc false but this problem still > > > happened. > > > > OK, thanks. > > > > On the second thought, it's good to split the existing endpoint setup > > to two parts, and apply the setups involving with the buffer > > allocation at hw_params while the USB interface setup is done at > > prepare. It'll reduce the unnecessary buffer re-allocation, too, so > > I > > had such a change in my mind and already cooked some time ago. > > > > Could you try the patch below? If this actually helps for your use > > case, we should put more information about the good side-effect, too. > > > > > > Takashi > > > > -- 8< -- > > From: Takashi Iwai <tiwai@xxxxxxx> > > Subject: [PATCH] ALSA: usb-audio: Split endpoint setups for hw_params > > and > > prepare > > > > One of the former changes for the endpoint management was the more > > consistent setup of endpoints at hw_params. > > snd_usb_endpoint_configure() is a single function that does the full > > setup, and it's called from both PCM hw_params and prepare callbacks. > > Although the EP setup at the prepare phase is usually skipped (by > > checking need_setup flag), it may be still effective in some cases > > like suspend/resume that requires the interface setup again. > > > > As it's a full and single setup, the invocation of > > snd_usb_endpoint_configure() includes not only the USB interface > > setup > > but also the buffer release and allocation. OTOH, doing the buffer > > release and re-allocation at PCM prepare phase is rather superfluous, > > and better to be only in the hw_params phase. > > > > For those optimizations, this patch splits the endpoint setup to two > > phases: snd_usb_endpoint_set_params() and snd_usb_endpoint_prepare(), > > to be called from hw_params and from prepare, respectively. > > > > This changes the operation slightly, effectively moving the USB > > interface setup again to PCM prepare stage instead of hw_params > > stage, while the buffer allocation and such initializations are still > > done at hw_params stage. > > > > Signed-off-by: Takashi Iwai <tiwai@xxxxxxx> > > --- > > sound/usb/endpoint.c | 23 +++++++++-------------- > > sound/usb/endpoint.h | 6 ++++-- > > sound/usb/pcm.c | 14 ++++++++++---- > > 3 files changed, 23 insertions(+), 20 deletions(-) > > > > diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c > > index 0d7b73bf7945..a42f2ce19455 100644 > > --- a/sound/usb/endpoint.c > > +++ b/sound/usb/endpoint.c > > @@ -758,7 +758,8 @@ bool snd_usb_endpoint_compatible(struct > > snd_usb_audio *chip, > > * The endpoint needs to be closed via snd_usb_endpoint_close() > > later. > > * > > * Note that this function doesn't configure the endpoint. The > > substream > > - * needs to set it up later via snd_usb_endpoint_configure(). > > + * needs to set it up later via snd_usb_endpoint_set_params() and > > + * snd_usb_endpoint_prepare(). > > */ > > struct snd_usb_endpoint * > > snd_usb_endpoint_open(struct snd_usb_audio *chip, > > @@ -1290,12 +1291,13 @@ static int sync_ep_set_params(struct > > snd_usb_endpoint *ep) > > /* > > * snd_usb_endpoint_set_params: configure an snd_usb_endpoint > > * > > + * It's called either from hw_params callback. > > * Determine the number of URBs to be used on this endpoint. > > * An endpoint must be configured before it can be started. > > * An endpoint that is already running can not be reconfigured. > > */ > > -static int snd_usb_endpoint_set_params(struct snd_usb_audio *chip, > > - struct snd_usb_endpoint *ep) > > +int snd_usb_endpoint_set_params(struct snd_usb_audio *chip, > > + struct snd_usb_endpoint *ep) > > { > > const struct audioformat *fmt = ep->cur_audiofmt; > > int err; > > @@ -1378,18 +1380,18 @@ static int init_sample_rate(struct > > snd_usb_audio *chip, > > } > > > > /* > > - * snd_usb_endpoint_configure: Configure the endpoint > > + * snd_usb_endpoint_prepare: Prepare the endpoint > > * > > * This function sets up the EP to be fully usable state. > > - * It's called either from hw_params or prepare callback. > > + * It's called either from prepare callback. > > * The function checks need_setup flag, and performs nothing unless > > needed, > > * so it's safe to call this multiple times. > > * > > * This returns zero if unchanged, 1 if the configuration has > > changed, > > * or a negative error code. > > */ > > -int snd_usb_endpoint_configure(struct snd_usb_audio *chip, > > - struct snd_usb_endpoint *ep) > > +int snd_usb_endpoint_prepare(struct snd_usb_audio *chip, > > + struct snd_usb_endpoint *ep) > > { > > bool iface_first; > > int err = 0; > > @@ -1410,9 +1412,6 @@ int snd_usb_endpoint_configure(struct > > snd_usb_audio *chip, > > if (err < 0) > > goto unlock; > > } > > - err = snd_usb_endpoint_set_params(chip, ep); > > - if (err < 0) > > - goto unlock; > > goto done; > > } > > > > @@ -1440,10 +1439,6 @@ int snd_usb_endpoint_configure(struct > > snd_usb_audio *chip, > > if (err < 0) > > goto unlock; > > > > - err = snd_usb_endpoint_set_params(chip, ep); > > - if (err < 0) > > - goto unlock; > > - > > err = snd_usb_select_mode_quirk(chip, ep->cur_audiofmt); > > if (err < 0) > > goto unlock; > > diff --git a/sound/usb/endpoint.h b/sound/usb/endpoint.h > > index 6a9af04cf175..e67ea28faa54 100644 > > --- a/sound/usb/endpoint.h > > +++ b/sound/usb/endpoint.h > > @@ -17,8 +17,10 @@ snd_usb_endpoint_open(struct snd_usb_audio *chip, > > bool is_sync_ep); > > void snd_usb_endpoint_close(struct snd_usb_audio *chip, > > struct snd_usb_endpoint *ep); > > -int snd_usb_endpoint_configure(struct snd_usb_audio *chip, > > - struct snd_usb_endpoint *ep); > > +int snd_usb_endpoint_set_params(struct snd_usb_audio *chip, > > + struct snd_usb_endpoint *ep); > > +int snd_usb_endpoint_prepare(struct snd_usb_audio *chip, > > + struct snd_usb_endpoint *ep); > > int snd_usb_endpoint_get_clock_rate(struct snd_usb_audio *chip, int > > clock); > > > > bool snd_usb_endpoint_compatible(struct snd_usb_audio *chip, > > diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c > > index d45d1d7e6664..b604f7e95e82 100644 > > --- a/sound/usb/pcm.c > > +++ b/sound/usb/pcm.c > > @@ -443,17 +443,17 @@ static int configure_endpoints(struct > > snd_usb_audio *chip, > > if (stop_endpoints(subs, false)) > > sync_pending_stops(subs); > > if (subs->sync_endpoint) { > > - err = snd_usb_endpoint_configure(chip, subs- > > >sync_endpoint); > > + err = snd_usb_endpoint_prepare(chip, subs- > > >sync_endpoint); > > if (err < 0) > > return err; > > } > > - err = snd_usb_endpoint_configure(chip, subs- > > >data_endpoint); > > + err = snd_usb_endpoint_prepare(chip, subs- > > >data_endpoint); > > if (err < 0) > > return err; > > snd_usb_set_format_quirk(subs, subs->cur_audiofmt); > > } else { > > if (subs->sync_endpoint) { > > - err = snd_usb_endpoint_configure(chip, subs- > > >sync_endpoint); > > + err = snd_usb_endpoint_prepare(chip, subs- > > >sync_endpoint); > > if (err < 0) > > return err; > > } > > @@ -551,7 +551,13 @@ static int snd_usb_hw_params(struct > > snd_pcm_substream *substream, > > subs->cur_audiofmt = fmt; > > mutex_unlock(&chip->mutex); > > > > - ret = configure_endpoints(chip, subs); > > + if (subs->sync_endpoint) { > > + ret = snd_usb_endpoint_set_params(chip, subs- > > >sync_endpoint); > > + if (ret < 0) > > + goto unlock; > > + } > > + > > + ret = snd_usb_endpoint_set_params(chip, subs->data_endpoint); > > > > unlock: > > if (ret < 0) >