[RFC v1 0/9] Parallel submission of dma fence jobs and LR jobs with shared hardware resources

[RFC v1 0/9] Parallel submission of dma fence jobs and LR jobs with shared hardware resources

[Francois Dugast]

Currently Xe KMD only allows either all VMs on the device to be page-faulting
VMs, or none of them to be page-faulting VMs. This prevents page-faulting
workloads from waiting for a dma-fence in the fault handler, as the page fault
would then hold the execution resources, which means the dma-fence would never
signal and this would create a deadlock.

This limitation in the driver prevents mixing dma-fence jobs and long-running
faulting jobs, for example if an application would submit 3D jobs for the
compositor but also SVM compute jobs on the same device. To safely lift this
restriction, a finer approach is introduced in this series.

Hardware engines which share resources and would block each other are assigned
to the same hardware engine group. This group ensures mutual exclusion of the
execution of dma fence jobs and long running jobs on the shared hardware
resources.

If a long running job is executing when a dma fence job is submitted, the long
running job is preempted, the dma fence job executes, then the long running
job is resumed. If a dma fence job is executing when a long running job is
submitted, we wait for completion of the dma fence job before executing the
long running job.

This has been tested on PVC with new IGT tests [1].

[1] https://patchwork.freedesktop.org/series/136191/

Francois Dugast (9):
  drm/xe/hw_engine_group: Introduce xe_hw_engine_group
  drm/xe/exec_queue: Add list link for the hw engine group
  drm/xe/hw_engine_group: Register hw engine group's exec queues
  drm/xe/hw_engine_group: Add helper to suspend LR jobs
  drm/xe/hw_engine_group: Add helper to wait for dma fence jobs
  drm/xe/hw_engine_group: Ensure safe transition between execution modes
  drm/xe/exec: Switch hw engine group execution mode upon job submission
  drm/xe/hw_engine_group: Resume LR exec queues suspended by dma fence
    jobs
  drm/xe/vm: Remove restriction that all VMs must be faulting if one is

 drivers/gpu/drm/xe/xe_device.h           |  10 -
 drivers/gpu/drm/xe/xe_exec.c             |  14 +-
 drivers/gpu/drm/xe/xe_exec_queue.c       |   7 +
 drivers/gpu/drm/xe/xe_exec_queue_types.h |   2 +
 drivers/gpu/drm/xe/xe_hw_engine.c        | 256 +++++++++++++++++++++++
 drivers/gpu/drm/xe/xe_hw_engine.h        |   9 +
 drivers/gpu/drm/xe/xe_hw_engine_types.h  |  31 +++
 drivers/gpu/drm/xe/xe_vm.c               |   8 -
 8 files changed, 318 insertions(+), 19 deletions(-)

-- 
2.43.0

[RFC v1 1/9] drm/xe/hw_engine_group: Introduce xe_hw_engine_group

[Francois Dugast]

A xe_hw_engine_group is a group of hw engines. Two hw engines belong
to the same xe_hw_engine_group if one hw engine cannot make progress
while the other is stuck on a page fault.

Typically, hw engines of the same group share some resources such as
EUs. This depends on the hardware configuration of the platforms.

Currently all engines share EUs so for now only one group is created
and assigned to all hw engines.

Signed-off-by: Francois Dugast <francois.dugast@intel.com>
---
 drivers/gpu/drm/xe/xe_hw_engine.c       | 48 +++++++++++++++++++++++++
 drivers/gpu/drm/xe/xe_hw_engine_types.h | 31 ++++++++++++++++
 2 files changed, 79 insertions(+)

diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c b/drivers/gpu/drm/xe/xe_hw_engine.c
index 78b50d3a6501..f8df85d25617 100644
--- a/drivers/gpu/drm/xe/xe_hw_engine.c
+++ b/drivers/gpu/drm/xe/xe_hw_engine.c
@@ -431,6 +431,53 @@ hw_engine_setup_default_state(struct xe_hw_engine *hwe)
 	xe_rtp_process_to_sr(&ctx, engine_entries, &hwe->reg_sr);
 }
 
+static struct xe_hw_engine_group *
+hw_engine_group_alloc(struct xe_device *xe)
+{
+	struct xe_hw_engine_group *group;
+
+	group = kzalloc(sizeof(*group), GFP_KERNEL);
+	init_rwsem(&group->mode_sem);
+	INIT_LIST_HEAD(&group->exec_queue_list);
+
+	return group;
+}
+
+static void
+hw_engine_setup_groups(struct xe_gt *gt)
+{
+	struct xe_hw_engine *hwe;
+	enum xe_hw_engine_id id;
+	struct xe_hw_engine_group *group_rcs_ccs, *group_bcs, *group_vcs_vecs;
+	struct xe_device *xe = gt_to_xe(gt);
+
+	/*
+	 * Current partitioning implies all engines share EUs therefore
+	 * belong to the same group, so create this group and assign it
+	 * to all engines.
+	 */
+	group_rcs_ccs = hw_engine_group_alloc(xe);
+	group_bcs = hw_engine_group_alloc(xe);
+	group_vcs_vecs = hw_engine_group_alloc(xe);
+	for_each_hw_engine(hwe, gt, id) {
+		switch (hwe->class) {
+		case XE_ENGINE_CLASS_COPY:
+			hwe->hw_engine_group = group_bcs;
+			break;
+		case XE_ENGINE_CLASS_RENDER:
+		case XE_ENGINE_CLASS_COMPUTE:
+			hwe->hw_engine_group = group_rcs_ccs;
+			break;
+		case XE_ENGINE_CLASS_VIDEO_DECODE:
+		case XE_ENGINE_CLASS_VIDEO_ENHANCE:
+			hwe->hw_engine_group = group_vcs_vecs;
+			break;
+		default:
+			drm_warn(&xe->drm, "hw engine class not handled");
+		}
+	}
+}
+
 static void hw_engine_init_early(struct xe_gt *gt, struct xe_hw_engine *hwe,
 				 enum xe_hw_engine_id id)
 {
@@ -761,6 +808,7 @@ int xe_hw_engines_init(struct xe_gt *gt)
 	}
 
 	hw_engine_setup_logical_mapping(gt);
+	hw_engine_setup_groups(gt);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/xe/xe_hw_engine_types.h b/drivers/gpu/drm/xe/xe_hw_engine_types.h
index 70e6434f150d..c6f7bbcb2a41 100644
--- a/drivers/gpu/drm/xe/xe_hw_engine_types.h
+++ b/drivers/gpu/drm/xe/xe_hw_engine_types.h
@@ -100,6 +100,35 @@ struct xe_hw_engine_class_intf {
 	} sched_props, defaults;
 };
 
+/** enum xe_hw_engine_group_execution_mode - possible execution modes of a hw engine group */
+enum xe_hw_engine_group_execution_mode {
+	EXEC_MODE_LR,
+	EXEC_MODE_DMA_FENCE,
+};
+
+/**
+ * struct xe_hw_engine_group - Hardware engine group
+ *
+ * hw engines belong to the same group if they share hardware resources in
+ * a way that prevents them from making progress when one is stuck on a
+ * page fault.
+ */
+struct xe_hw_engine_group {
+	/** @exec_queue_list: list of exec queues attached to this xe_hw_engine_group */
+	struct list_head exec_queue_list;
+	/** @resume_work: worker to resume LR exec queues */
+	struct work_struct resume_work;
+	/** @resume_wq: workqueue to resume LR exec queues */
+	struct workqueue_struct *resume_wq;
+	/** @mode_sem: used to protect this group's hardware resources and
+	 * ensure mutual exclusion between execution only in LR mode
+	 * and execution only in DMA_FENCE mode
+	 */
+	struct rw_semaphore mode_sem;
+	/** @cur_mode: current execution mode of this hw engine group */
+	enum xe_hw_engine_group_execution_mode cur_mode;
+};
+
 /**
  * struct xe_hw_engine - Hardware engine
  *
@@ -150,6 +179,8 @@ struct xe_hw_engine {
 	struct xe_hw_engine_class_intf *eclass;
 	/** @oa_unit: oa unit for this hw engine */
 	struct xe_oa_unit *oa_unit;
+	/** @hw_engine_group: the group of hw engines this one belongs to */
+	struct xe_hw_engine_group *hw_engine_group;
 };
 
 /**
-- 
2.43.0

[RFC v1 2/9] drm/xe/exec_queue: Add list link for the hw engine group

[Francois Dugast]

The exec queues running on hw engines belonging to the same hw engine
group share hardware resources. It will be necessary to navigate
between exec queues within a group for synchronization, hence this
new list link.

Signed-off-by: Francois Dugast <francois.dugast@intel.com>
---
 drivers/gpu/drm/xe/xe_exec_queue_types.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/xe/xe_exec_queue_types.h b/drivers/gpu/drm/xe/xe_exec_queue_types.h
index 201588ec33c3..7df2b532f340 100644
--- a/drivers/gpu/drm/xe/xe_exec_queue_types.h
+++ b/drivers/gpu/drm/xe/xe_exec_queue_types.h
@@ -143,6 +143,8 @@ struct xe_exec_queue {
 	u64 old_run_ticks;
 	/** @run_ticks: hw engine class run time in ticks for this exec queue */
 	u64 run_ticks;
+	/** @hw_engine_group_link: link into exec queues in the same hw engine group */
+	struct list_head hw_engine_group_link;
 	/** @lrc: logical ring context for this exec queue */
 	struct xe_lrc *lrc[];
 };
-- 
2.43.0

[RFC v1 3/9] drm/xe/hw_engine_group: Register hw engine group's exec queues

[Francois Dugast]

Add helpers to safely add and delete the exec queues attached to a hw
engine group, and make use them at the time of creation and destruction
of the exec queues. Keeping track of them is required to control the
execution mode of the hw engine group.

Signed-off-by: Francois Dugast <francois.dugast@intel.com>
---
 drivers/gpu/drm/xe/xe_exec_queue.c |  7 +++++
 drivers/gpu/drm/xe/xe_hw_engine.c  | 45 ++++++++++++++++++++++++++++++
 drivers/gpu/drm/xe/xe_hw_engine.h  |  4 +++
 3 files changed, 56 insertions(+)

diff --git a/drivers/gpu/drm/xe/xe_exec_queue.c b/drivers/gpu/drm/xe/xe_exec_queue.c
index 0ba37835849b..645423a63434 100644
--- a/drivers/gpu/drm/xe/xe_exec_queue.c
+++ b/drivers/gpu/drm/xe/xe_exec_queue.c
@@ -192,6 +192,9 @@ void xe_exec_queue_destroy(struct kref *ref)
 			xe_exec_queue_put(eq);
 	}
 
+	if (q->vm)
+		xe_hw_engine_group_del_exec_queue(q->hwe->hw_engine_group, q);
+
 	q->ops->fini(q);
 }
 
@@ -640,6 +643,10 @@ int xe_exec_queue_create_ioctl(struct drm_device *dev, void *data,
 			if (XE_IOCTL_DBG(xe, err))
 				goto put_exec_queue;
 		}
+
+		err = xe_hw_engine_group_add_exec_queue(q->hwe->hw_engine_group, q);
+		if (err)
+			goto put_exec_queue;
 	}
 
 	mutex_lock(&xef->exec_queue.lock);
diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c b/drivers/gpu/drm/xe/xe_hw_engine.c
index f8df85d25617..4dcc885a55c8 100644
--- a/drivers/gpu/drm/xe/xe_hw_engine.c
+++ b/drivers/gpu/drm/xe/xe_hw_engine.c
@@ -1178,3 +1178,48 @@ u64 xe_hw_engine_read_timestamp(struct xe_hw_engine *hwe)
 {
 	return xe_mmio_read64_2x32(hwe->gt, RING_TIMESTAMP(hwe->mmio_base));
 }
+
+/**
+ * xe_hw_engine_group_add_exec_queue() - Add an exec queue to a hw engine group
+ * @group: The hw engine group
+ * @q: The exec_queue
+ *
+ * Return: 0 on success,
+ *	    -EINTR if the lock could not be acquired
+ */
+int xe_hw_engine_group_add_exec_queue(struct xe_hw_engine_group *group, struct xe_exec_queue *q)
+{
+	int err;
+
+	err = down_write_killable(&group->mode_sem);
+	if (err)
+		return err;
+
+	list_add(&q->hw_engine_group_link, &group->exec_queue_list);
+	up_write(&group->mode_sem);
+
+	return 0;
+}
+
+/**
+ * xe_hw_engine_group_del_exec_queue() - Delete an exec queue from a hw engine group
+ * @group: The hw engine group
+ * @q: The exec_queue
+ *
+ * Return: 0 on success,
+ *	    -EINTR if the lock could not be acquired
+ */
+int xe_hw_engine_group_del_exec_queue(struct xe_hw_engine_group *group, struct xe_exec_queue *q)
+{
+	int err;
+
+	err = down_write_killable(&group->mode_sem);
+	if (err)
+		return err;
+
+	if (!list_empty(&group->exec_queue_list))
+		list_del(&q->hw_engine_group_link);
+	up_write(&group->mode_sem);
+
+	return 0;
+}
diff --git a/drivers/gpu/drm/xe/xe_hw_engine.h b/drivers/gpu/drm/xe/xe_hw_engine.h
index 7f2d27c0ba1a..ce59d83a75ad 100644
--- a/drivers/gpu/drm/xe/xe_hw_engine.h
+++ b/drivers/gpu/drm/xe/xe_hw_engine.h
@@ -9,6 +9,7 @@
 #include "xe_hw_engine_types.h"
 
 struct drm_printer;
+struct xe_exec_queue;
 
 #ifdef CONFIG_DRM_XE_JOB_TIMEOUT_MIN
 #define XE_HW_ENGINE_JOB_TIMEOUT_MIN CONFIG_DRM_XE_JOB_TIMEOUT_MIN
@@ -70,4 +71,7 @@ static inline bool xe_hw_engine_is_valid(struct xe_hw_engine *hwe)
 const char *xe_hw_engine_class_to_str(enum xe_engine_class class);
 u64 xe_hw_engine_read_timestamp(struct xe_hw_engine *hwe);
 
+int xe_hw_engine_group_add_exec_queue(struct xe_hw_engine_group *group, struct xe_exec_queue *q);
+int xe_hw_engine_group_del_exec_queue(struct xe_hw_engine_group *group, struct xe_exec_queue *q);
+
 #endif
-- 
2.43.0

[RFC v1 4/9] drm/xe/hw_engine_group: Add helper to suspend LR jobs

[Francois Dugast]

This is a required feature for dma fence jobs to preempt long running
jobs in order to ensure mutual exclusion on a given hw engine group.

Signed-off-by: Francois Dugast <francois.dugast@intel.com>
---
 drivers/gpu/drm/xe/xe_hw_engine.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c b/drivers/gpu/drm/xe/xe_hw_engine.c
index 4dcc885a55c8..850f7b15b154 100644
--- a/drivers/gpu/drm/xe/xe_hw_engine.c
+++ b/drivers/gpu/drm/xe/xe_hw_engine.c
@@ -1223,3 +1223,31 @@ int xe_hw_engine_group_del_exec_queue(struct xe_hw_engine_group *group, struct x
 
 	return 0;
 }
+
+/**
+ * xe_hw_engine_group_suspend_lr_jobs() - Suspend the long running jobs of this hw engine group
+ * @group: The hw engine group
+ *
+ * Return: 0 on success,
+ *	   -EINVAL if one jobs could not be suspended
+ */
+static int xe_hw_engine_group_suspend_lr_jobs(struct xe_hw_engine_group *group)
+{
+	int err;
+	struct xe_exec_queue *q;
+
+	lockdep_assert_held(&group->mode_sem);
+
+	list_for_each_entry(q, &group->exec_queue_list, hw_engine_group_link) {
+		if (!xe_vm_in_lr_mode(q->vm))
+			continue;
+
+		err = q->ops->suspend(q);
+		if (err)
+			return err;
+
+		q->ops->suspend_wait(q);
+	}
+
+	return 0;
+}
-- 
2.43.0

[RFC v1 5/9] drm/xe/hw_engine_group: Add helper to wait for dma fence jobs

[Francois Dugast]

This is a required feature for long running jobs not to be submitted
while dma fence jobs are running on the hw engine group.

Signed-off-by: Francois Dugast <francois.dugast@intel.com>
---
 drivers/gpu/drm/xe/xe_hw_engine.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c b/drivers/gpu/drm/xe/xe_hw_engine.c
index 850f7b15b154..dc75dfe6187a 100644
--- a/drivers/gpu/drm/xe/xe_hw_engine.c
+++ b/drivers/gpu/drm/xe/xe_hw_engine.c
@@ -1251,3 +1251,30 @@ static int xe_hw_engine_group_suspend_lr_jobs(struct xe_hw_engine_group *group)
 
 	return 0;
 }
+
+/**
+ * xe_hw_engine_group_wait_for_dma_fence_jobs() - Wait for dma fence jobs to complete
+ * @group: The hw engine group
+ *
+ * Return: 0 on success,
+ *	   -ETIME if waiting for one job failed
+ */
+static int xe_hw_engine_group_wait_for_dma_fence_jobs(struct xe_hw_engine_group *group)
+{
+	long timeout;
+	struct xe_exec_queue *q;
+
+	lockdep_assert_held(&group->mode_sem);
+
+	list_for_each_entry(q, &group->exec_queue_list, hw_engine_group_link) {
+		if (xe_vm_in_lr_mode(q->vm))
+			continue;
+
+		timeout = dma_fence_wait(q->last_fence, false);
+
+		if (timeout < 0)
+			return -ETIME;
+	}
+
+	return 0;
+}
-- 
2.43.0

[RFC v1 6/9] drm/xe/hw_engine_group: Ensure safe transition between execution modes

[Francois Dugast]

Provide a way to safely transition execution modes of the hw engine
group ahead of the actual execution. When necessary, either wait for
running jobs to complete or preempt them, thus ensuring mutual
exclusion between modes EXEC_MODE_LR and EXEC_MODE_DMA_FENCE.

Unlike a mutex, the rw_semaphore used in this context allows multiple
submissions in the same mode.

Signed-off-by: Francois Dugast <francois.dugast@intel.com>
---
 drivers/gpu/drm/xe/xe_hw_engine.c | 67 +++++++++++++++++++++++++++++++
 drivers/gpu/drm/xe/xe_hw_engine.h |  3 ++
 2 files changed, 70 insertions(+)

diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c b/drivers/gpu/drm/xe/xe_hw_engine.c
index dc75dfe6187a..4f539711357a 100644
--- a/drivers/gpu/drm/xe/xe_hw_engine.c
+++ b/drivers/gpu/drm/xe/xe_hw_engine.c
@@ -1278,3 +1278,70 @@ static int xe_hw_engine_group_wait_for_dma_fence_jobs(struct xe_hw_engine_group
 
 	return 0;
 }
+
+static int switch_mode(struct xe_hw_engine_group *group,
+		       enum xe_hw_engine_group_execution_mode new_mode)
+{
+	int err = 0;
+
+	lockdep_assert_held(&group->mode_sem);
+
+	if (group->cur_mode == new_mode)
+		return 0;
+	else if (group->cur_mode == EXEC_MODE_LR && new_mode == EXEC_MODE_DMA_FENCE)
+		err = xe_hw_engine_group_suspend_lr_jobs(group);
+	else if (group->cur_mode == EXEC_MODE_DMA_FENCE && new_mode == EXEC_MODE_LR)
+		err = xe_hw_engine_group_wait_for_dma_fence_jobs(group);
+	else
+		err = -EINVAL;
+
+	if (err)
+		return err;
+
+	group->cur_mode = new_mode;
+
+	return 0;
+}
+
+/**
+ * xe_hw_engine_group_get_mode() - Get the group to execute in the new mode
+ * @group: The hw engine group
+ * @mode: The new execution mode
+ *
+ * Return: 0 if successful, -EINTR if locking failed.
+ */
+int xe_hw_engine_group_get_mode(struct xe_hw_engine_group *group,
+				enum xe_hw_engine_group_execution_mode mode)
+{
+	int err = down_read_interruptible(&group->mode_sem);
+
+	if (err)
+		return err;
+
+	if (mode != group->cur_mode) {
+		up_read(&group->mode_sem);
+		err = down_write_killable(&group->mode_sem);
+		if (err)
+			return err;
+
+		if (mode != group->cur_mode) {
+			err = switch_mode(group, mode);
+			if (err) {
+				up_write(&group->mode_sem);
+				return err;
+			}
+		}
+		downgrade_write(&group->mode_sem);
+	}
+
+	return err;
+}
+
+/**
+ * xe_hw_engine_group_put() - Put the group
+ * @group: The hw engine group
+ */
+void xe_hw_engine_group_put(struct xe_hw_engine_group *group)
+{
+	up_read(&group->mode_sem);
+}
diff --git a/drivers/gpu/drm/xe/xe_hw_engine.h b/drivers/gpu/drm/xe/xe_hw_engine.h
index ce59d83a75ad..fce0adf6a7c4 100644
--- a/drivers/gpu/drm/xe/xe_hw_engine.h
+++ b/drivers/gpu/drm/xe/xe_hw_engine.h
@@ -73,5 +73,8 @@ u64 xe_hw_engine_read_timestamp(struct xe_hw_engine *hwe);
 
 int xe_hw_engine_group_add_exec_queue(struct xe_hw_engine_group *group, struct xe_exec_queue *q);
 int xe_hw_engine_group_del_exec_queue(struct xe_hw_engine_group *group, struct xe_exec_queue *q);
+int xe_hw_engine_group_get_mode(struct xe_hw_engine_group *group,
+				enum xe_hw_engine_group_execution_mode mode);
+void xe_hw_engine_group_put(struct xe_hw_engine_group *group);
 
 #endif
-- 
2.43.0

[RFC v1 7/9] drm/xe/exec: Switch hw engine group execution mode upon job submission

[Francois Dugast]

Update the current execution mode of the hw engine group which will be
used to run the job that is about to be submitted. This triggers the
required operations to ensure mutual exclusion of executions modes in
this hw engine group.

Signed-off-by: Francois Dugast <francois.dugast@intel.com>
---
 drivers/gpu/drm/xe/xe_exec.c      | 14 +++++++++++++-
 drivers/gpu/drm/xe/xe_hw_engine.c | 13 +++++++++++++
 drivers/gpu/drm/xe/xe_hw_engine.h |  2 ++
 3 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/xe/xe_exec.c b/drivers/gpu/drm/xe/xe_exec.c
index 2d72cdec3a0b..35418b5d1f5c 100644
--- a/drivers/gpu/drm/xe/xe_exec.c
+++ b/drivers/gpu/drm/xe/xe_exec.c
@@ -14,6 +14,7 @@
 #include "xe_bo.h"
 #include "xe_device.h"
 #include "xe_exec_queue.h"
+#include "xe_hw_engine.h"
 #include "xe_macros.h"
 #include "xe_ring_ops_types.h"
 #include "xe_sched_job.h"
@@ -124,6 +125,8 @@ int xe_exec_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
 	bool write_locked, skip_retry = false;
 	ktime_t end = 0;
 	int err = 0;
+	struct xe_hw_engine_group *group;
+	enum xe_hw_engine_group_execution_mode mode;
 
 	if (XE_IOCTL_DBG(xe, args->extensions) ||
 	    XE_IOCTL_DBG(xe, args->pad[0] || args->pad[1] || args->pad[2]) ||
@@ -182,6 +185,13 @@ int xe_exec_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
 		}
 	}
 
+	group = q->hwe->hw_engine_group;
+	mode = xe_hw_engine_group_find_exec_mode(q);
+
+	err = xe_hw_engine_group_get_mode(group, mode);
+	if (err)
+		goto err_syncs;
+
 retry:
 	if (!xe_vm_in_lr_mode(vm) && xe_vm_userptr_check_repin(vm)) {
 		err = down_write_killable(&vm->lock);
@@ -199,7 +209,7 @@ int xe_exec_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
 		downgrade_write(&vm->lock);
 		write_locked = false;
 		if (err)
-			goto err_unlock_list;
+			goto err_hw_exec_mode;
 	}
 
 	if (!args->num_batch_buffer) {
@@ -324,6 +334,8 @@ int xe_exec_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
 	up_read(&vm->lock);
 	if (err == -EAGAIN && !skip_retry)
 		goto retry;
+err_hw_exec_mode:
+	xe_hw_engine_group_put(group);
 err_syncs:
 	for (i = 0; i < num_syncs; i++)
 		xe_sync_entry_cleanup(&syncs[i]);
diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c b/drivers/gpu/drm/xe/xe_hw_engine.c
index 4f539711357a..e6c755a04fd8 100644
--- a/drivers/gpu/drm/xe/xe_hw_engine.c
+++ b/drivers/gpu/drm/xe/xe_hw_engine.c
@@ -1345,3 +1345,16 @@ void xe_hw_engine_group_put(struct xe_hw_engine_group *group)
 {
 	up_read(&group->mode_sem);
 }
+
+/**
+ * xe_hw_engine_group_find_exec_mode() - Find the execution mode for this exec queue
+ * @q: The exec_queue
+ */
+enum xe_hw_engine_group_execution_mode
+xe_hw_engine_group_find_exec_mode(struct xe_exec_queue *q)
+{
+	if (xe_vm_in_lr_mode(q->vm))
+		return EXEC_MODE_LR;
+	else
+		return EXEC_MODE_DMA_FENCE;
+}
diff --git a/drivers/gpu/drm/xe/xe_hw_engine.h b/drivers/gpu/drm/xe/xe_hw_engine.h
index fce0adf6a7c4..6dfebb18cbb7 100644
--- a/drivers/gpu/drm/xe/xe_hw_engine.h
+++ b/drivers/gpu/drm/xe/xe_hw_engine.h
@@ -76,5 +76,7 @@ int xe_hw_engine_group_del_exec_queue(struct xe_hw_engine_group *group, struct x
 int xe_hw_engine_group_get_mode(struct xe_hw_engine_group *group,
 				enum xe_hw_engine_group_execution_mode mode);
 void xe_hw_engine_group_put(struct xe_hw_engine_group *group);
+enum xe_hw_engine_group_execution_mode
+xe_hw_engine_group_find_exec_mode(struct xe_exec_queue *q);
 
 #endif
-- 
2.43.0

[RFC v1 8/9] drm/xe/hw_engine_group: Resume LR exec queues suspended by dma fence jobs

[Francois Dugast]

Submission of a dma fence job leads to suspending the long running exec
queues of the hw engine group. Work is queued in the resume worker for
this group and execution is resumed on the attached exec queues in long
running mode.

This is another entry point for execution on the hw engine group so the
execution mode is updated.

Signed-off-by: Francois Dugast <francois.dugast@intel.com>
---
 drivers/gpu/drm/xe/xe_hw_engine.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c b/drivers/gpu/drm/xe/xe_hw_engine.c
index e6c755a04fd8..dd8ef65cbf2d 100644
--- a/drivers/gpu/drm/xe/xe_hw_engine.c
+++ b/drivers/gpu/drm/xe/xe_hw_engine.c
@@ -431,6 +431,26 @@ hw_engine_setup_default_state(struct xe_hw_engine *hwe)
 	xe_rtp_process_to_sr(&ctx, engine_entries, &hwe->reg_sr);
 }
 
+static void hw_engine_group_resume_work_func(struct work_struct *w)
+{
+	struct xe_exec_queue *q;
+	struct xe_hw_engine_group *group = container_of(w, struct xe_hw_engine_group, resume_work);
+	int err;
+
+	err = xe_hw_engine_group_get_mode(group, EXEC_MODE_LR);
+	if (err)
+		return;
+
+	list_for_each_entry(q, &group->exec_queue_list, hw_engine_group_link) {
+		if (!xe_vm_in_lr_mode(q->vm))
+			continue;
+
+		q->ops->resume(q);
+	}
+
+	xe_hw_engine_group_put(group);
+}
+
 static struct xe_hw_engine_group *
 hw_engine_group_alloc(struct xe_device *xe)
 {
@@ -438,6 +458,8 @@ hw_engine_group_alloc(struct xe_device *xe)
 
 	group = kzalloc(sizeof(*group), GFP_KERNEL);
 	init_rwsem(&group->mode_sem);
+	group->resume_wq = alloc_ordered_workqueue("xe-resume-lr-jobs-wq", 0);
+	INIT_WORK(&group->resume_work, hw_engine_group_resume_work_func);
 	INIT_LIST_HEAD(&group->exec_queue_list);
 
 	return group;
@@ -1235,6 +1257,7 @@ static int xe_hw_engine_group_suspend_lr_jobs(struct xe_hw_engine_group *group)
 {
 	int err;
 	struct xe_exec_queue *q;
+	bool suspended_queue = false;
 
 	lockdep_assert_held(&group->mode_sem);
 
@@ -1247,8 +1270,13 @@ static int xe_hw_engine_group_suspend_lr_jobs(struct xe_hw_engine_group *group)
 			return err;
 
 		q->ops->suspend_wait(q);
+
+		suspended_queue = true;
 	}
 
+	if (suspended_queue)
+		queue_work(group->resume_wq, &group->resume_work);
+
 	return 0;
 }
 
-- 
2.43.0

[RFC v1 9/9] drm/xe/vm: Remove restriction that all VMs must be faulting if one is

[Francois Dugast]

With this restriction, all VMs on the device must be faulting VMs
if there is already one faulting VM, in which case the device is
considered in fault mode. This prevents for example an application
from running 3D jobs for the compositor while submitting a SVM
compute job on the same device.

Now that mutual exclusion of LR jobs and dma fence jobs is ensured
on the hw engine group, remove this restriction to allow running
faulting and non-faulting VMs on the same device.

Signed-off-by: Francois Dugast <francois.dugast@intel.com>
---
 drivers/gpu/drm/xe/xe_device.h | 10 ----------
 drivers/gpu/drm/xe/xe_vm.c     |  8 --------
 2 files changed, 18 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_device.h b/drivers/gpu/drm/xe/xe_device.h
index bb07f5669dbb..cb69ecf02c25 100644
--- a/drivers/gpu/drm/xe/xe_device.h
+++ b/drivers/gpu/drm/xe/xe_device.h
@@ -129,16 +129,6 @@ static inline struct xe_force_wake *gt_to_fw(struct xe_gt *gt)
 
 void xe_device_assert_mem_access(struct xe_device *xe);
 
-static inline bool xe_device_in_fault_mode(struct xe_device *xe)
-{
-	return xe->usm.num_vm_in_fault_mode != 0;
-}
-
-static inline bool xe_device_in_non_fault_mode(struct xe_device *xe)
-{
-	return xe->usm.num_vm_in_non_fault_mode != 0;
-}
-
 static inline bool xe_device_has_flat_ccs(struct xe_device *xe)
 {
 	return xe->info.has_flat_ccs;
diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
index 5b166fa03684..f0d2dc41a25d 100644
--- a/drivers/gpu/drm/xe/xe_vm.c
+++ b/drivers/gpu/drm/xe/xe_vm.c
@@ -1875,14 +1875,6 @@ int xe_vm_create_ioctl(struct drm_device *dev, void *data,
 			 args->flags & DRM_XE_VM_CREATE_FLAG_FAULT_MODE))
 		return -EINVAL;
 
-	if (XE_IOCTL_DBG(xe, args->flags & DRM_XE_VM_CREATE_FLAG_FAULT_MODE &&
-			 xe_device_in_non_fault_mode(xe)))
-		return -EINVAL;
-
-	if (XE_IOCTL_DBG(xe, !(args->flags & DRM_XE_VM_CREATE_FLAG_FAULT_MODE) &&
-			 xe_device_in_fault_mode(xe)))
-		return -EINVAL;
-
 	if (XE_IOCTL_DBG(xe, args->extensions))
 		return -EINVAL;
 
-- 
2.43.0

✗ CI.Patch_applied: failure for Parallel submission of dma fence jobs and LR jobs with shared hardware resources

[Patchwork]

== Series Details ==

Series: Parallel submission of dma fence jobs and LR jobs with shared hardware resources
URL   : https://patchwork.freedesktop.org/series/136192/
State : failure

== Summary ==

=== Applying kernel patches on branch 'drm-tip' with base: ===
Base commit: cf52b34c47ed drm-tip: 2024y-07m-17d-11h-07m-11s UTC integration manifest
=== git am output follows ===
error: patch failed: drivers/gpu/drm/xe/xe_hw_engine.c:1178
error: drivers/gpu/drm/xe/xe_hw_engine.c: patch does not apply
error: patch failed: drivers/gpu/drm/xe/xe_hw_engine.h:70
error: drivers/gpu/drm/xe/xe_hw_engine.h: patch does not apply
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Applying: drm/xe/hw_engine_group: Introduce xe_hw_engine_group
Applying: drm/xe/exec_queue: Add list link for the hw engine group
Applying: drm/xe/hw_engine_group: Register hw engine group's exec queues
Patch failed at 0003 drm/xe/hw_engine_group: Register hw engine group's exec queues
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

Re: [RFC v1 1/9] drm/xe/hw_engine_group: Introduce xe_hw_engine_group

[Matthew Brost]

On Wed, Jul 17, 2024 at 03:07:22PM +0200, Francois Dugast wrote:
> A xe_hw_engine_group is a group of hw engines. Two hw engines belong
> to the same xe_hw_engine_group if one hw engine cannot make progress
> while the other is stuck on a page fault.
> 
> Typically, hw engines of the same group share some resources such as
> EUs. This depends on the hardware configuration of the platforms.
> 
> Currently all engines share EUs so for now only one group is created
> and assigned to all hw engines.
> 
> Signed-off-by: Francois Dugast <francois.dugast@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_hw_engine.c       | 48 +++++++++++++++++++++++++
>  drivers/gpu/drm/xe/xe_hw_engine_types.h | 31 ++++++++++++++++
>  2 files changed, 79 insertions(+)
> 
> diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c b/drivers/gpu/drm/xe/xe_hw_engine.c
> index 78b50d3a6501..f8df85d25617 100644
> --- a/drivers/gpu/drm/xe/xe_hw_engine.c
> +++ b/drivers/gpu/drm/xe/xe_hw_engine.c
> @@ -431,6 +431,53 @@ hw_engine_setup_default_state(struct xe_hw_engine *hwe)
>  	xe_rtp_process_to_sr(&ctx, engine_entries, &hwe->reg_sr);
>  }
>  
> +static struct xe_hw_engine_group *
> +hw_engine_group_alloc(struct xe_device *xe)
> +{
> +	struct xe_hw_engine_group *group;
> +
> +	group = kzalloc(sizeof(*group), GFP_KERNEL);

Need to handle kzalloc failing here, e.g.

if (!group)
	return ERR_PTR(-ENOMEM);

> +	init_rwsem(&group->mode_sem);
> +	INIT_LIST_HEAD(&group->exec_queue_list);
> +
> +	return group;
> +}
> +
> +static void

This can fail, so:

static int

> +hw_engine_setup_groups(struct xe_gt *gt)
> +{
> +	struct xe_hw_engine *hwe;
> +	enum xe_hw_engine_id id;
> +	struct xe_hw_engine_group *group_rcs_ccs, *group_bcs, *group_vcs_vecs;
> +	struct xe_device *xe = gt_to_xe(gt);
> +
> +	/*
> +	 * Current partitioning implies all engines share EUs therefore
> +	 * belong to the same group, so create this group and assign it
> +	 * to all engines.
> +	 */
> +	group_rcs_ccs = hw_engine_group_alloc(xe);

if (IS_ERR(group_rcs_ccs))
	return PTR_ERR(group_rcs_ccs);

Also you will need to cleanup these allocations on failures and on
driver unload. For driver unload cleanup use drmm_add_action_or_reset, I
suggest adding that below.

> +	group_bcs = hw_engine_group_alloc(xe);
> +	group_vcs_vecs = hw_engine_group_alloc(xe);
> +	for_each_hw_engine(hwe, gt, id) {
> +		switch (hwe->class) {
> +		case XE_ENGINE_CLASS_COPY:
> +			hwe->hw_engine_group = group_bcs;
> +			break;
> +		case XE_ENGINE_CLASS_RENDER:
> +		case XE_ENGINE_CLASS_COMPUTE:
> +			hwe->hw_engine_group = group_rcs_ccs;
> +			break;
> +		case XE_ENGINE_CLASS_VIDEO_DECODE:
> +		case XE_ENGINE_CLASS_VIDEO_ENHANCE:
> +			hwe->hw_engine_group = group_vcs_vecs;
> +			break;
> +		default:
> +			drm_warn(&xe->drm, "hw engine class not handled");

I don't think this is a warn as the GSC (XE_ENGINE_CLASS_OTHER) doesn't
need a group as it is not exposed to user space but pretty sure shows up
in the for_each_hw_engine loop.

> +		}
> +	}
> +}
> +
>  static void hw_engine_init_early(struct xe_gt *gt, struct xe_hw_engine *hwe,
>  				 enum xe_hw_engine_id id)
>  {
> @@ -761,6 +808,7 @@ int xe_hw_engines_init(struct xe_gt *gt)
>  	}
>  
>  	hw_engine_setup_logical_mapping(gt);
> +	hw_engine_setup_groups(gt);

err = hw_engine_setup_groups(gt);
if (err)
	return err;

>  
>  	return 0;

return drmm_add_action_or_reset(..., function_to_kfree_groups);

>  }
> diff --git a/drivers/gpu/drm/xe/xe_hw_engine_types.h b/drivers/gpu/drm/xe/xe_hw_engine_types.h
> index 70e6434f150d..c6f7bbcb2a41 100644
> --- a/drivers/gpu/drm/xe/xe_hw_engine_types.h
> +++ b/drivers/gpu/drm/xe/xe_hw_engine_types.h
> @@ -100,6 +100,35 @@ struct xe_hw_engine_class_intf {
>  	} sched_props, defaults;
>  };
>  
> +/** enum xe_hw_engine_group_execution_mode - possible execution modes of a hw engine group */
> +enum xe_hw_engine_group_execution_mode {
> +	EXEC_MODE_LR,
> +	EXEC_MODE_DMA_FENCE,
> +};
> +
> +/**
> + * struct xe_hw_engine_group - Hardware engine group
> + *
> + * hw engines belong to the same group if they share hardware resources in
> + * a way that prevents them from making progress when one is stuck on a
> + * page fault.
> + */
> +struct xe_hw_engine_group {
> +	/** @exec_queue_list: list of exec queues attached to this xe_hw_engine_group */
> +	struct list_head exec_queue_list;
> +	/** @resume_work: worker to resume LR exec queues */
> +	struct work_struct resume_work;
> +	/** @resume_wq: workqueue to resume LR exec queues */
> +	struct workqueue_struct *resume_wq;

Assume these two members (resume_*) are setup later in the series?

> +	/** @mode_sem: used to protect this group's hardware resources and
> +	 * ensure mutual exclusion between execution only in LR mode
> +	 * and execution only in DMA_FENCE mode
> +	 */

Prefer this style for multi line kernel doc:

/**
 * @foo: some description...
 */

Matt

> +	struct rw_semaphore mode_sem;
> +	/** @cur_mode: current execution mode of this hw engine group */
> +	enum xe_hw_engine_group_execution_mode cur_mode;
> +};
> +
>  /**
>   * struct xe_hw_engine - Hardware engine
>   *
> @@ -150,6 +179,8 @@ struct xe_hw_engine {
>  	struct xe_hw_engine_class_intf *eclass;
>  	/** @oa_unit: oa unit for this hw engine */
>  	struct xe_oa_unit *oa_unit;
> +	/** @hw_engine_group: the group of hw engines this one belongs to */
> +	struct xe_hw_engine_group *hw_engine_group;
>  };
>  
>  /**
> -- 
> 2.43.0
> 

Re: [RFC v1 2/9] drm/xe/exec_queue: Add list link for the hw engine group

[Matthew Brost]

On Wed, Jul 17, 2024 at 03:07:23PM +0200, Francois Dugast wrote:
> The exec queues running on hw engines belonging to the same hw engine
> group share hardware resources. It will be necessary to navigate
> between exec queues within a group for synchronization, hence this
> new list link.
> 
> Signed-off-by: Francois Dugast <francois.dugast@intel.com>

I'd squash this into the next patch.

Matt

> ---
>  drivers/gpu/drm/xe/xe_exec_queue_types.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/xe/xe_exec_queue_types.h b/drivers/gpu/drm/xe/xe_exec_queue_types.h
> index 201588ec33c3..7df2b532f340 100644
> --- a/drivers/gpu/drm/xe/xe_exec_queue_types.h
> +++ b/drivers/gpu/drm/xe/xe_exec_queue_types.h
> @@ -143,6 +143,8 @@ struct xe_exec_queue {
>  	u64 old_run_ticks;
>  	/** @run_ticks: hw engine class run time in ticks for this exec queue */
>  	u64 run_ticks;
> +	/** @hw_engine_group_link: link into exec queues in the same hw engine group */
> +	struct list_head hw_engine_group_link;
>  	/** @lrc: logical ring context for this exec queue */
>  	struct xe_lrc *lrc[];
>  };
> -- 
> 2.43.0
> 

Re: [RFC v1 3/9] drm/xe/hw_engine_group: Register hw engine group's exec queues

[Matthew Brost]

On Wed, Jul 17, 2024 at 03:07:24PM +0200, Francois Dugast wrote:
> Add helpers to safely add and delete the exec queues attached to a hw
> engine group, and make use them at the time of creation and destruction
> of the exec queues. Keeping track of them is required to control the
> execution mode of the hw engine group.
> 
> Signed-off-by: Francois Dugast <francois.dugast@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_exec_queue.c |  7 +++++
>  drivers/gpu/drm/xe/xe_hw_engine.c  | 45 ++++++++++++++++++++++++++++++
>  drivers/gpu/drm/xe/xe_hw_engine.h  |  4 +++
>  3 files changed, 56 insertions(+)
> 
> diff --git a/drivers/gpu/drm/xe/xe_exec_queue.c b/drivers/gpu/drm/xe/xe_exec_queue.c
> index 0ba37835849b..645423a63434 100644
> --- a/drivers/gpu/drm/xe/xe_exec_queue.c
> +++ b/drivers/gpu/drm/xe/xe_exec_queue.c
> @@ -192,6 +192,9 @@ void xe_exec_queue_destroy(struct kref *ref)
>  			xe_exec_queue_put(eq);
>  	}
>  
> +	if (q->vm)
> +		xe_hw_engine_group_del_exec_queue(q->hwe->hw_engine_group, q);
> +
>  	q->ops->fini(q);
>  }
>  
> @@ -640,6 +643,10 @@ int xe_exec_queue_create_ioctl(struct drm_device *dev, void *data,
>  			if (XE_IOCTL_DBG(xe, err))
>  				goto put_exec_queue;
>  		}
> +
> +		err = xe_hw_engine_group_add_exec_queue(q->hwe->hw_engine_group, q);
> +		if (err)
> +			goto put_exec_queue;
>  	}
>  
>  	mutex_lock(&xef->exec_queue.lock);
> diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c b/drivers/gpu/drm/xe/xe_hw_engine.c
> index f8df85d25617..4dcc885a55c8 100644
> --- a/drivers/gpu/drm/xe/xe_hw_engine.c
> +++ b/drivers/gpu/drm/xe/xe_hw_engine.c
> @@ -1178,3 +1178,48 @@ u64 xe_hw_engine_read_timestamp(struct xe_hw_engine *hwe)
>  {
>  	return xe_mmio_read64_2x32(hwe->gt, RING_TIMESTAMP(hwe->mmio_base));
>  }
> +
> +/**
> + * xe_hw_engine_group_add_exec_queue() - Add an exec queue to a hw engine group
> + * @group: The hw engine group
> + * @q: The exec_queue
> + *
> + * Return: 0 on success,
> + *	    -EINTR if the lock could not be acquired
> + */
> +int xe_hw_engine_group_add_exec_queue(struct xe_hw_engine_group *group, struct xe_exec_queue *q)
> +{
> +	int err;
> +
> +	err = down_write_killable(&group->mode_sem);

I think it is overkill have this killable, but fine leave it as is as it
ok fail here.

> +	if (err)
> +		return err;
> +
> +	list_add(&q->hw_engine_group_link, &group->exec_queue_list);
> +	up_write(&group->mode_sem);
> +
> +	return 0;
> +}
> +
> +/**
> + * xe_hw_engine_group_del_exec_queue() - Delete an exec queue from a hw engine group
> + * @group: The hw engine group
> + * @q: The exec_queue
> + *
> + * Return: 0 on success,
> + *	    -EINTR if the lock could not be acquired
> + */
> +int xe_hw_engine_group_del_exec_queue(struct xe_hw_engine_group *group, struct xe_exec_queue *q)
> +{
> +	int err;
> +
> +	err = down_write_killable(&group->mode_sem);

So here, I don't we can fail as this a cleanup. So 'void
xe_hw_engine_group_del_exec_queue' and
s/down_write_killable/down_write/

Matt

> +	if (err)
> +		return err;
> +
> +	if (!list_empty(&group->exec_queue_list))
> +		list_del(&q->hw_engine_group_link);
> +	up_write(&group->mode_sem);
> +
> +	return 0;
> +}
> diff --git a/drivers/gpu/drm/xe/xe_hw_engine.h b/drivers/gpu/drm/xe/xe_hw_engine.h
> index 7f2d27c0ba1a..ce59d83a75ad 100644
> --- a/drivers/gpu/drm/xe/xe_hw_engine.h
> +++ b/drivers/gpu/drm/xe/xe_hw_engine.h
> @@ -9,6 +9,7 @@
>  #include "xe_hw_engine_types.h"
>  
>  struct drm_printer;
> +struct xe_exec_queue;
>  
>  #ifdef CONFIG_DRM_XE_JOB_TIMEOUT_MIN
>  #define XE_HW_ENGINE_JOB_TIMEOUT_MIN CONFIG_DRM_XE_JOB_TIMEOUT_MIN
> @@ -70,4 +71,7 @@ static inline bool xe_hw_engine_is_valid(struct xe_hw_engine *hwe)
>  const char *xe_hw_engine_class_to_str(enum xe_engine_class class);
>  u64 xe_hw_engine_read_timestamp(struct xe_hw_engine *hwe);
>  
> +int xe_hw_engine_group_add_exec_queue(struct xe_hw_engine_group *group, struct xe_exec_queue *q);
> +int xe_hw_engine_group_del_exec_queue(struct xe_hw_engine_group *group, struct xe_exec_queue *q);
> +
>  #endif
> -- 
> 2.43.0
> 

Re: [RFC v1 3/9] drm/xe/hw_engine_group: Register hw engine group's exec queues

[Matthew Brost]

On Wed, Jul 17, 2024 at 03:07:24PM +0200, Francois Dugast wrote:
> Add helpers to safely add and delete the exec queues attached to a hw
> engine group, and make use them at the time of creation and destruction
> of the exec queues. Keeping track of them is required to control the
> execution mode of the hw engine group.
> 

Missed one thing...

> Signed-off-by: Francois Dugast <francois.dugast@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_exec_queue.c |  7 +++++
>  drivers/gpu/drm/xe/xe_hw_engine.c  | 45 ++++++++++++++++++++++++++++++
>  drivers/gpu/drm/xe/xe_hw_engine.h  |  4 +++
>  3 files changed, 56 insertions(+)
> 
> diff --git a/drivers/gpu/drm/xe/xe_exec_queue.c b/drivers/gpu/drm/xe/xe_exec_queue.c
> index 0ba37835849b..645423a63434 100644
> --- a/drivers/gpu/drm/xe/xe_exec_queue.c
> +++ b/drivers/gpu/drm/xe/xe_exec_queue.c
> @@ -192,6 +192,9 @@ void xe_exec_queue_destroy(struct kref *ref)
>  			xe_exec_queue_put(eq);
>  	}
>  
> +	if (q->vm)
> +		xe_hw_engine_group_del_exec_queue(q->hwe->hw_engine_group, q);
> +
>  	q->ops->fini(q);
>  }
>  
> @@ -640,6 +643,10 @@ int xe_exec_queue_create_ioctl(struct drm_device *dev, void *data,
>  			if (XE_IOCTL_DBG(xe, err))
>  				goto put_exec_queue;
>  		}
> +
> +		err = xe_hw_engine_group_add_exec_queue(q->hwe->hw_engine_group, q);
> +		if (err)
> +			goto put_exec_queue;
>  	}
>  
>  	mutex_lock(&xef->exec_queue.lock);
> diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c b/drivers/gpu/drm/xe/xe_hw_engine.c
> index f8df85d25617..4dcc885a55c8 100644
> --- a/drivers/gpu/drm/xe/xe_hw_engine.c
> +++ b/drivers/gpu/drm/xe/xe_hw_engine.c
> @@ -1178,3 +1178,48 @@ u64 xe_hw_engine_read_timestamp(struct xe_hw_engine *hwe)
>  {
>  	return xe_mmio_read64_2x32(hwe->gt, RING_TIMESTAMP(hwe->mmio_base));
>  }
> +
> +/**
> + * xe_hw_engine_group_add_exec_queue() - Add an exec queue to a hw engine group
> + * @group: The hw engine group
> + * @q: The exec_queue
> + *
> + * Return: 0 on success,
> + *	    -EINTR if the lock could not be acquired
> + */
> +int xe_hw_engine_group_add_exec_queue(struct xe_hw_engine_group *group, struct xe_exec_queue *q)
> +{
> +	int err;
> +

Let's be paranoid...

xe_assert(xe, q->vm);

> +	err = down_write_killable(&group->mode_sem);
> +	if (err)
> +		return err;
> +
> +	list_add(&q->hw_engine_group_link, &group->exec_queue_list);
> +	up_write(&group->mode_sem);
> +
> +	return 0;
> +}
> +
> +/**
> + * xe_hw_engine_group_del_exec_queue() - Delete an exec queue from a hw engine group
> + * @group: The hw engine group
> + * @q: The exec_queue
> + *
> + * Return: 0 on success,
> + *	    -EINTR if the lock could not be acquired
> + */
> +int xe_hw_engine_group_del_exec_queue(struct xe_hw_engine_group *group, struct xe_exec_queue *q)
> +{
> +	int err;
> +

Here too:

xe_assert(xe, q->vm);

Matt

> +	err = down_write_killable(&group->mode_sem);
> +	if (err)
> +		return err;
> +
> +	if (!list_empty(&group->exec_queue_list))
> +		list_del(&q->hw_engine_group_link);
> +	up_write(&group->mode_sem);
> +
> +	return 0;
> +}
> diff --git a/drivers/gpu/drm/xe/xe_hw_engine.h b/drivers/gpu/drm/xe/xe_hw_engine.h
> index 7f2d27c0ba1a..ce59d83a75ad 100644
> --- a/drivers/gpu/drm/xe/xe_hw_engine.h
> +++ b/drivers/gpu/drm/xe/xe_hw_engine.h
> @@ -9,6 +9,7 @@
>  #include "xe_hw_engine_types.h"
>  
>  struct drm_printer;
> +struct xe_exec_queue;
>  
>  #ifdef CONFIG_DRM_XE_JOB_TIMEOUT_MIN
>  #define XE_HW_ENGINE_JOB_TIMEOUT_MIN CONFIG_DRM_XE_JOB_TIMEOUT_MIN
> @@ -70,4 +71,7 @@ static inline bool xe_hw_engine_is_valid(struct xe_hw_engine *hwe)
>  const char *xe_hw_engine_class_to_str(enum xe_engine_class class);
>  u64 xe_hw_engine_read_timestamp(struct xe_hw_engine *hwe);
>  
> +int xe_hw_engine_group_add_exec_queue(struct xe_hw_engine_group *group, struct xe_exec_queue *q);
> +int xe_hw_engine_group_del_exec_queue(struct xe_hw_engine_group *group, struct xe_exec_queue *q);
> +
>  #endif
> -- 
> 2.43.0
> 

Re: [RFC v1 4/9] drm/xe/hw_engine_group: Add helper to suspend LR jobs

[Matthew Brost]

On Wed, Jul 17, 2024 at 03:07:25PM +0200, Francois Dugast wrote:
> This is a required feature for dma fence jobs to preempt long running
> jobs in order to ensure mutual exclusion on a given hw engine group.
> 
> Signed-off-by: Francois Dugast <francois.dugast@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_hw_engine.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c b/drivers/gpu/drm/xe/xe_hw_engine.c
> index 4dcc885a55c8..850f7b15b154 100644
> --- a/drivers/gpu/drm/xe/xe_hw_engine.c
> +++ b/drivers/gpu/drm/xe/xe_hw_engine.c
> @@ -1223,3 +1223,31 @@ int xe_hw_engine_group_del_exec_queue(struct xe_hw_engine_group *group, struct x
>  
>  	return 0;
>  }
> +
> +/**
> + * xe_hw_engine_group_suspend_lr_jobs() - Suspend the long running jobs of this hw engine group
> + * @group: The hw engine group
> + *
> + * Return: 0 on success,
> + *	   -EINVAL if one jobs could not be suspended
> + */
> +static int xe_hw_engine_group_suspend_lr_jobs(struct xe_hw_engine_group *group)
> +{
> +	int err;
> +	struct xe_exec_queue *q;
> +
> +	lockdep_assert_held(&group->mode_sem);
> +
> +	list_for_each_entry(q, &group->exec_queue_list, hw_engine_group_link) {
> +		if (!xe_vm_in_lr_mode(q->vm))
> +			continue;
> +
> +		err = q->ops->suspend(q);
> +		if (err)
> +			return err;

Hmm, this error handling might not be correct. I think it ok for a kill
/ wedge / reset to race here and we'd still want to continue this loop.
Perhaps just add a drm_warn message for now and continue the loop.

Also same deal if suspend_wait() fails.

> +
> +		q->ops->suspend_wait(q);

Pipeline these as suspend() takes a non-zero ammount of time and the GuC
does the suspend async AFIAK (e.g. it issues a suspend, then moves onto
something else, so multiple suspends can be running in parallel).

So...

list_for_each_entry()
	q->ops->suspend(q);

list_for_each_entry()
	q->ops->suspend_wait(q);

Matt

> +	}
> +
> +	return 0;
> +}
> -- 
> 2.43.0
> 

Re: [RFC v1 3/9] drm/xe/hw_engine_group: Register hw engine group's exec queues

[Matthew Brost]

On Wed, Jul 17, 2024 at 03:07:24PM +0200, Francois Dugast wrote:
> Add helpers to safely add and delete the exec queues attached to a hw
> engine group, and make use them at the time of creation and destruction
> of the exec queues. Keeping track of them is required to control the
> execution mode of the hw engine group.
> 

Missed a few more things...

> Signed-off-by: Francois Dugast <francois.dugast@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_exec_queue.c |  7 +++++
>  drivers/gpu/drm/xe/xe_hw_engine.c  | 45 ++++++++++++++++++++++++++++++
>  drivers/gpu/drm/xe/xe_hw_engine.h  |  4 +++
>  3 files changed, 56 insertions(+)
> 
> diff --git a/drivers/gpu/drm/xe/xe_exec_queue.c b/drivers/gpu/drm/xe/xe_exec_queue.c
> index 0ba37835849b..645423a63434 100644
> --- a/drivers/gpu/drm/xe/xe_exec_queue.c
> +++ b/drivers/gpu/drm/xe/xe_exec_queue.c
> @@ -192,6 +192,9 @@ void xe_exec_queue_destroy(struct kref *ref)
>  			xe_exec_queue_put(eq);
>  	}
>  
> +	if (q->vm)

I think this code path can be called GSC exec queues which don't have a
q->hwe->hw_engine_group. So I think:

if (q->vm && q->hwe->hw_engine_group)
	xe_hw_engine_group_del_exec_queue(q->hwe->hw_engine_group, q);

> +		xe_hw_engine_group_del_exec_queue(q->hwe->hw_engine_group, q);
> +
>  	q->ops->fini(q);
>  }
>  
> @@ -640,6 +643,10 @@ int xe_exec_queue_create_ioctl(struct drm_device *dev, void *data,
>  			if (XE_IOCTL_DBG(xe, err))
>  				goto put_exec_queue;
>  		}
> +
> +		err = xe_hw_engine_group_add_exec_queue(q->hwe->hw_engine_group, q);

So this only called on non-VM bind exec queues. I think techincally this
wrong as VM bind exec queues can use dma-fences plus the copy engine. I
plan dropping the copy engine for these [1] and I don't think it worth
fixing VM bind path with mode switching if we only are going to drop
this requirement soon. Let me just respin [1] and hopefully we can
prioritize though reviews so these two series land at roughly the same
time. 

[1] https://patchwork.freedesktop.org/patch/582003/?series=125608&rev=5

> +		if (err)
> +			goto put_exec_queue;
>  	}
>  
>  	mutex_lock(&xef->exec_queue.lock);
> diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c b/drivers/gpu/drm/xe/xe_hw_engine.c
> index f8df85d25617..4dcc885a55c8 100644
> --- a/drivers/gpu/drm/xe/xe_hw_engine.c
> +++ b/drivers/gpu/drm/xe/xe_hw_engine.c
> @@ -1178,3 +1178,48 @@ u64 xe_hw_engine_read_timestamp(struct xe_hw_engine *hwe)
>  {
>  	return xe_mmio_read64_2x32(hwe->gt, RING_TIMESTAMP(hwe->mmio_base));
>  }
> +
> +/**
> + * xe_hw_engine_group_add_exec_queue() - Add an exec queue to a hw engine group
> + * @group: The hw engine group
> + * @q: The exec_queue
> + *
> + * Return: 0 on success,
> + *	    -EINTR if the lock could not be acquired
> + */
> +int xe_hw_engine_group_add_exec_queue(struct xe_hw_engine_group *group, struct xe_exec_queue *q)
> +{
> +	int err;
> +

Also assert here this is not a VM bind queue (e.g. EXEC_QUEUE_FLAG_VM is
clear).

> +	err = down_write_killable(&group->mode_sem);
> +	if (err)
> +		return err;
> +
> +	list_add(&q->hw_engine_group_link, &group->exec_queue_list);

I don't see where INIT_LIST_HEAD is called on group->exec_queue_list. I
think that should unconditionally be done in __xe_exec_queue_alloc.

Matt

> +	up_write(&group->mode_sem);
> +
> +	return 0;
> +}
> +
> +/**
> + * xe_hw_engine_group_del_exec_queue() - Delete an exec queue from a hw engine group
> + * @group: The hw engine group
> + * @q: The exec_queue
> + *
> + * Return: 0 on success,
> + *	    -EINTR if the lock could not be acquired
> + */
> +int xe_hw_engine_group_del_exec_queue(struct xe_hw_engine_group *group, struct xe_exec_queue *q)
> +{
> +	int err;
> +
> +	err = down_write_killable(&group->mode_sem);
> +	if (err)
> +		return err;
> +
> +	if (!list_empty(&group->exec_queue_list))
> +		list_del(&q->hw_engine_group_link);
> +	up_write(&group->mode_sem);
> +
> +	return 0;
> +}
> diff --git a/drivers/gpu/drm/xe/xe_hw_engine.h b/drivers/gpu/drm/xe/xe_hw_engine.h
> index 7f2d27c0ba1a..ce59d83a75ad 100644
> --- a/drivers/gpu/drm/xe/xe_hw_engine.h
> +++ b/drivers/gpu/drm/xe/xe_hw_engine.h
> @@ -9,6 +9,7 @@
>  #include "xe_hw_engine_types.h"
>  
>  struct drm_printer;
> +struct xe_exec_queue;
>  
>  #ifdef CONFIG_DRM_XE_JOB_TIMEOUT_MIN
>  #define XE_HW_ENGINE_JOB_TIMEOUT_MIN CONFIG_DRM_XE_JOB_TIMEOUT_MIN
> @@ -70,4 +71,7 @@ static inline bool xe_hw_engine_is_valid(struct xe_hw_engine *hwe)
>  const char *xe_hw_engine_class_to_str(enum xe_engine_class class);
>  u64 xe_hw_engine_read_timestamp(struct xe_hw_engine *hwe);
>  
> +int xe_hw_engine_group_add_exec_queue(struct xe_hw_engine_group *group, struct xe_exec_queue *q);
> +int xe_hw_engine_group_del_exec_queue(struct xe_hw_engine_group *group, struct xe_exec_queue *q);
> +
>  #endif
> -- 
> 2.43.0
> 

Re: [RFC v1 5/9] drm/xe/hw_engine_group: Add helper to wait for dma fence jobs

[Matthew Brost]

On Wed, Jul 17, 2024 at 03:07:26PM +0200, Francois Dugast wrote:
> This is a required feature for long running jobs not to be submitted
> while dma fence jobs are running on the hw engine group.
> 
> Signed-off-by: Francois Dugast <francois.dugast@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_hw_engine.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c b/drivers/gpu/drm/xe/xe_hw_engine.c
> index 850f7b15b154..dc75dfe6187a 100644
> --- a/drivers/gpu/drm/xe/xe_hw_engine.c
> +++ b/drivers/gpu/drm/xe/xe_hw_engine.c
> @@ -1251,3 +1251,30 @@ static int xe_hw_engine_group_suspend_lr_jobs(struct xe_hw_engine_group *group)
>  
>  	return 0;
>  }
> +
> +/**
> + * xe_hw_engine_group_wait_for_dma_fence_jobs() - Wait for dma fence jobs to complete
> + * @group: The hw engine group
> + *
> + * Return: 0 on success,
> + *	   -ETIME if waiting for one job failed
> + */
> +static int xe_hw_engine_group_wait_for_dma_fence_jobs(struct xe_hw_engine_group *group)
> +{
> +	long timeout;
> +	struct xe_exec_queue *q;
> +
> +	lockdep_assert_held(&group->mode_sem);
> +
> +	list_for_each_entry(q, &group->exec_queue_list, hw_engine_group_link) {
> +		if (xe_vm_in_lr_mode(q->vm))
> +			continue;
> +
> +		timeout = dma_fence_wait(q->last_fence, false);

Hmm, I don't think this is safe to just reference q->last_fence because
it can be NULL or for locking reasons.

See xe_exec_queue_last_fence_lockdep_assert for the current locking
requirments.

We are going to have to rethinking
xe_exec_queue_last_fence_lockdep_assert. I think we can safely reference
q->last_fence if this function is called in group->mode_sem in write
mode. Let me think on this and maybe will have more thoughts once I get
through the entire series.

wrt to dma_fence_wait not being interruptable, if this is called from a
user IOCTL can can't really do that. Same goes for suspend_wait in the
previous patch, we'd have to make that interruptable as well. Again let
me get through the entire series and maybe will have more thoughts here.

Matt

> +
> +		if (timeout < 0)
> +			return -ETIME;
> +	}
> +
> +	return 0;
> +}
> -- 
> 2.43.0
> 

Re: [RFC v1 6/9] drm/xe/hw_engine_group: Ensure safe transition between execution modes

[Matthew Brost]

On Wed, Jul 17, 2024 at 03:07:27PM +0200, Francois Dugast wrote:
> Provide a way to safely transition execution modes of the hw engine
> group ahead of the actual execution. When necessary, either wait for
> running jobs to complete or preempt them, thus ensuring mutual
> exclusion between modes EXEC_MODE_LR and EXEC_MODE_DMA_FENCE.
> 
> Unlike a mutex, the rw_semaphore used in this context allows multiple
> submissions in the same mode.
> 
> Signed-off-by: Francois Dugast <francois.dugast@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_hw_engine.c | 67 +++++++++++++++++++++++++++++++
>  drivers/gpu/drm/xe/xe_hw_engine.h |  3 ++
>  2 files changed, 70 insertions(+)
> 
> diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c b/drivers/gpu/drm/xe/xe_hw_engine.c
> index dc75dfe6187a..4f539711357a 100644
> --- a/drivers/gpu/drm/xe/xe_hw_engine.c
> +++ b/drivers/gpu/drm/xe/xe_hw_engine.c
> @@ -1278,3 +1278,70 @@ static int xe_hw_engine_group_wait_for_dma_fence_jobs(struct xe_hw_engine_group
>  
>  	return 0;
>  }
> +
> +static int switch_mode(struct xe_hw_engine_group *group,
> +		       enum xe_hw_engine_group_execution_mode new_mode)
> +{
> +	int err = 0;
> +
> +	lockdep_assert_held(&group->mode_sem);

lockdep_assert_held_write

> +
> +	if (group->cur_mode == new_mode)
> +		return 0;

This is redunant as the caller checks this.

> +	else if (group->cur_mode == EXEC_MODE_LR && new_mode == EXEC_MODE_DMA_FENCE)
> +		err = xe_hw_engine_group_suspend_lr_jobs(group);
> +	else if (group->cur_mode == EXEC_MODE_DMA_FENCE && new_mode == EXEC_MODE_LR)
> +		err = xe_hw_engine_group_wait_for_dma_fence_jobs(group);

These functions lockdep annotation should be lockdep_assert_held_write too.

> +	else
> +		err = -EINVAL;
> +
> +	if (err)
> +		return err;
> +
> +	group->cur_mode = new_mode;
> +
> +	return 0;
> +}
> +
> +/**
> + * xe_hw_engine_group_get_mode() - Get the group to execute in the new mode
> + * @group: The hw engine group
> + * @mode: The new execution mode
> + *
> + * Return: 0 if successful, -EINTR if locking failed.
> + */
> +int xe_hw_engine_group_get_mode(struct xe_hw_engine_group *group,
> +				enum xe_hw_engine_group_execution_mode mode)

I think you need this annotaion. Unsure if the lock being interruptible
though if this is needed. I'd double check on that.

__acquires(&group->mode_sem);

> +{
> +	int err = down_read_interruptible(&group->mode_sem);
> +
> +	if (err)
> +		return err;
> +
> +	if (mode != group->cur_mode) {
> +		up_read(&group->mode_sem);
> +		err = down_write_killable(&group->mode_sem);
> +		if (err)
> +			return err;
> +
> +		if (mode != group->cur_mode) {
> +			err = switch_mode(group, mode);
> +			if (err) {
> +				up_write(&group->mode_sem);
> +				return err;
> +			}
> +		}
> +		downgrade_write(&group->mode_sem);
> +	}
> +
> +	return err;
> +}
> +
> +/**
> + * xe_hw_engine_group_put() - Put the group
> + * @group: The hw engine group
> + */
> +void xe_hw_engine_group_put(struct xe_hw_engine_group *group)

__releases(&group->mode_sem);

> +{
> +	up_read(&group->mode_sem);
> +}
> diff --git a/drivers/gpu/drm/xe/xe_hw_engine.h b/drivers/gpu/drm/xe/xe_hw_engine.h
> index ce59d83a75ad..fce0adf6a7c4 100644
> --- a/drivers/gpu/drm/xe/xe_hw_engine.h
> +++ b/drivers/gpu/drm/xe/xe_hw_engine.h
> @@ -73,5 +73,8 @@ u64 xe_hw_engine_read_timestamp(struct xe_hw_engine *hwe);
>  
>  int xe_hw_engine_group_add_exec_queue(struct xe_hw_engine_group *group, struct xe_exec_queue *q);
>  int xe_hw_engine_group_del_exec_queue(struct xe_hw_engine_group *group, struct xe_exec_queue *q);
> +int xe_hw_engine_group_get_mode(struct xe_hw_engine_group *group,
> +				enum xe_hw_engine_group_execution_mode mode);

Maybe consider putting these function in there own files:

xe_hw_engine_group.h
xe_hw_engine_group_types.h
xe_hw_engine_group.c

Matt

> +void xe_hw_engine_group_put(struct xe_hw_engine_group *group);
>  
>  #endif
> -- 
> 2.43.0
> 

Re: [RFC v1 7/9] drm/xe/exec: Switch hw engine group execution mode upon job submission

[Matthew Brost]

On Wed, Jul 17, 2024 at 03:07:28PM +0200, Francois Dugast wrote:
> Update the current execution mode of the hw engine group which will be
> used to run the job that is about to be submitted. This triggers the
> required operations to ensure mutual exclusion of executions modes in
> this hw engine group.
> 
> Signed-off-by: Francois Dugast <francois.dugast@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_exec.c      | 14 +++++++++++++-
>  drivers/gpu/drm/xe/xe_hw_engine.c | 13 +++++++++++++
>  drivers/gpu/drm/xe/xe_hw_engine.h |  2 ++
>  3 files changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_exec.c b/drivers/gpu/drm/xe/xe_exec.c
> index 2d72cdec3a0b..35418b5d1f5c 100644
> --- a/drivers/gpu/drm/xe/xe_exec.c
> +++ b/drivers/gpu/drm/xe/xe_exec.c
> @@ -14,6 +14,7 @@
>  #include "xe_bo.h"
>  #include "xe_device.h"
>  #include "xe_exec_queue.h"
> +#include "xe_hw_engine.h"
>  #include "xe_macros.h"
>  #include "xe_ring_ops_types.h"
>  #include "xe_sched_job.h"
> @@ -124,6 +125,8 @@ int xe_exec_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
>  	bool write_locked, skip_retry = false;
>  	ktime_t end = 0;
>  	int err = 0;
> +	struct xe_hw_engine_group *group;
> +	enum xe_hw_engine_group_execution_mode mode;
>  
>  	if (XE_IOCTL_DBG(xe, args->extensions) ||
>  	    XE_IOCTL_DBG(xe, args->pad[0] || args->pad[1] || args->pad[2]) ||
> @@ -182,6 +185,13 @@ int xe_exec_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
>  		}
>  	}
>  
> +	group = q->hwe->hw_engine_group;
> +	mode = xe_hw_engine_group_find_exec_mode(q);
> +

So you only need to call xe_hw_engine_group_get_mode if in dma-fence
mode as LR submissions are allowed in the suspended state. They are just
held in GuC until submission is enabled.

> +	err = xe_hw_engine_group_get_mode(group, mode);
> +	if (err)
> +		goto err_syncs;
> +
>  retry:
>  	if (!xe_vm_in_lr_mode(vm) && xe_vm_userptr_check_repin(vm)) {
>  		err = down_write_killable(&vm->lock);
> @@ -199,7 +209,7 @@ int xe_exec_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
>  		downgrade_write(&vm->lock);
>  		write_locked = false;
>  		if (err)
> -			goto err_unlock_list;
> +			goto err_hw_exec_mode;
>  	}
>  
>  	if (!args->num_batch_buffer) {
> @@ -324,6 +334,8 @@ int xe_exec_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
>  	up_read(&vm->lock);
>  	if (err == -EAGAIN && !skip_retry)
>  		goto retry;
> +err_hw_exec_mode:
> +	xe_hw_engine_group_put(group);
>  err_syncs:
>  	for (i = 0; i < num_syncs; i++)
>  		xe_sync_entry_cleanup(&syncs[i]);
> diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c b/drivers/gpu/drm/xe/xe_hw_engine.c
> index 4f539711357a..e6c755a04fd8 100644
> --- a/drivers/gpu/drm/xe/xe_hw_engine.c
> +++ b/drivers/gpu/drm/xe/xe_hw_engine.c
> @@ -1345,3 +1345,16 @@ void xe_hw_engine_group_put(struct xe_hw_engine_group *group)
>  {
>  	up_read(&group->mode_sem);
>  }
> +
> +/**
> + * xe_hw_engine_group_find_exec_mode() - Find the execution mode for this exec queue
> + * @q: The exec_queue
> + */
> +enum xe_hw_engine_group_execution_mode
> +xe_hw_engine_group_find_exec_mode(struct xe_exec_queue *q)
> +{
> +	if (xe_vm_in_lr_mode(q->vm))
> +		return EXEC_MODE_LR;
> +	else
> +		return EXEC_MODE_DMA_FENCE;
> +}
> diff --git a/drivers/gpu/drm/xe/xe_hw_engine.h b/drivers/gpu/drm/xe/xe_hw_engine.h
> index fce0adf6a7c4..6dfebb18cbb7 100644
> --- a/drivers/gpu/drm/xe/xe_hw_engine.h
> +++ b/drivers/gpu/drm/xe/xe_hw_engine.h
> @@ -76,5 +76,7 @@ int xe_hw_engine_group_del_exec_queue(struct xe_hw_engine_group *group, struct x
>  int xe_hw_engine_group_get_mode(struct xe_hw_engine_group *group,
>  				enum xe_hw_engine_group_execution_mode mode);
>  void xe_hw_engine_group_put(struct xe_hw_engine_group *group);
> +enum xe_hw_engine_group_execution_mode
> +xe_hw_engine_group_find_exec_mode(struct xe_exec_queue *q);

Same comment as previous patch, consider a standalone file for
xe_hw_engine_group.

Matt

>  
>  #endif
> -- 
> 2.43.0
> 

Re: [RFC v1 8/9] drm/xe/hw_engine_group: Resume LR exec queues suspended by dma fence jobs

[Matthew Brost]

On Wed, Jul 17, 2024 at 03:07:29PM +0200, Francois Dugast wrote:
> Submission of a dma fence job leads to suspending the long running exec
> queues of the hw engine group. Work is queued in the resume worker for
> this group and execution is resumed on the attached exec queues in long
> running mode.
> 
> This is another entry point for execution on the hw engine group so the
> execution mode is updated.
> 
> Signed-off-by: Francois Dugast <francois.dugast@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_hw_engine.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c b/drivers/gpu/drm/xe/xe_hw_engine.c
> index e6c755a04fd8..dd8ef65cbf2d 100644
> --- a/drivers/gpu/drm/xe/xe_hw_engine.c
> +++ b/drivers/gpu/drm/xe/xe_hw_engine.c
> @@ -431,6 +431,26 @@ hw_engine_setup_default_state(struct xe_hw_engine *hwe)
>  	xe_rtp_process_to_sr(&ctx, engine_entries, &hwe->reg_sr);
>  }
>  
> +static void hw_engine_group_resume_work_func(struct work_struct *w)
> +{
> +	struct xe_exec_queue *q;
> +	struct xe_hw_engine_group *group = container_of(w, struct xe_hw_engine_group, resume_work);
> +	int err;
> +
> +	err = xe_hw_engine_group_get_mode(group, EXEC_MODE_LR);
> +	if (err)
> +		return;
> +

I think techincally if we were previously in EXEC_MODE_LR the resume
loop can be skipped. It is harmless though to call resume twice though.

> +	list_for_each_entry(q, &group->exec_queue_list, hw_engine_group_link) {
> +		if (!xe_vm_in_lr_mode(q->vm))
> +			continue;
> +
> +		q->ops->resume(q);
> +	}
> +
> +	xe_hw_engine_group_put(group);
> +}
> +
>  static struct xe_hw_engine_group *
>  hw_engine_group_alloc(struct xe_device *xe)
>  {
> @@ -438,6 +458,8 @@ hw_engine_group_alloc(struct xe_device *xe)
>  
>  	group = kzalloc(sizeof(*group), GFP_KERNEL);
>  	init_rwsem(&group->mode_sem);
> +	group->resume_wq = alloc_ordered_workqueue("xe-resume-lr-jobs-wq", 0);

I think just 1 non-ordered workqueue for all resumes should be fine. You
will also need to destroy this via drmm on driver unload. 

> +	INIT_WORK(&group->resume_work, hw_engine_group_resume_work_func);
>  	INIT_LIST_HEAD(&group->exec_queue_list);
>  
>  	return group;
> @@ -1235,6 +1257,7 @@ static int xe_hw_engine_group_suspend_lr_jobs(struct xe_hw_engine_group *group)
>  {
>  	int err;
>  	struct xe_exec_queue *q;
> +	bool suspended_queue = false;
>

I think you call this function, it implied that at 1 exec queue is in LR
mode, right?
 
>  	lockdep_assert_held(&group->mode_sem);
>  
> @@ -1247,8 +1270,13 @@ static int xe_hw_engine_group_suspend_lr_jobs(struct xe_hw_engine_group *group)
>  			return err;
>  
>  		q->ops->suspend_wait(q);
> +
> +		suspended_queue = true;
>  	}
>  
> +	if (suspended_queue)

With above, maybe xe_assert(suspended_queue);

Matt

> +		queue_work(group->resume_wq, &group->resume_work);
> +
>  	return 0;
>  }
>  
> -- 
> 2.43.0
> 

Re: [RFC v1 9/9] drm/xe/vm: Remove restriction that all VMs must be faulting if one is

[Matthew Brost]

On Wed, Jul 17, 2024 at 03:07:30PM +0200, Francois Dugast wrote:
> With this restriction, all VMs on the device must be faulting VMs
> if there is already one faulting VM, in which case the device is
> considered in fault mode. This prevents for example an application
> from running 3D jobs for the compositor while submitting a SVM
> compute job on the same device.
> 
> Now that mutual exclusion of LR jobs and dma fence jobs is ensured
> on the hw engine group, remove this restriction to allow running
> faulting and non-faulting VMs on the same device.
> 
> Signed-off-by: Francois Dugast <francois.dugast@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_device.h | 10 ----------
>  drivers/gpu/drm/xe/xe_vm.c     |  8 --------
>  2 files changed, 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_device.h b/drivers/gpu/drm/xe/xe_device.h
> index bb07f5669dbb..cb69ecf02c25 100644
> --- a/drivers/gpu/drm/xe/xe_device.h
> +++ b/drivers/gpu/drm/xe/xe_device.h
> @@ -129,16 +129,6 @@ static inline struct xe_force_wake *gt_to_fw(struct xe_gt *gt)
>  
>  void xe_device_assert_mem_access(struct xe_device *xe);
>  
> -static inline bool xe_device_in_fault_mode(struct xe_device *xe)
> -{
> -	return xe->usm.num_vm_in_fault_mode != 0;
> -}
> -
> -static inline bool xe_device_in_non_fault_mode(struct xe_device *xe)
> -{
> -	return xe->usm.num_vm_in_non_fault_mode != 0;
> -}
> -

These are used in a few other places:

mbrost@lstrano-desk:xe$ grep usm.num_vm_in_ *.c *.h
xe_vm.c:                xe->usm.num_vm_in_fault_mode++;
xe_vm.c:                xe->usm.num_vm_in_non_fault_mode++;
xe_vm.c:                xe->usm.num_vm_in_fault_mode--;
xe_vm.c:                xe->usm.num_vm_in_non_fault_mode--;
xe_device.h:    return xe->usm.num_vm_in_fault_mode != 0;
xe_device.h:    return xe->usm.num_vm_in_non_fault_mode != 0;
xe_device_types.h:              /** @usm.num_vm_in_fault_mode: number of VM in fault mode */
xe_device_types.h:              /** @usm.num_vm_in_non_fault_mode: number of VM in non-fault mode */

Remove this usage everywhere.

Matt

>  static inline bool xe_device_has_flat_ccs(struct xe_device *xe)
>  {
>  	return xe->info.has_flat_ccs;
> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index 5b166fa03684..f0d2dc41a25d 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -1875,14 +1875,6 @@ int xe_vm_create_ioctl(struct drm_device *dev, void *data,
>  			 args->flags & DRM_XE_VM_CREATE_FLAG_FAULT_MODE))
>  		return -EINVAL;
>  
> -	if (XE_IOCTL_DBG(xe, args->flags & DRM_XE_VM_CREATE_FLAG_FAULT_MODE &&
> -			 xe_device_in_non_fault_mode(xe)))
> -		return -EINVAL;
> -
> -	if (XE_IOCTL_DBG(xe, !(args->flags & DRM_XE_VM_CREATE_FLAG_FAULT_MODE) &&
> -			 xe_device_in_fault_mode(xe)))
> -		return -EINVAL;
> -
>  	if (XE_IOCTL_DBG(xe, args->extensions))
>  		return -EINVAL;
>  
> -- 
> 2.43.0
> 

Re: [RFC v1 4/9] drm/xe/hw_engine_group: Add helper to suspend LR jobs

[Matthew Brost]

On Wed, Jul 17, 2024 at 03:07:25PM +0200, Francois Dugast wrote:
> This is a required feature for dma fence jobs to preempt long running
> jobs in order to ensure mutual exclusion on a given hw engine group.
> 

Sorry, keep on missing things.

> Signed-off-by: Francois Dugast <francois.dugast@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_hw_engine.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c b/drivers/gpu/drm/xe/xe_hw_engine.c
> index 4dcc885a55c8..850f7b15b154 100644
> --- a/drivers/gpu/drm/xe/xe_hw_engine.c
> +++ b/drivers/gpu/drm/xe/xe_hw_engine.c
> @@ -1223,3 +1223,31 @@ int xe_hw_engine_group_del_exec_queue(struct xe_hw_engine_group *group, struct x
>  
>  	return 0;
>  }
> +
> +/**
> + * xe_hw_engine_group_suspend_lr_jobs() - Suspend the long running jobs of this hw engine group
> + * @group: The hw engine group
> + *
> + * Return: 0 on success,
> + *	   -EINVAL if one jobs could not be suspended
> + */
> +static int xe_hw_engine_group_suspend_lr_jobs(struct xe_hw_engine_group *group)
> +{
> +	int err;
> +	struct xe_exec_queue *q;
> +
> +	lockdep_assert_held(&group->mode_sem);
> +
> +	list_for_each_entry(q, &group->exec_queue_list, hw_engine_group_link) {
> +		if (!xe_vm_in_lr_mode(q->vm))

if (!xe_vm_in_fault_mode(q->vm))
	continue;

We don't need to suspend LR mode /w preempt fences as the scheduler +
dma-resv + preempt fences does this for us.

Matt

> +			continue;
> +
> +		err = q->ops->suspend(q);
> +		if (err)
> +			return err;
> +
> +		q->ops->suspend_wait(q);
> +	}
> +
> +	return 0;
> +}
> -- 
> 2.43.0
> 

Re: [RFC v1 3/9] drm/xe/hw_engine_group: Register hw engine group's exec queues

[Matthew Brost]

On Wed, Jul 17, 2024 at 03:07:24PM +0200, Francois Dugast wrote:
> Add helpers to safely add and delete the exec queues attached to a hw
> engine group, and make use them at the time of creation and destruction
> of the exec queues. Keeping track of them is required to control the
> execution mode of the hw engine group.
> 

Ugh, missed one thing.

> Signed-off-by: Francois Dugast <francois.dugast@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_exec_queue.c |  7 +++++
>  drivers/gpu/drm/xe/xe_hw_engine.c  | 45 ++++++++++++++++++++++++++++++
>  drivers/gpu/drm/xe/xe_hw_engine.h  |  4 +++
>  3 files changed, 56 insertions(+)
> 
> diff --git a/drivers/gpu/drm/xe/xe_exec_queue.c b/drivers/gpu/drm/xe/xe_exec_queue.c
> index 0ba37835849b..645423a63434 100644
> --- a/drivers/gpu/drm/xe/xe_exec_queue.c
> +++ b/drivers/gpu/drm/xe/xe_exec_queue.c
> @@ -192,6 +192,9 @@ void xe_exec_queue_destroy(struct kref *ref)
>  			xe_exec_queue_put(eq);
>  	}
>  
> +	if (q->vm)
> +		xe_hw_engine_group_del_exec_queue(q->hwe->hw_engine_group, q);
> +
>  	q->ops->fini(q);
>  }
>  
> @@ -640,6 +643,10 @@ int xe_exec_queue_create_ioctl(struct drm_device *dev, void *data,
>  			if (XE_IOCTL_DBG(xe, err))
>  				goto put_exec_queue;
>  		}
> +
> +		err = xe_hw_engine_group_add_exec_queue(q->hwe->hw_engine_group, q);
> +		if (err)
> +			goto put_exec_queue;
>  	}
>  
>  	mutex_lock(&xef->exec_queue.lock);
> diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c b/drivers/gpu/drm/xe/xe_hw_engine.c
> index f8df85d25617..4dcc885a55c8 100644
> --- a/drivers/gpu/drm/xe/xe_hw_engine.c
> +++ b/drivers/gpu/drm/xe/xe_hw_engine.c
> @@ -1178,3 +1178,48 @@ u64 xe_hw_engine_read_timestamp(struct xe_hw_engine *hwe)
>  {
>  	return xe_mmio_read64_2x32(hwe->gt, RING_TIMESTAMP(hwe->mmio_base));
>  }
> +
> +/**
> + * xe_hw_engine_group_add_exec_queue() - Add an exec queue to a hw engine group
> + * @group: The hw engine group
> + * @q: The exec_queue
> + *
> + * Return: 0 on success,
> + *	    -EINTR if the lock could not be acquired
> + */
> +int xe_hw_engine_group_add_exec_queue(struct xe_hw_engine_group *group, struct xe_exec_queue *q)
> +{
> +	int err;
> +
> +	err = down_write_killable(&group->mode_sem);
> +	if (err)
> +		return err;
> +

You need to ensure exec queue in fault mode as in the suspend state if
the group is in dma_fence mode:

if (xe_vm_in_fault_mode(q->vm) && group->cur_mode == EXEC_MODE_DMA_FENCE) {
	q->suspend();
	q->suspend_wait();
	queue_work(resume);
}

Matt

> +	list_add(&q->hw_engine_group_link, &group->exec_queue_list);
> +	up_write(&group->mode_sem);
> +
> +	return 0;
> +}
> +
> +/**
> + * xe_hw_engine_group_del_exec_queue() - Delete an exec queue from a hw engine group
> + * @group: The hw engine group
> + * @q: The exec_queue
> + *
> + * Return: 0 on success,
> + *	    -EINTR if the lock could not be acquired
> + */
> +int xe_hw_engine_group_del_exec_queue(struct xe_hw_engine_group *group, struct xe_exec_queue *q)
> +{
> +	int err;
> +
> +	err = down_write_killable(&group->mode_sem);
> +	if (err)
> +		return err;
> +
> +	if (!list_empty(&group->exec_queue_list))
> +		list_del(&q->hw_engine_group_link);
> +	up_write(&group->mode_sem);
> +
> +	return 0;
> +}
> diff --git a/drivers/gpu/drm/xe/xe_hw_engine.h b/drivers/gpu/drm/xe/xe_hw_engine.h
> index 7f2d27c0ba1a..ce59d83a75ad 100644
> --- a/drivers/gpu/drm/xe/xe_hw_engine.h
> +++ b/drivers/gpu/drm/xe/xe_hw_engine.h
> @@ -9,6 +9,7 @@
>  #include "xe_hw_engine_types.h"
>  
>  struct drm_printer;
> +struct xe_exec_queue;
>  
>  #ifdef CONFIG_DRM_XE_JOB_TIMEOUT_MIN
>  #define XE_HW_ENGINE_JOB_TIMEOUT_MIN CONFIG_DRM_XE_JOB_TIMEOUT_MIN
> @@ -70,4 +71,7 @@ static inline bool xe_hw_engine_is_valid(struct xe_hw_engine *hwe)
>  const char *xe_hw_engine_class_to_str(enum xe_engine_class class);
>  u64 xe_hw_engine_read_timestamp(struct xe_hw_engine *hwe);
>  
> +int xe_hw_engine_group_add_exec_queue(struct xe_hw_engine_group *group, struct xe_exec_queue *q);
> +int xe_hw_engine_group_del_exec_queue(struct xe_hw_engine_group *group, struct xe_exec_queue *q);
> +
>  #endif
> -- 
> 2.43.0
> 

Re: [RFC v1 7/9] drm/xe/exec: Switch hw engine group execution mode upon job submission

[Matthew Brost]

On Wed, Jul 17, 2024 at 10:57:06PM +0000, Matthew Brost wrote:
> On Wed, Jul 17, 2024 at 03:07:28PM +0200, Francois Dugast wrote:
> > Update the current execution mode of the hw engine group which will be
> > used to run the job that is about to be submitted. This triggers the
> > required operations to ensure mutual exclusion of executions modes in
> > this hw engine group.
> > 
> > Signed-off-by: Francois Dugast <francois.dugast@intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_exec.c      | 14 +++++++++++++-
> >  drivers/gpu/drm/xe/xe_hw_engine.c | 13 +++++++++++++
> >  drivers/gpu/drm/xe/xe_hw_engine.h |  2 ++
> >  3 files changed, 28 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_exec.c b/drivers/gpu/drm/xe/xe_exec.c
> > index 2d72cdec3a0b..35418b5d1f5c 100644
> > --- a/drivers/gpu/drm/xe/xe_exec.c
> > +++ b/drivers/gpu/drm/xe/xe_exec.c
> > @@ -14,6 +14,7 @@
> >  #include "xe_bo.h"
> >  #include "xe_device.h"
> >  #include "xe_exec_queue.h"
> > +#include "xe_hw_engine.h"
> >  #include "xe_macros.h"
> >  #include "xe_ring_ops_types.h"
> >  #include "xe_sched_job.h"
> > @@ -124,6 +125,8 @@ int xe_exec_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
> >  	bool write_locked, skip_retry = false;
> >  	ktime_t end = 0;
> >  	int err = 0;
> > +	struct xe_hw_engine_group *group;
> > +	enum xe_hw_engine_group_execution_mode mode;
> >  
> >  	if (XE_IOCTL_DBG(xe, args->extensions) ||
> >  	    XE_IOCTL_DBG(xe, args->pad[0] || args->pad[1] || args->pad[2]) ||
> > @@ -182,6 +185,13 @@ int xe_exec_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
> >  		}
> >  	}
> >  
> > +	group = q->hwe->hw_engine_group;
> > +	mode = xe_hw_engine_group_find_exec_mode(q);
> > +
> 
> So you only need to call xe_hw_engine_group_get_mode if in dma-fence
> mode as LR submissions are allowed in the suspended state. They are just
> held in GuC until submission is enabled.
> 
>

You will have to kick the worker though to resume if in fault mode
though. Kinda a trade off I guess but I'd lean towards just kicking the
worker.

> > +	err = xe_hw_engine_group_get_mode(group, mode);
> > +	if (err)
> > +		goto err_syncs;
> > +
> >  retry:
> >  	if (!xe_vm_in_lr_mode(vm) && xe_vm_userptr_check_repin(vm)) {
> >  		err = down_write_killable(&vm->lock);
> > @@ -199,7 +209,7 @@ int xe_exec_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
> >  		downgrade_write(&vm->lock);
> >  		write_locked = false;
> >  		if (err)
> > -			goto err_unlock_list;
> > +			goto err_hw_exec_mode;
> >  	}
> >  
> >  	if (!args->num_batch_buffer) {
> > @@ -324,6 +334,8 @@ int xe_exec_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
> >  	up_read(&vm->lock);
> >  	if (err == -EAGAIN && !skip_retry)
> >  		goto retry;
> > +err_hw_exec_mode:
> > +	xe_hw_engine_group_put(group);
> >  err_syncs:
> >  	for (i = 0; i < num_syncs; i++)
> >  		xe_sync_entry_cleanup(&syncs[i]);
> > diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c b/drivers/gpu/drm/xe/xe_hw_engine.c
> > index 4f539711357a..e6c755a04fd8 100644
> > --- a/drivers/gpu/drm/xe/xe_hw_engine.c
> > +++ b/drivers/gpu/drm/xe/xe_hw_engine.c
> > @@ -1345,3 +1345,16 @@ void xe_hw_engine_group_put(struct xe_hw_engine_group *group)
> >  {
> >  	up_read(&group->mode_sem);
> >  }
> > +
> > +/**
> > + * xe_hw_engine_group_find_exec_mode() - Find the execution mode for this exec queue
> > + * @q: The exec_queue
> > + */
> > +enum xe_hw_engine_group_execution_mode
> > +xe_hw_engine_group_find_exec_mode(struct xe_exec_queue *q)
> > +{
> > +	if (xe_vm_in_lr_mode(q->vm))
> > +		return EXEC_MODE_LR;
> > +	else
> > +		return EXEC_MODE_DMA_FENCE;
> > +}
> > diff --git a/drivers/gpu/drm/xe/xe_hw_engine.h b/drivers/gpu/drm/xe/xe_hw_engine.h
> > index fce0adf6a7c4..6dfebb18cbb7 100644
> > --- a/drivers/gpu/drm/xe/xe_hw_engine.h
> > +++ b/drivers/gpu/drm/xe/xe_hw_engine.h
> > @@ -76,5 +76,7 @@ int xe_hw_engine_group_del_exec_queue(struct xe_hw_engine_group *group, struct x
> >  int xe_hw_engine_group_get_mode(struct xe_hw_engine_group *group,
> >  				enum xe_hw_engine_group_execution_mode mode);
> >  void xe_hw_engine_group_put(struct xe_hw_engine_group *group);
> > +enum xe_hw_engine_group_execution_mode
> > +xe_hw_engine_group_find_exec_mode(struct xe_exec_queue *q);
> 
> Same comment as previous patch, consider a standalone file for
> xe_hw_engine_group.
> 
> Matt
> 
> >  
> >  #endif
> > -- 
> > 2.43.0
> > 

Re: [RFC v1 1/9] drm/xe/hw_engine_group: Introduce xe_hw_engine_group

[Francois Dugast]

On Wed, Jul 17, 2024 at 07:29:07PM +0000, Matthew Brost wrote:
> On Wed, Jul 17, 2024 at 03:07:22PM +0200, Francois Dugast wrote:
> > A xe_hw_engine_group is a group of hw engines. Two hw engines belong
> > to the same xe_hw_engine_group if one hw engine cannot make progress
> > while the other is stuck on a page fault.
> > 
> > Typically, hw engines of the same group share some resources such as
> > EUs. This depends on the hardware configuration of the platforms.
> > 
> > Currently all engines share EUs so for now only one group is created
> > and assigned to all hw engines.
> > 
> > Signed-off-by: Francois Dugast <francois.dugast@intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_hw_engine.c       | 48 +++++++++++++++++++++++++
> >  drivers/gpu/drm/xe/xe_hw_engine_types.h | 31 ++++++++++++++++
> >  2 files changed, 79 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c b/drivers/gpu/drm/xe/xe_hw_engine.c
> > index 78b50d3a6501..f8df85d25617 100644
> > --- a/drivers/gpu/drm/xe/xe_hw_engine.c
> > +++ b/drivers/gpu/drm/xe/xe_hw_engine.c
> > @@ -431,6 +431,53 @@ hw_engine_setup_default_state(struct xe_hw_engine *hwe)
> >  	xe_rtp_process_to_sr(&ctx, engine_entries, &hwe->reg_sr);
> >  }
> >  
> > +static struct xe_hw_engine_group *
> > +hw_engine_group_alloc(struct xe_device *xe)
> > +{
> > +	struct xe_hw_engine_group *group;
> > +
> > +	group = kzalloc(sizeof(*group), GFP_KERNEL);
> 
> Need to handle kzalloc failing here, e.g.
> 
> if (!group)
> 	return ERR_PTR(-ENOMEM);
> 
> > +	init_rwsem(&group->mode_sem);
> > +	INIT_LIST_HEAD(&group->exec_queue_list);
> > +
> > +	return group;
> > +}
> > +
> > +static void
> 
> This can fail, so:
> 
> static int
> 
> > +hw_engine_setup_groups(struct xe_gt *gt)
> > +{
> > +	struct xe_hw_engine *hwe;
> > +	enum xe_hw_engine_id id;
> > +	struct xe_hw_engine_group *group_rcs_ccs, *group_bcs, *group_vcs_vecs;
> > +	struct xe_device *xe = gt_to_xe(gt);
> > +
> > +	/*
> > +	 * Current partitioning implies all engines share EUs therefore
> > +	 * belong to the same group, so create this group and assign it
> > +	 * to all engines.
> > +	 */
> > +	group_rcs_ccs = hw_engine_group_alloc(xe);
> 
> if (IS_ERR(group_rcs_ccs))
> 	return PTR_ERR(group_rcs_ccs);
> 
> Also you will need to cleanup these allocations on failures and on
> driver unload. For driver unload cleanup use drmm_add_action_or_reset, I
> suggest adding that below.
> 
> > +	group_bcs = hw_engine_group_alloc(xe);
> > +	group_vcs_vecs = hw_engine_group_alloc(xe);
> > +	for_each_hw_engine(hwe, gt, id) {
> > +		switch (hwe->class) {
> > +		case XE_ENGINE_CLASS_COPY:
> > +			hwe->hw_engine_group = group_bcs;
> > +			break;
> > +		case XE_ENGINE_CLASS_RENDER:
> > +		case XE_ENGINE_CLASS_COMPUTE:
> > +			hwe->hw_engine_group = group_rcs_ccs;
> > +			break;
> > +		case XE_ENGINE_CLASS_VIDEO_DECODE:
> > +		case XE_ENGINE_CLASS_VIDEO_ENHANCE:
> > +			hwe->hw_engine_group = group_vcs_vecs;
> > +			break;
> > +		default:
> > +			drm_warn(&xe->drm, "hw engine class not handled");
> 
> I don't think this is a warn as the GSC (XE_ENGINE_CLASS_OTHER) doesn't
> need a group as it is not exposed to user space but pretty sure shows up
> in the for_each_hw_engine loop.
> 
> > +		}
> > +	}
> > +}
> > +
> >  static void hw_engine_init_early(struct xe_gt *gt, struct xe_hw_engine *hwe,
> >  				 enum xe_hw_engine_id id)
> >  {
> > @@ -761,6 +808,7 @@ int xe_hw_engines_init(struct xe_gt *gt)
> >  	}
> >  
> >  	hw_engine_setup_logical_mapping(gt);
> > +	hw_engine_setup_groups(gt);
> 
> err = hw_engine_setup_groups(gt);
> if (err)
> 	return err;
> 
> >  
> >  	return 0;
> 
> return drmm_add_action_or_reset(..., function_to_kfree_groups);
> 
> >  }
> > diff --git a/drivers/gpu/drm/xe/xe_hw_engine_types.h b/drivers/gpu/drm/xe/xe_hw_engine_types.h
> > index 70e6434f150d..c6f7bbcb2a41 100644
> > --- a/drivers/gpu/drm/xe/xe_hw_engine_types.h
> > +++ b/drivers/gpu/drm/xe/xe_hw_engine_types.h
> > @@ -100,6 +100,35 @@ struct xe_hw_engine_class_intf {
> >  	} sched_props, defaults;
> >  };
> >  
> > +/** enum xe_hw_engine_group_execution_mode - possible execution modes of a hw engine group */
> > +enum xe_hw_engine_group_execution_mode {
> > +	EXEC_MODE_LR,
> > +	EXEC_MODE_DMA_FENCE,
> > +};
> > +
> > +/**
> > + * struct xe_hw_engine_group - Hardware engine group
> > + *
> > + * hw engines belong to the same group if they share hardware resources in
> > + * a way that prevents them from making progress when one is stuck on a
> > + * page fault.
> > + */
> > +struct xe_hw_engine_group {
> > +	/** @exec_queue_list: list of exec queues attached to this xe_hw_engine_group */
> > +	struct list_head exec_queue_list;
> > +	/** @resume_work: worker to resume LR exec queues */
> > +	struct work_struct resume_work;
> > +	/** @resume_wq: workqueue to resume LR exec queues */
> > +	struct workqueue_struct *resume_wq;
> 
> Assume these two members (resume_*) are setup later in the series?

Yes they are, in "drm/xe/hw_engine_group: Resume LR exec queues suspended by dma fence jobs".

Francois

> 
> > +	/** @mode_sem: used to protect this group's hardware resources and
> > +	 * ensure mutual exclusion between execution only in LR mode
> > +	 * and execution only in DMA_FENCE mode
> > +	 */
> 
> Prefer this style for multi line kernel doc:
> 
> /**
>  * @foo: some description...
>  */
> 
> Matt
> 
> > +	struct rw_semaphore mode_sem;
> > +	/** @cur_mode: current execution mode of this hw engine group */
> > +	enum xe_hw_engine_group_execution_mode cur_mode;
> > +};
> > +
> >  /**
> >   * struct xe_hw_engine - Hardware engine
> >   *
> > @@ -150,6 +179,8 @@ struct xe_hw_engine {
> >  	struct xe_hw_engine_class_intf *eclass;
> >  	/** @oa_unit: oa unit for this hw engine */
> >  	struct xe_oa_unit *oa_unit;
> > +	/** @hw_engine_group: the group of hw engines this one belongs to */
> > +	struct xe_hw_engine_group *hw_engine_group;
> >  };
> >  
> >  /**
> > -- 
> > 2.43.0
> > 

Re: [RFC v1 3/9] drm/xe/hw_engine_group: Register hw engine group's exec queues

[Francois Dugast]

On Wed, Jul 17, 2024 at 08:09:28PM +0000, Matthew Brost wrote:
> On Wed, Jul 17, 2024 at 03:07:24PM +0200, Francois Dugast wrote:
> > Add helpers to safely add and delete the exec queues attached to a hw
> > engine group, and make use them at the time of creation and destruction
> > of the exec queues. Keeping track of them is required to control the
> > execution mode of the hw engine group.
> > 
> 
> Missed a few more things...
> 
> > Signed-off-by: Francois Dugast <francois.dugast@intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_exec_queue.c |  7 +++++
> >  drivers/gpu/drm/xe/xe_hw_engine.c  | 45 ++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/xe/xe_hw_engine.h  |  4 +++
> >  3 files changed, 56 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_exec_queue.c b/drivers/gpu/drm/xe/xe_exec_queue.c
> > index 0ba37835849b..645423a63434 100644
> > --- a/drivers/gpu/drm/xe/xe_exec_queue.c
> > +++ b/drivers/gpu/drm/xe/xe_exec_queue.c
> > @@ -192,6 +192,9 @@ void xe_exec_queue_destroy(struct kref *ref)
> >  			xe_exec_queue_put(eq);
> >  	}
> >  
> > +	if (q->vm)
> 
> I think this code path can be called GSC exec queues which don't have a
> q->hwe->hw_engine_group. So I think:
> 
> if (q->vm && q->hwe->hw_engine_group)
> 	xe_hw_engine_group_del_exec_queue(q->hwe->hw_engine_group, q);
> 
> > +		xe_hw_engine_group_del_exec_queue(q->hwe->hw_engine_group, q);
> > +
> >  	q->ops->fini(q);
> >  }
> >  
> > @@ -640,6 +643,10 @@ int xe_exec_queue_create_ioctl(struct drm_device *dev, void *data,
> >  			if (XE_IOCTL_DBG(xe, err))
> >  				goto put_exec_queue;
> >  		}
> > +
> > +		err = xe_hw_engine_group_add_exec_queue(q->hwe->hw_engine_group, q);
> 
> So this only called on non-VM bind exec queues. I think techincally this
> wrong as VM bind exec queues can use dma-fences plus the copy engine. I
> plan dropping the copy engine for these [1] and I don't think it worth
> fixing VM bind path with mode switching if we only are going to drop
> this requirement soon. Let me just respin [1] and hopefully we can
> prioritize though reviews so these two series land at roughly the same
> time. 
> 
> [1] https://patchwork.freedesktop.org/patch/582003/?series=125608&rev=5
> 
> > +		if (err)
> > +			goto put_exec_queue;
> >  	}
> >  
> >  	mutex_lock(&xef->exec_queue.lock);
> > diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c b/drivers/gpu/drm/xe/xe_hw_engine.c
> > index f8df85d25617..4dcc885a55c8 100644
> > --- a/drivers/gpu/drm/xe/xe_hw_engine.c
> > +++ b/drivers/gpu/drm/xe/xe_hw_engine.c
> > @@ -1178,3 +1178,48 @@ u64 xe_hw_engine_read_timestamp(struct xe_hw_engine *hwe)
> >  {
> >  	return xe_mmio_read64_2x32(hwe->gt, RING_TIMESTAMP(hwe->mmio_base));
> >  }
> > +
> > +/**
> > + * xe_hw_engine_group_add_exec_queue() - Add an exec queue to a hw engine group
> > + * @group: The hw engine group
> > + * @q: The exec_queue
> > + *
> > + * Return: 0 on success,
> > + *	    -EINTR if the lock could not be acquired
> > + */
> > +int xe_hw_engine_group_add_exec_queue(struct xe_hw_engine_group *group, struct xe_exec_queue *q)
> > +{
> > +	int err;
> > +
> 
> Also assert here this is not a VM bind queue (e.g. EXEC_QUEUE_FLAG_VM is
> clear).
> 
> > +	err = down_write_killable(&group->mode_sem);
> > +	if (err)
> > +		return err;
> > +
> > +	list_add(&q->hw_engine_group_link, &group->exec_queue_list);
> 
> I don't see where INIT_LIST_HEAD is called on group->exec_queue_list. I
> think that should unconditionally be done in __xe_exec_queue_alloc.

A hw engine group's list of exec queue is initialized at the time the group
is setup. This is done in the first patch in the series:
 "drm/xe/hw_engine_group: Introduce xe_hw_engine_group"

Francois

> 
> Matt
> 
> > +	up_write(&group->mode_sem);
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * xe_hw_engine_group_del_exec_queue() - Delete an exec queue from a hw engine group
> > + * @group: The hw engine group
> > + * @q: The exec_queue
> > + *
> > + * Return: 0 on success,
> > + *	    -EINTR if the lock could not be acquired
> > + */
> > +int xe_hw_engine_group_del_exec_queue(struct xe_hw_engine_group *group, struct xe_exec_queue *q)
> > +{
> > +	int err;
> > +
> > +	err = down_write_killable(&group->mode_sem);
> > +	if (err)
> > +		return err;
> > +
> > +	if (!list_empty(&group->exec_queue_list))
> > +		list_del(&q->hw_engine_group_link);
> > +	up_write(&group->mode_sem);
> > +
> > +	return 0;
> > +}
> > diff --git a/drivers/gpu/drm/xe/xe_hw_engine.h b/drivers/gpu/drm/xe/xe_hw_engine.h
> > index 7f2d27c0ba1a..ce59d83a75ad 100644
> > --- a/drivers/gpu/drm/xe/xe_hw_engine.h
> > +++ b/drivers/gpu/drm/xe/xe_hw_engine.h
> > @@ -9,6 +9,7 @@
> >  #include "xe_hw_engine_types.h"
> >  
> >  struct drm_printer;
> > +struct xe_exec_queue;
> >  
> >  #ifdef CONFIG_DRM_XE_JOB_TIMEOUT_MIN
> >  #define XE_HW_ENGINE_JOB_TIMEOUT_MIN CONFIG_DRM_XE_JOB_TIMEOUT_MIN
> > @@ -70,4 +71,7 @@ static inline bool xe_hw_engine_is_valid(struct xe_hw_engine *hwe)
> >  const char *xe_hw_engine_class_to_str(enum xe_engine_class class);
> >  u64 xe_hw_engine_read_timestamp(struct xe_hw_engine *hwe);
> >  
> > +int xe_hw_engine_group_add_exec_queue(struct xe_hw_engine_group *group, struct xe_exec_queue *q);
> > +int xe_hw_engine_group_del_exec_queue(struct xe_hw_engine_group *group, struct xe_exec_queue *q);
> > +
> >  #endif
> > -- 
> > 2.43.0
> > 

Re: [RFC v1 3/9] drm/xe/hw_engine_group: Register hw engine group's exec queues

[Francois Dugast]

On Wed, Jul 17, 2024 at 11:19:13PM +0000, Matthew Brost wrote:
> On Wed, Jul 17, 2024 at 03:07:24PM +0200, Francois Dugast wrote:
> > Add helpers to safely add and delete the exec queues attached to a hw
> > engine group, and make use them at the time of creation and destruction
> > of the exec queues. Keeping track of them is required to control the
> > execution mode of the hw engine group.
> > 
> 
> Ugh, missed one thing.
> 
> > Signed-off-by: Francois Dugast <francois.dugast@intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_exec_queue.c |  7 +++++
> >  drivers/gpu/drm/xe/xe_hw_engine.c  | 45 ++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/xe/xe_hw_engine.h  |  4 +++
> >  3 files changed, 56 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_exec_queue.c b/drivers/gpu/drm/xe/xe_exec_queue.c
> > index 0ba37835849b..645423a63434 100644
> > --- a/drivers/gpu/drm/xe/xe_exec_queue.c
> > +++ b/drivers/gpu/drm/xe/xe_exec_queue.c
> > @@ -192,6 +192,9 @@ void xe_exec_queue_destroy(struct kref *ref)
> >  			xe_exec_queue_put(eq);
> >  	}
> >  
> > +	if (q->vm)
> > +		xe_hw_engine_group_del_exec_queue(q->hwe->hw_engine_group, q);
> > +
> >  	q->ops->fini(q);
> >  }
> >  
> > @@ -640,6 +643,10 @@ int xe_exec_queue_create_ioctl(struct drm_device *dev, void *data,
> >  			if (XE_IOCTL_DBG(xe, err))
> >  				goto put_exec_queue;
> >  		}
> > +
> > +		err = xe_hw_engine_group_add_exec_queue(q->hwe->hw_engine_group, q);
> > +		if (err)
> > +			goto put_exec_queue;
> >  	}
> >  
> >  	mutex_lock(&xef->exec_queue.lock);
> > diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c b/drivers/gpu/drm/xe/xe_hw_engine.c
> > index f8df85d25617..4dcc885a55c8 100644
> > --- a/drivers/gpu/drm/xe/xe_hw_engine.c
> > +++ b/drivers/gpu/drm/xe/xe_hw_engine.c
> > @@ -1178,3 +1178,48 @@ u64 xe_hw_engine_read_timestamp(struct xe_hw_engine *hwe)
> >  {
> >  	return xe_mmio_read64_2x32(hwe->gt, RING_TIMESTAMP(hwe->mmio_base));
> >  }
> > +
> > +/**
> > + * xe_hw_engine_group_add_exec_queue() - Add an exec queue to a hw engine group
> > + * @group: The hw engine group
> > + * @q: The exec_queue
> > + *
> > + * Return: 0 on success,
> > + *	    -EINTR if the lock could not be acquired
> > + */
> > +int xe_hw_engine_group_add_exec_queue(struct xe_hw_engine_group *group, struct xe_exec_queue *q)
> > +{
> > +	int err;
> > +
> > +	err = down_write_killable(&group->mode_sem);
> > +	if (err)
> > +		return err;
> > +
> 
> You need to ensure exec queue in fault mode as in the suspend state if
> the group is in dma_fence mode:
> 
> if (xe_vm_in_fault_mode(q->vm) && group->cur_mode == EXEC_MODE_DMA_FENCE) {
> 	q->suspend();
> 	q->suspend_wait();
> 	queue_work(resume);
> }

This happens later in the series when switching execution mode to EXEC_MODE_DMA_FENCE
at the time of job submission. But this function here is called at the time of exec
queue creation, am I missing a path where the exec queue would be executing before
a job submission happens?

Francois

> 
> Matt
> 
> > +	list_add(&q->hw_engine_group_link, &group->exec_queue_list);
> > +	up_write(&group->mode_sem);
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * xe_hw_engine_group_del_exec_queue() - Delete an exec queue from a hw engine group
> > + * @group: The hw engine group
> > + * @q: The exec_queue
> > + *
> > + * Return: 0 on success,
> > + *	    -EINTR if the lock could not be acquired
> > + */
> > +int xe_hw_engine_group_del_exec_queue(struct xe_hw_engine_group *group, struct xe_exec_queue *q)
> > +{
> > +	int err;
> > +
> > +	err = down_write_killable(&group->mode_sem);
> > +	if (err)
> > +		return err;
> > +
> > +	if (!list_empty(&group->exec_queue_list))
> > +		list_del(&q->hw_engine_group_link);
> > +	up_write(&group->mode_sem);
> > +
> > +	return 0;
> > +}
> > diff --git a/drivers/gpu/drm/xe/xe_hw_engine.h b/drivers/gpu/drm/xe/xe_hw_engine.h
> > index 7f2d27c0ba1a..ce59d83a75ad 100644
> > --- a/drivers/gpu/drm/xe/xe_hw_engine.h
> > +++ b/drivers/gpu/drm/xe/xe_hw_engine.h
> > @@ -9,6 +9,7 @@
> >  #include "xe_hw_engine_types.h"
> >  
> >  struct drm_printer;
> > +struct xe_exec_queue;
> >  
> >  #ifdef CONFIG_DRM_XE_JOB_TIMEOUT_MIN
> >  #define XE_HW_ENGINE_JOB_TIMEOUT_MIN CONFIG_DRM_XE_JOB_TIMEOUT_MIN
> > @@ -70,4 +71,7 @@ static inline bool xe_hw_engine_is_valid(struct xe_hw_engine *hwe)
> >  const char *xe_hw_engine_class_to_str(enum xe_engine_class class);
> >  u64 xe_hw_engine_read_timestamp(struct xe_hw_engine *hwe);
> >  
> > +int xe_hw_engine_group_add_exec_queue(struct xe_hw_engine_group *group, struct xe_exec_queue *q);
> > +int xe_hw_engine_group_del_exec_queue(struct xe_hw_engine_group *group, struct xe_exec_queue *q);
> > +
> >  #endif
> > -- 
> > 2.43.0
> > 

Re: [RFC v1 3/9] drm/xe/hw_engine_group: Register hw engine group's exec queues

[Matthew Brost]

On Mon, Jul 22, 2024 at 10:31:49AM +0200, Francois Dugast wrote:
> On Wed, Jul 17, 2024 at 11:19:13PM +0000, Matthew Brost wrote:
> > On Wed, Jul 17, 2024 at 03:07:24PM +0200, Francois Dugast wrote:
> > > Add helpers to safely add and delete the exec queues attached to a hw
> > > engine group, and make use them at the time of creation and destruction
> > > of the exec queues. Keeping track of them is required to control the
> > > execution mode of the hw engine group.
> > > 
> > 
> > Ugh, missed one thing.
> > 
> > > Signed-off-by: Francois Dugast <francois.dugast@intel.com>
> > > ---
> > >  drivers/gpu/drm/xe/xe_exec_queue.c |  7 +++++
> > >  drivers/gpu/drm/xe/xe_hw_engine.c  | 45 ++++++++++++++++++++++++++++++
> > >  drivers/gpu/drm/xe/xe_hw_engine.h  |  4 +++
> > >  3 files changed, 56 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/xe/xe_exec_queue.c b/drivers/gpu/drm/xe/xe_exec_queue.c
> > > index 0ba37835849b..645423a63434 100644
> > > --- a/drivers/gpu/drm/xe/xe_exec_queue.c
> > > +++ b/drivers/gpu/drm/xe/xe_exec_queue.c
> > > @@ -192,6 +192,9 @@ void xe_exec_queue_destroy(struct kref *ref)
> > >  			xe_exec_queue_put(eq);
> > >  	}
> > >  
> > > +	if (q->vm)
> > > +		xe_hw_engine_group_del_exec_queue(q->hwe->hw_engine_group, q);
> > > +
> > >  	q->ops->fini(q);
> > >  }
> > >  
> > > @@ -640,6 +643,10 @@ int xe_exec_queue_create_ioctl(struct drm_device *dev, void *data,
> > >  			if (XE_IOCTL_DBG(xe, err))
> > >  				goto put_exec_queue;
> > >  		}
> > > +
> > > +		err = xe_hw_engine_group_add_exec_queue(q->hwe->hw_engine_group, q);
> > > +		if (err)
> > > +			goto put_exec_queue;
> > >  	}
> > >  
> > >  	mutex_lock(&xef->exec_queue.lock);
> > > diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c b/drivers/gpu/drm/xe/xe_hw_engine.c
> > > index f8df85d25617..4dcc885a55c8 100644
> > > --- a/drivers/gpu/drm/xe/xe_hw_engine.c
> > > +++ b/drivers/gpu/drm/xe/xe_hw_engine.c
> > > @@ -1178,3 +1178,48 @@ u64 xe_hw_engine_read_timestamp(struct xe_hw_engine *hwe)
> > >  {
> > >  	return xe_mmio_read64_2x32(hwe->gt, RING_TIMESTAMP(hwe->mmio_base));
> > >  }
> > > +
> > > +/**
> > > + * xe_hw_engine_group_add_exec_queue() - Add an exec queue to a hw engine group
> > > + * @group: The hw engine group
> > > + * @q: The exec_queue
> > > + *
> > > + * Return: 0 on success,
> > > + *	    -EINTR if the lock could not be acquired
> > > + */
> > > +int xe_hw_engine_group_add_exec_queue(struct xe_hw_engine_group *group, struct xe_exec_queue *q)
> > > +{
> > > +	int err;
> > > +
> > > +	err = down_write_killable(&group->mode_sem);
> > > +	if (err)
> > > +		return err;
> > > +
> > 
> > You need to ensure exec queue in fault mode as in the suspend state if
> > the group is in dma_fence mode:
> > 
> > if (xe_vm_in_fault_mode(q->vm) && group->cur_mode == EXEC_MODE_DMA_FENCE) {
> > 	q->suspend();
> > 	q->suspend_wait();
> > 	queue_work(resume);
> > }
> 
> This happens later in the series when switching execution mode to EXEC_MODE_DMA_FENCE
> at the time of job submission. But this function here is called at the time of exec
> queue creation, am I missing a path where the exec queue would be executing before
> a job submission happens?
> 

Oh, yea I guess if always switch modes in the exec IOCTL is this
probably wouldn't be needed. It would be needed resume worker is just
kicked in the exec IOCTL.

Conceptually I think it probably best to make all the queues in the
group in the same state though. Having mixed state just seems like a
good way to get into trouble.

Matt

> Francois
> 
> > 
> > Matt
> > 
> > > +	list_add(&q->hw_engine_group_link, &group->exec_queue_list);
> > > +	up_write(&group->mode_sem);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +/**
> > > + * xe_hw_engine_group_del_exec_queue() - Delete an exec queue from a hw engine group
> > > + * @group: The hw engine group
> > > + * @q: The exec_queue
> > > + *
> > > + * Return: 0 on success,
> > > + *	    -EINTR if the lock could not be acquired
> > > + */
> > > +int xe_hw_engine_group_del_exec_queue(struct xe_hw_engine_group *group, struct xe_exec_queue *q)
> > > +{
> > > +	int err;
> > > +
> > > +	err = down_write_killable(&group->mode_sem);
> > > +	if (err)
> > > +		return err;
> > > +
> > > +	if (!list_empty(&group->exec_queue_list))
> > > +		list_del(&q->hw_engine_group_link);
> > > +	up_write(&group->mode_sem);
> > > +
> > > +	return 0;
> > > +}
> > > diff --git a/drivers/gpu/drm/xe/xe_hw_engine.h b/drivers/gpu/drm/xe/xe_hw_engine.h
> > > index 7f2d27c0ba1a..ce59d83a75ad 100644
> > > --- a/drivers/gpu/drm/xe/xe_hw_engine.h
> > > +++ b/drivers/gpu/drm/xe/xe_hw_engine.h
> > > @@ -9,6 +9,7 @@
> > >  #include "xe_hw_engine_types.h"
> > >  
> > >  struct drm_printer;
> > > +struct xe_exec_queue;
> > >  
> > >  #ifdef CONFIG_DRM_XE_JOB_TIMEOUT_MIN
> > >  #define XE_HW_ENGINE_JOB_TIMEOUT_MIN CONFIG_DRM_XE_JOB_TIMEOUT_MIN
> > > @@ -70,4 +71,7 @@ static inline bool xe_hw_engine_is_valid(struct xe_hw_engine *hwe)
> > >  const char *xe_hw_engine_class_to_str(enum xe_engine_class class);
> > >  u64 xe_hw_engine_read_timestamp(struct xe_hw_engine *hwe);
> > >  
> > > +int xe_hw_engine_group_add_exec_queue(struct xe_hw_engine_group *group, struct xe_exec_queue *q);
> > > +int xe_hw_engine_group_del_exec_queue(struct xe_hw_engine_group *group, struct xe_exec_queue *q);
> > > +
> > >  #endif
> > > -- 
> > > 2.43.0
> > > 

Re: [RFC v1 3/9] drm/xe/hw_engine_group: Register hw engine group's exec queues

[Matthew Brost]

On Mon, Jul 22, 2024 at 10:17:06AM +0200, Francois Dugast wrote:
> On Wed, Jul 17, 2024 at 08:09:28PM +0000, Matthew Brost wrote:
> > On Wed, Jul 17, 2024 at 03:07:24PM +0200, Francois Dugast wrote:
> > > Add helpers to safely add and delete the exec queues attached to a hw
> > > engine group, and make use them at the time of creation and destruction
> > > of the exec queues. Keeping track of them is required to control the
> > > execution mode of the hw engine group.
> > > 
> > 
> > Missed a few more things...
> > 
> > > Signed-off-by: Francois Dugast <francois.dugast@intel.com>
> > > ---
> > >  drivers/gpu/drm/xe/xe_exec_queue.c |  7 +++++
> > >  drivers/gpu/drm/xe/xe_hw_engine.c  | 45 ++++++++++++++++++++++++++++++
> > >  drivers/gpu/drm/xe/xe_hw_engine.h  |  4 +++
> > >  3 files changed, 56 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/xe/xe_exec_queue.c b/drivers/gpu/drm/xe/xe_exec_queue.c
> > > index 0ba37835849b..645423a63434 100644
> > > --- a/drivers/gpu/drm/xe/xe_exec_queue.c
> > > +++ b/drivers/gpu/drm/xe/xe_exec_queue.c
> > > @@ -192,6 +192,9 @@ void xe_exec_queue_destroy(struct kref *ref)
> > >  			xe_exec_queue_put(eq);
> > >  	}
> > >  
> > > +	if (q->vm)
> > 
> > I think this code path can be called GSC exec queues which don't have a
> > q->hwe->hw_engine_group. So I think:
> > 
> > if (q->vm && q->hwe->hw_engine_group)
> > 	xe_hw_engine_group_del_exec_queue(q->hwe->hw_engine_group, q);
> > 
> > > +		xe_hw_engine_group_del_exec_queue(q->hwe->hw_engine_group, q);
> > > +
> > >  	q->ops->fini(q);
> > >  }
> > >  
> > > @@ -640,6 +643,10 @@ int xe_exec_queue_create_ioctl(struct drm_device *dev, void *data,
> > >  			if (XE_IOCTL_DBG(xe, err))
> > >  				goto put_exec_queue;
> > >  		}
> > > +
> > > +		err = xe_hw_engine_group_add_exec_queue(q->hwe->hw_engine_group, q);
> > 
> > So this only called on non-VM bind exec queues. I think techincally this
> > wrong as VM bind exec queues can use dma-fences plus the copy engine. I
> > plan dropping the copy engine for these [1] and I don't think it worth
> > fixing VM bind path with mode switching if we only are going to drop
> > this requirement soon. Let me just respin [1] and hopefully we can
> > prioritize though reviews so these two series land at roughly the same
> > time. 
> > 
> > [1] https://patchwork.freedesktop.org/patch/582003/?series=125608&rev=5
> > 
> > > +		if (err)
> > > +			goto put_exec_queue;
> > >  	}
> > >  
> > >  	mutex_lock(&xef->exec_queue.lock);
> > > diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c b/drivers/gpu/drm/xe/xe_hw_engine.c
> > > index f8df85d25617..4dcc885a55c8 100644
> > > --- a/drivers/gpu/drm/xe/xe_hw_engine.c
> > > +++ b/drivers/gpu/drm/xe/xe_hw_engine.c
> > > @@ -1178,3 +1178,48 @@ u64 xe_hw_engine_read_timestamp(struct xe_hw_engine *hwe)
> > >  {
> > >  	return xe_mmio_read64_2x32(hwe->gt, RING_TIMESTAMP(hwe->mmio_base));
> > >  }
> > > +
> > > +/**
> > > + * xe_hw_engine_group_add_exec_queue() - Add an exec queue to a hw engine group
> > > + * @group: The hw engine group
> > > + * @q: The exec_queue
> > > + *
> > > + * Return: 0 on success,
> > > + *	    -EINTR if the lock could not be acquired
> > > + */
> > > +int xe_hw_engine_group_add_exec_queue(struct xe_hw_engine_group *group, struct xe_exec_queue *q)
> > > +{
> > > +	int err;
> > > +
> > 
> > Also assert here this is not a VM bind queue (e.g. EXEC_QUEUE_FLAG_VM is
> > clear).
> > 
> > > +	err = down_write_killable(&group->mode_sem);
> > > +	if (err)
> > > +		return err;
> > > +
> > > +	list_add(&q->hw_engine_group_link, &group->exec_queue_list);
> > 
> > I don't see where INIT_LIST_HEAD is called on group->exec_queue_list. I
> > think that should unconditionally be done in __xe_exec_queue_alloc.
> 
> A hw engine group's list of exec queue is initialized at the time the group
> is setup. This is done in the first patch in the series:
>  "drm/xe/hw_engine_group: Introduce xe_hw_engine_group"
> 

Typo on my end...

I don't see where q->hw_engine_group_link has INIT_LIST_HEAD called.
__xe_exec_queue_alloc seems to be the correct place to do that.

Matt

> Francois
> 
> > 
> > Matt
> > 
> > > +	up_write(&group->mode_sem);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +/**
> > > + * xe_hw_engine_group_del_exec_queue() - Delete an exec queue from a hw engine group
> > > + * @group: The hw engine group
> > > + * @q: The exec_queue
> > > + *
> > > + * Return: 0 on success,
> > > + *	    -EINTR if the lock could not be acquired
> > > + */
> > > +int xe_hw_engine_group_del_exec_queue(struct xe_hw_engine_group *group, struct xe_exec_queue *q)
> > > +{
> > > +	int err;
> > > +
> > > +	err = down_write_killable(&group->mode_sem);
> > > +	if (err)
> > > +		return err;
> > > +
> > > +	if (!list_empty(&group->exec_queue_list))
> > > +		list_del(&q->hw_engine_group_link);
> > > +	up_write(&group->mode_sem);
> > > +
> > > +	return 0;
> > > +}
> > > diff --git a/drivers/gpu/drm/xe/xe_hw_engine.h b/drivers/gpu/drm/xe/xe_hw_engine.h
> > > index 7f2d27c0ba1a..ce59d83a75ad 100644
> > > --- a/drivers/gpu/drm/xe/xe_hw_engine.h
> > > +++ b/drivers/gpu/drm/xe/xe_hw_engine.h
> > > @@ -9,6 +9,7 @@
> > >  #include "xe_hw_engine_types.h"
> > >  
> > >  struct drm_printer;
> > > +struct xe_exec_queue;
> > >  
> > >  #ifdef CONFIG_DRM_XE_JOB_TIMEOUT_MIN
> > >  #define XE_HW_ENGINE_JOB_TIMEOUT_MIN CONFIG_DRM_XE_JOB_TIMEOUT_MIN
> > > @@ -70,4 +71,7 @@ static inline bool xe_hw_engine_is_valid(struct xe_hw_engine *hwe)
> > >  const char *xe_hw_engine_class_to_str(enum xe_engine_class class);
> > >  u64 xe_hw_engine_read_timestamp(struct xe_hw_engine *hwe);
> > >  
> > > +int xe_hw_engine_group_add_exec_queue(struct xe_hw_engine_group *group, struct xe_exec_queue *q);
> > > +int xe_hw_engine_group_del_exec_queue(struct xe_hw_engine_group *group, struct xe_exec_queue *q);
> > > +
> > >  #endif
> > > -- 
> > > 2.43.0
> > > 

Re: [RFC v1 3/9] drm/xe/hw_engine_group: Register hw engine group's exec queues

[Thomas Hellström]

On Wed, 2024-07-17 at 20:09 +0000, Matthew Brost wrote:
> On Wed, Jul 17, 2024 at 03:07:24PM +0200, Francois Dugast wrote:
> > Add helpers to safely add and delete the exec queues attached to a
> > hw
> > engine group, and make use them at the time of creation and
> > destruction
> > of the exec queues. Keeping track of them is required to control
> > the
> > execution mode of the hw engine group.
> > 
> 
> Missed a few more things...
> 
> > Signed-off-by: Francois Dugast <francois.dugast@intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_exec_queue.c |  7 +++++
> >  drivers/gpu/drm/xe/xe_hw_engine.c  | 45
> > ++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/xe/xe_hw_engine.h  |  4 +++
> >  3 files changed, 56 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_exec_queue.c
> > b/drivers/gpu/drm/xe/xe_exec_queue.c
> > index 0ba37835849b..645423a63434 100644
> > --- a/drivers/gpu/drm/xe/xe_exec_queue.c
> > +++ b/drivers/gpu/drm/xe/xe_exec_queue.c
> > @@ -192,6 +192,9 @@ void xe_exec_queue_destroy(struct kref *ref)
> >  			xe_exec_queue_put(eq);
> >  	}
> >  
> > +	if (q->vm)
> 
> I think this code path can be called GSC exec queues which don't have
> a
> q->hwe->hw_engine_group. So I think:
> 
> if (q->vm && q->hwe->hw_engine_group)
> 	xe_hw_engine_group_del_exec_queue(q->hwe->hw_engine_group,
> q);
> 
> > +		xe_hw_engine_group_del_exec_queue(q->hwe-
> > >hw_engine_group, q);
> > +
> >  	q->ops->fini(q);
> >  }
> >  
> > @@ -640,6 +643,10 @@ int xe_exec_queue_create_ioctl(struct
> > drm_device *dev, void *data,
> >  			if (XE_IOCTL_DBG(xe, err))
> >  				goto put_exec_queue;
> >  		}
> > +
> > +		err = xe_hw_engine_group_add_exec_queue(q->hwe-
> > >hw_engine_group, q);
> 
> So this only called on non-VM bind exec queues. I think techincally
> this
> wrong as VM bind exec queues can use dma-fences plus the copy engine.
> I
> plan dropping the copy engine for these [1] and I don't think it
> worth
> fixing VM bind path with mode switching if we only are going to drop
> this requirement soon. Let me just respin [1] and hopefully we can
> prioritize though reviews so these two series land at roughly the
> same
> time. 
> 
> [1]
> https://patchwork.freedesktop.org/patch/582003/?series=125608&rev=5


Matt, Francois

I have expressed concerns a couple of times about ripping out the
capability of gpu binding, mainly for two reasons.

1) I think a pretty common use-case for a bo is
a) clearing/readback
b) binding

If we can have those executed by the same exec_queue then the binding's
dependencies are implicitly resolved and no latency incurred. If
executed by the cpu, we have to add the latency of signalling the
clearing fence and waking up the executing thread.

2) Considering using page-table bos out of non-mappable VRAM.

It's entirely possible these concerns were considered non-valid at some
point but in case they weren't, just a reminder.

How difficult would it be to perform the switching also on these VM
binds?

/Thomas

> 
> > +		if (err)
> > +			goto put_exec_queue;
> >  	}
> >  
> >  	mutex_lock(&xef->exec_queue.lock);
> > diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c
> > b/drivers/gpu/drm/xe/xe_hw_engine.c
> > index f8df85d25617..4dcc885a55c8 100644
> > --- a/drivers/gpu/drm/xe/xe_hw_engine.c
> > +++ b/drivers/gpu/drm/xe/xe_hw_engine.c
> > @@ -1178,3 +1178,48 @@ u64 xe_hw_engine_read_timestamp(struct
> > xe_hw_engine *hwe)
> >  {
> >  	return xe_mmio_read64_2x32(hwe->gt, RING_TIMESTAMP(hwe-
> > >mmio_base));
> >  }
> > +
> > +/**
> > + * xe_hw_engine_group_add_exec_queue() - Add an exec queue to a hw
> > engine group
> > + * @group: The hw engine group
> > + * @q: The exec_queue
> > + *
> > + * Return: 0 on success,
> > + *	    -EINTR if the lock could not be acquired
> > + */
> > +int xe_hw_engine_group_add_exec_queue(struct xe_hw_engine_group
> > *group, struct xe_exec_queue *q)
> > +{
> > +	int err;
> > +
> 
> Also assert here this is not a VM bind queue (e.g. EXEC_QUEUE_FLAG_VM
> is
> clear).
> 
> > +	err = down_write_killable(&group->mode_sem);
> > +	if (err)
> > +		return err;
> > +
> > +	list_add(&q->hw_engine_group_link, &group-
> > >exec_queue_list);
> 
> I don't see where INIT_LIST_HEAD is called on group->exec_queue_list.
> I
> think that should unconditionally be done in __xe_exec_queue_alloc.
> 
> Matt
> 
> > +	up_write(&group->mode_sem);
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * xe_hw_engine_group_del_exec_queue() - Delete an exec queue from
> > a hw engine group
> > + * @group: The hw engine group
> > + * @q: The exec_queue
> > + *
> > + * Return: 0 on success,
> > + *	    -EINTR if the lock could not be acquired
> > + */
> > +int xe_hw_engine_group_del_exec_queue(struct xe_hw_engine_group
> > *group, struct xe_exec_queue *q)
> > +{
> > +	int err;
> > +
> > +	err = down_write_killable(&group->mode_sem);
> > +	if (err)
> > +		return err;
> > +
> > +	if (!list_empty(&group->exec_queue_list))
> > +		list_del(&q->hw_engine_group_link);
> > +	up_write(&group->mode_sem);
> > +
> > +	return 0;
> > +}
> > diff --git a/drivers/gpu/drm/xe/xe_hw_engine.h
> > b/drivers/gpu/drm/xe/xe_hw_engine.h
> > index 7f2d27c0ba1a..ce59d83a75ad 100644
> > --- a/drivers/gpu/drm/xe/xe_hw_engine.h
> > +++ b/drivers/gpu/drm/xe/xe_hw_engine.h
> > @@ -9,6 +9,7 @@
> >  #include "xe_hw_engine_types.h"
> >  
> >  struct drm_printer;
> > +struct xe_exec_queue;
> >  
> >  #ifdef CONFIG_DRM_XE_JOB_TIMEOUT_MIN
> >  #define XE_HW_ENGINE_JOB_TIMEOUT_MIN CONFIG_DRM_XE_JOB_TIMEOUT_MIN
> > @@ -70,4 +71,7 @@ static inline bool xe_hw_engine_is_valid(struct
> > xe_hw_engine *hwe)
> >  const char *xe_hw_engine_class_to_str(enum xe_engine_class class);
> >  u64 xe_hw_engine_read_timestamp(struct xe_hw_engine *hwe);
> >  
> > +int xe_hw_engine_group_add_exec_queue(struct xe_hw_engine_group
> > *group, struct xe_exec_queue *q);
> > +int xe_hw_engine_group_del_exec_queue(struct xe_hw_engine_group
> > *group, struct xe_exec_queue *q);
> > +
> >  #endif
> > -- 
> > 2.43.0
> > 

Re: [RFC v1 3/9] drm/xe/hw_engine_group: Register hw engine group's exec queues

[Matthew Brost]

On Tue, Aug 13, 2024 at 02:24:27PM +0200, Thomas Hellström wrote:
> On Wed, 2024-07-17 at 20:09 +0000, Matthew Brost wrote:
> > On Wed, Jul 17, 2024 at 03:07:24PM +0200, Francois Dugast wrote:
> > > Add helpers to safely add and delete the exec queues attached to a
> > > hw
> > > engine group, and make use them at the time of creation and
> > > destruction
> > > of the exec queues. Keeping track of them is required to control
> > > the
> > > execution mode of the hw engine group.
> > > 
> > 
> > Missed a few more things...
> > 
> > > Signed-off-by: Francois Dugast <francois.dugast@intel.com>
> > > ---
> > >  drivers/gpu/drm/xe/xe_exec_queue.c |  7 +++++
> > >  drivers/gpu/drm/xe/xe_hw_engine.c  | 45
> > > ++++++++++++++++++++++++++++++
> > >  drivers/gpu/drm/xe/xe_hw_engine.h  |  4 +++
> > >  3 files changed, 56 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/xe/xe_exec_queue.c
> > > b/drivers/gpu/drm/xe/xe_exec_queue.c
> > > index 0ba37835849b..645423a63434 100644
> > > --- a/drivers/gpu/drm/xe/xe_exec_queue.c
> > > +++ b/drivers/gpu/drm/xe/xe_exec_queue.c
> > > @@ -192,6 +192,9 @@ void xe_exec_queue_destroy(struct kref *ref)
> > >  			xe_exec_queue_put(eq);
> > >  	}
> > >  
> > > +	if (q->vm)
> > 
> > I think this code path can be called GSC exec queues which don't have
> > a
> > q->hwe->hw_engine_group. So I think:
> > 
> > if (q->vm && q->hwe->hw_engine_group)
> > 	xe_hw_engine_group_del_exec_queue(q->hwe->hw_engine_group,
> > q);
> > 
> > > +		xe_hw_engine_group_del_exec_queue(q->hwe-
> > > >hw_engine_group, q);
> > > +
> > >  	q->ops->fini(q);
> > >  }
> > >  
> > > @@ -640,6 +643,10 @@ int xe_exec_queue_create_ioctl(struct
> > > drm_device *dev, void *data,
> > >  			if (XE_IOCTL_DBG(xe, err))
> > >  				goto put_exec_queue;
> > >  		}
> > > +
> > > +		err = xe_hw_engine_group_add_exec_queue(q->hwe-
> > > >hw_engine_group, q);
> > 
> > So this only called on non-VM bind exec queues. I think techincally
> > this
> > wrong as VM bind exec queues can use dma-fences plus the copy engine.
> > I
> > plan dropping the copy engine for these [1] and I don't think it
> > worth
> > fixing VM bind path with mode switching if we only are going to drop
> > this requirement soon. Let me just respin [1] and hopefully we can
> > prioritize though reviews so these two series land at roughly the
> > same
> > time. 
> > 
> > [1]
> > https://patchwork.freedesktop.org/patch/582003/?series=125608&rev=5
> 
> 
> Matt, Francois
> 
> I have expressed concerns a couple of times about ripping out the
> capability of gpu binding, mainly for two reasons.
> 
> 1) I think a pretty common use-case for a bo is
> a) clearing/readback
> b) binding
> 
> If we can have those executed by the same exec_queue then the binding's

In current design all clearing/readback is done by a kernel migrate
queue. Only kernel binds (page faults, rebinds in exec IOCTL, and rebinds
preempt worker) are done on this queue. 

All user binds on a per VM queue (or a user can create multiple queues
per VM, use case is VK sparse binding). Thus regardless if using GPU or
CPU for user binds, if a user binds depend on a BO move the bind job
will be held in the DRM scheduler until the BO move is complete. When a
job is ready in the scheduler, using the CPU will certainly have lower
latency than the GPU. Lastly, in general I have seen it is fairly easy
overwhelm the GuC with context switching lots of jobs, so anyway we can
relieve pressure their is also a win.

Changing the queue usage model would be pretty invasive and sharing
kernel migrate queue with user binds could create scenarios where kernel
operations are stuck behind unsignaled user input fences. That might
even deadlock, have to think it through. 

> dependencies are implicitly resolved and no latency incurred. If
> executed by the cpu, we have to add the latency of signalling the
> clearing fence and waking up the executing thread.
> 
> 2) Considering using page-table bos out of non-mappable VRAM.
> 
> It's entirely possible these concerns were considered non-valid at some
> point but in case they weren't, just a reminder.
> 

We'd have to rewrite our binding code anyways if pages-tables are
non-mappale VRAM. The CPU is always used to program the private part
page table entries. If dependecies exist, the GPU is used prune the
shared page tables via a job. Our code doesn't support using the GPU
writing the private part, that would basically be a rewrie.

> How difficult would it be to perform the switching also on these VM
> binds?

Not following this part.

Matt

> 
> /Thomas
> 
> > 
> > > +		if (err)
> > > +			goto put_exec_queue;
> > >  	}
> > >  
> > >  	mutex_lock(&xef->exec_queue.lock);
> > > diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c
> > > b/drivers/gpu/drm/xe/xe_hw_engine.c
> > > index f8df85d25617..4dcc885a55c8 100644
> > > --- a/drivers/gpu/drm/xe/xe_hw_engine.c
> > > +++ b/drivers/gpu/drm/xe/xe_hw_engine.c
> > > @@ -1178,3 +1178,48 @@ u64 xe_hw_engine_read_timestamp(struct
> > > xe_hw_engine *hwe)
> > >  {
> > >  	return xe_mmio_read64_2x32(hwe->gt, RING_TIMESTAMP(hwe-
> > > >mmio_base));
> > >  }
> > > +
> > > +/**
> > > + * xe_hw_engine_group_add_exec_queue() - Add an exec queue to a hw
> > > engine group
> > > + * @group: The hw engine group
> > > + * @q: The exec_queue
> > > + *
> > > + * Return: 0 on success,
> > > + *	    -EINTR if the lock could not be acquired
> > > + */
> > > +int xe_hw_engine_group_add_exec_queue(struct xe_hw_engine_group
> > > *group, struct xe_exec_queue *q)
> > > +{
> > > +	int err;
> > > +
> > 
> > Also assert here this is not a VM bind queue (e.g. EXEC_QUEUE_FLAG_VM
> > is
> > clear).
> > 
> > > +	err = down_write_killable(&group->mode_sem);
> > > +	if (err)
> > > +		return err;
> > > +
> > > +	list_add(&q->hw_engine_group_link, &group-
> > > >exec_queue_list);
> > 
> > I don't see where INIT_LIST_HEAD is called on group->exec_queue_list.
> > I
> > think that should unconditionally be done in __xe_exec_queue_alloc.
> > 
> > Matt
> > 
> > > +	up_write(&group->mode_sem);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +/**
> > > + * xe_hw_engine_group_del_exec_queue() - Delete an exec queue from
> > > a hw engine group
> > > + * @group: The hw engine group
> > > + * @q: The exec_queue
> > > + *
> > > + * Return: 0 on success,
> > > + *	    -EINTR if the lock could not be acquired
> > > + */
> > > +int xe_hw_engine_group_del_exec_queue(struct xe_hw_engine_group
> > > *group, struct xe_exec_queue *q)
> > > +{
> > > +	int err;
> > > +
> > > +	err = down_write_killable(&group->mode_sem);
> > > +	if (err)
> > > +		return err;
> > > +
> > > +	if (!list_empty(&group->exec_queue_list))
> > > +		list_del(&q->hw_engine_group_link);
> > > +	up_write(&group->mode_sem);
> > > +
> > > +	return 0;
> > > +}
> > > diff --git a/drivers/gpu/drm/xe/xe_hw_engine.h
> > > b/drivers/gpu/drm/xe/xe_hw_engine.h
> > > index 7f2d27c0ba1a..ce59d83a75ad 100644
> > > --- a/drivers/gpu/drm/xe/xe_hw_engine.h
> > > +++ b/drivers/gpu/drm/xe/xe_hw_engine.h
> > > @@ -9,6 +9,7 @@
> > >  #include "xe_hw_engine_types.h"
> > >  
> > >  struct drm_printer;
> > > +struct xe_exec_queue;
> > >  
> > >  #ifdef CONFIG_DRM_XE_JOB_TIMEOUT_MIN
> > >  #define XE_HW_ENGINE_JOB_TIMEOUT_MIN CONFIG_DRM_XE_JOB_TIMEOUT_MIN
> > > @@ -70,4 +71,7 @@ static inline bool xe_hw_engine_is_valid(struct
> > > xe_hw_engine *hwe)
> > >  const char *xe_hw_engine_class_to_str(enum xe_engine_class class);
> > >  u64 xe_hw_engine_read_timestamp(struct xe_hw_engine *hwe);
> > >  
> > > +int xe_hw_engine_group_add_exec_queue(struct xe_hw_engine_group
> > > *group, struct xe_exec_queue *q);
> > > +int xe_hw_engine_group_del_exec_queue(struct xe_hw_engine_group
> > > *group, struct xe_exec_queue *q);
> > > +
> > >  #endif
> > > -- 
> > > 2.43.0
> > > 
>