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

[Matthew Brost <matthew.brost@intel.com>]

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, also this reduce complexity.
> 
> Also it was missed the code to destroy the mutex.
> 

drmm_mutex_init provides the destroy.

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

Matt

> Cc: Ashutosh Dixit <ashutosh.dixit@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@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
>