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
- first time call snd_usb_hw_params(), do nothing
- 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;@@ -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)