[15/27] ALSA: hda - Use timecounter_initialize interface
diff mbox

Message ID 1513323522-15021-16-git-send-email-sagar.a.kamble@intel.com
State New
Headers show

Commit Message

sagar.a.kamble@intel.com Dec. 15, 2017, 7:38 a.m. UTC
With new interface timecounter_initialize we can initialize timecounter
fields and underlying cyclecounter together. Update azx timecounter
init with this new function.

Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Richard Cochran <richardcochran@gmail.com>
Cc: Jaroslav Kysela <perex@perex.cz>
Cc: Takashi Iwai <tiwai@suse.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Vinod Koul <vinod.koul@intel.com>
Cc: alsa-devel@alsa-project.org
Cc: linux-kernel@vger.kernel.org
---
 sound/hda/hdac_stream.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

Comments

Takashi Iwai Dec. 15, 2017, 11:10 a.m. UTC | #1
On Fri, 15 Dec 2017 08:38:30 +0100,
Sagar Arun Kamble wrote:
> 
> With new interface timecounter_initialize we can initialize timecounter
> fields and underlying cyclecounter together. Update azx timecounter
> init with this new function.
> 
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Richard Cochran <richardcochran@gmail.com>
> Cc: Jaroslav Kysela <perex@perex.cz>
> Cc: Takashi Iwai <tiwai@suse.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Vinod Koul <vinod.koul@intel.com>
> Cc: alsa-devel@alsa-project.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  sound/hda/hdac_stream.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/sound/hda/hdac_stream.c b/sound/hda/hdac_stream.c
> index 9426c1a..ad91dde 100644
> --- a/sound/hda/hdac_stream.c
> +++ b/sound/hda/hdac_stream.c
> @@ -477,12 +477,8 @@ static void azx_timecounter_init(struct hdac_stream *azx_dev,
>  				 bool force, u64 last)
>  {
>  	struct timecounter *tc = &azx_dev->tc;
> -	struct cyclecounter *cc = &azx_dev->tc.cc;
>  	u64 nsec;
>  
> -	cc->read = azx_cc_read;
> -	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,
> @@ -493,11 +489,13 @@ static void azx_timecounter_init(struct hdac_stream *azx_dev,
>  	 * overflows occur after about 4 hours or less, not a option.
>  	 */
>  
> -	cc->mult = 125; /* saturation after 195 years */
> -	cc->shift = 0;
> -
>  	nsec = 0; /* audio time is elapsed time since trigger */
> -	timecounter_init(tc, nsec);
> +	timecounter_initialize(tc,
> +			       azx_cc_read,
> +			       CLOCKSOURCE_MASK(32),
> +			       125, /* saturation after 195 years */
> +			       0,
> +			       nsec);

Hmm, a function with so many arguments is difficult to remember and is
often error-prone.  By this transition, it becomes harder to read
through.


thanks,

Takashi
Richard Cochran Dec. 15, 2017, 4:51 p.m. UTC | #2
On Fri, Dec 15, 2017 at 12:10:47PM +0100, Takashi Iwai wrote:

> > -	struct cyclecounter *cc = &azx_dev->tc.cc;

> > -	cc->read = azx_cc_read;
> > -	cc->mask = CLOCKSOURCE_MASK(32);

> > -	cc->mult = 125; /* saturation after 195 years */
> > -	cc->shift = 0;

I want to get away from this mess of open coded structure
initialization and use a proper functional interface instead.

> >  	nsec = 0; /* audio time is elapsed time since trigger */
> > -	timecounter_init(tc, nsec);
> > +	timecounter_initialize(tc,
> > +			       azx_cc_read,
> > +			       CLOCKSOURCE_MASK(32),
> > +			       125, /* saturation after 195 years */
> > +			       0,
> > +			       nsec);
> 
> Hmm, a function with so many arguments is difficult to remember and is
> often error-prone.  By this transition, it becomes harder to read
> through.

Please suggest a better way.

Thanks,
Richard
Takashi Iwai Dec. 15, 2017, 5:10 p.m. UTC | #3
On Fri, 15 Dec 2017 17:51:25 +0100,
Richard Cochran wrote:
> 
> On Fri, Dec 15, 2017 at 12:10:47PM +0100, Takashi Iwai wrote:
> 
> > > -	struct cyclecounter *cc = &azx_dev->tc.cc;
> 
> > > -	cc->read = azx_cc_read;
> > > -	cc->mask = CLOCKSOURCE_MASK(32);
> 
> > > -	cc->mult = 125; /* saturation after 195 years */
> > > -	cc->shift = 0;
> 
> I want to get away from this mess of open coded structure
> initialization and use a proper functional interface instead.

I agree that a proper functional interface would be better, too.
But not a form like foo(501, 21, 10, 499, 5678).  
In C syntax, you may more easily pass a wrong value than open codes.

> > >  	nsec = 0; /* audio time is elapsed time since trigger */
> > > -	timecounter_init(tc, nsec);
> > > +	timecounter_initialize(tc,
> > > +			       azx_cc_read,
> > > +			       CLOCKSOURCE_MASK(32),
> > > +			       125, /* saturation after 195 years */
> > > +			       0,
> > > +			       nsec);
> > 
> > Hmm, a function with so many arguments is difficult to remember and is
> > often error-prone.  By this transition, it becomes harder to read
> > through.
> 
> Please suggest a better way.

I have no good idea ATM, sorry.

Or can we provide simpler versions for covering some defaults?  At
least reducing the number of arguments would make things easier.


Takashi
sagar.a.kamble@intel.com Dec. 26, 2017, 7:37 a.m. UTC | #4
On 12/15/2017 10:40 PM, Takashi Iwai wrote:
> On Fri, 15 Dec 2017 17:51:25 +0100,
> Richard Cochran wrote:
>> On Fri, Dec 15, 2017 at 12:10:47PM +0100, Takashi Iwai wrote:
>>
>>>> -	struct cyclecounter *cc = &azx_dev->tc.cc;
>>>> -	cc->read = azx_cc_read;
>>>> -	cc->mask = CLOCKSOURCE_MASK(32);
>>>> -	cc->mult = 125; /* saturation after 195 years */
>>>> -	cc->shift = 0;
>> I want to get away from this mess of open coded structure
>> initialization and use a proper functional interface instead.
> I agree that a proper functional interface would be better, too.
> But not a form like foo(501, 21, 10, 499, 5678).
> In C syntax, you may more easily pass a wrong value than open codes.
>
>>>>   	nsec = 0; /* audio time is elapsed time since trigger */
>>>> -	timecounter_init(tc, nsec);
>>>> +	timecounter_initialize(tc,
>>>> +			       azx_cc_read,
>>>> +			       CLOCKSOURCE_MASK(32),
>>>> +			       125, /* saturation after 195 years */
>>>> +			       0,
>>>> +			       nsec);
>>> Hmm, a function with so many arguments is difficult to remember and is
>>> often error-prone.  By this transition, it becomes harder to read
>>> through.
>> Please suggest a better way.
> I have no good idea ATM, sorry.
>
> Or can we provide simpler versions for covering some defaults?  At
> least reducing the number of arguments would make things easier.
Thought about specifying 1. cyclecounter read func 2. frequency 3. width 
of counter as parameters here
which can get rid of mult, shift params. But this is not easy as most of 
the drivers do not specify
cyclecounter frequency and instead hard-code the mult/shift factors.
How about passing initialized cyclecounter struct?
>
> Takashi
Richard Cochran Dec. 28, 2017, 4:49 p.m. UTC | #5
On Tue, Dec 26, 2017 at 01:07:35PM +0530, Sagar Arun Kamble wrote:
> > Or can we provide simpler versions for covering some defaults?  At
> > least reducing the number of arguments would make things easier.
> Thought about specifying 1. cyclecounter read func 2. frequency 3. width of
> counter as parameters here
> which can get rid of mult, shift params. But this is not easy as most of the
> drivers do not specify
> cyclecounter frequency and instead hard-code the mult/shift factors.

You are talking about using clocks_calc_mult_shift() here, right? (See
the usage example in drivers/net/ethernet/ti/cpts.c).

This is a good idea, and it is worth getting the driver authors' input
to figure out the correct parameters.

I bet we can use that almost everywhere.  If there are any drivers
that cannot be converted, then we can leave some sort of low level
legacy initialization method.

Thanks,
Richard
sagar.a.kamble@intel.com Jan. 2, 2018, 6:03 a.m. UTC | #6
On 12/28/2017 10:19 PM, Richard Cochran wrote:
> On Tue, Dec 26, 2017 at 01:07:35PM +0530, Sagar Arun Kamble wrote:
>>> Or can we provide simpler versions for covering some defaults?  At
>>> least reducing the number of arguments would make things easier.
>> Thought about specifying 1. cyclecounter read func 2. frequency 3. width of
>> counter as parameters here
>> which can get rid of mult, shift params. But this is not easy as most of the
>> drivers do not specify
>> cyclecounter frequency and instead hard-code the mult/shift factors.
> You are talking about using clocks_calc_mult_shift() here, right? (See
> the usage example in drivers/net/ethernet/ti/cpts.c).
Yes
> This is a good idea, and it is worth getting the driver authors' input
> to figure out the correct parameters.
>
> I bet we can use that almost everywhere.  If there are any drivers
> that cannot be converted, then we can leave some sort of low level
> legacy initialization method.
Agree
> Thanks,
> Richard
>
Pierre-Louis Bossart Jan. 2, 2018, 5:15 p.m. UTC | #7
On 1/2/18 12:03 AM, Sagar Arun Kamble wrote:
> 
> 
> On 12/28/2017 10:19 PM, Richard Cochran wrote:
>> On Tue, Dec 26, 2017 at 01:07:35PM +0530, Sagar Arun Kamble wrote:
>>>> Or can we provide simpler versions for covering some defaults?  At
>>>> least reducing the number of arguments would make things easier.
>>> Thought about specifying 1. cyclecounter read func 2. frequency 3. 
>>> width of
>>> counter as parameters here
>>> which can get rid of mult, shift params. But this is not easy as most 
>>> of the
>>> drivers do not specify
>>> cyclecounter frequency and instead hard-code the mult/shift factors.
>> You are talking about using clocks_calc_mult_shift() here, right? (See
>> the usage example in drivers/net/ethernet/ti/cpts.c).
> Yes
>> This is a good idea, and it is worth getting the driver authors' input
>> to figure out the correct parameters.

I wrote the code for HDaudio and I remember wasting time trying to 
figure out the gory details of the cycle counter stuff when all I wanted 
was a conversion from a 24MHz counter to ns values using a 125/3 
operation in the right order - as explained in the comments

If there was a helper to set those mult/shift values it'd make the 
HDaudio code clearer (and also help support newer modes of operation 
with a 12 and 6 MHz MCLK).

The initial proposal with hard-coded values in arguments instead of 
structure members didn't really make the code clearer.


>>
>> I bet we can use that almost everywhere.  If there are any drivers
>> that cannot be converted, then we can leave some sort of low level
>> legacy initialization method.
> Agree
>> Thanks,
>> Richard
>>
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Richard Cochran Jan. 2, 2018, 6:21 p.m. UTC | #8
On Tue, Jan 02, 2018 at 11:15:45AM -0600, Pierre-Louis Bossart wrote:
> I wrote the code for HDaudio and I remember wasting time trying to figure
> out the gory details of the cycle counter stuff when all I wanted was a
> conversion from a 24MHz counter to ns values using a 125/3 operation in the
> right order - as explained in the comments

Would using clocks_calc_mult_shift() work for you?

Thanks,
Richard
Pierre-Louis Bossart Jan. 2, 2018, 7:53 p.m. UTC | #9
On 1/2/18 12:21 PM, Richard Cochran wrote:
> On Tue, Jan 02, 2018 at 11:15:45AM -0600, Pierre-Louis Bossart wrote:
>> I wrote the code for HDaudio and I remember wasting time trying to figure
>> out the gory details of the cycle counter stuff when all I wanted was a
>> conversion from a 24MHz counter to ns values using a 125/3 operation in the
>> right order - as explained in the comments
> 
> Would using clocks_calc_mult_shift() work for you?

In theory yes, but I'd need to re-check what the results would be.
I remember applying the 1/3 factor separately to avoid wrap-around after 
4 hours [1], but I can't remember the details on the analysis. I can't 
figure out what the 'maxsec' argument should be either.

[1] 
http://elixir.free-electrons.com/linux/latest/source/sound/hda/hdac_stream.c#L486
sagar.a.kamble@intel.com Jan. 5, 2018, 10:06 a.m. UTC | #10
On 1/3/2018 1:23 AM, Pierre-Louis Bossart wrote:
> On 1/2/18 12:21 PM, Richard Cochran wrote:
>> On Tue, Jan 02, 2018 at 11:15:45AM -0600, Pierre-Louis Bossart wrote:
>>> I wrote the code for HDaudio and I remember wasting time trying to 
>>> figure
>>> out the gory details of the cycle counter stuff when all I wanted was a
>>> conversion from a 24MHz counter to ns values using a 125/3 operation 
>>> in the
>>> right order - as explained in the comments
>>
>> Would using clocks_calc_mult_shift() work for you?
>
> In theory yes, but I'd need to re-check what the results would be.
> I remember applying the 1/3 factor separately to avoid wrap-around 
> after 4 hours [1], but I can't remember the details on the analysis. I 
> can't figure out what the 'maxsec' argument should be either.
>
I am not sure if I understood the wrap-around correctly. Is 
AZX_REG_WALL_CLK 64bit or 32bit and in the comments which 20 bits are 
being referred.

Keeping maxsec at lower value will ensure good precision but the mult 
factor derived then might lead to overflow if the interval of counter 
read is big.
Keeping maxsec high will reduce the mult factor and will marginally 
impact the precision (of the order of 6 decimal places of fraction nano 
second).

For 24mhz clock I am getting following scale factors at different maxsec 
values. I think these are as good as 125/3
125/3 gives scale factor of 41.666666666666666666666666666667

maxsec, mult, shift, scale factor (mult/(2^shift))
0, 2796202667, 26,     41.66666667163372039794921875
3600, 87381333, 21,   41.666666507720947265625
14400, 21845333, 19, 41.6666660308837890625

I see sound driver uses only timecounter_read so conversions should be fine.
If there are usages of timecounter_cyc2time then we will have to take 
care of updating the timecounter often as
timecounter API internally counts time backwards if counter is spaced 
more than 1/2 the range.

Thanks
Sagar
> [1] 
> http://elixir.free-electrons.com/linux/latest/source/sound/hda/hdac_stream.c#L486 
>
Pierre-Louis Bossart Jan. 5, 2018, 3:43 p.m. UTC | #11
On 1/5/18 4:06 AM, Sagar Arun Kamble wrote:
> 
> 
> On 1/3/2018 1:23 AM, Pierre-Louis Bossart wrote:
>> On 1/2/18 12:21 PM, Richard Cochran wrote:
>>> On Tue, Jan 02, 2018 at 11:15:45AM -0600, Pierre-Louis Bossart wrote:
>>>> I wrote the code for HDaudio and I remember wasting time trying to 
>>>> figure
>>>> out the gory details of the cycle counter stuff when all I wanted was a
>>>> conversion from a 24MHz counter to ns values using a 125/3 operation 
>>>> in the
>>>> right order - as explained in the comments
>>>
>>> Would using clocks_calc_mult_shift() work for you?
>>
>> In theory yes, but I'd need to re-check what the results would be.
>> I remember applying the 1/3 factor separately to avoid wrap-around 
>> after 4 hours [1], but I can't remember the details on the analysis. I 
>> can't figure out what the 'maxsec' argument should be either.
>>
> I am not sure if I understood the wrap-around correctly. Is 
> AZX_REG_WALL_CLK 64bit or 32bit and in the comments which 20 bits are 
> being referred.

it's a 32-bit counter.
off the top of my head, the idea was that the integer arithmetic should 
not degrade the precision (42ns) and that means you need to be careful 
with the fractional part, especially if the errors with the fractional 
part accumulate over time (I think this was the case when I looked 
several years ago). That's the main reason why I did the division by 3 
last, after the read, so that the precision is not impacted by the 
interval between two reads.

You also need to be careful with the multiplication factor otherwise you 
will exceed the 64-bit resolution. For example with the 14400 factor, 
you cannot handle a counter larger than 2^64/14400, which gives you 
14826 hours or 1.69 years. It's one of those 'nobody will ever need more 
than 640KB' value. The 125 factor gives you 195 years without saturating.

> 
> Keeping maxsec at lower value will ensure good precision but the mult 
> factor derived then might lead to overflow if the interval of counter 
> read is big.
> Keeping maxsec high will reduce the mult factor and will marginally 
> impact the precision (of the order of 6 decimal places of fraction nano 
> second).
> 
> For 24mhz clock I am getting following scale factors at different maxsec 
> values. I think these are as good as 125/3
> 125/3 gives scale factor of 41.666666666666666666666666666667
> 
> maxsec, mult, shift, scale factor (mult/(2^shift))
> 0, 2796202667, 26,     41.66666667163372039794921875
> 3600, 87381333, 21,   41.666666507720947265625
> 14400, 21845333, 19, 41.6666660308837890625
> 
> I see sound driver uses only timecounter_read so conversions should be 
> fine.
> If there are usages of timecounter_cyc2time then we will have to take 
> care of updating the timecounter often as
> timecounter API internally counts time backwards if counter is spaced 
> more than 1/2 the range.
> 
> Thanks
> Sagar
>> [1] 
>> http://elixir.free-electrons.com/linux/latest/source/sound/hda/hdac_stream.c#L486 
>>
>
sagar.a.kamble@intel.com Jan. 9, 2018, 10:09 a.m. UTC | #12
On 1/5/2018 9:13 PM, Pierre-Louis Bossart wrote:
> On 1/5/18 4:06 AM, Sagar Arun Kamble wrote:
>>
>>
>> On 1/3/2018 1:23 AM, Pierre-Louis Bossart wrote:
>>> On 1/2/18 12:21 PM, Richard Cochran wrote:
>>>> On Tue, Jan 02, 2018 at 11:15:45AM -0600, Pierre-Louis Bossart wrote:
>>>>> I wrote the code for HDaudio and I remember wasting time trying to 
>>>>> figure
>>>>> out the gory details of the cycle counter stuff when all I wanted 
>>>>> was a
>>>>> conversion from a 24MHz counter to ns values using a 125/3 
>>>>> operation in the
>>>>> right order - as explained in the comments
>>>>
>>>> Would using clocks_calc_mult_shift() work for you?
>>>
>>> In theory yes, but I'd need to re-check what the results would be.
>>> I remember applying the 1/3 factor separately to avoid wrap-around 
>>> after 4 hours [1], but I can't remember the details on the analysis. 
>>> I can't figure out what the 'maxsec' argument should be either.
>>>
>> I am not sure if I understood the wrap-around correctly. Is 
>> AZX_REG_WALL_CLK 64bit or 32bit and in the comments which 20 bits are 
>> being referred.
>
> it's a 32-bit counter.
> off the top of my head, the idea was that the integer arithmetic 
> should not degrade the precision (42ns) and that means you need to be 
> careful with the fractional part, especially if the errors with the 
> fractional part accumulate over time (I think this was the case when I 
> looked several years ago). That's the main reason why I did the 
> division by 3 last, after the read, so that the precision is not 
> impacted by the interval between two reads.
>
timecounter interface ensures to add the fractional ns time so it has 
good precision. In the table below 3600 indicates interval of 1min 
between two counter reads
and 14400 indicates that interval of 4 minutes.  Shift factor controls 
the ns precision we want.
Considering duration of 195 years of interval between counter reads at 
24mhz we get mult=83 and shift=1 which will give cycle duration = 41.5.
What is the maximum duration of interval over which this read can be 
done in audio driver.
> You also need to be careful with the multiplication factor otherwise 
> you will exceed the 64-bit resolution. For example with the 14400 
> factor, you cannot handle a counter larger than 2^64/14400, which 
> gives you 14826 hours or 1.69 years. It's one of those 'nobody will 
> ever need more than 640KB' value. The 125 factor gives you 195 years 
> without saturating.
>
>>
>> Keeping maxsec at lower value will ensure good precision but the mult 
>> factor derived then might lead to overflow if the interval of counter 
>> read is big.
>> Keeping maxsec high will reduce the mult factor and will marginally 
>> impact the precision (of the order of 6 decimal places of fraction 
>> nano second).
>>
>> For 24mhz clock I am getting following scale factors at different 
>> maxsec values. I think these are as good as 125/3
>> 125/3 gives scale factor of 41.666666666666666666666666666667
>>
>> maxsec, mult, shift, scale factor (mult/(2^shift))
>> 0, 2796202667, 26,     41.66666667163372039794921875
>> 3600, 87381333, 21,   41.666666507720947265625
>> 14400, 21845333, 19, 41.6666660308837890625
>>
>> I see sound driver uses only timecounter_read so conversions should 
>> be fine.
>> If there are usages of timecounter_cyc2time then we will have to take 
>> care of updating the timecounter often as
>> timecounter API internally counts time backwards if counter is spaced 
>> more than 1/2 the range.
>>
>> Thanks
>> Sagar
>>> [1] 
>>> http://elixir.free-electrons.com/linux/latest/source/sound/hda/hdac_stream.c#L486 
>>>
>>
>

Patch
diff mbox

diff --git a/sound/hda/hdac_stream.c b/sound/hda/hdac_stream.c
index 9426c1a..ad91dde 100644
--- a/sound/hda/hdac_stream.c
+++ b/sound/hda/hdac_stream.c
@@ -477,12 +477,8 @@  static void azx_timecounter_init(struct hdac_stream *azx_dev,
 				 bool force, u64 last)
 {
 	struct timecounter *tc = &azx_dev->tc;
-	struct cyclecounter *cc = &azx_dev->tc.cc;
 	u64 nsec;
 
-	cc->read = azx_cc_read;
-	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,
@@ -493,11 +489,13 @@  static void azx_timecounter_init(struct hdac_stream *azx_dev,
 	 * overflows occur after about 4 hours or less, not a option.
 	 */
 
-	cc->mult = 125; /* saturation after 195 years */
-	cc->shift = 0;
-
 	nsec = 0; /* audio time is elapsed time since trigger */
-	timecounter_init(tc, nsec);
+	timecounter_initialize(tc,
+			       azx_cc_read,
+			       CLOCKSOURCE_MASK(32),
+			       125, /* saturation after 195 years */
+			       0,
+			       nsec);
 	if (force) {
 		/*
 		 * force timecounter to use predefined value,