Re: [PATCH 9/9] drm/xe/rtp: Ensure locking/ref counting for OA whitelists

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

On Wed, 27 May 2026 13:04:31 -0700, Umesh Nerlige Ramappa wrote:
>

Hi Umesh,

> On Mon, May 18, 2026 at 04:47:16PM -0700, Ashutosh Dixit wrote:
> > Since multiple OA streams might be open in parallel on a gt, ensure that
> > proper locking is in place. Also ensure that OA registers are whitelisted
> > when the first OA stream is open and de-whitelisted after the last OA
> > stream is closed.
> >
> > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> > ---
> > drivers/gpu/drm/xe/xe_oa_types.h      | 3 +++
> > drivers/gpu/drm/xe/xe_reg_whitelist.c | 9 +++++++++
> > 2 files changed, 12 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/xe/xe_oa_types.h b/drivers/gpu/drm/xe/xe_oa_types.h
> > index 3d9ec8490899c..e876e9be92ba5 100644
> > --- a/drivers/gpu/drm/xe/xe_oa_types.h
> > +++ b/drivers/gpu/drm/xe/xe_oa_types.h
> > @@ -126,6 +126,9 @@ struct xe_oa_gt {
> >
> >	/** @oa_unit: array of oa_units */
> >	struct xe_oa_unit *oa_unit;
> > +
> > +	/** @whitelist_count: number of open streams for which oa registers are whitelisted */
> > +	u32 whitelist_count;
> > };
> >
> > /**
> > diff --git a/drivers/gpu/drm/xe/xe_reg_whitelist.c b/drivers/gpu/drm/xe/xe_reg_whitelist.c
> > index 50b34c5008df7..1b72f0d2935a2 100644
> > --- a/drivers/gpu/drm/xe/xe_reg_whitelist.c
> > +++ b/drivers/gpu/drm/xe/xe_reg_whitelist.c
> > @@ -255,6 +255,10 @@ void xe_reg_whitelist_oa_regs(struct xe_gt *gt)
> >	struct xe_hw_engine *hwe;
> >	enum xe_hw_engine_id id;
> >
> > +	lockdep_assert_held(&gt->oa.gt_lock);
> > +	if (gt->oa.whitelist_count++)
> > +		return;
>
> Any reason why not using kref here?

Maybe I am wrong, but I looked into it and to me it seems kref is not
appropriate to use here. That is because:

1. A kref is first initialized to value 1 using kref_init().
2. Then subsequently you do kref_get()'s and kref_put()'s
3. Even if we have matching kref_get()'s and kref_put()'s, a final
   kref_put() is needed to bring the value down to 0 and trigger the
   release() function

Also, there is only a release() function, but nothing to trigger something
when the value goes from 0 to 1. So in this case, you'd explicitly need to
check the kref value.

Further, kref's are really useful for tracking arbitrary/asymmetrical
producers/consumers (say as part of dma_fence's), but in a strictly
symmetric situation such as what we have here, kref's don't seem to be very
useful. (See also previous use of kref in 'struct xe_oa_config' e.g.).

So here what we want is, whitelist OA registers when gt->oa.whitelist_count
goes from 0 to 1 and dewhitelist them when gt->oa.whitelist_count goes from
1 to 0. Also, we already take 'gt->oa.gt_lock' in both these cases, so even
an atomic_t refcount is not needed.

So, overall, it seems to me that if we use a kref here, it will look forced
and the code will be uglier than what we have here.

Let me know what you think about this, but for now I am tending to leaving
this as is.

Thanks.
--
Ashutosh

> > +
> >	for_each_hw_engine(hwe, gt, id)
> >		__whitelist_oa_regs(hwe, true);
> > }
> > @@ -270,6 +274,11 @@ void xe_reg_dewhitelist_oa_regs(struct xe_gt *gt)
> >	struct xe_hw_engine *hwe;
> >	enum xe_hw_engine_id id;
> >
> > +	lockdep_assert_held(&gt->oa.gt_lock);
> > +	xe_assert(gt_to_xe(gt), gt->oa.whitelist_count);
> > +	if (--gt->oa.whitelist_count)
> > +		return;
> > +
> >	for_each_hw_engine(hwe, gt, id)
> >		__whitelist_oa_regs(hwe, false);
> > }
> > --
> > 2.54.0
> >