Re: [PATCH v5 1/4] drm/xe/lrc: Add xe lrc ring tail function definitions

["Summers, Stuart" <stuart.summers@intel.com>]

On Mon, 2024-04-15 at 20:41 +0000, Cavitt, Jonathan wrote:
> -----Original Message-----
> From: Brost, Matthew <matthew.brost@intel.com> 
> Sent: Monday, April 15, 2024 1:29 PM
> To: Cavitt, Jonathan <jonathan.cavitt@intel.com>
> Cc: Summers, Stuart <stuart.summers@intel.com>;
> intel-xe@lists.freedesktop.org; Harrison, John C
> <john.c.harrison@intel.com>; Vishwanathapura, Niranjana
> <niranjana.vishwanathapura@intel.com>; Gupta, saurabhg
> <saurabhg.gupta@intel.com>; Roper, Matthew D
> <matthew.d.roper@intel.com>; Welty, Brian <brian.welty@intel.com>
> Subject: Re: [PATCH v5 1/4] drm/xe/lrc: Add xe lrc ring tail function
> definitions
> > 
> > On Mon, Apr 15, 2024 at 02:26:38PM -0600, Cavitt, Jonathan wrote:
> > > -----Original Message-----
> > > From: Brost, Matthew <matthew.brost@intel.com> 
> > > Sent: Monday, April 15, 2024 12:44 PM
> > > To: Summers, Stuart <stuart.summers@intel.com>
> > > Cc: intel-xe@lists.freedesktop.org; Cavitt, Jonathan
> > > <jonathan.cavitt@intel.com>; Harrison, John C
> > > <john.c.harrison@intel.com>; Vishwanathapura, Niranjana
> > > <niranjana.vishwanathapura@intel.com>; Gupta, saurabhg
> > > <saurabhg.gupta@intel.com>; Roper, Matthew D
> > > <matthew.d.roper@intel.com>; Welty, Brian <brian.welty@intel.com>
> > > Subject: Re: [PATCH v5 1/4] drm/xe/lrc: Add xe lrc ring tail
> > > function definitions
> > > > 
> > > > On Mon, Apr 15, 2024 at 01:31:32PM -0600, Summers, Stuart
> > > > wrote:
> > > > > On Mon, 2024-04-15 at 11:46 -0700, Jonathan Cavitt wrote:
> > > > > > Add defintions for the functions xe_lrc_set_ring_tail and
> > > > > > xe_lrc_ring_tail to match the definitions for
> > > > > > xe_lrc_set_ring_head
> > > > > > and
> > > > > > xe_lrc_ring_head, respectively, except tarting the lrc
> > > > > > tail.
> > > > > > 
> > > > > > Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
> > > > > > CC: Niranjana Vishwanathapura
> > > > > > <niranjana.vishwanathapura@intel.com>
> > > > > > CC: Brian Welty <brian.welty@intel.com>
> > > > > > CC: Matt Roper <matthew.d.roper@intel.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/xe/xe_lrc.c | 10 ++++++++++
> > > > > >  drivers/gpu/drm/xe/xe_lrc.h |  2 ++
> > > > > >  2 files changed, 12 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/xe/xe_lrc.c
> > > > > > b/drivers/gpu/drm/xe/xe_lrc.c
> > > > > > index 615bbc372ac62..e91ecbcceba5c 100644
> > > > > > --- a/drivers/gpu/drm/xe/xe_lrc.c
> > > > > > +++ b/drivers/gpu/drm/xe/xe_lrc.c
> > > > > > @@ -834,6 +834,16 @@ void xe_lrc_finish(struct xe_lrc *lrc)
> > > > > >         xe_bo_put(lrc->bo);
> > > > > >  }
> > > > > >  
> > > > > > +void xe_lrc_set_ring_tail(struct xe_lrc *lrc, u32 tail)
> > > > > > +{
> > > > > > +       xe_lrc_write_ctx_reg(lrc, CTX_RING_TAIL, tail);
> > > > > > +}
> > > > > > +
> > > > > > +u32 xe_lrc_ring_tail(struct xe_lrc *lrc)
> > > > > > +{
> > > > > > +       return xe_lrc_read_ctx_reg(lrc, CTX_RING_TAIL) &
> > > > > > TAIL_ADDR;
> > > > > > +}
> > > > > > +
> > > > > >  void xe_lrc_set_ring_head(struct xe_lrc *lrc, u32 head)
> > > > > >  {
> > > > > 
> > > > > I see lower in xe_lrc.c we're doing this
> > > > > (xe_lrc_snapshot_capture):
> > > > > snapshot->tail.memory = xe_lrc_read_ctx_reg(lrc,
> > > > > CTX_RING_TAIL);
> > > > > 
> > > > > Should we change this call to use the new wrapper also?
> > > > > 
> > > > 
> > > > Yea. Also xe_guc_submit.c sets the LRC tail. It should be
> > > > updated to use
> > > > the helper too.
> > > 
> > > Please list all of the places where you would like me to use
> > > xe_lrc_set_ring_tail
> > > and xe_lrc_ring_tail, and I will have those ready for the next
> > > revision.
> > > -Jonathan Cavitt
> > > 
> > 
> > grep is your friend...
> > 
> > mbrost@lstrano-desk:xe$ grep CTX_RING_TAIL *.c *.h
> > xe_execlist.c:  xe_lrc_write_ctx_reg(lrc, CTX_RING_TAIL, lrc-
> > >ring.tail);
> > xe_guc_submit.c:                xe_lrc_write_ctx_reg(lrc,
> > CTX_RING_TAIL, lrc->ring.tail);
> > xe_lrc.c:       xe_lrc_write_ctx_reg(lrc, CTX_RING_TAIL, lrc-
> > >ring.tail);
> > xe_lrc.c:       snapshot->tail.memory = xe_lrc_read_ctx_reg(lrc,
> > CTX_RING_TAIL);
> 
> And in the snapshot case, we're sure we want to use xe_lrc_ring_tail
> instead of
> the current raw call?  Because xe_lrc_ring_tail performs a bitwise
> AND on the
> result, which we might not want.
> Granted, I don't know why we wouldn't want it, but that's still a
> point of contention
> that may come up in post-review.

Honestly to me the AND there is actually making this more accurate for
the snapshot anyway - something we probably should add anyway there.
I'd suggest using the same routine everywhere.

Thanks,
Stuart

> -Jonathan Cavitt
> 
> > 
> > Matt
> > 
> > > > 
> > > > Matt
> > > > 
> > > > > Thanks,
> > > > > Stuart
> > > > > 
> > > > > >         xe_lrc_write_ctx_reg(lrc, CTX_RING_HEAD, head);
> > > > > > diff --git a/drivers/gpu/drm/xe/xe_lrc.h
> > > > > > b/drivers/gpu/drm/xe/xe_lrc.h
> > > > > > index d32fa31faa2cf..f355b0eeb5658 100644
> > > > > > --- a/drivers/gpu/drm/xe/xe_lrc.h
> > > > > > +++ b/drivers/gpu/drm/xe/xe_lrc.h
> > > > > > @@ -24,6 +24,8 @@ void xe_lrc_finish(struct xe_lrc *lrc);
> > > > > >  size_t xe_lrc_size(struct xe_device *xe, enum
> > > > > > xe_engine_class
> > > > > > class);
> > > > > >  u32 xe_lrc_pphwsp_offset(struct xe_lrc *lrc);
> > > > > >  
> > > > > > +void xe_lrc_set_ring_tail(struct xe_lrc *lrc, u32 tail);
> > > > > > +u32 xe_lrc_ring_tail(struct xe_lrc *lrc);
> > > > > >  void xe_lrc_set_ring_head(struct xe_lrc *lrc, u32 head);
> > > > > >  u32 xe_lrc_ring_head(struct xe_lrc *lrc);
> > > > > >  u32 xe_lrc_ring_space(struct xe_lrc *lrc);
> > > > > 
> > > > 
> >