Re: missing sound on kernel-5.15

[chihhao chen <chihhao.chen@mediatek.com>]

Hi Takashi,

The patch fixes this problem.
I tried it with "Product: Samsung USB C Earphone" and missing sound
problem cannot be reproduced.

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)