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

[Rodrigo Vivi]

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 at 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 at intel.com>
> > > > > > Signed-off-by: Francois Dugast <francois.dugast at intel.com>
> > > > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi at 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.

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