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@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)
