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

[Pankaj Bansal <pankaj.bansal@nxp.com>]

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