[PATCH] drm/xe: kill the secondary bind queues on multi-GT teardown

Ali Ahmet Memis <[email protected]>
Newsgroups org.freedesktop.lists.dri-devel,org.freedesktop.lists.intel-xe,org.kernel.vger.linux-kernel,org.kernel.vger.stable
Message-ID <[email protected]>
xe_exec_queue_kill() walks the multi-GT list with its own cursor as the
list head:

	struct xe_exec_queue *eq = q, *next;

	list_for_each_entry_safe(eq, next, &eq->multi_gt_list,
				 multi_gt_link) {

The head expression is re-evaluated on every loop condition, and by then
eq is the cursor rather than q. multi_gt_list and multi_gt_link are two
names for the same member:

	union {
		struct list_head multi_gt_list;
		struct list_head multi_gt_link;
	};

so list_entry_is_head() compares &eq->multi_gt_link with
&eq->multi_gt_list, which is the same address, and the loop stops before
its first iteration. Modelling the layout and the list macros in a small
program shows it plainly:

	head = &eq->multi_gt_list : 0 secondary queue(s) visited
	head = &q->multi_gt_list  : 2 secondary queue(s) visited

For a VM bind queue on a multi-tile device, xe_exec_queue_create_ioctl()
creates one queue per tile and links the others behind the first, so the
loop is what is meant to stop them. Nothing else does: the two calls
after the loop only cover the primary. The secondaries are still freed,
because __xe_exec_queue_free() walks the same list with &q->multi_gt_list
and drops their references, so they go away without q->ops->kill() ever
running and without being removed from the VM with
xe_vm_remove_compute_exec_queue().

Use the primary as the head, the way the other two walks of this list in
the driver already do, in __xe_exec_queue_free() and in
xe_sync_entry_add_deps().

Single tile devices are unaffected, the list is empty there.

I have no multi-tile device, so this is from reading the code and the
program above rather than measured on hardware.

Fixes: dd08ebf6c352 ("drm/xe: Introduce a new DRM driver for Intel GPUs")
Cc: [email protected]
Signed-off-by: Ali Ahmet Memis <[email protected]>
---
The program the commit message refers to, in case it is useful. It uses
the same union layout and the list macros from include/linux/list.h:

  struct xe_exec_queue {
	int id;
	union {
		struct list_head multi_gt_list;
		struct list_head multi_gt_link;
	};
  };

  eq = &prim;
  list_for_each_entry_safe(eq, next, &eq->multi_gt_list, multi_gt_link)
	n++;			/* n == 0 */

  list_for_each_entry_safe(eq, next, &prim.multi_gt_list, multi_gt_link)
	n++;			/* n == 2 */

Compile tested only, I have no Xe device.
---
 drivers/gpu/drm/xe/xe_exec_queue.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_exec_queue.c b/drivers/gpu/drm/xe/xe_exec_queue.c
index 1b5ca3ce578a..85a167857159 100644
--- a/drivers/gpu/drm/xe/xe_exec_queue.c
+++ b/drivers/gpu/drm/xe/xe_exec_queue.c
@@ -1571,9 +1571,9 @@ void xe_exec_queue_update_run_ticks(struct xe_exec_queue *q)
  */
 void xe_exec_queue_kill(struct xe_exec_queue *q)
 {
-	struct xe_exec_queue *eq = q, *next;
+	struct xe_exec_queue *eq, *next;
 
-	list_for_each_entry_safe(eq, next, &eq->multi_gt_list,
+	list_for_each_entry_safe(eq, next, &q->multi_gt_list,
 				 multi_gt_link) {
 		q->ops->kill(eq);
 		xe_vm_remove_compute_exec_queue(q->vm, eq);

base-commit: 0d839570765118029aa8bf4a95444c6a11aacf85
-- 
2.55.0
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.