[PATCH 1/8] drm/scheduler: properly forward fence errors

[PATCH 1/8] drm/scheduler: properly forward fence errors

[Christian König]

When a hw fence is signaled with an error properly forward that to the
finished fence.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/scheduler/sched_entity.c |  4 +---
 drivers/gpu/drm/scheduler/sched_fence.c  |  4 +++-
 drivers/gpu/drm/scheduler/sched_main.c   | 18 ++++++++----------
 include/drm/gpu_scheduler.h              |  2 +-
 4 files changed, 13 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
index 15d04a0ec623..eaf71fe15ed3 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -144,7 +144,7 @@ static void drm_sched_entity_kill_jobs_work(struct work_struct *wrk)
 {
 	struct drm_sched_job *job = container_of(wrk, typeof(*job), work);
 
-	drm_sched_fence_finished(job->s_fence);
+	drm_sched_fence_finished(job->s_fence, -ESRCH);
 	WARN_ON(job->s_fence->parent);
 	job->sched->ops->free_job(job);
 }
@@ -195,8 +195,6 @@ static void drm_sched_entity_kill(struct drm_sched_entity *entity)
 	while ((job = to_drm_sched_job(spsc_queue_pop(&entity->job_queue)))) {
 		struct drm_sched_fence *s_fence = job->s_fence;
 
-		dma_fence_set_error(&s_fence->finished, -ESRCH);
-
 		dma_fence_get(&s_fence->finished);
 		if (!prev || dma_fence_add_callback(prev, &job->finish_cb,
 					   drm_sched_entity_kill_jobs_cb))
diff --git a/drivers/gpu/drm/scheduler/sched_fence.c b/drivers/gpu/drm/scheduler/sched_fence.c
index 7fd869520ef2..1a6bea98c5cc 100644
--- a/drivers/gpu/drm/scheduler/sched_fence.c
+++ b/drivers/gpu/drm/scheduler/sched_fence.c
@@ -53,8 +53,10 @@ void drm_sched_fence_scheduled(struct drm_sched_fence *fence)
 	dma_fence_signal(&fence->scheduled);
 }
 
-void drm_sched_fence_finished(struct drm_sched_fence *fence)
+void drm_sched_fence_finished(struct drm_sched_fence *fence, int result)
 {
+	if (result)
+		dma_fence_set_error(&fence->finished, result);
 	dma_fence_signal(&fence->finished);
 }
 
diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index fcd4bfef7415..649fac2e1ccb 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -257,7 +257,7 @@ drm_sched_rq_select_entity_fifo(struct drm_sched_rq *rq)
  *
  * Finish the job's fence and wake up the worker thread.
  */
-static void drm_sched_job_done(struct drm_sched_job *s_job)
+static void drm_sched_job_done(struct drm_sched_job *s_job, int result)
 {
 	struct drm_sched_fence *s_fence = s_job->s_fence;
 	struct drm_gpu_scheduler *sched = s_fence->sched;
@@ -268,7 +268,7 @@ static void drm_sched_job_done(struct drm_sched_job *s_job)
 	trace_drm_sched_process_job(s_fence);
 
 	dma_fence_get(&s_fence->finished);
-	drm_sched_fence_finished(s_fence);
+	drm_sched_fence_finished(s_fence, result);
 	dma_fence_put(&s_fence->finished);
 	wake_up_interruptible(&sched->wake_up_worker);
 }
@@ -282,7 +282,7 @@ static void drm_sched_job_done_cb(struct dma_fence *f, struct dma_fence_cb *cb)
 {
 	struct drm_sched_job *s_job = container_of(cb, struct drm_sched_job, cb);
 
-	drm_sched_job_done(s_job);
+	drm_sched_job_done(s_job, f->error);
 }
 
 /**
@@ -533,12 +533,12 @@ void drm_sched_start(struct drm_gpu_scheduler *sched, bool full_recovery)
 			r = dma_fence_add_callback(fence, &s_job->cb,
 						   drm_sched_job_done_cb);
 			if (r == -ENOENT)
-				drm_sched_job_done(s_job);
+				drm_sched_job_done(s_job, fence->error);
 			else if (r)
 				DRM_DEV_ERROR(sched->dev, "fence add callback failed (%d)\n",
 					  r);
 		} else
-			drm_sched_job_done(s_job);
+			drm_sched_job_done(s_job, 0);
 	}
 
 	if (full_recovery) {
@@ -1010,15 +1010,13 @@ static int drm_sched_main(void *param)
 			r = dma_fence_add_callback(fence, &sched_job->cb,
 						   drm_sched_job_done_cb);
 			if (r == -ENOENT)
-				drm_sched_job_done(sched_job);
+				drm_sched_job_done(sched_job, fence->error);
 			else if (r)
 				DRM_DEV_ERROR(sched->dev, "fence add callback failed (%d)\n",
 					  r);
 		} else {
-			if (IS_ERR(fence))
-				dma_fence_set_error(&s_fence->finished, PTR_ERR(fence));
-
-			drm_sched_job_done(sched_job);
+			drm_sched_job_done(sched_job, IS_ERR(fence) ?
+					   PTR_ERR(fence) : 0);
 		}
 
 		wake_up(&sched->job_scheduled);
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index ca857ec9e7eb..5c1df6b12ced 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -569,7 +569,7 @@ void drm_sched_fence_init(struct drm_sched_fence *fence,
 void drm_sched_fence_free(struct drm_sched_fence *fence);
 
 void drm_sched_fence_scheduled(struct drm_sched_fence *fence);
-void drm_sched_fence_finished(struct drm_sched_fence *fence);
+void drm_sched_fence_finished(struct drm_sched_fence *fence, int result);
 
 unsigned long drm_sched_suspend_timeout(struct drm_gpu_scheduler *sched);
 void drm_sched_resume_timeout(struct drm_gpu_scheduler *sched,
-- 
2.34.1

[PATCH 2/8] drm/scheduler: add drm_sched_entity_error and use rcu for last_scheduled

[Christian König]

Switch to using RCU handling for the last scheduled job and add a
function to return the error code of it.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/scheduler/sched_entity.c | 39 +++++++++++++++++++-----
 include/drm/gpu_scheduler.h              |  3 +-
 2 files changed, 33 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
index eaf71fe15ed3..d3f4ada6a68e 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -72,7 +72,7 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
 	entity->num_sched_list = num_sched_list;
 	entity->priority = priority;
 	entity->sched_list = num_sched_list > 1 ? sched_list : NULL;
-	entity->last_scheduled = NULL;
+	RCU_INIT_POINTER(entity->last_scheduled, NULL);
 	RB_CLEAR_NODE(&entity->rb_tree_node);
 
 	if(num_sched_list)
@@ -140,6 +140,27 @@ bool drm_sched_entity_is_ready(struct drm_sched_entity *entity)
 	return true;
 }
 
+/**
+ * drm_sched_entity_error - return error of last scheduled job
+ * @entity: scheduler entity to check
+ *
+ * Opportunistically return the error of the last scheduled job. Result can
+ * change any time when new jobs are pushed to the hw.
+ */
+int drm_sched_entity_error(struct drm_sched_entity *entity)
+{
+	struct dma_fence *fence;
+	int r;
+
+	rcu_read_lock();
+	fence = rcu_dereference(entity->last_scheduled);
+	r = fence ? fence->error : 0;
+	rcu_read_unlock();
+
+	return r;
+}
+EXPORT_SYMBOL(drm_sched_entity_error);
+
 static void drm_sched_entity_kill_jobs_work(struct work_struct *wrk)
 {
 	struct drm_sched_job *job = container_of(wrk, typeof(*job), work);
@@ -191,7 +212,9 @@ static void drm_sched_entity_kill(struct drm_sched_entity *entity)
 	/* Make sure this entity is not used by the scheduler at the moment */
 	wait_for_completion(&entity->entity_idle);
 
-	prev = dma_fence_get(entity->last_scheduled);
+	/* The entity is guaranteed to not be used by the scheduler */
+	prev = rcu_dereference_check(entity->last_scheduled, true);
+	dma_fence_get(prev);
 	while ((job = to_drm_sched_job(spsc_queue_pop(&entity->job_queue)))) {
 		struct drm_sched_fence *s_fence = job->s_fence;
 
@@ -278,8 +301,8 @@ void drm_sched_entity_fini(struct drm_sched_entity *entity)
 		entity->dependency = NULL;
 	}
 
-	dma_fence_put(entity->last_scheduled);
-	entity->last_scheduled = NULL;
+	dma_fence_put(rcu_dereference_check(entity->last_scheduled, true));
+	RCU_INIT_POINTER(entity->last_scheduled, NULL);
 }
 EXPORT_SYMBOL(drm_sched_entity_fini);
 
@@ -421,9 +444,9 @@ struct drm_sched_job *drm_sched_entity_pop_job(struct drm_sched_entity *entity)
 	if (entity->guilty && atomic_read(entity->guilty))
 		dma_fence_set_error(&sched_job->s_fence->finished, -ECANCELED);
 
-	dma_fence_put(entity->last_scheduled);
-
-	entity->last_scheduled = dma_fence_get(&sched_job->s_fence->finished);
+	dma_fence_put(rcu_dereference_check(entity->last_scheduled, true));
+	rcu_assign_pointer(entity->last_scheduled,
+			   dma_fence_get(&sched_job->s_fence->finished));
 
 	/*
 	 * If the queue is empty we allow drm_sched_entity_select_rq() to
@@ -471,7 +494,7 @@ void drm_sched_entity_select_rq(struct drm_sched_entity *entity)
 	 */
 	smp_rmb();
 
-	fence = entity->last_scheduled;
+	fence = rcu_dereference_check(entity->last_scheduled, true);
 
 	/* stay on the same engine if the previous job hasn't finished */
 	if (fence && !dma_fence_is_signaled(fence))
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index 5c1df6b12ced..6084459b2def 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -190,7 +190,7 @@ struct drm_sched_entity {
 	 * by the scheduler thread, can be accessed locklessly from
 	 * drm_sched_job_arm() iff the queue is empty.
 	 */
-	struct dma_fence                *last_scheduled;
+	struct dma_fence __rcu		*last_scheduled;
 
 	/**
 	 * @last_user: last group leader pushing a job into the entity.
@@ -561,6 +561,7 @@ void drm_sched_entity_push_job(struct drm_sched_job *sched_job);
 void drm_sched_entity_set_priority(struct drm_sched_entity *entity,
 				   enum drm_sched_priority priority);
 bool drm_sched_entity_is_ready(struct drm_sched_entity *entity);
+int drm_sched_entity_error(struct drm_sched_entity *entity);
 
 struct drm_sched_fence *drm_sched_fence_alloc(
 	struct drm_sched_entity *s_entity, void *owner);
-- 
2.34.1

[PATCH 3/8] drm/amdgpu: add amdgpu_error_* debugfs file

[Christian König]

This allows us to insert some error codes into the bottom of the pipeline
on an engine.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c | 24 +++++++++++++++++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c  | 15 ++++++++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h  |  1 +
 3 files changed, 40 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
index f52d0ba91a77..877fae84b8ed 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
@@ -693,6 +693,30 @@ void amdgpu_fence_driver_clear_job_fences(struct amdgpu_ring *ring)
 	}
 }
 
+/**
+ * amdgpu_fence_driver_set_error - set error code on fences
+ * @ring: the ring which contains the fences
+ * @error: the error code to set
+ *
+ * Set an error code to all the fences pending on the ring.
+ */
+void amdgpu_fence_driver_set_error(struct amdgpu_ring *ring, int error)
+{
+	struct amdgpu_fence_driver *drv = &ring->fence_drv;
+	unsigned long flags;
+
+	spin_lock_irqsave(&drv->lock, flags);
+	for (unsigned int i = 0; i <= drv->num_fences_mask; ++i) {
+		struct dma_fence *fence;
+
+		fence = rcu_dereference_protected(drv->fences[i],
+						  lockdep_is_held(&drv->lock));
+		if (fence && !dma_fence_is_signaled_locked(fence))
+			dma_fence_set_error(fence, error);
+	}
+	spin_unlock_irqrestore(&drv->lock, flags);
+}
+
 /**
  * amdgpu_fence_driver_force_completion - force signal latest fence of ring
  *
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
index f676c236b657..d3ad29d932b8 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
@@ -507,6 +507,17 @@ static const struct file_operations amdgpu_debugfs_ring_fops = {
 	.llseek = default_llseek
 };
 
+static int amdgpu_debugfs_ring_error(void *data, u64 val)
+{
+	struct amdgpu_ring *ring = data;
+
+	amdgpu_fence_driver_set_error(ring, val);
+	return 0;
+}
+
+DEFINE_DEBUGFS_ATTRIBUTE_SIGNED(amdgpu_debugfs_error_fops, NULL,
+				amdgpu_debugfs_ring_error, "%lld\n");
+
 #endif
 
 void amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
@@ -522,6 +533,10 @@ void amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
 				 &amdgpu_debugfs_ring_fops,
 				 ring->ring_size + 12);
 
+	sprintf(name, "amdgpu_error_%s", ring->name);
+	debugfs_create_file(name, 0200, root, ring,
+			    &amdgpu_debugfs_error_fops);
+
 #endif
 }
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
index e0d02cd8e63c..04ac055c3942 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
@@ -125,6 +125,7 @@ struct amdgpu_fence_driver {
 extern const struct drm_sched_backend_ops amdgpu_sched_ops;
 
 void amdgpu_fence_driver_clear_job_fences(struct amdgpu_ring *ring);
+void amdgpu_fence_driver_set_error(struct amdgpu_ring *ring, int error);
 void amdgpu_fence_driver_force_completion(struct amdgpu_ring *ring);
 
 int amdgpu_fence_driver_init_ring(struct amdgpu_ring *ring);
-- 
2.34.1

[PATCH 4/8] drm/amdgpu: mark force completed fences with -ECANCELED

[Christian König]

When we force complete fences we should mark them as canceled.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
index 877fae84b8ed..a7627cc0118d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
@@ -725,6 +725,7 @@ void amdgpu_fence_driver_set_error(struct amdgpu_ring *ring, int error)
  */
 void amdgpu_fence_driver_force_completion(struct amdgpu_ring *ring)
 {
+	amdgpu_fence_driver_set_error(ring, -ECANCELED);
 	amdgpu_fence_write(ring, ring->fence_drv.sync_seq);
 	amdgpu_fence_process(ring);
 }
-- 
2.34.1

[PATCH 5/8] drm/amdgpu: mark soft recovered fences with -ENODATA

[Christian König]

Set the fence error code before trying to soft-recover it.

It gets overwritten when a hard recovery is required.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
index d3ad29d932b8..083b1020b421 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
@@ -432,11 +432,18 @@ void amdgpu_ring_emit_reg_write_reg_wait_helper(struct amdgpu_ring *ring,
 bool amdgpu_ring_soft_recovery(struct amdgpu_ring *ring, unsigned int vmid,
 			       struct dma_fence *fence)
 {
+	unsigned long flags;
+
 	ktime_t deadline = ktime_add_us(ktime_get(), 10000);
 
 	if (amdgpu_sriov_vf(ring->adev) || !ring->funcs->soft_recovery || !fence)
 		return false;
 
+	spin_lock_irqsave(fence->lock, flags);
+	if (!dma_fence_is_signaled_locked(fence))
+		dma_fence_set_error(fence, -ENODATA);
+	spin_unlock_irqrestore(fence->lock, flags);
+
 	atomic_inc(&ring->adev->gpu_reset_counter);
 	while (!dma_fence_is_signaled(fence) &&
 	       ktime_to_ns(ktime_sub(deadline, ktime_get())) > 0)
-- 
2.34.1

[PATCH 6/8] drm/amdgpu: abort submissions during prepare on error

[Christian König]

Forward errors from previous submissions to this one.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
index 0a950c1c8782..5462f0406b02 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
@@ -256,16 +256,27 @@ amdgpu_job_prepare_job(struct drm_sched_job *sched_job,
 	struct dma_fence *fence = NULL;
 	int r;
 
+	/* Ignore soft recovered fences here */
+	r = drm_sched_entity_error(s_entity);
+	if (r && r != -ENODATA)
+		goto error;
+
 	if (job->gang_submit)
 		fence = amdgpu_device_switch_gang(ring->adev, job->gang_submit);
 
 	while (!fence && job->vm && !job->vmid) {
 		r = amdgpu_vmid_grab(job->vm, ring, job, &fence);
-		if (r)
+		if (r) {
 			DRM_ERROR("Error getting VM ID (%d)\n", r);
+			goto error;
+		}
 	}
 
 	return fence;
+
+error:
+	dma_fence_set_error(&job->base.s_fence->finished, r);
+	return NULL;
 }
 
 static struct dma_fence *amdgpu_job_run(struct drm_sched_job *sched_job)
-- 
2.34.1

[PATCH 7/8] drm/amdgpu: reset VM when an error is detected

[Christian König]

When some problem with the updates of page tables is detected reset the
state machine of the VM and re-create all page tables from scratch.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 81 +++++++++++++++++++++-----
 1 file changed, 65 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index fa8f48a0fa84..8082c9e006b1 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -265,6 +265,32 @@ static void amdgpu_vm_bo_done(struct amdgpu_vm_bo_base *vm_bo)
 	spin_unlock(&vm_bo->vm->status_lock);
 }
 
+/**
+ * amdgpu_vm_bo_reset_state_machine - reset the vm_bo state machine
+ * @vm: the VM which state machine to reset
+ *
+ * Move all vm_bo object in the VM into a state where they will be updated
+ * again during validation.
+ */
+static void amdgpu_vm_bo_reset_state_machine(struct amdgpu_vm *vm)
+{
+	struct amdgpu_vm_bo_base *vm_bo, *tmp;
+
+	spin_lock(&vm->status_lock);
+	list_splice_init(&vm->done, &vm->invalidated);
+	list_for_each_entry(vm_bo, &vm->invalidated, vm_status)
+		vm_bo->moved = true;
+	list_for_each_entry_safe(vm_bo, tmp, &vm->idle, vm_status) {
+		struct amdgpu_bo *bo = vm_bo->bo;
+
+		if (!bo || bo->tbo.type != ttm_bo_type_kernel)
+			list_move(&vm_bo->vm_status, &vm_bo->vm->moved);
+		else if (bo->parent)
+			list_move(&vm_bo->vm_status, &vm_bo->vm->relocated);
+	}
+	spin_unlock(&vm->status_lock);
+}
+
 /**
  * amdgpu_vm_bo_base_init - Adds bo to the list of bos associated with the vm
  *
@@ -350,6 +376,34 @@ void amdgpu_vm_move_to_lru_tail(struct amdgpu_device *adev,
 	spin_unlock(&adev->mman.bdev.lru_lock);
 }
 
+/* Create scheduler entities for page table updates */
+static int amdgpu_vm_init_entities(struct amdgpu_device *adev,
+				   struct amdgpu_vm *vm)
+{
+	int r;
+
+	r = drm_sched_entity_init(&vm->immediate, DRM_SCHED_PRIORITY_NORMAL,
+				  adev->vm_manager.vm_pte_scheds,
+				  adev->vm_manager.vm_pte_num_scheds, NULL);
+	if (r)
+		goto error;
+
+	return drm_sched_entity_init(&vm->delayed, DRM_SCHED_PRIORITY_NORMAL,
+				     adev->vm_manager.vm_pte_scheds,
+				     adev->vm_manager.vm_pte_num_scheds, NULL);
+
+error:
+	drm_sched_entity_destroy(&vm->immediate);
+	return r;
+}
+
+/* Destroy the entities for page table updates again */
+static void amdgpu_vm_fini_entities(struct amdgpu_vm *vm)
+{
+	drm_sched_entity_destroy(&vm->immediate);
+	drm_sched_entity_destroy(&vm->delayed);
+}
+
 /**
  * amdgpu_vm_validate_pt_bos - validate the page table BOs
  *
@@ -372,6 +426,14 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 	struct amdgpu_bo *bo;
 	int r;
 
+	if (drm_sched_entity_error(&vm->delayed)) {
+		amdgpu_vm_bo_reset_state_machine(vm);
+		amdgpu_vm_fini_entities(vm);
+		r = amdgpu_vm_init_entities(adev, vm);
+		if (r)
+			return r;
+	}
+
 	spin_lock(&vm->status_lock);
 	while (!list_empty(&vm->evicted)) {
 		bo_base = list_first_entry(&vm->evicted,
@@ -2037,19 +2099,10 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm)
 	INIT_LIST_HEAD(&vm->pt_freed);
 	INIT_WORK(&vm->pt_free_work, amdgpu_vm_pt_free_work);
 
-	/* create scheduler entities for page table updates */
-	r = drm_sched_entity_init(&vm->immediate, DRM_SCHED_PRIORITY_NORMAL,
-				  adev->vm_manager.vm_pte_scheds,
-				  adev->vm_manager.vm_pte_num_scheds, NULL);
+	r = amdgpu_vm_init_entities(adev, vm);
 	if (r)
 		return r;
 
-	r = drm_sched_entity_init(&vm->delayed, DRM_SCHED_PRIORITY_NORMAL,
-				  adev->vm_manager.vm_pte_scheds,
-				  adev->vm_manager.vm_pte_num_scheds, NULL);
-	if (r)
-		goto error_free_immediate;
-
 	vm->pte_support_ats = false;
 	vm->is_compute_context = false;
 
@@ -2109,10 +2162,7 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm)
 error_free_delayed:
 	dma_fence_put(vm->last_tlb_flush);
 	dma_fence_put(vm->last_unlocked);
-	drm_sched_entity_destroy(&vm->delayed);
-
-error_free_immediate:
-	drm_sched_entity_destroy(&vm->immediate);
+	amdgpu_vm_fini_entities(vm);
 
 	return r;
 }
@@ -2265,8 +2315,7 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm)
 	amdgpu_bo_unref(&root);
 	WARN_ON(vm->root.bo);
 
-	drm_sched_entity_destroy(&vm->immediate);
-	drm_sched_entity_destroy(&vm->delayed);
+	amdgpu_vm_fini_entities(vm);
 
 	if (!RB_EMPTY_ROOT(&vm->va.rb_root)) {
 		dev_err(adev->dev, "still active bo inside vm\n");
-- 
2.34.1

[PATCH 8/8] drm/amdgpu: add VM generation token

[Christian König]

Instead of using the VRAM lost counter add a 64bit token which indicates
if a context or job is still valid to use.

Should the VRAM be lost or the page tables need re-creation the token will
change indicating that userspace needs to act and re-create the contexts
and re-submit the work.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c  |  2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c |  5 +++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h |  2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_job.c |  4 ++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_job.h |  2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c  | 26 +++++++++++++++++++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h  |  4 ++++
 7 files changed, 38 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index 49db1d0c923a..4beec4220aed 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -306,7 +306,7 @@ static int amdgpu_cs_pass1(struct amdgpu_cs_parser *p,
 	}
 	p->gang_leader = p->jobs[p->gang_leader_idx];
 
-	if (p->ctx->vram_lost_counter != p->gang_leader->vram_lost_counter) {
+	if (p->ctx->generation != p->gang_leader->generation) {
 		ret = -ECANCELED;
 		goto free_all_kdata;
 	}
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
index d2139ac12159..24994c9e65ad 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
@@ -303,6 +303,7 @@ static int amdgpu_ctx_get_stable_pstate(struct amdgpu_ctx *ctx,
 static int amdgpu_ctx_init(struct amdgpu_ctx_mgr *mgr, int32_t priority,
 			   struct drm_file *filp, struct amdgpu_ctx *ctx)
 {
+	struct amdgpu_fpriv *fpriv = filp->driver_priv;
 	u32 current_stable_pstate;
 	int r;
 
@@ -318,7 +319,7 @@ static int amdgpu_ctx_init(struct amdgpu_ctx_mgr *mgr, int32_t priority,
 
 	ctx->reset_counter = atomic_read(&mgr->adev->gpu_reset_counter);
 	ctx->reset_counter_query = ctx->reset_counter;
-	ctx->vram_lost_counter = atomic_read(&mgr->adev->vram_lost_counter);
+	ctx->generation = amdgpu_vm_generation(mgr->adev, &fpriv->vm);
 	ctx->init_priority = priority;
 	ctx->override_priority = AMDGPU_CTX_PRIORITY_UNSET;
 
@@ -570,7 +571,7 @@ static int amdgpu_ctx_query2(struct amdgpu_device *adev,
 	if (ctx->reset_counter != atomic_read(&adev->gpu_reset_counter))
 		out->state.flags |= AMDGPU_CTX_QUERY2_FLAGS_RESET;
 
-	if (ctx->vram_lost_counter != atomic_read(&adev->vram_lost_counter))
+	if (ctx->generation != amdgpu_vm_generation(adev, &fpriv->vm))
 		out->state.flags |= AMDGPU_CTX_QUERY2_FLAGS_VRAMLOST;
 
 	if (atomic_read(&ctx->guilty))
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
index 0fa0e56daf67..5fd79f94e2d0 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
@@ -47,7 +47,7 @@ struct amdgpu_ctx {
 	struct amdgpu_ctx_mgr		*mgr;
 	unsigned			reset_counter;
 	unsigned			reset_counter_query;
-	uint32_t			vram_lost_counter;
+	uint64_t			generation;
 	spinlock_t			ring_lock;
 	struct amdgpu_ctx_entity	*entities[AMDGPU_HW_IP_NUM][AMDGPU_MAX_ENTITY_NUM];
 	bool				preamble_presented;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
index 5462f0406b02..57f8f8b3cd8a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
@@ -107,7 +107,7 @@ int amdgpu_job_alloc(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 	(*job)->vm = vm;
 
 	amdgpu_sync_create(&(*job)->explicit_sync);
-	(*job)->vram_lost_counter = atomic_read(&adev->vram_lost_counter);
+	(*job)->generation = amdgpu_vm_generation(adev, vm);
 	(*job)->vm_pd_addr = AMDGPU_BO_INVALID_OFFSET;
 
 	if (!entity)
@@ -293,7 +293,7 @@ static struct dma_fence *amdgpu_job_run(struct drm_sched_job *sched_job)
 	trace_amdgpu_sched_run_job(job);
 
 	/* Skip job if VRAM is lost and never resubmit gangs */
-	if (job->vram_lost_counter != atomic_read(&adev->vram_lost_counter) ||
+	if (job->generation != amdgpu_vm_generation(adev, job->vm) ||
 	    (job->job_run_counter && job->gang_submit))
 		dma_fence_set_error(finished, -ECANCELED);
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
index 52f2e313ea17..a2931ff08dad 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
@@ -61,7 +61,7 @@ struct amdgpu_job {
 	uint32_t		gds_base, gds_size;
 	uint32_t		gws_base, gws_size;
 	uint32_t		oa_base, oa_size;
-	uint32_t		vram_lost_counter;
+	uint64_t		generation;
 
 	/* user fence handling */
 	uint64_t		uf_addr;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 8082c9e006b1..158176b2f47e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -404,6 +404,30 @@ static void amdgpu_vm_fini_entities(struct amdgpu_vm *vm)
 	drm_sched_entity_destroy(&vm->delayed);
 }
 
+/**
+ * amdgpu_vm_generation - return the page table re-generation counter
+ * @adev: the amdgpu_device
+ * @vm: optional VM to check, might be NULL
+ *
+ * Returns a page table re-generation token to allow checking if submissions
+ * are still valid to use this VM. The VM parameter might be NULL in which case
+ * just the VRAM lost counter will be used.
+ */
+uint64_t amdgpu_vm_generation(struct amdgpu_device *adev, struct amdgpu_vm *vm)
+{
+	uint64_t result = (u64)atomic_read(&adev->vram_lost_counter) << 32;
+
+	if (!vm)
+		return result;
+
+	result += vm->generation;
+	/* Add one if the page tables will be re-generated on next CS */
+	if (drm_sched_entity_error(&vm->delayed))
+		++result;
+
+	return result;
+}
+
 /**
  * amdgpu_vm_validate_pt_bos - validate the page table BOs
  *
@@ -427,6 +451,7 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 	int r;
 
 	if (drm_sched_entity_error(&vm->delayed)) {
+		++vm->generation;
 		amdgpu_vm_bo_reset_state_machine(vm);
 		amdgpu_vm_fini_entities(vm);
 		r = amdgpu_vm_init_entities(adev, vm);
@@ -2122,6 +2147,7 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm)
 	vm->last_update = NULL;
 	vm->last_unlocked = dma_fence_get_stub();
 	vm->last_tlb_flush = dma_fence_get_stub();
+	vm->generation = 0;
 
 	mutex_init(&vm->eviction_lock);
 	vm->evicting = false;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index 8ae45a0896cd..e8390d439895 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -292,6 +292,9 @@ struct amdgpu_vm {
 	atomic64_t		tlb_seq;
 	struct dma_fence	*last_tlb_flush;
 
+	/* How many times we had to re-generate the page tables */
+	uint64_t		generation;
+
 	/* Last unlocked submission to the scheduler entities */
 	struct dma_fence	*last_unlocked;
 
@@ -391,6 +394,7 @@ void amdgpu_vm_get_pd_bo(struct amdgpu_vm *vm,
 			 struct list_head *validated,
 			 struct amdgpu_bo_list_entry *entry);
 bool amdgpu_vm_ready(struct amdgpu_vm *vm);
+uint64_t amdgpu_vm_generation(struct amdgpu_device *adev, struct amdgpu_vm *vm);
 int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 			      int (*callback)(void *p, struct amdgpu_bo *bo),
 			      void *param);
-- 
2.34.1

Re: [PATCH 1/8] drm/scheduler: properly forward fence errors

[Luben Tuikov]

Hi Christian,

Thanks for working on this.

Series is,
Reviewed-by: Luben Tuikov <luben.tuikov@amd.com>

Regards,
Luben

On 2023-04-20 07:57, Christian König wrote:
> When a hw fence is signaled with an error properly forward that to the
> finished fence.
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/gpu/drm/scheduler/sched_entity.c |  4 +---
>  drivers/gpu/drm/scheduler/sched_fence.c  |  4 +++-
>  drivers/gpu/drm/scheduler/sched_main.c   | 18 ++++++++----------
>  include/drm/gpu_scheduler.h              |  2 +-
>  4 files changed, 13 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
> index 15d04a0ec623..eaf71fe15ed3 100644
> --- a/drivers/gpu/drm/scheduler/sched_entity.c
> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
> @@ -144,7 +144,7 @@ static void drm_sched_entity_kill_jobs_work(struct work_struct *wrk)
>  {
>  	struct drm_sched_job *job = container_of(wrk, typeof(*job), work);
>  
> -	drm_sched_fence_finished(job->s_fence);
> +	drm_sched_fence_finished(job->s_fence, -ESRCH);
>  	WARN_ON(job->s_fence->parent);
>  	job->sched->ops->free_job(job);
>  }
> @@ -195,8 +195,6 @@ static void drm_sched_entity_kill(struct drm_sched_entity *entity)
>  	while ((job = to_drm_sched_job(spsc_queue_pop(&entity->job_queue)))) {
>  		struct drm_sched_fence *s_fence = job->s_fence;
>  
> -		dma_fence_set_error(&s_fence->finished, -ESRCH);
> -
>  		dma_fence_get(&s_fence->finished);
>  		if (!prev || dma_fence_add_callback(prev, &job->finish_cb,
>  					   drm_sched_entity_kill_jobs_cb))
> diff --git a/drivers/gpu/drm/scheduler/sched_fence.c b/drivers/gpu/drm/scheduler/sched_fence.c
> index 7fd869520ef2..1a6bea98c5cc 100644
> --- a/drivers/gpu/drm/scheduler/sched_fence.c
> +++ b/drivers/gpu/drm/scheduler/sched_fence.c
> @@ -53,8 +53,10 @@ void drm_sched_fence_scheduled(struct drm_sched_fence *fence)
>  	dma_fence_signal(&fence->scheduled);
>  }
>  
> -void drm_sched_fence_finished(struct drm_sched_fence *fence)
> +void drm_sched_fence_finished(struct drm_sched_fence *fence, int result)
>  {
> +	if (result)
> +		dma_fence_set_error(&fence->finished, result);
>  	dma_fence_signal(&fence->finished);
>  }
>  
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> index fcd4bfef7415..649fac2e1ccb 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -257,7 +257,7 @@ drm_sched_rq_select_entity_fifo(struct drm_sched_rq *rq)
>   *
>   * Finish the job's fence and wake up the worker thread.
>   */
> -static void drm_sched_job_done(struct drm_sched_job *s_job)
> +static void drm_sched_job_done(struct drm_sched_job *s_job, int result)
>  {
>  	struct drm_sched_fence *s_fence = s_job->s_fence;
>  	struct drm_gpu_scheduler *sched = s_fence->sched;
> @@ -268,7 +268,7 @@ static void drm_sched_job_done(struct drm_sched_job *s_job)
>  	trace_drm_sched_process_job(s_fence);
>  
>  	dma_fence_get(&s_fence->finished);
> -	drm_sched_fence_finished(s_fence);
> +	drm_sched_fence_finished(s_fence, result);
>  	dma_fence_put(&s_fence->finished);
>  	wake_up_interruptible(&sched->wake_up_worker);
>  }
> @@ -282,7 +282,7 @@ static void drm_sched_job_done_cb(struct dma_fence *f, struct dma_fence_cb *cb)
>  {
>  	struct drm_sched_job *s_job = container_of(cb, struct drm_sched_job, cb);
>  
> -	drm_sched_job_done(s_job);
> +	drm_sched_job_done(s_job, f->error);
>  }
>  
>  /**
> @@ -533,12 +533,12 @@ void drm_sched_start(struct drm_gpu_scheduler *sched, bool full_recovery)
>  			r = dma_fence_add_callback(fence, &s_job->cb,
>  						   drm_sched_job_done_cb);
>  			if (r == -ENOENT)
> -				drm_sched_job_done(s_job);
> +				drm_sched_job_done(s_job, fence->error);
>  			else if (r)
>  				DRM_DEV_ERROR(sched->dev, "fence add callback failed (%d)\n",
>  					  r);
>  		} else
> -			drm_sched_job_done(s_job);
> +			drm_sched_job_done(s_job, 0);
>  	}
>  
>  	if (full_recovery) {
> @@ -1010,15 +1010,13 @@ static int drm_sched_main(void *param)
>  			r = dma_fence_add_callback(fence, &sched_job->cb,
>  						   drm_sched_job_done_cb);
>  			if (r == -ENOENT)
> -				drm_sched_job_done(sched_job);
> +				drm_sched_job_done(sched_job, fence->error);
>  			else if (r)
>  				DRM_DEV_ERROR(sched->dev, "fence add callback failed (%d)\n",
>  					  r);
>  		} else {
> -			if (IS_ERR(fence))
> -				dma_fence_set_error(&s_fence->finished, PTR_ERR(fence));
> -
> -			drm_sched_job_done(sched_job);
> +			drm_sched_job_done(sched_job, IS_ERR(fence) ?
> +					   PTR_ERR(fence) : 0);
>  		}
>  
>  		wake_up(&sched->job_scheduled);
> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> index ca857ec9e7eb..5c1df6b12ced 100644
> --- a/include/drm/gpu_scheduler.h
> +++ b/include/drm/gpu_scheduler.h
> @@ -569,7 +569,7 @@ void drm_sched_fence_init(struct drm_sched_fence *fence,
>  void drm_sched_fence_free(struct drm_sched_fence *fence);
>  
>  void drm_sched_fence_scheduled(struct drm_sched_fence *fence);
> -void drm_sched_fence_finished(struct drm_sched_fence *fence);
> +void drm_sched_fence_finished(struct drm_sched_fence *fence, int result);
>  
>  unsigned long drm_sched_suspend_timeout(struct drm_gpu_scheduler *sched);
>  void drm_sched_resume_timeout(struct drm_gpu_scheduler *sched,

Re: [PATCH 1/8] drm/scheduler: properly forward fence errors

[Christian König]

Alex can I merge that through drm-misc-next or do we really need 
amd-staging-drm-next?

Christian.

Am 21.04.23 um 07:22 schrieb Luben Tuikov:
> Hi Christian,
>
> Thanks for working on this.
>
> Series is,
> Reviewed-by: Luben Tuikov <luben.tuikov@amd.com>
>
> Regards,
> Luben
>
> On 2023-04-20 07:57, Christian König wrote:
>> When a hw fence is signaled with an error properly forward that to the
>> finished fence.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/gpu/drm/scheduler/sched_entity.c |  4 +---
>>   drivers/gpu/drm/scheduler/sched_fence.c  |  4 +++-
>>   drivers/gpu/drm/scheduler/sched_main.c   | 18 ++++++++----------
>>   include/drm/gpu_scheduler.h              |  2 +-
>>   4 files changed, 13 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
>> index 15d04a0ec623..eaf71fe15ed3 100644
>> --- a/drivers/gpu/drm/scheduler/sched_entity.c
>> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
>> @@ -144,7 +144,7 @@ static void drm_sched_entity_kill_jobs_work(struct work_struct *wrk)
>>   {
>>   	struct drm_sched_job *job = container_of(wrk, typeof(*job), work);
>>   
>> -	drm_sched_fence_finished(job->s_fence);
>> +	drm_sched_fence_finished(job->s_fence, -ESRCH);
>>   	WARN_ON(job->s_fence->parent);
>>   	job->sched->ops->free_job(job);
>>   }
>> @@ -195,8 +195,6 @@ static void drm_sched_entity_kill(struct drm_sched_entity *entity)
>>   	while ((job = to_drm_sched_job(spsc_queue_pop(&entity->job_queue)))) {
>>   		struct drm_sched_fence *s_fence = job->s_fence;
>>   
>> -		dma_fence_set_error(&s_fence->finished, -ESRCH);
>> -
>>   		dma_fence_get(&s_fence->finished);
>>   		if (!prev || dma_fence_add_callback(prev, &job->finish_cb,
>>   					   drm_sched_entity_kill_jobs_cb))
>> diff --git a/drivers/gpu/drm/scheduler/sched_fence.c b/drivers/gpu/drm/scheduler/sched_fence.c
>> index 7fd869520ef2..1a6bea98c5cc 100644
>> --- a/drivers/gpu/drm/scheduler/sched_fence.c
>> +++ b/drivers/gpu/drm/scheduler/sched_fence.c
>> @@ -53,8 +53,10 @@ void drm_sched_fence_scheduled(struct drm_sched_fence *fence)
>>   	dma_fence_signal(&fence->scheduled);
>>   }
>>   
>> -void drm_sched_fence_finished(struct drm_sched_fence *fence)
>> +void drm_sched_fence_finished(struct drm_sched_fence *fence, int result)
>>   {
>> +	if (result)
>> +		dma_fence_set_error(&fence->finished, result);
>>   	dma_fence_signal(&fence->finished);
>>   }
>>   
>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
>> index fcd4bfef7415..649fac2e1ccb 100644
>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>> @@ -257,7 +257,7 @@ drm_sched_rq_select_entity_fifo(struct drm_sched_rq *rq)
>>    *
>>    * Finish the job's fence and wake up the worker thread.
>>    */
>> -static void drm_sched_job_done(struct drm_sched_job *s_job)
>> +static void drm_sched_job_done(struct drm_sched_job *s_job, int result)
>>   {
>>   	struct drm_sched_fence *s_fence = s_job->s_fence;
>>   	struct drm_gpu_scheduler *sched = s_fence->sched;
>> @@ -268,7 +268,7 @@ static void drm_sched_job_done(struct drm_sched_job *s_job)
>>   	trace_drm_sched_process_job(s_fence);
>>   
>>   	dma_fence_get(&s_fence->finished);
>> -	drm_sched_fence_finished(s_fence);
>> +	drm_sched_fence_finished(s_fence, result);
>>   	dma_fence_put(&s_fence->finished);
>>   	wake_up_interruptible(&sched->wake_up_worker);
>>   }
>> @@ -282,7 +282,7 @@ static void drm_sched_job_done_cb(struct dma_fence *f, struct dma_fence_cb *cb)
>>   {
>>   	struct drm_sched_job *s_job = container_of(cb, struct drm_sched_job, cb);
>>   
>> -	drm_sched_job_done(s_job);
>> +	drm_sched_job_done(s_job, f->error);
>>   }
>>   
>>   /**
>> @@ -533,12 +533,12 @@ void drm_sched_start(struct drm_gpu_scheduler *sched, bool full_recovery)
>>   			r = dma_fence_add_callback(fence, &s_job->cb,
>>   						   drm_sched_job_done_cb);
>>   			if (r == -ENOENT)
>> -				drm_sched_job_done(s_job);
>> +				drm_sched_job_done(s_job, fence->error);
>>   			else if (r)
>>   				DRM_DEV_ERROR(sched->dev, "fence add callback failed (%d)\n",
>>   					  r);
>>   		} else
>> -			drm_sched_job_done(s_job);
>> +			drm_sched_job_done(s_job, 0);
>>   	}
>>   
>>   	if (full_recovery) {
>> @@ -1010,15 +1010,13 @@ static int drm_sched_main(void *param)
>>   			r = dma_fence_add_callback(fence, &sched_job->cb,
>>   						   drm_sched_job_done_cb);
>>   			if (r == -ENOENT)
>> -				drm_sched_job_done(sched_job);
>> +				drm_sched_job_done(sched_job, fence->error);
>>   			else if (r)
>>   				DRM_DEV_ERROR(sched->dev, "fence add callback failed (%d)\n",
>>   					  r);
>>   		} else {
>> -			if (IS_ERR(fence))
>> -				dma_fence_set_error(&s_fence->finished, PTR_ERR(fence));
>> -
>> -			drm_sched_job_done(sched_job);
>> +			drm_sched_job_done(sched_job, IS_ERR(fence) ?
>> +					   PTR_ERR(fence) : 0);
>>   		}
>>   
>>   		wake_up(&sched->job_scheduled);
>> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
>> index ca857ec9e7eb..5c1df6b12ced 100644
>> --- a/include/drm/gpu_scheduler.h
>> +++ b/include/drm/gpu_scheduler.h
>> @@ -569,7 +569,7 @@ void drm_sched_fence_init(struct drm_sched_fence *fence,
>>   void drm_sched_fence_free(struct drm_sched_fence *fence);
>>   
>>   void drm_sched_fence_scheduled(struct drm_sched_fence *fence);
>> -void drm_sched_fence_finished(struct drm_sched_fence *fence);
>> +void drm_sched_fence_finished(struct drm_sched_fence *fence, int result);
>>   
>>   unsigned long drm_sched_suspend_timeout(struct drm_gpu_scheduler *sched);
>>   void drm_sched_resume_timeout(struct drm_gpu_scheduler *sched,

Re: [PATCH 1/8] drm/scheduler: properly forward fence errors

[Deucher, Alexander]

[-- Attachment #1: Type: text/plain, Size: 7041 bytes --]

[AMD Official Use Only - General]

Sure.  We can pull it into amd-staging-drm-next as well if we need it for any customers in the short term.

Alex
________________________________
From: Christian König <ckoenig.leichtzumerken@gmail.com>
Sent: Friday, April 21, 2023 9:27 AM
To: amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>; Deucher, Alexander <Alexander.Deucher@amd.com>
Cc: Tuikov, Luben <Luben.Tuikov@amd.com>
Subject: Re: [PATCH 1/8] drm/scheduler: properly forward fence errors

Alex can I merge that through drm-misc-next or do we really need
amd-staging-drm-next?

Christian.

Am 21.04.23 um 07:22 schrieb Luben Tuikov:
> Hi Christian,
>
> Thanks for working on this.
>
> Series is,
> Reviewed-by: Luben Tuikov <luben.tuikov@amd.com>
>
> Regards,
> Luben
>
> On 2023-04-20 07:57, Christian König wrote:
>> When a hw fence is signaled with an error properly forward that to the
>> finished fence.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/gpu/drm/scheduler/sched_entity.c |  4 +---
>>   drivers/gpu/drm/scheduler/sched_fence.c  |  4 +++-
>>   drivers/gpu/drm/scheduler/sched_main.c   | 18 ++++++++----------
>>   include/drm/gpu_scheduler.h              |  2 +-
>>   4 files changed, 13 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
>> index 15d04a0ec623..eaf71fe15ed3 100644
>> --- a/drivers/gpu/drm/scheduler/sched_entity.c
>> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
>> @@ -144,7 +144,7 @@ static void drm_sched_entity_kill_jobs_work(struct work_struct *wrk)
>>   {
>>       struct drm_sched_job *job = container_of(wrk, typeof(*job), work);
>>
>> -    drm_sched_fence_finished(job->s_fence);
>> +    drm_sched_fence_finished(job->s_fence, -ESRCH);
>>       WARN_ON(job->s_fence->parent);
>>       job->sched->ops->free_job(job);
>>   }
>> @@ -195,8 +195,6 @@ static void drm_sched_entity_kill(struct drm_sched_entity *entity)
>>       while ((job = to_drm_sched_job(spsc_queue_pop(&entity->job_queue)))) {
>>               struct drm_sched_fence *s_fence = job->s_fence;
>>
>> -            dma_fence_set_error(&s_fence->finished, -ESRCH);
>> -
>>               dma_fence_get(&s_fence->finished);
>>               if (!prev || dma_fence_add_callback(prev, &job->finish_cb,
>>                                          drm_sched_entity_kill_jobs_cb))
>> diff --git a/drivers/gpu/drm/scheduler/sched_fence.c b/drivers/gpu/drm/scheduler/sched_fence.c
>> index 7fd869520ef2..1a6bea98c5cc 100644
>> --- a/drivers/gpu/drm/scheduler/sched_fence.c
>> +++ b/drivers/gpu/drm/scheduler/sched_fence.c
>> @@ -53,8 +53,10 @@ void drm_sched_fence_scheduled(struct drm_sched_fence *fence)
>>       dma_fence_signal(&fence->scheduled);
>>   }
>>
>> -void drm_sched_fence_finished(struct drm_sched_fence *fence)
>> +void drm_sched_fence_finished(struct drm_sched_fence *fence, int result)
>>   {
>> +    if (result)
>> +            dma_fence_set_error(&fence->finished, result);
>>       dma_fence_signal(&fence->finished);
>>   }
>>
>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
>> index fcd4bfef7415..649fac2e1ccb 100644
>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>> @@ -257,7 +257,7 @@ drm_sched_rq_select_entity_fifo(struct drm_sched_rq *rq)
>>    *
>>    * Finish the job's fence and wake up the worker thread.
>>    */
>> -static void drm_sched_job_done(struct drm_sched_job *s_job)
>> +static void drm_sched_job_done(struct drm_sched_job *s_job, int result)
>>   {
>>       struct drm_sched_fence *s_fence = s_job->s_fence;
>>       struct drm_gpu_scheduler *sched = s_fence->sched;
>> @@ -268,7 +268,7 @@ static void drm_sched_job_done(struct drm_sched_job *s_job)
>>       trace_drm_sched_process_job(s_fence);
>>
>>       dma_fence_get(&s_fence->finished);
>> -    drm_sched_fence_finished(s_fence);
>> +    drm_sched_fence_finished(s_fence, result);
>>       dma_fence_put(&s_fence->finished);
>>       wake_up_interruptible(&sched->wake_up_worker);
>>   }
>> @@ -282,7 +282,7 @@ static void drm_sched_job_done_cb(struct dma_fence *f, struct dma_fence_cb *cb)
>>   {
>>       struct drm_sched_job *s_job = container_of(cb, struct drm_sched_job, cb);
>>
>> -    drm_sched_job_done(s_job);
>> +    drm_sched_job_done(s_job, f->error);
>>   }
>>
>>   /**
>> @@ -533,12 +533,12 @@ void drm_sched_start(struct drm_gpu_scheduler *sched, bool full_recovery)
>>                       r = dma_fence_add_callback(fence, &s_job->cb,
>>                                                  drm_sched_job_done_cb);
>>                       if (r == -ENOENT)
>> -                            drm_sched_job_done(s_job);
>> +                            drm_sched_job_done(s_job, fence->error);
>>                       else if (r)
>>                               DRM_DEV_ERROR(sched->dev, "fence add callback failed (%d)\n",
>>                                         r);
>>               } else
>> -                    drm_sched_job_done(s_job);
>> +                    drm_sched_job_done(s_job, 0);
>>       }
>>
>>       if (full_recovery) {
>> @@ -1010,15 +1010,13 @@ static int drm_sched_main(void *param)
>>                       r = dma_fence_add_callback(fence, &sched_job->cb,
>>                                                  drm_sched_job_done_cb);
>>                       if (r == -ENOENT)
>> -                            drm_sched_job_done(sched_job);
>> +                            drm_sched_job_done(sched_job, fence->error);
>>                       else if (r)
>>                               DRM_DEV_ERROR(sched->dev, "fence add callback failed (%d)\n",
>>                                         r);
>>               } else {
>> -                    if (IS_ERR(fence))
>> -                            dma_fence_set_error(&s_fence->finished, PTR_ERR(fence));
>> -
>> -                    drm_sched_job_done(sched_job);
>> +                    drm_sched_job_done(sched_job, IS_ERR(fence) ?
>> +                                       PTR_ERR(fence) : 0);
>>               }
>>
>>               wake_up(&sched->job_scheduled);
>> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
>> index ca857ec9e7eb..5c1df6b12ced 100644
>> --- a/include/drm/gpu_scheduler.h
>> +++ b/include/drm/gpu_scheduler.h
>> @@ -569,7 +569,7 @@ void drm_sched_fence_init(struct drm_sched_fence *fence,
>>   void drm_sched_fence_free(struct drm_sched_fence *fence);
>>
>>   void drm_sched_fence_scheduled(struct drm_sched_fence *fence);
>> -void drm_sched_fence_finished(struct drm_sched_fence *fence);
>> +void drm_sched_fence_finished(struct drm_sched_fence *fence, int result);
>>
>>   unsigned long drm_sched_suspend_timeout(struct drm_gpu_scheduler *sched);
>>   void drm_sched_resume_timeout(struct drm_gpu_scheduler *sched,


[-- Attachment #2: Type: text/html, Size: 14883 bytes --]

Re: [PATCH 1/8] drm/scheduler: properly forward fence errors

[Christian König]

[-- Attachment #1: Type: text/plain, Size: 7582 bytes --]

I've pushed the scheduler patch to drm-misc-next and the whole set to 
amd-staging-drm-next.

Christian.

Am 21.04.23 um 15:40 schrieb Deucher, Alexander:
>
> [AMD Official Use Only - General]
>
>
> Sure.  We can pull it into amd-staging-drm-next as well if we need it 
> for any customers in the short term.
>
> Alex
> ------------------------------------------------------------------------
> *From:* Christian König <ckoenig.leichtzumerken@gmail.com>
> *Sent:* Friday, April 21, 2023 9:27 AM
> *To:* amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>; 
> Deucher, Alexander <Alexander.Deucher@amd.com>
> *Cc:* Tuikov, Luben <Luben.Tuikov@amd.com>
> *Subject:* Re: [PATCH 1/8] drm/scheduler: properly forward fence errors
> Alex can I merge that through drm-misc-next or do we really need
> amd-staging-drm-next?
>
> Christian.
>
> Am 21.04.23 um 07:22 schrieb Luben Tuikov:
> > Hi Christian,
> >
> > Thanks for working on this.
> >
> > Series is,
> > Reviewed-by: Luben Tuikov <luben.tuikov@amd.com>
> >
> > Regards,
> > Luben
> >
> > On 2023-04-20 07:57, Christian König wrote:
> >> When a hw fence is signaled with an error properly forward that to the
> >> finished fence.
> >>
> >> Signed-off-by: Christian König <christian.koenig@amd.com>
> >> ---
> >>   drivers/gpu/drm/scheduler/sched_entity.c |  4 +---
> >>   drivers/gpu/drm/scheduler/sched_fence.c  |  4 +++-
> >>   drivers/gpu/drm/scheduler/sched_main.c   | 18 ++++++++----------
> >>   include/drm/gpu_scheduler.h              |  2 +-
> >>   4 files changed, 13 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c 
> b/drivers/gpu/drm/scheduler/sched_entity.c
> >> index 15d04a0ec623..eaf71fe15ed3 100644
> >> --- a/drivers/gpu/drm/scheduler/sched_entity.c
> >> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
> >> @@ -144,7 +144,7 @@ static void 
> drm_sched_entity_kill_jobs_work(struct work_struct *wrk)
> >>   {
> >>       struct drm_sched_job *job = container_of(wrk, typeof(*job), 
> work);
> >>
> >> -    drm_sched_fence_finished(job->s_fence);
> >> +    drm_sched_fence_finished(job->s_fence, -ESRCH);
> >>       WARN_ON(job->s_fence->parent);
> >>       job->sched->ops->free_job(job);
> >>   }
> >> @@ -195,8 +195,6 @@ static void drm_sched_entity_kill(struct 
> drm_sched_entity *entity)
> >>       while ((job = 
> to_drm_sched_job(spsc_queue_pop(&entity->job_queue)))) {
> >>               struct drm_sched_fence *s_fence = job->s_fence;
> >>
> >> - dma_fence_set_error(&s_fence->finished, -ESRCH);
> >> -
> >> dma_fence_get(&s_fence->finished);
> >>               if (!prev || dma_fence_add_callback(prev, 
> &job->finish_cb,
> >> drm_sched_entity_kill_jobs_cb))
> >> diff --git a/drivers/gpu/drm/scheduler/sched_fence.c 
> b/drivers/gpu/drm/scheduler/sched_fence.c
> >> index 7fd869520ef2..1a6bea98c5cc 100644
> >> --- a/drivers/gpu/drm/scheduler/sched_fence.c
> >> +++ b/drivers/gpu/drm/scheduler/sched_fence.c
> >> @@ -53,8 +53,10 @@ void drm_sched_fence_scheduled(struct 
> drm_sched_fence *fence)
> >> dma_fence_signal(&fence->scheduled);
> >>   }
> >>
> >> -void drm_sched_fence_finished(struct drm_sched_fence *fence)
> >> +void drm_sched_fence_finished(struct drm_sched_fence *fence, int 
> result)
> >>   {
> >> +    if (result)
> >> + dma_fence_set_error(&fence->finished, result);
> >> dma_fence_signal(&fence->finished);
> >>   }
> >>
> >> diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
> b/drivers/gpu/drm/scheduler/sched_main.c
> >> index fcd4bfef7415..649fac2e1ccb 100644
> >> --- a/drivers/gpu/drm/scheduler/sched_main.c
> >> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> >> @@ -257,7 +257,7 @@ drm_sched_rq_select_entity_fifo(struct 
> drm_sched_rq *rq)
> >>    *
> >>    * Finish the job's fence and wake up the worker thread.
> >>    */
> >> -static void drm_sched_job_done(struct drm_sched_job *s_job)
> >> +static void drm_sched_job_done(struct drm_sched_job *s_job, int 
> result)
> >>   {
> >>       struct drm_sched_fence *s_fence = s_job->s_fence;
> >>       struct drm_gpu_scheduler *sched = s_fence->sched;
> >> @@ -268,7 +268,7 @@ static void drm_sched_job_done(struct 
> drm_sched_job *s_job)
> >>       trace_drm_sched_process_job(s_fence);
> >>
> >>       dma_fence_get(&s_fence->finished);
> >> -    drm_sched_fence_finished(s_fence);
> >> +    drm_sched_fence_finished(s_fence, result);
> >>       dma_fence_put(&s_fence->finished);
> >> wake_up_interruptible(&sched->wake_up_worker);
> >>   }
> >> @@ -282,7 +282,7 @@ static void drm_sched_job_done_cb(struct 
> dma_fence *f, struct dma_fence_cb *cb)
> >>   {
> >>       struct drm_sched_job *s_job = container_of(cb, struct 
> drm_sched_job, cb);
> >>
> >> -    drm_sched_job_done(s_job);
> >> +    drm_sched_job_done(s_job, f->error);
> >>   }
> >>
> >>   /**
> >> @@ -533,12 +533,12 @@ void drm_sched_start(struct drm_gpu_scheduler 
> *sched, bool full_recovery)
> >>                       r = dma_fence_add_callback(fence, &s_job->cb,
> >>                                                 drm_sched_job_done_cb);
> >>                       if (r == -ENOENT)
> >> - drm_sched_job_done(s_job);
> >> + drm_sched_job_done(s_job, fence->error);
> >>                       else if (r)
> >> DRM_DEV_ERROR(sched->dev, "fence add callback failed (%d)\n",
> >>                                         r);
> >>               } else
> >> -                    drm_sched_job_done(s_job);
> >> +                    drm_sched_job_done(s_job, 0);
> >>       }
> >>
> >>       if (full_recovery) {
> >> @@ -1010,15 +1010,13 @@ static int drm_sched_main(void *param)
> >>                       r = dma_fence_add_callback(fence, &sched_job->cb,
> >>                                                 drm_sched_job_done_cb);
> >>                       if (r == -ENOENT)
> >> - drm_sched_job_done(sched_job);
> >> + drm_sched_job_done(sched_job, fence->error);
> >>                       else if (r)
> >> DRM_DEV_ERROR(sched->dev, "fence add callback failed (%d)\n",
> >>                                         r);
> >>               } else {
> >> -                    if (IS_ERR(fence))
> >> - dma_fence_set_error(&s_fence->finished, PTR_ERR(fence));
> >> -
> >> - drm_sched_job_done(sched_job);
> >> + drm_sched_job_done(sched_job, IS_ERR(fence) ?
> >> + PTR_ERR(fence) : 0);
> >>               }
> >>
> >> wake_up(&sched->job_scheduled);
> >> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> >> index ca857ec9e7eb..5c1df6b12ced 100644
> >> --- a/include/drm/gpu_scheduler.h
> >> +++ b/include/drm/gpu_scheduler.h
> >> @@ -569,7 +569,7 @@ void drm_sched_fence_init(struct 
> drm_sched_fence *fence,
> >>   void drm_sched_fence_free(struct drm_sched_fence *fence);
> >>
> >>   void drm_sched_fence_scheduled(struct drm_sched_fence *fence);
> >> -void drm_sched_fence_finished(struct drm_sched_fence *fence);
> >> +void drm_sched_fence_finished(struct drm_sched_fence *fence, int 
> result);
> >>
> >>   unsigned long drm_sched_suspend_timeout(struct drm_gpu_scheduler 
> *sched);
> >>   void drm_sched_resume_timeout(struct drm_gpu_scheduler *sched,
>

[-- Attachment #2: Type: text/html, Size: 15537 bytes --]

RE: [PATCH 1/8] drm/scheduler: properly forward fence errors

[Yin, ZhenGuo (Chris)]

[AMD Official Use Only - General]

Hi, Christian

diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index fcd4bfef7415..649fac2e1ccb 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -533,12 +533,12 @@ void drm_sched_start(struct drm_gpu_scheduler *sched, bool full_recovery)
 			r = dma_fence_add_callback(fence, &s_job->cb,
 						   drm_sched_job_done_cb);
 			if (r == -ENOENT)
-				drm_sched_job_done(s_job);
+				drm_sched_job_done(s_job, fence->error);
 			else if (r)
 				DRM_DEV_ERROR(sched->dev, "fence add callback failed (%d)\n",
 					  r);
 		} else
-			drm_sched_job_done(s_job);
+			drm_sched_job_done(s_job, 0);
 	}
 
 	if (full_recovery) {

I believe that the finished fence of some skipped jobs during FLR HASN'T been set to -ECANCELED.
In function drm_sched_stop, the callback has been removed from hw_fence and s_fence->parent has been set to NULL, see commit 45ecaea738830b9d521c93520c8f201359dcbd95(drm/sched: Partial revert of 'drm/sched: Keep s_fence->parent pointer').
In functnion drm_sched_start, jobs in the pending list pretend to be done without any errors(drm_sched_job_done(s_job, 0)).


Best,
Zhenguo
Cloud-GPU Core team, SRDC

-----Original Message-----
From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Christian König
Sent: Thursday, April 20, 2023 7:58 PM
To: amd-gfx@lists.freedesktop.org
Cc: Tuikov, Luben <Luben.Tuikov@amd.com>
Subject: [PATCH 1/8] drm/scheduler: properly forward fence errors

When a hw fence is signaled with an error properly forward that to the finished fence.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/scheduler/sched_entity.c |  4 +---  drivers/gpu/drm/scheduler/sched_fence.c  |  4 +++-
 drivers/gpu/drm/scheduler/sched_main.c   | 18 ++++++++----------
 include/drm/gpu_scheduler.h              |  2 +-
 4 files changed, 13 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
index 15d04a0ec623..eaf71fe15ed3 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -144,7 +144,7 @@ static void drm_sched_entity_kill_jobs_work(struct work_struct *wrk)  {
 	struct drm_sched_job *job = container_of(wrk, typeof(*job), work);
 
-	drm_sched_fence_finished(job->s_fence);
+	drm_sched_fence_finished(job->s_fence, -ESRCH);
 	WARN_ON(job->s_fence->parent);
 	job->sched->ops->free_job(job);
 }
@@ -195,8 +195,6 @@ static void drm_sched_entity_kill(struct drm_sched_entity *entity)
 	while ((job = to_drm_sched_job(spsc_queue_pop(&entity->job_queue)))) {
 		struct drm_sched_fence *s_fence = job->s_fence;
 
-		dma_fence_set_error(&s_fence->finished, -ESRCH);
-
 		dma_fence_get(&s_fence->finished);
 		if (!prev || dma_fence_add_callback(prev, &job->finish_cb,
 					   drm_sched_entity_kill_jobs_cb)) diff --git a/drivers/gpu/drm/scheduler/sched_fence.c b/drivers/gpu/drm/scheduler/sched_fence.c
index 7fd869520ef2..1a6bea98c5cc 100644
--- a/drivers/gpu/drm/scheduler/sched_fence.c
+++ b/drivers/gpu/drm/scheduler/sched_fence.c
@@ -53,8 +53,10 @@ void drm_sched_fence_scheduled(struct drm_sched_fence *fence)
 	dma_fence_signal(&fence->scheduled);
 }
 
-void drm_sched_fence_finished(struct drm_sched_fence *fence)
+void drm_sched_fence_finished(struct drm_sched_fence *fence, int 
+result)
 {
+	if (result)
+		dma_fence_set_error(&fence->finished, result);
 	dma_fence_signal(&fence->finished);
 }
 
diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index fcd4bfef7415..649fac2e1ccb 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -257,7 +257,7 @@ drm_sched_rq_select_entity_fifo(struct drm_sched_rq *rq)
  *
  * Finish the job's fence and wake up the worker thread.
  */
-static void drm_sched_job_done(struct drm_sched_job *s_job)
+static void drm_sched_job_done(struct drm_sched_job *s_job, int result)
 {
 	struct drm_sched_fence *s_fence = s_job->s_fence;
 	struct drm_gpu_scheduler *sched = s_fence->sched; @@ -268,7 +268,7 @@ static void drm_sched_job_done(struct drm_sched_job *s_job)
 	trace_drm_sched_process_job(s_fence);
 
 	dma_fence_get(&s_fence->finished);
-	drm_sched_fence_finished(s_fence);
+	drm_sched_fence_finished(s_fence, result);
 	dma_fence_put(&s_fence->finished);
 	wake_up_interruptible(&sched->wake_up_worker);
 }
@@ -282,7 +282,7 @@ static void drm_sched_job_done_cb(struct dma_fence *f, struct dma_fence_cb *cb)  {
 	struct drm_sched_job *s_job = container_of(cb, struct drm_sched_job, cb);
 
-	drm_sched_job_done(s_job);
+	drm_sched_job_done(s_job, f->error);
 }
 
 /**
@@ -533,12 +533,12 @@ void drm_sched_start(struct drm_gpu_scheduler *sched, bool full_recovery)
 			r = dma_fence_add_callback(fence, &s_job->cb,
 						   drm_sched_job_done_cb);
 			if (r == -ENOENT)
-				drm_sched_job_done(s_job);
+				drm_sched_job_done(s_job, fence->error);
 			else if (r)
 				DRM_DEV_ERROR(sched->dev, "fence add callback failed (%d)\n",
 					  r);
 		} else
-			drm_sched_job_done(s_job);
+			drm_sched_job_done(s_job, 0);
 	}
 
 	if (full_recovery) {
@@ -1010,15 +1010,13 @@ static int drm_sched_main(void *param)
 			r = dma_fence_add_callback(fence, &sched_job->cb,
 						   drm_sched_job_done_cb);
 			if (r == -ENOENT)
-				drm_sched_job_done(sched_job);
+				drm_sched_job_done(sched_job, fence->error);
 			else if (r)
 				DRM_DEV_ERROR(sched->dev, "fence add callback failed (%d)\n",
 					  r);
 		} else {
-			if (IS_ERR(fence))
-				dma_fence_set_error(&s_fence->finished, PTR_ERR(fence));
-
-			drm_sched_job_done(sched_job);
+			drm_sched_job_done(sched_job, IS_ERR(fence) ?
+					   PTR_ERR(fence) : 0);
 		}
 
 		wake_up(&sched->job_scheduled);
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h index ca857ec9e7eb..5c1df6b12ced 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -569,7 +569,7 @@ void drm_sched_fence_init(struct drm_sched_fence *fence,  void drm_sched_fence_free(struct drm_sched_fence *fence);
 
 void drm_sched_fence_scheduled(struct drm_sched_fence *fence); -void drm_sched_fence_finished(struct drm_sched_fence *fence);
+void drm_sched_fence_finished(struct drm_sched_fence *fence, int 
+result);
 
 unsigned long drm_sched_suspend_timeout(struct drm_gpu_scheduler *sched);  void drm_sched_resume_timeout(struct drm_gpu_scheduler *sched,
--
2.34.1

Re: [PATCH 1/8] drm/scheduler: properly forward fence errors

[Christian König]

Well good point, but as part of the effort of the Intel team to move the 
scheduler over to a work item based design those two functions are 
probably about to be removed.

Since we will probably have that in the internal package for a bit 
longer I'm going to send a fix for this.

Regards,
Christian.

Am 27.04.23 um 12:35 schrieb Yin, ZhenGuo (Chris):
> [AMD Official Use Only - General]
>
> Hi, Christian
>
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> index fcd4bfef7415..649fac2e1ccb 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -533,12 +533,12 @@ void drm_sched_start(struct drm_gpu_scheduler *sched, bool full_recovery)
>   			r = dma_fence_add_callback(fence, &s_job->cb,
>   						   drm_sched_job_done_cb);
>   			if (r == -ENOENT)
> -				drm_sched_job_done(s_job);
> +				drm_sched_job_done(s_job, fence->error);
>   			else if (r)
>   				DRM_DEV_ERROR(sched->dev, "fence add callback failed (%d)\n",
>   					  r);
>   		} else
> -			drm_sched_job_done(s_job);
> +			drm_sched_job_done(s_job, 0);
>   	}
>   
>   	if (full_recovery) {
>
> I believe that the finished fence of some skipped jobs during FLR HASN'T been set to -ECANCELED.
> In function drm_sched_stop, the callback has been removed from hw_fence and s_fence->parent has been set to NULL, see commit 45ecaea738830b9d521c93520c8f201359dcbd95(drm/sched: Partial revert of 'drm/sched: Keep s_fence->parent pointer').
> In functnion drm_sched_start, jobs in the pending list pretend to be done without any errors(drm_sched_job_done(s_job, 0)).
>
>
> Best,
> Zhenguo
> Cloud-GPU Core team, SRDC
>
> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Christian König
> Sent: Thursday, April 20, 2023 7:58 PM
> To: amd-gfx@lists.freedesktop.org
> Cc: Tuikov, Luben <Luben.Tuikov@amd.com>
> Subject: [PATCH 1/8] drm/scheduler: properly forward fence errors
>
> When a hw fence is signaled with an error properly forward that to the finished fence.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/scheduler/sched_entity.c |  4 +---  drivers/gpu/drm/scheduler/sched_fence.c  |  4 +++-
>   drivers/gpu/drm/scheduler/sched_main.c   | 18 ++++++++----------
>   include/drm/gpu_scheduler.h              |  2 +-
>   4 files changed, 13 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
> index 15d04a0ec623..eaf71fe15ed3 100644
> --- a/drivers/gpu/drm/scheduler/sched_entity.c
> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
> @@ -144,7 +144,7 @@ static void drm_sched_entity_kill_jobs_work(struct work_struct *wrk)  {
>   	struct drm_sched_job *job = container_of(wrk, typeof(*job), work);
>   
> -	drm_sched_fence_finished(job->s_fence);
> +	drm_sched_fence_finished(job->s_fence, -ESRCH);
>   	WARN_ON(job->s_fence->parent);
>   	job->sched->ops->free_job(job);
>   }
> @@ -195,8 +195,6 @@ static void drm_sched_entity_kill(struct drm_sched_entity *entity)
>   	while ((job = to_drm_sched_job(spsc_queue_pop(&entity->job_queue)))) {
>   		struct drm_sched_fence *s_fence = job->s_fence;
>   
> -		dma_fence_set_error(&s_fence->finished, -ESRCH);
> -
>   		dma_fence_get(&s_fence->finished);
>   		if (!prev || dma_fence_add_callback(prev, &job->finish_cb,
>   					   drm_sched_entity_kill_jobs_cb)) diff --git a/drivers/gpu/drm/scheduler/sched_fence.c b/drivers/gpu/drm/scheduler/sched_fence.c
> index 7fd869520ef2..1a6bea98c5cc 100644
> --- a/drivers/gpu/drm/scheduler/sched_fence.c
> +++ b/drivers/gpu/drm/scheduler/sched_fence.c
> @@ -53,8 +53,10 @@ void drm_sched_fence_scheduled(struct drm_sched_fence *fence)
>   	dma_fence_signal(&fence->scheduled);
>   }
>   
> -void drm_sched_fence_finished(struct drm_sched_fence *fence)
> +void drm_sched_fence_finished(struct drm_sched_fence *fence, int
> +result)
>   {
> +	if (result)
> +		dma_fence_set_error(&fence->finished, result);
>   	dma_fence_signal(&fence->finished);
>   }
>   
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> index fcd4bfef7415..649fac2e1ccb 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -257,7 +257,7 @@ drm_sched_rq_select_entity_fifo(struct drm_sched_rq *rq)
>    *
>    * Finish the job's fence and wake up the worker thread.
>    */
> -static void drm_sched_job_done(struct drm_sched_job *s_job)
> +static void drm_sched_job_done(struct drm_sched_job *s_job, int result)
>   {
>   	struct drm_sched_fence *s_fence = s_job->s_fence;
>   	struct drm_gpu_scheduler *sched = s_fence->sched; @@ -268,7 +268,7 @@ static void drm_sched_job_done(struct drm_sched_job *s_job)
>   	trace_drm_sched_process_job(s_fence);
>   
>   	dma_fence_get(&s_fence->finished);
> -	drm_sched_fence_finished(s_fence);
> +	drm_sched_fence_finished(s_fence, result);
>   	dma_fence_put(&s_fence->finished);
>   	wake_up_interruptible(&sched->wake_up_worker);
>   }
> @@ -282,7 +282,7 @@ static void drm_sched_job_done_cb(struct dma_fence *f, struct dma_fence_cb *cb)  {
>   	struct drm_sched_job *s_job = container_of(cb, struct drm_sched_job, cb);
>   
> -	drm_sched_job_done(s_job);
> +	drm_sched_job_done(s_job, f->error);
>   }
>   
>   /**
> @@ -533,12 +533,12 @@ void drm_sched_start(struct drm_gpu_scheduler *sched, bool full_recovery)
>   			r = dma_fence_add_callback(fence, &s_job->cb,
>   						   drm_sched_job_done_cb);
>   			if (r == -ENOENT)
> -				drm_sched_job_done(s_job);
> +				drm_sched_job_done(s_job, fence->error);
>   			else if (r)
>   				DRM_DEV_ERROR(sched->dev, "fence add callback failed (%d)\n",
>   					  r);
>   		} else
> -			drm_sched_job_done(s_job);
> +			drm_sched_job_done(s_job, 0);
>   	}
>   
>   	if (full_recovery) {
> @@ -1010,15 +1010,13 @@ static int drm_sched_main(void *param)
>   			r = dma_fence_add_callback(fence, &sched_job->cb,
>   						   drm_sched_job_done_cb);
>   			if (r == -ENOENT)
> -				drm_sched_job_done(sched_job);
> +				drm_sched_job_done(sched_job, fence->error);
>   			else if (r)
>   				DRM_DEV_ERROR(sched->dev, "fence add callback failed (%d)\n",
>   					  r);
>   		} else {
> -			if (IS_ERR(fence))
> -				dma_fence_set_error(&s_fence->finished, PTR_ERR(fence));
> -
> -			drm_sched_job_done(sched_job);
> +			drm_sched_job_done(sched_job, IS_ERR(fence) ?
> +					   PTR_ERR(fence) : 0);
>   		}
>   
>   		wake_up(&sched->job_scheduled);
> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h index ca857ec9e7eb..5c1df6b12ced 100644
> --- a/include/drm/gpu_scheduler.h
> +++ b/include/drm/gpu_scheduler.h
> @@ -569,7 +569,7 @@ void drm_sched_fence_init(struct drm_sched_fence *fence,  void drm_sched_fence_free(struct drm_sched_fence *fence);
>   
>   void drm_sched_fence_scheduled(struct drm_sched_fence *fence); -void drm_sched_fence_finished(struct drm_sched_fence *fence);
> +void drm_sched_fence_finished(struct drm_sched_fence *fence, int
> +result);
>   
>   unsigned long drm_sched_suspend_timeout(struct drm_gpu_scheduler *sched);  void drm_sched_resume_timeout(struct drm_gpu_scheduler *sched,
> --
> 2.34.1

RE: [PATCH 1/8] drm/scheduler: properly forward fence errors

[Yin, ZhenGuo (Chris)]

[AMD Official Use Only - General]

Hi, @Christian König

Any updates for the fix?
Recently we found that there will be a page fault after FLR, since an SDMA job in the pending list was dropped without forwarding fence errors.


Best,
Zhenguo
Cloud-GPU Core team, SRDC

-----Original Message-----
From: Christian König <ckoenig.leichtzumerken@gmail.com>
Sent: Thursday, April 27, 2023 8:05 PM
To: Yin, ZhenGuo (Chris) <ZhenGuo.Yin@amd.com>; amd-gfx@lists.freedesktop.org
Cc: Tuikov, Luben <Luben.Tuikov@amd.com>; Chen, JingWen (Wayne) <JingWen.Chen2@amd.com>; Liu, Monk <Monk.Liu@amd.com>
Subject: Re: [PATCH 1/8] drm/scheduler: properly forward fence errors

Well good point, but as part of the effort of the Intel team to move the scheduler over to a work item based design those two functions are probably about to be removed.

Since we will probably have that in the internal package for a bit longer I'm going to send a fix for this.

Regards,
Christian.

Am 27.04.23 um 12:35 schrieb Yin, ZhenGuo (Chris):
> [AMD Official Use Only - General]
>
> Hi, Christian
>
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c
> b/drivers/gpu/drm/scheduler/sched_main.c
> index fcd4bfef7415..649fac2e1ccb 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -533,12 +533,12 @@ void drm_sched_start(struct drm_gpu_scheduler *sched, bool full_recovery)
>                       r = dma_fence_add_callback(fence, &s_job->cb,
>                                                  drm_sched_job_done_cb);
>                       if (r == -ENOENT)
> -                             drm_sched_job_done(s_job);
> +                             drm_sched_job_done(s_job, fence->error);
>                       else if (r)
>                               DRM_DEV_ERROR(sched->dev, "fence add callback failed (%d)\n",
>                                         r);
>               } else
> -                     drm_sched_job_done(s_job);
> +                     drm_sched_job_done(s_job, 0);
>       }
>
>       if (full_recovery) {
>
> I believe that the finished fence of some skipped jobs during FLR HASN'T been set to -ECANCELED.
> In function drm_sched_stop, the callback has been removed from hw_fence and s_fence->parent has been set to NULL, see commit 45ecaea738830b9d521c93520c8f201359dcbd95(drm/sched: Partial revert of 'drm/sched: Keep s_fence->parent pointer').
> In functnion drm_sched_start, jobs in the pending list pretend to be done without any errors(drm_sched_job_done(s_job, 0)).
>
>
> Best,
> Zhenguo
> Cloud-GPU Core team, SRDC
>
> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
> Christian König
> Sent: Thursday, April 20, 2023 7:58 PM
> To: amd-gfx@lists.freedesktop.org
> Cc: Tuikov, Luben <Luben.Tuikov@amd.com>
> Subject: [PATCH 1/8] drm/scheduler: properly forward fence errors
>
> When a hw fence is signaled with an error properly forward that to the finished fence.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/scheduler/sched_entity.c |  4 +---  drivers/gpu/drm/scheduler/sched_fence.c  |  4 +++-
>   drivers/gpu/drm/scheduler/sched_main.c   | 18 ++++++++----------
>   include/drm/gpu_scheduler.h              |  2 +-
>   4 files changed, 13 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c
> b/drivers/gpu/drm/scheduler/sched_entity.c
> index 15d04a0ec623..eaf71fe15ed3 100644
> --- a/drivers/gpu/drm/scheduler/sched_entity.c
> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
> @@ -144,7 +144,7 @@ static void drm_sched_entity_kill_jobs_work(struct work_struct *wrk)  {
>       struct drm_sched_job *job = container_of(wrk, typeof(*job), work);
>
> -     drm_sched_fence_finished(job->s_fence);
> +     drm_sched_fence_finished(job->s_fence, -ESRCH);
>       WARN_ON(job->s_fence->parent);
>       job->sched->ops->free_job(job);
>   }
> @@ -195,8 +195,6 @@ static void drm_sched_entity_kill(struct drm_sched_entity *entity)
>       while ((job = to_drm_sched_job(spsc_queue_pop(&entity->job_queue)))) {
>               struct drm_sched_fence *s_fence = job->s_fence;
>
> -             dma_fence_set_error(&s_fence->finished, -ESRCH);
> -
>               dma_fence_get(&s_fence->finished);
>               if (!prev || dma_fence_add_callback(prev, &job->finish_cb,
>                                          drm_sched_entity_kill_jobs_cb)) diff --git
> a/drivers/gpu/drm/scheduler/sched_fence.c
> b/drivers/gpu/drm/scheduler/sched_fence.c
> index 7fd869520ef2..1a6bea98c5cc 100644
> --- a/drivers/gpu/drm/scheduler/sched_fence.c
> +++ b/drivers/gpu/drm/scheduler/sched_fence.c
> @@ -53,8 +53,10 @@ void drm_sched_fence_scheduled(struct drm_sched_fence *fence)
>       dma_fence_signal(&fence->scheduled);
>   }
>
> -void drm_sched_fence_finished(struct drm_sched_fence *fence)
> +void drm_sched_fence_finished(struct drm_sched_fence *fence, int
> +result)
>   {
> +     if (result)
> +             dma_fence_set_error(&fence->finished, result);
>       dma_fence_signal(&fence->finished);
>   }
>
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c
> b/drivers/gpu/drm/scheduler/sched_main.c
> index fcd4bfef7415..649fac2e1ccb 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -257,7 +257,7 @@ drm_sched_rq_select_entity_fifo(struct drm_sched_rq *rq)
>    *
>    * Finish the job's fence and wake up the worker thread.
>    */
> -static void drm_sched_job_done(struct drm_sched_job *s_job)
> +static void drm_sched_job_done(struct drm_sched_job *s_job, int
> +result)
>   {
>       struct drm_sched_fence *s_fence = s_job->s_fence;
>       struct drm_gpu_scheduler *sched = s_fence->sched; @@ -268,7 +268,7 @@ static void drm_sched_job_done(struct drm_sched_job *s_job)
>       trace_drm_sched_process_job(s_fence);
>
>       dma_fence_get(&s_fence->finished);
> -     drm_sched_fence_finished(s_fence);
> +     drm_sched_fence_finished(s_fence, result);
>       dma_fence_put(&s_fence->finished);
>       wake_up_interruptible(&sched->wake_up_worker);
>   }
> @@ -282,7 +282,7 @@ static void drm_sched_job_done_cb(struct dma_fence *f, struct dma_fence_cb *cb)  {
>       struct drm_sched_job *s_job = container_of(cb, struct
> drm_sched_job, cb);
>
> -     drm_sched_job_done(s_job);
> +     drm_sched_job_done(s_job, f->error);
>   }
>
>   /**
> @@ -533,12 +533,12 @@ void drm_sched_start(struct drm_gpu_scheduler *sched, bool full_recovery)
>                       r = dma_fence_add_callback(fence, &s_job->cb,
>                                                  drm_sched_job_done_cb);
>                       if (r == -ENOENT)
> -                             drm_sched_job_done(s_job);
> +                             drm_sched_job_done(s_job, fence->error);
>                       else if (r)
>                               DRM_DEV_ERROR(sched->dev, "fence add callback failed (%d)\n",
>                                         r);
>               } else
> -                     drm_sched_job_done(s_job);
> +                     drm_sched_job_done(s_job, 0);
>       }
>
>       if (full_recovery) {
> @@ -1010,15 +1010,13 @@ static int drm_sched_main(void *param)
>                       r = dma_fence_add_callback(fence, &sched_job->cb,
>                                                  drm_sched_job_done_cb);
>                       if (r == -ENOENT)
> -                             drm_sched_job_done(sched_job);
> +                             drm_sched_job_done(sched_job, fence->error);
>                       else if (r)
>                               DRM_DEV_ERROR(sched->dev, "fence add callback failed (%d)\n",
>                                         r);
>               } else {
> -                     if (IS_ERR(fence))
> -                             dma_fence_set_error(&s_fence->finished, PTR_ERR(fence));
> -
> -                     drm_sched_job_done(sched_job);
> +                     drm_sched_job_done(sched_job, IS_ERR(fence) ?
> +                                        PTR_ERR(fence) : 0);
>               }
>
>               wake_up(&sched->job_scheduled);
> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> index ca857ec9e7eb..5c1df6b12ced 100644
> --- a/include/drm/gpu_scheduler.h
> +++ b/include/drm/gpu_scheduler.h
> @@ -569,7 +569,7 @@ void drm_sched_fence_init(struct drm_sched_fence
> *fence,  void drm_sched_fence_free(struct drm_sched_fence *fence);
>
>   void drm_sched_fence_scheduled(struct drm_sched_fence *fence); -void
> drm_sched_fence_finished(struct drm_sched_fence *fence);
> +void drm_sched_fence_finished(struct drm_sched_fence *fence, int
> +result);
>
>   unsigned long drm_sched_suspend_timeout(struct drm_gpu_scheduler
> *sched);  void drm_sched_resume_timeout(struct drm_gpu_scheduler
> *sched,
> --
> 2.34.1

RE: [PATCH 1/8] drm/scheduler: properly forward fence errors

[Yin, ZhenGuo (Chris)]

[AMD Official Use Only - General]

Ping..

Actually, I prepare a patch aiming to fix this issue.
But I'm not sure whether this is proper for drm/scheduler.

diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index 9654e8942382..35dc0b86a18e 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -463,6 +463,7 @@ void drm_sched_stop(struct drm_gpu_scheduler *sched, struct drm_sched_job *bad)
                                              &s_job->cb)) {
                        dma_fence_put(s_job->s_fence->parent);
                        s_job->s_fence->parent = NULL;
+                       dma_fence_set_error(&s_job->s_fence->finished, -EHWPOISON);
                        atomic_dec(&sched->hw_rq_count);
                } else {
                        /*
Best,
Zhenguo
Cloud-GPU Core team, SRDC

-----Original Message-----
From: Yin, ZhenGuo (Chris)
Sent: Thursday, August 17, 2023 4:17 PM
To: Christian König <ckoenig.leichtzumerken@gmail.com>; amd-gfx@lists.freedesktop.org
Cc: Tuikov, Luben <Luben.Tuikov@amd.com>; Chen, JingWen (Wayne) <JingWen.Chen2@amd.com>; Liu, Monk <Monk.Liu@amd.com>; Li, Chong(Alan) <chong.li@amd.com>; cao, lin <lin.cao@amd.com>
Subject: RE: [PATCH 1/8] drm/scheduler: properly forward fence errors

Hi, @Christian König

Any updates for the fix?
Recently we found that there will be a page fault after FLR, since an SDMA job in the pending list was dropped without forwarding fence errors.


Best,
Zhenguo
Cloud-GPU Core team, SRDC

-----Original Message-----
From: Christian König <ckoenig.leichtzumerken@gmail.com>
Sent: Thursday, April 27, 2023 8:05 PM
To: Yin, ZhenGuo (Chris) <ZhenGuo.Yin@amd.com>; amd-gfx@lists.freedesktop.org
Cc: Tuikov, Luben <Luben.Tuikov@amd.com>; Chen, JingWen (Wayne) <JingWen.Chen2@amd.com>; Liu, Monk <Monk.Liu@amd.com>
Subject: Re: [PATCH 1/8] drm/scheduler: properly forward fence errors

Well good point, but as part of the effort of the Intel team to move the scheduler over to a work item based design those two functions are probably about to be removed.

Since we will probably have that in the internal package for a bit longer I'm going to send a fix for this.

Regards,
Christian.

Am 27.04.23 um 12:35 schrieb Yin, ZhenGuo (Chris):
> [AMD Official Use Only - General]
>
> Hi, Christian
>
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c
> b/drivers/gpu/drm/scheduler/sched_main.c
> index fcd4bfef7415..649fac2e1ccb 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -533,12 +533,12 @@ void drm_sched_start(struct drm_gpu_scheduler *sched, bool full_recovery)
>                       r = dma_fence_add_callback(fence, &s_job->cb,
>                                                  drm_sched_job_done_cb);
>                       if (r == -ENOENT)
> -                             drm_sched_job_done(s_job);
> +                             drm_sched_job_done(s_job, fence->error);
>                       else if (r)
>                               DRM_DEV_ERROR(sched->dev, "fence add callback failed (%d)\n",
>                                         r);
>               } else
> -                     drm_sched_job_done(s_job);
> +                     drm_sched_job_done(s_job, 0);
>       }
>
>       if (full_recovery) {
>
> I believe that the finished fence of some skipped jobs during FLR HASN'T been set to -ECANCELED.
> In function drm_sched_stop, the callback has been removed from hw_fence and s_fence->parent has been set to NULL, see commit 45ecaea738830b9d521c93520c8f201359dcbd95(drm/sched: Partial revert of 'drm/sched: Keep s_fence->parent pointer').
> In functnion drm_sched_start, jobs in the pending list pretend to be done without any errors(drm_sched_job_done(s_job, 0)).
>
>
> Best,
> Zhenguo
> Cloud-GPU Core team, SRDC
>
> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
> Christian König
> Sent: Thursday, April 20, 2023 7:58 PM
> To: amd-gfx@lists.freedesktop.org
> Cc: Tuikov, Luben <Luben.Tuikov@amd.com>
> Subject: [PATCH 1/8] drm/scheduler: properly forward fence errors
>
> When a hw fence is signaled with an error properly forward that to the finished fence.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/scheduler/sched_entity.c |  4 +---  drivers/gpu/drm/scheduler/sched_fence.c  |  4 +++-
>   drivers/gpu/drm/scheduler/sched_main.c   | 18 ++++++++----------
>   include/drm/gpu_scheduler.h              |  2 +-
>   4 files changed, 13 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c
> b/drivers/gpu/drm/scheduler/sched_entity.c
> index 15d04a0ec623..eaf71fe15ed3 100644
> --- a/drivers/gpu/drm/scheduler/sched_entity.c
> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
> @@ -144,7 +144,7 @@ static void drm_sched_entity_kill_jobs_work(struct work_struct *wrk)  {
>       struct drm_sched_job *job = container_of(wrk, typeof(*job), work);
>
> -     drm_sched_fence_finished(job->s_fence);
> +     drm_sched_fence_finished(job->s_fence, -ESRCH);
>       WARN_ON(job->s_fence->parent);
>       job->sched->ops->free_job(job);
>   }
> @@ -195,8 +195,6 @@ static void drm_sched_entity_kill(struct drm_sched_entity *entity)
>       while ((job = to_drm_sched_job(spsc_queue_pop(&entity->job_queue)))) {
>               struct drm_sched_fence *s_fence = job->s_fence;
>
> -             dma_fence_set_error(&s_fence->finished, -ESRCH);
> -
>               dma_fence_get(&s_fence->finished);
>               if (!prev || dma_fence_add_callback(prev, &job->finish_cb,
>                                          drm_sched_entity_kill_jobs_cb)) diff --git
> a/drivers/gpu/drm/scheduler/sched_fence.c
> b/drivers/gpu/drm/scheduler/sched_fence.c
> index 7fd869520ef2..1a6bea98c5cc 100644
> --- a/drivers/gpu/drm/scheduler/sched_fence.c
> +++ b/drivers/gpu/drm/scheduler/sched_fence.c
> @@ -53,8 +53,10 @@ void drm_sched_fence_scheduled(struct drm_sched_fence *fence)
>       dma_fence_signal(&fence->scheduled);
>   }
>
> -void drm_sched_fence_finished(struct drm_sched_fence *fence)
> +void drm_sched_fence_finished(struct drm_sched_fence *fence, int
> +result)
>   {
> +     if (result)
> +             dma_fence_set_error(&fence->finished, result);
>       dma_fence_signal(&fence->finished);
>   }
>
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c
> b/drivers/gpu/drm/scheduler/sched_main.c
> index fcd4bfef7415..649fac2e1ccb 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -257,7 +257,7 @@ drm_sched_rq_select_entity_fifo(struct drm_sched_rq *rq)
>    *
>    * Finish the job's fence and wake up the worker thread.
>    */
> -static void drm_sched_job_done(struct drm_sched_job *s_job)
> +static void drm_sched_job_done(struct drm_sched_job *s_job, int
> +result)
>   {
>       struct drm_sched_fence *s_fence = s_job->s_fence;
>       struct drm_gpu_scheduler *sched = s_fence->sched; @@ -268,7 +268,7 @@ static void drm_sched_job_done(struct drm_sched_job *s_job)
>       trace_drm_sched_process_job(s_fence);
>
>       dma_fence_get(&s_fence->finished);
> -     drm_sched_fence_finished(s_fence);
> +     drm_sched_fence_finished(s_fence, result);
>       dma_fence_put(&s_fence->finished);
>       wake_up_interruptible(&sched->wake_up_worker);
>   }
> @@ -282,7 +282,7 @@ static void drm_sched_job_done_cb(struct dma_fence *f, struct dma_fence_cb *cb)  {
>       struct drm_sched_job *s_job = container_of(cb, struct
> drm_sched_job, cb);
>
> -     drm_sched_job_done(s_job);
> +     drm_sched_job_done(s_job, f->error);
>   }
>
>   /**
> @@ -533,12 +533,12 @@ void drm_sched_start(struct drm_gpu_scheduler *sched, bool full_recovery)
>                       r = dma_fence_add_callback(fence, &s_job->cb,
>                                                  drm_sched_job_done_cb);
>                       if (r == -ENOENT)
> -                             drm_sched_job_done(s_job);
> +                             drm_sched_job_done(s_job, fence->error);
>                       else if (r)
>                               DRM_DEV_ERROR(sched->dev, "fence add callback failed (%d)\n",
>                                         r);
>               } else
> -                     drm_sched_job_done(s_job);
> +                     drm_sched_job_done(s_job, 0);
>       }
>
>       if (full_recovery) {
> @@ -1010,15 +1010,13 @@ static int drm_sched_main(void *param)
>                       r = dma_fence_add_callback(fence, &sched_job->cb,
>                                                  drm_sched_job_done_cb);
>                       if (r == -ENOENT)
> -                             drm_sched_job_done(sched_job);
> +                             drm_sched_job_done(sched_job, fence->error);
>                       else if (r)
>                               DRM_DEV_ERROR(sched->dev, "fence add callback failed (%d)\n",
>                                         r);
>               } else {
> -                     if (IS_ERR(fence))
> -                             dma_fence_set_error(&s_fence->finished, PTR_ERR(fence));
> -
> -                     drm_sched_job_done(sched_job);
> +                     drm_sched_job_done(sched_job, IS_ERR(fence) ?
> +                                        PTR_ERR(fence) : 0);
>               }
>
>               wake_up(&sched->job_scheduled);
> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> index ca857ec9e7eb..5c1df6b12ced 100644
> --- a/include/drm/gpu_scheduler.h
> +++ b/include/drm/gpu_scheduler.h
> @@ -569,7 +569,7 @@ void drm_sched_fence_init(struct drm_sched_fence
> *fence,  void drm_sched_fence_free(struct drm_sched_fence *fence);
>
>   void drm_sched_fence_scheduled(struct drm_sched_fence *fence); -void
> drm_sched_fence_finished(struct drm_sched_fence *fence);
> +void drm_sched_fence_finished(struct drm_sched_fence *fence, int
> +result);
>
>   unsigned long drm_sched_suspend_timeout(struct drm_gpu_scheduler
> *sched);  void drm_sched_resume_timeout(struct drm_gpu_scheduler
> *sched,
> --
> 2.34.1

Re: [PATCH 1/8] drm/scheduler: properly forward fence errors

[Christian König]

This was fixed here:

commit 03877d621db082610c9b7602c6e8cd6ebcb75a8f
Author: Christian König <christian.koenig@amd.com>
Date:   Thu Apr 27 14:05:43 2023 +0200

     drm/scheduler: mark jobs without fence as canceled

     When no hw fence is provided for a job that means that the job 
didn't executed.

     Signed-off-by: Christian König <christian.koenig@amd.com>
     Reviewed-by: Luben Tuikov <luben.tuikov@amd.com>
     Link: 
https://patchwork.freedesktop.org/patch/msgid/20230427122726.1290170-1-christian.koenig@amd.com

Could be that the patch hasn't been merged into the internal branches yet.

Regards,
Christian.

Am 23.08.23 um 10:12 schrieb Yin, ZhenGuo (Chris):
> [AMD Official Use Only - General]
>
> Ping..
>
> Actually, I prepare a patch aiming to fix this issue.
> But I'm not sure whether this is proper for drm/scheduler.
>
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> index 9654e8942382..35dc0b86a18e 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -463,6 +463,7 @@ void drm_sched_stop(struct drm_gpu_scheduler *sched, struct drm_sched_job *bad)
>                                                &s_job->cb)) {
>                          dma_fence_put(s_job->s_fence->parent);
>                          s_job->s_fence->parent = NULL;
> +                       dma_fence_set_error(&s_job->s_fence->finished, -EHWPOISON);
>                          atomic_dec(&sched->hw_rq_count);
>                  } else {
>                          /*
> Best,
> Zhenguo
> Cloud-GPU Core team, SRDC
>
> -----Original Message-----
> From: Yin, ZhenGuo (Chris)
> Sent: Thursday, August 17, 2023 4:17 PM
> To: Christian König <ckoenig.leichtzumerken@gmail.com>; amd-gfx@lists.freedesktop.org
> Cc: Tuikov, Luben <Luben.Tuikov@amd.com>; Chen, JingWen (Wayne) <JingWen.Chen2@amd.com>; Liu, Monk <Monk.Liu@amd.com>; Li, Chong(Alan) <chong.li@amd.com>; cao, lin <lin.cao@amd.com>
> Subject: RE: [PATCH 1/8] drm/scheduler: properly forward fence errors
>
> Hi, @Christian König
>
> Any updates for the fix?
> Recently we found that there will be a page fault after FLR, since an SDMA job in the pending list was dropped without forwarding fence errors.
>
>
> Best,
> Zhenguo
> Cloud-GPU Core team, SRDC
>
> -----Original Message-----
> From: Christian König <ckoenig.leichtzumerken@gmail.com>
> Sent: Thursday, April 27, 2023 8:05 PM
> To: Yin, ZhenGuo (Chris) <ZhenGuo.Yin@amd.com>; amd-gfx@lists.freedesktop.org
> Cc: Tuikov, Luben <Luben.Tuikov@amd.com>; Chen, JingWen (Wayne) <JingWen.Chen2@amd.com>; Liu, Monk <Monk.Liu@amd.com>
> Subject: Re: [PATCH 1/8] drm/scheduler: properly forward fence errors
>
> Well good point, but as part of the effort of the Intel team to move the scheduler over to a work item based design those two functions are probably about to be removed.
>
> Since we will probably have that in the internal package for a bit longer I'm going to send a fix for this.
>
> Regards,
> Christian.
>
> Am 27.04.23 um 12:35 schrieb Yin, ZhenGuo (Chris):
>> [AMD Official Use Only - General]
>>
>> Hi, Christian
>>
>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c
>> b/drivers/gpu/drm/scheduler/sched_main.c
>> index fcd4bfef7415..649fac2e1ccb 100644
>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>> @@ -533,12 +533,12 @@ void drm_sched_start(struct drm_gpu_scheduler *sched, bool full_recovery)
>>                        r = dma_fence_add_callback(fence, &s_job->cb,
>>                                                   drm_sched_job_done_cb);
>>                        if (r == -ENOENT)
>> -                             drm_sched_job_done(s_job);
>> +                             drm_sched_job_done(s_job, fence->error);
>>                        else if (r)
>>                                DRM_DEV_ERROR(sched->dev, "fence add callback failed (%d)\n",
>>                                          r);
>>                } else
>> -                     drm_sched_job_done(s_job);
>> +                     drm_sched_job_done(s_job, 0);
>>        }
>>
>>        if (full_recovery) {
>>
>> I believe that the finished fence of some skipped jobs during FLR HASN'T been set to -ECANCELED.
>> In function drm_sched_stop, the callback has been removed from hw_fence and s_fence->parent has been set to NULL, see commit 45ecaea738830b9d521c93520c8f201359dcbd95(drm/sched: Partial revert of 'drm/sched: Keep s_fence->parent pointer').
>> In functnion drm_sched_start, jobs in the pending list pretend to be done without any errors(drm_sched_job_done(s_job, 0)).
>>
>>
>> Best,
>> Zhenguo
>> Cloud-GPU Core team, SRDC
>>
>> -----Original Message-----
>> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
>> Christian König
>> Sent: Thursday, April 20, 2023 7:58 PM
>> To: amd-gfx@lists.freedesktop.org
>> Cc: Tuikov, Luben <Luben.Tuikov@amd.com>
>> Subject: [PATCH 1/8] drm/scheduler: properly forward fence errors
>>
>> When a hw fence is signaled with an error properly forward that to the finished fence.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>    drivers/gpu/drm/scheduler/sched_entity.c |  4 +---  drivers/gpu/drm/scheduler/sched_fence.c  |  4 +++-
>>    drivers/gpu/drm/scheduler/sched_main.c   | 18 ++++++++----------
>>    include/drm/gpu_scheduler.h              |  2 +-
>>    4 files changed, 13 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c
>> b/drivers/gpu/drm/scheduler/sched_entity.c
>> index 15d04a0ec623..eaf71fe15ed3 100644
>> --- a/drivers/gpu/drm/scheduler/sched_entity.c
>> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
>> @@ -144,7 +144,7 @@ static void drm_sched_entity_kill_jobs_work(struct work_struct *wrk)  {
>>        struct drm_sched_job *job = container_of(wrk, typeof(*job), work);
>>
>> -     drm_sched_fence_finished(job->s_fence);
>> +     drm_sched_fence_finished(job->s_fence, -ESRCH);
>>        WARN_ON(job->s_fence->parent);
>>        job->sched->ops->free_job(job);
>>    }
>> @@ -195,8 +195,6 @@ static void drm_sched_entity_kill(struct drm_sched_entity *entity)
>>        while ((job = to_drm_sched_job(spsc_queue_pop(&entity->job_queue)))) {
>>                struct drm_sched_fence *s_fence = job->s_fence;
>>
>> -             dma_fence_set_error(&s_fence->finished, -ESRCH);
>> -
>>                dma_fence_get(&s_fence->finished);
>>                if (!prev || dma_fence_add_callback(prev, &job->finish_cb,
>>                                           drm_sched_entity_kill_jobs_cb)) diff --git
>> a/drivers/gpu/drm/scheduler/sched_fence.c
>> b/drivers/gpu/drm/scheduler/sched_fence.c
>> index 7fd869520ef2..1a6bea98c5cc 100644
>> --- a/drivers/gpu/drm/scheduler/sched_fence.c
>> +++ b/drivers/gpu/drm/scheduler/sched_fence.c
>> @@ -53,8 +53,10 @@ void drm_sched_fence_scheduled(struct drm_sched_fence *fence)
>>        dma_fence_signal(&fence->scheduled);
>>    }
>>
>> -void drm_sched_fence_finished(struct drm_sched_fence *fence)
>> +void drm_sched_fence_finished(struct drm_sched_fence *fence, int
>> +result)
>>    {
>> +     if (result)
>> +             dma_fence_set_error(&fence->finished, result);
>>        dma_fence_signal(&fence->finished);
>>    }
>>
>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c
>> b/drivers/gpu/drm/scheduler/sched_main.c
>> index fcd4bfef7415..649fac2e1ccb 100644
>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>> @@ -257,7 +257,7 @@ drm_sched_rq_select_entity_fifo(struct drm_sched_rq *rq)
>>     *
>>     * Finish the job's fence and wake up the worker thread.
>>     */
>> -static void drm_sched_job_done(struct drm_sched_job *s_job)
>> +static void drm_sched_job_done(struct drm_sched_job *s_job, int
>> +result)
>>    {
>>        struct drm_sched_fence *s_fence = s_job->s_fence;
>>        struct drm_gpu_scheduler *sched = s_fence->sched; @@ -268,7 +268,7 @@ static void drm_sched_job_done(struct drm_sched_job *s_job)
>>        trace_drm_sched_process_job(s_fence);
>>
>>        dma_fence_get(&s_fence->finished);
>> -     drm_sched_fence_finished(s_fence);
>> +     drm_sched_fence_finished(s_fence, result);
>>        dma_fence_put(&s_fence->finished);
>>        wake_up_interruptible(&sched->wake_up_worker);
>>    }
>> @@ -282,7 +282,7 @@ static void drm_sched_job_done_cb(struct dma_fence *f, struct dma_fence_cb *cb)  {
>>        struct drm_sched_job *s_job = container_of(cb, struct
>> drm_sched_job, cb);
>>
>> -     drm_sched_job_done(s_job);
>> +     drm_sched_job_done(s_job, f->error);
>>    }
>>
>>    /**
>> @@ -533,12 +533,12 @@ void drm_sched_start(struct drm_gpu_scheduler *sched, bool full_recovery)
>>                        r = dma_fence_add_callback(fence, &s_job->cb,
>>                                                   drm_sched_job_done_cb);
>>                        if (r == -ENOENT)
>> -                             drm_sched_job_done(s_job);
>> +                             drm_sched_job_done(s_job, fence->error);
>>                        else if (r)
>>                                DRM_DEV_ERROR(sched->dev, "fence add callback failed (%d)\n",
>>                                          r);
>>                } else
>> -                     drm_sched_job_done(s_job);
>> +                     drm_sched_job_done(s_job, 0);
>>        }
>>
>>        if (full_recovery) {
>> @@ -1010,15 +1010,13 @@ static int drm_sched_main(void *param)
>>                        r = dma_fence_add_callback(fence, &sched_job->cb,
>>                                                   drm_sched_job_done_cb);
>>                        if (r == -ENOENT)
>> -                             drm_sched_job_done(sched_job);
>> +                             drm_sched_job_done(sched_job, fence->error);
>>                        else if (r)
>>                                DRM_DEV_ERROR(sched->dev, "fence add callback failed (%d)\n",
>>                                          r);
>>                } else {
>> -                     if (IS_ERR(fence))
>> -                             dma_fence_set_error(&s_fence->finished, PTR_ERR(fence));
>> -
>> -                     drm_sched_job_done(sched_job);
>> +                     drm_sched_job_done(sched_job, IS_ERR(fence) ?
>> +                                        PTR_ERR(fence) : 0);
>>                }
>>
>>                wake_up(&sched->job_scheduled);
>> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
>> index ca857ec9e7eb..5c1df6b12ced 100644
>> --- a/include/drm/gpu_scheduler.h
>> +++ b/include/drm/gpu_scheduler.h
>> @@ -569,7 +569,7 @@ void drm_sched_fence_init(struct drm_sched_fence
>> *fence,  void drm_sched_fence_free(struct drm_sched_fence *fence);
>>
>>    void drm_sched_fence_scheduled(struct drm_sched_fence *fence); -void
>> drm_sched_fence_finished(struct drm_sched_fence *fence);
>> +void drm_sched_fence_finished(struct drm_sched_fence *fence, int
>> +result);
>>
>>    unsigned long drm_sched_suspend_timeout(struct drm_gpu_scheduler
>> *sched);  void drm_sched_resume_timeout(struct drm_gpu_scheduler
>> *sched,
>> --
>> 2.34.1