Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Sun, 15 Sep 2019 17:51:26 +0200
Message-ID: <s5hef0hzhfl.wl-tiwai@suse.de>
From:   Takashi Iwai <tiwai@suse.de>
To:     Stefan Wahren <wahrenst@gmx.net>
Cc:     Eric Anholt <eric@anholt.net>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-rpi-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] staging: bcm2835-audio: Fix draining behavior regression
In-Reply-To: <3b41702b-1cc2-233d-94a6-ece8f5a19957@gmx.net>
References: <20190914152405.7416-1-tiwai@suse.de>
        <3b41702b-1cc2-233d-94a6-ece8f5a19957@gmx.net>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka)
 FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/25.3
 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka")
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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 = {
>