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. >> 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. > 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@xxxxxxxxx> >>> Signed-off-by: Takashi Iwai <tiwai@xxxxxxx> >>> --- >>> 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); >>> } >>> >>
