[alsa-devel] [PATCH] ASoC: Intel: Bsw: Read sst DSP DMA pointer after period elapse occurs
From: "Fang, Yang A" yang.a.fang@intel.com
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 yang.a.fang@intel.com Acked-by: Vinod Koul vinod.koul@intel.com --- sound/soc/intel/atom/sst-mfld-platform-pcm.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-)
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; }
On Sun, Aug 16, 2015 at 11:12:25PM -0700, yang.a.fang@intel.com 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?
-----Original Message----- From: Mark Brown [mailto:broonie@kernel.org] Sent: Monday, August 17, 2015 12:56 PM To: Fang, Yang A Cc: lgirdwood@gmail.com; alsa-devel@alsa-project.org; dgreid@chromium.org; Nujella, Sathyanarayana; kevin.strasser@linux.intel.com; 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, yang.a.fang@intel.com 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.
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.
-----Original Message----- From: Mark Brown [mailto:broonie@kernel.org] Sent: Tuesday, August 18, 2015 5:35 PM To: Fang, Yang A Cc: lgirdwood@gmail.com; alsa-devel@alsa-project.org; dgreid@chromium.org; Nujella, Sathyanarayana; kevin.strasser@linux.intel.com; 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.
-----Original Message----- From: Fang, Yang A Sent: Wednesday, August 19, 2015 6:25 PM To: 'Mark Brown' Cc: lgirdwood@gmail.com; alsa-devel@alsa-project.org; dgreid@chromium.org; Nujella, Sathyanarayana; kevin.strasser@linux.intel.com; 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:broonie@kernel.org] Sent: Tuesday, August 18, 2015 5:35 PM To: Fang, Yang A Cc: lgirdwood@gmail.com; alsa-devel@alsa-project.org; dgreid@chromium.org; Nujella, Sathyanarayana; kevin.strasser@linux.intel.com; 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
-----Original Message----- From: Fang, Yang A Sent: Wednesday, August 19, 2015 6:30 PM To: 'Mark Brown' Cc: 'lgirdwood@gmail.com'; 'alsa-devel@alsa-project.org'; 'dgreid@chromium.org'; Nujella, Sathyanarayana; 'kevin.strasser@linux.intel.com'; 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: lgirdwood@gmail.com; alsa-devel@alsa-project.org; dgreid@chromium.org; Nujella, Sathyanarayana; kevin.strasser@linux.intel.com; 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:broonie@kernel.org] Sent: Tuesday, August 18, 2015 5:35 PM To: Fang, Yang A Cc: lgirdwood@gmail.com; alsa-devel@alsa-project.org; dgreid@chromium.org; Nujella, Sathyanarayana; kevin.strasser@linux.intel.com; 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
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.
participants (3)
-
Fang, Yang A -
Mark Brown -
yang.a.fang＠intel.com