Re: [PATCH v2 2/2] drm/xe: Use the filelist from drm for ccs_mode change

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

On Tue, Oct 08, 2024 at 01:06:28PM +0530, Balasubramani Vivekanandan wrote:
> Drop the exclusive client count tracking and use the filelist from the
> drm to track the active clients. This also ensures the clients created
> internally by the driver won't block changing the ccs mode.
> 
> Fixes: ce8c161cbad4 ("drm/xe: Add ref counting for xe_file")

Is this really fixing anything. As far as I can tell nothing upstream
opens a file internally (i.e. xe_file_open) is never called directly.

> Signed-off-by: Balasubramani Vivekanandan <balasubramani.vivekanandan@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_device.c       | 10 ----------
>  drivers/gpu/drm/xe/xe_device_types.h |  9 ---------
>  drivers/gpu/drm/xe/xe_gt_ccs_mode.c  |  9 +++++----
>  3 files changed, 5 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index a33d0c772616..c6bd6888cb7b 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
> @@ -91,10 +91,6 @@ static int xe_file_open(struct drm_device *dev, struct drm_file *file)
>  	mutex_init(&xef->exec_queue.lock);
>  	xa_init_flags(&xef->exec_queue.xa, XA_FLAGS_ALLOC1);
>  
> -	spin_lock(&xe->clients.lock);
> -	xe->clients.count++;
> -	spin_unlock(&xe->clients.lock);
> -
>  	file->driver_priv = xef;
>  	kref_init(&xef->refcount);
>  
> @@ -111,17 +107,12 @@ static int xe_file_open(struct drm_device *dev, struct drm_file *file)
>  static void xe_file_destroy(struct kref *ref)
>  {
>  	struct xe_file *xef = container_of(ref, struct xe_file, refcount);
> -	struct xe_device *xe = xef->xe;
>  
>  	xa_destroy(&xef->exec_queue.xa);
>  	mutex_destroy(&xef->exec_queue.lock);
>  	xa_destroy(&xef->vm.xa);
>  	mutex_destroy(&xef->vm.lock);
>  
> -	spin_lock(&xe->clients.lock);
> -	xe->clients.count--;
> -	spin_unlock(&xe->clients.lock);
> -
>  	xe_drm_client_put(xef->client);
>  	kfree(xef->process_name);
>  	kfree(xef);
> @@ -352,7 +343,6 @@ struct xe_device *xe_device_create(struct pci_dev *pdev,
>  	xe->info.force_execlist = xe_modparam.force_execlist;
>  
>  	spin_lock_init(&xe->irq.lock);
> -	spin_lock_init(&xe->clients.lock);
>  
>  	init_waitqueue_head(&xe->ufence_wq);
>  
> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> index b97cd9828883..d4d53bb24740 100644
> --- a/drivers/gpu/drm/xe/xe_device_types.h
> +++ b/drivers/gpu/drm/xe/xe_device_types.h
> @@ -425,15 +425,6 @@ struct xe_device {
>  		struct workqueue_struct *wq;
>  	} sriov;
>  
> -	/** @clients: drm clients info */
> -	struct {
> -		/** @clients.lock: Protects drm clients info */
> -		spinlock_t lock;
> -
> -		/** @clients.count: number of drm clients */
> -		u64 count;
> -	} clients;
> -
>  	/** @usm: unified memory state */
>  	struct {
>  		/** @usm.asid: convert a ASID to VM */
> diff --git a/drivers/gpu/drm/xe/xe_gt_ccs_mode.c b/drivers/gpu/drm/xe/xe_gt_ccs_mode.c
> index 246190b3e2bb..b6adfb9f2030 100644
> --- a/drivers/gpu/drm/xe/xe_gt_ccs_mode.c
> +++ b/drivers/gpu/drm/xe/xe_gt_ccs_mode.c
> @@ -139,9 +139,10 @@ ccs_mode_store(struct device *kdev, struct device_attribute *attr,
>  	}
>  
>  	/* CCS mode can only be updated when there are no drm clients */
> -	spin_lock(&xe->clients.lock);
> -	if (xe->clients.count) {
> -		spin_unlock(&xe->clients.lock);
> +	mutex_lock(&xe->drm.filelist_mutex);

I don't particularly like this change. It is pretty poor practice to
reach into a different layer, take a lock, and look at something
internal within that layer.

Unless we have a very strong reason to this I much prefer to keep the
internal client counting.

If we start creating internal files, I rather see the vfunc 'open'
call a internal helper with a flag indicating this a an external client
and the exported function which creates an internal file calling the
helper with a flag indicating this is an internal client. The flag would
then control if 'xe->clients.count' is incremented.

Matt

> +	if (!list_empty(&xe->drm.filelist)) {
> +		mutex_unlock(&xe->drm.filelist_mutex);
> +		xe_gt_dbg(gt, "Rejecting compute mode change as there are active drm clients\n");
>  		return -EBUSY;
>  	}
>  
> @@ -152,7 +153,7 @@ ccs_mode_store(struct device *kdev, struct device_attribute *attr,
>  		xe_gt_reset_async(gt);
>  	}
>  
> -	spin_unlock(&xe->clients.lock);
> +	mutex_unlock(&xe->drm.filelist_mutex);
>  
>  	return count;
>  }
> -- 
> 2.34.1
>