ASoC: Intel: Bsw: Read sst DSP DMA pointer after period elapse occurs

Commit Message

[e-mail] Aug. 17, 2015, 6:12 a.m. UTC

From: "Fang, Yang A" <[e-mail]>

We should not read sst hw_ptr and pcm_delay in sst_platform_pcm_pointer
Since it will be ricky if sst_platform_pcm_pointer is called while dsp
is updating the timestamp.Now read sst hw_ptr after period elapse
interrupt occurs.

Signed-off-by: Fang, Yang A <[e-mail]>
Acked-by: Vinod Koul <[e-mail]>
---
 sound/soc/intel/atom/sst-mfld-platform-pcm.c |   18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

Comments

Mark Brown Aug. 17, 2015, 7:55 p.m. UTC | #1

On Sun, Aug 16, 2015 at 11:12:25PM -0700, [e-mail] wrote:

> We should not read sst hw_ptr and pcm_delay in sst_platform_pcm_pointer
> Since it will be ricky if sst_platform_pcm_pointer is called while dsp
> is updating the timestamp.Now read sst hw_ptr after period elapse
> interrupt occurs.

So I guess there's still some risk that we could run into problems here
if we take too long to service the interrupt and/or the period is very
short?  Is there anything we can do to improve that?

[e-mail] Aug. 18, 2015, 5:33 p.m. UTC | #2

> -----Original Message-----
> From: Mark Brown [mailto:[e-mail]]
> Sent: Monday, August 17, 2015 12:56 PM
> To: Fang, Yang A
> Cc: [e-mail]; [e-mail];
> [e-mail]; Nujella, Sathyanarayana;
> [e-mail]; Sripathi, Srinivas; Iriawan, Denny; Jain,
> Praveen K; Koul, Vinod; Mirche, Dinesh
> Subject: Re: [PATCH] ASoC: Intel: Bsw: Read sst DSP DMA pointer after
> period elapse occurs
> 
> On Sun, Aug 16, 2015 at 11:12:25PM -0700, [e-mail] wrote:
> 
> > We should not read sst hw_ptr and pcm_delay in
> > sst_platform_pcm_pointer Since it will be ricky if
> > sst_platform_pcm_pointer is called while dsp is updating the
> > timestamp.Now read sst hw_ptr after period elapse interrupt occurs.
> 
> So I guess there's still some risk that we could run into problems here if we
> take too long to service the interrupt and/or the period is very short?  Is
> there anything we can do to improve that?
Hi Mark,
 I think before and after the patch. There is no difference in term of 
 handling the period elapse interrupt . because before the patch 
 sst_period_elapsed calls the snd_pcm_period_elapsed which in turn 
 calling the sst_platform_pcm_pointer which calls the stream_read_tstamp.

Mark Brown Aug. 19, 2015, 12:35 a.m. UTC | #3

On Tue, Aug 18, 2015 at 05:33:33PM +0000, Fang, Yang A wrote:

> > > We should not read sst hw_ptr and pcm_delay in
> > > sst_platform_pcm_pointer Since it will be ricky if
> > > sst_platform_pcm_pointer is called while dsp is updating the
> > > timestamp.Now read sst hw_ptr after period elapse interrupt occurs.

> > So I guess there's still some risk that we could run into problems here if we
> > take too long to service the interrupt and/or the period is very short?  Is
> > there anything we can do to improve that?

>  I think before and after the patch. There is no difference in term of 
>  handling the period elapse interrupt . because before the patch 
>  sst_period_elapsed calls the snd_pcm_period_elapsed which in turn 
>  calling the sst_platform_pcm_pointer which calls the stream_read_tstamp.

I'm not sure I understand what the patch fixes then?  My concern is that
we're just moving the timing around which changes but doesn't address
the race condition.

[e-mail] Aug. 20, 2015, 1:25 a.m. UTC | #4

> -----Original Message-----
> From: Mark Brown [mailto:[e-mail]]
> Sent: Tuesday, August 18, 2015 5:35 PM
> To: Fang, Yang A
> Cc: [e-mail]; [e-mail];
> [e-mail]; Nujella, Sathyanarayana;
> [e-mail]; Sripathi, Srinivas; Iriawan, Denny; Jain,
> Praveen K; Koul, Vinod; Mirche, Dinesh
> Subject: Re: [PATCH] ASoC: Intel: Bsw: Read sst DSP DMA pointer after
> period elapse occurs
> 
> On Tue, Aug 18, 2015 at 05:33:33PM +0000, Fang, Yang A wrote:
> 
> > > > We should not read sst hw_ptr and pcm_delay in
> > > > sst_platform_pcm_pointer Since it will be ricky if
> > > > sst_platform_pcm_pointer is called while dsp is updating the
> > > > timestamp.Now read sst hw_ptr after period elapse interrupt occurs.
> 
> > > So I guess there's still some risk that we could run into problems
> > > here if we take too long to service the interrupt and/or the period
> > > is very short?  Is there anything we can do to improve that?
> 
> >  I think before and after the patch. There is no difference in term of
> > handling the period elapse interrupt . because before the patch
> > sst_period_elapsed calls the snd_pcm_period_elapsed which in turn
> > calling the sst_platform_pcm_pointer which calls the stream_read_tstamp.
> 
> I'm not sure I understand what the patch fixes then?  My concern is that
> we're just moving the timing around which changes but doesn't address the
> race condition.

We are seeing the issue during the long time playback.  
The ring_buffer_counter (part of snd_sst_tstamp  struct )supposes only increasing by the firmware  once firmware fetches  one period size of data.
 It will update the ring_buffer_counter in the mailbox and trigger a period elapse interrupt.  We have found the ring_buffer_counter got decreased 
during long time playback if we  call stream_read_tstamp to read mailbox  in sst_platform_pcm_pointer function.

There are two cases sst_platform_pcm_pointer will be getting called. One is called by sst_period_elapsed function.
 The second case is triggered by the userspace ioctl . The second case supposes reading the same case  ring_buffer_counter  value as last  read 
when period elapse interrupt  occurs . However we saw it somehow decreases.This causes audio stutter.

[e-mail] Aug. 20, 2015, 1:30 a.m. UTC | #5

> -----Original Message-----
> From: Fang, Yang A
> Sent: Wednesday, August 19, 2015 6:25 PM
> To: 'Mark Brown'
> Cc: [e-mail]; [e-mail];
> [e-mail]; Nujella, Sathyanarayana;
> [e-mail]; Sripathi, Srinivas; Iriawan, Denny; Jain,
> Praveen K; Koul, Vinod; Mirche, Dinesh
> Subject: RE: [PATCH] ASoC: Intel: Bsw: Read sst DSP DMA pointer after
> period elapse occurs
> 
> 
> 
> > -----Original Message-----
> > From: Mark Brown [mailto:[e-mail]]
> > Sent: Tuesday, August 18, 2015 5:35 PM
> > To: Fang, Yang A
> > Cc: [e-mail]; [e-mail];
> > [e-mail]; Nujella, Sathyanarayana;
> > [e-mail]; Sripathi, Srinivas; Iriawan, Denny;
> > Jain, Praveen K; Koul, Vinod; Mirche, Dinesh
> > Subject: Re: [PATCH] ASoC: Intel: Bsw: Read sst DSP DMA pointer after
> > period elapse occurs
> >
> > On Tue, Aug 18, 2015 at 05:33:33PM +0000, Fang, Yang A wrote:
> >
> > > > > We should not read sst hw_ptr and pcm_delay in
> > > > > sst_platform_pcm_pointer Since it will be ricky if
> > > > > sst_platform_pcm_pointer is called while dsp is updating the
> > > > > timestamp.Now read sst hw_ptr after period elapse interrupt occurs.
> >
> > > > So I guess there's still some risk that we could run into problems
> > > > here if we take too long to service the interrupt and/or the
> > > > period is very short?  Is there anything we can do to improve that?
> >
> > >  I think before and after the patch. There is no difference in term
> > > of handling the period elapse interrupt . because before the patch
> > > sst_period_elapsed calls the snd_pcm_period_elapsed which in turn
> > > calling the sst_platform_pcm_pointer which calls the
> stream_read_tstamp.
> >
> > I'm not sure I understand what the patch fixes then?  My concern is
> > that we're just moving the timing around which changes but doesn't
> > address the race condition.
> 
> We are seeing the issue during the long time playback.
> The ring_buffer_counter (part of snd_sst_tstamp  struct )supposes only
> increasing by the firmware  once firmware fetches  one period size of data.
>  It will update the ring_buffer_counter in the mailbox and trigger a period
> elapse interrupt.  We have found the ring_buffer_counter got decreased
> during long time playback if we  call stream_read_tstamp to read mailbox  in
> sst_platform_pcm_pointer function.
> 
> There are two cases sst_platform_pcm_pointer will be getting called. One is
> called by sst_period_elapsed function.
>  The second case is triggered by the userspace ioctl . The second case
> supposes reading the same case  ring_buffer_counter  value as last  read
> when period elapse interrupt  occurs . However we saw it somehow
> decreases.This causes audio stutter.

So I moved  stream_read_tstamp  to read the mailbox sst_period_elapsed to make sure read after receving the interrupt
and return a cached value if other place tried to read. you are right this is indeed a workaround patch for the time being

[e-mail] Sept. 9, 2015, 6:50 p.m. UTC | #6

> -----Original Message-----
> From: Fang, Yang A
> Sent: Wednesday, August 19, 2015 6:30 PM
> To: 'Mark Brown'
> Cc: '[e-mail]'; '[e-mail]';
> '[e-mail]'; Nujella, Sathyanarayana;
> '[e-mail]'; Sripathi, Srinivas; Iriawan, Denny; Jain,
> Praveen K; Koul, Vinod; Mirche, Dinesh
> Subject: RE: [PATCH] ASoC: Intel: Bsw: Read sst DSP DMA pointer after
> period elapse occurs
> 
> 
> 
> > -----Original Message-----
> > From: Fang, Yang A
> > Sent: Wednesday, August 19, 2015 6:25 PM
> > To: 'Mark Brown'
> > Cc: [e-mail]; [e-mail];
> > [e-mail]; Nujella, Sathyanarayana;
> > [e-mail]; Sripathi, Srinivas; Iriawan, Denny;
> > Jain, Praveen K; Koul, Vinod; Mirche, Dinesh
> > Subject: RE: [PATCH] ASoC: Intel: Bsw: Read sst DSP DMA pointer after
> > period elapse occurs
> >
> >
> >
> > > -----Original Message-----
> > > From: Mark Brown [mailto:[e-mail]]
> > > Sent: Tuesday, August 18, 2015 5:35 PM
> > > To: Fang, Yang A
> > > Cc: [e-mail]; [e-mail];
> > > [e-mail]; Nujella, Sathyanarayana;
> > > [e-mail]; Sripathi, Srinivas; Iriawan, Denny;
> > > Jain, Praveen K; Koul, Vinod; Mirche, Dinesh
> > > Subject: Re: [PATCH] ASoC: Intel: Bsw: Read sst DSP DMA pointer
> > > after period elapse occurs
> > >
> > > On Tue, Aug 18, 2015 at 05:33:33PM +0000, Fang, Yang A wrote:
> > >
> > > > > > We should not read sst hw_ptr and pcm_delay in
> > > > > > sst_platform_pcm_pointer Since it will be ricky if
> > > > > > sst_platform_pcm_pointer is called while dsp is updating the
> > > > > > timestamp.Now read sst hw_ptr after period elapse interrupt
> occurs.
> > >
> > > > > So I guess there's still some risk that we could run into
> > > > > problems here if we take too long to service the interrupt
> > > > > and/or the period is very short?  Is there anything we can do to
> improve that?
> > >
> > > >  I think before and after the patch. There is no difference in
> > > > term of handling the period elapse interrupt . because before the
> > > > patch sst_period_elapsed calls the snd_pcm_period_elapsed which in
> > > > turn calling the sst_platform_pcm_pointer which calls the
> > stream_read_tstamp.
> > >
> > > I'm not sure I understand what the patch fixes then?  My concern is
> > > that we're just moving the timing around which changes but doesn't
> > > address the race condition.
> >
> > We are seeing the issue during the long time playback.
> > The ring_buffer_counter (part of snd_sst_tstamp  struct )supposes only
> > increasing by the firmware  once firmware fetches  one period size of data.
> >  It will update the ring_buffer_counter in the mailbox and trigger a
> > period elapse interrupt.  We have found the ring_buffer_counter got
> > decreased during long time playback if we  call stream_read_tstamp to
> > read mailbox  in sst_platform_pcm_pointer function.
> >
> > There are two cases sst_platform_pcm_pointer will be getting called.
> > One is called by sst_period_elapsed function.
> >  The second case is triggered by the userspace ioctl . The second case
> > supposes reading the same case  ring_buffer_counter  value as last
> > read when period elapse interrupt  occurs . However we saw it somehow
> > decreases.This causes audio stutter.
> 
> So I moved  stream_read_tstamp  to read the mailbox sst_period_elapsed to
> make sure read after receving the interrupt and return a cached value if
> other place tried to read. you are right this is indeed a workaround patch for
> the time being
Hi Mark,
Could you please let me know your further comment? 
Thanks,
Yang

Mark Brown Sept. 11, 2015, 11:49 a.m. UTC | #7

On Wed, Sep 09, 2015 at 06:50:24PM +0000, Fang, Yang A wrote:

> Could you please let me know your further comment? 

At the very least this needs a better, more comprehensible patch which
makes it clear that this is just a workaround rather than actually
fixing anything.  Of course I'd be much happier with a patch which was a
clear fix.

Patch

diff --git a/sound/soc/intel/atom/sst-mfld-platform-pcm.c b/sound/soc/intel/atom/sst-mfld-platform-pcm.c
index 683e501..d58230b 100644
--- a/sound/soc/intel/atom/sst-mfld-platform-pcm.c
+++ b/sound/soc/intel/atom/sst-mfld-platform-pcm.c
@@ -263,17 +263,27 @@  static int sst_platform_alloc_stream(struct snd_pcm_substream *substream,
 static void sst_period_elapsed(void *arg)
 {
 	struct snd_pcm_substream *substream = arg;
+	struct snd_soc_pcm_runtime *rtd;
 	struct sst_runtime_stream *stream;
+	struct pcm_stream_info *str_info;
 	int status;
+	int ret_val;
 
 	if (!substream || !substream->runtime)
 		return;
+	rtd = substream->private_data;
 	stream = substream->runtime->private_data;
 	if (!stream)
 		return;
 	status = sst_get_stream_status(stream);
 	if (status != SST_PLATFORM_RUNNING)
 		return;
+	str_info = &stream->stream_info;
+	ret_val = stream->ops->stream_read_tstamp(sst->dev, str_info);
+	if (ret_val) {
+		dev_err(rtd->dev, "sst: err code = %d\n", ret_val);
+		return;
+	}
 	snd_pcm_period_elapsed(substream);
 }
 
@@ -660,20 +670,14 @@  static snd_pcm_uframes_t sst_platform_pcm_pointer
 			(struct snd_pcm_substream *substream)
 {
 	struct sst_runtime_stream *stream;
-	int ret_val, status;
+	int status;
 	struct pcm_stream_info *str_info;
-	struct snd_soc_pcm_runtime *rtd = substream->private_data;
 
 	stream = substream->runtime->private_data;
 	status = sst_get_stream_status(stream);
 	if (status == SST_PLATFORM_INIT)
 		return 0;
 	str_info = &stream->stream_info;
-	ret_val = stream->ops->stream_read_tstamp(sst->dev, str_info);
-	if (ret_val) {
-		dev_err(rtd->dev, "sst: error code = %d\n", ret_val);
-		return ret_val;
-	}
 	substream->runtime->delay = str_info->pcm_delay;
 	return str_info->buffer_ptr;
 }