diff mbox series

[7/7] accel/qaic: Add QAIC_DETACH_SLICE_BO IOCTL

Message ID 20230901172247.11410-8-quic_jhugo@quicinc.com (mailing list archive)
State New, archived
Headers show
Series accel/qaic: Extend uAPI to support undoing ATTACH_SLICE_BO | expand

Commit Message

Jeffrey Hugo Sept. 1, 2023, 5:22 p.m. UTC
From: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>

Once a BO is attached with slicing configuration that BO can only be used
for that particular setting. With this new feature user can detach slicing
configuration off an already sliced BO and attach new slicing configuration
using QAIC_ATTACH_SLICE_BO.

This will support BO recycling.

detach_slice_bo() detaches slicing configuration from a BO. This new
helper function can also be used in release_dbc() as we are doing the
exact same thing.

Signed-off-by: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>
Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
[jhugo: add documentation for new ioctl]
Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
---
 Documentation/accel/qaic/qaic.rst |  10 +++
 drivers/accel/qaic/qaic.h         |   4 +-
 drivers/accel/qaic/qaic_data.c    | 119 +++++++++++++++++++++++++++---
 drivers/accel/qaic/qaic_drv.c     |   1 +
 include/uapi/drm/qaic_accel.h     |  12 +++
 5 files changed, 135 insertions(+), 11 deletions(-)

Comments

Stanislaw Gruszka Sept. 17, 2023, 8:56 a.m. UTC | #1
On Fri, Sep 01, 2023 at 11:22:47AM -0600, Jeffrey Hugo wrote:
> From: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>
> 
> Once a BO is attached with slicing configuration that BO can only be used
> for that particular setting. With this new feature user can detach slicing
> configuration off an already sliced BO and attach new slicing configuration
> using QAIC_ATTACH_SLICE_BO.
> 
> This will support BO recycling.
> 
> detach_slice_bo() detaches slicing configuration from a BO. This new
> helper function can also be used in release_dbc() as we are doing the
> exact same thing.
> 
> Signed-off-by: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>
> Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
> [jhugo: add documentation for new ioctl]
> Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
<snip>

> +	/* Check if BO is committed to H/W for DMA */
> +	spin_lock_irqsave(&dbc->xfer_lock, flags);
> +	if (bo->queued) {
> +		spin_unlock_irqrestore(&dbc->xfer_lock, flags);
> +		ret = -EBUSY;
> +		goto unlock_ch_srcu;
> +	}
> +	spin_unlock_irqrestore(&dbc->xfer_lock, flags);

This looks like race condition. If some other thread will take the xfer_lock
and set bo->queued (HERE just after _unlock())  we will not return -EBUSY.
Something seems to be missing here or xfer_lock is not needed to protect
bo->queued.

Regards
Stanislaw
Jeffrey Hugo Sept. 22, 2023, 3:16 p.m. UTC | #2
On 9/17/2023 2:56 AM, Stanislaw Gruszka wrote:
> On Fri, Sep 01, 2023 at 11:22:47AM -0600, Jeffrey Hugo wrote:
>> From: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>
>>
>> Once a BO is attached with slicing configuration that BO can only be used
>> for that particular setting. With this new feature user can detach slicing
>> configuration off an already sliced BO and attach new slicing configuration
>> using QAIC_ATTACH_SLICE_BO.
>>
>> This will support BO recycling.
>>
>> detach_slice_bo() detaches slicing configuration from a BO. This new
>> helper function can also be used in release_dbc() as we are doing the
>> exact same thing.
>>
>> Signed-off-by: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>
>> Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
>> [jhugo: add documentation for new ioctl]
>> Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
> <snip>
> 
>> +	/* Check if BO is committed to H/W for DMA */
>> +	spin_lock_irqsave(&dbc->xfer_lock, flags);
>> +	if (bo->queued) {
>> +		spin_unlock_irqrestore(&dbc->xfer_lock, flags);
>> +		ret = -EBUSY;
>> +		goto unlock_ch_srcu;
>> +	}
>> +	spin_unlock_irqrestore(&dbc->xfer_lock, flags);
> 
> This looks like race condition. If some other thread will take the xfer_lock
> and set bo->queued (HERE just after _unlock())  we will not return -EBUSY.
> Something seems to be missing here or xfer_lock is not needed to protect
> bo->queued.

The other thread would also need to take the bo->lock, which is held 
here and not released until after detach_slice_bo().  xfer_lock actually 
protects xfer_list, but bo->queued is a quick check to see if the bo is 
in the list, rather than iterating the list.  I can see how this is 
misleading.  I will ponder how to improve it.

-Jeff
diff mbox series

Patch

diff --git a/Documentation/accel/qaic/qaic.rst b/Documentation/accel/qaic/qaic.rst
index 72a70ab6e3a8..c88502383136 100644
--- a/Documentation/accel/qaic/qaic.rst
+++ b/Documentation/accel/qaic/qaic.rst
@@ -123,6 +123,16 @@  DRM_IOCTL_QAIC_PART_DEV
   AIC100 device and can be used for limiting a process to some subset of
   resources.
 
+DRM_IOCTL_QAIC_DETACH_SLICE_BO
+  This IOCTL allows userspace to remove the slicing information from a BO that
+  was originally provided by a call to DRM_IOCTL_QAIC_ATTACH_SLICE_BO. This
+  is the inverse of DRM_IOCTL_QAIC_ATTACH_SLICE_BO. The BO must be idle for
+  DRM_IOCTL_QAIC_DETACH_SLICE_BO to be called. After a successful detach slice
+  operation the BO may have new slicing information attached with a new call
+  to DRM_IOCTL_QAIC_ATTACH_SLICE_BO. After detach slice, the BO cannot be
+  executed until after a new attach slice operation. Combining attach slice
+  and detach slice calls allows userspace to use a BO with multiple workloads.
+
 Userspace Client Isolation
 ==========================
 
diff --git a/drivers/accel/qaic/qaic.h b/drivers/accel/qaic/qaic.h
index 27cf66dbd5a5..28f1e81a1465 100644
--- a/drivers/accel/qaic/qaic.h
+++ b/drivers/accel/qaic/qaic.h
@@ -219,7 +219,8 @@  struct qaic_bo {
 		 */
 		u32		queue_level_before;
 	} perf_stats;
-
+	/* Synchronizes BO operations */
+	struct mutex		lock;
 };
 
 struct bo_slice {
@@ -275,6 +276,7 @@  int qaic_execute_bo_ioctl(struct drm_device *dev, void *data, struct drm_file *f
 int qaic_partial_execute_bo_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv);
 int qaic_wait_bo_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv);
 int qaic_perf_stats_bo_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv);
+int qaic_detach_slice_bo_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv);
 void irq_polling_work(struct work_struct *work);
 
 #endif /* _QAIC_H_ */
diff --git a/drivers/accel/qaic/qaic_data.c b/drivers/accel/qaic/qaic_data.c
index 2acb9dbac88b..c90fa6a430f6 100644
--- a/drivers/accel/qaic/qaic_data.c
+++ b/drivers/accel/qaic/qaic_data.c
@@ -624,6 +624,7 @@  static void qaic_free_object(struct drm_gem_object *obj)
 		qaic_free_sgt(bo->sgt);
 	}
 
+	mutex_destroy(&bo->lock);
 	drm_gem_object_release(obj);
 	kfree(bo);
 }
@@ -641,6 +642,7 @@  static void qaic_init_bo(struct qaic_bo *bo, bool reinit)
 		bo->sliced = false;
 		reinit_completion(&bo->xfer_done);
 	} else {
+		mutex_init(&bo->lock);
 		init_completion(&bo->xfer_done);
 	}
 	complete_all(&bo->xfer_done);
@@ -1002,10 +1004,13 @@  int qaic_attach_slice_bo_ioctl(struct drm_device *dev, void *data, struct drm_fi
 	}
 
 	bo = to_qaic_bo(obj);
+	ret = mutex_lock_interruptible(&bo->lock);
+	if (ret)
+		goto put_bo;
 
 	if (bo->sliced) {
 		ret = -EINVAL;
-		goto put_bo;
+		goto unlock_bo;
 	}
 
 	dbc = &qdev->dbc[args->hdr.dbc_id];
@@ -1029,7 +1034,7 @@  int qaic_attach_slice_bo_ioctl(struct drm_device *dev, void *data, struct drm_fi
 	bo->sliced = true;
 	list_add_tail(&bo->bo_list, &bo->dbc->bo_lists);
 	srcu_read_unlock(&dbc->ch_lock, rcu_id);
-	drm_gem_object_put(obj);
+	mutex_unlock(&bo->lock);
 	srcu_read_unlock(&qdev->dev_lock, qdev_rcu_id);
 	srcu_read_unlock(&usr->qddev_lock, usr_rcu_id);
 
@@ -1039,6 +1044,8 @@  int qaic_attach_slice_bo_ioctl(struct drm_device *dev, void *data, struct drm_fi
 	qaic_unprepare_bo(qdev, bo);
 unlock_ch_srcu:
 	srcu_read_unlock(&dbc->ch_lock, rcu_id);
+unlock_bo:
+	mutex_unlock(&bo->lock);
 put_bo:
 	drm_gem_object_put(obj);
 free_slice_ent:
@@ -1193,15 +1200,18 @@  static int send_bo_list_to_device(struct qaic_device *qdev, struct drm_file *fil
 		}
 
 		bo = to_qaic_bo(obj);
+		ret = mutex_lock_interruptible(&bo->lock);
+		if (ret)
+			goto failed_to_send_bo;
 
 		if (!bo->sliced) {
 			ret = -EINVAL;
-			goto failed_to_send_bo;
+			goto unlock_bo;
 		}
 
 		if (is_partial && pexec[i].resize > bo->base.size) {
 			ret = -EINVAL;
-			goto failed_to_send_bo;
+			goto unlock_bo;
 		}
 
 		spin_lock_irqsave(&dbc->xfer_lock, flags);
@@ -1210,7 +1220,7 @@  static int send_bo_list_to_device(struct qaic_device *qdev, struct drm_file *fil
 		if (queued) {
 			spin_unlock_irqrestore(&dbc->xfer_lock, flags);
 			ret = -EINVAL;
-			goto failed_to_send_bo;
+			goto unlock_bo;
 		}
 
 		bo->req_id = dbc->next_req_id++;
@@ -1241,17 +1251,20 @@  static int send_bo_list_to_device(struct qaic_device *qdev, struct drm_file *fil
 			if (ret) {
 				bo->queued = false;
 				spin_unlock_irqrestore(&dbc->xfer_lock, flags);
-				goto failed_to_send_bo;
+				goto unlock_bo;
 			}
 		}
 		reinit_completion(&bo->xfer_done);
 		list_add_tail(&bo->xfer_list, &dbc->xfer_list);
 		spin_unlock_irqrestore(&dbc->xfer_lock, flags);
 		dma_sync_sgtable_for_device(&qdev->pdev->dev, bo->sgt, bo->dir);
+		mutex_unlock(&bo->lock);
 	}
 
 	return 0;
 
+unlock_bo:
+	mutex_unlock(&bo->lock);
 failed_to_send_bo:
 	if (likely(obj))
 		drm_gem_object_put(obj);
@@ -1807,6 +1820,91 @@  int qaic_perf_stats_bo_ioctl(struct drm_device *dev, void *data, struct drm_file
 	return ret;
 }
 
+static void detach_slice_bo(struct qaic_device *qdev, struct qaic_bo *bo)
+{
+	qaic_free_slices_bo(bo);
+	qaic_unprepare_bo(qdev, bo);
+	qaic_init_bo(bo, true);
+	list_del(&bo->bo_list);
+	drm_gem_object_put(&bo->base);
+}
+
+int qaic_detach_slice_bo_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv)
+{
+	struct qaic_detach_slice *args = data;
+	int rcu_id, usr_rcu_id, qdev_rcu_id;
+	struct dma_bridge_chan *dbc;
+	struct drm_gem_object *obj;
+	struct qaic_device *qdev;
+	struct qaic_user *usr;
+	unsigned long flags;
+	struct qaic_bo *bo;
+	int ret;
+
+	if (args->pad != 0)
+		return -EINVAL;
+
+	usr = file_priv->driver_priv;
+	usr_rcu_id = srcu_read_lock(&usr->qddev_lock);
+	if (!usr->qddev) {
+		ret = -ENODEV;
+		goto unlock_usr_srcu;
+	}
+
+	qdev = usr->qddev->qdev;
+	qdev_rcu_id = srcu_read_lock(&qdev->dev_lock);
+	if (qdev->in_reset) {
+		ret = -ENODEV;
+		goto unlock_dev_srcu;
+	}
+
+	obj = drm_gem_object_lookup(file_priv, args->handle);
+	if (!obj) {
+		ret = -ENOENT;
+		goto unlock_dev_srcu;
+	}
+
+	bo = to_qaic_bo(obj);
+	ret = mutex_lock_interruptible(&bo->lock);
+	if (ret)
+		goto put_bo;
+
+	if (!bo->sliced) {
+		ret = -EINVAL;
+		goto unlock_bo;
+	}
+
+	dbc = bo->dbc;
+	rcu_id = srcu_read_lock(&dbc->ch_lock);
+	if (dbc->usr != usr) {
+		ret = -EINVAL;
+		goto unlock_ch_srcu;
+	}
+
+	/* Check if BO is committed to H/W for DMA */
+	spin_lock_irqsave(&dbc->xfer_lock, flags);
+	if (bo->queued) {
+		spin_unlock_irqrestore(&dbc->xfer_lock, flags);
+		ret = -EBUSY;
+		goto unlock_ch_srcu;
+	}
+	spin_unlock_irqrestore(&dbc->xfer_lock, flags);
+
+	detach_slice_bo(qdev, bo);
+
+unlock_ch_srcu:
+	srcu_read_unlock(&dbc->ch_lock, rcu_id);
+unlock_bo:
+	mutex_unlock(&bo->lock);
+put_bo:
+	drm_gem_object_put(obj);
+unlock_dev_srcu:
+	srcu_read_unlock(&qdev->dev_lock, qdev_rcu_id);
+unlock_usr_srcu:
+	srcu_read_unlock(&usr->qddev_lock, usr_rcu_id);
+	return ret;
+}
+
 static void empty_xfer_list(struct qaic_device *qdev, struct dma_bridge_chan *dbc)
 {
 	unsigned long flags;
@@ -1888,10 +1986,11 @@  void release_dbc(struct qaic_device *qdev, u32 dbc_id)
 	dbc->usr = NULL;
 
 	list_for_each_entry_safe(bo, bo_temp, &dbc->bo_lists, bo_list) {
-		qaic_free_slices_bo(bo);
-		qaic_unprepare_bo(qdev, bo);
-		qaic_init_bo(bo, true);
-		list_del(&bo->bo_list);
+		drm_gem_object_get(&bo->base);
+		mutex_lock(&bo->lock);
+		detach_slice_bo(qdev, bo);
+		mutex_unlock(&bo->lock);
+		drm_gem_object_put(&bo->base);
 	}
 
 	dbc->in_use = false;
diff --git a/drivers/accel/qaic/qaic_drv.c b/drivers/accel/qaic/qaic_drv.c
index b5de82e6eb4d..e2bfb4eaf852 100644
--- a/drivers/accel/qaic/qaic_drv.c
+++ b/drivers/accel/qaic/qaic_drv.c
@@ -150,6 +150,7 @@  static const struct drm_ioctl_desc qaic_drm_ioctls[] = {
 	DRM_IOCTL_DEF_DRV(QAIC_PARTIAL_EXECUTE_BO, qaic_partial_execute_bo_ioctl, 0),
 	DRM_IOCTL_DEF_DRV(QAIC_WAIT_BO, qaic_wait_bo_ioctl, 0),
 	DRM_IOCTL_DEF_DRV(QAIC_PERF_STATS_BO, qaic_perf_stats_bo_ioctl, 0),
+	DRM_IOCTL_DEF_DRV(QAIC_DETACH_SLICE_BO, qaic_detach_slice_bo_ioctl, 0),
 };
 
 static const struct drm_driver qaic_accel_driver = {
diff --git a/include/uapi/drm/qaic_accel.h b/include/uapi/drm/qaic_accel.h
index f89880b7bfb6..43ac5d864512 100644
--- a/include/uapi/drm/qaic_accel.h
+++ b/include/uapi/drm/qaic_accel.h
@@ -372,6 +372,16 @@  struct qaic_perf_stats_entry {
 	__u32 pad;
 };
 
+/**
+ * struct qaic_detach_slice - Detaches slicing configuration from BO.
+ * @handle: In. GEM handle of the BO to detach slicing configuration.
+ * @pad: Structure padding. Must be 0.
+ */
+struct qaic_detach_slice {
+	__u32 handle;
+	__u32 pad;
+};
+
 #define DRM_QAIC_MANAGE				0x00
 #define DRM_QAIC_CREATE_BO			0x01
 #define DRM_QAIC_MMAP_BO			0x02
@@ -380,6 +390,7 @@  struct qaic_perf_stats_entry {
 #define DRM_QAIC_PARTIAL_EXECUTE_BO		0x05
 #define DRM_QAIC_WAIT_BO			0x06
 #define DRM_QAIC_PERF_STATS_BO			0x07
+#define DRM_QAIC_DETACH_SLICE_BO		0x08
 
 #define DRM_IOCTL_QAIC_MANAGE			DRM_IOWR(DRM_COMMAND_BASE + DRM_QAIC_MANAGE, struct qaic_manage_msg)
 #define DRM_IOCTL_QAIC_CREATE_BO		DRM_IOWR(DRM_COMMAND_BASE + DRM_QAIC_CREATE_BO,	struct qaic_create_bo)
@@ -389,6 +400,7 @@  struct qaic_perf_stats_entry {
 #define DRM_IOCTL_QAIC_PARTIAL_EXECUTE_BO	DRM_IOW(DRM_COMMAND_BASE + DRM_QAIC_PARTIAL_EXECUTE_BO,	struct qaic_execute)
 #define DRM_IOCTL_QAIC_WAIT_BO			DRM_IOW(DRM_COMMAND_BASE + DRM_QAIC_WAIT_BO, struct qaic_wait)
 #define DRM_IOCTL_QAIC_PERF_STATS_BO		DRM_IOWR(DRM_COMMAND_BASE + DRM_QAIC_PERF_STATS_BO, struct qaic_perf_stats)
+#define DRM_IOCTL_QAIC_DETACH_SLICE_BO		DRM_IOW(DRM_COMMAND_BASE + DRM_QAIC_DETACH_SLICE_BO, struct qaic_detach_slice)
 
 #if defined(__cplusplus)
 }