Re: [PATCH v3 4/4] ASoC: SOF: Intel: add .ack support for HDaudio platforms

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
> 



[Index of Archives]     [ALSA User]     [Linux Audio Users]     [Pulse Audio]     [Kernel Archive]     [Asterisk PBX]     [Photo Sharing]     [Linux Sound]     [Video 4 Linux]     [Gimp]     [Yosemite News]

  Powered by Linux