[PATCH 1/3] drm/xe/query: Increase timestamp width

[Matt Roper]

On Thu, Oct 10, 2024 at 01:28:38PM -0500, Lucas De Marchi wrote:
> On Thu, Oct 10, 2024 at 10:10:51AM -0700, Matt Roper wrote:
> > On Wed, Oct 09, 2024 at 11:08:12PM -0500, Lucas De Marchi wrote:
> > > On Wed, Oct 09, 2024 at 08:48:45PM -0700, Lucas De Marchi wrote:
> > > > Starting with Xe2 the timestamp is a full 64 bit counter, contrary to
> > > > the 36 bit that was available before. Although 36 should be sufficient
> > > > for any reasonable delta calculation (for Xe2, of about 30min), it's
> > > > surprising to userspace to get sommething truncated. Also if the
> > > > timestamp being compared to is coming from the GPU and the application
> > > > is not careful enough to apply the width there, a delta calculation
> > > > would be wrong.
> > > >
> > > > Extend it to full 64-bits for non-media engines starting with Xe2.
> > > >
> > > > Bspec: 60411
> > > > Cc: Szymon Morek <szymon.morek at intel.com>
> > > > Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
> > > > ---
> > > > drivers/gpu/drm/xe/xe_query.c | 6 +++++-
> > > > 1 file changed, 5 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/xe/xe_query.c b/drivers/gpu/drm/xe/xe_query.c
> > > > index 158629971eab3..a1f4cc25bea68 100644
> > > > --- a/drivers/gpu/drm/xe/xe_query.c
> > > > +++ b/drivers/gpu/drm/xe/xe_query.c
> > > > @@ -164,7 +164,11 @@ query_engine_cycles(struct xe_device *xe,
> > > > 			  cpu_clock);
> > > >
> > > > 	xe_force_wake_put(gt_to_fw(gt), XE_FORCEWAKE_ALL);
> > > > -	resp.width = 36;
> > > > +
> > > > +	if (!xe_gt_is_media_type(gt) && GRAPHICS_VER(xe) >= 20)
> > > 
> > > I sent this to follow the spec to the letter, but I'm thinking the spec
> > > is not tagged correctly here.... the counter is the same, replicated to
> > > all engines. It doesn't make sense to be different in the media gt.
> > 
> > Yeah, when REMOVEDBY() tags are used against a top-level IP family
> > (top-level Xe2 in this case) rather than a specific release, the bspec
> > output often gets confused and leaves behind some stray markings like
> > "XE2_M" that aren't actually true.  I don't think those marks are
> > actually used so they don't have the proper IP inheritance rules
> > defined, which is why they still show up where they shouldn't.
> > 
> > tldr:  I think the 64-bit width here should apply to both the primary
> > and media GT from Xe2 onward.
> 
> thanks for confirming.  Another thing, should we mask the value to the
> width or just rely on reserved fields returning 0?
> 
> one problem with relying on them to be 0 is that we could end up returning
> more bits read from the HW than we report in the width. Example from LNL
> without this patch:
> 
> (xe_query:5738) DEBUG: [1] cpu_ts before 4549113780840, reg read time 2694
> (xe_query:5738) DEBUG: [1] engine_ts 87847861517, freq 19200000 Hz, width 36
> (xe_query:5738) DEBUG: [2] cpu_ts before 4549113801981, reg read time 2615
> (xe_query:5738) DEBUG: [2] engine_ts 87847861922, freq 19200000 Hz, width 36
> (xe_query:5738) DEBUG: delta_cpu[21141], delta_cs[21093]
> (xe_query:5738) DEBUG: delta_delta 48
> 
> That is not a problem anymore after this patch since we went straight
> from 36 to 64 and all the previous platforms do return 0 on those bits.
> So maybe just leave this alone. Agreed?

Yeah, reserved register bits should return 0.  Since it sounds like
that's working properly on the older platforms it should be fine to
leverage that.


Matt

> 
> thanks
> Lucas De Marchi

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation