Re: [Intel-xe] [PATCH v2 03/20] drm/xe: Correlate engine and cpu timestamps with better accuracy

[Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>]

On Tue, Sep 26, 2023 at 05:42:29PM -0400, Rodrigo Vivi wrote:
>On Tue, Sep 26, 2023 at 12:52:10PM -0700, Umesh Nerlige Ramappa wrote:
>> On Tue, Sep 26, 2023 at 03:07:20PM -0400, Rodrigo Vivi wrote:
>> > On Tue, Sep 26, 2023 at 11:58:58AM -0700, Umesh Nerlige Ramappa wrote:
>> > > On Tue, Sep 26, 2023 at 02:48:13PM -0400, Rodrigo Vivi wrote:
>> > > > On Mon, Sep 25, 2023 at 06:37:12PM -0700, Umesh Nerlige Ramappa wrote:
>> > > > > On Wed, Sep 20, 2023 at 03:29:23PM -0400, Rodrigo Vivi wrote:
>> > > > > > From: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
>> > > > > >
>> > > > > > Perf measurements rely on CPU and engine timestamps to correlate
>> > > > > > events of interest across these time domains. Current mechanisms get
>> > > > > > these timestamps separately and the calculated delta between these
>> > > > > > timestamps lack enough accuracy.
>> > > > > >
>> > > > > > To improve the accuracy of these time measurements to within a few us,
>> > > > > > add a query that returns the engine and cpu timestamps captured as
>> > > > > > close to each other as possible.
>> > > > > >
>> > > > > > Prior work: https://patchwork.freedesktop.org/series/87552/
>> > > > > >
>> > > > > > Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
>> > > > > > Signed-off-by: Francois Dugast <francois.dugast@intel.com>
>> > > > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> > > > > > ---
>> > > > >
>> > > > > Should already have an R-b from Jose based on this -
>> > > > > https://patchwork.freedesktop.org/patch/552682/?series=122440&rev=1
>> > > >
>> > > > While incorporating that and fixing IGT, I noticed the inconsistency
>> > > > present on the v2 that was not part of v1.
>> > > >
>> > > > Why we have the struct name as engine_cycles now but the query itself
>> > > > is still cs_cycles? Which one is correct? why do we need both and
>> > > > cannot align in a single name?
>> > >
>> > > Jose had commented that XE does not have the concept of CS and asked for a
>> > > rename to engine. In the latest revision of this series, I had replaces cs
>> > > with engine everywhere.
>> > >
>> > > We should use engine.
>> > >
>> > > Latest series - https://patchwork.freedesktop.org/series/122440/
>> >
>> > yeap, I got from there...
>> > I still see
>> > +#define DRM_XE_QUERY_CS_CYCLES		6
>> > there
>> > ;)
>> >
>> > I can do the renaming locally if this is the right thing to do...
>>
>> oh, I think I missed that. It should be changed to
>> DRM_XE_QUERY_ENGINE_CYCLES.
>
>while finishing the rename here and looking to this query, I got
>myself thinking, why isn't this already doing the gpu_timestamp
>calculation and showing that directly instead of sending the cycles
>and the crystal clock reference for umd to calculate?
>
>the gpu_timestamp directly would align better with the other fields
>in the struct. and by itself already avoid the gt reference clock
>duplication in the uapi.

Depends on what the user is using the GPU time/ticks for. In OA, the 
user space tool compares the cs_cycles directly to the timestamp field 
in the OA reports (which is also in cycles). Providing both ref clock 
and cycles gives the user flexibility to use it in different scenarios.

Thanks,
Umesh


>
>>
>> Thanks,
>> Umesh
>> >
>> > >
>> > > Thanks,
>> > > Umesh
>> > > >
>> > > > >
>> > > > > Thanks,
>> > > > > Umesh