| Message ID | 1490275926-2270-10-git-send-email-jeeja.kp@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 3/23/17 8:32 AM, jeeja.kp@intel.com wrote: > From: Hardik T Shah <hardik.t.shah@intel.com> > > As per hardware recommendation, for every capture stream completion > following operations need to be done in order to reflect the actual > data that is received in position buffer. > > 1. Wait for 20us before reading the DMA position in buffer once the > interrupt is generated for stream completion. > 2. Read any of the register to flush the DMA position value. This is > dummy read operation. > > Signed-off-by: Dharageswari R <dharageswari.r@intel.com> > Signed-off-by: Hardik T Shah <hardik.t.shah@intel.com> > Signed-off-by: Jeeja KP <jeeja.kp@intel.com> > --- > sound/soc/intel/skylake/skl-pcm.c | 22 ++++++++++++++++++++-- > 1 file changed, 20 insertions(+), 2 deletions(-) > > diff --git a/sound/soc/intel/skylake/skl-pcm.c b/sound/soc/intel/skylake/skl-pcm.c > index ef440d8..149221c 100644 > --- a/sound/soc/intel/skylake/skl-pcm.c > +++ b/sound/soc/intel/skylake/skl-pcm.c > @@ -21,6 +21,7 @@ > > #include <linux/pci.h> > #include <linux/pm_runtime.h> > +#include <linux/delay.h> > #include <sound/pcm_params.h> > #include <sound/soc.h> > #include "skl.h" > @@ -1063,13 +1064,30 @@ static snd_pcm_uframes_t skl_platform_pcm_pointer > * HAD space reflects the actual data that is transferred. > * Use the position buffer for capture, as DPIB write gets > * completed earlier than the actual data written to the DDR. > + * > + * For capture stream following workaround is required to fix the > + * incorrect position reporting. > + * > + * 1. Wait for 20us before reading the DMA position in buffer once > + * the interrupt is generated for stream completion. is this really 20us regardless of the sampling frequency/channel count? 20us is one sample at 48kHz so wondering how generic this work-around is... > + * 2. Read DPIB register to flush the DMA position value. This dummy > + * read is required to flush DMA position value. > + * 3. Read the DMA Position-in-Buffer. This value now will be equal to > + * or greater than period boundary. > */ > - if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) > + > + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { > pos = readl(ebus->bus.remap_addr + AZX_REG_VS_SDXDPIB_XBASE + > (AZX_REG_VS_SDXDPIB_XINTERVAL * > hdac_stream(hstream)->index)); > - else > + } else { > + udelay(20); > + readl(ebus->bus.remap_addr + > + AZX_REG_VS_SDXDPIB_XBASE + > + (AZX_REG_VS_SDXDPIB_XINTERVAL * > + hdac_stream(hstream)->index)); > pos = snd_hdac_stream_get_pos_posbuf(hdac_stream(hstream)); > + } > > if (pos >= hdac_stream(hstream)->bufsize) > pos = 0; >
>-----Original Message----- >From: Pierre-Louis Bossart [mailto:pierre-louis.bossart@linux.intel.com] >Sent: Thursday, March 23, 2017 7:13 AM >To: Kp, Jeeja <jeeja.kp@intel.com>; alsa-devel@alsa-project.org >Cc: R, Dharageswari <dharageswari.r@intel.com>; tiwai@suse.de; Shah, Hardik >T <hardik.t.shah@intel.com>; Patches Audio <patches.audio@intel.com>; >broonie@kernel.org; Girdwood, Liam R <liam.r.girdwood@intel.com> >Subject: Re: [alsa-devel] [PATCH 09/11] ASoC: Intel: Skylake: Fix DMA position >reporting for capture stream > >On 3/23/17 8:32 AM, jeeja.kp@intel.com wrote: >> From: Hardik T Shah <hardik.t.shah@intel.com> >> >> As per hardware recommendation, for every capture stream completion >> following operations need to be done in order to reflect the actual >> data that is received in position buffer. >> >> 1. Wait for 20us before reading the DMA position in buffer once the >> interrupt is generated for stream completion. >> 2. Read any of the register to flush the DMA position value. This is >> dummy read operation. >> >> Signed-off-by: Dharageswari R <dharageswari.r@intel.com> >> Signed-off-by: Hardik T Shah <hardik.t.shah@intel.com> >> Signed-off-by: Jeeja KP <jeeja.kp@intel.com> >> --- >> sound/soc/intel/skylake/skl-pcm.c | 22 ++++++++++++++++++++-- >> 1 file changed, 20 insertions(+), 2 deletions(-) >> >> diff --git a/sound/soc/intel/skylake/skl-pcm.c >b/sound/soc/intel/skylake/skl-pcm.c >> index ef440d8..149221c 100644 >> --- a/sound/soc/intel/skylake/skl-pcm.c >> +++ b/sound/soc/intel/skylake/skl-pcm.c >> @@ -21,6 +21,7 @@ >> >> #include <linux/pci.h> >> #include <linux/pm_runtime.h> >> +#include <linux/delay.h> >> #include <sound/pcm_params.h> >> #include <sound/soc.h> >> #include "skl.h" >> @@ -1063,13 +1064,30 @@ static snd_pcm_uframes_t >skl_platform_pcm_pointer >> * HAD space reflects the actual data that is transferred. >> * Use the position buffer for capture, as DPIB write gets >> * completed earlier than the actual data written to the DDR. >> + * >> + * For capture stream following workaround is required to fix the >> + * incorrect position reporting. >> + * >> + * 1. Wait for 20us before reading the DMA position in buffer once >> + * the interrupt is generated for stream completion. > >is this really 20us regardless of the sampling frequency/channel count? >20us is one sample at 48kHz so wondering how generic this work-around is... Yes, this is independent of PCM parameters. Update happens on the HDA frame boundary i.e. 20.833uSec. > >> + * 2. Read DPIB register to flush the DMA position value. This dummy >> + * read is required to flush DMA position value. >> + * 3. Read the DMA Position-in-Buffer. This value now will be equal to >> + * or greater than period boundary. >> */ >> - if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) >> + >> + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { >> pos = readl(ebus->bus.remap_addr + >AZX_REG_VS_SDXDPIB_XBASE + >> (AZX_REG_VS_SDXDPIB_XINTERVAL * >> hdac_stream(hstream)->index)); >> - else >> + } else { >> + udelay(20); >> + readl(ebus->bus.remap_addr + >> + AZX_REG_VS_SDXDPIB_XBASE + >> + (AZX_REG_VS_SDXDPIB_XINTERVAL * >> + hdac_stream(hstream)->index)); >> pos = >snd_hdac_stream_get_pos_posbuf(hdac_stream(hstream)); >> + } >> >> if (pos >= hdac_stream(hstream)->bufsize) >> pos = 0; >>
>>> * HAD space reflects the actual data that is transferred. >>> * Use the position buffer for capture, as DPIB write gets >>> * completed earlier than the actual data written to the DDR. >>> + * >>> + * For capture stream following workaround is required to fix the >>> + * incorrect position reporting. >>> + * >>> + * 1. Wait for 20us before reading the DMA position in buffer once >>> + * the interrupt is generated for stream completion. >> >> is this really 20us regardless of the sampling frequency/channel count? >> 20us is one sample at 48kHz so wondering how generic this work-around is... > > Yes, this is independent of PCM parameters. Update happens on the > HDA frame boundary i.e. 20.833uSec. Makes sense. it's worth updating the comments though to make it self-explanatory.
diff --git a/sound/soc/intel/skylake/skl-pcm.c b/sound/soc/intel/skylake/skl-pcm.c index ef440d8..149221c 100644 --- a/sound/soc/intel/skylake/skl-pcm.c +++ b/sound/soc/intel/skylake/skl-pcm.c @@ -21,6 +21,7 @@ #include <linux/pci.h> #include <linux/pm_runtime.h> +#include <linux/delay.h> #include <sound/pcm_params.h> #include <sound/soc.h> #include "skl.h" @@ -1063,13 +1064,30 @@ static snd_pcm_uframes_t skl_platform_pcm_pointer * HAD space reflects the actual data that is transferred. * Use the position buffer for capture, as DPIB write gets * completed earlier than the actual data written to the DDR. + * + * For capture stream following workaround is required to fix the + * incorrect position reporting. + * + * 1. Wait for 20us before reading the DMA position in buffer once + * the interrupt is generated for stream completion. + * 2. Read DPIB register to flush the DMA position value. This dummy + * read is required to flush DMA position value. + * 3. Read the DMA Position-in-Buffer. This value now will be equal to + * or greater than period boundary. */ - if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) + + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { pos = readl(ebus->bus.remap_addr + AZX_REG_VS_SDXDPIB_XBASE + (AZX_REG_VS_SDXDPIB_XINTERVAL * hdac_stream(hstream)->index)); - else + } else { + udelay(20); + readl(ebus->bus.remap_addr + + AZX_REG_VS_SDXDPIB_XBASE + + (AZX_REG_VS_SDXDPIB_XINTERVAL * + hdac_stream(hstream)->index)); pos = snd_hdac_stream_get_pos_posbuf(hdac_stream(hstream)); + } if (pos >= hdac_stream(hstream)->bufsize) pos = 0;