From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from alsa0.perex.cz (alsa0.perex.cz [77.48.224.243]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id E5546C433EF for ; Fri, 26 Nov 2021 08:56:50 +0000 (UTC) Received: from alsa1.perex.cz (alsa1.perex.cz [207.180.221.201]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by alsa0.perex.cz (Postfix) with ESMTPS id 2CCF018DF; Fri, 26 Nov 2021 09:55:58 +0100 (CET) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa0.perex.cz 2CCF018DF DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=alsa-project.org; s=default; t=1637917008; bh=QmAjfTegY44iTVHUZVa6uxD042LBrRoYeUCFo9trGWA=; h=Date:From:To:Subject:In-Reply-To:References:Cc:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=a4xUWdhWuOOYOkO2G28wsjGrO1M7HycMwa2zSdezEx7HHyO9FnugDmlI/HR7UI/K3 KQn3V7h+eNvikb4i+tPzR/pmU7J+AGKO1bsKXSeMRHXR0DO0ZA+/e1Ey7rhbIwph7w LoIWUBt8v2khzUytno6DPP6qIij4BpCPUQZKX4qs= Received: from alsa1.perex.cz (localhost.localdomain [127.0.0.1]) by alsa1.perex.cz (Postfix) with ESMTP id 9DD12F801F7; Fri, 26 Nov 2021 09:55:57 +0100 (CET) Received: by alsa1.perex.cz (Postfix, from userid 50401) id 3A7CBF80212; Fri, 26 Nov 2021 09:55:55 +0100 (CET) Received: from smtp-out1.suse.de (smtp-out1.suse.de [195.135.220.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by alsa1.perex.cz (Postfix) with ESMTPS id A09F0F80166 for ; Fri, 26 Nov 2021 09:55:46 +0100 (CET) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa1.perex.cz A09F0F80166 Authentication-Results: alsa1.perex.cz; dkim=pass (1024-bit key) header.d=suse.de header.i=@suse.de header.b="QDubRnoF"; dkim=permerror (0-bit key) header.d=suse.de header.i=@suse.de header.b="4MMAPU+x" Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out1.suse.de (Postfix) with ESMTP id 23228212BC; Fri, 26 Nov 2021 08:55:45 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1637916945; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=NoDLuvtLqM5derAZ9Lqjf99UB8mwCd87lURkljhybuw=; b=QDubRnoFsCou5ByoWZPyTmZHqfP3C0xTZrX/zVoI4cv5EIEOaeCTGB2TR4jLeFjXdeSCRu tqJREcci7T3/WSckwAt0Z05CyrcrEyMvTKWGtIYCSEs09jlrELSg/EaNLO0DUffSln8oi2 YSvUkrMoU9pX56pNkMNG0NkqTJpIEB0= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1637916945; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=NoDLuvtLqM5derAZ9Lqjf99UB8mwCd87lURkljhybuw=; b=4MMAPU+xElQjfwqrzP2652ULJ2Tz3u9PWwj1XZ9wmeiN3+7U+a2NahZhpHP+7O0WpcKscB oWOn3EtbJwF27uBw== Received: from alsa1.suse.de (alsa1.suse.de [10.160.4.42]) by relay2.suse.de (Postfix) with ESMTP id 089A4A3B90; Fri, 26 Nov 2021 08:55:45 +0000 (UTC) Date: Fri, 26 Nov 2021 09:55:44 +0100 Message-ID: From: Takashi Iwai To: Pierre-Louis Bossart Subject: Re: ALSA: hda: Make proper use of timecounter In-Reply-To: <871r35kwji.ffs@tglx> References: <871r35kwji.ffs@tglx> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/25.3 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=US-ASCII Cc: Cezary Rojewski , alsa-devel@alsa-project.org, Jie Yang , Takashi Iwai , LKML , Liam Girdwood , Thomas Gleixner X-BeenThere: alsa-devel@alsa-project.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: "Alsa-devel mailing list for ALSA developers - http://www.alsa-project.org" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: "Alsa-devel" 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 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); > > From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 26D93C433EF for ; Fri, 26 Nov 2021 08:57:48 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1353706AbhKZJA7 (ORCPT ); Fri, 26 Nov 2021 04:00:59 -0500 Received: from smtp-out1.suse.de ([195.135.220.28]:54014 "EHLO smtp-out1.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1359401AbhKZI66 (ORCPT ); Fri, 26 Nov 2021 03:58:58 -0500 Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out1.suse.de (Postfix) with ESMTP id 23228212BC; Fri, 26 Nov 2021 08:55:45 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1637916945; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=NoDLuvtLqM5derAZ9Lqjf99UB8mwCd87lURkljhybuw=; b=QDubRnoFsCou5ByoWZPyTmZHqfP3C0xTZrX/zVoI4cv5EIEOaeCTGB2TR4jLeFjXdeSCRu tqJREcci7T3/WSckwAt0Z05CyrcrEyMvTKWGtIYCSEs09jlrELSg/EaNLO0DUffSln8oi2 YSvUkrMoU9pX56pNkMNG0NkqTJpIEB0= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1637916945; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=NoDLuvtLqM5derAZ9Lqjf99UB8mwCd87lURkljhybuw=; b=4MMAPU+xElQjfwqrzP2652ULJ2Tz3u9PWwj1XZ9wmeiN3+7U+a2NahZhpHP+7O0WpcKscB oWOn3EtbJwF27uBw== Received: from alsa1.suse.de (alsa1.suse.de [10.160.4.42]) by relay2.suse.de (Postfix) with ESMTP id 089A4A3B90; Fri, 26 Nov 2021 08:55:45 +0000 (UTC) Date: Fri, 26 Nov 2021 09:55:44 +0100 Message-ID: From: Takashi Iwai To: Pierre-Louis Bossart Cc: Thomas Gleixner , LKML , Jaroslav Kysela , Takashi Iwai , Cezary Rojewski , Liam Girdwood , Jie Yang , alsa-devel@alsa-project.org Subject: Re: ALSA: hda: Make proper use of timecounter In-Reply-To: <871r35kwji.ffs@tglx> References: <871r35kwji.ffs@tglx> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/25.3 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=US-ASCII Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 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); > >