[PATCH 1/2] drm/amd/amdgpu embed hw_fence into amdgpu_job

[PATCH 1/2] drm/amd/amdgpu embed hw_fence into amdgpu_job

[Jingwen Chen]

From: Jack Zhang <Jack.Zhang1@amd.com>

Why: Previously hw fence is alloced separately with job.
It caused historical lifetime issues and corner cases.
The ideal situation is to take fence to manage both job
and fence's lifetime, and simplify the design of gpu-scheduler.

How:
We propose to embed hw_fence into amdgpu_job.
1. We cover the normal job submission by this method.
2. For ib_test, and submit without a parent job keep the
legacy way to create a hw fence separately.

Signed-off-by: Jingwen Chen <Jingwen.Chen2@amd.com>
Signed-off-by: Jack Zhang <Jack.Zhang1@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c  |  1 -
 drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c |  2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c   | 62 ++++++++++++++++-----
 drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c      |  2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_job.c     | 35 ++++++++----
 drivers/gpu/drm/amd/amdgpu/amdgpu_job.h     |  4 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h    |  2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c      |  2 +-
 8 files changed, 80 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
index b6d33f13b476..bad403978bac 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
@@ -714,7 +714,6 @@ int amdgpu_amdkfd_submit_ib(struct kgd_dev *kgd, enum kgd_engine_type engine,
 	ret = dma_fence_wait(f, false);
 
 err_ib_sched:
-	dma_fence_put(f);
 	amdgpu_job_free(job);
 err:
 	return ret;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
index 536005bff24a..277128846dd1 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
@@ -1414,7 +1414,7 @@ static void amdgpu_ib_preempt_mark_partial_job(struct amdgpu_ring *ring)
 			continue;
 		}
 		job = to_amdgpu_job(s_job);
-		if (preempted && job->fence == fence)
+		if (preempted && (&job->hw_fence) == fence)
 			/* mark the job as preempted */
 			job->preemption_status |= AMDGPU_IB_PREEMPTED;
 	}
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
index 30772608eac6..eecf21d8ec33 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
@@ -133,25 +133,40 @@ static u32 amdgpu_fence_read(struct amdgpu_ring *ring)
  * Emits a fence command on the requested ring (all asics).
  * Returns 0 on success, -ENOMEM on failure.
  */
-int amdgpu_fence_emit(struct amdgpu_ring *ring, struct dma_fence **f,
+int amdgpu_fence_emit(struct amdgpu_ring *ring, struct dma_fence **f, struct amdgpu_job *job,
 		      unsigned flags)
 {
 	struct amdgpu_device *adev = ring->adev;
-	struct amdgpu_fence *fence;
+	struct dma_fence *fence;
+	struct amdgpu_fence *am_fence;
 	struct dma_fence __rcu **ptr;
 	uint32_t seq;
 	int r;
 
-	fence = kmem_cache_alloc(amdgpu_fence_slab, GFP_KERNEL);
-	if (fence == NULL)
-		return -ENOMEM;
+	if (job == NULL) {
+		/* create a sperate hw fence */
+		am_fence = kmem_cache_alloc(amdgpu_fence_slab, GFP_KERNEL);
+		if (am_fence == NULL)
+			return -ENOMEM;
+		fence = &am_fence->base;
+		am_fence->ring = ring;
+	} else {
+		/* take use of job-embedded fence */
+		fence = &job->hw_fence;
+		job->ring = ring;
+	}
 
 	seq = ++ring->fence_drv.sync_seq;
-	fence->ring = ring;
-	dma_fence_init(&fence->base, &amdgpu_fence_ops,
+	dma_fence_init(fence, &amdgpu_fence_ops,
 		       &ring->fence_drv.lock,
 		       adev->fence_context + ring->idx,
 		       seq);
+
+	if (job != NULL) {
+		/* mark this fence has a parent job */
+		set_bit(DMA_FENCE_FLAG_USER_BITS, &fence->flags);
+	}
+
 	amdgpu_ring_emit_fence(ring, ring->fence_drv.gpu_addr,
 			       seq, flags | AMDGPU_FENCE_FLAG_INT);
 	pm_runtime_get_noresume(adev_to_drm(adev)->dev);
@@ -174,9 +189,9 @@ int amdgpu_fence_emit(struct amdgpu_ring *ring, struct dma_fence **f,
 	/* This function can't be called concurrently anyway, otherwise
 	 * emitting the fence would mess up the hardware ring buffer.
 	 */
-	rcu_assign_pointer(*ptr, dma_fence_get(&fence->base));
+	rcu_assign_pointer(*ptr, dma_fence_get(fence));
 
-	*f = &fence->base;
+	*f = fence;
 
 	return 0;
 }
@@ -636,8 +651,16 @@ static const char *amdgpu_fence_get_driver_name(struct dma_fence *fence)
 
 static const char *amdgpu_fence_get_timeline_name(struct dma_fence *f)
 {
-	struct amdgpu_fence *fence = to_amdgpu_fence(f);
-	return (const char *)fence->ring->name;
+	struct amdgpu_ring *ring;
+
+	if (test_bit(DMA_FENCE_FLAG_USER_BITS, &f->flags)) {
+		struct amdgpu_job *job = container_of(f, struct amdgpu_job, hw_fence);
+
+		ring = job->ring;
+	} else {
+		ring = to_amdgpu_fence(f)->ring;
+	}
+	return (const char *)ring->name;
 }
 
 /**
@@ -671,8 +694,20 @@ static bool amdgpu_fence_enable_signaling(struct dma_fence *f)
 static void amdgpu_fence_free(struct rcu_head *rcu)
 {
 	struct dma_fence *f = container_of(rcu, struct dma_fence, rcu);
-	struct amdgpu_fence *fence = to_amdgpu_fence(f);
-	kmem_cache_free(amdgpu_fence_slab, fence);
+
+	if (test_bit(DMA_FENCE_FLAG_USER_BITS, &f->flags)) {
+	/* free job if fence has a parent job */
+		struct amdgpu_job *job;
+
+		job = container_of(f, struct amdgpu_job, hw_fence);
+		kfree(job);
+	} else {
+	/* free fence_slab if it's separated fence*/
+		struct amdgpu_fence *fence;
+
+		fence = to_amdgpu_fence(f);
+		kmem_cache_free(amdgpu_fence_slab, fence);
+	}
 }
 
 /**
@@ -695,6 +730,7 @@ static const struct dma_fence_ops amdgpu_fence_ops = {
 	.release = amdgpu_fence_release,
 };
 
+
 /*
  * Fence debugfs
  */
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
index ec65ab0ddf89..c076a6b9a5a2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
@@ -262,7 +262,7 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, unsigned num_ibs,
 				       fence_flags | AMDGPU_FENCE_FLAG_64BIT);
 	}
 
-	r = amdgpu_fence_emit(ring, f, fence_flags);
+	r = amdgpu_fence_emit(ring, f, job, fence_flags);
 	if (r) {
 		dev_err(adev->dev, "failed to emit fence (%d)\n", r);
 		if (job && job->vmid)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
index 759b34799221..7c426e225b24 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
@@ -114,11 +114,16 @@ void amdgpu_job_free_resources(struct amdgpu_job *job)
 {
 	struct amdgpu_ring *ring = to_amdgpu_ring(job->base.sched);
 	struct dma_fence *f;
+	struct dma_fence *hw_fence;
 	unsigned i;
 
-	/* use sched fence if available */
-	f = job->base.s_fence ? &job->base.s_fence->finished : job->fence;
+	if (job->hw_fence.ops == NULL)
+		hw_fence = job->external_hw_fence;
+	else
+		hw_fence = &job->hw_fence;
 
+	/* use sched fence if available */
+	f = job->base.s_fence ? &job->base.s_fence->finished : hw_fence;
 	for (i = 0; i < job->num_ibs; ++i)
 		amdgpu_ib_free(ring->adev, &job->ibs[i], f);
 }
@@ -129,20 +134,27 @@ static void amdgpu_job_free_cb(struct drm_sched_job *s_job)
 
 	drm_sched_job_cleanup(s_job);
 
-	dma_fence_put(job->fence);
 	amdgpu_sync_free(&job->sync);
 	amdgpu_sync_free(&job->sched_sync);
-	kfree(job);
+
+    /* only put the hw fence if has embedded fence */
+	if (job->hw_fence.ops != NULL)
+		dma_fence_put(&job->hw_fence);
+	else
+		kfree(job);
 }
 
 void amdgpu_job_free(struct amdgpu_job *job)
 {
 	amdgpu_job_free_resources(job);
-
-	dma_fence_put(job->fence);
 	amdgpu_sync_free(&job->sync);
 	amdgpu_sync_free(&job->sched_sync);
-	kfree(job);
+
+	/* only put the hw fence if has embedded fence */
+	if (job->hw_fence.ops != NULL)
+		dma_fence_put(&job->hw_fence);
+	else
+		kfree(job);
 }
 
 int amdgpu_job_submit(struct amdgpu_job *job, struct drm_sched_entity *entity,
@@ -171,11 +183,14 @@ int amdgpu_job_submit_direct(struct amdgpu_job *job, struct amdgpu_ring *ring,
 
 	job->base.sched = &ring->sched;
 	r = amdgpu_ib_schedule(ring, job->num_ibs, job->ibs, NULL, fence);
-	job->fence = dma_fence_get(*fence);
+	/* record external_hw_fence for direct submit */
+	job->external_hw_fence = dma_fence_get(*fence);
 	if (r)
 		return r;
 
 	amdgpu_job_free(job);
+	dma_fence_put(*fence);
+
 	return 0;
 }
 
@@ -233,10 +248,8 @@ static struct dma_fence *amdgpu_job_run(struct drm_sched_job *sched_job)
 		if (r)
 			DRM_ERROR("Error scheduling IBs (%d)\n", r);
 	}
-	/* if gpu reset, hw fence will be replaced here */
-	dma_fence_put(job->fence);
-	job->fence = dma_fence_get(fence);
 
+	dma_fence_get(fence);
 	amdgpu_job_free_resources(job);
 
 	fence = r ? ERR_PTR(r) : fence;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
index 81caac9b958a..92324c978534 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
@@ -46,7 +46,9 @@ struct amdgpu_job {
 	struct amdgpu_sync	sync;
 	struct amdgpu_sync	sched_sync;
 	struct amdgpu_ib	*ibs;
-	struct dma_fence	*fence; /* the hw fence */
+	struct dma_fence	hw_fence;
+	struct amdgpu_ring *ring;
+	struct dma_fence	*external_hw_fence;
 	uint32_t		preamble_status;
 	uint32_t                preemption_status;
 	uint32_t		num_ibs;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
index ca1622835296..2306424cbcb4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
@@ -118,7 +118,7 @@ int amdgpu_fence_driver_start_ring(struct amdgpu_ring *ring,
 				   unsigned irq_type);
 void amdgpu_fence_driver_suspend(struct amdgpu_device *adev);
 void amdgpu_fence_driver_resume(struct amdgpu_device *adev);
-int amdgpu_fence_emit(struct amdgpu_ring *ring, struct dma_fence **fence,
+int amdgpu_fence_emit(struct amdgpu_ring *ring, struct dma_fence **fence, struct amdgpu_job *job,
 		      unsigned flags);
 int amdgpu_fence_emit_polling(struct amdgpu_ring *ring, uint32_t *s,
 			      uint32_t timeout);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 358316d6a38c..0670f43ef22a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1216,7 +1216,7 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
 		amdgpu_gmc_emit_pasid_mapping(ring, job->vmid, job->pasid);
 
 	if (vm_flush_needed || pasid_mapping_needed) {
-		r = amdgpu_fence_emit(ring, &fence, 0);
+		r = amdgpu_fence_emit(ring, &fence, NULL, 0);
 		if (r)
 			return r;
 	}
-- 
2.25.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 2/2] drm: add tdr support for embeded hw_fence

[Jingwen Chen]

[Why]
After embeded hw_fence to amdgpu_job, we need to add tdr support
for this feature.

[How]
1. Add a resubmit_flag for resubmit jobs.
2. Clear job fence from RCU and force complete vm flush fences in
   pre_asic_reset
3. skip dma_fence_get for resubmit jobs and add a dma_fence_put
   for guilty jobs.

Signed-off-by: Jack Zhang <Jack.Zhang1@amd.com>
Signed-off-by: Jingwen Chen <Jingwen.Chen2@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 12 +++++++++++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c  | 16 +++++++++++-----
 drivers/gpu/drm/amd/amdgpu/amdgpu_job.c    |  4 +++-
 drivers/gpu/drm/scheduler/sched_main.c     |  1 +
 include/drm/gpu_scheduler.h                |  1 +
 5 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 40461547701a..fe0237f72a09 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -4382,7 +4382,7 @@ int amdgpu_device_mode1_reset(struct amdgpu_device *adev)
 int amdgpu_device_pre_asic_reset(struct amdgpu_device *adev,
 				 struct amdgpu_reset_context *reset_context)
 {
-	int i, r = 0;
+	int i, j, r = 0;
 	struct amdgpu_job *job = NULL;
 	bool need_full_reset =
 		test_bit(AMDGPU_NEED_FULL_RESET, &reset_context->flags);
@@ -4406,6 +4406,16 @@ int amdgpu_device_pre_asic_reset(struct amdgpu_device *adev,
 		if (!ring || !ring->sched.thread)
 			continue;
 
+		/*clear job fence from fence drv to avoid force_completion
+		 *leave NULL and vm flush fence in fence drv */
+		for (j = 0; j <= ring->fence_drv.num_fences_mask; j ++) {
+			struct dma_fence *old,**ptr;
+			ptr = &ring->fence_drv.fences[j];
+			old = rcu_dereference_protected(*ptr, 1);
+			if (old && test_bit(DMA_FENCE_FLAG_USER_BITS, &old->flags))) {
+				RCU_INIT_POINTER(*ptr, NULL);
+			}
+		}
 		/* after all hw jobs are reset, hw fence is meaningless, so force_completion */
 		amdgpu_fence_driver_force_completion(ring);
 	}
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
index eecf21d8ec33..815776c9a013 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
@@ -156,11 +156,17 @@ int amdgpu_fence_emit(struct amdgpu_ring *ring, struct dma_fence **f, struct amd
 		job->ring = ring;
 	}
 
-	seq = ++ring->fence_drv.sync_seq;
-	dma_fence_init(fence, &amdgpu_fence_ops,
-		       &ring->fence_drv.lock,
-		       adev->fence_context + ring->idx,
-		       seq);
+	if (job != NULL && job->base.resubmit_flag == 1) {
+		/* reinit seq for resubmitted jobs */
+		seq = ++ring->fence_drv.sync_seq;
+		fence->seqno = seq;
+	} else {
+		seq = ++ring->fence_drv.sync_seq;
+		dma_fence_init(fence, &amdgpu_fence_ops,
+				&ring->fence_drv.lock,
+				adev->fence_context + ring->idx,
+				seq);
+	}
 
 	if (job != NULL) {
 		/* mark this fence has a parent job */
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
index 7c426e225b24..d6f848adc3f4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
@@ -241,6 +241,7 @@ static struct dma_fence *amdgpu_job_run(struct drm_sched_job *sched_job)
 		dma_fence_set_error(finished, -ECANCELED);/* skip IB as well if VRAM lost */
 
 	if (finished->error < 0) {
+		dma_fence_put(&job->hw_fence);
 		DRM_INFO("Skip scheduling IBs!\n");
 	} else {
 		r = amdgpu_ib_schedule(ring, job->num_ibs, job->ibs, job,
@@ -249,7 +250,8 @@ static struct dma_fence *amdgpu_job_run(struct drm_sched_job *sched_job)
 			DRM_ERROR("Error scheduling IBs (%d)\n", r);
 	}
 
-	dma_fence_get(fence);
+	if (!job->base.resubmit_flag)
+		dma_fence_get(fence);
 	amdgpu_job_free_resources(job);
 
 	fence = r ? ERR_PTR(r) : fence;
diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index f4f474944169..5a36ab5aea2d 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -544,6 +544,7 @@ void drm_sched_resubmit_jobs_ext(struct drm_gpu_scheduler *sched, int max)
 			dma_fence_set_error(&s_fence->finished, -ECANCELED);
 
 		dma_fence_put(s_job->s_fence->parent);
+		s_job->resubmit_flag = 1;
 		fence = sched->ops->run_job(s_job);
 		i++;
 
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index 4ea8606d91fe..06c101af1f71 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -198,6 +198,7 @@ struct drm_sched_job {
 	enum drm_sched_priority		s_priority;
 	struct drm_sched_entity         *entity;
 	struct dma_fence_cb		cb;
+	int resubmit_flag;
 };
 
 static inline bool drm_sched_invalidate_job(struct drm_sched_job *s_job,
-- 
2.25.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm: add tdr support for embeded hw_fence

[Andrey Grodzovsky]

On 2021-07-20 11:13 p.m., Jingwen Chen wrote:
> [Why]
> After embeded hw_fence to amdgpu_job, we need to add tdr support
> for this feature.
>
> [How]
> 1. Add a resubmit_flag for resubmit jobs.
> 2. Clear job fence from RCU and force complete vm flush fences in
>     pre_asic_reset
> 3. skip dma_fence_get for resubmit jobs and add a dma_fence_put
>     for guilty jobs.
>
> Signed-off-by: Jack Zhang <Jack.Zhang1@amd.com>
> Signed-off-by: Jingwen Chen <Jingwen.Chen2@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 12 +++++++++++-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c  | 16 +++++++++++-----
>   drivers/gpu/drm/amd/amdgpu/amdgpu_job.c    |  4 +++-
>   drivers/gpu/drm/scheduler/sched_main.c     |  1 +
>   include/drm/gpu_scheduler.h                |  1 +
>   5 files changed, 27 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 40461547701a..fe0237f72a09 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -4382,7 +4382,7 @@ int amdgpu_device_mode1_reset(struct amdgpu_device *adev)
>   int amdgpu_device_pre_asic_reset(struct amdgpu_device *adev,
>   				 struct amdgpu_reset_context *reset_context)
>   {
> -	int i, r = 0;
> +	int i, j, r = 0;
>   	struct amdgpu_job *job = NULL;
>   	bool need_full_reset =
>   		test_bit(AMDGPU_NEED_FULL_RESET, &reset_context->flags);
> @@ -4406,6 +4406,16 @@ int amdgpu_device_pre_asic_reset(struct amdgpu_device *adev,
>   		if (!ring || !ring->sched.thread)
>   			continue;
>   
> +		/*clear job fence from fence drv to avoid force_completion
> +		 *leave NULL and vm flush fence in fence drv */
> +		for (j = 0; j <= ring->fence_drv.num_fences_mask; j ++) {
> +			struct dma_fence *old,**ptr;
> +			ptr = &ring->fence_drv.fences[j];
> +			old = rcu_dereference_protected(*ptr, 1);
> +			if (old && test_bit(DMA_FENCE_FLAG_USER_BITS, &old->flags))) {
> +				RCU_INIT_POINTER(*ptr, NULL);
> +			}


Is this to avoid premature job free because of dma_fence_put inside 
amdgpu_fence_process ?
I can't currently remember why but we probably want all the HW fences 
currently in the ring to
be forced signaled - maybe better to test for DMA_FENCE_FLAG_USER_BITS 
inside amdgpu_fence_process
and still do the signaling but not the dma_fence_put part

Andrey


> +		}
>   		/* after all hw jobs are reset, hw fence is meaningless, so force_completion */
>   		amdgpu_fence_driver_force_completion(ring);
>   	}
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> index eecf21d8ec33..815776c9a013 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> @@ -156,11 +156,17 @@ int amdgpu_fence_emit(struct amdgpu_ring *ring, struct dma_fence **f, struct amd
>   		job->ring = ring;
>   	}
>   
> -	seq = ++ring->fence_drv.sync_seq;
> -	dma_fence_init(fence, &amdgpu_fence_ops,
> -		       &ring->fence_drv.lock,
> -		       adev->fence_context + ring->idx,
> -		       seq);
> +	if (job != NULL && job->base.resubmit_flag == 1) {
> +		/* reinit seq for resubmitted jobs */
> +		seq = ++ring->fence_drv.sync_seq;
> +		fence->seqno = seq;
> +	} else {
> +		seq = ++ring->fence_drv.sync_seq;


Seems like you could do the above line only once above if-else as it was 
before


> +		dma_fence_init(fence, &amdgpu_fence_ops,
> +				&ring->fence_drv.lock,
> +				adev->fence_context + ring->idx,
> +				seq);
> +	}
>   
>   	if (job != NULL) {
>   		/* mark this fence has a parent job */
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> index 7c426e225b24..d6f848adc3f4 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> @@ -241,6 +241,7 @@ static struct dma_fence *amdgpu_job_run(struct drm_sched_job *sched_job)
>   		dma_fence_set_error(finished, -ECANCELED);/* skip IB as well if VRAM lost */
>   
>   	if (finished->error < 0) {
> +		dma_fence_put(&job->hw_fence);
>   		DRM_INFO("Skip scheduling IBs!\n");
>   	} else {
>   		r = amdgpu_ib_schedule(ring, job->num_ibs, job->ibs, job,
> @@ -249,7 +250,8 @@ static struct dma_fence *amdgpu_job_run(struct drm_sched_job *sched_job)
>   			DRM_ERROR("Error scheduling IBs (%d)\n", r);
>   	}
>   
> -	dma_fence_get(fence);
> +	if (!job->base.resubmit_flag)
> +		dma_fence_get(fence);
>   	amdgpu_job_free_resources(job);
>   
>   	fence = r ? ERR_PTR(r) : fence;
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> index f4f474944169..5a36ab5aea2d 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -544,6 +544,7 @@ void drm_sched_resubmit_jobs_ext(struct drm_gpu_scheduler *sched, int max)
>   			dma_fence_set_error(&s_fence->finished, -ECANCELED);
>   
>   		dma_fence_put(s_job->s_fence->parent);
> +		s_job->resubmit_flag = 1;
>   		fence = sched->ops->run_job(s_job);
>   		i++;
>   
> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> index 4ea8606d91fe..06c101af1f71 100644
> --- a/include/drm/gpu_scheduler.h
> +++ b/include/drm/gpu_scheduler.h
> @@ -198,6 +198,7 @@ struct drm_sched_job {
>   	enum drm_sched_priority		s_priority;
>   	struct drm_sched_entity         *entity;
>   	struct dma_fence_cb		cb;
> +	int resubmit_flag;
>   };
>   
>   static inline bool drm_sched_invalidate_job(struct drm_sched_job *s_job,
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm: add tdr support for embeded hw_fence

[Jingwen Chen]

On Wed Jul 21, 2021 at 12:53:51PM -0400, Andrey Grodzovsky wrote:
> 
> On 2021-07-20 11:13 p.m., Jingwen Chen wrote:
> > [Why]
> > After embeded hw_fence to amdgpu_job, we need to add tdr support
> > for this feature.
> > 
> > [How]
> > 1. Add a resubmit_flag for resubmit jobs.
> > 2. Clear job fence from RCU and force complete vm flush fences in
> >     pre_asic_reset
> > 3. skip dma_fence_get for resubmit jobs and add a dma_fence_put
> >     for guilty jobs.
> > 
> > Signed-off-by: Jack Zhang <Jack.Zhang1@amd.com>
> > Signed-off-by: Jingwen Chen <Jingwen.Chen2@amd.com>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 12 +++++++++++-
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c  | 16 +++++++++++-----
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_job.c    |  4 +++-
> >   drivers/gpu/drm/scheduler/sched_main.c     |  1 +
> >   include/drm/gpu_scheduler.h                |  1 +
> >   5 files changed, 27 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > index 40461547701a..fe0237f72a09 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > @@ -4382,7 +4382,7 @@ int amdgpu_device_mode1_reset(struct amdgpu_device *adev)
> >   int amdgpu_device_pre_asic_reset(struct amdgpu_device *adev,
> >   				 struct amdgpu_reset_context *reset_context)
> >   {
> > -	int i, r = 0;
> > +	int i, j, r = 0;
> >   	struct amdgpu_job *job = NULL;
> >   	bool need_full_reset =
> >   		test_bit(AMDGPU_NEED_FULL_RESET, &reset_context->flags);
> > @@ -4406,6 +4406,16 @@ int amdgpu_device_pre_asic_reset(struct amdgpu_device *adev,
> >   		if (!ring || !ring->sched.thread)
> >   			continue;
> > +		/*clear job fence from fence drv to avoid force_completion
> > +		 *leave NULL and vm flush fence in fence drv */
> > +		for (j = 0; j <= ring->fence_drv.num_fences_mask; j ++) {
> > +			struct dma_fence *old,**ptr;
> > +			ptr = &ring->fence_drv.fences[j];
> > +			old = rcu_dereference_protected(*ptr, 1);
> > +			if (old && test_bit(DMA_FENCE_FLAG_USER_BITS, &old->flags))) {
> > +				RCU_INIT_POINTER(*ptr, NULL);
> > +			}
> 
> 
> Is this to avoid premature job free because of dma_fence_put inside
> amdgpu_fence_process ?
> I can't currently remember why but we probably want all the HW fences
> currently in the ring to
> be forced signaled - maybe better to test for DMA_FENCE_FLAG_USER_BITS
> inside amdgpu_fence_process
> and still do the signaling but not the dma_fence_put part
> 
> Andrey

Hi Andrey,

This is to avoid signaling the same fence twice. If we still do the
signaling, then the job in the pending list will be signaled first in
force_completion, and later be signaled in resubmit. This will go to
BUG() in amdgpu_fence_process.

> 
> > +		}
> >   		/* after all hw jobs are reset, hw fence is meaningless, so force_completion */
> >   		amdgpu_fence_driver_force_completion(ring);
> >   	}
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> > index eecf21d8ec33..815776c9a013 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> > @@ -156,11 +156,17 @@ int amdgpu_fence_emit(struct amdgpu_ring *ring, struct dma_fence **f, struct amd
> >   		job->ring = ring;
> >   	}
> > -	seq = ++ring->fence_drv.sync_seq;
> > -	dma_fence_init(fence, &amdgpu_fence_ops,
> > -		       &ring->fence_drv.lock,
> > -		       adev->fence_context + ring->idx,
> > -		       seq);
> > +	if (job != NULL && job->base.resubmit_flag == 1) {
> > +		/* reinit seq for resubmitted jobs */
> > +		seq = ++ring->fence_drv.sync_seq;
> > +		fence->seqno = seq;
> > +	} else {
> > +		seq = ++ring->fence_drv.sync_seq;
> 
> 
> Seems like you could do the above line only once above if-else as it was
> before

Sure, I will modify this.


Best Regards,
JingWen Chen
> 
> > +		dma_fence_init(fence, &amdgpu_fence_ops,
> > +				&ring->fence_drv.lock,
> > +				adev->fence_context + ring->idx,
> > +				seq);
> > +	}
> >   	if (job != NULL) {
> >   		/* mark this fence has a parent job */
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > index 7c426e225b24..d6f848adc3f4 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > @@ -241,6 +241,7 @@ static struct dma_fence *amdgpu_job_run(struct drm_sched_job *sched_job)
> >   		dma_fence_set_error(finished, -ECANCELED);/* skip IB as well if VRAM lost */
> >   	if (finished->error < 0) {
> > +		dma_fence_put(&job->hw_fence);
> >   		DRM_INFO("Skip scheduling IBs!\n");
> >   	} else {
> >   		r = amdgpu_ib_schedule(ring, job->num_ibs, job->ibs, job,
> > @@ -249,7 +250,8 @@ static struct dma_fence *amdgpu_job_run(struct drm_sched_job *sched_job)
> >   			DRM_ERROR("Error scheduling IBs (%d)\n", r);
> >   	}
> > -	dma_fence_get(fence);
> > +	if (!job->base.resubmit_flag)
> > +		dma_fence_get(fence);
> >   	amdgpu_job_free_resources(job);
> >   	fence = r ? ERR_PTR(r) : fence;
> > diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> > index f4f474944169..5a36ab5aea2d 100644
> > --- a/drivers/gpu/drm/scheduler/sched_main.c
> > +++ b/drivers/gpu/drm/scheduler/sched_main.c
> > @@ -544,6 +544,7 @@ void drm_sched_resubmit_jobs_ext(struct drm_gpu_scheduler *sched, int max)
> >   			dma_fence_set_error(&s_fence->finished, -ECANCELED);
> >   		dma_fence_put(s_job->s_fence->parent);
> > +		s_job->resubmit_flag = 1;
> >   		fence = sched->ops->run_job(s_job);
> >   		i++;
> > diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> > index 4ea8606d91fe..06c101af1f71 100644
> > --- a/include/drm/gpu_scheduler.h
> > +++ b/include/drm/gpu_scheduler.h
> > @@ -198,6 +198,7 @@ struct drm_sched_job {
> >   	enum drm_sched_priority		s_priority;
> >   	struct drm_sched_entity         *entity;
> >   	struct dma_fence_cb		cb;
> > +	int resubmit_flag;
> >   };
> >   static inline bool drm_sched_invalidate_job(struct drm_sched_job *s_job,
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm: add tdr support for embeded hw_fence

[Andrey Grodzovsky]

On 2021-07-22 6:45 a.m., Jingwen Chen wrote:
> On Wed Jul 21, 2021 at 12:53:51PM -0400, Andrey Grodzovsky wrote:
>> On 2021-07-20 11:13 p.m., Jingwen Chen wrote:
>>> [Why]
>>> After embeded hw_fence to amdgpu_job, we need to add tdr support
>>> for this feature.
>>>
>>> [How]
>>> 1. Add a resubmit_flag for resubmit jobs.
>>> 2. Clear job fence from RCU and force complete vm flush fences in
>>>      pre_asic_reset
>>> 3. skip dma_fence_get for resubmit jobs and add a dma_fence_put
>>>      for guilty jobs.
>>>
>>> Signed-off-by: Jack Zhang <Jack.Zhang1@amd.com>
>>> Signed-off-by: Jingwen Chen <Jingwen.Chen2@amd.com>
>>> ---
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 12 +++++++++++-
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c  | 16 +++++++++++-----
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_job.c    |  4 +++-
>>>    drivers/gpu/drm/scheduler/sched_main.c     |  1 +
>>>    include/drm/gpu_scheduler.h                |  1 +
>>>    5 files changed, 27 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> index 40461547701a..fe0237f72a09 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> @@ -4382,7 +4382,7 @@ int amdgpu_device_mode1_reset(struct amdgpu_device *adev)
>>>    int amdgpu_device_pre_asic_reset(struct amdgpu_device *adev,
>>>    				 struct amdgpu_reset_context *reset_context)
>>>    {
>>> -	int i, r = 0;
>>> +	int i, j, r = 0;
>>>    	struct amdgpu_job *job = NULL;
>>>    	bool need_full_reset =
>>>    		test_bit(AMDGPU_NEED_FULL_RESET, &reset_context->flags);
>>> @@ -4406,6 +4406,16 @@ int amdgpu_device_pre_asic_reset(struct amdgpu_device *adev,
>>>    		if (!ring || !ring->sched.thread)
>>>    			continue;
>>> +		/*clear job fence from fence drv to avoid force_completion
>>> +		 *leave NULL and vm flush fence in fence drv */
>>> +		for (j = 0; j <= ring->fence_drv.num_fences_mask; j ++) {
>>> +			struct dma_fence *old,**ptr;
>>> +			ptr = &ring->fence_drv.fences[j];
>>> +			old = rcu_dereference_protected(*ptr, 1);
>>> +			if (old && test_bit(DMA_FENCE_FLAG_USER_BITS, &old->flags))) {
>>> +				RCU_INIT_POINTER(*ptr, NULL);
>>> +			}
>>
>> Is this to avoid premature job free because of dma_fence_put inside
>> amdgpu_fence_process ?
>> I can't currently remember why but we probably want all the HW fences
>> currently in the ring to
>> be forced signaled - maybe better to test for DMA_FENCE_FLAG_USER_BITS
>> inside amdgpu_fence_process
>> and still do the signaling but not the dma_fence_put part
>>
>> Andrey
> Hi Andrey,
>
> This is to avoid signaling the same fence twice. If we still do the
> signaling, then the job in the pending list will be signaled first in
> force_completion, and later be signaled in resubmit. This will go to
> BUG() in amdgpu_fence_process.


Oh, i see, how about just adding 'skip' flag to amdgpu_ring and setting 
it before calling
amdgpu_fence_driver_force_completion and resetting it after, then inside 
amdgpu_fence_driver_force_completion
you can just skip the signaling part with this flag for fences with 
DMA_FENCE_FLAG_USER_BITS set
Less lines of code at least.

Andrey


>
>>> +		}
>>>    		/* after all hw jobs are reset, hw fence is meaningless, so force_completion */
>>>    		amdgpu_fence_driver_force_completion(ring);
>>>    	}
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>> index eecf21d8ec33..815776c9a013 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>> @@ -156,11 +156,17 @@ int amdgpu_fence_emit(struct amdgpu_ring *ring, struct dma_fence **f, struct amd
>>>    		job->ring = ring;
>>>    	}
>>> -	seq = ++ring->fence_drv.sync_seq;
>>> -	dma_fence_init(fence, &amdgpu_fence_ops,
>>> -		       &ring->fence_drv.lock,
>>> -		       adev->fence_context + ring->idx,
>>> -		       seq);
>>> +	if (job != NULL && job->base.resubmit_flag == 1) {
>>> +		/* reinit seq for resubmitted jobs */
>>> +		seq = ++ring->fence_drv.sync_seq;
>>> +		fence->seqno = seq;
>>> +	} else {
>>> +		seq = ++ring->fence_drv.sync_seq;
>>
>> Seems like you could do the above line only once above if-else as it was
>> before
> Sure, I will modify this.
>
>
> Best Regards,
> JingWen Chen
>>> +		dma_fence_init(fence, &amdgpu_fence_ops,
>>> +				&ring->fence_drv.lock,
>>> +				adev->fence_context + ring->idx,
>>> +				seq);
>>> +	}
>>>    	if (job != NULL) {
>>>    		/* mark this fence has a parent job */
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>> index 7c426e225b24..d6f848adc3f4 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>> @@ -241,6 +241,7 @@ static struct dma_fence *amdgpu_job_run(struct drm_sched_job *sched_job)
>>>    		dma_fence_set_error(finished, -ECANCELED);/* skip IB as well if VRAM lost */
>>>    	if (finished->error < 0) {
>>> +		dma_fence_put(&job->hw_fence);
>>>    		DRM_INFO("Skip scheduling IBs!\n");
>>>    	} else {
>>>    		r = amdgpu_ib_schedule(ring, job->num_ibs, job->ibs, job,
>>> @@ -249,7 +250,8 @@ static struct dma_fence *amdgpu_job_run(struct drm_sched_job *sched_job)
>>>    			DRM_ERROR("Error scheduling IBs (%d)\n", r);
>>>    	}
>>> -	dma_fence_get(fence);
>>> +	if (!job->base.resubmit_flag)
>>> +		dma_fence_get(fence);
>>>    	amdgpu_job_free_resources(job);
>>>    	fence = r ? ERR_PTR(r) : fence;
>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
>>> index f4f474944169..5a36ab5aea2d 100644
>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>> @@ -544,6 +544,7 @@ void drm_sched_resubmit_jobs_ext(struct drm_gpu_scheduler *sched, int max)
>>>    			dma_fence_set_error(&s_fence->finished, -ECANCELED);
>>>    		dma_fence_put(s_job->s_fence->parent);
>>> +		s_job->resubmit_flag = 1;
>>>    		fence = sched->ops->run_job(s_job);
>>>    		i++;
>>> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
>>> index 4ea8606d91fe..06c101af1f71 100644
>>> --- a/include/drm/gpu_scheduler.h
>>> +++ b/include/drm/gpu_scheduler.h
>>> @@ -198,6 +198,7 @@ struct drm_sched_job {
>>>    	enum drm_sched_priority		s_priority;
>>>    	struct drm_sched_entity         *entity;
>>>    	struct dma_fence_cb		cb;
>>> +	int resubmit_flag;
>>>    };
>>>    static inline bool drm_sched_invalidate_job(struct drm_sched_job *s_job,
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm: add tdr support for embeded hw_fence

[Jingwen Chen]

On Thu Jul 22, 2021 at 10:45:40AM -0400, Andrey Grodzovsky wrote:
> 
> On 2021-07-22 6:45 a.m., Jingwen Chen wrote:
> > On Wed Jul 21, 2021 at 12:53:51PM -0400, Andrey Grodzovsky wrote:
> > > On 2021-07-20 11:13 p.m., Jingwen Chen wrote:
> > > > [Why]
> > > > After embeded hw_fence to amdgpu_job, we need to add tdr support
> > > > for this feature.
> > > > 
> > > > [How]
> > > > 1. Add a resubmit_flag for resubmit jobs.
> > > > 2. Clear job fence from RCU and force complete vm flush fences in
> > > >      pre_asic_reset
> > > > 3. skip dma_fence_get for resubmit jobs and add a dma_fence_put
> > > >      for guilty jobs.
> > > > 
> > > > Signed-off-by: Jack Zhang <Jack.Zhang1@amd.com>
> > > > Signed-off-by: Jingwen Chen <Jingwen.Chen2@amd.com>
> > > > ---
> > > >    drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 12 +++++++++++-
> > > >    drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c  | 16 +++++++++++-----
> > > >    drivers/gpu/drm/amd/amdgpu/amdgpu_job.c    |  4 +++-
> > > >    drivers/gpu/drm/scheduler/sched_main.c     |  1 +
> > > >    include/drm/gpu_scheduler.h                |  1 +
> > > >    5 files changed, 27 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > > > index 40461547701a..fe0237f72a09 100644
> > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > > > @@ -4382,7 +4382,7 @@ int amdgpu_device_mode1_reset(struct amdgpu_device *adev)
> > > >    int amdgpu_device_pre_asic_reset(struct amdgpu_device *adev,
> > > >    				 struct amdgpu_reset_context *reset_context)
> > > >    {
> > > > -	int i, r = 0;
> > > > +	int i, j, r = 0;
> > > >    	struct amdgpu_job *job = NULL;
> > > >    	bool need_full_reset =
> > > >    		test_bit(AMDGPU_NEED_FULL_RESET, &reset_context->flags);
> > > > @@ -4406,6 +4406,16 @@ int amdgpu_device_pre_asic_reset(struct amdgpu_device *adev,
> > > >    		if (!ring || !ring->sched.thread)
> > > >    			continue;
> > > > +		/*clear job fence from fence drv to avoid force_completion
> > > > +		 *leave NULL and vm flush fence in fence drv */
> > > > +		for (j = 0; j <= ring->fence_drv.num_fences_mask; j ++) {
> > > > +			struct dma_fence *old,**ptr;
> > > > +			ptr = &ring->fence_drv.fences[j];
> > > > +			old = rcu_dereference_protected(*ptr, 1);
> > > > +			if (old && test_bit(DMA_FENCE_FLAG_USER_BITS, &old->flags))) {
> > > > +				RCU_INIT_POINTER(*ptr, NULL);
> > > > +			}
> > > 
> > > Is this to avoid premature job free because of dma_fence_put inside
> > > amdgpu_fence_process ?
> > > I can't currently remember why but we probably want all the HW fences
> > > currently in the ring to
> > > be forced signaled - maybe better to test for DMA_FENCE_FLAG_USER_BITS
> > > inside amdgpu_fence_process
> > > and still do the signaling but not the dma_fence_put part
> > > 
> > > Andrey
> > Hi Andrey,
> > 
> > This is to avoid signaling the same fence twice. If we still do the
> > signaling, then the job in the pending list will be signaled first in
> > force_completion, and later be signaled in resubmit. This will go to
> > BUG() in amdgpu_fence_process.
> 
> 
> Oh, i see, how about just adding 'skip' flag to amdgpu_ring and setting it
> before calling
> amdgpu_fence_driver_force_completion and resetting it after, then inside
> amdgpu_fence_driver_force_completion
> you can just skip the signaling part with this flag for fences with
> DMA_FENCE_FLAG_USER_BITS set
> Less lines of code at least.
> 
> Andrey
Hi Andrey,

In this way, this issue still exists. If we just skip it in the
force_completion, these fences are still in the RCU fence array. So when
the FLR finishes, the eop interrupt function of ib_test will call
amdgpu_fence_process, the skipped fences will be signaled along with
ib_test fences and signaled again during resubmission.


Best Regards,
JingWen Chen
> 
> > 
> > > > +		}
> > > >    		/* after all hw jobs are reset, hw fence is meaningless, so force_completion */
> > > >    		amdgpu_fence_driver_force_completion(ring);
> > > >    	}
> > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> > > > index eecf21d8ec33..815776c9a013 100644
> > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> > > > @@ -156,11 +156,17 @@ int amdgpu_fence_emit(struct amdgpu_ring *ring, struct dma_fence **f, struct amd
> > > >    		job->ring = ring;
> > > >    	}
> > > > -	seq = ++ring->fence_drv.sync_seq;
> > > > -	dma_fence_init(fence, &amdgpu_fence_ops,
> > > > -		       &ring->fence_drv.lock,
> > > > -		       adev->fence_context + ring->idx,
> > > > -		       seq);
> > > > +	if (job != NULL && job->base.resubmit_flag == 1) {
> > > > +		/* reinit seq for resubmitted jobs */
> > > > +		seq = ++ring->fence_drv.sync_seq;
> > > > +		fence->seqno = seq;
> > > > +	} else {
> > > > +		seq = ++ring->fence_drv.sync_seq;
> > > 
> > > Seems like you could do the above line only once above if-else as it was
> > > before
> > Sure, I will modify this.
> > 
> > 
> > Best Regards,
> > JingWen Chen
> > > > +		dma_fence_init(fence, &amdgpu_fence_ops,
> > > > +				&ring->fence_drv.lock,
> > > > +				adev->fence_context + ring->idx,
> > > > +				seq);
> > > > +	}
> > > >    	if (job != NULL) {
> > > >    		/* mark this fence has a parent job */
> > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > > > index 7c426e225b24..d6f848adc3f4 100644
> > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > > > @@ -241,6 +241,7 @@ static struct dma_fence *amdgpu_job_run(struct drm_sched_job *sched_job)
> > > >    		dma_fence_set_error(finished, -ECANCELED);/* skip IB as well if VRAM lost */
> > > >    	if (finished->error < 0) {
> > > > +		dma_fence_put(&job->hw_fence);
> > > >    		DRM_INFO("Skip scheduling IBs!\n");
> > > >    	} else {
> > > >    		r = amdgpu_ib_schedule(ring, job->num_ibs, job->ibs, job,
> > > > @@ -249,7 +250,8 @@ static struct dma_fence *amdgpu_job_run(struct drm_sched_job *sched_job)
> > > >    			DRM_ERROR("Error scheduling IBs (%d)\n", r);
> > > >    	}
> > > > -	dma_fence_get(fence);
> > > > +	if (!job->base.resubmit_flag)
> > > > +		dma_fence_get(fence);
> > > >    	amdgpu_job_free_resources(job);
> > > >    	fence = r ? ERR_PTR(r) : fence;
> > > > diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> > > > index f4f474944169..5a36ab5aea2d 100644
> > > > --- a/drivers/gpu/drm/scheduler/sched_main.c
> > > > +++ b/drivers/gpu/drm/scheduler/sched_main.c
> > > > @@ -544,6 +544,7 @@ void drm_sched_resubmit_jobs_ext(struct drm_gpu_scheduler *sched, int max)
> > > >    			dma_fence_set_error(&s_fence->finished, -ECANCELED);
> > > >    		dma_fence_put(s_job->s_fence->parent);
> > > > +		s_job->resubmit_flag = 1;
> > > >    		fence = sched->ops->run_job(s_job);
> > > >    		i++;
> > > > diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> > > > index 4ea8606d91fe..06c101af1f71 100644
> > > > --- a/include/drm/gpu_scheduler.h
> > > > +++ b/include/drm/gpu_scheduler.h
> > > > @@ -198,6 +198,7 @@ struct drm_sched_job {
> > > >    	enum drm_sched_priority		s_priority;
> > > >    	struct drm_sched_entity         *entity;
> > > >    	struct dma_fence_cb		cb;
> > > > +	int resubmit_flag;
> > > >    };
> > > >    static inline bool drm_sched_invalidate_job(struct drm_sched_job *s_job,
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm: add tdr support for embeded hw_fence

[Christian König]

Am 22.07.21 um 16:45 schrieb Andrey Grodzovsky:
>
> On 2021-07-22 6:45 a.m., Jingwen Chen wrote:
>> On Wed Jul 21, 2021 at 12:53:51PM -0400, Andrey Grodzovsky wrote:
>>> On 2021-07-20 11:13 p.m., Jingwen Chen wrote:
>>>> [Why]
>>>> After embeded hw_fence to amdgpu_job, we need to add tdr support
>>>> for this feature.
>>>>
>>>> [How]
>>>> 1. Add a resubmit_flag for resubmit jobs.
>>>> 2. Clear job fence from RCU and force complete vm flush fences in
>>>>      pre_asic_reset
>>>> 3. skip dma_fence_get for resubmit jobs and add a dma_fence_put
>>>>      for guilty jobs.
>>>>
>>>> Signed-off-by: Jack Zhang <Jack.Zhang1@amd.com>
>>>> Signed-off-by: Jingwen Chen <Jingwen.Chen2@amd.com>
>>>> ---
>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 12 +++++++++++-
>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c  | 16 +++++++++++-----
>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_job.c    |  4 +++-
>>>>    drivers/gpu/drm/scheduler/sched_main.c     |  1 +
>>>>    include/drm/gpu_scheduler.h                |  1 +
>>>>    5 files changed, 27 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>> index 40461547701a..fe0237f72a09 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>> @@ -4382,7 +4382,7 @@ int amdgpu_device_mode1_reset(struct 
>>>> amdgpu_device *adev)
>>>>    int amdgpu_device_pre_asic_reset(struct amdgpu_device *adev,
>>>>                     struct amdgpu_reset_context *reset_context)
>>>>    {
>>>> -    int i, r = 0;
>>>> +    int i, j, r = 0;
>>>>        struct amdgpu_job *job = NULL;
>>>>        bool need_full_reset =
>>>>            test_bit(AMDGPU_NEED_FULL_RESET, &reset_context->flags);
>>>> @@ -4406,6 +4406,16 @@ int amdgpu_device_pre_asic_reset(struct 
>>>> amdgpu_device *adev,
>>>>            if (!ring || !ring->sched.thread)
>>>>                continue;
>>>> +        /*clear job fence from fence drv to avoid force_completion
>>>> +         *leave NULL and vm flush fence in fence drv */
>>>> +        for (j = 0; j <= ring->fence_drv.num_fences_mask; j ++) {
>>>> +            struct dma_fence *old,**ptr;
>>>> +            ptr = &ring->fence_drv.fences[j];
>>>> +            old = rcu_dereference_protected(*ptr, 1);
>>>> +            if (old && test_bit(DMA_FENCE_FLAG_USER_BITS, 
>>>> &old->flags))) {
>>>> +                RCU_INIT_POINTER(*ptr, NULL);
>>>> +            }
>>>
>>> Is this to avoid premature job free because of dma_fence_put inside
>>> amdgpu_fence_process ?
>>> I can't currently remember why but we probably want all the HW fences
>>> currently in the ring to
>>> be forced signaled - maybe better to test for DMA_FENCE_FLAG_USER_BITS
>>> inside amdgpu_fence_process
>>> and still do the signaling but not the dma_fence_put part
>>>
>>> Andrey
>> Hi Andrey,
>>
>> This is to avoid signaling the same fence twice. If we still do the
>> signaling, then the job in the pending list will be signaled first in
>> force_completion, and later be signaled in resubmit. This will go to
>> BUG() in amdgpu_fence_process.
>
>
> Oh, i see, how about just adding 'skip' flag to amdgpu_ring and 
> setting it before calling
> amdgpu_fence_driver_force_completion and resetting it after, then 
> inside amdgpu_fence_driver_force_completion
> you can just skip the signaling part with this flag for fences with 
> DMA_FENCE_FLAG_USER_BITS set
> Less lines of code at least.

Still sounds quite a bit hacky.

I would rather suggest to completely drop the approach with 
amdgpu_fence_driver_force_completion(). I could never see why we would 
want that in the first place.

Regards,
Christian.

>
> Andrey
>
>
>>
>>>> +        }
>>>>            /* after all hw jobs are reset, hw fence is meaningless, 
>>>> so force_completion */
>>>>            amdgpu_fence_driver_force_completion(ring);
>>>>        }
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>>> index eecf21d8ec33..815776c9a013 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>>> @@ -156,11 +156,17 @@ int amdgpu_fence_emit(struct amdgpu_ring 
>>>> *ring, struct dma_fence **f, struct amd
>>>>            job->ring = ring;
>>>>        }
>>>> -    seq = ++ring->fence_drv.sync_seq;
>>>> -    dma_fence_init(fence, &amdgpu_fence_ops,
>>>> -               &ring->fence_drv.lock,
>>>> -               adev->fence_context + ring->idx,
>>>> -               seq);
>>>> +    if (job != NULL && job->base.resubmit_flag == 1) {
>>>> +        /* reinit seq for resubmitted jobs */
>>>> +        seq = ++ring->fence_drv.sync_seq;
>>>> +        fence->seqno = seq;
>>>> +    } else {
>>>> +        seq = ++ring->fence_drv.sync_seq;
>>>
>>> Seems like you could do the above line only once above if-else as it 
>>> was
>>> before
>> Sure, I will modify this.
>>
>>
>> Best Regards,
>> JingWen Chen
>>>> +        dma_fence_init(fence, &amdgpu_fence_ops,
>>>> +                &ring->fence_drv.lock,
>>>> +                adev->fence_context + ring->idx,
>>>> +                seq);
>>>> +    }
>>>>        if (job != NULL) {
>>>>            /* mark this fence has a parent job */
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>> index 7c426e225b24..d6f848adc3f4 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>> @@ -241,6 +241,7 @@ static struct dma_fence *amdgpu_job_run(struct 
>>>> drm_sched_job *sched_job)
>>>>            dma_fence_set_error(finished, -ECANCELED);/* skip IB as 
>>>> well if VRAM lost */
>>>>        if (finished->error < 0) {
>>>> +        dma_fence_put(&job->hw_fence);
>>>>            DRM_INFO("Skip scheduling IBs!\n");
>>>>        } else {
>>>>            r = amdgpu_ib_schedule(ring, job->num_ibs, job->ibs, job,
>>>> @@ -249,7 +250,8 @@ static struct dma_fence *amdgpu_job_run(struct 
>>>> drm_sched_job *sched_job)
>>>>                DRM_ERROR("Error scheduling IBs (%d)\n", r);
>>>>        }
>>>> -    dma_fence_get(fence);
>>>> +    if (!job->base.resubmit_flag)
>>>> +        dma_fence_get(fence);
>>>>        amdgpu_job_free_resources(job);
>>>>        fence = r ? ERR_PTR(r) : fence;
>>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
>>>> b/drivers/gpu/drm/scheduler/sched_main.c
>>>> index f4f474944169..5a36ab5aea2d 100644
>>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>>> @@ -544,6 +544,7 @@ void drm_sched_resubmit_jobs_ext(struct 
>>>> drm_gpu_scheduler *sched, int max)
>>>> dma_fence_set_error(&s_fence->finished, -ECANCELED);
>>>>            dma_fence_put(s_job->s_fence->parent);
>>>> +        s_job->resubmit_flag = 1;
>>>>            fence = sched->ops->run_job(s_job);
>>>>            i++;
>>>> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
>>>> index 4ea8606d91fe..06c101af1f71 100644
>>>> --- a/include/drm/gpu_scheduler.h
>>>> +++ b/include/drm/gpu_scheduler.h
>>>> @@ -198,6 +198,7 @@ struct drm_sched_job {
>>>>        enum drm_sched_priority        s_priority;
>>>>        struct drm_sched_entity         *entity;
>>>>        struct dma_fence_cb        cb;
>>>> +    int resubmit_flag;
>>>>    };
>>>>    static inline bool drm_sched_invalidate_job(struct drm_sched_job 
>>>> *s_job,

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm: add tdr support for embeded hw_fence

[Jingwen Chen]

On Thu Jul 22, 2021 at 06:24:28PM +0200, Christian König wrote:
> Am 22.07.21 um 16:45 schrieb Andrey Grodzovsky:
> > 
> > On 2021-07-22 6:45 a.m., Jingwen Chen wrote:
> > > On Wed Jul 21, 2021 at 12:53:51PM -0400, Andrey Grodzovsky wrote:
> > > > On 2021-07-20 11:13 p.m., Jingwen Chen wrote:
> > > > > [Why]
> > > > > After embeded hw_fence to amdgpu_job, we need to add tdr support
> > > > > for this feature.
> > > > > 
> > > > > [How]
> > > > > 1. Add a resubmit_flag for resubmit jobs.
> > > > > 2. Clear job fence from RCU and force complete vm flush fences in
> > > > >      pre_asic_reset
> > > > > 3. skip dma_fence_get for resubmit jobs and add a dma_fence_put
> > > > >      for guilty jobs.
> > > > > 
> > > > > Signed-off-by: Jack Zhang <Jack.Zhang1@amd.com>
> > > > > Signed-off-by: Jingwen Chen <Jingwen.Chen2@amd.com>
> > > > > ---
> > > > >    drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 12 +++++++++++-
> > > > >    drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c  | 16 +++++++++++-----
> > > > >    drivers/gpu/drm/amd/amdgpu/amdgpu_job.c    |  4 +++-
> > > > >    drivers/gpu/drm/scheduler/sched_main.c     |  1 +
> > > > >    include/drm/gpu_scheduler.h                |  1 +
> > > > >    5 files changed, 27 insertions(+), 7 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > > > > index 40461547701a..fe0237f72a09 100644
> > > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > > > > @@ -4382,7 +4382,7 @@ int amdgpu_device_mode1_reset(struct
> > > > > amdgpu_device *adev)
> > > > >    int amdgpu_device_pre_asic_reset(struct amdgpu_device *adev,
> > > > >                     struct amdgpu_reset_context *reset_context)
> > > > >    {
> > > > > -    int i, r = 0;
> > > > > +    int i, j, r = 0;
> > > > >        struct amdgpu_job *job = NULL;
> > > > >        bool need_full_reset =
> > > > >            test_bit(AMDGPU_NEED_FULL_RESET, &reset_context->flags);
> > > > > @@ -4406,6 +4406,16 @@ int
> > > > > amdgpu_device_pre_asic_reset(struct amdgpu_device *adev,
> > > > >            if (!ring || !ring->sched.thread)
> > > > >                continue;
> > > > > +        /*clear job fence from fence drv to avoid force_completion
> > > > > +         *leave NULL and vm flush fence in fence drv */
> > > > > +        for (j = 0; j <= ring->fence_drv.num_fences_mask; j ++) {
> > > > > +            struct dma_fence *old,**ptr;
> > > > > +            ptr = &ring->fence_drv.fences[j];
> > > > > +            old = rcu_dereference_protected(*ptr, 1);
> > > > > +            if (old && test_bit(DMA_FENCE_FLAG_USER_BITS,
> > > > > &old->flags))) {
> > > > > +                RCU_INIT_POINTER(*ptr, NULL);
> > > > > +            }
> > > > 
> > > > Is this to avoid premature job free because of dma_fence_put inside
> > > > amdgpu_fence_process ?
> > > > I can't currently remember why but we probably want all the HW fences
> > > > currently in the ring to
> > > > be forced signaled - maybe better to test for DMA_FENCE_FLAG_USER_BITS
> > > > inside amdgpu_fence_process
> > > > and still do the signaling but not the dma_fence_put part
> > > > 
> > > > Andrey
> > > Hi Andrey,
> > > 
> > > This is to avoid signaling the same fence twice. If we still do the
> > > signaling, then the job in the pending list will be signaled first in
> > > force_completion, and later be signaled in resubmit. This will go to
> > > BUG() in amdgpu_fence_process.
> > 
> > 
> > Oh, i see, how about just adding 'skip' flag to amdgpu_ring and setting
> > it before calling
> > amdgpu_fence_driver_force_completion and resetting it after, then inside
> > amdgpu_fence_driver_force_completion
> > you can just skip the signaling part with this flag for fences with
> > DMA_FENCE_FLAG_USER_BITS set
> > Less lines of code at least.
> 
> Still sounds quite a bit hacky.
> 
> I would rather suggest to completely drop the approach with
> amdgpu_fence_driver_force_completion(). I could never see why we would want
> that in the first place.
> 
> Regards,
> Christian.
> 
Hi Christian,

I keep the amdgpu_fence_driver_force_completion here to make sure the vm
flush fence is signaled and put. 
So the key question is whether the fence of ib test should be the first
fence to be signaled after reset.
If it should be, it means not only fences with DMA_FENCE_FLAG_USER_BITS
but also vm flush fences should be removed from RCU fence array before
ib_test. Then we must do the force_completion here for vm flush
fence, otherwise there will be a memory leak here as no one will signal
and put it after we remove it from RCU.
If not, then the first fence to signal could be vm flush fence. And I'm
OK with drop amdgpu_fence_driver_force_completion here.


Best Regards,
JingWen Chen
> > 
> > Andrey
> > 
> > 
> > > 
> > > > > +        }
> > > > >            /* after all hw jobs are reset, hw fence is
> > > > > meaningless, so force_completion */
> > > > >            amdgpu_fence_driver_force_completion(ring);
> > > > >        }
> > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> > > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> > > > > index eecf21d8ec33..815776c9a013 100644
> > > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> > > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> > > > > @@ -156,11 +156,17 @@ int amdgpu_fence_emit(struct
> > > > > amdgpu_ring *ring, struct dma_fence **f, struct amd
> > > > >            job->ring = ring;
> > > > >        }
> > > > > -    seq = ++ring->fence_drv.sync_seq;
> > > > > -    dma_fence_init(fence, &amdgpu_fence_ops,
> > > > > -               &ring->fence_drv.lock,
> > > > > -               adev->fence_context + ring->idx,
> > > > > -               seq);
> > > > > +    if (job != NULL && job->base.resubmit_flag == 1) {
> > > > > +        /* reinit seq for resubmitted jobs */
> > > > > +        seq = ++ring->fence_drv.sync_seq;
> > > > > +        fence->seqno = seq;
> > > > > +    } else {
> > > > > +        seq = ++ring->fence_drv.sync_seq;
> > > > 
> > > > Seems like you could do the above line only once above if-else
> > > > as it was
> > > > before
> > > Sure, I will modify this.
> > > 
> > > 
> > > Best Regards,
> > > JingWen Chen
> > > > > +        dma_fence_init(fence, &amdgpu_fence_ops,
> > > > > +                &ring->fence_drv.lock,
> > > > > +                adev->fence_context + ring->idx,
> > > > > +                seq);
> > > > > +    }
> > > > >        if (job != NULL) {
> > > > >            /* mark this fence has a parent job */
> > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > > > > index 7c426e225b24..d6f848adc3f4 100644
> > > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > > > > @@ -241,6 +241,7 @@ static struct dma_fence
> > > > > *amdgpu_job_run(struct drm_sched_job *sched_job)
> > > > >            dma_fence_set_error(finished, -ECANCELED);/* skip
> > > > > IB as well if VRAM lost */
> > > > >        if (finished->error < 0) {
> > > > > +        dma_fence_put(&job->hw_fence);
> > > > >            DRM_INFO("Skip scheduling IBs!\n");
> > > > >        } else {
> > > > >            r = amdgpu_ib_schedule(ring, job->num_ibs, job->ibs, job,
> > > > > @@ -249,7 +250,8 @@ static struct dma_fence
> > > > > *amdgpu_job_run(struct drm_sched_job *sched_job)
> > > > >                DRM_ERROR("Error scheduling IBs (%d)\n", r);
> > > > >        }
> > > > > -    dma_fence_get(fence);
> > > > > +    if (!job->base.resubmit_flag)
> > > > > +        dma_fence_get(fence);
> > > > >        amdgpu_job_free_resources(job);
> > > > >        fence = r ? ERR_PTR(r) : fence;
> > > > > diff --git a/drivers/gpu/drm/scheduler/sched_main.c
> > > > > b/drivers/gpu/drm/scheduler/sched_main.c
> > > > > index f4f474944169..5a36ab5aea2d 100644
> > > > > --- a/drivers/gpu/drm/scheduler/sched_main.c
> > > > > +++ b/drivers/gpu/drm/scheduler/sched_main.c
> > > > > @@ -544,6 +544,7 @@ void drm_sched_resubmit_jobs_ext(struct
> > > > > drm_gpu_scheduler *sched, int max)
> > > > > dma_fence_set_error(&s_fence->finished, -ECANCELED);
> > > > >            dma_fence_put(s_job->s_fence->parent);
> > > > > +        s_job->resubmit_flag = 1;
> > > > >            fence = sched->ops->run_job(s_job);
> > > > >            i++;
> > > > > diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> > > > > index 4ea8606d91fe..06c101af1f71 100644
> > > > > --- a/include/drm/gpu_scheduler.h
> > > > > +++ b/include/drm/gpu_scheduler.h
> > > > > @@ -198,6 +198,7 @@ struct drm_sched_job {
> > > > >        enum drm_sched_priority        s_priority;
> > > > >        struct drm_sched_entity         *entity;
> > > > >        struct dma_fence_cb        cb;
> > > > > +    int resubmit_flag;
> > > > >    };
> > > > >    static inline bool drm_sched_invalidate_job(struct
> > > > > drm_sched_job *s_job,
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm: add tdr support for embeded hw_fence

[Andrey Grodzovsky]

On 2021-07-22 12:47 p.m., Jingwen Chen wrote:
> On Thu Jul 22, 2021 at 06:24:28PM +0200, Christian König wrote:
>> Am 22.07.21 um 16:45 schrieb Andrey Grodzovsky:
>>> On 2021-07-22 6:45 a.m., Jingwen Chen wrote:
>>>> On Wed Jul 21, 2021 at 12:53:51PM -0400, Andrey Grodzovsky wrote:
>>>>> On 2021-07-20 11:13 p.m., Jingwen Chen wrote:
>>>>>> [Why]
>>>>>> After embeded hw_fence to amdgpu_job, we need to add tdr support
>>>>>> for this feature.
>>>>>>
>>>>>> [How]
>>>>>> 1. Add a resubmit_flag for resubmit jobs.
>>>>>> 2. Clear job fence from RCU and force complete vm flush fences in
>>>>>>       pre_asic_reset
>>>>>> 3. skip dma_fence_get for resubmit jobs and add a dma_fence_put
>>>>>>       for guilty jobs.
>>>>>>
>>>>>> Signed-off-by: Jack Zhang <Jack.Zhang1@amd.com>
>>>>>> Signed-off-by: Jingwen Chen <Jingwen.Chen2@amd.com>
>>>>>> ---
>>>>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 12 +++++++++++-
>>>>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c  | 16 +++++++++++-----
>>>>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_job.c    |  4 +++-
>>>>>>     drivers/gpu/drm/scheduler/sched_main.c     |  1 +
>>>>>>     include/drm/gpu_scheduler.h                |  1 +
>>>>>>     5 files changed, 27 insertions(+), 7 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>>>> index 40461547701a..fe0237f72a09 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>>>> @@ -4382,7 +4382,7 @@ int amdgpu_device_mode1_reset(struct
>>>>>> amdgpu_device *adev)
>>>>>>     int amdgpu_device_pre_asic_reset(struct amdgpu_device *adev,
>>>>>>                      struct amdgpu_reset_context *reset_context)
>>>>>>     {
>>>>>> -    int i, r = 0;
>>>>>> +    int i, j, r = 0;
>>>>>>         struct amdgpu_job *job = NULL;
>>>>>>         bool need_full_reset =
>>>>>>             test_bit(AMDGPU_NEED_FULL_RESET, &reset_context->flags);
>>>>>> @@ -4406,6 +4406,16 @@ int
>>>>>> amdgpu_device_pre_asic_reset(struct amdgpu_device *adev,
>>>>>>             if (!ring || !ring->sched.thread)
>>>>>>                 continue;
>>>>>> +        /*clear job fence from fence drv to avoid force_completion
>>>>>> +         *leave NULL and vm flush fence in fence drv */
>>>>>> +        for (j = 0; j <= ring->fence_drv.num_fences_mask; j ++) {
>>>>>> +            struct dma_fence *old,**ptr;
>>>>>> +            ptr = &ring->fence_drv.fences[j];
>>>>>> +            old = rcu_dereference_protected(*ptr, 1);
>>>>>> +            if (old && test_bit(DMA_FENCE_FLAG_USER_BITS,
>>>>>> &old->flags))) {
>>>>>> +                RCU_INIT_POINTER(*ptr, NULL);
>>>>>> +            }
>>>>> Is this to avoid premature job free because of dma_fence_put inside
>>>>> amdgpu_fence_process ?
>>>>> I can't currently remember why but we probably want all the HW fences
>>>>> currently in the ring to
>>>>> be forced signaled - maybe better to test for DMA_FENCE_FLAG_USER_BITS
>>>>> inside amdgpu_fence_process
>>>>> and still do the signaling but not the dma_fence_put part
>>>>>
>>>>> Andrey
>>>> Hi Andrey,
>>>>
>>>> This is to avoid signaling the same fence twice. If we still do the
>>>> signaling, then the job in the pending list will be signaled first in
>>>> force_completion, and later be signaled in resubmit. This will go to
>>>> BUG() in amdgpu_fence_process.
>>>
>>> Oh, i see, how about just adding 'skip' flag to amdgpu_ring and setting
>>> it before calling
>>> amdgpu_fence_driver_force_completion and resetting it after, then inside
>>> amdgpu_fence_driver_force_completion
>>> you can just skip the signaling part with this flag for fences with
>>> DMA_FENCE_FLAG_USER_BITS set
>>> Less lines of code at least.
>> Still sounds quite a bit hacky.
>>
>> I would rather suggest to completely drop the approach with
>> amdgpu_fence_driver_force_completion(). I could never see why we would want
>> that in the first place.
>>
>> Regards,
>> Christian.
>>
> Hi Christian,
>
> I keep the amdgpu_fence_driver_force_completion here to make sure the vm
> flush fence is signaled and put.


Right, so we need to do force completion for the sake of all the fences
without parent jobs since there is code which wait directly on them.


> So the key question is whether the fence of ib test should be the first
> fence to be signaled after reset.


What do you mean by 'after reset' - at this point in the code there was
no ASIC reset yet, we just stopped the schedulers and detached the
HW fences from their parent jobs (sched_fence)


> If it should be, it means not only fences with DMA_FENCE_FLAG_USER_BITS
> but also vm flush fences should be removed from RCU fence array before
> ib_test.


Which ib_test is it, the one after ASIC reset ?

Andrey

>   Then we must do the force_completion here for vm flush
> fence, otherwise there will be a memory leak here as no one will signal
> and put it after we remove it from RCU.
> If not, then the first fence to signal could be vm flush fence. And I'm
> OK with drop amdgpu_fence_driver_force_completion here.
>
>
> Best Regards,
> JingWen Chen
>>> Andrey
>>>
>>>
>>>>>> +        }
>>>>>>             /* after all hw jobs are reset, hw fence is
>>>>>> meaningless, so force_completion */
>>>>>>             amdgpu_fence_driver_force_completion(ring);
>>>>>>         }
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>>>>> index eecf21d8ec33..815776c9a013 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>>>>> @@ -156,11 +156,17 @@ int amdgpu_fence_emit(struct
>>>>>> amdgpu_ring *ring, struct dma_fence **f, struct amd
>>>>>>             job->ring = ring;
>>>>>>         }
>>>>>> -    seq = ++ring->fence_drv.sync_seq;
>>>>>> -    dma_fence_init(fence, &amdgpu_fence_ops,
>>>>>> -               &ring->fence_drv.lock,
>>>>>> -               adev->fence_context + ring->idx,
>>>>>> -               seq);
>>>>>> +    if (job != NULL && job->base.resubmit_flag == 1) {
>>>>>> +        /* reinit seq for resubmitted jobs */
>>>>>> +        seq = ++ring->fence_drv.sync_seq;
>>>>>> +        fence->seqno = seq;
>>>>>> +    } else {
>>>>>> +        seq = ++ring->fence_drv.sync_seq;
>>>>> Seems like you could do the above line only once above if-else
>>>>> as it was
>>>>> before
>>>> Sure, I will modify this.
>>>>
>>>>
>>>> Best Regards,
>>>> JingWen Chen
>>>>>> +        dma_fence_init(fence, &amdgpu_fence_ops,
>>>>>> +                &ring->fence_drv.lock,
>>>>>> +                adev->fence_context + ring->idx,
>>>>>> +                seq);
>>>>>> +    }
>>>>>>         if (job != NULL) {
>>>>>>             /* mark this fence has a parent job */
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>>>> index 7c426e225b24..d6f848adc3f4 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>>>> @@ -241,6 +241,7 @@ static struct dma_fence
>>>>>> *amdgpu_job_run(struct drm_sched_job *sched_job)
>>>>>>             dma_fence_set_error(finished, -ECANCELED);/* skip
>>>>>> IB as well if VRAM lost */
>>>>>>         if (finished->error < 0) {
>>>>>> +        dma_fence_put(&job->hw_fence);
>>>>>>             DRM_INFO("Skip scheduling IBs!\n");
>>>>>>         } else {
>>>>>>             r = amdgpu_ib_schedule(ring, job->num_ibs, job->ibs, job,
>>>>>> @@ -249,7 +250,8 @@ static struct dma_fence
>>>>>> *amdgpu_job_run(struct drm_sched_job *sched_job)
>>>>>>                 DRM_ERROR("Error scheduling IBs (%d)\n", r);
>>>>>>         }
>>>>>> -    dma_fence_get(fence);
>>>>>> +    if (!job->base.resubmit_flag)
>>>>>> +        dma_fence_get(fence);
>>>>>>         amdgpu_job_free_resources(job);
>>>>>>         fence = r ? ERR_PTR(r) : fence;
>>>>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c
>>>>>> b/drivers/gpu/drm/scheduler/sched_main.c
>>>>>> index f4f474944169..5a36ab5aea2d 100644
>>>>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>>>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>>>>> @@ -544,6 +544,7 @@ void drm_sched_resubmit_jobs_ext(struct
>>>>>> drm_gpu_scheduler *sched, int max)
>>>>>> dma_fence_set_error(&s_fence->finished, -ECANCELED);
>>>>>>             dma_fence_put(s_job->s_fence->parent);
>>>>>> +        s_job->resubmit_flag = 1;
>>>>>>             fence = sched->ops->run_job(s_job);
>>>>>>             i++;
>>>>>> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
>>>>>> index 4ea8606d91fe..06c101af1f71 100644
>>>>>> --- a/include/drm/gpu_scheduler.h
>>>>>> +++ b/include/drm/gpu_scheduler.h
>>>>>> @@ -198,6 +198,7 @@ struct drm_sched_job {
>>>>>>         enum drm_sched_priority        s_priority;
>>>>>>         struct drm_sched_entity         *entity;
>>>>>>         struct dma_fence_cb        cb;
>>>>>> +    int resubmit_flag;
>>>>>>     };
>>>>>>     static inline bool drm_sched_invalidate_job(struct
>>>>>> drm_sched_job *s_job,
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=04%7C01%7Candrey.grodzovsky%40amd.com%7C9749ced7ce6645bd832408d94d305aaa%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637625692355112825%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=OhLndCUDlWcrhg%2FA0QQ%2FoONxdmQ46CT7P%2F8uvSTGnQ8%3D&amp;reserved=0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm: add tdr support for embeded hw_fence

[Jingwen Chen]

On Thu Jul 22, 2021 at 01:17:13PM -0400, Andrey Grodzovsky wrote:
> 
> On 2021-07-22 12:47 p.m., Jingwen Chen wrote:
> > On Thu Jul 22, 2021 at 06:24:28PM +0200, Christian König wrote:
> > > Am 22.07.21 um 16:45 schrieb Andrey Grodzovsky:
> > > > On 2021-07-22 6:45 a.m., Jingwen Chen wrote:
> > > > > On Wed Jul 21, 2021 at 12:53:51PM -0400, Andrey Grodzovsky wrote:
> > > > > > On 2021-07-20 11:13 p.m., Jingwen Chen wrote:
> > > > > > > [Why]
> > > > > > > After embeded hw_fence to amdgpu_job, we need to add tdr support
> > > > > > > for this feature.
> > > > > > > 
> > > > > > > [How]
> > > > > > > 1. Add a resubmit_flag for resubmit jobs.
> > > > > > > 2. Clear job fence from RCU and force complete vm flush fences in
> > > > > > >       pre_asic_reset
> > > > > > > 3. skip dma_fence_get for resubmit jobs and add a dma_fence_put
> > > > > > >       for guilty jobs.
> > > > > > > 
> > > > > > > Signed-off-by: Jack Zhang <Jack.Zhang1@amd.com>
> > > > > > > Signed-off-by: Jingwen Chen <Jingwen.Chen2@amd.com>
> > > > > > > ---
> > > > > > >     drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 12 +++++++++++-
> > > > > > >     drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c  | 16 +++++++++++-----
> > > > > > >     drivers/gpu/drm/amd/amdgpu/amdgpu_job.c    |  4 +++-
> > > > > > >     drivers/gpu/drm/scheduler/sched_main.c     |  1 +
> > > > > > >     include/drm/gpu_scheduler.h                |  1 +
> > > > > > >     5 files changed, 27 insertions(+), 7 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > > > > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > > > > > > index 40461547701a..fe0237f72a09 100644
> > > > > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > > > > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > > > > > > @@ -4382,7 +4382,7 @@ int amdgpu_device_mode1_reset(struct
> > > > > > > amdgpu_device *adev)
> > > > > > >     int amdgpu_device_pre_asic_reset(struct amdgpu_device *adev,
> > > > > > >                      struct amdgpu_reset_context *reset_context)
> > > > > > >     {
> > > > > > > -    int i, r = 0;
> > > > > > > +    int i, j, r = 0;
> > > > > > >         struct amdgpu_job *job = NULL;
> > > > > > >         bool need_full_reset =
> > > > > > >             test_bit(AMDGPU_NEED_FULL_RESET, &reset_context->flags);
> > > > > > > @@ -4406,6 +4406,16 @@ int
> > > > > > > amdgpu_device_pre_asic_reset(struct amdgpu_device *adev,
> > > > > > >             if (!ring || !ring->sched.thread)
> > > > > > >                 continue;
> > > > > > > +        /*clear job fence from fence drv to avoid force_completion
> > > > > > > +         *leave NULL and vm flush fence in fence drv */
> > > > > > > +        for (j = 0; j <= ring->fence_drv.num_fences_mask; j ++) {
> > > > > > > +            struct dma_fence *old,**ptr;
> > > > > > > +            ptr = &ring->fence_drv.fences[j];
> > > > > > > +            old = rcu_dereference_protected(*ptr, 1);
> > > > > > > +            if (old && test_bit(DMA_FENCE_FLAG_USER_BITS,
> > > > > > > &old->flags))) {
> > > > > > > +                RCU_INIT_POINTER(*ptr, NULL);
> > > > > > > +            }
> > > > > > Is this to avoid premature job free because of dma_fence_put inside
> > > > > > amdgpu_fence_process ?
> > > > > > I can't currently remember why but we probably want all the HW fences
> > > > > > currently in the ring to
> > > > > > be forced signaled - maybe better to test for DMA_FENCE_FLAG_USER_BITS
> > > > > > inside amdgpu_fence_process
> > > > > > and still do the signaling but not the dma_fence_put part
> > > > > > 
> > > > > > Andrey
> > > > > Hi Andrey,
> > > > > 
> > > > > This is to avoid signaling the same fence twice. If we still do the
> > > > > signaling, then the job in the pending list will be signaled first in
> > > > > force_completion, and later be signaled in resubmit. This will go to
> > > > > BUG() in amdgpu_fence_process.
> > > > 
> > > > Oh, i see, how about just adding 'skip' flag to amdgpu_ring and setting
> > > > it before calling
> > > > amdgpu_fence_driver_force_completion and resetting it after, then inside
> > > > amdgpu_fence_driver_force_completion
> > > > you can just skip the signaling part with this flag for fences with
> > > > DMA_FENCE_FLAG_USER_BITS set
> > > > Less lines of code at least.
> > > Still sounds quite a bit hacky.
> > > 
> > > I would rather suggest to completely drop the approach with
> > > amdgpu_fence_driver_force_completion(). I could never see why we would want
> > > that in the first place.
> > > 
> > > Regards,
> > > Christian.
> > > 
> > Hi Christian,
> > 
> > I keep the amdgpu_fence_driver_force_completion here to make sure the vm
> > flush fence is signaled and put.
> 
> 
> Right, so we need to do force completion for the sake of all the fences
> without parent jobs since there is code which wait directly on them.
> 
> 
> > So the key question is whether the fence of ib test should be the first
> > fence to be signaled after reset.
> 
> 
> What do you mean by 'after reset' - at this point in the code there was
> no ASIC reset yet, we just stopped the schedulers and detached the
> HW fences from their parent jobs (sched_fence)
I mean the ASIC reset. There will be a ib_test for each ring after ASIC
reset.
> 
> 
> > If it should be, it means not only fences with DMA_FENCE_FLAG_USER_BITS
> > but also vm flush fences should be removed from RCU fence array before
> > ib_test.
> 
> 
> Which ib_test is it, the one after ASIC reset ?
> 
Yes


Best Regards,
JingWen Chen
> Andrey
> 
> >   Then we must do the force_completion here for vm flush
> > fence, otherwise there will be a memory leak here as no one will signal
> > and put it after we remove it from RCU.
> > If not, then the first fence to signal could be vm flush fence. And I'm
> > OK with drop amdgpu_fence_driver_force_completion here.
> > 
> > 
> > Best Regards,
> > JingWen Chen
> > > > Andrey
> > > > 
> > > > 
> > > > > > > +        }
> > > > > > >             /* after all hw jobs are reset, hw fence is
> > > > > > > meaningless, so force_completion */
> > > > > > >             amdgpu_fence_driver_force_completion(ring);
> > > > > > >         }
> > > > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> > > > > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> > > > > > > index eecf21d8ec33..815776c9a013 100644
> > > > > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> > > > > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> > > > > > > @@ -156,11 +156,17 @@ int amdgpu_fence_emit(struct
> > > > > > > amdgpu_ring *ring, struct dma_fence **f, struct amd
> > > > > > >             job->ring = ring;
> > > > > > >         }
> > > > > > > -    seq = ++ring->fence_drv.sync_seq;
> > > > > > > -    dma_fence_init(fence, &amdgpu_fence_ops,
> > > > > > > -               &ring->fence_drv.lock,
> > > > > > > -               adev->fence_context + ring->idx,
> > > > > > > -               seq);
> > > > > > > +    if (job != NULL && job->base.resubmit_flag == 1) {
> > > > > > > +        /* reinit seq for resubmitted jobs */
> > > > > > > +        seq = ++ring->fence_drv.sync_seq;
> > > > > > > +        fence->seqno = seq;
> > > > > > > +    } else {
> > > > > > > +        seq = ++ring->fence_drv.sync_seq;
> > > > > > Seems like you could do the above line only once above if-else
> > > > > > as it was
> > > > > > before
> > > > > Sure, I will modify this.
> > > > > 
> > > > > 
> > > > > Best Regards,
> > > > > JingWen Chen
> > > > > > > +        dma_fence_init(fence, &amdgpu_fence_ops,
> > > > > > > +                &ring->fence_drv.lock,
> > > > > > > +                adev->fence_context + ring->idx,
> > > > > > > +                seq);
> > > > > > > +    }
> > > > > > >         if (job != NULL) {
> > > > > > >             /* mark this fence has a parent job */
> > > > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > > > > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > > > > > > index 7c426e225b24..d6f848adc3f4 100644
> > > > > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > > > > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > > > > > > @@ -241,6 +241,7 @@ static struct dma_fence
> > > > > > > *amdgpu_job_run(struct drm_sched_job *sched_job)
> > > > > > >             dma_fence_set_error(finished, -ECANCELED);/* skip
> > > > > > > IB as well if VRAM lost */
> > > > > > >         if (finished->error < 0) {
> > > > > > > +        dma_fence_put(&job->hw_fence);
> > > > > > >             DRM_INFO("Skip scheduling IBs!\n");
> > > > > > >         } else {
> > > > > > >             r = amdgpu_ib_schedule(ring, job->num_ibs, job->ibs, job,
> > > > > > > @@ -249,7 +250,8 @@ static struct dma_fence
> > > > > > > *amdgpu_job_run(struct drm_sched_job *sched_job)
> > > > > > >                 DRM_ERROR("Error scheduling IBs (%d)\n", r);
> > > > > > >         }
> > > > > > > -    dma_fence_get(fence);
> > > > > > > +    if (!job->base.resubmit_flag)
> > > > > > > +        dma_fence_get(fence);
> > > > > > >         amdgpu_job_free_resources(job);
> > > > > > >         fence = r ? ERR_PTR(r) : fence;
> > > > > > > diff --git a/drivers/gpu/drm/scheduler/sched_main.c
> > > > > > > b/drivers/gpu/drm/scheduler/sched_main.c
> > > > > > > index f4f474944169..5a36ab5aea2d 100644
> > > > > > > --- a/drivers/gpu/drm/scheduler/sched_main.c
> > > > > > > +++ b/drivers/gpu/drm/scheduler/sched_main.c
> > > > > > > @@ -544,6 +544,7 @@ void drm_sched_resubmit_jobs_ext(struct
> > > > > > > drm_gpu_scheduler *sched, int max)
> > > > > > > dma_fence_set_error(&s_fence->finished, -ECANCELED);
> > > > > > >             dma_fence_put(s_job->s_fence->parent);
> > > > > > > +        s_job->resubmit_flag = 1;
> > > > > > >             fence = sched->ops->run_job(s_job);
> > > > > > >             i++;
> > > > > > > diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> > > > > > > index 4ea8606d91fe..06c101af1f71 100644
> > > > > > > --- a/include/drm/gpu_scheduler.h
> > > > > > > +++ b/include/drm/gpu_scheduler.h
> > > > > > > @@ -198,6 +198,7 @@ struct drm_sched_job {
> > > > > > >         enum drm_sched_priority        s_priority;
> > > > > > >         struct drm_sched_entity         *entity;
> > > > > > >         struct dma_fence_cb        cb;
> > > > > > > +    int resubmit_flag;
> > > > > > >     };
> > > > > > >     static inline bool drm_sched_invalidate_job(struct
> > > > > > > drm_sched_job *s_job,
> > _______________________________________________
> > amd-gfx mailing list
> > amd-gfx@lists.freedesktop.org
> > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=04%7C01%7Candrey.grodzovsky%40amd.com%7C9749ced7ce6645bd832408d94d305aaa%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637625692355112825%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=OhLndCUDlWcrhg%2FA0QQ%2FoONxdmQ46CT7P%2F8uvSTGnQ8%3D&amp;reserved=0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm: add tdr support for embeded hw_fence

[Andrey Grodzovsky]

On 2021-07-22 1:27 p.m., Jingwen Chen wrote:
> On Thu Jul 22, 2021 at 01:17:13PM -0400, Andrey Grodzovsky wrote:
>> On 2021-07-22 12:47 p.m., Jingwen Chen wrote:
>>> On Thu Jul 22, 2021 at 06:24:28PM +0200, Christian König wrote:
>>>> Am 22.07.21 um 16:45 schrieb Andrey Grodzovsky:
>>>>> On 2021-07-22 6:45 a.m., Jingwen Chen wrote:
>>>>>> On Wed Jul 21, 2021 at 12:53:51PM -0400, Andrey Grodzovsky wrote:
>>>>>>> On 2021-07-20 11:13 p.m., Jingwen Chen wrote:
>>>>>>>> [Why]
>>>>>>>> After embeded hw_fence to amdgpu_job, we need to add tdr support
>>>>>>>> for this feature.
>>>>>>>>
>>>>>>>> [How]
>>>>>>>> 1. Add a resubmit_flag for resubmit jobs.
>>>>>>>> 2. Clear job fence from RCU and force complete vm flush fences in
>>>>>>>>        pre_asic_reset
>>>>>>>> 3. skip dma_fence_get for resubmit jobs and add a dma_fence_put
>>>>>>>>        for guilty jobs.
>>>>>>>>
>>>>>>>> Signed-off-by: Jack Zhang <Jack.Zhang1@amd.com>
>>>>>>>> Signed-off-by: Jingwen Chen <Jingwen.Chen2@amd.com>
>>>>>>>> ---
>>>>>>>>      drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 12 +++++++++++-
>>>>>>>>      drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c  | 16 +++++++++++-----
>>>>>>>>      drivers/gpu/drm/amd/amdgpu/amdgpu_job.c    |  4 +++-
>>>>>>>>      drivers/gpu/drm/scheduler/sched_main.c     |  1 +
>>>>>>>>      include/drm/gpu_scheduler.h                |  1 +
>>>>>>>>      5 files changed, 27 insertions(+), 7 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>>>>>> index 40461547701a..fe0237f72a09 100644
>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>>>>>> @@ -4382,7 +4382,7 @@ int amdgpu_device_mode1_reset(struct
>>>>>>>> amdgpu_device *adev)
>>>>>>>>      int amdgpu_device_pre_asic_reset(struct amdgpu_device *adev,
>>>>>>>>                       struct amdgpu_reset_context *reset_context)
>>>>>>>>      {
>>>>>>>> -    int i, r = 0;
>>>>>>>> +    int i, j, r = 0;
>>>>>>>>          struct amdgpu_job *job = NULL;
>>>>>>>>          bool need_full_reset =
>>>>>>>>              test_bit(AMDGPU_NEED_FULL_RESET, &reset_context->flags);
>>>>>>>> @@ -4406,6 +4406,16 @@ int
>>>>>>>> amdgpu_device_pre_asic_reset(struct amdgpu_device *adev,
>>>>>>>>              if (!ring || !ring->sched.thread)
>>>>>>>>                  continue;
>>>>>>>> +        /*clear job fence from fence drv to avoid force_completion
>>>>>>>> +         *leave NULL and vm flush fence in fence drv */
>>>>>>>> +        for (j = 0; j <= ring->fence_drv.num_fences_mask; j ++) {
>>>>>>>> +            struct dma_fence *old,**ptr;
>>>>>>>> +            ptr = &ring->fence_drv.fences[j];
>>>>>>>> +            old = rcu_dereference_protected(*ptr, 1);
>>>>>>>> +            if (old && test_bit(DMA_FENCE_FLAG_USER_BITS,
>>>>>>>> &old->flags))) {
>>>>>>>> +                RCU_INIT_POINTER(*ptr, NULL);
>>>>>>>> +            }
>>>>>>> Is this to avoid premature job free because of dma_fence_put inside
>>>>>>> amdgpu_fence_process ?
>>>>>>> I can't currently remember why but we probably want all the HW fences
>>>>>>> currently in the ring to
>>>>>>> be forced signaled - maybe better to test for DMA_FENCE_FLAG_USER_BITS
>>>>>>> inside amdgpu_fence_process
>>>>>>> and still do the signaling but not the dma_fence_put part
>>>>>>>
>>>>>>> Andrey
>>>>>> Hi Andrey,
>>>>>>
>>>>>> This is to avoid signaling the same fence twice. If we still do the
>>>>>> signaling, then the job in the pending list will be signaled first in
>>>>>> force_completion, and later be signaled in resubmit. This will go to
>>>>>> BUG() in amdgpu_fence_process.
>>>>> Oh, i see, how about just adding 'skip' flag to amdgpu_ring and setting
>>>>> it before calling
>>>>> amdgpu_fence_driver_force_completion and resetting it after, then inside
>>>>> amdgpu_fence_driver_force_completion
>>>>> you can just skip the signaling part with this flag for fences with
>>>>> DMA_FENCE_FLAG_USER_BITS set
>>>>> Less lines of code at least.
>>>> Still sounds quite a bit hacky.
>>>>
>>>> I would rather suggest to completely drop the approach with
>>>> amdgpu_fence_driver_force_completion(). I could never see why we would want
>>>> that in the first place.
>>>>
>>>> Regards,
>>>> Christian.
>>>>
>>> Hi Christian,
>>>
>>> I keep the amdgpu_fence_driver_force_completion here to make sure the vm
>>> flush fence is signaled and put.
>>
>> Right, so we need to do force completion for the sake of all the fences
>> without parent jobs since there is code which wait directly on them.
>>
>>
>>> So the key question is whether the fence of ib test should be the first
>>> fence to be signaled after reset.
>>
>> What do you mean by 'after reset' - at this point in the code there was
>> no ASIC reset yet, we just stopped the schedulers and detached the
>> HW fences from their parent jobs (sched_fence)
> I mean the ASIC reset. There will be a ib_test for each ring after ASIC
> reset.


Then why wouldn't they be the first ? They will emit new fences into the 
ring
which will be signaled right away because the ASIC went through reset 
and is not
hung anymore.  All the previous fences, including VM flush once from 
before the
reset will be already signaled by this time from 
amdgpu_fence_driver_force_completion.


>>
>>> If it should be, it means not only fences with DMA_FENCE_FLAG_USER_BITS
>>> but also vm flush fences should be removed from RCU fence array before
>>> ib_test.


As I mentioned above, not clear to me why VM fences should get special 
treatment here.

Andrey


>>
>> Which ib_test is it, the one after ASIC reset ?
>>
> Yes
>
>
> Best Regards,
> JingWen Chen
>> Andrey
>>
>>>    Then we must do the force_completion here for vm flush
>>> fence, otherwise there will be a memory leak here as no one will signal
>>> and put it after we remove it from RCU.
>>> If not, then the first fence to signal could be vm flush fence. And I'm
>>> OK with drop amdgpu_fence_driver_force_completion here.
>>>
>>>
>>> Best Regards,
>>> JingWen Chen
>>>>> Andrey
>>>>>
>>>>>
>>>>>>>> +        }
>>>>>>>>              /* after all hw jobs are reset, hw fence is
>>>>>>>> meaningless, so force_completion */
>>>>>>>>              amdgpu_fence_driver_force_completion(ring);
>>>>>>>>          }
>>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>>>>>>> index eecf21d8ec33..815776c9a013 100644
>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>>>>>>> @@ -156,11 +156,17 @@ int amdgpu_fence_emit(struct
>>>>>>>> amdgpu_ring *ring, struct dma_fence **f, struct amd
>>>>>>>>              job->ring = ring;
>>>>>>>>          }
>>>>>>>> -    seq = ++ring->fence_drv.sync_seq;
>>>>>>>> -    dma_fence_init(fence, &amdgpu_fence_ops,
>>>>>>>> -               &ring->fence_drv.lock,
>>>>>>>> -               adev->fence_context + ring->idx,
>>>>>>>> -               seq);
>>>>>>>> +    if (job != NULL && job->base.resubmit_flag == 1) {
>>>>>>>> +        /* reinit seq for resubmitted jobs */
>>>>>>>> +        seq = ++ring->fence_drv.sync_seq;
>>>>>>>> +        fence->seqno = seq;
>>>>>>>> +    } else {
>>>>>>>> +        seq = ++ring->fence_drv.sync_seq;
>>>>>>> Seems like you could do the above line only once above if-else
>>>>>>> as it was
>>>>>>> before
>>>>>> Sure, I will modify this.
>>>>>>
>>>>>>
>>>>>> Best Regards,
>>>>>> JingWen Chen
>>>>>>>> +        dma_fence_init(fence, &amdgpu_fence_ops,
>>>>>>>> +                &ring->fence_drv.lock,
>>>>>>>> +                adev->fence_context + ring->idx,
>>>>>>>> +                seq);
>>>>>>>> +    }
>>>>>>>>          if (job != NULL) {
>>>>>>>>              /* mark this fence has a parent job */
>>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>>>>>> index 7c426e225b24..d6f848adc3f4 100644
>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>>>>>> @@ -241,6 +241,7 @@ static struct dma_fence
>>>>>>>> *amdgpu_job_run(struct drm_sched_job *sched_job)
>>>>>>>>              dma_fence_set_error(finished, -ECANCELED);/* skip
>>>>>>>> IB as well if VRAM lost */
>>>>>>>>          if (finished->error < 0) {
>>>>>>>> +        dma_fence_put(&job->hw_fence);
>>>>>>>>              DRM_INFO("Skip scheduling IBs!\n");
>>>>>>>>          } else {
>>>>>>>>              r = amdgpu_ib_schedule(ring, job->num_ibs, job->ibs, job,
>>>>>>>> @@ -249,7 +250,8 @@ static struct dma_fence
>>>>>>>> *amdgpu_job_run(struct drm_sched_job *sched_job)
>>>>>>>>                  DRM_ERROR("Error scheduling IBs (%d)\n", r);
>>>>>>>>          }
>>>>>>>> -    dma_fence_get(fence);
>>>>>>>> +    if (!job->base.resubmit_flag)
>>>>>>>> +        dma_fence_get(fence);
>>>>>>>>          amdgpu_job_free_resources(job);
>>>>>>>>          fence = r ? ERR_PTR(r) : fence;
>>>>>>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c
>>>>>>>> b/drivers/gpu/drm/scheduler/sched_main.c
>>>>>>>> index f4f474944169..5a36ab5aea2d 100644
>>>>>>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>>>>>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>>>>>>> @@ -544,6 +544,7 @@ void drm_sched_resubmit_jobs_ext(struct
>>>>>>>> drm_gpu_scheduler *sched, int max)
>>>>>>>> dma_fence_set_error(&s_fence->finished, -ECANCELED);
>>>>>>>>              dma_fence_put(s_job->s_fence->parent);
>>>>>>>> +        s_job->resubmit_flag = 1;
>>>>>>>>              fence = sched->ops->run_job(s_job);
>>>>>>>>              i++;
>>>>>>>> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
>>>>>>>> index 4ea8606d91fe..06c101af1f71 100644
>>>>>>>> --- a/include/drm/gpu_scheduler.h
>>>>>>>> +++ b/include/drm/gpu_scheduler.h
>>>>>>>> @@ -198,6 +198,7 @@ struct drm_sched_job {
>>>>>>>>          enum drm_sched_priority        s_priority;
>>>>>>>>          struct drm_sched_entity         *entity;
>>>>>>>>          struct dma_fence_cb        cb;
>>>>>>>> +    int resubmit_flag;
>>>>>>>>      };
>>>>>>>>      static inline bool drm_sched_invalidate_job(struct
>>>>>>>> drm_sched_job *s_job,
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx@lists.freedesktop.org
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=04%7C01%7Candrey.grodzovsky%40amd.com%7C9749ced7ce6645bd832408d94d305aaa%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637625692355112825%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=OhLndCUDlWcrhg%2FA0QQ%2FoONxdmQ46CT7P%2F8uvSTGnQ8%3D&amp;reserved=0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm: add tdr support for embeded hw_fence

[Jingwen Chen]

On Thu Jul 22, 2021 at 01:50:09PM -0400, Andrey Grodzovsky wrote:
> 
> On 2021-07-22 1:27 p.m., Jingwen Chen wrote:
> > On Thu Jul 22, 2021 at 01:17:13PM -0400, Andrey Grodzovsky wrote:
> > > On 2021-07-22 12:47 p.m., Jingwen Chen wrote:
> > > > On Thu Jul 22, 2021 at 06:24:28PM +0200, Christian König wrote:
> > > > > Am 22.07.21 um 16:45 schrieb Andrey Grodzovsky:
> > > > > > On 2021-07-22 6:45 a.m., Jingwen Chen wrote:
> > > > > > > On Wed Jul 21, 2021 at 12:53:51PM -0400, Andrey Grodzovsky wrote:
> > > > > > > > On 2021-07-20 11:13 p.m., Jingwen Chen wrote:
> > > > > > > > > [Why]
> > > > > > > > > After embeded hw_fence to amdgpu_job, we need to add tdr support
> > > > > > > > > for this feature.
> > > > > > > > > 
> > > > > > > > > [How]
> > > > > > > > > 1. Add a resubmit_flag for resubmit jobs.
> > > > > > > > > 2. Clear job fence from RCU and force complete vm flush fences in
> > > > > > > > >        pre_asic_reset
> > > > > > > > > 3. skip dma_fence_get for resubmit jobs and add a dma_fence_put
> > > > > > > > >        for guilty jobs.
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Jack Zhang <Jack.Zhang1@amd.com>
> > > > > > > > > Signed-off-by: Jingwen Chen <Jingwen.Chen2@amd.com>
> > > > > > > > > ---
> > > > > > > > >      drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 12 +++++++++++-
> > > > > > > > >      drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c  | 16 +++++++++++-----
> > > > > > > > >      drivers/gpu/drm/amd/amdgpu/amdgpu_job.c    |  4 +++-
> > > > > > > > >      drivers/gpu/drm/scheduler/sched_main.c     |  1 +
> > > > > > > > >      include/drm/gpu_scheduler.h                |  1 +
> > > > > > > > >      5 files changed, 27 insertions(+), 7 deletions(-)
> > > > > > > > > 
> > > > > > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > > > > > > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > > > > > > > > index 40461547701a..fe0237f72a09 100644
> > > > > > > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > > > > > > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > > > > > > > > @@ -4382,7 +4382,7 @@ int amdgpu_device_mode1_reset(struct
> > > > > > > > > amdgpu_device *adev)
> > > > > > > > >      int amdgpu_device_pre_asic_reset(struct amdgpu_device *adev,
> > > > > > > > >                       struct amdgpu_reset_context *reset_context)
> > > > > > > > >      {
> > > > > > > > > -    int i, r = 0;
> > > > > > > > > +    int i, j, r = 0;
> > > > > > > > >          struct amdgpu_job *job = NULL;
> > > > > > > > >          bool need_full_reset =
> > > > > > > > >              test_bit(AMDGPU_NEED_FULL_RESET, &reset_context->flags);
> > > > > > > > > @@ -4406,6 +4406,16 @@ int
> > > > > > > > > amdgpu_device_pre_asic_reset(struct amdgpu_device *adev,
> > > > > > > > >              if (!ring || !ring->sched.thread)
> > > > > > > > >                  continue;
> > > > > > > > > +        /*clear job fence from fence drv to avoid force_completion
> > > > > > > > > +         *leave NULL and vm flush fence in fence drv */
> > > > > > > > > +        for (j = 0; j <= ring->fence_drv.num_fences_mask; j ++) {
> > > > > > > > > +            struct dma_fence *old,**ptr;
> > > > > > > > > +            ptr = &ring->fence_drv.fences[j];
> > > > > > > > > +            old = rcu_dereference_protected(*ptr, 1);
> > > > > > > > > +            if (old && test_bit(DMA_FENCE_FLAG_USER_BITS,
> > > > > > > > > &old->flags))) {
> > > > > > > > > +                RCU_INIT_POINTER(*ptr, NULL);
> > > > > > > > > +            }
> > > > > > > > Is this to avoid premature job free because of dma_fence_put inside
> > > > > > > > amdgpu_fence_process ?
> > > > > > > > I can't currently remember why but we probably want all the HW fences
> > > > > > > > currently in the ring to
> > > > > > > > be forced signaled - maybe better to test for DMA_FENCE_FLAG_USER_BITS
> > > > > > > > inside amdgpu_fence_process
> > > > > > > > and still do the signaling but not the dma_fence_put part
> > > > > > > > 
> > > > > > > > Andrey
> > > > > > > Hi Andrey,
> > > > > > > 
> > > > > > > This is to avoid signaling the same fence twice. If we still do the
> > > > > > > signaling, then the job in the pending list will be signaled first in
> > > > > > > force_completion, and later be signaled in resubmit. This will go to
> > > > > > > BUG() in amdgpu_fence_process.
> > > > > > Oh, i see, how about just adding 'skip' flag to amdgpu_ring and setting
> > > > > > it before calling
> > > > > > amdgpu_fence_driver_force_completion and resetting it after, then inside
> > > > > > amdgpu_fence_driver_force_completion
> > > > > > you can just skip the signaling part with this flag for fences with
> > > > > > DMA_FENCE_FLAG_USER_BITS set
> > > > > > Less lines of code at least.
> > > > > Still sounds quite a bit hacky.
> > > > > 
> > > > > I would rather suggest to completely drop the approach with
> > > > > amdgpu_fence_driver_force_completion(). I could never see why we would want
> > > > > that in the first place.
> > > > > 
> > > > > Regards,
> > > > > Christian.
> > > > > 
> > > > Hi Christian,
> > > > 
> > > > I keep the amdgpu_fence_driver_force_completion here to make sure the vm
> > > > flush fence is signaled and put.
> > > 
> > > Right, so we need to do force completion for the sake of all the fences
> > > without parent jobs since there is code which wait directly on them.
> > > 
> > > 
> > > > So the key question is whether the fence of ib test should be the first
> > > > fence to be signaled after reset.
> > > 
> > > What do you mean by 'after reset' - at this point in the code there was
> > > no ASIC reset yet, we just stopped the schedulers and detached the
> > > HW fences from their parent jobs (sched_fence)
> > I mean the ASIC reset. There will be a ib_test for each ring after ASIC
> > reset.
> 
> 
> Then why wouldn't they be the first ? They will emit new fences into the
> ring
> which will be signaled right away because the ASIC went through reset and is
> not
> hung anymore.  All the previous fences, including VM flush once from before
> the
> reset will be already signaled by this time from
> amdgpu_fence_driver_force_completion.
> 
Hi Andrey,

Sorry I didn't make it clear. I mean if we drop force_completion here,
and keep other code unchanged, then the ib_test wouldn't be the first to
be signaled.


Best Regards,
JingWen Chen
> 
> > > 
> > > > If it should be, it means not only fences with DMA_FENCE_FLAG_USER_BITS
> > > > but also vm flush fences should be removed from RCU fence array before
> > > > ib_test.
> 
> 
> As I mentioned above, not clear to me why VM fences should get special
> treatment here.
> 
> Andrey
> 
> 
> > > 
> > > Which ib_test is it, the one after ASIC reset ?
> > > 
> > Yes
> > 
> > 
> > Best Regards,
> > JingWen Chen
> > > Andrey
> > > 
> > > >    Then we must do the force_completion here for vm flush
> > > > fence, otherwise there will be a memory leak here as no one will signal
> > > > and put it after we remove it from RCU.
> > > > If not, then the first fence to signal could be vm flush fence. And I'm
> > > > OK with drop amdgpu_fence_driver_force_completion here.
> > > > 
> > > > 
> > > > Best Regards,
> > > > JingWen Chen
> > > > > > Andrey
> > > > > > 
> > > > > > 
> > > > > > > > > +        }
> > > > > > > > >              /* after all hw jobs are reset, hw fence is
> > > > > > > > > meaningless, so force_completion */
> > > > > > > > >              amdgpu_fence_driver_force_completion(ring);
> > > > > > > > >          }
> > > > > > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> > > > > > > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> > > > > > > > > index eecf21d8ec33..815776c9a013 100644
> > > > > > > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> > > > > > > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> > > > > > > > > @@ -156,11 +156,17 @@ int amdgpu_fence_emit(struct
> > > > > > > > > amdgpu_ring *ring, struct dma_fence **f, struct amd
> > > > > > > > >              job->ring = ring;
> > > > > > > > >          }
> > > > > > > > > -    seq = ++ring->fence_drv.sync_seq;
> > > > > > > > > -    dma_fence_init(fence, &amdgpu_fence_ops,
> > > > > > > > > -               &ring->fence_drv.lock,
> > > > > > > > > -               adev->fence_context + ring->idx,
> > > > > > > > > -               seq);
> > > > > > > > > +    if (job != NULL && job->base.resubmit_flag == 1) {
> > > > > > > > > +        /* reinit seq for resubmitted jobs */
> > > > > > > > > +        seq = ++ring->fence_drv.sync_seq;
> > > > > > > > > +        fence->seqno = seq;
> > > > > > > > > +    } else {
> > > > > > > > > +        seq = ++ring->fence_drv.sync_seq;
> > > > > > > > Seems like you could do the above line only once above if-else
> > > > > > > > as it was
> > > > > > > > before
> > > > > > > Sure, I will modify this.
> > > > > > > 
> > > > > > > 
> > > > > > > Best Regards,
> > > > > > > JingWen Chen
> > > > > > > > > +        dma_fence_init(fence, &amdgpu_fence_ops,
> > > > > > > > > +                &ring->fence_drv.lock,
> > > > > > > > > +                adev->fence_context + ring->idx,
> > > > > > > > > +                seq);
> > > > > > > > > +    }
> > > > > > > > >          if (job != NULL) {
> > > > > > > > >              /* mark this fence has a parent job */
> > > > > > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > > > > > > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > > > > > > > > index 7c426e225b24..d6f848adc3f4 100644
> > > > > > > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > > > > > > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > > > > > > > > @@ -241,6 +241,7 @@ static struct dma_fence
> > > > > > > > > *amdgpu_job_run(struct drm_sched_job *sched_job)
> > > > > > > > >              dma_fence_set_error(finished, -ECANCELED);/* skip
> > > > > > > > > IB as well if VRAM lost */
> > > > > > > > >          if (finished->error < 0) {
> > > > > > > > > +        dma_fence_put(&job->hw_fence);
> > > > > > > > >              DRM_INFO("Skip scheduling IBs!\n");
> > > > > > > > >          } else {
> > > > > > > > >              r = amdgpu_ib_schedule(ring, job->num_ibs, job->ibs, job,
> > > > > > > > > @@ -249,7 +250,8 @@ static struct dma_fence
> > > > > > > > > *amdgpu_job_run(struct drm_sched_job *sched_job)
> > > > > > > > >                  DRM_ERROR("Error scheduling IBs (%d)\n", r);
> > > > > > > > >          }
> > > > > > > > > -    dma_fence_get(fence);
> > > > > > > > > +    if (!job->base.resubmit_flag)
> > > > > > > > > +        dma_fence_get(fence);
> > > > > > > > >          amdgpu_job_free_resources(job);
> > > > > > > > >          fence = r ? ERR_PTR(r) : fence;
> > > > > > > > > diff --git a/drivers/gpu/drm/scheduler/sched_main.c
> > > > > > > > > b/drivers/gpu/drm/scheduler/sched_main.c
> > > > > > > > > index f4f474944169..5a36ab5aea2d 100644
> > > > > > > > > --- a/drivers/gpu/drm/scheduler/sched_main.c
> > > > > > > > > +++ b/drivers/gpu/drm/scheduler/sched_main.c
> > > > > > > > > @@ -544,6 +544,7 @@ void drm_sched_resubmit_jobs_ext(struct
> > > > > > > > > drm_gpu_scheduler *sched, int max)
> > > > > > > > > dma_fence_set_error(&s_fence->finished, -ECANCELED);
> > > > > > > > >              dma_fence_put(s_job->s_fence->parent);
> > > > > > > > > +        s_job->resubmit_flag = 1;
> > > > > > > > >              fence = sched->ops->run_job(s_job);
> > > > > > > > >              i++;
> > > > > > > > > diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> > > > > > > > > index 4ea8606d91fe..06c101af1f71 100644
> > > > > > > > > --- a/include/drm/gpu_scheduler.h
> > > > > > > > > +++ b/include/drm/gpu_scheduler.h
> > > > > > > > > @@ -198,6 +198,7 @@ struct drm_sched_job {
> > > > > > > > >          enum drm_sched_priority        s_priority;
> > > > > > > > >          struct drm_sched_entity         *entity;
> > > > > > > > >          struct dma_fence_cb        cb;
> > > > > > > > > +    int resubmit_flag;
> > > > > > > > >      };
> > > > > > > > >      static inline bool drm_sched_invalidate_job(struct
> > > > > > > > > drm_sched_job *s_job,
> > > > _______________________________________________
> > > > amd-gfx mailing list
> > > > amd-gfx@lists.freedesktop.org
> > > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=04%7C01%7Candrey.grodzovsky%40amd.com%7C9749ced7ce6645bd832408d94d305aaa%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637625692355112825%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=OhLndCUDlWcrhg%2FA0QQ%2FoONxdmQ46CT7P%2F8uvSTGnQ8%3D&amp;reserved=0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm: add tdr support for embeded hw_fence

[Andrey Grodzovsky]

On 2021-07-22 8:20 p.m., Jingwen Chen wrote:
> On Thu Jul 22, 2021 at 01:50:09PM -0400, Andrey Grodzovsky wrote:
>> On 2021-07-22 1:27 p.m., Jingwen Chen wrote:
>>> On Thu Jul 22, 2021 at 01:17:13PM -0400, Andrey Grodzovsky wrote:
>>>> On 2021-07-22 12:47 p.m., Jingwen Chen wrote:
>>>>> On Thu Jul 22, 2021 at 06:24:28PM +0200, Christian König wrote:
>>>>>> Am 22.07.21 um 16:45 schrieb Andrey Grodzovsky:
>>>>>>> On 2021-07-22 6:45 a.m., Jingwen Chen wrote:
>>>>>>>> On Wed Jul 21, 2021 at 12:53:51PM -0400, Andrey Grodzovsky wrote:
>>>>>>>>> On 2021-07-20 11:13 p.m., Jingwen Chen wrote:
>>>>>>>>>> [Why]
>>>>>>>>>> After embeded hw_fence to amdgpu_job, we need to add tdr support
>>>>>>>>>> for this feature.
>>>>>>>>>>
>>>>>>>>>> [How]
>>>>>>>>>> 1. Add a resubmit_flag for resubmit jobs.
>>>>>>>>>> 2. Clear job fence from RCU and force complete vm flush fences in
>>>>>>>>>>         pre_asic_reset
>>>>>>>>>> 3. skip dma_fence_get for resubmit jobs and add a dma_fence_put
>>>>>>>>>>         for guilty jobs.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Jack Zhang <Jack.Zhang1@amd.com>
>>>>>>>>>> Signed-off-by: Jingwen Chen <Jingwen.Chen2@amd.com>
>>>>>>>>>> ---
>>>>>>>>>>       drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 12 +++++++++++-
>>>>>>>>>>       drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c  | 16 +++++++++++-----
>>>>>>>>>>       drivers/gpu/drm/amd/amdgpu/amdgpu_job.c    |  4 +++-
>>>>>>>>>>       drivers/gpu/drm/scheduler/sched_main.c     |  1 +
>>>>>>>>>>       include/drm/gpu_scheduler.h                |  1 +
>>>>>>>>>>       5 files changed, 27 insertions(+), 7 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>>>>>>>> index 40461547701a..fe0237f72a09 100644
>>>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>>>>>>>> @@ -4382,7 +4382,7 @@ int amdgpu_device_mode1_reset(struct
>>>>>>>>>> amdgpu_device *adev)
>>>>>>>>>>       int amdgpu_device_pre_asic_reset(struct amdgpu_device *adev,
>>>>>>>>>>                        struct amdgpu_reset_context *reset_context)
>>>>>>>>>>       {
>>>>>>>>>> -    int i, r = 0;
>>>>>>>>>> +    int i, j, r = 0;
>>>>>>>>>>           struct amdgpu_job *job = NULL;
>>>>>>>>>>           bool need_full_reset =
>>>>>>>>>>               test_bit(AMDGPU_NEED_FULL_RESET, &reset_context->flags);
>>>>>>>>>> @@ -4406,6 +4406,16 @@ int
>>>>>>>>>> amdgpu_device_pre_asic_reset(struct amdgpu_device *adev,
>>>>>>>>>>               if (!ring || !ring->sched.thread)
>>>>>>>>>>                   continue;
>>>>>>>>>> +        /*clear job fence from fence drv to avoid force_completion
>>>>>>>>>> +         *leave NULL and vm flush fence in fence drv */
>>>>>>>>>> +        for (j = 0; j <= ring->fence_drv.num_fences_mask; j ++) {
>>>>>>>>>> +            struct dma_fence *old,**ptr;
>>>>>>>>>> +            ptr = &ring->fence_drv.fences[j];
>>>>>>>>>> +            old = rcu_dereference_protected(*ptr, 1);
>>>>>>>>>> +            if (old && test_bit(DMA_FENCE_FLAG_USER_BITS,
>>>>>>>>>> &old->flags))) {
>>>>>>>>>> +                RCU_INIT_POINTER(*ptr, NULL);
>>>>>>>>>> +            }
>>>>>>>>> Is this to avoid premature job free because of dma_fence_put inside
>>>>>>>>> amdgpu_fence_process ?
>>>>>>>>> I can't currently remember why but we probably want all the HW fences
>>>>>>>>> currently in the ring to
>>>>>>>>> be forced signaled - maybe better to test for DMA_FENCE_FLAG_USER_BITS
>>>>>>>>> inside amdgpu_fence_process
>>>>>>>>> and still do the signaling but not the dma_fence_put part
>>>>>>>>>
>>>>>>>>> Andrey
>>>>>>>> Hi Andrey,
>>>>>>>>
>>>>>>>> This is to avoid signaling the same fence twice. If we still do the
>>>>>>>> signaling, then the job in the pending list will be signaled first in
>>>>>>>> force_completion, and later be signaled in resubmit. This will go to
>>>>>>>> BUG() in amdgpu_fence_process.
>>>>>>> Oh, i see, how about just adding 'skip' flag to amdgpu_ring and setting
>>>>>>> it before calling
>>>>>>> amdgpu_fence_driver_force_completion and resetting it after, then inside
>>>>>>> amdgpu_fence_driver_force_completion
>>>>>>> you can just skip the signaling part with this flag for fences with
>>>>>>> DMA_FENCE_FLAG_USER_BITS set
>>>>>>> Less lines of code at least.
>>>>>> Still sounds quite a bit hacky.
>>>>>>
>>>>>> I would rather suggest to completely drop the approach with
>>>>>> amdgpu_fence_driver_force_completion(). I could never see why we would want
>>>>>> that in the first place.
>>>>>>
>>>>>> Regards,
>>>>>> Christian.
>>>>>>
>>>>> Hi Christian,
>>>>>
>>>>> I keep the amdgpu_fence_driver_force_completion here to make sure the vm
>>>>> flush fence is signaled and put.
>>>> Right, so we need to do force completion for the sake of all the fences
>>>> without parent jobs since there is code which wait directly on them.
>>>>
>>>>
>>>>> So the key question is whether the fence of ib test should be the first
>>>>> fence to be signaled after reset.
>>>> What do you mean by 'after reset' - at this point in the code there was
>>>> no ASIC reset yet, we just stopped the schedulers and detached the
>>>> HW fences from their parent jobs (sched_fence)
>>> I mean the ASIC reset. There will be a ib_test for each ring after ASIC
>>> reset.
>>
>> Then why wouldn't they be the first ? They will emit new fences into the
>> ring
>> which will be signaled right away because the ASIC went through reset and is
>> not
>> hung anymore.  All the previous fences, including VM flush once from before
>> the
>> reset will be already signaled by this time from
>> amdgpu_fence_driver_force_completion.
>>
> Hi Andrey,
>
> Sorry I didn't make it clear. I mean if we drop force_completion here,
> and keep other code unchanged, then the ib_test wouldn't be the first to
> be signaled.


At least in my opinion the order is not important of who will be 
signaled first. I do think it's important
to force signal all the old fences before HW reset for 2 reasons: 1st - 
it's conceptually wrong to leave them
unsignaled and let them be signaled post ASIC reset because  it's not 
them who actually completed whatever
they were doing but rather ASIC reset allowed later fences (with higher 
sync_seq) to signal (e.g. ring test) and
those earlier remaining fences (e.g. VM_FLUSH)  just were signaled as 
part of this. 2nd - Possible deadlock where
ring->fence_drv.fences is full with fences before ASIC reset because we 
don't force signal and so don't empty it and then
after reset we call amdgpu_fence_emit as part of ib_test but it 
de-references old yet unsignaled  fence and does
dma_fence_wait to make sure it completed which it didn't and will not 
because EOP ISR will be called only after actual
fence emission which is blocked by the wait.

Andrey


>
>
> Best Regards,
> JingWen Chen
>>>>> If it should be, it means not only fences with DMA_FENCE_FLAG_USER_BITS
>>>>> but also vm flush fences should be removed from RCU fence array before
>>>>> ib_test.
>>
>> As I mentioned above, not clear to me why VM fences should get special
>> treatment here.
>>
>> Andrey
>>
>>
>>>> Which ib_test is it, the one after ASIC reset ?
>>>>
>>> Yes
>>>
>>>
>>> Best Regards,
>>> JingWen Chen
>>>> Andrey
>>>>
>>>>>     Then we must do the force_completion here for vm flush
>>>>> fence, otherwise there will be a memory leak here as no one will signal
>>>>> and put it after we remove it from RCU.
>>>>> If not, then the first fence to signal could be vm flush fence. And I'm
>>>>> OK with drop amdgpu_fence_driver_force_completion here.
>>>>>
>>>>>
>>>>> Best Regards,
>>>>> JingWen Chen
>>>>>>> Andrey
>>>>>>>
>>>>>>>
>>>>>>>>>> +        }
>>>>>>>>>>               /* after all hw jobs are reset, hw fence is
>>>>>>>>>> meaningless, so force_completion */
>>>>>>>>>>               amdgpu_fence_driver_force_completion(ring);
>>>>>>>>>>           }
>>>>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>>>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>>>>>>>>> index eecf21d8ec33..815776c9a013 100644
>>>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>>>>>>>>> @@ -156,11 +156,17 @@ int amdgpu_fence_emit(struct
>>>>>>>>>> amdgpu_ring *ring, struct dma_fence **f, struct amd
>>>>>>>>>>               job->ring = ring;
>>>>>>>>>>           }
>>>>>>>>>> -    seq = ++ring->fence_drv.sync_seq;
>>>>>>>>>> -    dma_fence_init(fence, &amdgpu_fence_ops,
>>>>>>>>>> -               &ring->fence_drv.lock,
>>>>>>>>>> -               adev->fence_context + ring->idx,
>>>>>>>>>> -               seq);
>>>>>>>>>> +    if (job != NULL && job->base.resubmit_flag == 1) {
>>>>>>>>>> +        /* reinit seq for resubmitted jobs */
>>>>>>>>>> +        seq = ++ring->fence_drv.sync_seq;
>>>>>>>>>> +        fence->seqno = seq;
>>>>>>>>>> +    } else {
>>>>>>>>>> +        seq = ++ring->fence_drv.sync_seq;
>>>>>>>>> Seems like you could do the above line only once above if-else
>>>>>>>>> as it was
>>>>>>>>> before
>>>>>>>> Sure, I will modify this.
>>>>>>>>
>>>>>>>>
>>>>>>>> Best Regards,
>>>>>>>> JingWen Chen
>>>>>>>>>> +        dma_fence_init(fence, &amdgpu_fence_ops,
>>>>>>>>>> +                &ring->fence_drv.lock,
>>>>>>>>>> +                adev->fence_context + ring->idx,
>>>>>>>>>> +                seq);
>>>>>>>>>> +    }
>>>>>>>>>>           if (job != NULL) {
>>>>>>>>>>               /* mark this fence has a parent job */
>>>>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>>>>>>>> index 7c426e225b24..d6f848adc3f4 100644
>>>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>>>>>>>> @@ -241,6 +241,7 @@ static struct dma_fence
>>>>>>>>>> *amdgpu_job_run(struct drm_sched_job *sched_job)
>>>>>>>>>>               dma_fence_set_error(finished, -ECANCELED);/* skip
>>>>>>>>>> IB as well if VRAM lost */
>>>>>>>>>>           if (finished->error < 0) {
>>>>>>>>>> +        dma_fence_put(&job->hw_fence);
>>>>>>>>>>               DRM_INFO("Skip scheduling IBs!\n");
>>>>>>>>>>           } else {
>>>>>>>>>>               r = amdgpu_ib_schedule(ring, job->num_ibs, job->ibs, job,
>>>>>>>>>> @@ -249,7 +250,8 @@ static struct dma_fence
>>>>>>>>>> *amdgpu_job_run(struct drm_sched_job *sched_job)
>>>>>>>>>>                   DRM_ERROR("Error scheduling IBs (%d)\n", r);
>>>>>>>>>>           }
>>>>>>>>>> -    dma_fence_get(fence);
>>>>>>>>>> +    if (!job->base.resubmit_flag)
>>>>>>>>>> +        dma_fence_get(fence);
>>>>>>>>>>           amdgpu_job_free_resources(job);
>>>>>>>>>>           fence = r ? ERR_PTR(r) : fence;
>>>>>>>>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c
>>>>>>>>>> b/drivers/gpu/drm/scheduler/sched_main.c
>>>>>>>>>> index f4f474944169..5a36ab5aea2d 100644
>>>>>>>>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>>>>>>>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>>>>>>>>> @@ -544,6 +544,7 @@ void drm_sched_resubmit_jobs_ext(struct
>>>>>>>>>> drm_gpu_scheduler *sched, int max)
>>>>>>>>>> dma_fence_set_error(&s_fence->finished, -ECANCELED);
>>>>>>>>>>               dma_fence_put(s_job->s_fence->parent);
>>>>>>>>>> +        s_job->resubmit_flag = 1;
>>>>>>>>>>               fence = sched->ops->run_job(s_job);
>>>>>>>>>>               i++;
>>>>>>>>>> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
>>>>>>>>>> index 4ea8606d91fe..06c101af1f71 100644
>>>>>>>>>> --- a/include/drm/gpu_scheduler.h
>>>>>>>>>> +++ b/include/drm/gpu_scheduler.h
>>>>>>>>>> @@ -198,6 +198,7 @@ struct drm_sched_job {
>>>>>>>>>>           enum drm_sched_priority        s_priority;
>>>>>>>>>>           struct drm_sched_entity         *entity;
>>>>>>>>>>           struct dma_fence_cb        cb;
>>>>>>>>>> +    int resubmit_flag;
>>>>>>>>>>       };
>>>>>>>>>>       static inline bool drm_sched_invalidate_job(struct
>>>>>>>>>> drm_sched_job *s_job,
>>>>> _______________________________________________
>>>>> amd-gfx mailing list
>>>>> amd-gfx@lists.freedesktop.org
>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=04%7C01%7Candrey.grodzovsky%40amd.com%7C9749ced7ce6645bd832408d94d305aaa%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637625692355112825%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=OhLndCUDlWcrhg%2FA0QQ%2FoONxdmQ46CT7P%2F8uvSTGnQ8%3D&amp;reserved=0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm: add tdr support for embeded hw_fence

[Jingwen Chen]

On Fri Jul 23, 2021 at 12:06:32AM -0400, Andrey Grodzovsky wrote:
> 
> On 2021-07-22 8:20 p.m., Jingwen Chen wrote:
> > On Thu Jul 22, 2021 at 01:50:09PM -0400, Andrey Grodzovsky wrote:
> > > On 2021-07-22 1:27 p.m., Jingwen Chen wrote:
> > > > On Thu Jul 22, 2021 at 01:17:13PM -0400, Andrey Grodzovsky wrote:
> > > > > On 2021-07-22 12:47 p.m., Jingwen Chen wrote:
> > > > > > On Thu Jul 22, 2021 at 06:24:28PM +0200, Christian König wrote:
> > > > > > > Am 22.07.21 um 16:45 schrieb Andrey Grodzovsky:
> > > > > > > > On 2021-07-22 6:45 a.m., Jingwen Chen wrote:
> > > > > > > > > On Wed Jul 21, 2021 at 12:53:51PM -0400, Andrey Grodzovsky wrote:
> > > > > > > > > > On 2021-07-20 11:13 p.m., Jingwen Chen wrote:
> > > > > > > > > > > [Why]
> > > > > > > > > > > After embeded hw_fence to amdgpu_job, we need to add tdr support
> > > > > > > > > > > for this feature.
> > > > > > > > > > > 
> > > > > > > > > > > [How]
> > > > > > > > > > > 1. Add a resubmit_flag for resubmit jobs.
> > > > > > > > > > > 2. Clear job fence from RCU and force complete vm flush fences in
> > > > > > > > > > >         pre_asic_reset
> > > > > > > > > > > 3. skip dma_fence_get for resubmit jobs and add a dma_fence_put
> > > > > > > > > > >         for guilty jobs.
> > > > > > > > > > > 
> > > > > > > > > > > Signed-off-by: Jack Zhang <Jack.Zhang1@amd.com>
> > > > > > > > > > > Signed-off-by: Jingwen Chen <Jingwen.Chen2@amd.com>
> > > > > > > > > > > ---
> > > > > > > > > > >       drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 12 +++++++++++-
> > > > > > > > > > >       drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c  | 16 +++++++++++-----
> > > > > > > > > > >       drivers/gpu/drm/amd/amdgpu/amdgpu_job.c    |  4 +++-
> > > > > > > > > > >       drivers/gpu/drm/scheduler/sched_main.c     |  1 +
> > > > > > > > > > >       include/drm/gpu_scheduler.h                |  1 +
> > > > > > > > > > >       5 files changed, 27 insertions(+), 7 deletions(-)
> > > > > > > > > > > 
> > > > > > > > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > > > > > > > > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > > > > > > > > > > index 40461547701a..fe0237f72a09 100644
> > > > > > > > > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > > > > > > > > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > > > > > > > > > > @@ -4382,7 +4382,7 @@ int amdgpu_device_mode1_reset(struct
> > > > > > > > > > > amdgpu_device *adev)
> > > > > > > > > > >       int amdgpu_device_pre_asic_reset(struct amdgpu_device *adev,
> > > > > > > > > > >                        struct amdgpu_reset_context *reset_context)
> > > > > > > > > > >       {
> > > > > > > > > > > -    int i, r = 0;
> > > > > > > > > > > +    int i, j, r = 0;
> > > > > > > > > > >           struct amdgpu_job *job = NULL;
> > > > > > > > > > >           bool need_full_reset =
> > > > > > > > > > >               test_bit(AMDGPU_NEED_FULL_RESET, &reset_context->flags);
> > > > > > > > > > > @@ -4406,6 +4406,16 @@ int
> > > > > > > > > > > amdgpu_device_pre_asic_reset(struct amdgpu_device *adev,
> > > > > > > > > > >               if (!ring || !ring->sched.thread)
> > > > > > > > > > >                   continue;
> > > > > > > > > > > +        /*clear job fence from fence drv to avoid force_completion
> > > > > > > > > > > +         *leave NULL and vm flush fence in fence drv */
> > > > > > > > > > > +        for (j = 0; j <= ring->fence_drv.num_fences_mask; j ++) {
> > > > > > > > > > > +            struct dma_fence *old,**ptr;
> > > > > > > > > > > +            ptr = &ring->fence_drv.fences[j];
> > > > > > > > > > > +            old = rcu_dereference_protected(*ptr, 1);
> > > > > > > > > > > +            if (old && test_bit(DMA_FENCE_FLAG_USER_BITS,
> > > > > > > > > > > &old->flags))) {
> > > > > > > > > > > +                RCU_INIT_POINTER(*ptr, NULL);
> > > > > > > > > > > +            }
> > > > > > > > > > Is this to avoid premature job free because of dma_fence_put inside
> > > > > > > > > > amdgpu_fence_process ?
> > > > > > > > > > I can't currently remember why but we probably want all the HW fences
> > > > > > > > > > currently in the ring to
> > > > > > > > > > be forced signaled - maybe better to test for DMA_FENCE_FLAG_USER_BITS
> > > > > > > > > > inside amdgpu_fence_process
> > > > > > > > > > and still do the signaling but not the dma_fence_put part
> > > > > > > > > > 
> > > > > > > > > > Andrey
> > > > > > > > > Hi Andrey,
> > > > > > > > > 
> > > > > > > > > This is to avoid signaling the same fence twice. If we still do the
> > > > > > > > > signaling, then the job in the pending list will be signaled first in
> > > > > > > > > force_completion, and later be signaled in resubmit. This will go to
> > > > > > > > > BUG() in amdgpu_fence_process.
> > > > > > > > Oh, i see, how about just adding 'skip' flag to amdgpu_ring and setting
> > > > > > > > it before calling
> > > > > > > > amdgpu_fence_driver_force_completion and resetting it after, then inside
> > > > > > > > amdgpu_fence_driver_force_completion
> > > > > > > > you can just skip the signaling part with this flag for fences with
> > > > > > > > DMA_FENCE_FLAG_USER_BITS set
> > > > > > > > Less lines of code at least.
> > > > > > > Still sounds quite a bit hacky.
> > > > > > > 
> > > > > > > I would rather suggest to completely drop the approach with
> > > > > > > amdgpu_fence_driver_force_completion(). I could never see why we would want
> > > > > > > that in the first place.
> > > > > > > 
> > > > > > > Regards,
> > > > > > > Christian.
> > > > > > > 
> > > > > > Hi Christian,
> > > > > > 
> > > > > > I keep the amdgpu_fence_driver_force_completion here to make sure the vm
> > > > > > flush fence is signaled and put.
> > > > > Right, so we need to do force completion for the sake of all the fences
> > > > > without parent jobs since there is code which wait directly on them.
> > > > > 
> > > > > 
> > > > > > So the key question is whether the fence of ib test should be the first
> > > > > > fence to be signaled after reset.
> > > > > What do you mean by 'after reset' - at this point in the code there was
> > > > > no ASIC reset yet, we just stopped the schedulers and detached the
> > > > > HW fences from their parent jobs (sched_fence)
> > > > I mean the ASIC reset. There will be a ib_test for each ring after ASIC
> > > > reset.
> > > 
> > > Then why wouldn't they be the first ? They will emit new fences into the
> > > ring
> > > which will be signaled right away because the ASIC went through reset and is
> > > not
> > > hung anymore.  All the previous fences, including VM flush once from before
> > > the
> > > reset will be already signaled by this time from
> > > amdgpu_fence_driver_force_completion.
> > > 
> > Hi Andrey,
> > 
> > Sorry I didn't make it clear. I mean if we drop force_completion here,
> > and keep other code unchanged, then the ib_test wouldn't be the first to
> > be signaled.
> 
> 
> At least in my opinion the order is not important of who will be signaled
> first. I do think it's important
> to force signal all the old fences before HW reset for 2 reasons: 1st - it's
> conceptually wrong to leave them
> unsignaled and let them be signaled post ASIC reset because  it's not them
> who actually completed whatever
> they were doing but rather ASIC reset allowed later fences (with higher
> sync_seq) to signal (e.g. ring test) and
> those earlier remaining fences (e.g. VM_FLUSH)  just were signaled as part
> of this. 2nd - Possible deadlock where
> ring->fence_drv.fences is full with fences before ASIC reset because we
> don't force signal and so don't empty it and then
> after reset we call amdgpu_fence_emit as part of ib_test but it
> de-references old yet unsignaled  fence and does
> dma_fence_wait to make sure it completed which it didn't and will not
> because EOP ISR will be called only after actual
> fence emission which is blocked by the wait.
> 
> Andrey
Hi Andrey

Yes, I forget to talk about the deadlock previously. Actually I have
encountered a deadlock during debug already which is exactly the same
reason you mentioned.

I totally agree with not dropping force_completion.
> 
> 
> > 
> > 
> > Best Regards,
> > JingWen Chen
> > > > > > If it should be, it means not only fences with DMA_FENCE_FLAG_USER_BITS
> > > > > > but also vm flush fences should be removed from RCU fence array before
> > > > > > ib_test.
> > > 
> > > As I mentioned above, not clear to me why VM fences should get special
> > > treatment here.
> > > 
> > > Andrey
> > > 
> > > 
> > > > > Which ib_test is it, the one after ASIC reset ?
> > > > > 
> > > > Yes
> > > > 
> > > > 
> > > > Best Regards,
> > > > JingWen Chen
> > > > > Andrey
> > > > > 
> > > > > >     Then we must do the force_completion here for vm flush
> > > > > > fence, otherwise there will be a memory leak here as no one will signal
> > > > > > and put it after we remove it from RCU.
> > > > > > If not, then the first fence to signal could be vm flush fence. And I'm
> > > > > > OK with drop amdgpu_fence_driver_force_completion here.
> > > > > > 
> > > > > > 
> > > > > > Best Regards,
> > > > > > JingWen Chen
> > > > > > > > Andrey
> > > > > > > > 
> > > > > > > > 
> > > > > > > > > > > +        }
> > > > > > > > > > >               /* after all hw jobs are reset, hw fence is
> > > > > > > > > > > meaningless, so force_completion */
> > > > > > > > > > >               amdgpu_fence_driver_force_completion(ring);
> > > > > > > > > > >           }
> > > > > > > > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> > > > > > > > > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> > > > > > > > > > > index eecf21d8ec33..815776c9a013 100644
> > > > > > > > > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> > > > > > > > > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> > > > > > > > > > > @@ -156,11 +156,17 @@ int amdgpu_fence_emit(struct
> > > > > > > > > > > amdgpu_ring *ring, struct dma_fence **f, struct amd
> > > > > > > > > > >               job->ring = ring;
> > > > > > > > > > >           }
> > > > > > > > > > > -    seq = ++ring->fence_drv.sync_seq;
> > > > > > > > > > > -    dma_fence_init(fence, &amdgpu_fence_ops,
> > > > > > > > > > > -               &ring->fence_drv.lock,
> > > > > > > > > > > -               adev->fence_context + ring->idx,
> > > > > > > > > > > -               seq);
> > > > > > > > > > > +    if (job != NULL && job->base.resubmit_flag == 1) {
> > > > > > > > > > > +        /* reinit seq for resubmitted jobs */
> > > > > > > > > > > +        seq = ++ring->fence_drv.sync_seq;
> > > > > > > > > > > +        fence->seqno = seq;
> > > > > > > > > > > +    } else {
> > > > > > > > > > > +        seq = ++ring->fence_drv.sync_seq;
> > > > > > > > > > Seems like you could do the above line only once above if-else
> > > > > > > > > > as it was
> > > > > > > > > > before
> > > > > > > > > Sure, I will modify this.
> > > > > > > > > 
> > > > > > > > > 
I will modify this part and upload a v2 patch. Can you help add a ACK or
review by?

Best Regards,
JingWen Chen
> > > > > > > > > Best Regards,
> > > > > > > > > JingWen Chen
> > > > > > > > > > > +        dma_fence_init(fence, &amdgpu_fence_ops,
> > > > > > > > > > > +                &ring->fence_drv.lock,
> > > > > > > > > > > +                adev->fence_context + ring->idx,
> > > > > > > > > > > +                seq);
> > > > > > > > > > > +    }
> > > > > > > > > > >           if (job != NULL) {
> > > > > > > > > > >               /* mark this fence has a parent job */
> > > > > > > > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > > > > > > > > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > > > > > > > > > > index 7c426e225b24..d6f848adc3f4 100644
> > > > > > > > > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > > > > > > > > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > > > > > > > > > > @@ -241,6 +241,7 @@ static struct dma_fence
> > > > > > > > > > > *amdgpu_job_run(struct drm_sched_job *sched_job)
> > > > > > > > > > >               dma_fence_set_error(finished, -ECANCELED);/* skip
> > > > > > > > > > > IB as well if VRAM lost */
> > > > > > > > > > >           if (finished->error < 0) {
> > > > > > > > > > > +        dma_fence_put(&job->hw_fence);
> > > > > > > > > > >               DRM_INFO("Skip scheduling IBs!\n");
> > > > > > > > > > >           } else {
> > > > > > > > > > >               r = amdgpu_ib_schedule(ring, job->num_ibs, job->ibs, job,
> > > > > > > > > > > @@ -249,7 +250,8 @@ static struct dma_fence
> > > > > > > > > > > *amdgpu_job_run(struct drm_sched_job *sched_job)
> > > > > > > > > > >                   DRM_ERROR("Error scheduling IBs (%d)\n", r);
> > > > > > > > > > >           }
> > > > > > > > > > > -    dma_fence_get(fence);
> > > > > > > > > > > +    if (!job->base.resubmit_flag)
> > > > > > > > > > > +        dma_fence_get(fence);
> > > > > > > > > > >           amdgpu_job_free_resources(job);
> > > > > > > > > > >           fence = r ? ERR_PTR(r) : fence;
> > > > > > > > > > > diff --git a/drivers/gpu/drm/scheduler/sched_main.c
> > > > > > > > > > > b/drivers/gpu/drm/scheduler/sched_main.c
> > > > > > > > > > > index f4f474944169..5a36ab5aea2d 100644
> > > > > > > > > > > --- a/drivers/gpu/drm/scheduler/sched_main.c
> > > > > > > > > > > +++ b/drivers/gpu/drm/scheduler/sched_main.c
> > > > > > > > > > > @@ -544,6 +544,7 @@ void drm_sched_resubmit_jobs_ext(struct
> > > > > > > > > > > drm_gpu_scheduler *sched, int max)
> > > > > > > > > > > dma_fence_set_error(&s_fence->finished, -ECANCELED);
> > > > > > > > > > >               dma_fence_put(s_job->s_fence->parent);
> > > > > > > > > > > +        s_job->resubmit_flag = 1;
> > > > > > > > > > >               fence = sched->ops->run_job(s_job);
> > > > > > > > > > >               i++;
> > > > > > > > > > > diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> > > > > > > > > > > index 4ea8606d91fe..06c101af1f71 100644
> > > > > > > > > > > --- a/include/drm/gpu_scheduler.h
> > > > > > > > > > > +++ b/include/drm/gpu_scheduler.h
> > > > > > > > > > > @@ -198,6 +198,7 @@ struct drm_sched_job {
> > > > > > > > > > >           enum drm_sched_priority        s_priority;
> > > > > > > > > > >           struct drm_sched_entity         *entity;
> > > > > > > > > > >           struct dma_fence_cb        cb;
> > > > > > > > > > > +    int resubmit_flag;
> > > > > > > > > > >       };
> > > > > > > > > > >       static inline bool drm_sched_invalidate_job(struct
> > > > > > > > > > > drm_sched_job *s_job,
> > > > > > _______________________________________________
> > > > > > amd-gfx mailing list
> > > > > > amd-gfx@lists.freedesktop.org
> > > > > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=04%7C01%7Candrey.grodzovsky%40amd.com%7C9749ced7ce6645bd832408d94d305aaa%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637625692355112825%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=OhLndCUDlWcrhg%2FA0QQ%2FoONxdmQ46CT7P%2F8uvSTGnQ8%3D&amp;reserved=0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm: add tdr support for embeded hw_fence

[Christian König]

Am 22.07.21 um 18:47 schrieb Jingwen Chen:
> On Thu Jul 22, 2021 at 06:24:28PM +0200, Christian König wrote:
>> Am 22.07.21 um 16:45 schrieb Andrey Grodzovsky:
>>> On 2021-07-22 6:45 a.m., Jingwen Chen wrote:
>>>> On Wed Jul 21, 2021 at 12:53:51PM -0400, Andrey Grodzovsky wrote:
>>>>> On 2021-07-20 11:13 p.m., Jingwen Chen wrote:
>>>>>> [Why]
>>>>>> After embeded hw_fence to amdgpu_job, we need to add tdr support
>>>>>> for this feature.
>>>>>>
>>>>>> [How]
>>>>>> 1. Add a resubmit_flag for resubmit jobs.
>>>>>> 2. Clear job fence from RCU and force complete vm flush fences in
>>>>>>       pre_asic_reset
>>>>>> 3. skip dma_fence_get for resubmit jobs and add a dma_fence_put
>>>>>>       for guilty jobs.
>>>>>>
>>>>>> Signed-off-by: Jack Zhang <Jack.Zhang1@amd.com>
>>>>>> Signed-off-by: Jingwen Chen <Jingwen.Chen2@amd.com>
>>>>>> ---
>>>>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 12 +++++++++++-
>>>>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c  | 16 +++++++++++-----
>>>>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_job.c    |  4 +++-
>>>>>>     drivers/gpu/drm/scheduler/sched_main.c     |  1 +
>>>>>>     include/drm/gpu_scheduler.h                |  1 +
>>>>>>     5 files changed, 27 insertions(+), 7 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>>>> index 40461547701a..fe0237f72a09 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>>>> @@ -4382,7 +4382,7 @@ int amdgpu_device_mode1_reset(struct
>>>>>> amdgpu_device *adev)
>>>>>>     int amdgpu_device_pre_asic_reset(struct amdgpu_device *adev,
>>>>>>                      struct amdgpu_reset_context *reset_context)
>>>>>>     {
>>>>>> -    int i, r = 0;
>>>>>> +    int i, j, r = 0;
>>>>>>         struct amdgpu_job *job = NULL;
>>>>>>         bool need_full_reset =
>>>>>>             test_bit(AMDGPU_NEED_FULL_RESET, &reset_context->flags);
>>>>>> @@ -4406,6 +4406,16 @@ int
>>>>>> amdgpu_device_pre_asic_reset(struct amdgpu_device *adev,
>>>>>>             if (!ring || !ring->sched.thread)
>>>>>>                 continue;
>>>>>> +        /*clear job fence from fence drv to avoid force_completion
>>>>>> +         *leave NULL and vm flush fence in fence drv */
>>>>>> +        for (j = 0; j <= ring->fence_drv.num_fences_mask; j ++) {
>>>>>> +            struct dma_fence *old,**ptr;
>>>>>> +            ptr = &ring->fence_drv.fences[j];
>>>>>> +            old = rcu_dereference_protected(*ptr, 1);
>>>>>> +            if (old && test_bit(DMA_FENCE_FLAG_USER_BITS,
>>>>>> &old->flags))) {
>>>>>> +                RCU_INIT_POINTER(*ptr, NULL);
>>>>>> +            }
>>>>> Is this to avoid premature job free because of dma_fence_put inside
>>>>> amdgpu_fence_process ?
>>>>> I can't currently remember why but we probably want all the HW fences
>>>>> currently in the ring to
>>>>> be forced signaled - maybe better to test for DMA_FENCE_FLAG_USER_BITS
>>>>> inside amdgpu_fence_process
>>>>> and still do the signaling but not the dma_fence_put part
>>>>>
>>>>> Andrey
>>>> Hi Andrey,
>>>>
>>>> This is to avoid signaling the same fence twice. If we still do the
>>>> signaling, then the job in the pending list will be signaled first in
>>>> force_completion, and later be signaled in resubmit. This will go to
>>>> BUG() in amdgpu_fence_process.
>>>
>>> Oh, i see, how about just adding 'skip' flag to amdgpu_ring and setting
>>> it before calling
>>> amdgpu_fence_driver_force_completion and resetting it after, then inside
>>> amdgpu_fence_driver_force_completion
>>> you can just skip the signaling part with this flag for fences with
>>> DMA_FENCE_FLAG_USER_BITS set
>>> Less lines of code at least.
>> Still sounds quite a bit hacky.
>>
>> I would rather suggest to completely drop the approach with
>> amdgpu_fence_driver_force_completion(). I could never see why we would want
>> that in the first place.
>>
>> Regards,
>> Christian.
>>
> Hi Christian,
>
> I keep the amdgpu_fence_driver_force_completion here to make sure the vm
> flush fence is signaled and put.
> So the key question is whether the fence of ib test should be the first
> fence to be signaled after reset.
> If it should be, it means not only fences with DMA_FENCE_FLAG_USER_BITS
> but also vm flush fences should be removed from RCU fence array before
> ib_test. Then we must do the force_completion here for vm flush
> fence, otherwise there will be a memory leak here as no one will signal
> and put it after we remove it from RCU.
> If not, then the first fence to signal could be vm flush fence. And I'm
> OK with drop amdgpu_fence_driver_force_completion here.

The key problem is that amdgpu_fence_driver_force_completion() goes over 
the RCU array and signals everything there.

What we should do instead is to go over the jobs and cleanup the ones we 
don't want to re-submit and keep the rest.

And please drop any usage of DMA_FENCE_FLAG_USER_BITS. That is not 
something which should be abused for reset handling.

Regards,
Christian.

>
>
> Best Regards,
> JingWen Chen
>>> Andrey
>>>
>>>
>>>>>> +        }
>>>>>>             /* after all hw jobs are reset, hw fence is
>>>>>> meaningless, so force_completion */
>>>>>>             amdgpu_fence_driver_force_completion(ring);
>>>>>>         }
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>>>>> index eecf21d8ec33..815776c9a013 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>>>>> @@ -156,11 +156,17 @@ int amdgpu_fence_emit(struct
>>>>>> amdgpu_ring *ring, struct dma_fence **f, struct amd
>>>>>>             job->ring = ring;
>>>>>>         }
>>>>>> -    seq = ++ring->fence_drv.sync_seq;
>>>>>> -    dma_fence_init(fence, &amdgpu_fence_ops,
>>>>>> -               &ring->fence_drv.lock,
>>>>>> -               adev->fence_context + ring->idx,
>>>>>> -               seq);
>>>>>> +    if (job != NULL && job->base.resubmit_flag == 1) {
>>>>>> +        /* reinit seq for resubmitted jobs */
>>>>>> +        seq = ++ring->fence_drv.sync_seq;
>>>>>> +        fence->seqno = seq;
>>>>>> +    } else {
>>>>>> +        seq = ++ring->fence_drv.sync_seq;
>>>>> Seems like you could do the above line only once above if-else
>>>>> as it was
>>>>> before
>>>> Sure, I will modify this.
>>>>
>>>>
>>>> Best Regards,
>>>> JingWen Chen
>>>>>> +        dma_fence_init(fence, &amdgpu_fence_ops,
>>>>>> +                &ring->fence_drv.lock,
>>>>>> +                adev->fence_context + ring->idx,
>>>>>> +                seq);
>>>>>> +    }
>>>>>>         if (job != NULL) {
>>>>>>             /* mark this fence has a parent job */
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>>>> index 7c426e225b24..d6f848adc3f4 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>>>> @@ -241,6 +241,7 @@ static struct dma_fence
>>>>>> *amdgpu_job_run(struct drm_sched_job *sched_job)
>>>>>>             dma_fence_set_error(finished, -ECANCELED);/* skip
>>>>>> IB as well if VRAM lost */
>>>>>>         if (finished->error < 0) {
>>>>>> +        dma_fence_put(&job->hw_fence);
>>>>>>             DRM_INFO("Skip scheduling IBs!\n");
>>>>>>         } else {
>>>>>>             r = amdgpu_ib_schedule(ring, job->num_ibs, job->ibs, job,
>>>>>> @@ -249,7 +250,8 @@ static struct dma_fence
>>>>>> *amdgpu_job_run(struct drm_sched_job *sched_job)
>>>>>>                 DRM_ERROR("Error scheduling IBs (%d)\n", r);
>>>>>>         }
>>>>>> -    dma_fence_get(fence);
>>>>>> +    if (!job->base.resubmit_flag)
>>>>>> +        dma_fence_get(fence);
>>>>>>         amdgpu_job_free_resources(job);
>>>>>>         fence = r ? ERR_PTR(r) : fence;
>>>>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c
>>>>>> b/drivers/gpu/drm/scheduler/sched_main.c
>>>>>> index f4f474944169..5a36ab5aea2d 100644
>>>>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>>>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>>>>> @@ -544,6 +544,7 @@ void drm_sched_resubmit_jobs_ext(struct
>>>>>> drm_gpu_scheduler *sched, int max)
>>>>>> dma_fence_set_error(&s_fence->finished, -ECANCELED);
>>>>>>             dma_fence_put(s_job->s_fence->parent);
>>>>>> +        s_job->resubmit_flag = 1;
>>>>>>             fence = sched->ops->run_job(s_job);
>>>>>>             i++;
>>>>>> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
>>>>>> index 4ea8606d91fe..06c101af1f71 100644
>>>>>> --- a/include/drm/gpu_scheduler.h
>>>>>> +++ b/include/drm/gpu_scheduler.h
>>>>>> @@ -198,6 +198,7 @@ struct drm_sched_job {
>>>>>>         enum drm_sched_priority        s_priority;
>>>>>>         struct drm_sched_entity         *entity;
>>>>>>         struct dma_fence_cb        cb;
>>>>>> +    int resubmit_flag;
>>>>>>     };
>>>>>>     static inline bool drm_sched_invalidate_job(struct
>>>>>> drm_sched_job *s_job,

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm: add tdr support for embeded hw_fence

[Jingwen Chen]

On Fri Jul 23, 2021 at 08:33:02AM +0200, Christian König wrote:
> Am 22.07.21 um 18:47 schrieb Jingwen Chen:
> > On Thu Jul 22, 2021 at 06:24:28PM +0200, Christian König wrote:
> > > Am 22.07.21 um 16:45 schrieb Andrey Grodzovsky:
> > > > On 2021-07-22 6:45 a.m., Jingwen Chen wrote:
> > > > > On Wed Jul 21, 2021 at 12:53:51PM -0400, Andrey Grodzovsky wrote:
> > > > > > On 2021-07-20 11:13 p.m., Jingwen Chen wrote:
> > > > > > > [Why]
> > > > > > > After embeded hw_fence to amdgpu_job, we need to add tdr support
> > > > > > > for this feature.
> > > > > > > 
> > > > > > > [How]
> > > > > > > 1. Add a resubmit_flag for resubmit jobs.
> > > > > > > 2. Clear job fence from RCU and force complete vm flush fences in
> > > > > > >       pre_asic_reset
> > > > > > > 3. skip dma_fence_get for resubmit jobs and add a dma_fence_put
> > > > > > >       for guilty jobs.
> > > > > > > 
> > > > > > > Signed-off-by: Jack Zhang <Jack.Zhang1@amd.com>
> > > > > > > Signed-off-by: Jingwen Chen <Jingwen.Chen2@amd.com>
> > > > > > > ---
> > > > > > >     drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 12 +++++++++++-
> > > > > > >     drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c  | 16 +++++++++++-----
> > > > > > >     drivers/gpu/drm/amd/amdgpu/amdgpu_job.c    |  4 +++-
> > > > > > >     drivers/gpu/drm/scheduler/sched_main.c     |  1 +
> > > > > > >     include/drm/gpu_scheduler.h                |  1 +
> > > > > > >     5 files changed, 27 insertions(+), 7 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > > > > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > > > > > > index 40461547701a..fe0237f72a09 100644
> > > > > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > > > > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > > > > > > @@ -4382,7 +4382,7 @@ int amdgpu_device_mode1_reset(struct
> > > > > > > amdgpu_device *adev)
> > > > > > >     int amdgpu_device_pre_asic_reset(struct amdgpu_device *adev,
> > > > > > >                      struct amdgpu_reset_context *reset_context)
> > > > > > >     {
> > > > > > > -    int i, r = 0;
> > > > > > > +    int i, j, r = 0;
> > > > > > >         struct amdgpu_job *job = NULL;
> > > > > > >         bool need_full_reset =
> > > > > > >             test_bit(AMDGPU_NEED_FULL_RESET, &reset_context->flags);
> > > > > > > @@ -4406,6 +4406,16 @@ int
> > > > > > > amdgpu_device_pre_asic_reset(struct amdgpu_device *adev,
> > > > > > >             if (!ring || !ring->sched.thread)
> > > > > > >                 continue;
> > > > > > > +        /*clear job fence from fence drv to avoid force_completion
> > > > > > > +         *leave NULL and vm flush fence in fence drv */
> > > > > > > +        for (j = 0; j <= ring->fence_drv.num_fences_mask; j ++) {
> > > > > > > +            struct dma_fence *old,**ptr;
> > > > > > > +            ptr = &ring->fence_drv.fences[j];
> > > > > > > +            old = rcu_dereference_protected(*ptr, 1);
> > > > > > > +            if (old && test_bit(DMA_FENCE_FLAG_USER_BITS,
> > > > > > > &old->flags))) {
> > > > > > > +                RCU_INIT_POINTER(*ptr, NULL);
> > > > > > > +            }
> > > > > > Is this to avoid premature job free because of dma_fence_put inside
> > > > > > amdgpu_fence_process ?
> > > > > > I can't currently remember why but we probably want all the HW fences
> > > > > > currently in the ring to
> > > > > > be forced signaled - maybe better to test for DMA_FENCE_FLAG_USER_BITS
> > > > > > inside amdgpu_fence_process
> > > > > > and still do the signaling but not the dma_fence_put part
> > > > > > 
> > > > > > Andrey
> > > > > Hi Andrey,
> > > > > 
> > > > > This is to avoid signaling the same fence twice. If we still do the
> > > > > signaling, then the job in the pending list will be signaled first in
> > > > > force_completion, and later be signaled in resubmit. This will go to
> > > > > BUG() in amdgpu_fence_process.
> > > > 
> > > > Oh, i see, how about just adding 'skip' flag to amdgpu_ring and setting
> > > > it before calling
> > > > amdgpu_fence_driver_force_completion and resetting it after, then inside
> > > > amdgpu_fence_driver_force_completion
> > > > you can just skip the signaling part with this flag for fences with
> > > > DMA_FENCE_FLAG_USER_BITS set
> > > > Less lines of code at least.
> > > Still sounds quite a bit hacky.
> > > 
> > > I would rather suggest to completely drop the approach with
> > > amdgpu_fence_driver_force_completion(). I could never see why we would want
> > > that in the first place.
> > > 
> > > Regards,
> > > Christian.
> > > 
> > Hi Christian,
> > 
> > I keep the amdgpu_fence_driver_force_completion here to make sure the vm
> > flush fence is signaled and put.
> > So the key question is whether the fence of ib test should be the first
> > fence to be signaled after reset.
> > If it should be, it means not only fences with DMA_FENCE_FLAG_USER_BITS
> > but also vm flush fences should be removed from RCU fence array before
> > ib_test. Then we must do the force_completion here for vm flush
> > fence, otherwise there will be a memory leak here as no one will signal
> > and put it after we remove it from RCU.
> > If not, then the first fence to signal could be vm flush fence. And I'm
> > OK with drop amdgpu_fence_driver_force_completion here.
> 
> The key problem is that amdgpu_fence_driver_force_completion() goes over the
> RCU array and signals everything there.
> 
> What we should do instead is to go over the jobs and cleanup the ones we
> don't want to re-submit and keep the rest.
> 
Hi Christian,

The thing is vm flush fence has no job passed to amdgpu_fence_emit, so
go through the jobs cannot help find the vm flush fence. And keep the
rest fences in the RCU array will lead to signaling them in the ib_test
right after ASIC reset. While they will be signaled again during
resubmission. What I'm doning here is just trying to cleanup the fences
without a parent job and make sure the rest fences won't be signaled
twice.

> And please drop any usage of DMA_FENCE_FLAG_USER_BITS. That is not something
> which should be abused for reset handling.
> 
The DMA_FENCE_FLAG_USER_BITS here is to mark this fence has a parent
job. If this is not a proper usage here, do you have any suggestions
about how to identify whether the fence has a parent job?

Best Regards,
JingWen Chen
> Regards,
> Christian.
> 
> > 
> > 
> > Best Regards,
> > JingWen Chen
> > > > Andrey
> > > > 
> > > > 
> > > > > > > +        }
> > > > > > >             /* after all hw jobs are reset, hw fence is
> > > > > > > meaningless, so force_completion */
> > > > > > >             amdgpu_fence_driver_force_completion(ring);
> > > > > > >         }
> > > > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> > > > > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> > > > > > > index eecf21d8ec33..815776c9a013 100644
> > > > > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> > > > > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> > > > > > > @@ -156,11 +156,17 @@ int amdgpu_fence_emit(struct
> > > > > > > amdgpu_ring *ring, struct dma_fence **f, struct amd
> > > > > > >             job->ring = ring;
> > > > > > >         }
> > > > > > > -    seq = ++ring->fence_drv.sync_seq;
> > > > > > > -    dma_fence_init(fence, &amdgpu_fence_ops,
> > > > > > > -               &ring->fence_drv.lock,
> > > > > > > -               adev->fence_context + ring->idx,
> > > > > > > -               seq);
> > > > > > > +    if (job != NULL && job->base.resubmit_flag == 1) {
> > > > > > > +        /* reinit seq for resubmitted jobs */
> > > > > > > +        seq = ++ring->fence_drv.sync_seq;
> > > > > > > +        fence->seqno = seq;
> > > > > > > +    } else {
> > > > > > > +        seq = ++ring->fence_drv.sync_seq;
> > > > > > Seems like you could do the above line only once above if-else
> > > > > > as it was
> > > > > > before
> > > > > Sure, I will modify this.
> > > > > 
> > > > > 
> > > > > Best Regards,
> > > > > JingWen Chen
> > > > > > > +        dma_fence_init(fence, &amdgpu_fence_ops,
> > > > > > > +                &ring->fence_drv.lock,
> > > > > > > +                adev->fence_context + ring->idx,
> > > > > > > +                seq);
> > > > > > > +    }
> > > > > > >         if (job != NULL) {
> > > > > > >             /* mark this fence has a parent job */
> > > > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > > > > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > > > > > > index 7c426e225b24..d6f848adc3f4 100644
> > > > > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > > > > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > > > > > > @@ -241,6 +241,7 @@ static struct dma_fence
> > > > > > > *amdgpu_job_run(struct drm_sched_job *sched_job)
> > > > > > >             dma_fence_set_error(finished, -ECANCELED);/* skip
> > > > > > > IB as well if VRAM lost */
> > > > > > >         if (finished->error < 0) {
> > > > > > > +        dma_fence_put(&job->hw_fence);
> > > > > > >             DRM_INFO("Skip scheduling IBs!\n");
> > > > > > >         } else {
> > > > > > >             r = amdgpu_ib_schedule(ring, job->num_ibs, job->ibs, job,
> > > > > > > @@ -249,7 +250,8 @@ static struct dma_fence
> > > > > > > *amdgpu_job_run(struct drm_sched_job *sched_job)
> > > > > > >                 DRM_ERROR("Error scheduling IBs (%d)\n", r);
> > > > > > >         }
> > > > > > > -    dma_fence_get(fence);
> > > > > > > +    if (!job->base.resubmit_flag)
> > > > > > > +        dma_fence_get(fence);
> > > > > > >         amdgpu_job_free_resources(job);
> > > > > > >         fence = r ? ERR_PTR(r) : fence;
> > > > > > > diff --git a/drivers/gpu/drm/scheduler/sched_main.c
> > > > > > > b/drivers/gpu/drm/scheduler/sched_main.c
> > > > > > > index f4f474944169..5a36ab5aea2d 100644
> > > > > > > --- a/drivers/gpu/drm/scheduler/sched_main.c
> > > > > > > +++ b/drivers/gpu/drm/scheduler/sched_main.c
> > > > > > > @@ -544,6 +544,7 @@ void drm_sched_resubmit_jobs_ext(struct
> > > > > > > drm_gpu_scheduler *sched, int max)
> > > > > > > dma_fence_set_error(&s_fence->finished, -ECANCELED);
> > > > > > >             dma_fence_put(s_job->s_fence->parent);
> > > > > > > +        s_job->resubmit_flag = 1;
> > > > > > >             fence = sched->ops->run_job(s_job);
> > > > > > >             i++;
> > > > > > > diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> > > > > > > index 4ea8606d91fe..06c101af1f71 100644
> > > > > > > --- a/include/drm/gpu_scheduler.h
> > > > > > > +++ b/include/drm/gpu_scheduler.h
> > > > > > > @@ -198,6 +198,7 @@ struct drm_sched_job {
> > > > > > >         enum drm_sched_priority        s_priority;
> > > > > > >         struct drm_sched_entity         *entity;
> > > > > > >         struct dma_fence_cb        cb;
> > > > > > > +    int resubmit_flag;
> > > > > > >     };
> > > > > > >     static inline bool drm_sched_invalidate_job(struct
> > > > > > > drm_sched_job *s_job,
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH 2/2] drm: add tdr support for embeded hw_fence

[Liu, Monk]

[AMD Official Use Only]

Hi Christian 

How about this way:

#define AMDGPU_DMA_FENCE_FLAG_INSIDE_JOB  DMA_FENCE_FLAG_USER_BITS

And we can use  AMDGPU_DMA_FENCE_FLAG_INSIDE_JOB instead of DMA_FENCE_FLAG_USER_BITS everywhere

Thanks 

------------------------------------------
Monk Liu | Cloud-GPU Core team
------------------------------------------

-----Original Message-----
From: Jingwen Chen <Jingwen.Chen2@amd.com> 
Sent: Friday, July 23, 2021 3:07 PM
To: Christian König <ckoenig.leichtzumerken@gmail.com>; Grodzovsky, Andrey <Andrey.Grodzovsky@amd.com>; amd-gfx@lists.freedesktop.org
Cc: Chen, Horace <Horace.Chen@amd.com>; Liu, Monk <Monk.Liu@amd.com>; Zhang, Jack (Jian) <Jack.Zhang1@amd.com>
Subject: Re: [PATCH 2/2] drm: add tdr support for embeded hw_fence

On Fri Jul 23, 2021 at 08:33:02AM +0200, Christian König wrote:
> Am 22.07.21 um 18:47 schrieb Jingwen Chen:
> > On Thu Jul 22, 2021 at 06:24:28PM +0200, Christian König wrote:
> > > Am 22.07.21 um 16:45 schrieb Andrey Grodzovsky:
> > > > On 2021-07-22 6:45 a.m., Jingwen Chen wrote:
> > > > > On Wed Jul 21, 2021 at 12:53:51PM -0400, Andrey Grodzovsky wrote:
> > > > > > On 2021-07-20 11:13 p.m., Jingwen Chen wrote:
> > > > > > > [Why]
> > > > > > > After embeded hw_fence to amdgpu_job, we need to add tdr 
> > > > > > > support for this feature.
> > > > > > > 
> > > > > > > [How]
> > > > > > > 1. Add a resubmit_flag for resubmit jobs.
> > > > > > > 2. Clear job fence from RCU and force complete vm flush 
> > > > > > > fences in
> > > > > > >       pre_asic_reset
> > > > > > > 3. skip dma_fence_get for resubmit jobs and add a 
> > > > > > > dma_fence_put
> > > > > > >       for guilty jobs.
> > > > > > > 
> > > > > > > Signed-off-by: Jack Zhang <Jack.Zhang1@amd.com>
> > > > > > > Signed-off-by: Jingwen Chen <Jingwen.Chen2@amd.com>
> > > > > > > ---
> > > > > > >     drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 12 
> > > > > > > +++++++++++-
> > > > > > >     drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c  | 16 
> > > > > > > +++++++++++-----
> > > > > > >     drivers/gpu/drm/amd/amdgpu/amdgpu_job.c    |  4 +++-
> > > > > > >     drivers/gpu/drm/scheduler/sched_main.c     |  1 +
> > > > > > >     include/drm/gpu_scheduler.h                |  1 +
> > > > > > >     5 files changed, 27 insertions(+), 7 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > > > > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > > > > > > index 40461547701a..fe0237f72a09 100644
> > > > > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > > > > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > > > > > > @@ -4382,7 +4382,7 @@ int amdgpu_device_mode1_reset(struct 
> > > > > > > amdgpu_device *adev)
> > > > > > >     int amdgpu_device_pre_asic_reset(struct amdgpu_device 
> > > > > > > *adev,
> > > > > > >                      struct amdgpu_reset_context 
> > > > > > > *reset_context)
> > > > > > >     {
> > > > > > > -    int i, r = 0;
> > > > > > > +    int i, j, r = 0;
> > > > > > >         struct amdgpu_job *job = NULL;
> > > > > > >         bool need_full_reset =
> > > > > > >             test_bit(AMDGPU_NEED_FULL_RESET, 
> > > > > > > &reset_context->flags); @@ -4406,6 +4406,16 @@ int 
> > > > > > > amdgpu_device_pre_asic_reset(struct amdgpu_device *adev,
> > > > > > >             if (!ring || !ring->sched.thread)
> > > > > > >                 continue;
> > > > > > > +        /*clear job fence from fence drv to avoid 
> > > > > > > +force_completion
> > > > > > > +         *leave NULL and vm flush fence in fence drv */
> > > > > > > +        for (j = 0; j <= ring->fence_drv.num_fences_mask; 
> > > > > > > +j ++) {
> > > > > > > +            struct dma_fence *old,**ptr;
> > > > > > > +            ptr = &ring->fence_drv.fences[j];
> > > > > > > +            old = rcu_dereference_protected(*ptr, 1);
> > > > > > > +            if (old && test_bit(DMA_FENCE_FLAG_USER_BITS,
> > > > > > > &old->flags))) {
> > > > > > > +                RCU_INIT_POINTER(*ptr, NULL);
> > > > > > > +            }
> > > > > > Is this to avoid premature job free because of dma_fence_put 
> > > > > > inside amdgpu_fence_process ?
> > > > > > I can't currently remember why but we probably want all the 
> > > > > > HW fences currently in the ring to be forced signaled - 
> > > > > > maybe better to test for DMA_FENCE_FLAG_USER_BITS inside 
> > > > > > amdgpu_fence_process and still do the signaling but not the 
> > > > > > dma_fence_put part
> > > > > > 
> > > > > > Andrey
> > > > > Hi Andrey,
> > > > > 
> > > > > This is to avoid signaling the same fence twice. If we still 
> > > > > do the signaling, then the job in the pending list will be 
> > > > > signaled first in force_completion, and later be signaled in 
> > > > > resubmit. This will go to
> > > > > BUG() in amdgpu_fence_process.
> > > > 
> > > > Oh, i see, how about just adding 'skip' flag to amdgpu_ring and 
> > > > setting it before calling amdgpu_fence_driver_force_completion 
> > > > and resetting it after, then inside 
> > > > amdgpu_fence_driver_force_completion
> > > > you can just skip the signaling part with this flag for fences 
> > > > with DMA_FENCE_FLAG_USER_BITS set Less lines of code at least.
> > > Still sounds quite a bit hacky.
> > > 
> > > I would rather suggest to completely drop the approach with 
> > > amdgpu_fence_driver_force_completion(). I could never see why we 
> > > would want that in the first place.
> > > 
> > > Regards,
> > > Christian.
> > > 
> > Hi Christian,
> > 
> > I keep the amdgpu_fence_driver_force_completion here to make sure 
> > the vm flush fence is signaled and put.
> > So the key question is whether the fence of ib test should be the 
> > first fence to be signaled after reset.
> > If it should be, it means not only fences with 
> > DMA_FENCE_FLAG_USER_BITS but also vm flush fences should be removed 
> > from RCU fence array before ib_test. Then we must do the 
> > force_completion here for vm flush fence, otherwise there will be a 
> > memory leak here as no one will signal and put it after we remove it from RCU.
> > If not, then the first fence to signal could be vm flush fence. And 
> > I'm OK with drop amdgpu_fence_driver_force_completion here.
> 
> The key problem is that amdgpu_fence_driver_force_completion() goes 
> over the RCU array and signals everything there.
> 
> What we should do instead is to go over the jobs and cleanup the ones 
> we don't want to re-submit and keep the rest.
> 
Hi Christian,

The thing is vm flush fence has no job passed to amdgpu_fence_emit, so go through the jobs cannot help find the vm flush fence. And keep the rest fences in the RCU array will lead to signaling them in the ib_test right after ASIC reset. While they will be signaled again during resubmission. What I'm doning here is just trying to cleanup the fences without a parent job and make sure the rest fences won't be signaled twice.

> And please drop any usage of DMA_FENCE_FLAG_USER_BITS. That is not 
> something which should be abused for reset handling.
> 
The DMA_FENCE_FLAG_USER_BITS here is to mark this fence has a parent job. If this is not a proper usage here, do you have any suggestions about how to identify whether the fence has a parent job?

Best Regards,
JingWen Chen
> Regards,
> Christian.
> 
> > 
> > 
> > Best Regards,
> > JingWen Chen
> > > > Andrey
> > > > 
> > > > 
> > > > > > > +        }
> > > > > > >             /* after all hw jobs are reset, hw fence is 
> > > > > > > meaningless, so force_completion */
> > > > > > >             amdgpu_fence_driver_force_completion(ring);
> > > > > > >         }
> > > > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> > > > > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> > > > > > > index eecf21d8ec33..815776c9a013 100644
> > > > > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> > > > > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> > > > > > > @@ -156,11 +156,17 @@ int amdgpu_fence_emit(struct 
> > > > > > > amdgpu_ring *ring, struct dma_fence **f, struct amd
> > > > > > >             job->ring = ring;
> > > > > > >         }
> > > > > > > -    seq = ++ring->fence_drv.sync_seq;
> > > > > > > -    dma_fence_init(fence, &amdgpu_fence_ops,
> > > > > > > -               &ring->fence_drv.lock,
> > > > > > > -               adev->fence_context + ring->idx,
> > > > > > > -               seq);
> > > > > > > +    if (job != NULL && job->base.resubmit_flag == 1) {
> > > > > > > +        /* reinit seq for resubmitted jobs */
> > > > > > > +        seq = ++ring->fence_drv.sync_seq;
> > > > > > > +        fence->seqno = seq;
> > > > > > > +    } else {
> > > > > > > +        seq = ++ring->fence_drv.sync_seq;
> > > > > > Seems like you could do the above line only once above 
> > > > > > if-else as it was before
> > > > > Sure, I will modify this.
> > > > > 
> > > > > 
> > > > > Best Regards,
> > > > > JingWen Chen
> > > > > > > +        dma_fence_init(fence, &amdgpu_fence_ops,
> > > > > > > +                &ring->fence_drv.lock,
> > > > > > > +                adev->fence_context + ring->idx,
> > > > > > > +                seq);
> > > > > > > +    }
> > > > > > >         if (job != NULL) {
> > > > > > >             /* mark this fence has a parent job */
> > > > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > > > > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > > > > > > index 7c426e225b24..d6f848adc3f4 100644
> > > > > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > > > > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > > > > > > @@ -241,6 +241,7 @@ static struct dma_fence
> > > > > > > *amdgpu_job_run(struct drm_sched_job *sched_job)
> > > > > > >             dma_fence_set_error(finished, -ECANCELED);/* skip
> > > > > > > IB as well if VRAM lost */
> > > > > > >         if (finished->error < 0) {
> > > > > > > +        dma_fence_put(&job->hw_fence);
> > > > > > >             DRM_INFO("Skip scheduling IBs!\n");
> > > > > > >         } else {
> > > > > > >             r = amdgpu_ib_schedule(ring, job->num_ibs, job->ibs, job,
> > > > > > > @@ -249,7 +250,8 @@ static struct dma_fence
> > > > > > > *amdgpu_job_run(struct drm_sched_job *sched_job)
> > > > > > >                 DRM_ERROR("Error scheduling IBs (%d)\n", r);
> > > > > > >         }
> > > > > > > -    dma_fence_get(fence);
> > > > > > > +    if (!job->base.resubmit_flag)
> > > > > > > +        dma_fence_get(fence);
> > > > > > >         amdgpu_job_free_resources(job);
> > > > > > >         fence = r ? ERR_PTR(r) : fence;
> > > > > > > diff --git a/drivers/gpu/drm/scheduler/sched_main.c
> > > > > > > b/drivers/gpu/drm/scheduler/sched_main.c
> > > > > > > index f4f474944169..5a36ab5aea2d 100644
> > > > > > > --- a/drivers/gpu/drm/scheduler/sched_main.c
> > > > > > > +++ b/drivers/gpu/drm/scheduler/sched_main.c
> > > > > > > @@ -544,6 +544,7 @@ void drm_sched_resubmit_jobs_ext(struct
> > > > > > > drm_gpu_scheduler *sched, int max)
> > > > > > > dma_fence_set_error(&s_fence->finished, -ECANCELED);
> > > > > > >             dma_fence_put(s_job->s_fence->parent);
> > > > > > > +        s_job->resubmit_flag = 1;
> > > > > > >             fence = sched->ops->run_job(s_job);
> > > > > > >             i++;
> > > > > > > diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> > > > > > > index 4ea8606d91fe..06c101af1f71 100644
> > > > > > > --- a/include/drm/gpu_scheduler.h
> > > > > > > +++ b/include/drm/gpu_scheduler.h
> > > > > > > @@ -198,6 +198,7 @@ struct drm_sched_job {
> > > > > > >         enum drm_sched_priority        s_priority;
> > > > > > >         struct drm_sched_entity         *entity;
> > > > > > >         struct dma_fence_cb        cb;
> > > > > > > +    int resubmit_flag;
> > > > > > >     };
> > > > > > >     static inline bool drm_sched_invalidate_job(struct
> > > > > > > drm_sched_job *s_job,
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm: add tdr support for embeded hw_fence

[Christian König]

Am 23.07.21 um 09:07 schrieb Jingwen Chen:
> [SNIP]
> Hi Christian,
>
> The thing is vm flush fence has no job passed to amdgpu_fence_emit, so
> go through the jobs cannot help find the vm flush fence. And keep the
> rest fences in the RCU array will lead to signaling them in the ib_test
> right after ASIC reset. While they will be signaled again during
> resubmission. What I'm doning here is just trying to cleanup the fences
> without a parent job and make sure the rest fences won't be signaled
> twice.

It took me a moment to realize what you are talking about here.

This is for the KIQ! You need different handling for the KIQ than for 
the scheduler controlled rings.

It is not only the flush jobs, but the KIQ needs a complete reset 
because of the register writes pushed there as well.

>> And please drop any usage of DMA_FENCE_FLAG_USER_BITS. That is not something
>> which should be abused for reset handling.
>>
> The DMA_FENCE_FLAG_USER_BITS here is to mark this fence has a parent
> job. If this is not a proper usage here, do you have any suggestions
> about how to identify whether the fence has a parent job?

You don't need to mark the fences at all. Everything on the KIQ ring 
needs to be force completed since none of the fences on that ring have a 
parent job.

Christian.

>
> Best Regards,
> JingWen Chen
>> Regards,
>> Christian.
>>
>>>
>>> Best Regards,
>>> JingWen Chen
>>>>> Andrey
>>>>>
>>>>>
>>>>>>>> +        }
>>>>>>>>              /* after all hw jobs are reset, hw fence is
>>>>>>>> meaningless, so force_completion */
>>>>>>>>              amdgpu_fence_driver_force_completion(ring);
>>>>>>>>          }
>>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>>>>>>> index eecf21d8ec33..815776c9a013 100644
>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>>>>>>> @@ -156,11 +156,17 @@ int amdgpu_fence_emit(struct
>>>>>>>> amdgpu_ring *ring, struct dma_fence **f, struct amd
>>>>>>>>              job->ring = ring;
>>>>>>>>          }
>>>>>>>> -    seq = ++ring->fence_drv.sync_seq;
>>>>>>>> -    dma_fence_init(fence, &amdgpu_fence_ops,
>>>>>>>> -               &ring->fence_drv.lock,
>>>>>>>> -               adev->fence_context + ring->idx,
>>>>>>>> -               seq);
>>>>>>>> +    if (job != NULL && job->base.resubmit_flag == 1) {
>>>>>>>> +        /* reinit seq for resubmitted jobs */
>>>>>>>> +        seq = ++ring->fence_drv.sync_seq;
>>>>>>>> +        fence->seqno = seq;
>>>>>>>> +    } else {
>>>>>>>> +        seq = ++ring->fence_drv.sync_seq;
>>>>>>> Seems like you could do the above line only once above if-else
>>>>>>> as it was
>>>>>>> before
>>>>>> Sure, I will modify this.
>>>>>>
>>>>>>
>>>>>> Best Regards,
>>>>>> JingWen Chen
>>>>>>>> +        dma_fence_init(fence, &amdgpu_fence_ops,
>>>>>>>> +                &ring->fence_drv.lock,
>>>>>>>> +                adev->fence_context + ring->idx,
>>>>>>>> +                seq);
>>>>>>>> +    }
>>>>>>>>          if (job != NULL) {
>>>>>>>>              /* mark this fence has a parent job */
>>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>>>>>> index 7c426e225b24..d6f848adc3f4 100644
>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>>>>>> @@ -241,6 +241,7 @@ static struct dma_fence
>>>>>>>> *amdgpu_job_run(struct drm_sched_job *sched_job)
>>>>>>>>              dma_fence_set_error(finished, -ECANCELED);/* skip
>>>>>>>> IB as well if VRAM lost */
>>>>>>>>          if (finished->error < 0) {
>>>>>>>> +        dma_fence_put(&job->hw_fence);
>>>>>>>>              DRM_INFO("Skip scheduling IBs!\n");
>>>>>>>>          } else {
>>>>>>>>              r = amdgpu_ib_schedule(ring, job->num_ibs, job->ibs, job,
>>>>>>>> @@ -249,7 +250,8 @@ static struct dma_fence
>>>>>>>> *amdgpu_job_run(struct drm_sched_job *sched_job)
>>>>>>>>                  DRM_ERROR("Error scheduling IBs (%d)\n", r);
>>>>>>>>          }
>>>>>>>> -    dma_fence_get(fence);
>>>>>>>> +    if (!job->base.resubmit_flag)
>>>>>>>> +        dma_fence_get(fence);
>>>>>>>>          amdgpu_job_free_resources(job);
>>>>>>>>          fence = r ? ERR_PTR(r) : fence;
>>>>>>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c
>>>>>>>> b/drivers/gpu/drm/scheduler/sched_main.c
>>>>>>>> index f4f474944169..5a36ab5aea2d 100644
>>>>>>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>>>>>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>>>>>>> @@ -544,6 +544,7 @@ void drm_sched_resubmit_jobs_ext(struct
>>>>>>>> drm_gpu_scheduler *sched, int max)
>>>>>>>> dma_fence_set_error(&s_fence->finished, -ECANCELED);
>>>>>>>>              dma_fence_put(s_job->s_fence->parent);
>>>>>>>> +        s_job->resubmit_flag = 1;
>>>>>>>>              fence = sched->ops->run_job(s_job);
>>>>>>>>              i++;
>>>>>>>> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
>>>>>>>> index 4ea8606d91fe..06c101af1f71 100644
>>>>>>>> --- a/include/drm/gpu_scheduler.h
>>>>>>>> +++ b/include/drm/gpu_scheduler.h
>>>>>>>> @@ -198,6 +198,7 @@ struct drm_sched_job {
>>>>>>>>          enum drm_sched_priority        s_priority;
>>>>>>>>          struct drm_sched_entity         *entity;
>>>>>>>>          struct dma_fence_cb        cb;
>>>>>>>> +    int resubmit_flag;
>>>>>>>>      };
>>>>>>>>      static inline bool drm_sched_invalidate_job(struct
>>>>>>>> drm_sched_job *s_job,

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm: add tdr support for embeded hw_fence

[Jingwen Chen]

On Fri Jul 23, 2021 at 10:45:49AM +0200, Christian König wrote:
> Am 23.07.21 um 09:07 schrieb Jingwen Chen:
> > [SNIP]
> > Hi Christian,
> > 
> > The thing is vm flush fence has no job passed to amdgpu_fence_emit, so
> > go through the jobs cannot help find the vm flush fence. And keep the
> > rest fences in the RCU array will lead to signaling them in the ib_test
> > right after ASIC reset. While they will be signaled again during
> > resubmission. What I'm doning here is just trying to cleanup the fences
> > without a parent job and make sure the rest fences won't be signaled
> > twice.
> 
> It took me a moment to realize what you are talking about here.
> 
> This is for the KIQ! You need different handling for the KIQ than for the
> scheduler controlled rings.
> 
> It is not only the flush jobs, but the KIQ needs a complete reset because of
> the register writes pushed there as well.
> 
> > > And please drop any usage of DMA_FENCE_FLAG_USER_BITS. That is not something
> > > which should be abused for reset handling.
> > > 
> > The DMA_FENCE_FLAG_USER_BITS here is to mark this fence has a parent
> > job. If this is not a proper usage here, do you have any suggestions
> > about how to identify whether the fence has a parent job?
> 
> You don't need to mark the fences at all. Everything on the KIQ ring needs
> to be force completed since none of the fences on that ring have a parent
> job.
> 
> Christian.
>
Hi Christian

Yes KIQ ring fences all need force_completion. But we do need to mark the
fences. Say we have a gfx ring job with vm_flush=1 sending to
amdgpu_ib_schedule, then in amdgpu_vm_flush, after the
amdgpu_ring_emit_vm_flush is done, we will create a vm flush fence on
gfx ring with amdgpu_fence_emit(ring, &fence, NULL, 0).

Then this vm flush fence we create here has no parent job but it's on
gfx ring. 
If we only do force_completion for KIQ ring and just clear the
RCU array for the scheduler controlled rings, nobody will signal and put this
gfx ring vm_flush fence again. When this job is resubmitted, it will
just create a new vm_flush fence. This is a memleak and I have seen this
memleak during my test.

Best Regards,
JingWen Chen
> > 
> > Best Regards,
> > JingWen Chen
> > > Regards,
> > > Christian.
> > > 
> > > > 
> > > > Best Regards,
> > > > JingWen Chen
> > > > > > Andrey
> > > > > > 
> > > > > > 
> > > > > > > > > +        }
> > > > > > > > >              /* after all hw jobs are reset, hw fence is
> > > > > > > > > meaningless, so force_completion */
> > > > > > > > >              amdgpu_fence_driver_force_completion(ring);
> > > > > > > > >          }
> > > > > > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> > > > > > > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> > > > > > > > > index eecf21d8ec33..815776c9a013 100644
> > > > > > > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> > > > > > > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> > > > > > > > > @@ -156,11 +156,17 @@ int amdgpu_fence_emit(struct
> > > > > > > > > amdgpu_ring *ring, struct dma_fence **f, struct amd
> > > > > > > > >              job->ring = ring;
> > > > > > > > >          }
> > > > > > > > > -    seq = ++ring->fence_drv.sync_seq;
> > > > > > > > > -    dma_fence_init(fence, &amdgpu_fence_ops,
> > > > > > > > > -               &ring->fence_drv.lock,
> > > > > > > > > -               adev->fence_context + ring->idx,
> > > > > > > > > -               seq);
> > > > > > > > > +    if (job != NULL && job->base.resubmit_flag == 1) {
> > > > > > > > > +        /* reinit seq for resubmitted jobs */
> > > > > > > > > +        seq = ++ring->fence_drv.sync_seq;
> > > > > > > > > +        fence->seqno = seq;
> > > > > > > > > +    } else {
> > > > > > > > > +        seq = ++ring->fence_drv.sync_seq;
> > > > > > > > Seems like you could do the above line only once above if-else
> > > > > > > > as it was
> > > > > > > > before
> > > > > > > Sure, I will modify this.
> > > > > > > 
> > > > > > > 
> > > > > > > Best Regards,
> > > > > > > JingWen Chen
> > > > > > > > > +        dma_fence_init(fence, &amdgpu_fence_ops,
> > > > > > > > > +                &ring->fence_drv.lock,
> > > > > > > > > +                adev->fence_context + ring->idx,
> > > > > > > > > +                seq);
> > > > > > > > > +    }
> > > > > > > > >          if (job != NULL) {
> > > > > > > > >              /* mark this fence has a parent job */
> > > > > > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > > > > > > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > > > > > > > > index 7c426e225b24..d6f848adc3f4 100644
> > > > > > > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > > > > > > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > > > > > > > > @@ -241,6 +241,7 @@ static struct dma_fence
> > > > > > > > > *amdgpu_job_run(struct drm_sched_job *sched_job)
> > > > > > > > >              dma_fence_set_error(finished, -ECANCELED);/* skip
> > > > > > > > > IB as well if VRAM lost */
> > > > > > > > >          if (finished->error < 0) {
> > > > > > > > > +        dma_fence_put(&job->hw_fence);
> > > > > > > > >              DRM_INFO("Skip scheduling IBs!\n");
> > > > > > > > >          } else {
> > > > > > > > >              r = amdgpu_ib_schedule(ring, job->num_ibs, job->ibs, job,
> > > > > > > > > @@ -249,7 +250,8 @@ static struct dma_fence
> > > > > > > > > *amdgpu_job_run(struct drm_sched_job *sched_job)
> > > > > > > > >                  DRM_ERROR("Error scheduling IBs (%d)\n", r);
> > > > > > > > >          }
> > > > > > > > > -    dma_fence_get(fence);
> > > > > > > > > +    if (!job->base.resubmit_flag)
> > > > > > > > > +        dma_fence_get(fence);
> > > > > > > > >          amdgpu_job_free_resources(job);
> > > > > > > > >          fence = r ? ERR_PTR(r) : fence;
> > > > > > > > > diff --git a/drivers/gpu/drm/scheduler/sched_main.c
> > > > > > > > > b/drivers/gpu/drm/scheduler/sched_main.c
> > > > > > > > > index f4f474944169..5a36ab5aea2d 100644
> > > > > > > > > --- a/drivers/gpu/drm/scheduler/sched_main.c
> > > > > > > > > +++ b/drivers/gpu/drm/scheduler/sched_main.c
> > > > > > > > > @@ -544,6 +544,7 @@ void drm_sched_resubmit_jobs_ext(struct
> > > > > > > > > drm_gpu_scheduler *sched, int max)
> > > > > > > > > dma_fence_set_error(&s_fence->finished, -ECANCELED);
> > > > > > > > >              dma_fence_put(s_job->s_fence->parent);
> > > > > > > > > +        s_job->resubmit_flag = 1;
> > > > > > > > >              fence = sched->ops->run_job(s_job);
> > > > > > > > >              i++;
> > > > > > > > > diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> > > > > > > > > index 4ea8606d91fe..06c101af1f71 100644
> > > > > > > > > --- a/include/drm/gpu_scheduler.h
> > > > > > > > > +++ b/include/drm/gpu_scheduler.h
> > > > > > > > > @@ -198,6 +198,7 @@ struct drm_sched_job {
> > > > > > > > >          enum drm_sched_priority        s_priority;
> > > > > > > > >          struct drm_sched_entity         *entity;
> > > > > > > > >          struct dma_fence_cb        cb;
> > > > > > > > > +    int resubmit_flag;
> > > > > > > > >      };
> > > > > > > > >      static inline bool drm_sched_invalidate_job(struct
> > > > > > > > > drm_sched_job *s_job,
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm: add tdr support for embeded hw_fence

[Christian König]

Am 23.07.21 um 11:25 schrieb Jingwen Chen:
> On Fri Jul 23, 2021 at 10:45:49AM +0200, Christian König wrote:
>> Am 23.07.21 um 09:07 schrieb Jingwen Chen:
>>> [SNIP]
>>> Hi Christian,
>>>
>>> The thing is vm flush fence has no job passed to amdgpu_fence_emit, so
>>> go through the jobs cannot help find the vm flush fence. And keep the
>>> rest fences in the RCU array will lead to signaling them in the ib_test
>>> right after ASIC reset. While they will be signaled again during
>>> resubmission. What I'm doning here is just trying to cleanup the fences
>>> without a parent job and make sure the rest fences won't be signaled
>>> twice.
>> It took me a moment to realize what you are talking about here.
>>
>> This is for the KIQ! You need different handling for the KIQ than for the
>> scheduler controlled rings.
>>
>> It is not only the flush jobs, but the KIQ needs a complete reset because of
>> the register writes pushed there as well.
>>
>>>> And please drop any usage of DMA_FENCE_FLAG_USER_BITS. That is not something
>>>> which should be abused for reset handling.
>>>>
>>> The DMA_FENCE_FLAG_USER_BITS here is to mark this fence has a parent
>>> job. If this is not a proper usage here, do you have any suggestions
>>> about how to identify whether the fence has a parent job?
>> You don't need to mark the fences at all. Everything on the KIQ ring needs
>> to be force completed since none of the fences on that ring have a parent
>> job.
>>
>> Christian.
>>
> Hi Christian
>
> Yes KIQ ring fences all need force_completion. But we do need to mark the
> fences. Say we have a gfx ring job with vm_flush=1 sending to
> amdgpu_ib_schedule, then in amdgpu_vm_flush, after the
> amdgpu_ring_emit_vm_flush is done, we will create a vm flush fence on
> gfx ring with amdgpu_fence_emit(ring, &fence, NULL, 0).

That is illegal and needs to be fixed as well.

> Then this vm flush fence we create here has no parent job but it's on
> gfx ring.
> If we only do force_completion for KIQ ring and just clear the
> RCU array for the scheduler controlled rings, nobody will signal and put this
> gfx ring vm_flush fence again. When this job is resubmitted, it will
> just create a new vm_flush fence. This is a memleak and I have seen this
> memleak during my test.

At least I'm better understanding the problem now as well.

But you are just trying to circumvent symptoms and not fixing the root 
cause.

See any memory allocation during command submission must be prevented. 
This also includes the flush fence.

Regards,
Christian.

>
> Best Regards,
> JingWen Chen
>>> Best Regards,
>>> JingWen Chen
>>>> Regards,
>>>> Christian.
>>>>
>>>>> Best Regards,
>>>>> JingWen Chen
>>>>>>> Andrey
>>>>>>>
>>>>>>>
>>>>>>>>>> +        }
>>>>>>>>>>               /* after all hw jobs are reset, hw fence is
>>>>>>>>>> meaningless, so force_completion */
>>>>>>>>>>               amdgpu_fence_driver_force_completion(ring);
>>>>>>>>>>           }
>>>>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>>>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>>>>>>>>> index eecf21d8ec33..815776c9a013 100644
>>>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>>>>>>>>> @@ -156,11 +156,17 @@ int amdgpu_fence_emit(struct
>>>>>>>>>> amdgpu_ring *ring, struct dma_fence **f, struct amd
>>>>>>>>>>               job->ring = ring;
>>>>>>>>>>           }
>>>>>>>>>> -    seq = ++ring->fence_drv.sync_seq;
>>>>>>>>>> -    dma_fence_init(fence, &amdgpu_fence_ops,
>>>>>>>>>> -               &ring->fence_drv.lock,
>>>>>>>>>> -               adev->fence_context + ring->idx,
>>>>>>>>>> -               seq);
>>>>>>>>>> +    if (job != NULL && job->base.resubmit_flag == 1) {
>>>>>>>>>> +        /* reinit seq for resubmitted jobs */
>>>>>>>>>> +        seq = ++ring->fence_drv.sync_seq;
>>>>>>>>>> +        fence->seqno = seq;
>>>>>>>>>> +    } else {
>>>>>>>>>> +        seq = ++ring->fence_drv.sync_seq;
>>>>>>>>> Seems like you could do the above line only once above if-else
>>>>>>>>> as it was
>>>>>>>>> before
>>>>>>>> Sure, I will modify this.
>>>>>>>>
>>>>>>>>
>>>>>>>> Best Regards,
>>>>>>>> JingWen Chen
>>>>>>>>>> +        dma_fence_init(fence, &amdgpu_fence_ops,
>>>>>>>>>> +                &ring->fence_drv.lock,
>>>>>>>>>> +                adev->fence_context + ring->idx,
>>>>>>>>>> +                seq);
>>>>>>>>>> +    }
>>>>>>>>>>           if (job != NULL) {
>>>>>>>>>>               /* mark this fence has a parent job */
>>>>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>>>>>>>> index 7c426e225b24..d6f848adc3f4 100644
>>>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>>>>>>>> @@ -241,6 +241,7 @@ static struct dma_fence
>>>>>>>>>> *amdgpu_job_run(struct drm_sched_job *sched_job)
>>>>>>>>>>               dma_fence_set_error(finished, -ECANCELED);/* skip
>>>>>>>>>> IB as well if VRAM lost */
>>>>>>>>>>           if (finished->error < 0) {
>>>>>>>>>> +        dma_fence_put(&job->hw_fence);
>>>>>>>>>>               DRM_INFO("Skip scheduling IBs!\n");
>>>>>>>>>>           } else {
>>>>>>>>>>               r = amdgpu_ib_schedule(ring, job->num_ibs, job->ibs, job,
>>>>>>>>>> @@ -249,7 +250,8 @@ static struct dma_fence
>>>>>>>>>> *amdgpu_job_run(struct drm_sched_job *sched_job)
>>>>>>>>>>                   DRM_ERROR("Error scheduling IBs (%d)\n", r);
>>>>>>>>>>           }
>>>>>>>>>> -    dma_fence_get(fence);
>>>>>>>>>> +    if (!job->base.resubmit_flag)
>>>>>>>>>> +        dma_fence_get(fence);
>>>>>>>>>>           amdgpu_job_free_resources(job);
>>>>>>>>>>           fence = r ? ERR_PTR(r) : fence;
>>>>>>>>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c
>>>>>>>>>> b/drivers/gpu/drm/scheduler/sched_main.c
>>>>>>>>>> index f4f474944169..5a36ab5aea2d 100644
>>>>>>>>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>>>>>>>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>>>>>>>>> @@ -544,6 +544,7 @@ void drm_sched_resubmit_jobs_ext(struct
>>>>>>>>>> drm_gpu_scheduler *sched, int max)
>>>>>>>>>> dma_fence_set_error(&s_fence->finished, -ECANCELED);
>>>>>>>>>>               dma_fence_put(s_job->s_fence->parent);
>>>>>>>>>> +        s_job->resubmit_flag = 1;
>>>>>>>>>>               fence = sched->ops->run_job(s_job);
>>>>>>>>>>               i++;
>>>>>>>>>> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
>>>>>>>>>> index 4ea8606d91fe..06c101af1f71 100644
>>>>>>>>>> --- a/include/drm/gpu_scheduler.h
>>>>>>>>>> +++ b/include/drm/gpu_scheduler.h
>>>>>>>>>> @@ -198,6 +198,7 @@ struct drm_sched_job {
>>>>>>>>>>           enum drm_sched_priority        s_priority;
>>>>>>>>>>           struct drm_sched_entity         *entity;
>>>>>>>>>>           struct dma_fence_cb        cb;
>>>>>>>>>> +    int resubmit_flag;
>>>>>>>>>>       };
>>>>>>>>>>       static inline bool drm_sched_invalidate_job(struct
>>>>>>>>>> drm_sched_job *s_job,

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx