[Intel-xe] [RFC v3 2/2] drm/xe: Avoid any races around ccs_mode update

[Welty, Brian]

On 11/13/2023 4:46 PM, Niranjana Vishwanathapura wrote:
> Ensure that there are no drm clients when changing CCS mode.
> Allow exec_queue creation only with enabled CCS engines.
> 
> Signed-off-by: Niranjana Vishwanathapura <niranjana.vishwanathapura at intel.com>
> ---
>   drivers/gpu/drm/xe/xe_device.c       | 9 +++++++++
>   drivers/gpu/drm/xe/xe_device_types.h | 9 +++++++++
>   drivers/gpu/drm/xe/xe_gt_sysfs.c     | 9 +++++++++
>   3 files changed, 27 insertions(+)
> 
> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index 1202f8007f79..27e6f0bf423e 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
> @@ -73,6 +73,10 @@ 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->ccs_mode.lock);
> +	xe->ccs_mode.num_drm_clients++;
> +	spin_unlock(&xe->ccs_mode.lock);
> +
>   	file->driver_priv = xef;
>   	return 0;
>   }
> @@ -105,6 +109,10 @@ static void xe_file_close(struct drm_device *dev, struct drm_file *file)
>   	xa_destroy(&xef->vm.xa);
>   	mutex_destroy(&xef->vm.lock);
>   
> +	spin_lock(&xe->ccs_mode.lock);
> +	xe->ccs_mode.num_drm_clients--;
> +	spin_unlock(&xe->ccs_mode.lock);
> +
>   	xe_drm_client_put(xef->client);
>   	kfree(xef);
>   }
> @@ -228,6 +236,7 @@ struct xe_device *xe_device_create(struct pci_dev *pdev,
>   	xe->info.force_execlist = force_execlist;
>   
>   	spin_lock_init(&xe->irq.lock);
> +	spin_lock_init(&xe->ccs_mode.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 be11cadccbd4..3382281146aa 100644
> --- a/drivers/gpu/drm/xe/xe_device_types.h
> +++ b/drivers/gpu/drm/xe/xe_device_types.h
> @@ -297,6 +297,15 @@ struct xe_device {
>   		struct ttm_resource_manager sys_mgr;
>   	} mem;
>   
> +	/** @ccs_mode: ccs mode */
> +	struct {
> +		/** @lock: Ensure there are no clients while setting ccs_mode */
> +		spinlock_t lock;
> +
> +		/** @num_drm_clients: number of drm clients */
> +		u64 num_drm_clients;
> +	} ccs_mode;

I think future features also want to track drm_clients?
Such as EU debugger: 
https://lists.freedesktop.org/archives/intel-xe/2023-November/017903.html
Above proposes xarray (or linked list) of drm clients.

Maybe rename above to 'clients' to match the sub-structure that EU 
debugger will introduce later?   Then they don't miss this and they can 
remove the num_drm_clients in favor of testing if list is empty?

-Brian


> +
>   	/** @usm: unified memory state */
>   	struct {
>   		/** @asid: convert a ASID to VM */
> diff --git a/drivers/gpu/drm/xe/xe_gt_sysfs.c b/drivers/gpu/drm/xe/xe_gt_sysfs.c
> index e53221c2894f..c4779efd65b4 100644
> --- a/drivers/gpu/drm/xe/xe_gt_sysfs.c
> +++ b/drivers/gpu/drm/xe/xe_gt_sysfs.c
> @@ -48,6 +48,7 @@ ccs_mode_store(struct device *kdev, struct device_attribute *attr,
>   	       const char *buff, size_t count)
>   {
>   	struct xe_gt *gt = kobj_to_gt(&kdev->kobj);
> +	struct xe_device *xe = gt_to_xe(gt);
>   	u32 num_engines, num_slices;
>   	int ret;
>   
> @@ -66,12 +67,20 @@ ccs_mode_store(struct device *kdev, struct device_attribute *attr,
>   		return -EINVAL;
>   	}
>   
> +	spin_lock(&xe->ccs_mode.lock);
> +	if (xe->ccs_mode.num_drm_clients) {
> +		spin_unlock(&xe->ccs_mode.lock);
> +		return -EBUSY;
> +	}
> +
>   	if (gt->ccs.num_engines != num_engines) {
>   		xe_gt_info(gt, "Setting compute mode to %d\n", num_engines);
>   		gt->ccs.num_engines = num_engines;
>   		xe_gt_reset_async(gt);
>   	}
>   
> +	spin_unlock(&xe->ccs_mode.lock);
> +
>   	return count;
>   }
>