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

Tue Nov 12 21:21:46 UTC 2024

-----Original Message-----
> From: Vivi, Rodrigo <rodrigo.vivi at intel.com> 
> Sent: Tuesday, November 12, 2024 12:46 PM
> To: Roper, Matthew D <matthew.d.roper at intel.com>
> Cc: Cavitt, Jonathan <jonathan.cavitt at intel.com>; Brost, Matthew <matthew.brost at intel.com>; Harrison, John C <john.c.harrison 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>
> Subject: Re: [PATCH 2/7] drm/xe: Add ring address to LRC snapshot
> 
> On Tue, Nov 12, 2024 at 12:30:06PM -0800, Matt Roper wrote:
> > 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.
> 
> I liked the variables named after address better. But I totally agree
> with Matt Roper here. Plus we cannot break userspace. There are tools
> out there already parsing 'Desc'.

Okay... Let's bring this back full circle.
Should the new variable for this patch be called "ring_addr" or "ring_desc"?
Ideally, it should be named after whatever the return value for __xe_lrc_ring_ggtt_addr
is, and on paper that should be a ring address, but the function xe_lrc_ggtt_addr
returns a context descriptor, and the function xe_lrc_indirect_ring_ggtt_addr
apparently doesn't return an address or a descriptor, so I just don't know any more.
-Jonathan Cavitt

> 
> > 
> > 
> > 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
>