* [PATCH] staging: bcm2835-audio: Fix draining behavior regression
@ 2019-09-14 15:24 Takashi Iwai
2019-09-15 13:54 ` Stefan Wahren
0 siblings, 1 reply; 3+ messages in thread
From: Takashi Iwai @ 2019-09-14 15:24 UTC (permalink / raw)
To: Eric Anholt, Stefan Wahren, Greg Kroah-Hartman
Cc: linux-rpi-kernel, linux-kernel
The PCM draining behavior got broken since the recent refactoring, and
this turned out to be the incorrect expectation of the firmware
behavior regarding "draining". While I expected the "drain" flag at
the stop operation would do processing the queued samples, it seems
rather dropping the samples.
As a quick fix, just drop the SNDRV_PCM_INFO_DRAIN_TRIGGER flag, so
that the driver uses the normal PCM draining procedure. Also, put
some caution comment to the function for future readers not to fall
into the same pitfall.
Fixes: d7ca3a71545b ("staging: bcm2835-audio: Operate non-atomic PCM ops")
BugLink: https://github.com/raspberrypi/linux/issues/2983
Cc: stable@vger.kernel.org
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c | 4 ++--
drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c | 1 +
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
index bc1eaa3a0773..826016c3431a 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
@@ -12,7 +12,7 @@
static const struct snd_pcm_hardware snd_bcm2835_playback_hw = {
.info = (SNDRV_PCM_INFO_INTERLEAVED | SNDRV_PCM_INFO_BLOCK_TRANSFER |
SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_MMAP_VALID |
- SNDRV_PCM_INFO_DRAIN_TRIGGER | SNDRV_PCM_INFO_SYNC_APPLPTR),
+ SNDRV_PCM_INFO_SYNC_APPLPTR),
.formats = SNDRV_PCM_FMTBIT_U8 | SNDRV_PCM_FMTBIT_S16_LE,
.rates = SNDRV_PCM_RATE_CONTINUOUS | SNDRV_PCM_RATE_8000_48000,
.rate_min = 8000,
@@ -29,7 +29,7 @@ static const struct snd_pcm_hardware snd_bcm2835_playback_hw = {
static const struct snd_pcm_hardware snd_bcm2835_playback_spdif_hw = {
.info = (SNDRV_PCM_INFO_INTERLEAVED | SNDRV_PCM_INFO_BLOCK_TRANSFER |
SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_MMAP_VALID |
- SNDRV_PCM_INFO_DRAIN_TRIGGER | SNDRV_PCM_INFO_SYNC_APPLPTR),
+ SNDRV_PCM_INFO_SYNC_APPLPTR),
.formats = SNDRV_PCM_FMTBIT_S16_LE,
.rates = SNDRV_PCM_RATE_CONTINUOUS | SNDRV_PCM_RATE_44100 |
SNDRV_PCM_RATE_48000,
diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
index 23fba01107b9..c6f9cf1913d2 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
@@ -289,6 +289,7 @@ int bcm2835_audio_stop(struct bcm2835_alsa_stream *alsa_stream)
VC_AUDIO_MSG_TYPE_STOP, false);
}
+/* FIXME: this doesn't seem working as expected for "draining" */
int bcm2835_audio_drain(struct bcm2835_alsa_stream *alsa_stream)
{
struct vc_audio_msg m = {
--
2.16.4
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] staging: bcm2835-audio: Fix draining behavior regression
2019-09-14 15:24 [PATCH] staging: bcm2835-audio: Fix draining behavior regression Takashi Iwai
@ 2019-09-15 13:54 ` Stefan Wahren
2019-09-15 15:51 ` Takashi Iwai
0 siblings, 1 reply; 3+ messages in thread
From: Stefan Wahren @ 2019-09-15 13:54 UTC (permalink / raw)
To: Takashi Iwai, Eric Anholt, Greg Kroah-Hartman
Cc: linux-rpi-kernel, linux-kernel
Hi Takashi,
Am 14.09.19 um 17:24 schrieb Takashi Iwai:
> The PCM draining behavior got broken since the recent refactoring, and
> this turned out to be the incorrect expectation of the firmware
> behavior regarding "draining". While I expected the "drain" flag at
> the stop operation would do processing the queued samples, it seems
> rather dropping the samples.
>
> As a quick fix, just drop the SNDRV_PCM_INFO_DRAIN_TRIGGER flag, so
> that the driver uses the normal PCM draining procedure. Also, put
> some caution comment to the function for future readers not to fall
> into the same pitfall.
>
> Fixes: d7ca3a71545b ("staging: bcm2835-audio: Operate non-atomic PCM ops")
> BugLink: https://github.com/raspberrypi/linux/issues/2983
thanks for taking care of this. Wouldn't it be better to add the link to
the new comment to provide more context of the unexpected behavior?
Nevertheless:
Acked-by: Stefan Wahren <wahrenst@gmx.net>
> Cc: stable@vger.kernel.org
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
> drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c | 4 ++--
> drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c | 1 +
> 2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
> index bc1eaa3a0773..826016c3431a 100644
> --- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
> +++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
> @@ -12,7 +12,7 @@
> static const struct snd_pcm_hardware snd_bcm2835_playback_hw = {
> .info = (SNDRV_PCM_INFO_INTERLEAVED | SNDRV_PCM_INFO_BLOCK_TRANSFER |
> SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_MMAP_VALID |
> - SNDRV_PCM_INFO_DRAIN_TRIGGER | SNDRV_PCM_INFO_SYNC_APPLPTR),
> + SNDRV_PCM_INFO_SYNC_APPLPTR),
> .formats = SNDRV_PCM_FMTBIT_U8 | SNDRV_PCM_FMTBIT_S16_LE,
> .rates = SNDRV_PCM_RATE_CONTINUOUS | SNDRV_PCM_RATE_8000_48000,
> .rate_min = 8000,
> @@ -29,7 +29,7 @@ static const struct snd_pcm_hardware snd_bcm2835_playback_hw = {
> static const struct snd_pcm_hardware snd_bcm2835_playback_spdif_hw = {
> .info = (SNDRV_PCM_INFO_INTERLEAVED | SNDRV_PCM_INFO_BLOCK_TRANSFER |
> SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_MMAP_VALID |
> - SNDRV_PCM_INFO_DRAIN_TRIGGER | SNDRV_PCM_INFO_SYNC_APPLPTR),
> + SNDRV_PCM_INFO_SYNC_APPLPTR),
> .formats = SNDRV_PCM_FMTBIT_S16_LE,
> .rates = SNDRV_PCM_RATE_CONTINUOUS | SNDRV_PCM_RATE_44100 |
> SNDRV_PCM_RATE_48000,
> diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
> index 23fba01107b9..c6f9cf1913d2 100644
> --- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
> +++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
> @@ -289,6 +289,7 @@ int bcm2835_audio_stop(struct bcm2835_alsa_stream *alsa_stream)
> VC_AUDIO_MSG_TYPE_STOP, false);
> }
>
> +/* FIXME: this doesn't seem working as expected for "draining" */
> int bcm2835_audio_drain(struct bcm2835_alsa_stream *alsa_stream)
> {
> struct vc_audio_msg m = {
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] staging: bcm2835-audio: Fix draining behavior regression
2019-09-15 13:54 ` Stefan Wahren
@ 2019-09-15 15:51 ` Takashi Iwai
0 siblings, 0 replies; 3+ messages in thread
From: Takashi Iwai @ 2019-09-15 15:51 UTC (permalink / raw)
To: Stefan Wahren
Cc: Eric Anholt, Greg Kroah-Hartman, linux-rpi-kernel, linux-kernel
On Sun, 15 Sep 2019 15:54:16 +0200,
Stefan Wahren wrote:
>
> Hi Takashi,
>
> Am 14.09.19 um 17:24 schrieb Takashi Iwai:
> > The PCM draining behavior got broken since the recent refactoring, and
> > this turned out to be the incorrect expectation of the firmware
> > behavior regarding "draining". While I expected the "drain" flag at
> > the stop operation would do processing the queued samples, it seems
> > rather dropping the samples.
> >
> > As a quick fix, just drop the SNDRV_PCM_INFO_DRAIN_TRIGGER flag, so
> > that the driver uses the normal PCM draining procedure. Also, put
> > some caution comment to the function for future readers not to fall
> > into the same pitfall.
> >
> > Fixes: d7ca3a71545b ("staging: bcm2835-audio: Operate non-atomic PCM ops")
> > BugLink: https://github.com/raspberrypi/linux/issues/2983
>
> thanks for taking care of this. Wouldn't it be better to add the link to
> the new comment to provide more context of the unexpected behavior?
Yeah, a bit more explanation would be better. Unfortunately I've been
traveling in the last week (and will be traveling again in the next
weeks), so had little time for caring and testing with any actual
hardware...
>
> Nevertheless:
>
> Acked-by: Stefan Wahren <wahrenst@gmx.net>
Thanks!
Takashi
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> > drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c | 4 ++--
> > drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c | 1 +
> > 2 files changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
> > index bc1eaa3a0773..826016c3431a 100644
> > --- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
> > +++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
> > @@ -12,7 +12,7 @@
> > static const struct snd_pcm_hardware snd_bcm2835_playback_hw = {
> > .info = (SNDRV_PCM_INFO_INTERLEAVED | SNDRV_PCM_INFO_BLOCK_TRANSFER |
> > SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_MMAP_VALID |
> > - SNDRV_PCM_INFO_DRAIN_TRIGGER | SNDRV_PCM_INFO_SYNC_APPLPTR),
> > + SNDRV_PCM_INFO_SYNC_APPLPTR),
> > .formats = SNDRV_PCM_FMTBIT_U8 | SNDRV_PCM_FMTBIT_S16_LE,
> > .rates = SNDRV_PCM_RATE_CONTINUOUS | SNDRV_PCM_RATE_8000_48000,
> > .rate_min = 8000,
> > @@ -29,7 +29,7 @@ static const struct snd_pcm_hardware snd_bcm2835_playback_hw = {
> > static const struct snd_pcm_hardware snd_bcm2835_playback_spdif_hw = {
> > .info = (SNDRV_PCM_INFO_INTERLEAVED | SNDRV_PCM_INFO_BLOCK_TRANSFER |
> > SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_MMAP_VALID |
> > - SNDRV_PCM_INFO_DRAIN_TRIGGER | SNDRV_PCM_INFO_SYNC_APPLPTR),
> > + SNDRV_PCM_INFO_SYNC_APPLPTR),
> > .formats = SNDRV_PCM_FMTBIT_S16_LE,
> > .rates = SNDRV_PCM_RATE_CONTINUOUS | SNDRV_PCM_RATE_44100 |
> > SNDRV_PCM_RATE_48000,
> > diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
> > index 23fba01107b9..c6f9cf1913d2 100644
> > --- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
> > +++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
> > @@ -289,6 +289,7 @@ int bcm2835_audio_stop(struct bcm2835_alsa_stream *alsa_stream)
> > VC_AUDIO_MSG_TYPE_STOP, false);
> > }
> >
> > +/* FIXME: this doesn't seem working as expected for "draining" */
> > int bcm2835_audio_drain(struct bcm2835_alsa_stream *alsa_stream)
> > {
> > struct vc_audio_msg m = {
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-09-15 15:51 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-14 15:24 [PATCH] staging: bcm2835-audio: Fix draining behavior regression Takashi Iwai
2019-09-15 13:54 ` Stefan Wahren
2019-09-15 15:51 ` Takashi Iwai
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).