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

[Niranjana Vishwanathapura]

On Tue, Nov 14, 2023 at 12:10:23PM -0800, Welty, Brian wrote:
>
>
>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.
>

Thanks Brian. I see..

>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?
>

Sure, will rename the sturct as 'clients'.

Niranjana

>-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;
>>  }