Re: [PATCH 7/9] drm/xe/rtp: (De-)whitelist OA registers for all hwe's for a gt

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

On Tue, 02 Jun 2026 15:47:48 -0700, Umesh Nerlige Ramappa wrote:
>
> On Fri, May 29, 2026 at 04:03:13PM -0700, Dixit, Ashutosh wrote:
> > On Wed, 27 May 2026 14:49:12 -0700, Umesh Nerlige Ramappa wrote:
> >>
> >
> > Hi Umesh,
> >
> >> On Mon, May 18, 2026 at 04:47:14PM -0700, Ashutosh Dixit wrote:
> >> > Whitelist or de-whitelist OA registers for all hwe's on the gt on which the
> >> > OA stream is opened. This simplifies the case where an oa unit has 0
> >> > attached hwe's (but which monitors OA events on the associated GT).
> >> >
> >> > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> >> > ---
> >> > drivers/gpu/drm/xe/xe_reg_whitelist.c | 32 ++++++++++++++++++++++++++-
> >> > drivers/gpu/drm/xe/xe_reg_whitelist.h |  4 ++++
> >> > 2 files changed, 35 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/xe/xe_reg_whitelist.c b/drivers/gpu/drm/xe/xe_reg_whitelist.c
> >> > index 0d2cf3d964a51..50b34c5008df7 100644
> >> > --- a/drivers/gpu/drm/xe/xe_reg_whitelist.c
> >> > +++ b/drivers/gpu/drm/xe/xe_reg_whitelist.c
> >> > @@ -229,7 +229,7 @@ void xe_reg_whitelist_process_engine(struct xe_hw_engine *hwe)
> >> >	whitelist_apply_to_hwe(hwe, &hwe->oa_whitelist, &hwe->reg_sr, nonpriv_slots);
> >> > }
> >> >
> >> > -__maybe_unused static void __whitelist_oa_regs(struct xe_hw_engine *hwe, bool whitelist)
> >> > +static void __whitelist_oa_regs(struct xe_hw_engine *hwe, bool whitelist)
> >> > {
> >> >	struct xe_reg_sr_entry *entry;
> >> >	unsigned long reg;
> >> > @@ -244,6 +244,36 @@ __maybe_unused static void __whitelist_oa_regs(struct xe_hw_engine *hwe, bool wh
> >> >	xe_reg_sr_apply_mmio(&hwe->oa_sr, hwe->gt);
> >> > }
> >> >
> >> > +/**
> >> > + * xe_reg_whitelist_oa_regs - whitelist oa registers for gt
> >> > + * @gt: gt to whitelist oa registers for
> >> > + *
> >> > + * Whitelist OA registers by resetting RING_FORCE_TO_NONPRIV_DENY
> >> > + */
> >> > +void xe_reg_whitelist_oa_regs(struct xe_gt *gt)
> >> > +{
> >> > +	struct xe_hw_engine *hwe;
> >> > +	enum xe_hw_engine_id id;
> >> > +
> >> > +	for_each_hw_engine(hwe, gt, id)
> >> > +		__whitelist_oa_regs(hwe, true);
> >>
> >> I think we should only apply the whitelist to the hwe that will use this
> >> specific OA unit. That will help do away with the ref counting in patch 9
> >> here.
> >
> > No, if we do this we will need to introduce per hwe refcounts (instead of
> > per gt refcounts, as done in Patch 9).
> >
> > Each OA unit has a set of attached hwe's. Then there are OA units such as
> > oam-sag and mertoa, which don't have any attached hwe's. For these we allow
> > mmio triggers to be submitted on any hwe's attached to that gt. Also some
> > hwe's like copy engines are common between OA units. Since multiple OA
> > streams might be open simultaneously, because of these reasons, we'd have
> > to introduce per hwe refcounts.
> >
> > Also, if the concern is that we are opening up all hwe's on a gt, rather
> > than hwe's attached to the OA unit on which an OA stream is opened, I do
> > not consider this a serious issue, because after all registers are being
> > whitlisted by an explicit permission from root. The root, e.g. is free to
> > load up a custom module which can whitelist whatever they want.
> >
> > So, if we want to implement whitelisting only hwe's attached to the an OA
> > unit (including OA units which don't have any hwe's attached, so would
> > include all hwe's on a gt), this is substantial increase in code
> > complexity, that will need a separate series of patches, over and above the
> > current series.
> >
> > So if we do this, that's a separate set of patches. Since the current
> > series is already a big improvement in closing down unconditional OA
> > register whitelisting, I'd say let's get this reviewed and merged first and
> > then we debate if we at all need to restrict the whitelisting further and
> > then do that if we need to, but I really don't think it is needed.
>
> I see what you are saying w.r.t. simplicity and reference tracking. At the
> same time I think it's wasteful to whitelist OAM_TRIGGER from ccs/rcs or
> and OAG register from media CS.

Given that media is a separate gt, these scenarios cannot happen.

>
> My intention is to use the NONPRIV slots sparingly. If not, we should have
> some sort of fallback for UMDs if we run out of NONPRIV slots (like provide
> a query that tells them whether whitelist is supported on an OA unit or
> not).
>
> We can discuss after this series is concluded.

Sure, but note that we didn't even use up 12 nonpriv slots, and should be
able to increase them to 20. And any such optimization/query can be
postponed till/if they are really needed.

Thanks!

> >
> >> > +}
> >> > +
> >> > +/**
> >> > + * xe_reg_dewhitelist_oa_regs - dewhitelist oa registers for gt
> >> > + * @gt: gt to dewhitelist oa registers for
> >> > + *
> >> > + * Dewhitelist OA registers by setting RING_FORCE_TO_NONPRIV_DENY
> >> > + */
> >> > +void xe_reg_dewhitelist_oa_regs(struct xe_gt *gt)
> >> > +{
> >> > +	struct xe_hw_engine *hwe;
> >> > +	enum xe_hw_engine_id id;
> >> > +
> >> > +	for_each_hw_engine(hwe, gt, id)
> >> > +		__whitelist_oa_regs(hwe, false);
> >> > +}
> >> > +
> >> > /**
> >> >  * xe_reg_whitelist_print_entry - print one whitelist entry
> >> >  * @p: DRM printer
> >> > diff --git a/drivers/gpu/drm/xe/xe_reg_whitelist.h b/drivers/gpu/drm/xe/xe_reg_whitelist.h
> >> > index 3b64b42fe96e9..e1eb1b7d5480b 100644
> >> > --- a/drivers/gpu/drm/xe/xe_reg_whitelist.h
> >> > +++ b/drivers/gpu/drm/xe/xe_reg_whitelist.h
> >> > @@ -9,12 +9,16 @@
> >> > #include <linux/types.h>
> >> >
> >> > struct drm_printer;
> >> > +struct xe_gt;
> >> > struct xe_hw_engine;
> >> > struct xe_reg_sr;
> >> > struct xe_reg_sr_entry;
> >> >
> >> > void xe_reg_whitelist_process_engine(struct xe_hw_engine *hwe);
> >> >
> >> > +void xe_reg_whitelist_oa_regs(struct xe_gt *gt);
> >> > +void xe_reg_dewhitelist_oa_regs(struct xe_gt *gt);
> >> > +
> >> > void xe_reg_whitelist_print_entry(struct drm_printer *p, unsigned int indent,
> >> >				  u32 reg, struct xe_reg_sr_entry *entry);
> >> >
> >> > --
> >> > 2.54.0
> >> >