drm/xe: Rework rebinding and allow same-vm eviction

For each rebind we insert a GuC TLB invalidation and add a corresponding unordered TLB invalidation fence. This might add a huge number of TLB invalidation fences to wait for so rather than doing that, defer the TLB invalidation to the next ring ops for each affected exec queue. Since the TLB is invalidated on exec_queue switch, we need to invalidate once for each affected exec_queue.

Fixes 5387e865d90e ("drm/xe: Add TLB invalidation fence after rebinds issued from execs")

Cc Matthew Brost <[email protected]>

Cc <[email protected]> # v6.8+

Signed-off-by Thomas Hellström <[email protected]>

---
 drivers/gpu/drm/xe/xe_exec_queue_types.h |  2 ++
 drivers/gpu/drm/xe/xe_pt.c               |  5 +++--
 drivers/gpu/drm/xe/xe_ring_ops.c         | 11 ++++-------
 drivers/gpu/drm/xe/xe_sched_job.c        | 11 +++++++++++
 drivers/gpu/drm/xe/xe_sched_job_types.h  |  2 ++
 drivers/gpu/drm/xe/xe_vm_types.h         |  5 +++++
 6 files changed, 27 insertions(+), 9 deletions(-)

drivers/gpu/drm/xe/xe_exec_queue_types.h +2 -0

index 62b3d9d1d7cd..891ad30e906f 100644
@@ -148,6 +148,8 @@ struct xe_exec_queue {
 	const struct xe_ring_ops *ring_ops;
 	/** @entity: DRM sched entity for this exec queue (1 to 1 relationship) */
 	struct drm_sched_entity *entity;
+	/** @tlb_flush_seqno: The seqno of the last rebind tlb flush performed */
+	u64 tlb_flush_seqno;
 	/** @lrc: logical ring context for this exec queue */
 	struct xe_lrc lrc[];
 };

drivers/gpu/drm/xe/xe_pt.c +0 -0

index 8d3922d2206e..21bc0d13fccf 100644

drivers/gpu/drm/xe/xe_pt.c +0 -0

 	}
 
 	rfence = kzalloc(sizeof(*rfence), GFP_KERNEL);

drivers/gpu/drm/xe/xe_ring_ops.c +4 -7

index c4edffcd4a32..5b2b37b59813 100644
@@ -219,10 +219,9 @@ static void __emit_job_gen12_simple(struct xe_sched_job *job, struct xe_lrc *lrc
 {
 	u32 dw[MAX_JOB_SIZE_DW], i = 0;
 	u32 ppgtt_flag = get_ppgtt_flag(job);
-	struct xe_vm *vm = job->q->vm;
 	struct xe_gt *gt = job->q->gt;
 
-	if (vm && vm->batch_invalidate_tlb) {
+	if (job->ring_ops_flush_tlb) {
 		dw[i++] = preparser_disable(true);
 		i = emit_flush_imm_ggtt(xe_lrc_start_seqno_ggtt_addr(lrc),
 					seqno, true, dw, i);

@@ -270,7 +269,6 @@ static void __emit_job_gen12_video(struct xe_sched_job *job, struct xe_lrc *lrc,
 	struct xe_gt *gt = job->q->gt;
 	struct xe_device *xe = gt_to_xe(gt);
 	bool decode = job->q->class == XE_ENGINE_CLASS_VIDEO_DECODE;
-	struct xe_vm *vm = job->q->vm;
 
 	dw[i++] = preparser_disable(true);
 

@@ -282,13 +280,13 @@ static void __emit_job_gen12_video(struct xe_sched_job *job, struct xe_lrc *lrc,
 			i = emit_aux_table_inv(gt, VE0_AUX_INV, dw, i);
 	}
 
-	if (vm && vm->batch_invalidate_tlb)
+	if (job->ring_ops_flush_tlb)
 		i = emit_flush_imm_ggtt(xe_lrc_start_seqno_ggtt_addr(lrc),
 					seqno, true, dw, i);
 
 	dw[i++] = preparser_disable(false);
 
-	if (!vm || !vm->batch_invalidate_tlb)
+	if (!job->ring_ops_flush_tlb)
 		i = emit_store_imm_ggtt(xe_lrc_start_seqno_ggtt_addr(lrc),
 					seqno, dw, i);
 

@@ -317,7 +315,6 @@ static void __emit_job_gen12_render_compute(struct xe_sched_job *job,
 	struct xe_gt *gt = job->q->gt;
 	struct xe_device *xe = gt_to_xe(gt);
 	bool lacks_render = !(gt->info.engine_mask & XE_HW_ENGINE_RCS_MASK);
-	struct xe_vm *vm = job->q->vm;
 	u32 mask_flags = 0;
 
 	dw[i++] = preparser_disable(true);

@@ -327,7 +324,7 @@ static void __emit_job_gen12_render_compute(struct xe_sched_job *job,
 		mask_flags = PIPE_CONTROL_3D_ENGINE_FLAGS;
 
 	/* See __xe_pt_bind_vma() for a discussion on TLB invalidations. */
-	i = emit_pipe_invalidate(mask_flags, vm && vm->batch_invalidate_tlb, dw, i);
+	i = emit_pipe_invalidate(mask_flags, job->ring_ops_flush_tlb, dw, i);
 
 	/* hsdes: 1809175790 */
 	if (has_aux_ccs(xe))

drivers/gpu/drm/xe/xe_sched_job.c +1 -0

index 8151ddafb940..d55458d915a9 100644
@@ -250,6 +250,17 @@ bool xe_sched_job_completed(struct xe_sched_job *job)
 
 void xe_sched_job_arm(struct xe_sched_job *job)
 {
+	struct xe_exec_queue *q = job->q;

drivers/gpu/drm/xe/xe_sched_job_types.h +0 -0

 	/** @batch_addr: batch buffer address of job */
 	u64 batch_addr[];
 };

drivers/gpu/drm/xe/xe_vm_types.h +5 -0

index ae5fb565f6bf..5747f136d24d 100644
@@ -264,6 +264,11 @@ struct xe_vm {
 		bool capture_once;
 	} error_capture;
 
+	/**
+	 * @tlb_flush_seqno: Required TLB flush seqno for the next exec.
+	 * protected by the vm resv.
+	 */
+	u64 tlb_flush_seqno;
 	/** @batch_invalidate_tlb: Always invalidate TLB before batch start */
 	bool batch_invalidate_tlb;
 	/** @xef: XE file handle for tracking this VM's drm client */

Signature · 1 line

2.44.0

Quoted 54 lines · Thomas Hellström

For each rebind we insert a GuC TLB invalidation and add a corresponding unordered TLB invalidation fence. This might add a huge number of TLB invalidation fences to wait for so rather than doing that, defer the TLB invalidation to the next ring ops for each affected exec queue. Since the TLB is invalidated on exec_queue switch, we need to invalidate once for each affected exec_queue.

Fixes 5387e865d90e ("drm/xe: Add TLB invalidation fence after

rebinds issued from execs")

Cc Matthew Brost <[email protected]>

Cc <[email protected]> # v6.8+

Signed-off-by Thomas Hellström <[email protected]>

---
 drivers/gpu/drm/xe/xe_exec_queue_types.h |  2 ++ drivers/gpu/drm/xe/xe_pt.c               |  5 +++-- drivers/gpu/drm/xe/xe_ring_ops.c         | 11 ++++------- drivers/gpu/drm/xe/xe_sched_job.c        | 11 +++++++++++ drivers/gpu/drm/xe/xe_sched_job_types.h  |  2 ++ drivers/gpu/drm/xe/xe_vm_types.h         |  5 +++++

 6 files changed, 27 insertions(+), 9 deletions(-)

drivers/gpu/drm/xe/xe_exec_queue_types.h +0 -0

index 62b3d9d1d7cd..891ad30e906f 100644
@@ -148,6 +148,8 @@ struct xe_exec_queue {
  	const struct xe_ring_ops *ring_ops;
  	/** @entity: DRM sched entity for this exec queue (1 to 1

relationship) */
  struct drm_sched_entity *entity;

drivers/gpu/drm/xe/xe_exec_queue_types.h +1 -0

+	/** @tlb_flush_seqno: The seqno of the last rebind tlb

flush performed */

drivers/gpu/drm/xe/xe_exec_queue_types.h +1 -0

+	u64 tlb_flush_seqno;

  /** @lrc: logical ring context for this exec queue */ struct xe_lrc lrc[];
 };

drivers/gpu/drm/xe/xe_pt.c +0 -0

index 8d3922d2206e..21bc0d13fccf 100644
@@ -1254,11 +1254,12 @@ __xe_pt_bind_vma(struct xe_tile *tile,

struct xe_vma *vma, struct xe_exec_queue
  * non-faulting LR, in particular on user-space batch buffer chaining,
  * it needs to be done here.
  */

drivers/gpu/drm/xe/xe_pt.c +0 -1

-	if ((rebind && !xe_vm_in_lr_mode(vm) && !vm-

Quoted 1 line

batch_invalidate_tlb) ||

drivers/gpu/drm/xe/xe_pt.c +0 -1

-	    (!rebind && xe_vm_has_scratch(vm) &&

xe_vm_in_preempt_fence_mode(vm))) {

drivers/gpu/drm/xe/xe_pt.c +1 -0

+	if ((!rebind && xe_vm_has_scratch(vm) &&

Quoted 1 line

xe_vm_in_preempt_fence_mode(vm))) {

Looked why this works in fault mode, we disallow scratch page in fault
mode. I thought at one point we had implementation for that [1] but it
looks like it never got merged. Some to keep an eye on.

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

drivers/gpu/drm/xe/xe_pt.c __xe_pt_bind_vma(struct xe_tile *tile, struct xe_vma *vma, struct xe_exec_queue · line 1262

Quoted 6 lines

  ifence = kzalloc(sizeof(*ifence), GFP_KERNEL); if (!ifence)
  return ERR_PTR(-ENOMEM);

drivers/gpu/drm/xe/xe_pt.c +1 -0

+	} else if (rebind && !xe_vm_in_lr_mode(vm) && !vm-

Quoted 1 line

batch_invalidate_tlb) {

drivers/gpu/drm/xe/xe_pt.c +1 -0

+		vm->tlb_flush_seqno++;

Can we unwind this if / else clause a bit?

I think batch_invalidate_tlb can only be true if
!xe_vm_in_lr_mode(vm).

So else if 'rebind && !xe_vm_in_lr_mode(vm)' should work. Also if batch_invalidate_tlb is we true we always issue TLB invalidate anyways
and incrementing the seqno is harmles too.

Side note, I'd be remiss if I didn't mention that I really do not like
updating these functions (__xe_pt_bind_vma / __xe_pt_unbind_vma) as they
are going away / being reworked here [2] in order to implement 1 job per
IOCTL / proper error handling.

[2] https://patchwork.freedesktop.org/series/125608/

Quoted 91 lines

  }

  rfence = kzalloc(sizeof(*rfence), GFP_KERNEL);

drivers/gpu/drm/xe/xe_ring_ops.c +0 -0

index c4edffcd4a32..5b2b37b59813 100644
@@ -219,10 +219,9 @@ static void __emit_job_gen12_simple(struct

xe_sched_job *job, struct xe_lrc *lrc
 {
  u32 dw[MAX_JOB_SIZE_DW], i = 0;
  u32 ppgtt_flag = get_ppgtt_flag(job);

drivers/gpu/drm/xe/xe_ring_ops.c +0 -1

-	struct xe_vm *vm = job->q->vm;

  struct xe_gt *gt = job->q->gt;

drivers/gpu/drm/xe/xe_ring_ops.c +1 -1

-	if (vm && vm->batch_invalidate_tlb) {
+	if (job->ring_ops_flush_tlb) {

  dw[i++] = preparser_disable(true); i = emit_flush_imm_ggtt(xe_lrc_start_seqno_ggtt_addr(lrc), seqno, true, dw, i);

drivers/gpu/drm/xe/xe_ring_ops.c +0 -0

@@ -270,7 +269,6 @@ static void __emit_job_gen12_video(struct
 xe_sched_job *job, struct xe_lrc *lrc,
  	struct xe_gt *gt = job->q->gt;
  	struct xe_device *xe = gt_to_xe(gt);

  bool decode = job->q->class ==
XE_ENGINE_CLASS_VIDEO_DECODE;

drivers/gpu/drm/xe/xe_ring_ops.c +0 -1

-	struct xe_vm *vm = job->q->vm;

  dw[i++] = preparser_disable(true);

drivers/gpu/drm/xe/xe_ring_ops.c +0 -0

@@ -282,13 +280,13 @@ static void __emit_job_gen12_video(struct
 xe_sched_job *job, struct xe_lrc *lrc,
  			i = emit_aux_table_inv(gt, VE0_AUX_INV,
 dw, i);
  	}

drivers/gpu/drm/xe/xe_ring_ops.c +1 -1

-	if (vm && vm->batch_invalidate_tlb)
+	if (job->ring_ops_flush_tlb)

  i = emit_flush_imm_ggtt(xe_lrc_start_seqno_ggtt_addr(lrc), seqno, true, dw, i);
  dw[i++] = preparser_disable(false);

drivers/gpu/drm/xe/xe_ring_ops.c +1 -1

-	if (!vm || !vm->batch_invalidate_tlb)
+	if (!job->ring_ops_flush_tlb)

  i = emit_store_imm_ggtt(xe_lrc_start_seqno_ggtt_addr(lrc), seqno, dw, i);

drivers/gpu/drm/xe/xe_ring_ops.c +0 -0

@@ -317,7 +315,6 @@ static void
 __emit_job_gen12_render_compute(struct xe_sched_job *job,
  	struct xe_gt *gt = job->q->gt;
  	struct xe_device *xe = gt_to_xe(gt);

  bool lacks_render = !(gt->info.engine_mask & XE_HW_ENGINE_RCS_MASK);

drivers/gpu/drm/xe/xe_ring_ops.c +0 -1

-	struct xe_vm *vm = job->q->vm;

  u32 mask_flags = 0;

  dw[i++] = preparser_disable(true);

drivers/gpu/drm/xe/xe_ring_ops.c +0 -0

@@ -327,7 +324,7 @@ static void
 __emit_job_gen12_render_compute(struct xe_sched_job *job,
  		mask_flags = PIPE_CONTROL_3D_ENGINE_FLAGS;

  /* See __xe_pt_bind_vma() for a discussion on TLB invalidations. */

drivers/gpu/drm/xe/xe_ring_ops.c +0 -1

-	i = emit_pipe_invalidate(mask_flags, vm && vm-

Quoted 1 line

batch_invalidate_tlb, dw, i);

drivers/gpu/drm/xe/xe_ring_ops.c +1 -0

+	i = emit_pipe_invalidate(mask_flags, job-

Quoted 1 line

ring_ops_flush_tlb, dw, i);

  /* hsdes: 1809175790 */
  if (has_aux_ccs(xe))

drivers/gpu/drm/xe/xe_sched_job.c +0 -0

index 8151ddafb940..d55458d915a9 100644
@@ -250,6 +250,17 @@ bool xe_sched_job_completed(struct
 xe_sched_job *job)

 void xe_sched_job_arm(struct xe_sched_job *job)
 {

drivers/gpu/drm/xe/xe_sched_job.c +4 -0

+	struct xe_exec_queue *q = job->q;
+	struct xe_vm *vm = q->vm;
+
+	if (vm && !xe_sched_job_is_migration(q) &&

!xe_vm_in_lr_mode(vm) &&

drivers/gpu/drm/xe/xe_sched_job.c bool xe_sched_job_completed(struct xe_sched_job *job) · line 263

Quoted 7 lines

drivers/gpu/drm/xe/xe_sched_job.c +7 -0

+	    vm->tlb_flush_seqno != q->tlb_flush_seqno) {
+		q->tlb_flush_seqno = vm->tlb_flush_seqno;
+		job->ring_ops_flush_tlb = true;
+	} else if (vm && vm->batch_invalidate_tlb) {
+		job->ring_ops_flush_tlb = true;
+	}
+

Can we simplify this too?

	if (vm && (vm->batch_invalidate_tlb || (vm->tlb_flush_seqno
!= q->tlb_flush_seqno))) {
		q->tlb_flush_seqno = vm->tlb_flush_seqno;
		job->ring_ops_flush_tlb = true;
	}

I think this works as xe_sched_job_is_migration has emit_migration_job_gen12 which doesn't look at job-

Quoted 1 line

ring_ops_flush_tlb,

so no need to xe_sched_job_is_migration.

Also no need to check xe_vm_in_lr_mode as we wouldn'y increment the seqno above if that true.

Lastly, harmless to increment q->tlb_flush_seqno in the case of batch_invalidate_tlb being true.

Quoted 15 lines

  drm_sched_job_arm(&job->drm);
 }

drivers/gpu/drm/xe/xe_sched_job_types.h +0 -0

index b1d83da50a53..5e12724219fd 100644
@@ -39,6 +39,8 @@ struct xe_sched_job {
  	} user_fence;
  	/** @migrate_flush_flags: Additional flush flags for

migration jobs */
  u32 migrate_flush_flags;

drivers/gpu/drm/xe/xe_sched_job_types.h +1 -0

+	/** @ring_ops_flush_tlb: The ring ops need to flush TLB

before payload. */

drivers/gpu/drm/xe/xe_sched_job_types.h struct xe_sched_job { · line 43

Quoted 1 line

drivers/gpu/drm/xe/xe_sched_job_types.h +1 -0

+	bool ring_ops_flush_tlb;

How about JOB_FLAG_FLUSH_TLB rather than a new field? See JOB_FLAG_SUBMIT flag usage.

Matt

Quoted 26 lines

  /** @batch_addr: batch buffer address of job */ u64 batch_addr[];
 };

drivers/gpu/drm/xe/xe_vm_types.h +0 -0

index ae5fb565f6bf..5747f136d24d 100644
@@ -264,6 +264,11 @@ struct xe_vm {
  		bool capture_once;
  	} error_capture;

drivers/gpu/drm/xe/xe_vm_types.h +2 -0

+	/**
+	 * @tlb_flush_seqno: Required TLB flush seqno for the next

exec.

drivers/gpu/drm/xe/xe_vm_types.h +3 -0

+	 * protected by the vm resv.
+	 */
+	u64 tlb_flush_seqno;

  /** @batch_invalidate_tlb: Always invalidate TLB before batch start */
  bool batch_invalidate_tlb;
  /** @xef: XE file handle for tracking this VM's drm client */

Signature · 1 line

2.44.0

Hi, Matthew,

Thanks for reviewing, please see inline.

Quoted 45 lines · Matthew Brost

Quoted 44 lines · Thomas Hellström

For each rebind we insert a GuC TLB invalidation and add a corresponding unordered TLB invalidation fence. This might add a huge number of TLB invalidation fences to wait for so rather than doing that, defer the TLB invalidation to the next ring ops for each affected exec queue. Since the TLB is invalidated on exec_queue switch, we need to invalidate once for each affected exec_queue.

Fixes 5387e865d90e ("drm/xe: Add TLB invalidation fence after

rebinds issued from execs")
Cc: Matthew Brost <[email protected]>
Cc: <[email protected]> # v6.8+
Signed-off-by: Thomas Hellström <[email protected]>
---
 drivers/gpu/drm/xe/xe_exec_queue_types.h |  2 ++ drivers/gpu/drm/xe/xe_pt.c               |  5 +++-- drivers/gpu/drm/xe/xe_ring_ops.c         | 11 ++++------- drivers/gpu/drm/xe/xe_sched_job.c        | 11 +++++++++++ drivers/gpu/drm/xe/xe_sched_job_types.h  |  2 ++ drivers/gpu/drm/xe/xe_vm_types.h         |  5 +++++

 6 files changed, 27 insertions(+), 9 deletions(-)

drivers/gpu/drm/xe/xe_exec_queue_types.h +0 -0

index 62b3d9d1d7cd..891ad30e906f 100644
@@ -148,6 +148,8 @@ struct xe_exec_queue {
  	const struct xe_ring_ops *ring_ops;
  	/** @entity: DRM sched entity for this exec queue (1 to

1
relationship) */
  struct drm_sched_entity *entity;

drivers/gpu/drm/xe/xe_exec_queue_types.h +1 -0

+	/** @tlb_flush_seqno: The seqno of the last rebind tlb

flush performed */

drivers/gpu/drm/xe/xe_exec_queue_types.h +1 -0

+	u64 tlb_flush_seqno;

  /** @lrc: logical ring context for this exec queue */ struct xe_lrc lrc[];
 };

drivers/gpu/drm/xe/xe_pt.c +0 -1

index 8d3922d2206e..21bc0d13fccf 100644
--- a/drivers/gpu/drm/xe/xe_pt.c

Quoted 8 lines

drivers/gpu/drm/xe/xe_pt.c

Quoted 8 lines

+++ b/drivers/gpu/drm/xe/xe_pt.c

drivers/gpu/drm/xe/xe_pt.c +0 -0

@@ -1254,11 +1254,12 @@ __xe_pt_bind_vma(struct xe_tile *tile,

struct xe_vma *vma, struct xe_exec_queue
  * non-faulting LR, in particular on user-space batch buffer chaining,
  * it needs to be done here.
  */

drivers/gpu/drm/xe/xe_pt.c +0 -1

-	if ((rebind && !xe_vm_in_lr_mode(vm) && !vm-

Quoted 54 lines · Thomas Hellström

Hi, Matthew,

Thanks for reviewing, please see inline.

Quoted 49 lines · Matthew Brost

Quoted 48 lines · Thomas Hellström

For each rebind we insert a GuC TLB invalidation and add a corresponding unordered TLB invalidation fence. This might add a huge number of TLB invalidation fences to wait for so rather than doing that, defer the TLB invalidation to the next ring ops for each affected exec queue. Since the TLB is invalidated on exec_queue switch, we need to invalidate once for each affected exec_queue.

Fixes 5387e865d90e ("drm/xe: Add TLB invalidation fence after

rebinds issued from execs")
Cc: Matthew Brost <[email protected]>
Cc: <[email protected]> # v6.8+
Signed-off-by: Thomas Hellström <[email protected]>
---
 drivers/gpu/drm/xe/xe_exec_queue_types.h |  2 ++ drivers/gpu/drm/xe/xe_pt.c               |  5 +++-- drivers/gpu/drm/xe/xe_ring_ops.c         | 11 ++++------- drivers/gpu/drm/xe/xe_sched_job.c        | 11 +++++++++++ drivers/gpu/drm/xe/xe_sched_job_types.h  |  2 ++ drivers/gpu/drm/xe/xe_vm_types.h         |  5 +++++

 6 files changed, 27 insertions(+), 9 deletions(-)

drivers/gpu/drm/xe/xe_exec_queue_types.h +0 -0

index 62b3d9d1d7cd..891ad30e906f 100644
@@ -148,6 +148,8 @@ struct xe_exec_queue {
  	const struct xe_ring_ops *ring_ops;
  	/** @entity: DRM sched entity for this exec queue (1 to

1
relationship) */
  struct drm_sched_entity *entity;

drivers/gpu/drm/xe/xe_exec_queue_types.h +1 -0

+	/** @tlb_flush_seqno: The seqno of the last rebind tlb

flush performed */

drivers/gpu/drm/xe/xe_exec_queue_types.h +1 -0

+	u64 tlb_flush_seqno;

  /** @lrc: logical ring context for this exec queue */ struct xe_lrc lrc[];
 };

drivers/gpu/drm/xe/xe_pt.c +0 -0

index 8d3922d2206e..21bc0d13fccf 100644
@@ -1254,11 +1254,12 @@ __xe_pt_bind_vma(struct xe_tile *tile,

struct xe_vma *vma, struct xe_exec_queue
  * non-faulting LR, in particular on user-space batch

Quoted 4 lines

Quoted 4 lines

drivers/gpu/drm/xe/xe_pt.c

Quoted 4 lines

buffer chaining,
  * it needs to be done here.
  */

drivers/gpu/drm/xe/xe_pt.c +0 -1

-	if ((rebind && !xe_vm_in_lr_mode(vm) && !vm-

While I remember it. When looking at your series I noticed that this line got incorrectly moved there. Looks like you used xe_vm_in_lr_mode() rather than !xe_vm_in_lr_mode()..

Thomas

Can we simplify this too?

	if (vm && (vm->batch_invalidate_tlb || (vm->tlb_flush_seqno
!= q->tlb_flush_seqno))) {
		q->tlb_flush_seqno = vm->tlb_flush_seqno;
		job->ring_ops_flush_tlb = true;
	}

I think this works as xe_sched_job_is_migration has emit_migration_job_gen12 which doesn't look at job-

Quoted 1 line

ring_ops_flush_tlb,

so no need to xe_sched_job_is_migration.

Also no need to check xe_vm_in_lr_mode as we wouldn'y increment the seqno above if that true.

Lastly, harmless to increment q->tlb_flush_seqno in the case of batch_invalidate_tlb being true.

Instead of handling the vm's rebind fence separately, which is error prone if they are not strictly ordered, attach rebind fences as kernel fences to the vm's resv.

Fixes dd08ebf6c352 ("drm/xe: Introduce a new DRM driver for Intel GPUs")

Cc Rodrigo Vivi <[email protected]>

Cc Matthew Brost <[email protected]>

Cc <[email protected]> # v6.8+

Signed-off-by Thomas Hellström <[email protected]>

---
 drivers/gpu/drm/xe/xe_exec.c     | 31 +++----------------------------
 drivers/gpu/drm/xe/xe_pt.c       |  2 +-
 drivers/gpu/drm/xe/xe_vm.c       | 27 +++++++++------------------
 drivers/gpu/drm/xe/xe_vm.h       |  2 +-
 drivers/gpu/drm/xe/xe_vm_types.h |  3 ---
 5 files changed, 14 insertions(+), 51 deletions(-)

drivers/gpu/drm/xe/xe_exec.c +3 -28

index 7692ebfe7d47..759497d4a102 100644
@@ -152,7 +152,6 @@ int xe_exec_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
 	struct drm_exec *exec = &vm_exec.exec;
 	u32 i, num_syncs = 0, num_ufence = 0;
 	struct xe_sched_job *job;
-	struct dma_fence *rebind_fence;
 	struct xe_vm *vm;
 	bool write_locked, skip_retry = false;
 	ktime_t end = 0;

@@ -294,35 +293,11 @@ int xe_exec_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
 	 * Rebind any invalidated userptr or evicted BOs in the VM, non-compute
 	 * VM mode only.
 	 */
-	rebind_fence = xe_vm_rebind(vm, false);
-	if (IS_ERR(rebind_fence)) {
-		err = PTR_ERR(rebind_fence);
+	err = xe_vm_rebind(vm, false);
+	if (err)
 		goto err_put_job;
-	}
-
-	/*
-	 * We store the rebind_fence in the VM so subsequent execs don't get
-	 * scheduled before the rebinds of userptrs / evicted BOs is complete.
-	 */
-	if (rebind_fence) {
-		dma_fence_put(vm->rebind_fence);
-		vm->rebind_fence = rebind_fence;
-	}
-	if (vm->rebind_fence) {
-		if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
-			     &vm->rebind_fence->flags)) {
-			dma_fence_put(vm->rebind_fence);
-			vm->rebind_fence = NULL;
-		} else {
-			dma_fence_get(vm->rebind_fence);
-			err = drm_sched_job_add_dependency(&job->drm,
-							   vm->rebind_fence);
-			if (err)
-				goto err_put_job;
-		}
-	}
 
-	/* Wait behind munmap style rebinds */
+	/* Wait behind rebinds */
 	if (!xe_vm_in_lr_mode(vm)) {
 		err = drm_sched_job_add_resv_dependencies(&job->drm,
 							  xe_vm_resv(vm),

drivers/gpu/drm/xe/xe_pt.c +1 -1

index 21bc0d13fccf..0484ed5b495f 100644
@@ -1298,7 +1298,7 @@ __xe_pt_bind_vma(struct xe_tile *tile, struct xe_vma *vma, struct xe_exec_queue
 		}
 
 		/* add shared fence now for pagetable delayed destroy */
-		dma_resv_add_fence(xe_vm_resv(vm), fence, !rebind &&
+		dma_resv_add_fence(xe_vm_resv(vm), fence, rebind ||
 				   last_munmap_rebind ?
 				   DMA_RESV_USAGE_KERNEL :
 				   DMA_RESV_USAGE_BOOKKEEP);

drivers/gpu/drm/xe/xe_vm.c +9 -18

index 80d43d75b1da..35fba6e3f889 100644
@@ -522,7 +522,6 @@ static void preempt_rebind_work_func(struct work_struct *w)
 {
 	struct xe_vm *vm = container_of(w, struct xe_vm, preempt.rebind_work);
 	struct drm_exec exec;
-	struct dma_fence *rebind_fence;
 	unsigned int fence_count = 0;
 	LIST_HEAD(preempt_fences);
 	ktime_t end = 0;

@@ -568,18 +567,11 @@ static void preempt_rebind_work_func(struct work_struct *w)
 	if (err)
 		goto out_unlock;
 
-	rebind_fence = xe_vm_rebind(vm, true);
-	if (IS_ERR(rebind_fence)) {
-		err = PTR_ERR(rebind_fence);
+	err = xe_vm_rebind(vm, true);
+	if (err)
 		goto out_unlock;
-	}
-
-	if (rebind_fence) {
-		dma_fence_wait(rebind_fence, false);
-		dma_fence_put(rebind_fence);
-	}
 
-	/* Wait on munmap style VM unbinds */
+	/* Wait on rebinds and munmap style VM unbinds */
 	wait = dma_resv_wait_timeout(xe_vm_resv(vm),
 				     DMA_RESV_USAGE_KERNEL,
 				     false, MAX_SCHEDULE_TIMEOUT);

@@ -773,14 +765,14 @@ xe_vm_bind_vma(struct xe_vma *vma, struct xe_exec_queue *q,
 	       struct xe_sync_entry *syncs, u32 num_syncs,
 	       bool first_op, bool last_op);
 
-struct dma_fence *xe_vm_rebind(struct xe_vm *vm, bool rebind_worker)
+int xe_vm_rebind(struct xe_vm *vm, bool rebind_worker)
 {
-	struct dma_fence *fence = NULL;
+	struct dma_fence *fence;
 	struct xe_vma *vma, *next;
 
 	lockdep_assert_held(&vm->lock);
 	if (xe_vm_in_lr_mode(vm) && !rebind_worker)
-		return NULL;
+		return 0;
 
 	xe_vm_assert_held(vm);
 	list_for_each_entry_safe(vma, next, &vm->rebind_list,

@@ -788,17 +780,17 @@ struct dma_fence *xe_vm_rebind(struct xe_vm *vm, bool rebind_worker)
 		xe_assert(vm->xe, vma->tile_present);
 
 		list_del_init(&vma->combined_links.rebind);
-		dma_fence_put(fence);
 		if (rebind_worker)
 			trace_xe_vma_rebind_worker(vma);
 		else
 			trace_xe_vma_rebind_exec(vma);
 		fence = xe_vm_bind_vma(vma, NULL, NULL, 0, false, false);
 		if (IS_ERR(fence))
-			return fence;
+			return PTR_ERR(fence);
+		dma_fence_put(fence);
 	}
 
-	return fence;
+	return 0;
 }
 
 static void xe_vma_free(struct xe_vma *vma)

@@ -1588,7 +1580,6 @@ static void vm_destroy_work_func(struct work_struct *w)
 		XE_WARN_ON(vm->pt_root[id]);
 
 	trace_xe_vm_free(vm);
-	dma_fence_put(vm->rebind_fence);
 	kfree(vm);
 }
 

drivers/gpu/drm/xe/xe_vm.h +1 -1

index 6df1f1c7f85d..4853354336f2 100644
@@ -207,7 +207,7 @@ int __xe_vm_userptr_needs_repin(struct xe_vm *vm);
 
 int xe_vm_userptr_check_repin(struct xe_vm *vm);
 
-struct dma_fence *xe_vm_rebind(struct xe_vm *vm, bool rebind_worker);
+int xe_vm_rebind(struct xe_vm *vm, bool rebind_worker);
 
 int xe_vm_invalidate_vma(struct xe_vma *vma);
 

drivers/gpu/drm/xe/xe_vm_types.h +0 -3

index 5747f136d24d..badf3945083d 100644
@@ -177,9 +177,6 @@ struct xe_vm {
 	 */
 	struct list_head rebind_list;
 
-	/** @rebind_fence: rebind fence from execbuf */
-	struct dma_fence *rebind_fence;
-
 	/**
 	 * @destroy_work: worker to destroy VM, needed as a dma_fence signaling
 	 * from an irq context can be last put and the destroy needs to be able

Signature · 1 line

2.44.0

Quoted 4 lines · Thomas Hellström

Instead of handling the vm's rebind fence separately, which is error prone if they are not strictly ordered, attach rebind fences as kernel fences to the vm's resv.

See comment from previous, do not like updates to __xe_pt_bind_vma but I
guess I can live with it. Otherwise LGTM.

With that:

Reviewed-by Matthew Brost <[email protected]>

Quoted 220 lines

Fixes dd08ebf6c352 ("drm/xe: Introduce a new DRM driver for Intel

GPUs")

Cc Rodrigo Vivi <[email protected]>

Cc Matthew Brost <[email protected]>

Cc <[email protected]> # v6.8+

Signed-off-by Thomas Hellström <[email protected]>

---
 drivers/gpu/drm/xe/xe_exec.c     | 31 +++-------------------------
---

 drivers/gpu/drm/xe/xe_pt.c       |  2 +-
 drivers/gpu/drm/xe/xe_vm.c       | 27 +++++++++------------------
 drivers/gpu/drm/xe/xe_vm.h       |  2 +-
 drivers/gpu/drm/xe/xe_vm_types.h |  3 ---
 5 files changed, 14 insertions(+), 51 deletions(-)

drivers/gpu/drm/xe/xe_exec.c +0 -1

index 7692ebfe7d47..759497d4a102 100644
@@ -152,7 +152,6 @@ int xe_exec_ioctl(struct drm_device *dev, void
 *data, struct drm_file *file)
  	struct drm_exec *exec = &vm_exec.exec;
  	u32 i, num_syncs = 0, num_ufence = 0;
  	struct xe_sched_job *job;
-	struct dma_fence *rebind_fence;
  	struct xe_vm *vm;
  	bool write_locked, skip_retry = false;

  ktime_t end = 0;

drivers/gpu/drm/xe/xe_exec.c +0 -0

@@ -294,35 +293,11 @@ int xe_exec_ioctl(struct drm_device *dev,
 void *data, struct drm_file *file)
  	 * Rebind any invalidated userptr or evicted BOs in the

VM, non-compute
  * VM mode only.
  */

drivers/gpu/drm/xe/xe_exec.c +2 -3

-	rebind_fence = xe_vm_rebind(vm, false);
-	if (IS_ERR(rebind_fence)) {
-		err = PTR_ERR(rebind_fence);
+	err = xe_vm_rebind(vm, false);
+	if (err)

  goto err_put_job;

drivers/gpu/drm/xe/xe_exec.c +0 -4

-	}
-
-	/*
-	 * We store the rebind_fence in the VM so subsequent execs

don't get

drivers/gpu/drm/xe/xe_exec.c +0 -1

-	 * scheduled before the rebinds of userptrs / evicted BOs

is complete.

drivers/gpu/drm/xe/xe_exec.c +0 -13

-	 */
-	if (rebind_fence) {
-		dma_fence_put(vm->rebind_fence);
-		vm->rebind_fence = rebind_fence;
-	}
-	if (vm->rebind_fence) {
-		if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
-			     &vm->rebind_fence->flags)) {
-			dma_fence_put(vm->rebind_fence);
-			vm->rebind_fence = NULL;
-		} else {
-			dma_fence_get(vm->rebind_fence);
-			err = drm_sched_job_add_dependency(&job-

Quoted 1 line

drm,

drivers/gpu/drm/xe/xe_exec.c +0 -1

-							   vm-

Quoted 1 line

rebind_fence);

drivers/gpu/drm/xe/xe_exec.c +0 -4

-			if (err)
-				goto err_put_job;
-		}
-	}

drivers/gpu/drm/xe/xe_exec.c +1 -1

-	/* Wait behind munmap style rebinds */
+	/* Wait behind rebinds */

  if (!xe_vm_in_lr_mode(vm)) {
  err = drm_sched_job_add_resv_dependencies(&job-

Quoted 1 line

drm,

xe_vm_resv(vm),

drivers/gpu/drm/xe/xe_pt.c +0 -0

index 21bc0d13fccf..0484ed5b495f 100644
@@ -1298,7 +1298,7 @@ __xe_pt_bind_vma(struct xe_tile *tile, struct

xe_vma *vma, struct xe_exec_queue
  }

  /* add shared fence now for pagetable delayed destroy */

drivers/gpu/drm/xe/xe_pt.c +0 -1

-		dma_resv_add_fence(xe_vm_resv(vm), fence, !rebind

&&

drivers/gpu/drm/xe/xe_pt.c +1 -0

+		dma_resv_add_fence(xe_vm_resv(vm), fence, rebind

||
     last_munmap_rebind ? DMA_RESV_USAGE_KERNEL :
     DMA_RESV_USAGE_BOOKKEEP);

drivers/gpu/drm/xe/xe_vm.c +0 -1

index 80d43d75b1da..35fba6e3f889 100644
@@ -522,7 +522,6 @@ static void preempt_rebind_work_func(struct
 work_struct *w)
  {
  	struct xe_vm *vm = container_of(w, struct xe_vm,
 preempt.rebind_work);
  	struct drm_exec exec;
-	struct dma_fence *rebind_fence;
  	unsigned int fence_count = 0;

  LIST_HEAD(preempt_fences);
  ktime_t end = 0;

drivers/gpu/drm/xe/xe_vm.c +0 -0

@@ -568,18 +567,11 @@ static void preempt_rebind_work_func(struct
 work_struct *w)
  	if (err)
  		goto out_unlock;

drivers/gpu/drm/xe/xe_vm.c +2 -3

-	rebind_fence = xe_vm_rebind(vm, true);
-	if (IS_ERR(rebind_fence)) {
-		err = PTR_ERR(rebind_fence);
+	err = xe_vm_rebind(vm, true);
+	if (err)

  goto out_unlock;

drivers/gpu/drm/xe/xe_vm.c +0 -6

-	}
-
-	if (rebind_fence) {
-		dma_fence_wait(rebind_fence, false);
-		dma_fence_put(rebind_fence);
-	}

drivers/gpu/drm/xe/xe_vm.c +1 -1

-	/* Wait on munmap style VM unbinds */
+	/* Wait on rebinds and munmap style VM unbinds */

  wait = dma_resv_wait_timeout(xe_vm_resv(vm), DMA_RESV_USAGE_KERNEL, false, MAX_SCHEDULE_TIMEOUT);

drivers/gpu/drm/xe/xe_vm.c +0 -0

@@ -773,14 +765,14 @@ xe_vm_bind_vma(struct xe_vma *vma, struct
 xe_exec_queue *q,
  	       struct xe_sync_entry *syncs, u32 num_syncs,
  	       bool first_op, bool last_op);

-struct dma_fence *xe_vm_rebind(struct xe_vm *vm, bool rebind_worker)
+int xe_vm_rebind(struct xe_vm *vm, bool rebind_worker) {

drivers/gpu/drm/xe/xe_vm.c +1 -1

-	struct dma_fence *fence = NULL;
+	struct dma_fence *fence;

  struct xe_vma *vma, *next;

  lockdep_assert_held(&vm->lock);
  if (xe_vm_in_lr_mode(vm) && !rebind_worker)

drivers/gpu/drm/xe/xe_vm.c +1 -1

-		return NULL;
+		return 0;

  xe_vm_assert_held(vm); list_for_each_entry_safe(vma, next, &vm->rebind_list,

drivers/gpu/drm/xe/xe_vm.c +0 -0

@@ -788,17 +780,17 @@ struct dma_fence *xe_vm_rebind(struct xe_vm
 *vm, bool rebind_worker)
  		xe_assert(vm->xe, vma->tile_present);

  list_del_init(&vma->combined_links.rebind);

drivers/gpu/drm/xe/xe_vm.c +0 -1

-		dma_fence_put(fence);

  if (rebind_worker) trace_xe_vma_rebind_worker(vma); else
  trace_xe_vma_rebind_exec(vma); fence = xe_vm_bind_vma(vma, NULL, NULL, 0, false, false);
  if (IS_ERR(fence))

drivers/gpu/drm/xe/xe_vm.c +2 -1

-			return fence;
+			return PTR_ERR(fence);
+		dma_fence_put(fence);

  }

drivers/gpu/drm/xe/xe_vm.c +1 -1

-	return fence;
+	return 0;

 }

 static void xe_vma_free(struct xe_vma *vma)

drivers/gpu/drm/xe/xe_vm.c +0 -0

@@ -1588,7 +1580,6 @@ static void vm_destroy_work_func(struct
 work_struct *w)
  		XE_WARN_ON(vm->pt_root[id]);

  trace_xe_vm_free(vm);

drivers/gpu/drm/xe/xe_vm.c +0 -1

-	dma_fence_put(vm->rebind_fence);

  kfree(vm);
 }

drivers/gpu/drm/xe/xe_vm.h +0 -0

index 6df1f1c7f85d..4853354336f2 100644
@@ -207,7 +207,7 @@ int __xe_vm_userptr_needs_repin(struct xe_vm
 *vm);

 int xe_vm_userptr_check_repin(struct xe_vm *vm);
-struct dma_fence *xe_vm_rebind(struct xe_vm *vm, bool rebind_worker);
+int xe_vm_rebind(struct xe_vm *vm, bool rebind_worker);
 int xe_vm_invalidate_vma(struct xe_vma *vma);

drivers/gpu/drm/xe/xe_vm_types.h +0 -0

index 5747f136d24d..badf3945083d 100644
@@ -177,9 +177,6 @@ struct xe_vm {
  	 */
  	struct list_head rebind_list;

drivers/gpu/drm/xe/xe_vm_types.h +0 -3

-	/** @rebind_fence: rebind fence from execbuf */
-	struct dma_fence *rebind_fence;
-

  /**
  * @destroy_work: worker to destroy VM, needed as a dma_fence signaling
  * from an irq context can be last put and the destroy needs to be able

Signature · 1 line

2.44.0

They can actually complete out-of-order, so allocate a unique fence context for each fence.

Fixes 5387e865d90e ("drm/xe: Add TLB invalidation fence after rebinds issued from execs")

Cc Matthew Brost <[email protected]>

Cc <[email protected]> # v6.8+

Signed-off-by Thomas Hellström <[email protected]>

---
 drivers/gpu/drm/xe/xe_gt_tlb_invalidation.c | 1 -
 drivers/gpu/drm/xe/xe_gt_types.h            | 7 -------
 drivers/gpu/drm/xe/xe_pt.c                  | 3 +--
 3 files changed, 1 insertion(+), 10 deletions(-)

drivers/gpu/drm/xe/xe_gt_tlb_invalidation.c +0 -1

index a3c4ffba679d..787cba5e49a1 100644
@@ -63,7 +63,6 @@ int xe_gt_tlb_invalidation_init(struct xe_gt *gt)
 	INIT_LIST_HEAD(&gt->tlb_invalidation.pending_fences);
 	spin_lock_init(&gt->tlb_invalidation.pending_lock);
 	spin_lock_init(&gt->tlb_invalidation.lock);
-	gt->tlb_invalidation.fence_context = dma_fence_context_alloc(1);
 	INIT_DELAYED_WORK(&gt->tlb_invalidation.fence_tdr,
 			  xe_gt_tlb_fence_timeout);
 

drivers/gpu/drm/xe/xe_gt_types.h +0 -7

index f6da2ad9719f..2143dffcaf11 100644
@@ -179,13 +179,6 @@ struct xe_gt {
 		 * xe_gt_tlb_fence_timeout after the timeut interval is over.
 		 */
 		struct delayed_work fence_tdr;
-		/** @tlb_invalidation.fence_context: context for TLB invalidation fences */
-		u64 fence_context;
-		/**
-		 * @tlb_invalidation.fence_seqno: seqno to TLB invalidation fences, protected by
-		 * tlb_invalidation.lock
-		 */
-		u32 fence_seqno;
 		/** @tlb_invalidation.lock: protects TLB invalidation fences */
 		spinlock_t lock;
 	} tlb_invalidation;

drivers/gpu/drm/xe/xe_pt.c +1 -2

index 0484ed5b495f..045a8c0845ba 100644
@@ -1135,8 +1135,7 @@ static int invalidation_fence_init(struct xe_gt *gt,
 	spin_lock_irq(&gt->tlb_invalidation.lock);
 	dma_fence_init(&ifence->base.base, &invalidation_fence_ops,
 		       &gt->tlb_invalidation.lock,
-		       gt->tlb_invalidation.fence_context,
-		       ++gt->tlb_invalidation.fence_seqno);
+		       dma_fence_context_alloc(1), 1);
 	spin_unlock_irq(&gt->tlb_invalidation.lock);
 
 	INIT_LIST_HEAD(&ifence->base.link);

Signature · 1 line

2.44.0

Reserve fence slots where actually needed rather than trying to predict how many fence slots will be needed over a complete wound-wait transaction.

Fixes 29f424eb8702 ("drm/xe/exec: move fence reservation")

Cc Matthew Auld <[email protected]>

Signed-off-by Thomas Hellström <[email protected]>

---
 drivers/gpu/drm/xe/xe_exec.c         | 27 +---------------
 drivers/gpu/drm/xe/xe_gt_pagefault.c |  3 +-
 drivers/gpu/drm/xe/xe_pt.c           | 14 ++++++++
 drivers/gpu/drm/xe/xe_vm.c           | 48 +++++++++++++---------------
 drivers/gpu/drm/xe/xe_vm.h           |  3 +-
 5 files changed, 40 insertions(+), 55 deletions(-)

drivers/gpu/drm/xe/xe_exec.c +1 -26

index 7692ebfe7d47..511d23426a5e 100644
@@ -96,41 +96,16 @@
 
 static int xe_exec_fn(struct drm_gpuvm_exec *vm_exec)
 {
-	struct xe_vm *vm = container_of(vm_exec->vm, struct xe_vm, gpuvm);
 	struct drm_gem_object *obj;
 	unsigned long index;
-	int num_fences;
 	int ret;
 
 	ret = drm_gpuvm_validate(vm_exec->vm, &vm_exec->exec);
 	if (ret)
 		return ret;
 
-	/*
-	 * 1 fence slot for the final submit, and 1 more for every per-tile for
-	 * GPU bind and 1 extra for CPU bind. Note that there are potentially
-	 * many vma per object/dma-resv, however the fence slot will just be
-	 * re-used, since they are largely the same timeline and the seqno
-	 * should be in order. In the case of CPU bind there is dummy fence used
-	 * for all CPU binds, so no need to have a per-tile slot for that.
-	 */
-	num_fences = 1 + 1 + vm->xe->info.tile_count;
-
-	/*
-	 * We don't know upfront exactly how many fence slots we will need at
-	 * the start of the exec, since the TTM bo_validate above can consume
-	 * numerous fence slots. Also due to how the dma_resv_reserve_fences()
-	 * works it only ensures that at least that many fence slots are
-	 * available i.e if there are already 10 slots available and we reserve
-	 * two more, it can just noop without reserving anything.  With this it
-	 * is quite possible that TTM steals some of the fence slots and then
-	 * when it comes time to do the vma binding and final exec stage we are
-	 * lacking enough fence slots, leading to some nasty BUG_ON() when
-	 * adding the fences. Hence just add our own fences here, after the
-	 * validate stage.
-	 */
 	drm_exec_for_each_locked_object(&vm_exec->exec, index, obj) {
-		ret = dma_resv_reserve_fences(obj->resv, num_fences);
+		ret = dma_resv_reserve_fences(obj->resv, 1);
 		if (ret)
 			return ret;
 	}

drivers/gpu/drm/xe/xe_gt_pagefault.c +1 -2

index 241c294270d9..fa9e9853c53b 100644
@@ -100,10 +100,9 @@ static int xe_pf_begin(struct drm_exec *exec, struct xe_vma *vma,
 {
 	struct xe_bo *bo = xe_vma_bo(vma);
 	struct xe_vm *vm = xe_vma_vm(vma);
-	unsigned int num_shared = 2; /* slots for bind + move */
 	int err;
 
-	err = xe_vm_prepare_vma(exec, vma, num_shared);
+	err = xe_vm_lock_vma(exec, vma);
 	if (err)
 		return err;
 

drivers/gpu/drm/xe/xe_pt.c +0 -0

index 109c26e5689e..7add08b2954e 100644

drivers/gpu/drm/xe/xe_pt.c +7 -0

 	xe_tile_assert(tile, num_entries <= ARRAY_SIZE(entries));
 
 	xe_vm_dbg_print_entries(tile_to_xe(tile), entries, num_entries);

@@ -1576,6 +1583,7 @@ __xe_pt_unbind_vma(struct xe_tile *tile, struct xe_vma *vma, struct xe_exec_queu
 	struct dma_fence *fence = NULL;
 	struct invalidation_fence *ifence;
 	struct xe_range_fence *rfence;
+	int err;
 
 	LLIST_HEAD(deferred);
 

@@ -1593,6 +1601,12 @@ __xe_pt_unbind_vma(struct xe_tile *tile, struct xe_vma *vma, struct xe_exec_queu
 	xe_pt_calc_rfence_interval(vma, &unbind_pt_update, entries,
 				   num_entries);
 
+	err = dma_resv_reserve_fences(xe_vm_resv(vm), 1);
+	if (!err && !xe_vma_has_no_bo(vma) && !xe_vma_bo(vma)->vm)
+		err = dma_resv_reserve_fences(xe_vma_bo(vma)->ttm.base.resv, 1);
+	if (err)
+		return ERR_PTR(err);
+
 	ifence = kzalloc(sizeof(*ifence), GFP_KERNEL);
 	if (!ifence)
 		return ERR_PTR(-ENOMEM);

drivers/gpu/drm/xe/xe_vm.c +3 -6

index 80d43d75b1da..cff8db605743 100644
@@ -485,14 +485,11 @@ static int xe_gpuvm_validate(struct drm_gpuvm_bo *vm_bo, struct drm_exec *exec)
 static int xe_preempt_work_begin(struct drm_exec *exec, struct xe_vm *vm,
 				 bool *done)
 {
+	struct drm_gem_object *obj;
+	unsigned long index;
 	int err;
 
-	/*
-	 * 1 fence for each preempt fence plus a fence for each tile from a
-	 * possible rebind
-	 */
-	err = drm_gpuvm_prepare_vm(&vm->gpuvm, exec, vm->preempt.num_exec_queues +
-				   vm->xe->info.tile_count);
+	err = drm_gpuvm_prepare_vm(&vm->gpuvm, exec, 0);
 	if (err)
 		return err;
 

drivers/gpu/drm/xe/xe_vm.c +0 -0

 	if (err)
 		return err;
 

drivers/gpu/drm/xe/xe_vm.c +13 -17

+		if (err)
+			return err;
+	}
+
+	return 0;
 }
 
 static void preempt_rebind_work_func(struct work_struct *w)

@@ -1004,35 +1011,26 @@ static void xe_vma_destroy(struct xe_vma *vma, struct dma_fence *fence)
 }
 
 /**
- * xe_vm_prepare_vma() - drm_exec utility to lock a vma
+ * xe_vm_lock_vma() - drm_exec utility to lock a vma
  * @exec: The drm_exec object we're currently locking for.
  * @vma: The vma for witch we want to lock the vm resv and any attached
  * object's resv.
- * @num_shared: The number of dma-fence slots to pre-allocate in the
- * objects' reservation objects.
  *
  * Return: 0 on success, negative error code on error. In particular
  * may return -EDEADLK on WW transaction contention and -EINTR if
  * an interruptible wait is terminated by a signal.
  */
-int xe_vm_prepare_vma(struct drm_exec *exec, struct xe_vma *vma,
-		      unsigned int num_shared)
+int xe_vm_lock_vma(struct drm_exec *exec, struct xe_vma *vma)
 {
 	struct xe_vm *vm = xe_vma_vm(vma);
 	struct xe_bo *bo = xe_vma_bo(vma);
 	int err;
 
 	XE_WARN_ON(!vm);
-	if (num_shared)
-		err = drm_exec_prepare_obj(exec, xe_vm_obj(vm), num_shared);
-	else
-		err = drm_exec_lock_obj(exec, xe_vm_obj(vm));
-	if (!err && bo && !bo->vm) {
-		if (num_shared)
-			err = drm_exec_prepare_obj(exec, &bo->ttm.base, num_shared);
-		else
-			err = drm_exec_lock_obj(exec, &bo->ttm.base);
-	}
+
+	err = drm_exec_lock_obj(exec, xe_vm_obj(vm));
+	if (!err && bo && !bo->vm)
+		err = drm_exec_lock_obj(exec, &bo->ttm.base);
 
 	return err;
 }

@@ -1044,7 +1042,7 @@ static void xe_vma_destroy_unlocked(struct xe_vma *vma)
 
 	drm_exec_init(&exec, 0, 0);
 	drm_exec_until_all_locked(&exec) {
-		err = xe_vm_prepare_vma(&exec, vma, 0);
+		err = xe_vm_lock_vma(&exec, vma);
 		drm_exec_retry_on_contention(&exec);
 		if (XE_WARN_ON(err))
 			break;

@@ -2511,7 +2509,7 @@ static int op_execute(struct drm_exec *exec, struct xe_vm *vm,
 
 	lockdep_assert_held_write(&vm->lock);
 
-	err = xe_vm_prepare_vma(exec, vma, 1);
+	err = xe_vm_lock_vma(exec, vma);
 	if (err)
 		return err;
 

drivers/gpu/drm/xe/xe_vm.h +1 -2

index 6df1f1c7f85d..47f3960120a0 100644
@@ -242,8 +242,7 @@ bool xe_vm_validate_should_retry(struct drm_exec *exec, int err, ktime_t *end);
 
 int xe_analyze_vm(struct drm_printer *p, struct xe_vm *vm, int gt_id);
 
-int xe_vm_prepare_vma(struct drm_exec *exec, struct xe_vma *vma,
-		      unsigned int num_shared);
+int xe_vm_lock_vma(struct drm_exec *exec, struct xe_vma *vma);
 
 /**
  * xe_vm_resv() - Return's the vm's reservation object

Signature · 1 line

2.44.0

Quoted 4 lines · Thomas Hellström

Reserve fence slots where actually needed rather than trying to predict how many fence slots will be needed over a complete wound-wait transaction.

You include this patch twice in the series.

Quoted 114 lines

Fixes 29f424eb8702 ("drm/xe/exec: move fence reservation")

Cc Matthew Auld <[email protected]>

Signed-off-by Thomas Hellström <[email protected]>

---
 drivers/gpu/drm/xe/xe_exec.c         | 27 +--------------- drivers/gpu/drm/xe/xe_gt_pagefault.c |  3 +- drivers/gpu/drm/xe/xe_pt.c           | 14 ++++++++ drivers/gpu/drm/xe/xe_vm.c           | 48 +++++++++++++-----------
----
 drivers/gpu/drm/xe/xe_vm.h           |  3 +-

 5 files changed, 40 insertions(+), 55 deletions(-)

drivers/gpu/drm/xe/xe_exec.c +0 -0

index 7692ebfe7d47..511d23426a5e 100644
@@ -96,41 +96,16 @@

 static int xe_exec_fn(struct drm_gpuvm_exec *vm_exec) {

drivers/gpu/drm/xe/xe_exec.c +0 -1

-	struct xe_vm *vm = container_of(vm_exec->vm, struct xe_vm,

gpuvm);
  struct drm_gem_object *obj;
  unsigned long index;

drivers/gpu/drm/xe/xe_exec.c +0 -1

-	int num_fences;

  int ret;

  ret = drm_gpuvm_validate(vm_exec->vm, &vm_exec->exec); if (ret)
  return ret;

drivers/gpu/drm/xe/xe_exec.c +0 -2

-	/*
-	 * 1 fence slot for the final submit, and 1 more for every

per-tile for

drivers/gpu/drm/xe/xe_exec.c +0 -1

-	 * GPU bind and 1 extra for CPU bind. Note that there are

potentially

drivers/gpu/drm/xe/xe_exec.c +0 -1

-	 * many vma per object/dma-resv, however the fence slot

will just be

drivers/gpu/drm/xe/xe_exec.c +0 -1

-	 * re-used, since they are largely the same timeline and

the seqno

drivers/gpu/drm/xe/xe_exec.c +0 -1

-	 * should be in order. In the case of CPU bind there is

dummy fence used

drivers/gpu/drm/xe/xe_exec.c +0 -1

-	 * for all CPU binds, so no need to have a per-tile slot

for that.

drivers/gpu/drm/xe/xe_exec.c +0 -5

-	 */
-	num_fences = 1 + 1 + vm->xe->info.tile_count;
-
-	/*
-	 * We don't know upfront exactly how many fence slots we

will need at

drivers/gpu/drm/xe/xe_exec.c +0 -1

-	 * the start of the exec, since the TTM bo_validate above

can consume

drivers/gpu/drm/xe/xe_exec.c +0 -1

-	 * numerous fence slots. Also due to how the

dma_resv_reserve_fences()

drivers/gpu/drm/xe/xe_exec.c +0 -1

-	 * works it only ensures that at least that many fence

slots are

drivers/gpu/drm/xe/xe_exec.c +0 -1

-	 * available i.e if there are already 10 slots available

and we reserve

drivers/gpu/drm/xe/xe_exec.c +0 -1

-	 * two more, it can just noop without reserving anything. 

With this it

drivers/gpu/drm/xe/xe_exec.c +0 -1

-	 * is quite possible that TTM steals some of the fence

slots and then

drivers/gpu/drm/xe/xe_exec.c +0 -1

-	 * when it comes time to do the vma binding and final exec

stage we are

drivers/gpu/drm/xe/xe_exec.c +0 -1

-	 * lacking enough fence slots, leading to some nasty

BUG_ON() when

drivers/gpu/drm/xe/xe_exec.c +0 -1

-	 * adding the fences. Hence just add our own fences here,

after the

drivers/gpu/drm/xe/xe_exec.c +0 -2

-	 * validate stage.
-	 */

  drm_exec_for_each_locked_object(&vm_exec->exec, index, obj) {

drivers/gpu/drm/xe/xe_exec.c +0 -1

-		ret = dma_resv_reserve_fences(obj->resv,

num_fences);

drivers/gpu/drm/xe/xe_exec.c +1 -0

+		ret = dma_resv_reserve_fences(obj->resv, 1);

  if (ret)
  return ret;
  }

drivers/gpu/drm/xe/xe_gt_pagefault.c +0 -1

index 241c294270d9..fa9e9853c53b 100644
@@ -100,10 +100,9 @@ static int xe_pf_begin(struct drm_exec *exec,
 struct xe_vma *vma,
  {
  	struct xe_bo *bo = xe_vma_bo(vma);
  	struct xe_vm *vm = xe_vma_vm(vma);
-	unsigned int num_shared = 2; /* slots for bind + move */
  	int err;

drivers/gpu/drm/xe/xe_gt_pagefault.c +1 -1

-	err = xe_vm_prepare_vma(exec, vma, num_shared);
+	err = xe_vm_lock_vma(exec, vma);

  if (err)
  return err;

drivers/gpu/drm/xe/xe_pt.c +0 -0

index 109c26e5689e..7add08b2954e 100644
@@ -1235,6 +1235,13 @@ __xe_pt_bind_vma(struct xe_tile *tile,

struct xe_vma *vma, struct xe_exec_queue
  err = xe_pt_prepare_bind(tile, vma, entries, &num_entries);
  if (err)
  goto err;

drivers/gpu/drm/xe/xe_pt.c +4 -0

+
+	err = dma_resv_reserve_fences(xe_vm_resv(vm), 1);
+	if (!err && !xe_vma_has_no_bo(vma) && !xe_vma_bo(vma)->vm)
+		err = dma_resv_reserve_fences(xe_vma_bo(vma)-

Quoted 1 line

ttm.base.resv, 1);

drivers/gpu/drm/xe/xe_pt.c __xe_pt_bind_vma(struct xe_tile *tile, struct xe_vma *vma, struct xe_exec_queue · line 1244

Quoted 3 lines

drivers/gpu/drm/xe/xe_pt.c +3 -0

+	if (err)
+		goto err;
+

Again not a huge fan of updating __xe_pt_bind_vma / __xe_pt_unbind_vma
here because these are going away [1] but again conceptually this is more or less correct the place.

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

Quoted 62 lines

  xe_tile_assert(tile, num_entries <= ARRAY_SIZE(entries));
  xe_vm_dbg_print_entries(tile_to_xe(tile), entries, num_entries);

drivers/gpu/drm/xe/xe_pt.c +0 -0

@@ -1576,6 +1583,7 @@ __xe_pt_unbind_vma(struct xe_tile *tile,

struct xe_vma *vma, struct xe_exec_queu
  struct dma_fence *fence = NULL;
  struct invalidation_fence *ifence;
  struct xe_range_fence *rfence;

drivers/gpu/drm/xe/xe_pt.c +1 -0

+	int err;

  LLIST_HEAD(deferred);

drivers/gpu/drm/xe/xe_pt.c +0 -0

@@ -1593,6 +1601,12 @@ __xe_pt_unbind_vma(struct xe_tile *tile,

struct xe_vma *vma, struct xe_exec_queu xe_pt_calc_rfence_interval(vma, &unbind_pt_update, entries,
     num_entries);

drivers/gpu/drm/xe/xe_pt.c +3 -0

+	err = dma_resv_reserve_fences(xe_vm_resv(vm), 1);
+	if (!err && !xe_vma_has_no_bo(vma) && !xe_vma_bo(vma)->vm)
+		err = dma_resv_reserve_fences(xe_vma_bo(vma)-

Quoted 1 line

ttm.base.resv, 1);

drivers/gpu/drm/xe/xe_pt.c +3 -0

+	if (err)
+		return ERR_PTR(err);
+

  ifence = kzalloc(sizeof(*ifence), GFP_KERNEL); if (!ifence)
  return ERR_PTR(-ENOMEM);

drivers/gpu/drm/xe/xe_vm.c +0 -0

index 80d43d75b1da..cff8db605743 100644
@@ -485,14 +485,11 @@ static int xe_gpuvm_validate(struct
 drm_gpuvm_bo *vm_bo, struct drm_exec *exec)

 static int xe_preempt_work_begin(struct drm_exec *exec, struct xe_vm *vm,
  bool *done)
 {

drivers/gpu/drm/xe/xe_vm.c +2 -0

+	struct drm_gem_object *obj;
+	unsigned long index;

  int err;

drivers/gpu/drm/xe/xe_vm.c +0 -2

-	/*
-	 * 1 fence for each preempt fence plus a fence for each

tile from a

drivers/gpu/drm/xe/xe_vm.c +0 -3

-	 * possible rebind
-	 */
-	err = drm_gpuvm_prepare_vm(&vm->gpuvm, exec, vm-

Quoted 1 line

preempt.num_exec_queues +

drivers/gpu/drm/xe/xe_vm.c +1 -1

-				   vm->xe->info.tile_count);
+	err = drm_gpuvm_prepare_vm(&vm->gpuvm, exec, 0);

  if (err)
  return err;

drivers/gpu/drm/xe/xe_vm.c +0 -0

@@ -507,7 +504,7 @@ static int xe_preempt_work_begin(struct
 drm_exec *exec, struct xe_vm *vm,
  		return 0;
  	}

drivers/gpu/drm/xe/xe_vm.c +0 -1

-	err = drm_gpuvm_prepare_objects(&vm->gpuvm, exec, vm-

drivers/gpu/drm/xe/xe_vm.c static int xe_preempt_work_begin(struct drm_exec *exec, struct xe_vm *vm, · line 507

Quoted 2 lines

Quoted 1 line

preempt.num_exec_queues);

drivers/gpu/drm/xe/xe_vm.c +1 -0

+	err = drm_gpuvm_prepare_objects(&vm->gpuvm, exec, 0);

I think this needs to be 1 as drm_gpuvm_validate needs a fence slot, right?

Quoted 16 lines

  if (err)
  return err;

drivers/gpu/drm/xe/xe_vm.c +0 -0

@@ -515,7 +512,17 @@ static int xe_preempt_work_begin(struct
 drm_exec *exec, struct xe_vm *vm,
  	if (err)
  		return err;

drivers/gpu/drm/xe/xe_vm.c +6 -1

-	return drm_gpuvm_validate(&vm->gpuvm, exec);
+	err = drm_gpuvm_validate(&vm->gpuvm, exec);
+	if (err)
+		return err;
+
+	drm_exec_for_each_locked_object(exec, index, obj) {
+		err = dma_resv_reserve_fences(obj->resv, vm-

Quoted 1 line

Quoted 1 line

preempt.num_exec_queues);

What if we only have external BOs? Do we allocate slots in the VM dma-resv slot with this loop?

Also what is the behavior of?

dma_resv_reserve_fences(obj, 1);
dma_resv_reserve_fences(obj, 1);

Do we have 2 slots now? e.g. to calls to reserve slots accumulate?

Lastly, strickly speaking I don't we need to patch in this series, do we? It doesn't appaer to being fixing anything unless I'm missing something.

Matt

Quoted 97 lines

drivers/gpu/drm/xe/xe_vm.c +5 -0

+		if (err)
+			return err;
+	}
+
+	return 0;

 }

 static void preempt_rebind_work_func(struct work_struct *w)

drivers/gpu/drm/xe/xe_vm.c +0 -0

@@ -1004,35 +1011,26 @@ static void xe_vma_destroy(struct xe_vma
 *vma, struct dma_fence *fence)
  }

 /**
- * xe_vm_prepare_vma() - drm_exec utility to lock a vma
+ * xe_vm_lock_vma() - drm_exec utility to lock a vma
  * @exec: The drm_exec object we're currently locking for.
  * @vma: The vma for witch we want to lock the vm resv and any attached
  * object's resv.
- * @num_shared: The number of dma-fence slots to pre-allocate in the
- * objects' reservation objects.
  *
  * Return: 0 on success, negative error code on error. In particular
  * may return -EDEADLK on WW transaction contention and -EINTR if
  * an interruptible wait is terminated by a signal.
  */

drivers/gpu/drm/xe/xe_vm.c +1 -2

-int xe_vm_prepare_vma(struct drm_exec *exec, struct xe_vma *vma,
-		      unsigned int num_shared)
+int xe_vm_lock_vma(struct drm_exec *exec, struct xe_vma *vma)

 {
  struct xe_vm *vm = xe_vma_vm(vma);
  struct xe_bo *bo = xe_vma_bo(vma);
  int err;

  XE_WARN_ON(!vm);

drivers/gpu/drm/xe/xe_vm.c +0 -2

-	if (num_shared)
-		err = drm_exec_prepare_obj(exec, xe_vm_obj(vm),

num_shared);

drivers/gpu/drm/xe/xe_vm.c +0 -5

-	else
-		err = drm_exec_lock_obj(exec, xe_vm_obj(vm));
-	if (!err && bo && !bo->vm) {
-		if (num_shared)
-			err = drm_exec_prepare_obj(exec, &bo-

Quoted 1 line

ttm.base, num_shared);

drivers/gpu/drm/xe/xe_vm.c +0 -2

-		else
-			err = drm_exec_lock_obj(exec, &bo-

Quoted 1 line

ttm.base);

drivers/gpu/drm/xe/xe_vm.c +4 -1

-	}
+
+	err = drm_exec_lock_obj(exec, xe_vm_obj(vm));
+	if (!err && bo && !bo->vm)
+		err = drm_exec_lock_obj(exec, &bo->ttm.base);

  return err;
 }

drivers/gpu/drm/xe/xe_vm.c +0 -0

@@ -1044,7 +1042,7 @@ static void xe_vma_destroy_unlocked(struct
 xe_vma *vma)

  drm_exec_init(&exec, 0, 0);
  drm_exec_until_all_locked(&exec) {

drivers/gpu/drm/xe/xe_vm.c +1 -1

-		err = xe_vm_prepare_vma(&exec, vma, 0);
+		err = xe_vm_lock_vma(&exec, vma);

  drm_exec_retry_on_contention(&exec); if (XE_WARN_ON(err))
  break;

drivers/gpu/drm/xe/xe_vm.c +0 -0

@@ -2511,7 +2509,7 @@ static int op_execute(struct drm_exec *exec,
 struct xe_vm *vm,

  lockdep_assert_held_write(&vm->lock);

drivers/gpu/drm/xe/xe_vm.c +1 -1

-	err = xe_vm_prepare_vma(exec, vma, 1);
+	err = xe_vm_lock_vma(exec, vma);

  if (err)
  return err;

drivers/gpu/drm/xe/xe_vm.h +0 -0

index 6df1f1c7f85d..47f3960120a0 100644
@@ -242,8 +242,7 @@ bool xe_vm_validate_should_retry(struct
 drm_exec *exec, int err, ktime_t *end);

 int xe_analyze_vm(struct drm_printer *p, struct xe_vm *vm, int gt_id);

drivers/gpu/drm/xe/xe_vm.h +1 -2

-int xe_vm_prepare_vma(struct drm_exec *exec, struct xe_vma *vma,
-		      unsigned int num_shared);
+int xe_vm_lock_vma(struct drm_exec *exec, struct xe_vma *vma);

 /**
  * xe_vm_resv() - Return's the vm's reservation object

Signature · 1 line

2.44.0