[PATCH 1/3] drm/xe/oa: Replace per GT oa mutex per a global one

[Dixit, Ashutosh]

On Thu, 15 Aug 2024 09:42:57 -0700, Matthew Brost wrote:
>
> On Thu, Aug 15, 2024 at 09:27:56AM -0700, José Roberto de Souza wrote:
>
> > This mutex is not frequently used so there is no reason to have one
> > per GT,

True, but still why bother? The mutex serializes concurrent stream create
and destroy's so it cannot be stream level. So it is next higher level,
which is gt. All OA units are organized per gt, so is the lock.

> > also this reduce complexity.

How?

> >
> > Also it was missed the code to destroy the mutex.
> >
>
> drmm_mutex_init provides the destroy.

Correct.

>
> Will stay out of this as Ashutosh owns this code but in general less
> locks is better per guidelines from Sima.

To me it is basically meh, can be per device or per gt. Don't seem much
reason to change it.

Thanks.
--
Ashutosh

> > Cc: Ashutosh Dixit <ashutosh.dixit at intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_oa.c       | 13 +++++++------
> >  drivers/gpu/drm/xe/xe_oa_types.h |  5 ++---
> >  2 files changed, 9 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c
> > index 3ef92eb8fbb1e..d6eec8caf4c51 100644
> > --- a/drivers/gpu/drm/xe/xe_oa.c
> > +++ b/drivers/gpu/drm/xe/xe_oa.c
> > @@ -1205,9 +1205,9 @@ static int xe_oa_release(struct inode *inode, struct file *file)
> >	struct xe_oa_stream *stream = file->private_data;
> >	struct xe_gt *gt = stream->gt;
> >
> > -	mutex_lock(&gt->oa.gt_lock);
> > +	mutex_lock(&gt->tile->xe->oa.streams_lock);
> >	xe_oa_destroy_locked(stream);
> > -	mutex_unlock(&gt->oa.gt_lock);
> > +	mutex_unlock(&gt->tile->xe->oa.streams_lock);
> >
> >	/* Release the reference the OA stream kept on the driver */
> >	drm_dev_put(&gt_to_xe(gt)->drm);
> > @@ -1875,9 +1875,9 @@ int xe_oa_stream_open_ioctl(struct drm_device *dev, u64 data, struct drm_file *f
> >		drm_dbg(&oa->xe->drm, "Using periodic sampling freq %lld Hz\n", oa_freq_hz);
> >	}
> >
> > -	mutex_lock(&param.hwe->gt->oa.gt_lock);
> > +	mutex_lock(&xe->oa.streams_lock);
> >	ret = xe_oa_stream_open_ioctl_locked(oa, &param);
> > -	mutex_unlock(&param.hwe->gt->oa.gt_lock);
> > +	mutex_unlock(&xe->oa.streams_lock);
> >  err_exec_q:
> >	if (ret < 0 && param.exec_q)
> >		xe_exec_queue_put(param.exec_q);
> > @@ -2388,8 +2388,6 @@ static int xe_oa_init_gt(struct xe_gt *gt)
> >
> >	__xe_oa_init_oa_units(gt);
> >
> > -	drmm_mutex_init(&gt_to_xe(gt)->drm, &gt->oa.gt_lock);
> > -
> >	return 0;
> >  }
> >
> > @@ -2472,6 +2470,8 @@ int xe_oa_init(struct xe_device *xe)
> >	oa->xe = xe;
> >	oa->oa_formats = oa_formats;
> >
> > +	mutex_init(&oa->streams_lock);
> > +
> >	drmm_mutex_init(&oa->xe->drm, &oa->metrics_lock);
> >	idr_init_base(&oa->metrics_idr, 1);
> >
> > @@ -2508,5 +2508,6 @@ void xe_oa_fini(struct xe_device *xe)
> >	idr_for_each(&oa->metrics_idr, destroy_config, oa);
> >	idr_destroy(&oa->metrics_idr);
> >
> > +	mutex_destroy(&oa->streams_lock);
> >	oa->xe = NULL;
> >  }
> > diff --git a/drivers/gpu/drm/xe/xe_oa_types.h b/drivers/gpu/drm/xe/xe_oa_types.h
> > index 540c3ec53a6d7..17e17b5b93640 100644
> > --- a/drivers/gpu/drm/xe/xe_oa_types.h
> > +++ b/drivers/gpu/drm/xe/xe_oa_types.h
> > @@ -112,9 +112,6 @@ struct xe_oa_unit {
> >   * struct xe_oa_gt - OA per-gt information
> >   */
> >  struct xe_oa_gt {
> > -	/** @gt_lock: lock protecting create/destroy OA streams */
> > -	struct mutex gt_lock;
> > -
> >	/** @num_oa_units: number of oa units for each gt */
> >	u32 num_oa_units;
> >
> > @@ -149,6 +146,8 @@ struct xe_oa {
> >
> >	/** @oa_unit_ids: tracks oa unit ids assigned across gt's */
> >	u16 oa_unit_ids;
> > +
> > +	struct mutex streams_lock;
> >  };
> >
> >  /** @xe_oa_buffer: State of the stream OA buffer */
> > --
> > 2.46.0
> >