On Fri, 15 Oct 2021 21:59:32 +0200,
Pierre-Louis Bossart wrote:
>
> From: Ranjani Sridharan <ranjani.sridharan@xxxxxxxxxxxxxxx>
>
> When we disable rewinds, then the .ack can be used to program SPIB
> with the application pointer, which allows the HDaudio DMA to save
> power by opportunistically bursting data transfers when the path to
> memory is enabled (and conversely to shut it down when there are no
> transfer requests).
>
> The SPIB register can only be programmed with incremental values with
> wrap-around after the DMA RUN bits are set. For simplicity, we set the
> INFO_NO_REWINDS flag in the .open callback when we already need to
> program the SNDRV_PCM_INFO_EXPLICIT_SYNC flag.
Using this flag itself isn't wrong, but if we need to check only
appl_ptr updates, a more appropriate flag is
SNDRV_PCM_INFO_SYNC_APPLPTR. This will still allow the mmap of status
(i.e. hwptr update) while the mmap of control is disabled for
appl_ptr. SNDRV_PCM_INFO_EXPLICIT_SYNC flag disables both, instead.
thanks,
Takashi
> Rewinds are not used by many applications. One notable application
> using rewinds is PulseAudio. Practical experiments with
> Ubuntu/PulseAudio default settings did not show any audible issues,
> but the user may hear volume changes and notification with a delay,
> depending on the size of the ring buffer and latency constraints.
>
> The choice of disabling rewinds is exposed as a kernel parameter and
> not a Kconfig option to avoid any undesirable side-effects.
>
> Reviewed-by: Péter Ujfalusi <peter.ujfalusi@xxxxxxxxxxxxxxx>
> Reviewed-by: Kai Vehmanen <kai.vehmanen@xxxxxxxxxxxxxxx>
> Reviewed-by: Ranjani Sridharan <ranjani.sridharan@xxxxxxxxxxxxxxx>
> Co-developed-by: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>
> Signed-off-by: Ranjani Sridharan <ranjani.sridharan@xxxxxxxxxxxxxxx>
> ---
> sound/soc/sof/intel/apl.c | 1 +
> sound/soc/sof/intel/cnl.c | 1 +
> sound/soc/sof/intel/hda-pcm.c | 41 ++++++++++++++++++++++++++++++--
> sound/soc/sof/intel/hda-stream.c | 2 ++
> sound/soc/sof/intel/hda.h | 1 +
> sound/soc/sof/intel/tgl.c | 1 +
> 6 files changed, 45 insertions(+), 2 deletions(-)
>
> diff --git a/sound/soc/sof/intel/apl.c b/sound/soc/sof/intel/apl.c
> index 917f78cf6daf..689679014ade 100644
> --- a/sound/soc/sof/intel/apl.c
> +++ b/sound/soc/sof/intel/apl.c
> @@ -78,6 +78,7 @@ const struct snd_sof_dsp_ops sof_apl_ops = {
> .pcm_hw_free = hda_dsp_stream_hw_free,
> .pcm_trigger = hda_dsp_pcm_trigger,
> .pcm_pointer = hda_dsp_pcm_pointer,
> + .pcm_ack = hda_dsp_pcm_ack,
>
> #if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA_PROBES)
> /* probe callbacks */
> diff --git a/sound/soc/sof/intel/cnl.c b/sound/soc/sof/intel/cnl.c
> index 3957e2b3db32..d128c08ba726 100644
> --- a/sound/soc/sof/intel/cnl.c
> +++ b/sound/soc/sof/intel/cnl.c
> @@ -283,6 +283,7 @@ const struct snd_sof_dsp_ops sof_cnl_ops = {
> .pcm_hw_free = hda_dsp_stream_hw_free,
> .pcm_trigger = hda_dsp_pcm_trigger,
> .pcm_pointer = hda_dsp_pcm_pointer,
> + .pcm_ack = hda_dsp_pcm_ack,
>
> #if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA_PROBES)
> /* probe callbacks */
> diff --git a/sound/soc/sof/intel/hda-pcm.c b/sound/soc/sof/intel/hda-pcm.c
> index cc8ddef37f37..875350283eac 100644
> --- a/sound/soc/sof/intel/hda-pcm.c
> +++ b/sound/soc/sof/intel/hda-pcm.c
> @@ -32,6 +32,10 @@ static bool hda_always_enable_dmi_l1;
> module_param_named(always_enable_dmi_l1, hda_always_enable_dmi_l1, bool, 0444);
> MODULE_PARM_DESC(always_enable_dmi_l1, "SOF HDA always enable DMI l1");
>
> +static bool hda_disable_rewinds = IS_ENABLED(CONFIG_SND_SOC_SOF_HDA_DISABLE_REWINDS);
> +module_param_named(disable_rewinds, hda_disable_rewinds, bool, 0444);
> +MODULE_PARM_DESC(disable_rewinds, "SOF HDA disable rewinds");
> +
> u32 hda_dsp_get_mult_div(struct snd_sof_dev *sdev, int rate)
> {
> switch (rate) {
> @@ -120,8 +124,11 @@ int hda_dsp_pcm_hw_params(struct snd_sof_dev *sdev,
> return ret;
> }
>
> - /* disable SPIB, to enable buffer wrap for stream */
> - hda_dsp_stream_spib_config(sdev, stream, HDA_DSP_SPIB_DISABLE, 0);
> + /* enable SPIB when rewinds are disabled */
> + if (hda_disable_rewinds)
> + hda_dsp_stream_spib_config(sdev, stream, HDA_DSP_SPIB_ENABLE, 0);
> + else
> + hda_dsp_stream_spib_config(sdev, stream, HDA_DSP_SPIB_DISABLE, 0);
>
> /* update no_stream_position flag for ipc params */
> if (hda && hda->no_ipc_position) {
> @@ -140,6 +147,29 @@ int hda_dsp_pcm_hw_params(struct snd_sof_dev *sdev,
> return 0;
> }
>
> +/* update SPIB register with appl position */
> +int hda_dsp_pcm_ack(struct snd_sof_dev *sdev, struct snd_pcm_substream *substream)
> +{
> + struct hdac_stream *hstream = substream->runtime->private_data;
> + struct hdac_ext_stream *stream = stream_to_hdac_ext_stream(hstream);
> + struct snd_pcm_runtime *runtime = substream->runtime;
> + ssize_t appl_pos, buf_size;
> + u32 spib;
> +
> + appl_pos = frames_to_bytes(runtime, runtime->control->appl_ptr);
> + buf_size = frames_to_bytes(runtime, runtime->buffer_size);
> +
> + spib = appl_pos % buf_size;
> +
> + /* Allowable value for SPIB is 1 byte to max buffer size */
> + if (!spib)
> + spib = buf_size;
> +
> + sof_io_write(sdev, stream->spib_addr, spib);
> +
> + return 0;
> +}
> +
> int hda_dsp_pcm_trigger(struct snd_sof_dev *sdev,
> struct snd_pcm_substream *substream, int cmd)
> {
> @@ -234,6 +264,13 @@ int hda_dsp_pcm_open(struct snd_sof_dev *sdev,
> return -EINVAL;
> }
>
> + /*
> + * if we want the .ack to work, we need to prevent the status and
> + * control from being mapped
> + */
> + if (hda_disable_rewinds)
> + runtime->hw.info |= SNDRV_PCM_INFO_NO_REWINDS | SNDRV_PCM_INFO_EXPLICIT_SYNC;
> +
> /*
> * All playback streams are DMI L1 capable, capture streams need
> * pause push/release to be disabled
> diff --git a/sound/soc/sof/intel/hda-stream.c b/sound/soc/sof/intel/hda-stream.c
> index 1d845c2cbc33..b6f037815344 100644
> --- a/sound/soc/sof/intel/hda-stream.c
> +++ b/sound/soc/sof/intel/hda-stream.c
> @@ -655,6 +655,8 @@ int hda_dsp_stream_hw_free(struct snd_sof_dev *sdev,
> SOF_HDA_REG_PP_PPCTL, mask, 0);
> spin_unlock_irq(&bus->reg_lock);
>
> + hda_dsp_stream_spib_config(sdev, link_dev, HDA_DSP_SPIB_DISABLE, 0);
> +
> stream->substream = NULL;
>
> return 0;
> diff --git a/sound/soc/sof/intel/hda.h b/sound/soc/sof/intel/hda.h
> index 1195018a1f4f..6829d36fbfe9 100644
> --- a/sound/soc/sof/intel/hda.h
> +++ b/sound/soc/sof/intel/hda.h
> @@ -533,6 +533,7 @@ int hda_dsp_pcm_trigger(struct snd_sof_dev *sdev,
> struct snd_pcm_substream *substream, int cmd);
> snd_pcm_uframes_t hda_dsp_pcm_pointer(struct snd_sof_dev *sdev,
> struct snd_pcm_substream *substream);
> +int hda_dsp_pcm_ack(struct snd_sof_dev *sdev, struct snd_pcm_substream *substream);
>
> /*
> * DSP Stream Operations.
> diff --git a/sound/soc/sof/intel/tgl.c b/sound/soc/sof/intel/tgl.c
> index 48da8e7a67bc..2a29058e0c20 100644
> --- a/sound/soc/sof/intel/tgl.c
> +++ b/sound/soc/sof/intel/tgl.c
> @@ -73,6 +73,7 @@ const struct snd_sof_dsp_ops sof_tgl_ops = {
> .pcm_hw_free = hda_dsp_stream_hw_free,
> .pcm_trigger = hda_dsp_pcm_trigger,
> .pcm_pointer = hda_dsp_pcm_pointer,
> + .pcm_ack = hda_dsp_pcm_ack,
>
> #if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA_PROBES)
> /* probe callbacks */
> --
> 2.25.1
>
