[PATCH 2/7] drm/xe: Add ring address to LRC snapshot

Tue Nov 12 20:30:06 UTC 2024

On Tue, Nov 12, 2024 at 12:16:46PM -0800, Cavitt, Jonathan wrote:
> -----Original Message-----
> From: Brost, Matthew <matthew.brost at intel.com> 
> Sent: Tuesday, November 12, 2024 10:18 AM
> To: Harrison, John C <john.c.harrison at intel.com>
> Cc: Cavitt, Jonathan <jonathan.cavitt at intel.com>; intel-xe at lists.freedesktop.org; Teres Alexis, Alan Previn <alan.previn.teres.alexis at intel.com>; Dong, Zhanjun <zhanjun.dong at intel.com>; Vivi, Rodrigo <rodrigo.vivi at intel.com>
> Subject: Re: [PATCH 2/7] drm/xe: Add ring address to LRC snapshot
> > 
> > On Tue, Nov 12, 2024 at 09:59:16AM -0800, John Harrison wrote:
> > > On 11/8/2024 15:34, Cavitt, Jonathan wrote:
> > > > -----Original Message-----
> > > > From: Brost, Matthew <matthew.brost at intel.com>
> > > > Sent: Friday, November 8, 2024 3:10 PM
> > > > To: Cavitt, Jonathan <jonathan.cavitt at intel.com>
> > > > Cc: intel-xe at lists.freedesktop.org; Teres Alexis, Alan Previn <alan.previn.teres.alexis at intel.com>; Dong, Zhanjun <zhanjun.dong at intel.com>; Vivi, Rodrigo <rodrigo.vivi at intel.com>
> > > > Subject: Re: [PATCH 2/7] drm/xe: Add ring address to LRC snapshot
> > > > > On Fri, Nov 08, 2024 at 03:05:34PM -0700, Cavitt, Jonathan wrote:
> > > > > > -----Original Message-----
> > > > > > From: Intel-xe <intel-xe-bounces at lists.freedesktop.org> On Behalf Of Matthew Brost
> > > > > > Sent: Friday, November 8, 2024 9:43 AM
> > > > > > To: intel-xe at lists.freedesktop.org
> > > > > > Cc: Teres Alexis, Alan Previn <alan.previn.teres.alexis at intel.com>; Dong, Zhanjun <zhanjun.dong at intel.com>; Vivi, Rodrigo <rodrigo.vivi at intel.com>
> > > > > > Subject: [PATCH 2/7] drm/xe: Add ring address to LRC snapshot
> > > > > > > The ring is currently in LRC BO but this may change going forward.
> > > > > > > Include the ring address in the snapshot protecting again any future
> > > > > > > changes.
> > > > > > > 
> > > > > > > Signed-off-by: Matthew Brost <matthew.brost at intel.com>
> > > > > > LGTM, though the terminology we're using to describe the various ggtt addresses
> > > > > > as "descriptors" is a bit confusing, even if it's consistent.  I wonder where that
> > > > > > terminology came from?
> > > > > > This is just a rhetorical question.  I'm not suggesting it be changed.
> > > > > > 
> > > > > LRC descriptors is name copied over from i915 and may even be in the bspec.
> > > > > 
> > > > > But yea, indirect_state_desc and ring_desc are bad names. Will change to
> > > > > 'ring_addr' here.
> > > > IMO it would probably be better to leave it as "ring_desc" for now as it's
> > > > consistent with surrounding struct members.  We can do a pass of the full
> > > > XE kernel for inaccurate uses of the "desc" qualifier in the near future and
> > > > fix the naming scheme here as a part of that fixup.
> > > I strongly disagree. Just because A is broken doesn't mean we should add a
> > > broken B and C! It makes no sense to add a name we know is bad just so that
> > > we can change it later. Anything new should be done properly from the start.
> > > 
> > 
> > I agree with John here.
> 
> Okay, I submitted a patch to change the name from context_desc to context_addr,
> and it was rejected because context_desc is actually correct.  But if context_desc
> is correct, then would ring_desc *also* be correct?
> 
> Pinging @Roper, Matthew D
> -Jonathan Cavitt

As I mentioned on the other thread, "context descriptor" is a formal
hardware term that has a very specific meaning --- it's the 64-bit value
that gets programmed into the ELSP.  The context descriptor contains the
various things inside of it, including the LRCA.  It's documented at
bspec 60419.

The problem today is that the capture code is using "context descriptor"
as a name for something else.  We need to decide what the capture code
should really be doing.  If we want to capture the true descriptor, as
used by the hardware, then we should fix the capture code to record and
dump the proper value.  If we only want to capture something else (e.g.,
the LRCA), then we should rename the fields for what they actually
contain.  But we definitely don't want patches making statements like
"context descriptor is a legacy i915 term" since that's just not true
and will confuse people who read this in the git history for years to
come.

Matt

> 
> > 
> > Matt
> > 
> > > John.
> > > 
> > > > 
> > > > My RB still stands either way.
> > > > -Jonathan Cavitt
> > > > 
> > > > > Matt
> > > > > 
> > > > > > Reviewed-by: Jonathan Cavitt <jonathan.cavitt at intel.com>
> > > > > > -Jonathan Cavitt
> > > > > > 
> > > > > > > ---
> > > > > > >   drivers/gpu/drm/xe/xe_lrc.c | 3 +++
> > > > > > >   drivers/gpu/drm/xe/xe_lrc.h | 1 +
> > > > > > >   2 files changed, 4 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/xe/xe_lrc.c b/drivers/gpu/drm/xe/xe_lrc.c
> > > > > > > index e219657535cf..afb0f4f44748 100644
> > > > > > > --- a/drivers/gpu/drm/xe/xe_lrc.c
> > > > > > > +++ b/drivers/gpu/drm/xe/xe_lrc.c
> > > > > > > @@ -1636,6 +1636,7 @@ struct xe_lrc_snapshot *xe_lrc_snapshot_capture(struct xe_lrc *lrc)
> > > > > > >   		xe_vm_get(lrc->bo->vm);
> > > > > > >   	snapshot->context_desc = xe_lrc_ggtt_addr(lrc);
> > > > > > > +	snapshot->ring_desc = __xe_lrc_ring_ggtt_addr(lrc);
> > > > > > >   	snapshot->indirect_context_desc = xe_lrc_indirect_ring_ggtt_addr(lrc);
> > > > > > >   	snapshot->head = xe_lrc_ring_head(lrc);
> > > > > > >   	snapshot->tail.internal = lrc->ring.tail;
> > > > > > > @@ -1693,6 +1694,8 @@ void xe_lrc_snapshot_print(struct xe_lrc_snapshot *snapshot, struct drm_printer
> > > > > > >   		return;
> > > > > > >   	drm_printf(p, "\tHW Context Desc: 0x%08x\n", snapshot->context_desc);
> > > > > > > +	drm_printf(p, "\tHW Ring: 0x%08x\n",
> > > > > > > +		   snapshot->ring_desc);
> > > > > > >   	drm_printf(p, "\tHW Indirect Ring State: 0x%08x\n",
> > > > > > >   		   snapshot->indirect_context_desc);
> > > > > > >   	drm_printf(p, "\tLRC Head: (memory) %u\n", snapshot->head);
> > > > > > > diff --git a/drivers/gpu/drm/xe/xe_lrc.h b/drivers/gpu/drm/xe/xe_lrc.h
> > > > > > > index 9d64cedc4d14..a2058a501353 100644
> > > > > > > --- a/drivers/gpu/drm/xe/xe_lrc.h
> > > > > > > +++ b/drivers/gpu/drm/xe/xe_lrc.h
> > > > > > > @@ -25,6 +25,7 @@ struct xe_lrc_snapshot {
> > > > > > >   	unsigned long lrc_size, lrc_offset;
> > > > > > >   	u32 context_desc;
> > > > > > > +	u32 ring_desc;
> > > > > > >   	u32 indirect_context_desc;
> > > > > > >   	u32 head;
> > > > > > >   	struct {
> > > > > > > -- 
> > > > > > > 2.34.1
> > > > > > > 
> > > > > > > 
> > > 
> > 

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