Re: missing sound on kernel-5.15

[Takashi Iwai <tiwai@suse.de>]

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@suse.de>
> > 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@suse.de>
> > ---
> >  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)
>