linux-kernel - Re: [PATCH] ASoC: Intel: Atom: use hardware counter to update hw

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7dec7fcd-0381-d279-4dac-77c27ea5f6fe@linux.intel.com>
Date:   Mon, 27 Jul 2020 09:09:33 -0500
From:   Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
To:     Brent Lu <brent.lu@...el.com>, alsa-devel@...a-project.org
Cc:     Cezary Rojewski <cezary.rojewski@...el.com>,
        Liam Girdwood <liam.r.girdwood@...ux.intel.com>,
        Jie Yang <yang.jie@...ux.intel.com>,
        Mark Brown <broonie@...nel.org>,
        Jaroslav Kysela <perex@...ex.cz>,
        Takashi Iwai <tiwai@...e.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ASoC: Intel: Atom: use hardware counter to update hw_ptr



On 7/26/20 11:08 AM, Brent Lu wrote:
> The ring buffer counter runs faster than hardware counter if the
> period size in hw_param is larger than 240. Although the differce is
> not much (around 2k frames), it causes false underrun in CRAS
> sometimes because it's using 256 frames as period size in hw_param.

All the Atom firmware assumes data chunks in multiples of 1ms (typically 
5, 10 or 20ms). I have never seen anyone use 256 frames, that's asking 
for trouble really.

it's actually the same with Skylake and SOF in most cases.

Is this a 'real' problem or a problem detected by the Chrome ALSA 
compliance tests, in the latter case that would hint at a too generic 
value of min_period.

> Using the hardware counter could provide precise hw_ptr to user space
> and avoid the false underrun in CRAS.
> 
> Signed-off-by: Brent Lu <brent.lu@...el.com>
> ---
>   sound/soc/intel/atom/sst/sst_drv_interface.c | 15 +++------------
>   1 file changed, 3 insertions(+), 12 deletions(-)
> 
> diff --git a/sound/soc/intel/atom/sst/sst_drv_interface.c b/sound/soc/intel/atom/sst/sst_drv_interface.c
> index 7624953..1949ad9 100644
> --- a/sound/soc/intel/atom/sst/sst_drv_interface.c
> +++ b/sound/soc/intel/atom/sst/sst_drv_interface.c
> @@ -485,7 +485,6 @@ static inline int sst_calc_tstamp(struct intel_sst_drv *ctx,
>   		struct snd_pcm_substream *substream,
>   		struct snd_sst_tstamp *fw_tstamp)
>   {
> -	size_t delay_bytes, delay_frames;
>   	size_t buffer_sz;
>   	u32 pointer_bytes, pointer_samples;
>   
> @@ -493,22 +492,14 @@ static inline int sst_calc_tstamp(struct intel_sst_drv *ctx,
>   			fw_tstamp->ring_buffer_counter);
>   	dev_dbg(ctx->dev, "mrfld hardware_counter %llu in bytes\n",
>   			 fw_tstamp->hardware_counter);
> -	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> -		delay_bytes = (size_t) (fw_tstamp->ring_buffer_counter -
> -					fw_tstamp->hardware_counter);
> -	else
> -		delay_bytes = (size_t) (fw_tstamp->hardware_counter -
> -					fw_tstamp->ring_buffer_counter);
> -	delay_frames = bytes_to_frames(substream->runtime, delay_bytes);
> +
>   	buffer_sz = snd_pcm_lib_buffer_bytes(substream);
> -	div_u64_rem(fw_tstamp->ring_buffer_counter, buffer_sz, &pointer_bytes);
> +	div_u64_rem(fw_tstamp->hardware_counter, buffer_sz, &pointer_bytes);
>   	pointer_samples = bytes_to_samples(substream->runtime, pointer_bytes);
>   
> -	dev_dbg(ctx->dev, "pcm delay %zu in bytes\n", delay_bytes);
> -
>   	info->buffer_ptr = pointer_samples / substream->runtime->channels;
> +	info->pcm_delay = 0;

and that seems also wrong? Why would the delay be zero?

> -	info->pcm_delay = delay_frames;
>   	dev_dbg(ctx->dev, "buffer ptr %llu pcm_delay rep: %llu\n",
>   			info->buffer_ptr, info->pcm_delay);
>   	return 0;
>