Re: [PATCH] ALSA: hda: Reduce udelay() at SKL+ position reporting

[Takashi Iwai <tiwai@suse.de>]

On Wed, 22 Sep 2021 00:18:14 +0200,
Pierre-Louis Bossart wrote:
> 
> Sorry Takashi for the delay, I missed your reply.
> 
> >>> The position reporting on Intel Skylake and later chips via
> >>> azx_get_pos_skl() contains a udelay(20) call for the capture streams.
> >>> A call for this alone doesn't sound too harmful.  However, as the
> >>> pointer PCM ops is one of the hottest path in the PCM operations --
> >>> especially for the timer-scheduled operations like PulseAudio -- such
> >>> a delay hogs CPU usage significantly in the total performance.
> >>>
> >>> The function there was taken from the original code in ASoC SST
> >>> Skylake driver blindly.  The udelay() is a workaround for the case
> >>> where the reported position is behind the period boundary at the
> >>> timing triggered from interrupts; applications often expect that the
> >>> full data is available for the whole period when returned (and also
> >>> that's the definition of the ALSA PCM period).
> >>
> >> that initial work-around from the Intel SST driver does not seem to be
> >> legit in the first place, when I checked with hardware folks recently no
> >> one understands why there are delays and special cases for capture. The
> >> only requirement wrt. DPIB is that the DDR update is valid across VC0
> >> and VC1, while the DPIB registers are only valid with VC0. For SOF we
> >> don't know of any VC1 use so will default to the DPIB vendor-specific
> >> registers.
> > 
> > What are those VC0 and VC1 registers?  I can't find the definitions in
> > the code, so I assume that none of ALSA/ASoC drivers use VC1.
> 
> These are PCI concepts/capabilities. VC stands for "Virtual Channel",
> which are mapped to Traffic Class (TC). These registers are typically
> set by the BIOS AFAIK. The point is that if VC1 is enabled only the DPIB
> updates are valid, the vendor-specific will report information can be
> misleading.
> 
> The recommendation from hardware folks is to use DPIB updates in DDR,
> which are known to work across both VC0 and VC1.
> 
> For SOF, we do know VC1 is never used so we'll use the vendor-specific
> registers.

OK, thanks for clarification.

> >> See https://github.com/thesofproject/linux/pull/3143 for my WIP fixes
> >> for SOF.
> >>
> >> I don't have the time to look at this specific patch today but wanted to
> >> make sure you are aware of my on-going fixes.
> >>
> >> Note that the use of DPIB works best if you don't use the IOC interrupt.
> >> when the interrupt is thrown, there is likely a delay for the DPIB
> >> information to be refreshed.
> > 
> > Thanks for the information!  The delay could be the reason of the
> > udelay(), and that's superfluous as mentioned in the commit.
> > 
> > So the remaining question seems to be which method is a better
> > approach for the capture stream: DPIB or posbuf.  I kept the posbuf
> > just for conservatism, but judging from your comment, we may use DPIB
> > for both directions safely?
> 
> sorry you lost me. Isn't DPIB updates in DDR precisely what posbuf reports?
> 
> I think what you need to use is use azx_get_pos_posbuf(chip, azx_dev);
> unconditionally, both for capture and playback, and remove the use of
> the skylake specific stuff and the delay.

When I measured, I saw a slight difference between values in DPIB and
posbuf, so I wonder which is actually more accurate.  The latter is
sometimes a bit behind the former.

If the posbuf is more correct in the sense for the PCM pointer, we can
simply move back to the posbuf like other platforms.


thanks,

Takashi


> > In anyway, the additional mechanism to check the delayed position
> > report in this patch can be kept no matter which way (DPIB or posbuf)
> > is used.
> 
> Agree!
> 
> > 
> > 
> > Takashi
> > 
> >>
> >>>
> >>> OTOH, HD-audio (legacy) driver has already some workarounds for the
> >>> delayed position reporting due to its relatively large FIFO, such as
> >>> the BDL position adjustment and the delayed period-elapsed call in the
> >>> work.  That said, the udelay() is almost superfluous for HD-audio
> >>> driver unlike SST, and we can drop the udelay().
> >>>
> >>> Though, the current code doesn't guarantee the full period readiness
> >>> as mentioned in the above, but rather it checks the wallclock and
> >>> detects the unexpected jump.  That's one missing piece, and the drop
> >>> of udelay() needs a bit more sanity checks for the delayed handling.
> >>>
> >>> This patch implements those: the drop of udelay() call in
> >>> azx_get_pos_skl() and the more proper check of hwptr in
> >>> azx_position_ok().  The latter change is applied only for the case
> >>> where the stream is running in the normal mode without
> >>> no_period_wakeup flag.  When no_period_wakeup is set, it essentially
> >>> ignores the period handling and rather concentrates only on the
> >>> current position; which implies that we don't need to care about the
> >>> period boundary at all.
> >>>
> >>> Fixes: f87e7f25893d ("ALSA: hda - Improved position reporting on SKL+")
> >>> Reported-by: Jens Axboe <axboe@kernel.dk>
> >>> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> >>> ---
> >>>  sound/pci/hda/hda_intel.c | 28 +++++++++++++++++++++++-----
> >>>  1 file changed, 23 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> >>> index 3aa432d814a2..faeeeb923d5e 100644
> >>> --- a/sound/pci/hda/hda_intel.c
> >>> +++ b/sound/pci/hda/hda_intel.c
> >>> @@ -637,13 +637,17 @@ static int azx_position_check(struct azx *chip, struct azx_dev *azx_dev)
> >>>   * the update-IRQ timing.  The IRQ is issued before actually the
> >>>   * data is processed.  So, we need to process it afterwords in a
> >>>   * workqueue.
> >>> + *
> >>> + * Returns 1 if OK to proceed, 0 for delay handling, -1 for skipping update
> >>>   */
> >>>  static int azx_position_ok(struct azx *chip, struct azx_dev *azx_dev)
> >>>  {
> >>>  	struct snd_pcm_substream *substream = azx_dev->core.substream;
> >>> +	struct snd_pcm_runtime *runtime = substream->runtime;
> >>>  	int stream = substream->stream;
> >>>  	u32 wallclk;
> >>>  	unsigned int pos;
> >>> +	snd_pcm_uframes_t hwptr, target;
> >>>  
> >>>  	wallclk = azx_readl(chip, WALLCLK) - azx_dev->core.start_wallclk;
> >>>  	if (wallclk < (azx_dev->core.period_wallclk * 2) / 3)
> >>> @@ -680,6 +684,24 @@ static int azx_position_ok(struct azx *chip, struct azx_dev *azx_dev)
> >>>  		/* NG - it's below the first next period boundary */
> >>>  		return chip->bdl_pos_adj ? 0 : -1;
> >>>  	azx_dev->core.start_wallclk += wallclk;
> >>> +
> >>> +	if (azx_dev->core.no_period_wakeup)
> >>> +		return 1; /* OK, no need to check period boundary */
> >>> +
> >>> +	if (runtime->hw_ptr_base != runtime->hw_ptr_interrupt)
> >>> +		return 1; /* OK, already in hwptr updating process */
> >>> +
> >>> +	/* check whether the period gets really elapsed */
> >>> +	pos = bytes_to_frames(runtime, pos);
> >>> +	hwptr = runtime->hw_ptr_base + pos;
> >>> +	if (hwptr < runtime->status->hw_ptr)
> >>> +		hwptr += runtime->buffer_size;
> >>> +	target = runtime->hw_ptr_interrupt + runtime->period_size;
> >>> +	if (hwptr < target) {
> >>> +		/* too early wakeup, process it later */
> >>> +		return chip->bdl_pos_adj ? 0 : -1;
> >>> +	}
> >>> +
> >>>  	return 1; /* OK, it's fine */
> >>>  }
> >>>  
> >>> @@ -874,11 +896,7 @@ static unsigned int azx_get_pos_skl(struct azx *chip, struct azx_dev *azx_dev)
> >>>  	if (azx_dev->core.substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> >>>  		return azx_skl_get_dpib_pos(chip, azx_dev);
> >>>  
> >>> -	/* For capture, we need to read posbuf, but it requires a delay
> >>> -	 * for the possible boundary overlap; the read of DPIB fetches the
> >>> -	 * actual posbuf
> >>> -	 */
> >>> -	udelay(20);
> >>> +	/* read of DPIB fetches the actual posbuf */
> >>>  	azx_skl_get_dpib_pos(chip, azx_dev);
> >>>  	return azx_get_pos_posbuf(chip, azx_dev);
> >>>  }
> >>>
> >>
>