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

Thanks.
--
Ashutosh

> > +}
> > +
> > +/**
> > + * 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
> >