[PATCH 1/2] MdePkg/TimerLib: Add a function to calculate elapsed time

[PATCH 1/2] MdePkg/TimerLib: Add a function to calculate elapsed time

[Pankaj Bansal]

This function calculates the time elaped in Naoseconds between call to
this function and BaseTime, which is passed as argument.

This is particularly useful in detecting timeout conditions.

Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>

diff --git a/MdePkg/Include/Library/TimerLib.h b/MdePkg/Include/Library/TimerLib.h
index ecc3ad3..82a5c5c 100644
--- a/MdePkg/Include/Library/TimerLib.h
+++ b/MdePkg/Include/Library/TimerLib.h
@@ -111,4 +111,20 @@ GetTimeInNanoSecond (
   IN      UINT64                     Ticks
   );
 
+/**
+  Get Elapsed time in Nanoseonds w.r.t BaseTime
+
+  This function calculates the time elaped in Naoseconds between call to this
+  function and BaseTime, which is passed as argument.
+
+  @param  BaseTime     BaseTime in NanoSeconds.
+
+  @return The elapsed time in nanoseconds.
+
+**/
+UINT64
+EFIAPI
+GetElapsedTime (
+  IN      UINT64                     BaseTime
+  );
 #endif
-- 
2.7.4

[PATCH 2/2] ArmPkg/ArmArchTimerLib: Implement GetElapsedTime function of TimerLib

[Pankaj Bansal]

This function calculates the time elaped in Naoseconds between call to
this function and BaseTime, which is passed as argument.

This is particularly useful in detecting timeout conditions.

Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>

diff --git a/ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.c b/ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.c
index b81293c..0898339 100644
--- a/ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.c
+++ b/ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.c
@@ -290,3 +290,39 @@ GetTimeInNanoSecond (
 
   return NanoSeconds;
 }
+
+/**
+  Get Elapsed time in Nanoseonds w.r.t BaseTime
+
+  This function calculates the time elaped in Naoseconds between call to this
+  function and BaseTime, which is passed as argument.
+
+  @param  BaseTime     BaseTime in NanoSeconds.
+
+  @return The elapsed time in nanoseconds.
+
+**/
+UINT64
+EFIAPI
+GetElapsedTime (
+  IN      UINT64                     BaseTime
+  )
+{
+  UINT64  NanoSeconds;
+  UINT64  Ticks;
+
+  //
+  // Get current Ticks.
+  //
+  Ticks = GetPerformanceCounter();
+
+  //
+  // Convert Ticks to Nanoseconds
+  //
+  NanoSeconds = GetTimeInNanoSecond (Ticks);
+
+  //
+  // return the difference
+  //
+  return (NanoSeconds - BaseTime);
+}
-- 
2.7.4

Re: [PATCH 2/2] ArmPkg/ArmArchTimerLib: Implement GetElapsedTime function of TimerLib

[Laszlo Ersek]

Hello Pankaj,

On 01/10/18 10:31, Pankaj Bansal wrote:
> This function calculates the time elaped in Naoseconds between call to
> this function and BaseTime, which is passed as argument.
>
> This is particularly useful in detecting timeout conditions.
>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
>
> diff --git a/ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.c b/ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.c
> index b81293c..0898339 100644
> --- a/ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.c
> +++ b/ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.c
> @@ -290,3 +290,39 @@ GetTimeInNanoSecond (
>
>    return NanoSeconds;
>  }
> +
> +/**
> +  Get Elapsed time in Nanoseonds w.r.t BaseTime
> +
> +  This function calculates the time elaped in Naoseconds between call to this
> +  function and BaseTime, which is passed as argument.
> +
> +  @param  BaseTime     BaseTime in NanoSeconds.
> +
> +  @return The elapsed time in nanoseconds.
> +
> +**/
> +UINT64
> +EFIAPI
> +GetElapsedTime (
> +  IN      UINT64                     BaseTime
> +  )
> +{
> +  UINT64  NanoSeconds;
> +  UINT64  Ticks;
> +
> +  //
> +  // Get current Ticks.
> +  //
> +  Ticks = GetPerformanceCounter();
> +
> +  //
> +  // Convert Ticks to Nanoseconds
> +  //
> +  NanoSeconds = GetTimeInNanoSecond (Ticks);
> +
> +  //
> +  // return the difference
> +  //
> +  return (NanoSeconds - BaseTime);
> +}
>

There are two problems with this patch set:

(1) The TimerLib.h file (in the first patch) is a public library class
header, for which several library instances (implementations) exist. So,
introducing a new API requires adding an implementation (the same
implementation, or different ones, as necessary) to *all* instances in
the edk2 tree.

(2) The calculation in your GetElapsedTime() function is wrong.
GetTimeInNanoSecond() converts a small *difference* of ticks to time. It
does not convert an absolute tick value to an absolute time.

Furthermore, tick differences are less trivial to calculate than one
might imagine, because (a) a performance counter may count down, and
*independently*, (b) the numeric low bound of the counter range may not
be zero.

Earlier I proposed a new TimerTickDiffLib class (and implementation) for
centralizing exactly this kind of calculation. Please see the thread at:

  [edk2] TimerTickDiffLib for MdePkg?
  http://mid.mail-archive.com/8cba2a58-1333-7733-031d-0883dbd844c6@redhat.com

Thanks
Laszlo

Re: [PATCH 2/2] ArmPkg/ArmArchTimerLib: Implement GetElapsedTime function of TimerLib

[Pankaj Bansal]

Hi Laszlo,

Thanks for reviewing my changes and thanks for pointing me to the thread that you worked on.
It was really helpful. please see my response inline ..

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, January 10, 2018 7:46 PM
> To: Pankaj Bansal <pankaj.bansal@nxp.com>; edk2-devel@lists.01.org
> Cc: Leif Lindholm <leif.lindholm@linaro.org>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>; Michael Kinney <michael.d.kinney@intel.com>
> Subject: Re: [edk2] [PATCH 2/2] ArmPkg/ArmArchTimerLib: Implement
> GetElapsedTime function of TimerLib
> 
> Hello Pankaj,
> 
> On 01/10/18 10:31, Pankaj Bansal wrote:
> > This function calculates the time elaped in Naoseconds between call to
> > this function and BaseTime, which is passed as argument.
> >
> > This is particularly useful in detecting timeout conditions.
> >
> > Cc: Leif Lindholm <leif.lindholm@linaro.org>
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
> >
> > diff --git a/ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.c
> > b/ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.c
> > index b81293c..0898339 100644
> > --- a/ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.c
> > +++ b/ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.c
> > @@ -290,3 +290,39 @@ GetTimeInNanoSecond (
> >
> >    return NanoSeconds;
> >  }
> > +
> > +/**
> > +  Get Elapsed time in Nanoseonds w.r.t BaseTime
> > +
> > +  This function calculates the time elaped in Naoseconds between call
> > + to this  function and BaseTime, which is passed as argument.
> > +
> > +  @param  BaseTime     BaseTime in NanoSeconds.
> > +
> > +  @return The elapsed time in nanoseconds.
> > +
> > +**/
> > +UINT64
> > +EFIAPI
> > +GetElapsedTime (
> > +  IN      UINT64                     BaseTime
> > +  )
> > +{
> > +  UINT64  NanoSeconds;
> > +  UINT64  Ticks;
> > +
> > +  //
> > +  // Get current Ticks.
> > +  //
> > +  Ticks = GetPerformanceCounter();
> > +
> > +  //
> > +  // Convert Ticks to Nanoseconds
> > +  //
> > +  NanoSeconds = GetTimeInNanoSecond (Ticks);
> > +
> > +  //
> > +  // return the difference
> > +  //
> > +  return (NanoSeconds - BaseTime);
> > +}
> >
> 
> There are two problems with this patch set:
> 
> (1) The TimerLib.h file (in the first patch) is a public library class header, for
> which several library instances (implementations) exist. So, introducing a
> new API requires adding an implementation (the same implementation, or
> different ones, as necessary) to *all* instances in the edk2 tree.

I agree with your intent to prevent code duplication. But I still feel that we should enhance TimerLib.
Because, a user intending to use various timer functionalities, should include only one header file (TimerLib.h) and one Library.
Like Mike pointed out in your thread to enhance the TimerTickDiffLib, in future if someone wishes to add some new functionality related
to timer and if he writes a new library for it, then its added cumbersome for users (user has to include 3 header files and libraries 3.)

To prevent code duplication, can we look at other ways like using weak symbols for such functions that are platform agnostic.
Or putting these generic functions in one .c file in MdePkg and include that file in inf file of TimerLib implementation?

> 
> (2) The calculation in your GetElapsedTime() function is wrong.
> GetTimeInNanoSecond() converts a small *difference* of ticks to time. It
> does not convert an absolute tick value to an absolute time.

The GetTimeInNanoSecond() is only concerned with the ticks. Those ticks can be a difference of ticks or absolute ticks from counter start.
The *difference* part of GetElapsedTime would depend on its usage. Like this :
Start = GetElapsedTime (0);
// do something 
End = GetElapsedTime (Start);

> 
> Furthermore, tick differences are less trivial to calculate than one might
> imagine, because (a) a performance counter may count down, and
> *independently*, (b) the numeric low bound of the counter range may not
> be zero.
> 

This is nice observation. It was an oversight on my part.

> Earlier I proposed a new TimerTickDiffLib class (and implementation) for
> centralizing exactly this kind of calculation. Please see the thread at:
> 
>   [edk2] TimerTickDiffLib for MdePkg?
> 
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fmid.
> mail-archive.com%2F8cba2a58-1333-7733-031d-
> 0883dbd844c6%40redhat.com&data=02%7C01%7Cpankaj.bansal%40nxp.co
> m%7C17efb96f35f0456be43108d55834c721%7C686ea1d3bc2b4c6fa92cd99c5
> c301635%7C0%7C0%7C636511906063092098&sdata=AIqII9VcJEQXrJmM79G
> DS%2BzBu3%2BCUva%2FZESawMRndzA%3D&reserved=0
> 

Your library has everything that I need. Can we work on to include this in MdePkg ?
Then I won't need my changes.

> Thanks
> Laszlo

Re: [PATCH 2/2] ArmPkg/ArmArchTimerLib: Implement GetElapsedTime function of TimerLib

[Laszlo Ersek]

Hi Pankaj,

On 01/10/18 17:05, Pankaj Bansal wrote:
> Hi Laszlo,
> 
> Thanks for reviewing my changes and thanks for pointing me to the thread that you worked on.
> It was really helpful. please see my response inline ..
> 
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Wednesday, January 10, 2018 7:46 PM
>> To: Pankaj Bansal <pankaj.bansal@nxp.com>; edk2-devel@lists.01.org
>> Cc: Leif Lindholm <leif.lindholm@linaro.org>; Ard Biesheuvel
>> <ard.biesheuvel@linaro.org>; Michael Kinney <michael.d.kinney@intel.com>
>> Subject: Re: [edk2] [PATCH 2/2] ArmPkg/ArmArchTimerLib: Implement
>> GetElapsedTime function of TimerLib
>>
>> Hello Pankaj,
>>
>> On 01/10/18 10:31, Pankaj Bansal wrote:
>>> This function calculates the time elaped in Naoseconds between call to
>>> this function and BaseTime, which is passed as argument.
>>>
>>> This is particularly useful in detecting timeout conditions.
>>>
>>> Cc: Leif Lindholm <leif.lindholm@linaro.org>
>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
>>>
>>> diff --git a/ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.c
>>> b/ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.c
>>> index b81293c..0898339 100644
>>> --- a/ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.c
>>> +++ b/ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.c
>>> @@ -290,3 +290,39 @@ GetTimeInNanoSecond (
>>>
>>>    return NanoSeconds;
>>>  }
>>> +
>>> +/**
>>> +  Get Elapsed time in Nanoseonds w.r.t BaseTime
>>> +
>>> +  This function calculates the time elaped in Naoseconds between call
>>> + to this  function and BaseTime, which is passed as argument.
>>> +
>>> +  @param  BaseTime     BaseTime in NanoSeconds.
>>> +
>>> +  @return The elapsed time in nanoseconds.
>>> +
>>> +**/
>>> +UINT64
>>> +EFIAPI
>>> +GetElapsedTime (
>>> +  IN      UINT64                     BaseTime
>>> +  )
>>> +{
>>> +  UINT64  NanoSeconds;
>>> +  UINT64  Ticks;
>>> +
>>> +  //
>>> +  // Get current Ticks.
>>> +  //
>>> +  Ticks = GetPerformanceCounter();
>>> +
>>> +  //
>>> +  // Convert Ticks to Nanoseconds
>>> +  //
>>> +  NanoSeconds = GetTimeInNanoSecond (Ticks);
>>> +
>>> +  //
>>> +  // return the difference
>>> +  //
>>> +  return (NanoSeconds - BaseTime);
>>> +}
>>>
>>
>> There are two problems with this patch set:
>>
>> (1) The TimerLib.h file (in the first patch) is a public library class header, for
>> which several library instances (implementations) exist. So, introducing a
>> new API requires adding an implementation (the same implementation, or
>> different ones, as necessary) to *all* instances in the edk2 tree.
> 
> I agree with your intent to prevent code duplication. But I still feel that we should enhance TimerLib.
> Because, a user intending to use various timer functionalities, should include only one header file (TimerLib.h) and one Library.
> Like Mike pointed out in your thread to enhance the TimerTickDiffLib, in future if someone wishes to add some new functionality related
> to timer and if he writes a new library for it, then its added cumbersome for users (user has to include 3 header files and libraries 3.)
> 
> To prevent code duplication, can we look at other ways like using weak symbols for such functions that are platform agnostic.
> Or putting these generic functions in one .c file in MdePkg and include that file in inf file of TimerLib implementation?
> 
>>
>> (2) The calculation in your GetElapsedTime() function is wrong.
>> GetTimeInNanoSecond() converts a small *difference* of ticks to time. It
>> does not convert an absolute tick value to an absolute time.
> 
> The GetTimeInNanoSecond() is only concerned with the ticks. Those ticks can be a difference of ticks or absolute ticks from counter start.
> The *difference* part of GetElapsedTime would depend on its usage. Like this :
> Start = GetElapsedTime (0);
> // do something 
> End = GetElapsedTime (Start);
> 
>>
>> Furthermore, tick differences are less trivial to calculate than one might
>> imagine, because (a) a performance counter may count down, and
>> *independently*, (b) the numeric low bound of the counter range may not
>> be zero.
>>
> 
> This is nice observation. It was an oversight on my part.
> 
>> Earlier I proposed a new TimerTickDiffLib class (and implementation) for
>> centralizing exactly this kind of calculation. Please see the thread at:
>>
>>   [edk2] TimerTickDiffLib for MdePkg?
>>
>> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fmid.
>> mail-archive.com%2F8cba2a58-1333-7733-031d-
>> 0883dbd844c6%40redhat.com&data=02%7C01%7Cpankaj.bansal%40nxp.co
>> m%7C17efb96f35f0456be43108d55834c721%7C686ea1d3bc2b4c6fa92cd99c5
>> c301635%7C0%7C0%7C636511906063092098&sdata=AIqII9VcJEQXrJmM79G
>> DS%2BzBu3%2BCUva%2FZESawMRndzA%3D&reserved=0
>>
> 
> Your library has everything that I need. Can we work on to include this in MdePkg ?
> Then I won't need my changes.

(1) Please feel free to pick up my code and work on upstreaming it --
just please preserve my S-o-b on the patch(es) (in addition to yours),
and the Red Hat copyright notice (in addition to your company's) in the
new files.

(2) Last time I counted (reading up on the original thread), there were
21 TimerLib instances in edk2. Adding the same function to every one of
them looks terrible. In edk2, I know of only two ways to avoid this:

- introduce a new library class (with class header file), and a simple
library instance (implementation)

- collect (=move) as many as possible of the existing library instances
under common subdirectories, so that their INF files are basically in
the same subdirectory. Then the new code can be implemented in a new C
file, under the same subdirectory, and then all INF files can refer to
the new C file directly. This kind of code sharing works very well if
the library instances are otherwise closely related. It does not work at
all if the library instances live in separate top-level packages (which
is BTW the case here).


It seems that Mike wasn't opposed to introducing a TimerTickDiffLib
class (or something similar) to MdePkg. However, more work looked
necessary than I had expected, esp. with regard to the caching of timer
hardware characteristics (please see the last few messages in the thread
-- either the code has to add its own caching, or all existing, suitable
TimerLib instances have to be verified about caching). Again, please
feel free to pick up the code and rework it as Mike suggests -- it's
basically the GetTickDifference() function only that adds value. Feel
free to hammer it into any shape or form necessary.

Thanks
Laszlo

Re: [PATCH 2/2] ArmPkg/ArmArchTimerLib: Implement GetElapsedTime function of TimerLib

[Pankaj Bansal]

Hi Laszlo,

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, January 10, 2018 10:34 PM
> To: Pankaj Bansal <pankaj.bansal@nxp.com>; edk2-devel@lists.01.org
> Cc: Leif Lindholm <leif.lindholm@linaro.org>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>; Michael Kinney <michael.d.kinney@intel.com>
> Subject: Re: [edk2] [PATCH 2/2] ArmPkg/ArmArchTimerLib: Implement
> GetElapsedTime function of TimerLib
> 
> Hi Pankaj,
> 
> On 01/10/18 17:05, Pankaj Bansal wrote:
> > Hi Laszlo,
> >
> > Thanks for reviewing my changes and thanks for pointing me to the thread
> that you worked on.
> > It was really helpful. please see my response inline ..
> >
> >> -----Original Message-----
> >> From: Laszlo Ersek [mailto:lersek@redhat.com]
> >> Sent: Wednesday, January 10, 2018 7:46 PM
> >> To: Pankaj Bansal <pankaj.bansal@nxp.com>; edk2-devel@lists.01.org
> >> Cc: Leif Lindholm <leif.lindholm@linaro.org>; Ard Biesheuvel
> >> <ard.biesheuvel@linaro.org>; Michael Kinney
> >> <michael.d.kinney@intel.com>
> >> Subject: Re: [edk2] [PATCH 2/2] ArmPkg/ArmArchTimerLib: Implement
> >> GetElapsedTime function of TimerLib
> >>
> >> Hello Pankaj,
> >>
> >> On 01/10/18 10:31, Pankaj Bansal wrote:
> >>> This function calculates the time elaped in Naoseconds between call
> >>> to this function and BaseTime, which is passed as argument.
> >>>
> >>> This is particularly useful in detecting timeout conditions.
> >>>
> >>> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> >>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >>> Contributed-under: TianoCore Contribution Agreement 1.1
> >>> Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
> >>>
> >>> diff --git a/ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.c
> >>> b/ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.c
> >>> index b81293c..0898339 100644
> >>> --- a/ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.c
> >>> +++ b/ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.c
> >>> @@ -290,3 +290,39 @@ GetTimeInNanoSecond (
> >>>
> >>>    return NanoSeconds;
> >>>  }
> >>> +
> >>> +/**
> >>> +  Get Elapsed time in Nanoseonds w.r.t BaseTime
> >>> +
> >>> +  This function calculates the time elaped in Naoseconds between
> >>> + call to this  function and BaseTime, which is passed as argument.
> >>> +
> >>> +  @param  BaseTime     BaseTime in NanoSeconds.
> >>> +
> >>> +  @return The elapsed time in nanoseconds.
> >>> +
> >>> +**/
> >>> +UINT64
> >>> +EFIAPI
> >>> +GetElapsedTime (
> >>> +  IN      UINT64                     BaseTime
> >>> +  )
> >>> +{
> >>> +  UINT64  NanoSeconds;
> >>> +  UINT64  Ticks;
> >>> +
> >>> +  //
> >>> +  // Get current Ticks.
> >>> +  //
> >>> +  Ticks = GetPerformanceCounter();
> >>> +
> >>> +  //
> >>> +  // Convert Ticks to Nanoseconds
> >>> +  //
> >>> +  NanoSeconds = GetTimeInNanoSecond (Ticks);
> >>> +
> >>> +  //
> >>> +  // return the difference
> >>> +  //
> >>> +  return (NanoSeconds - BaseTime);
> >>> +}
> >>>
> >>
> >> There are two problems with this patch set:
> >>
> >> (1) The TimerLib.h file (in the first patch) is a public library
> >> class header, for which several library instances (implementations)
> >> exist. So, introducing a new API requires adding an implementation
> >> (the same implementation, or different ones, as necessary) to *all*
> instances in the edk2 tree.
> >
> > I agree with your intent to prevent code duplication. But I still feel that we
> should enhance TimerLib.
> > Because, a user intending to use various timer functionalities, should
> include only one header file (TimerLib.h) and one Library.
> > Like Mike pointed out in your thread to enhance the TimerTickDiffLib,
> > in future if someone wishes to add some new functionality related to
> > timer and if he writes a new library for it, then its added cumbersome
> > for users (user has to include 3 header files and libraries 3.)
> >
> > To prevent code duplication, can we look at other ways like using weak
> symbols for such functions that are platform agnostic.
> > Or putting these generic functions in one .c file in MdePkg and include that
> file in inf file of TimerLib implementation?
> >
> >>
> >> (2) The calculation in your GetElapsedTime() function is wrong.
> >> GetTimeInNanoSecond() converts a small *difference* of ticks to time.
> >> It does not convert an absolute tick value to an absolute time.
> >
> > The GetTimeInNanoSecond() is only concerned with the ticks. Those ticks
> can be a difference of ticks or absolute ticks from counter start.
> > The *difference* part of GetElapsedTime would depend on its usage. Like
> this :
> > Start = GetElapsedTime (0);
> > // do something
> > End = GetElapsedTime (Start);
> >
> >>
> >> Furthermore, tick differences are less trivial to calculate than one
> >> might imagine, because (a) a performance counter may count down, and
> >> *independently*, (b) the numeric low bound of the counter range may
> >> not be zero.
> >>
> >
> > This is nice observation. It was an oversight on my part.
> >
> >> Earlier I proposed a new TimerTickDiffLib class (and implementation)
> >> for centralizing exactly this kind of calculation. Please see the thread at:
> >>
> >>   [edk2] TimerTickDiffLib for MdePkg?
> >>
> >>
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fmid.
> >> mail-archive.com%2F8cba2a58-1333-7733-031d-
> >>
> 0883dbd844c6%40redhat.com&data=02%7C01%7Cpankaj.bansal%40nxp.co
> >>
> m%7C17efb96f35f0456be43108d55834c721%7C686ea1d3bc2b4c6fa92cd99c5
> >>
> c301635%7C0%7C0%7C636511906063092098&sdata=AIqII9VcJEQXrJmM79G
> >> DS%2BzBu3%2BCUva%2FZESawMRndzA%3D&reserved=0
> >>
> >
> > Your library has everything that I need. Can we work on to include this in
> MdePkg ?
> > Then I won't need my changes.
> 
> (1) Please feel free to pick up my code and work on upstreaming it -- just
> please preserve my S-o-b on the patch(es) (in addition to yours), and the Red
> Hat copyright notice (in addition to your company's) in the new files.
> 
> (2) Last time I counted (reading up on the original thread), there were
> 21 TimerLib instances in edk2. Adding the same function to every one of
> them looks terrible. In edk2, I know of only two ways to avoid this:
> 
> - introduce a new library class (with class header file), and a simple library
> instance (implementation)
> 
> - collect (=move) as many as possible of the existing library instances under
> common subdirectories, so that their INF files are basically in the same
> subdirectory. Then the new code can be implemented in a new C file, under
> the same subdirectory, and then all INF files can refer to the new C file
> directly. This kind of code sharing works very well if the library instances are
> otherwise closely related. It does not work at all if the library instances live in
> separate top-level packages (which is BTW the case here).
> 
> 
> It seems that Mike wasn't opposed to introducing a TimerTickDiffLib class (or
> something similar) to MdePkg. However, more work looked necessary than I
> had expected, esp. with regard to the caching of timer hardware
> characteristics (please see the last few messages in the thread
> -- either the code has to add its own caching, or all existing, suitable TimerLib
> instances have to be verified about caching). Again, please feel free to pick
> up the code and rework it as Mike suggests -- it's basically the
> GetTickDifference() function only that adds value. Feel free to hammer it into
> any shape or form necessary.

Thank you. I will try to work on this problem of integrating these changes in multiple
TimerLib instances, when I have some room on my plate.
For now, I guess I will just add wrapper functions on top of TimerLib APIs for my Platform only.

> 
> Thanks
> Laszlo

Regards,
Pankaj Bansal

Re: [PATCH 2/2] ArmPkg/ArmArchTimerLib: Implement GetElapsedTime function of TimerLib

[Laszlo Ersek]

On 01/11/18 07:28, Pankaj Bansal wrote:
> Hi Laszlo,
> 
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Wednesday, January 10, 2018 10:34 PM
>> To: Pankaj Bansal <pankaj.bansal@nxp.com>; edk2-devel@lists.01.org
>> Cc: Leif Lindholm <leif.lindholm@linaro.org>; Ard Biesheuvel
>> <ard.biesheuvel@linaro.org>; Michael Kinney <michael.d.kinney@intel.com>
>> Subject: Re: [edk2] [PATCH 2/2] ArmPkg/ArmArchTimerLib: Implement
>> GetElapsedTime function of TimerLib

>> (1) Please feel free to pick up my code and work on upstreaming it -- just
>> please preserve my S-o-b on the patch(es) (in addition to yours), and the Red
>> Hat copyright notice (in addition to your company's) in the new files.
>>
>> (2) Last time I counted (reading up on the original thread), there were
>> 21 TimerLib instances in edk2. Adding the same function to every one of
>> them looks terrible. In edk2, I know of only two ways to avoid this:
>>
>> - introduce a new library class (with class header file), and a simple library
>> instance (implementation)
>>
>> - collect (=move) as many as possible of the existing library instances under
>> common subdirectories, so that their INF files are basically in the same
>> subdirectory. Then the new code can be implemented in a new C file, under
>> the same subdirectory, and then all INF files can refer to the new C file
>> directly. This kind of code sharing works very well if the library instances are
>> otherwise closely related. It does not work at all if the library instances live in
>> separate top-level packages (which is BTW the case here).
>>
>>
>> It seems that Mike wasn't opposed to introducing a TimerTickDiffLib class (or
>> something similar) to MdePkg. However, more work looked necessary than I
>> had expected, esp. with regard to the caching of timer hardware
>> characteristics (please see the last few messages in the thread
>> -- either the code has to add its own caching, or all existing, suitable TimerLib
>> instances have to be verified about caching). Again, please feel free to pick
>> up the code and rework it as Mike suggests -- it's basically the
>> GetTickDifference() function only that adds value. Feel free to hammer it into
>> any shape or form necessary.
> 
> Thank you. I will try to work on this problem of integrating these changes in multiple
> TimerLib instances, when I have some room on my plate.
> For now, I guess I will just add wrapper functions on top of TimerLib APIs for my Platform only.

Sure, that works as well -- feel free to scavenge the code as you see fit.

Thanks,
Laszlo