| Message ID | 20211220211130.88590-8-pavel.hofman@ivitera.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: gadget: audio: Multiple rates, dyn. bInterval | expand |
On Mon, Dec 20, 2021 at 10:11:26PM +0100, Pavel Hofman wrote: > When the USB host stops capture/playback, the corresponding PCM > substream (if activated and running) is stopped and drained. > > Signed-off-by: Pavel Hofman <pavel.hofman@ivitera.com> > --- > drivers/usb/gadget/function/u_audio.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/drivers/usb/gadget/function/u_audio.c b/drivers/usb/gadget/function/u_audio.c > index a6293415c071..9dbce51c2eb7 100644 > --- a/drivers/usb/gadget/function/u_audio.c > +++ b/drivers/usb/gadget/function/u_audio.c > @@ -544,6 +544,20 @@ static void set_reported_srate(struct uac_rtd_params *prm, int srate) > } > } > > +static void stop_substream(struct uac_rtd_params *prm) > +{ > + unsigned long _flags; > + struct snd_pcm_substream *substream; > + > + substream = prm->ss; > + if (substream) { > + snd_pcm_stream_lock_irqsave(substream, _flags); > + if (snd_pcm_running(substream)) > + snd_pcm_stop(substream, SNDRV_PCM_STATE_DRAINING); I'm not sure if this is right (and the series should probably be CC'd to alsa-devel to check the audio side of this). DRAINING seems to be right for capture, but for playback should this end up in state SETUP? Does this need to handle resuming a paused stream like snd_pcm_drain() does? > + snd_pcm_stream_unlock_irqrestore(substream, _flags); > + } > +} > + > int u_audio_start_capture(struct g_audio *audio_dev) > { > struct snd_uac_chip *uac = audio_dev->uac; > @@ -630,6 +644,7 @@ void u_audio_stop_capture(struct g_audio *audio_dev) > { > struct snd_uac_chip *uac = audio_dev->uac; > > + stop_substream(&uac->c_prm); > set_reported_srate(&uac->c_prm, 0); > if (audio_dev->in_ep_fback) > free_ep_fback(&uac->c_prm, audio_dev->in_ep_fback); > @@ -713,6 +728,7 @@ void u_audio_stop_playback(struct g_audio *audio_dev) > { > struct snd_uac_chip *uac = audio_dev->uac; > > + stop_substream(&uac->p_prm); > set_reported_srate(&uac->p_prm, 0); > free_ep(&uac->p_prm, audio_dev->in_ep); > } > -- > 2.25.1 >
Dne 21. 12. 21 v 13:18 John Keeping napsal(a): > On Mon, Dec 20, 2021 at 10:11:26PM +0100, Pavel Hofman wrote: >> When the USB host stops capture/playback, the corresponding PCM >> substream (if activated and running) is stopped and drained. >> >> Signed-off-by: Pavel Hofman <pavel.hofman@ivitera.com> >> --- >> drivers/usb/gadget/function/u_audio.c | 16 ++++++++++++++++ >> 1 file changed, 16 insertions(+) >> >> diff --git a/drivers/usb/gadget/function/u_audio.c b/drivers/usb/gadget/function/u_audio.c >> index a6293415c071..9dbce51c2eb7 100644 >> --- a/drivers/usb/gadget/function/u_audio.c >> +++ b/drivers/usb/gadget/function/u_audio.c >> @@ -544,6 +544,20 @@ static void set_reported_srate(struct uac_rtd_params *prm, int srate) >> } >> } >> >> +static void stop_substream(struct uac_rtd_params *prm) >> +{ >> + unsigned long _flags; >> + struct snd_pcm_substream *substream; >> + >> + substream = prm->ss; >> + if (substream) { >> + snd_pcm_stream_lock_irqsave(substream, _flags); >> + if (snd_pcm_running(substream)) >> + snd_pcm_stop(substream, SNDRV_PCM_STATE_DRAINING); > > I'm not sure if this is right (and the series should probably be CC'd to > alsa-devel to check the audio side of this). > > DRAINING seems to be right for capture, but for playback should this end > up in state SETUP? Does this need to handle resuming a paused stream > like snd_pcm_drain() does? Honestly, I do not know. This code comes from a SPDIF receiver driver where it handles interrupted incoming SPDIF stream. You are right it is related to capture. I will ask the alsa devs about the playback solution specifically. Yes I will CC the next version to alsa-dev just in case. > >> + snd_pcm_stream_unlock_irqrestore(substream, _flags); >> + } >> +} >> + >> int u_audio_start_capture(struct g_audio *audio_dev) >> { >> struct snd_uac_chip *uac = audio_dev->uac; >> @@ -630,6 +644,7 @@ void u_audio_stop_capture(struct g_audio *audio_dev) >> { >> struct snd_uac_chip *uac = audio_dev->uac; >> >> + stop_substream(&uac->c_prm); >> set_reported_srate(&uac->c_prm, 0); >> if (audio_dev->in_ep_fback) >> free_ep_fback(&uac->c_prm, audio_dev->in_ep_fback); >> @@ -713,6 +728,7 @@ void u_audio_stop_playback(struct g_audio *audio_dev) >> { >> struct snd_uac_chip *uac = audio_dev->uac; >> >> + stop_substream(&uac->p_prm); >> set_reported_srate(&uac->p_prm, 0); >> free_ep(&uac->p_prm, audio_dev->in_ep); >> }
Dne 22. 12. 21 v 13:26 Pavel Hofman napsal(a): > Dne 21. 12. 21 v 13:18 John Keeping napsal(a): >> On Mon, Dec 20, 2021 at 10:11:26PM +0100, Pavel Hofman wrote: >>> When the USB host stops capture/playback, the corresponding PCM >>> substream (if activated and running) is stopped and drained. >>> >>> Signed-off-by: Pavel Hofman <pavel.hofman@ivitera.com> >>> --- >>> drivers/usb/gadget/function/u_audio.c | 16 ++++++++++++++++ >>> 1 file changed, 16 insertions(+) >>> >>> diff --git a/drivers/usb/gadget/function/u_audio.c >>> b/drivers/usb/gadget/function/u_audio.c >>> index a6293415c071..9dbce51c2eb7 100644 >>> --- a/drivers/usb/gadget/function/u_audio.c >>> +++ b/drivers/usb/gadget/function/u_audio.c >>> @@ -544,6 +544,20 @@ static void set_reported_srate(struct >>> uac_rtd_params *prm, int srate) >>> } >>> } >>> +static void stop_substream(struct uac_rtd_params *prm) >>> +{ >>> + unsigned long _flags; >>> + struct snd_pcm_substream *substream; >>> + >>> + substream = prm->ss; >>> + if (substream) { >>> + snd_pcm_stream_lock_irqsave(substream, _flags); >>> + if (snd_pcm_running(substream)) >>> + snd_pcm_stop(substream, SNDRV_PCM_STATE_DRAINING); >> >> I'm not sure if this is right (and the series should probably be CC'd to >> alsa-devel to check the audio side of this). >> >> DRAINING seems to be right for capture, but for playback should this end >> up in state SETUP? Does this need to handle resuming a paused stream >> like snd_pcm_drain() does? > > Honestly, I do not know. This code comes from a SPDIF receiver driver > where it handles interrupted incoming SPDIF stream. You are right it is > related to capture. I will ask the alsa devs about the playback solution > specifically. > > Yes I will CC the next version to alsa-dev just in case. I have not received a response from the alsa devels yet, however based on existing variants in the other drivers in the kernel there are two options: if (substream) { snd_pcm_stream_lock_irqsave(substream, _flags); if (snd_pcm_running(substream)) { if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) snd_pcm_stop(substream, SNDRV_PCM_STATE_SETUP); else snd_pcm_stop(substream, SNDRV_PCM_STATE_DRAINING); } snd_pcm_stream_unlock_irqrestore(substream, _flags); } When testing playback/capture, sox does not exit with this version (only reporting an error), recovering if playback/capture resume within standard alsa blocking read/write timeout. Aplay/arecord exit. Another option is if (substream) { snd_pcm_stream_lock_irqsave(substream, _flags); if (snd_pcm_running(substream)) snd_pcm_stop(substream, SNDRV_PCM_STATE_DISCONNECTED); snd_pcm_stream_unlock_irqrestore(substream, _flags); } Here both sox and aplay/arecord exit when the pcm stream is stopped. From my use case I like this option better. Or the gadget can use SETUP/DRAINING for host-side playback/capture stop and DISCONNECTED for USB cable unplugging. But I am not sure whether from the gadget side POW there is a any real difference between the host stopping playback/capture and cable disconnection. Also this patch would change behaviour of the gadget for existing installations, as the existing version just stops data delivery/consumption. Maybe some installations already count on this behaviour. Perhaps the snd_pcm_stop call should be an opt-in via a new configfs parameter? Please can existing gadget users comment on this? Thanks a lot, Pavel.
diff --git a/drivers/usb/gadget/function/u_audio.c b/drivers/usb/gadget/function/u_audio.c index a6293415c071..9dbce51c2eb7 100644 --- a/drivers/usb/gadget/function/u_audio.c +++ b/drivers/usb/gadget/function/u_audio.c @@ -544,6 +544,20 @@ static void set_reported_srate(struct uac_rtd_params *prm, int srate) } } +static void stop_substream(struct uac_rtd_params *prm) +{ + unsigned long _flags; + struct snd_pcm_substream *substream; + + substream = prm->ss; + if (substream) { + snd_pcm_stream_lock_irqsave(substream, _flags); + if (snd_pcm_running(substream)) + snd_pcm_stop(substream, SNDRV_PCM_STATE_DRAINING); + snd_pcm_stream_unlock_irqrestore(substream, _flags); + } +} + int u_audio_start_capture(struct g_audio *audio_dev) { struct snd_uac_chip *uac = audio_dev->uac; @@ -630,6 +644,7 @@ void u_audio_stop_capture(struct g_audio *audio_dev) { struct snd_uac_chip *uac = audio_dev->uac; + stop_substream(&uac->c_prm); set_reported_srate(&uac->c_prm, 0); if (audio_dev->in_ep_fback) free_ep_fback(&uac->c_prm, audio_dev->in_ep_fback); @@ -713,6 +728,7 @@ void u_audio_stop_playback(struct g_audio *audio_dev) { struct snd_uac_chip *uac = audio_dev->uac; + stop_substream(&uac->p_prm); set_reported_srate(&uac->p_prm, 0); free_ep(&uac->p_prm, audio_dev->in_ep); }
When the USB host stops capture/playback, the corresponding PCM substream (if activated and running) is stopped and drained. Signed-off-by: Pavel Hofman <pavel.hofman@ivitera.com> --- drivers/usb/gadget/function/u_audio.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)