Re: ALSA: hda: Make proper use of timecounter

[Takashi Iwai]

On Wed, 24 Nov 2021 23:40:01 +0100,
Thomas Gleixner wrote:
> 
> HDA uses a timecounter to read a hardware clock running at 24 MHz. The
> conversion factor is set with a mult value of 125 and a shift value of 0,
> which is not converting the hardware clock to nanoseconds, it is converting
> to 1/3 nanoseconds because the conversion factor from 24Mhz to nanoseconds
> is 125/3. The usage sites divide the "nanoseconds" value returned by
> timecounter_read() by 3 to get a real nanoseconds value.
> 
> There is a lengthy comment in azx_timecounter_init() explaining this
> choice. That comment makes blatantly wrong assumptions about how
> timecounters work and what can overflow.
> 
> The comment says:
> 
>      * Applying the 1/3 factor as part of the multiplication
>      * requires at least 20 bits for a decent precision, however
>      * overflows occur after about 4 hours or less, not a option.
> 
> timecounters operate on time deltas between two readouts of a clock and use
> the mult/shift pair to calculate a precise nanoseconds value:
> 
>     delta_nsec = (delta_clock * mult) >> shift;
> 
> The fractional part is also taken into account and preserved to prevent
> accumulated rounding errors. For details see cyclecounter_cyc2ns().
> 
> The mult/shift pair has to be chosen so that the multiplication of the
> maximum expected delta value does not result in a 64bit overflow. As the
> counter wraps around on 32bit, the maximum observable delta between two
> reads is (1 << 32) - 1 which is about 178.9 seconds.
> 
> That in turn means the maximum multiplication factor which fits into an u32
> will not cause a 64bit overflow ever because it's guaranteed that:
> 
>      ((1 << 32) - 1) ^ 2 < (1 << 64)
> 
> The resulting correct multiplication factor is 2796202667 and the shift
> value is 26, i.e. 26 bit precision. The overflow of the multiplication
> would happen exactly at a clock readout delta of 6597069765 which is way
> after the wrap around of the hardware clock at around 274.8 seconds which
> is off from the claimed 4 hours by more than an order of magnitude.
> 
> If the counter ever wraps around the last read value then the calculation
> is off by the number of wrap arounds times 178.9 seconds because the
> overflow cannot be observed.
> 
> Use clocks_calc_mult_shift(), which calculates the most accurate mult/shift
> pair based on the given clock frequency, and remove the bogus comment along
> with the divisions at the readout sites.
> 
> Fixes: 5d890f591d15 ("ALSA: hda: support for wallclock timestamps")
> Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>

Looks like a nice fix / cleanup.  Pierre, could you check it?


thanks,

Takashi


> ---
>  sound/hda/hdac_stream.c           |   14 ++++----------
>  sound/pci/hda/hda_controller.c    |    1 -
>  sound/soc/intel/skylake/skl-pcm.c |    1 -
>  3 files changed, 4 insertions(+), 12 deletions(-)
> 
> --- a/sound/hda/hdac_stream.c
> +++ b/sound/hda/hdac_stream.c
> @@ -534,17 +534,11 @@ static void azx_timecounter_init(struct
>  	cc->mask = CLOCKSOURCE_MASK(32);
>  
>  	/*
> -	 * Converting from 24 MHz to ns means applying a 125/3 factor.
> -	 * To avoid any saturation issues in intermediate operations,
> -	 * the 125 factor is applied first. The division is applied
> -	 * last after reading the timecounter value.
> -	 * Applying the 1/3 factor as part of the multiplication
> -	 * requires at least 20 bits for a decent precision, however
> -	 * overflows occur after about 4 hours or less, not a option.
> +	 * Calculate the optimal mult/shift values. The counter wraps
> +	 * around after ~178.9 seconds.
>  	 */
> -
> -	cc->mult = 125; /* saturation after 195 years */
> -	cc->shift = 0;
> +	clocks_calc_mult_shift(&cc->mult, &cc->shift, 24000000,
> +			       NSEC_PER_SEC, 178);
>  
>  	nsec = 0; /* audio time is elapsed time since trigger */
>  	timecounter_init(tc, cc, nsec);
> --- a/sound/pci/hda/hda_controller.c
> +++ b/sound/pci/hda/hda_controller.c
> @@ -504,7 +504,6 @@ static int azx_get_time_info(struct snd_
>  		snd_pcm_gettime(substream->runtime, system_ts);
>  
>  		nsec = timecounter_read(&azx_dev->core.tc);
> -		nsec = div_u64(nsec, 3); /* can be optimized */
>  		if (audio_tstamp_config->report_delay)
>  			nsec = azx_adjust_codec_delay(substream, nsec);
>  
> --- a/sound/soc/intel/skylake/skl-pcm.c
> +++ b/sound/soc/intel/skylake/skl-pcm.c
> @@ -1251,7 +1251,6 @@ static int skl_platform_soc_get_time_inf
>  		snd_pcm_gettime(substream->runtime, system_ts);
>  
>  		nsec = timecounter_read(&hstr->tc);
> -		nsec = div_u64(nsec, 3); /* can be optimized */
>  		if (audio_tstamp_config->report_delay)
>  			nsec = skl_adjust_codec_delay(substream, nsec);
>  
>