Re: [PATCH 1/2] drm/xe/oa: Rename last argument of WHITELIST_OA_MMIO_TRG

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Wed, 03 Jun 2026 14:19:30 -0700, Dixit, Ashutosh wrote:
>
> On Wed, 03 Jun 2026 13:21:35 -0700, Dixit, Ashutosh wrote:
> >
> > On Wed, 03 Jun 2026 12:09:59 -0700, Umesh Nerlige Ramappa wrote:
> > >
> > > On Tue, Jun 02, 2026 at 04:09:07PM -0700, Ashutosh Dixit wrote:
> > > > OA head pointer registers are not used by UMD's and do not need to be
> > > > whitelisted, the last argument of WHITELIST_OA_MMIO_TRG is actually used
> > > > for whitelisting tail pointer and OA buffer registers. Rename the argument
> > > > to tail_buf to highlight this. OA head pointer is sometimes provided to the
> > > > WHITELIST_OA_MMIO_TRG to have the correct register offset alignment (16)
> > > > for RING_FORCE_TO_NONPRIV_RANGE_4.
> > > >
> > > > Fixes: ed455775c5a6 ("drm/xe/rtp: Refactor OAG MMIO trigger register whitelisting")
> > > > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> > > > ---
> > > > drivers/gpu/drm/xe/xe_reg_whitelist.c | 4 ++--
> > > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/xe/xe_reg_whitelist.c b/drivers/gpu/drm/xe/xe_reg_whitelist.c
> > > > index 2e84b1c49f374..a17ebacc1455b 100644
> > > > --- a/drivers/gpu/drm/xe/xe_reg_whitelist.c
> > > > +++ b/drivers/gpu/drm/xe/xe_reg_whitelist.c
> > > > @@ -104,10 +104,10 @@ static const struct xe_rtp_table_sr register_whitelist = XE_RTP_TABLE_SR(
> > > >				   RING_FORCE_TO_NONPRIV_ACCESS_RW))
> > > >	},
> > > >
> > > > -#define WHITELIST_OA_MMIO_TRG(trg, status, head) \
> > > > +#define WHITELIST_OA_MMIO_TRG(trg, status, tail_buf) \
> > > >	WHITELIST(trg, RING_FORCE_TO_NONPRIV_ACCESS_RW), \
> > > >	WHITELIST(status, RING_FORCE_TO_NONPRIV_ACCESS_RD), \
> > > > -	WHITELIST(head, RING_FORCE_TO_NONPRIV_ACCESS_RD | RING_FORCE_TO_NONPRIV_RANGE_4)
> > > > +	WHITELIST(tail_buf, RING_FORCE_TO_NONPRIV_ACCESS_RD | RING_FORCE_TO_NONPRIV_RANGE_4)
> > >
> > > I think this would be a good time to split the regs and do away with the
> > > RANGE_4.  If HEAD and STATUS are not used, then we should just use 2 slots
> > > - one for OA_TAIL and one for OA_BUFFER (single reg mode). Any idea if
> > > STATUS is needed? I thought the KMD already returns the status in an IOCTL.
> >
> > OK, yes I think OASTATUS doesn't need to be whitelisted and we can also do
> > away with RANGE_4.
>
> Actually, we can save one slot by retaining RANGE_4. So isn't it better to
> retain it? Since the whitlisting will anyway be gated by paranoid.
>
> Since there is a multiplicative effect: say OAM command streamers will
> whitelist registers for all 3 OAM units, so there is a x3 multiplier. With
> RANGE_4 we will use up 6 slots and without RANGE_4 we will use up 9 slots.
>
> So just thought I'll ask about this too. Thanks.

Hmm, looking at the code, we return OASTATUS correctly through the ioctl
only when the read() interface is used. If only mmap() interface interface
is used, currently the ioctl will always return OASTATUS as 0.

Let me see if the code can be changed to return OASTATUS correctly through
the ioctl, even when only mmap() interface is used. Because whitelisted
OASTATUS is currently not used by UMD's, I think it should be ok to switch
getting OASTATUS through the ioctl.

> > But can we review and merge Patch 2 for now, since that is a bug fix and
> > will need to be propagated also to previous kernel versions. So if we can
> > merge that first and then make these changes, it will simplify life quite a
> > bit.
> >
> > Thanks.
> > --
> > Ashutosh
> >
> >
> > >
> > > >
> > > > #define WHITELIST_OAG_MMIO_TRG \
> > > >	WHITELIST_OA_MMIO_TRG(OAG_MMIOTRIGGER, OAG_OASTATUS, OAG_OAHEADPTR)
> > > > --
> > > > 2.54.0
> > > >