[PATCH v6] drm/amd/amdgpu implement tdr advanced mode

[PATCH v6] drm/amd/amdgpu implement tdr advanced mode

[Jack Zhang]

[Why]
Previous tdr design treats the first job in job_timeout as the bad job.
But sometimes a later bad compute job can block a good gfx job and
cause an unexpected gfx job timeout because gfx and compute ring share
internal GC HW mutually.

[How]
This patch implements an advanced tdr mode.It involves an additinal
synchronous pre-resubmit step(Step0 Resubmit) before normal resubmit
step in order to find the real bad job.

1. At Step0 Resubmit stage, it synchronously submits and pends for the
first job being signaled. If it gets timeout, we identify it as guilty
and do hw reset. After that, we would do the normal resubmit step to
resubmit left jobs.

2. Re-insert Bailing job to mirror_list, and leave it to be handled by
the main reset thread.

3. For whole gpu reset(vram lost), do resubmit as the old way.

Signed-off-by: Jack Zhang <Jack.Zhang1@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 72 ++++++++++++++++++++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c    |  2 +-
 drivers/gpu/drm/scheduler/sched_main.c     | 75 ++++++++++++++++++++++
 include/drm/gpu_scheduler.h                |  2 +
 4 files changed, 148 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index e247c3a2ec08..dac0a242e5f5 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -4639,6 +4639,7 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
 	int i, r = 0;
 	bool need_emergency_restart = false;
 	bool audio_suspended = false;
+	int tmp_vram_lost_counter;
 
 	/*
 	 * Special case: RAS triggered and full reset isn't supported
@@ -4689,9 +4690,14 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
 		dev_info(adev->dev, "Bailing on TDR for s_job:%llx, as another already in progress",
 					job ? job->base.id : -1);
 
-		/* even we skipped this reset, still need to set the job to guilty */
-		if (job)
+		if (job) {
+			/* re-insert node to avoid memory leak */
+			spin_lock(&job->base.sched->job_list_lock);
+			list_add(&job->base.node, &job->base.sched->pending_list);
+			spin_unlock(&job->base.sched->job_list_lock);
+			/* even we skipped this reset, still need to set the job to guilty */
 			drm_sched_increase_karma(&job->base);
+		}
 		goto skip_recovery;
 	}
 
@@ -4788,6 +4794,7 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
 		}
 	}
 
+	tmp_vram_lost_counter = atomic_read(&((adev)->vram_lost_counter));
 	/* Actual ASIC resets if needed.*/
 	/* TODO Implement XGMI hive reset logic for SRIOV */
 	if (amdgpu_sriov_vf(adev)) {
@@ -4805,6 +4812,67 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
 	/* Post ASIC reset for all devs .*/
 	list_for_each_entry(tmp_adev, device_list_handle, gmc.xgmi.head) {
 
+		if (amdgpu_gpu_recovery == 2 &&
+			!(tmp_vram_lost_counter < atomic_read(&adev->vram_lost_counter))) {
+
+			for (i = 0; i < AMDGPU_MAX_RINGS; ++i) {
+				struct amdgpu_ring *ring = tmp_adev->rings[i];
+				int ret = 0;
+				struct drm_sched_job *s_job;
+
+				if (!ring || !ring->sched.thread)
+					continue;
+
+				/* No point to resubmit jobs if we didn't HW reset*/
+				if (!tmp_adev->asic_reset_res && !job_signaled) {
+
+					s_job = list_first_entry_or_null(&ring->sched.pending_list, struct drm_sched_job, list);
+					if (s_job == NULL)
+						continue;
+
+					/* clear job's guilty and depend the folowing step to decide the real one */
+					drm_sched_reset_karma(s_job);
+					drm_sched_resubmit_jobs2(&ring->sched, 1);
+					ret = dma_fence_wait_timeout(s_job->s_fence->parent, false, ring->sched.timeout);
+
+					if (ret == 0) { /* timeout */
+						DRM_ERROR("Found the real bad job! ring:%s, job_id:%llx\n", ring->sched.name, s_job->id);
+						/* set guilty */
+						drm_sched_increase_karma(s_job);
+
+						/* do hw reset */
+						if (amdgpu_sriov_vf(adev)) {
+							amdgpu_virt_fini_data_exchange(adev);
+							r = amdgpu_device_reset_sriov(adev, false);
+							if (r)
+								adev->asic_reset_res = r;
+						} else {
+							r  = amdgpu_do_asic_reset(hive, device_list_handle, &need_full_reset, false);
+							if (r && r == -EAGAIN)
+								goto retry;
+						}
+
+						/* add reset counter so that the following resubmitted job could flush vmid */
+						atomic_inc(&tmp_adev->gpu_reset_counter);
+						continue;
+					}
+
+					/* got the hw fence, signal finished fence */
+					atomic_dec(&ring->sched.num_jobs);
+					dma_fence_get(&s_job->s_fence->finished);
+					dma_fence_signal(&s_job->s_fence->finished);
+					dma_fence_put(&s_job->s_fence->finished);
+
+
+					/* remove node from list and free the job */
+					spin_lock(&ring->sched.job_list_lock);
+					list_del_init(&s_job->node);
+					spin_unlock(&ring->sched.job_list_lock);
+					ring->sched.ops->free_job(s_job);
+				}
+			}
+		}
+
 		for (i = 0; i < AMDGPU_MAX_RINGS; ++i) {
 			struct amdgpu_ring *ring = tmp_adev->rings[i];
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 865f924772b0..9c3f4edb7532 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -509,7 +509,7 @@ module_param_named(compute_multipipe, amdgpu_compute_multipipe, int, 0444);
  * DOC: gpu_recovery (int)
  * Set to enable GPU recovery mechanism (1 = enable, 0 = disable). The default is -1 (auto, disabled except SRIOV).
  */
-MODULE_PARM_DESC(gpu_recovery, "Enable GPU recovery mechanism, (1 = enable, 0 = disable, -1 = auto)");
+MODULE_PARM_DESC(gpu_recovery, "Enable GPU recovery mechanism, (2 = advanced tdr mode, 1 = enable, 0 = disable, -1 = auto)");
 module_param_named(gpu_recovery, amdgpu_gpu_recovery, int, 0444);
 
 /**
diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index d82a7ebf6099..340a193b4fb9 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -395,6 +395,81 @@ void drm_sched_increase_karma(struct drm_sched_job *bad)
 }
 EXPORT_SYMBOL(drm_sched_increase_karma);
 
+
+void drm_sched_resubmit_jobs2(struct drm_gpu_scheduler *sched, int max)
+{
+	struct drm_sched_job *s_job, *tmp;
+	uint64_t guilty_context;
+	bool found_guilty = false;
+	struct dma_fence *fence;
+	int i = 0;
+
+	list_for_each_entry_safe(s_job, tmp, &sched->pending_list, list) {
+		struct drm_sched_fence *s_fence = s_job->s_fence;
+
+		if (i >= max)
+			break;
+
+		if (!found_guilty && atomic_read(&s_job->karma) > sched->hang_limit) {
+			found_guilty = true;
+			guilty_context = s_job->s_fence->scheduled.context;
+		}
+
+		if (found_guilty && s_job->s_fence->scheduled.context == guilty_context)
+			dma_fence_set_error(&s_fence->finished, -ECANCELED);
+
+		dma_fence_put(s_job->s_fence->parent);
+		fence = sched->ops->run_job(s_job);
+		i++;
+
+		if (IS_ERR_OR_NULL(fence)) {
+			if (IS_ERR(fence))
+				dma_fence_set_error(&s_fence->finished, PTR_ERR(fence));
+
+			s_job->s_fence->parent = NULL;
+		} else {
+			s_job->s_fence->parent = fence;
+		}
+	}
+}
+EXPORT_SYMBOL(drm_sched_resubmit_jobs2);
+
+
+
+void drm_sched_reset_karma(struct drm_sched_job *bad)
+{
+	int i;
+	struct drm_sched_entity *tmp;
+	struct drm_sched_entity *entity;
+	struct drm_gpu_scheduler *sched = bad->sched;
+
+	/* don't reset @bad's karma if it's from KERNEL RQ,
+	 * because sometimes GPU hang would cause kernel jobs (like VM updating jobs)
+	 * corrupt but keep in mind that kernel jobs always considered good.
+	 */
+	if (bad->s_priority != DRM_SCHED_PRIORITY_KERNEL) {
+		atomic_set(&bad->karma, 0);
+		for (i = DRM_SCHED_PRIORITY_MIN; i < DRM_SCHED_PRIORITY_KERNEL;
+		     i++) {
+			struct drm_sched_rq *rq = &sched->sched_rq[i];
+
+			spin_lock(&rq->lock);
+			list_for_each_entry_safe(entity, tmp, &rq->entities, list) {
+				if (bad->s_fence->scheduled.context ==
+					entity->fence_context) {
+						if (entity->guilty)
+							atomic_set(entity->guilty, 0);
+					break;
+				}
+			}
+			spin_unlock(&rq->lock);
+			if (&entity->list != &rq->entities)
+				break;
+		}
+	}
+}
+EXPORT_SYMBOL(drm_sched_reset_karma);
+
 /**
  * drm_sched_stop - stop the scheduler
  *
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index 1c815e0a14ed..01c609149731 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -321,7 +321,9 @@ void drm_sched_wakeup(struct drm_gpu_scheduler *sched);
 void drm_sched_stop(struct drm_gpu_scheduler *sched, struct drm_sched_job *bad);
 void drm_sched_start(struct drm_gpu_scheduler *sched, bool full_recovery);
 void drm_sched_resubmit_jobs(struct drm_gpu_scheduler *sched);
+void drm_sched_resubmit_jobs2(struct drm_gpu_scheduler *sched, int max);
 void drm_sched_increase_karma(struct drm_sched_job *bad);
+void drm_sched_reset_karma(struct drm_sched_job *bad);
 bool drm_sched_dependency_optimized(struct dma_fence* fence,
 				    struct drm_sched_entity *entity);
 void drm_sched_fault(struct drm_gpu_scheduler *sched);
-- 
2.25.1

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

Re: [PATCH v6] drm/amd/amdgpu implement tdr advanced mode

[Andrey Grodzovsky]

On 2021-03-10 8:33 a.m., Jack Zhang wrote:
> [Why]
> Previous tdr design treats the first job in job_timeout as the bad job.
> But sometimes a later bad compute job can block a good gfx job and
> cause an unexpected gfx job timeout because gfx and compute ring share
> internal GC HW mutually.
> 
> [How]
> This patch implements an advanced tdr mode.It involves an additinal
> synchronous pre-resubmit step(Step0 Resubmit) before normal resubmit
> step in order to find the real bad job.
> 
> 1. At Step0 Resubmit stage, it synchronously submits and pends for the
> first job being signaled. If it gets timeout, we identify it as guilty
> and do hw reset. After that, we would do the normal resubmit step to
> resubmit left jobs.
> 
> 2. Re-insert Bailing job to mirror_list, and leave it to be handled by
> the main reset thread.
> 
> 3. For whole gpu reset(vram lost), do resubmit as the old way.
> 
> Signed-off-by: Jack Zhang <Jack.Zhang1@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 72 ++++++++++++++++++++-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c    |  2 +-
>   drivers/gpu/drm/scheduler/sched_main.c     | 75 ++++++++++++++++++++++
>   include/drm/gpu_scheduler.h                |  2 +
>   4 files changed, 148 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index e247c3a2ec08..dac0a242e5f5 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -4639,6 +4639,7 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
>   	int i, r = 0;
>   	bool need_emergency_restart = false;
>   	bool audio_suspended = false;
> +	int tmp_vram_lost_counter;
>   
>   	/*
>   	 * Special case: RAS triggered and full reset isn't supported
> @@ -4689,9 +4690,14 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
>   		dev_info(adev->dev, "Bailing on TDR for s_job:%llx, as another already in progress",
>   					job ? job->base.id : -1);
>   
> -		/* even we skipped this reset, still need to set the job to guilty */
> -		if (job)
> +		if (job) {
> +			/* re-insert node to avoid memory leak */
> +			spin_lock(&job->base.sched->job_list_lock);
> +			list_add(&job->base.node, &job->base.sched->pending_list);
> +			spin_unlock(&job->base.sched->job_list_lock);
> +			/* even we skipped this reset, still need to set the job to guilty */
>   			drm_sched_increase_karma(&job->base);
> +		}

I think this piece should be in a standalone
patch as it comes to fix a bug of leaking
jobs and not directly related to find actual
guilty job. Also, this is not the only place
where the problem arises - it also arises
in other drivers where they check that guilty
job's fence already signaled and bail out before
reinsertion of bad job to mirror list. Seems to me
better fix would be to handle this within scheduler
code by adding specific return value to
drm_sched_backend_ops.timedout_job marking that the code
terminated early - before calling drm_sched_stop and so 
drm_sched_job_timedout
would know this and then reinsert the job back to mirror_list
itself.

>   		goto skip_recovery;
>   	}
>   
> @@ -4788,6 +4794,7 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
>   		}
>   	}
>   
> +	tmp_vram_lost_counter = atomic_read(&((adev)->vram_lost_counter));
>   	/* Actual ASIC resets if needed.*/
>   	/* TODO Implement XGMI hive reset logic for SRIOV */
>   	if (amdgpu_sriov_vf(adev)) {
> @@ -4805,6 +4812,67 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
>   	/* Post ASIC reset for all devs .*/
>   	list_for_each_entry(tmp_adev, device_list_handle, gmc.xgmi.head) {
>   
> +		if (amdgpu_gpu_recovery == 2 &&
> +			!(tmp_vram_lost_counter < atomic_read(&adev->vram_lost_counter))) {
> +
> +			for (i = 0; i < AMDGPU_MAX_RINGS; ++i) {
> +				struct amdgpu_ring *ring = tmp_adev->rings[i];
> +				int ret = 0;
> +				struct drm_sched_job *s_job;
> +
> +				if (!ring || !ring->sched.thread)
> +					continue;
> +
> +				/* No point to resubmit jobs if we didn't HW reset*/
> +				if (!tmp_adev->asic_reset_res && !job_signaled) {
> +
> +					s_job = list_first_entry_or_null(&ring->sched.pending_list, struct drm_sched_job, list);
> +					if (s_job == NULL)
> +						continue;
> +
> +					/* clear job's guilty and depend the folowing step to decide the real one */
> +					drm_sched_reset_karma(s_job);
> +					drm_sched_resubmit_jobs2(&ring->sched, 1);
> +					ret = dma_fence_wait_timeout(s_job->s_fence->parent, false, ring->sched.timeout);
> +
> +					if (ret == 0) { /* timeout */
> +						DRM_ERROR("Found the real bad job! ring:%s, job_id:%llx\n", ring->sched.name, s_job->id);
> +						/* set guilty */
> +						drm_sched_increase_karma(s_job);
> +
> +						/* do hw reset */
> +						if (amdgpu_sriov_vf(adev)) {
> +							amdgpu_virt_fini_data_exchange(adev);
> +							r = amdgpu_device_reset_sriov(adev, false);
> +							if (r)
> +								adev->asic_reset_res = r;
> +						} else {
> +							r  = amdgpu_do_asic_reset(hive, device_list_handle, &need_full_reset, false);
> +							if (r && r == -EAGAIN)
> +								goto retry;
> +						}
> +
> +						/* add reset counter so that the following resubmitted job could flush vmid */
> +						atomic_inc(&tmp_adev->gpu_reset_counter);
> +						continue;
> +					}
> +
> +					/* got the hw fence, signal finished fence */
> +					atomic_dec(&ring->sched.num_jobs);
> +					dma_fence_get(&s_job->s_fence->finished);
> +					dma_fence_signal(&s_job->s_fence->finished);
> +					dma_fence_put(&s_job->s_fence->finished);
> +
> +
> +					/* remove node from list and free the job */
> +					spin_lock(&ring->sched.job_list_lock);
> +					list_del_init(&s_job->node);
> +					spin_unlock(&ring->sched.job_list_lock);
> +					ring->sched.ops->free_job(s_job);
> +				}
> +			}
> +		}
> +
>   		for (i = 0; i < AMDGPU_MAX_RINGS; ++i) {
>   			struct amdgpu_ring *ring = tmp_adev->rings[i];
>   
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 865f924772b0..9c3f4edb7532 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -509,7 +509,7 @@ module_param_named(compute_multipipe, amdgpu_compute_multipipe, int, 0444);
>    * DOC: gpu_recovery (int)
>    * Set to enable GPU recovery mechanism (1 = enable, 0 = disable). The default is -1 (auto, disabled except SRIOV).
>    */
> -MODULE_PARM_DESC(gpu_recovery, "Enable GPU recovery mechanism, (1 = enable, 0 = disable, -1 = auto)");
> +MODULE_PARM_DESC(gpu_recovery, "Enable GPU recovery mechanism, (2 = advanced tdr mode, 1 = enable, 0 = disable, -1 = auto)");
>   module_param_named(gpu_recovery, amdgpu_gpu_recovery, int, 0444);
>   
>   /**
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> index d82a7ebf6099..340a193b4fb9 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -395,6 +395,81 @@ void drm_sched_increase_karma(struct drm_sched_job *bad)
>   }
>   EXPORT_SYMBOL(drm_sched_increase_karma);
>   
> +
> +void drm_sched_resubmit_jobs2(struct drm_gpu_scheduler *sched, int max)
> +{
> +	struct drm_sched_job *s_job, *tmp;
> +	uint64_t guilty_context;
> +	bool found_guilty = false;
> +	struct dma_fence *fence;
> +	int i = 0;
> +
> +	list_for_each_entry_safe(s_job, tmp, &sched->pending_list, list) {
> +		struct drm_sched_fence *s_fence = s_job->s_fence;
> +
> +		if (i >= max)
> +			break;
> +
> +		if (!found_guilty && atomic_read(&s_job->karma) > sched->hang_limit) {
> +			found_guilty = true;
> +			guilty_context = s_job->s_fence->scheduled.context;
> +		}
> +
> +		if (found_guilty && s_job->s_fence->scheduled.context == guilty_context)
> +			dma_fence_set_error(&s_fence->finished, -ECANCELED);
> +
> +		dma_fence_put(s_job->s_fence->parent);
> +		fence = sched->ops->run_job(s_job);
> +		i++;
> +
> +		if (IS_ERR_OR_NULL(fence)) {
> +			if (IS_ERR(fence))
> +				dma_fence_set_error(&s_fence->finished, PTR_ERR(fence));
> +
> +			s_job->s_fence->parent = NULL;
> +		} else {
> +			s_job->s_fence->parent = fence;
> +		}
> +	}
> +}
> +EXPORT_SYMBOL(drm_sched_resubmit_jobs2);

This is identical to drm_sched_resubmit_jobs in all but the
'int max' handling, can't you reuse drm_sched_resubmit_jobs
by passing max argument==1 for the find guilty run and then, for the
later normal run passing max==MAX_INT to avoid break the iteration
to early ?

> +
> +
> +
> +void drm_sched_reset_karma(struct drm_sched_job *bad)
> +{
> +	int i;
> +	struct drm_sched_entity *tmp;
> +	struct drm_sched_entity *entity;
> +	struct drm_gpu_scheduler *sched = bad->sched;
> +
> +	/* don't reset @bad's karma if it's from KERNEL RQ,
> +	 * because sometimes GPU hang would cause kernel jobs (like VM updating jobs)
> +	 * corrupt but keep in mind that kernel jobs always considered good.
> +	 */
> +	if (bad->s_priority != DRM_SCHED_PRIORITY_KERNEL) {
> +		atomic_set(&bad->karma, 0);
> +		for (i = DRM_SCHED_PRIORITY_MIN; i < DRM_SCHED_PRIORITY_KERNEL;
> +		     i++) {
> +			struct drm_sched_rq *rq = &sched->sched_rq[i];
> +
> +			spin_lock(&rq->lock);
> +			list_for_each_entry_safe(entity, tmp, &rq->entities, list) {
> +				if (bad->s_fence->scheduled.context ==
> +					entity->fence_context) {
> +						if (entity->guilty)
> +							atomic_set(entity->guilty, 0);
> +					break;
> +				}
> +			}
> +			spin_unlock(&rq->lock);
> +			if (&entity->list != &rq->entities)
> +				break;
> +		}
> +	}
> +}
> +EXPORT_SYMBOL(drm_sched_reset_karma);

Same as above, very similar drm_sched_increase_karma, I would
add a flag and just reuse code instead of duplucation.

Andrey

> +
>   /**
>    * drm_sched_stop - stop the scheduler
>    *
> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> index 1c815e0a14ed..01c609149731 100644
> --- a/include/drm/gpu_scheduler.h
> +++ b/include/drm/gpu_scheduler.h
> @@ -321,7 +321,9 @@ void drm_sched_wakeup(struct drm_gpu_scheduler *sched);
>   void drm_sched_stop(struct drm_gpu_scheduler *sched, struct drm_sched_job *bad);
>   void drm_sched_start(struct drm_gpu_scheduler *sched, bool full_recovery);
>   void drm_sched_resubmit_jobs(struct drm_gpu_scheduler *sched);
> +void drm_sched_resubmit_jobs2(struct drm_gpu_scheduler *sched, int max);
>   void drm_sched_increase_karma(struct drm_sched_job *bad);
> +void drm_sched_reset_karma(struct drm_sched_job *bad);
>   bool drm_sched_dependency_optimized(struct dma_fence* fence,
>   				    struct drm_sched_entity *entity);
>   void drm_sched_fault(struct drm_gpu_scheduler *sched);
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH v6] drm/amd/amdgpu implement tdr advanced mode

[Zhang, Jack (Jian)]

[AMD Official Use Only - Internal Distribution Only]

Hi, Andrey,

Thank you for your review.

>> This is identical to drm_sched_resubmit_jobs in all but the 'int max' handling, can't you reuse drm_sched_resubmit_jobs by passing max argument==1 for the find guilty run and then, for the later normal run passing max==MAX_INT to avoid break the iteration to early ?

Due to C language doesn't support function overloading, we couldn’t define function like the following(compiler error):
void drm_sched_resubmit_jobs(struct drm_gpu_scheduler *sched, int max = INT_MAX);

We have to rewrite every vender's  code where call the drm_sched_resubmit_jobs if we defined the function like this.
void drm_sched_resubmit_jobs(struct drm_gpu_scheduler *sched, int max);

Meanwhile, I also explored the Variable Arguments in C, But it still cannot meet the needs very well.

Do you know some other methods that could help us to make this happen?
Otherwise, I'm afraid we have to define a separate ways as patch V6 did.

Thanks,
Jack

-----Original Message-----
From: Grodzovsky, Andrey <Andrey.Grodzovsky@amd.com>
Sent: Thursday, March 11, 2021 12:19 AM
To: Zhang, Jack (Jian) <Jack.Zhang1@amd.com>; amd-gfx@lists.freedesktop.org; Koenig, Christian <Christian.Koenig@amd.com>; Liu, Monk <Monk.Liu@amd.com>; Deng, Emily <Emily.Deng@amd.com>
Subject: Re: [PATCH v6] drm/amd/amdgpu implement tdr advanced mode



On 2021-03-10 8:33 a.m., Jack Zhang wrote:
> [Why]
> Previous tdr design treats the first job in job_timeout as the bad job.
> But sometimes a later bad compute job can block a good gfx job and
> cause an unexpected gfx job timeout because gfx and compute ring share
> internal GC HW mutually.
>
> [How]
> This patch implements an advanced tdr mode.It involves an additinal
> synchronous pre-resubmit step(Step0 Resubmit) before normal resubmit
> step in order to find the real bad job.
>
> 1. At Step0 Resubmit stage, it synchronously submits and pends for the
> first job being signaled. If it gets timeout, we identify it as guilty
> and do hw reset. After that, we would do the normal resubmit step to
> resubmit left jobs.
>
> 2. Re-insert Bailing job to mirror_list, and leave it to be handled by
> the main reset thread.
>
> 3. For whole gpu reset(vram lost), do resubmit as the old way.
>
> Signed-off-by: Jack Zhang <Jack.Zhang1@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 72 ++++++++++++++++++++-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c    |  2 +-
>   drivers/gpu/drm/scheduler/sched_main.c     | 75 ++++++++++++++++++++++
>   include/drm/gpu_scheduler.h                |  2 +
>   4 files changed, 148 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index e247c3a2ec08..dac0a242e5f5 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -4639,6 +4639,7 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
>   int i, r = 0;
>   bool need_emergency_restart = false;
>   bool audio_suspended = false;
> +int tmp_vram_lost_counter;
>
>   /*
>    * Special case: RAS triggered and full reset isn't supported @@
> -4689,9 +4690,14 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
>   dev_info(adev->dev, "Bailing on TDR for s_job:%llx, as another already in progress",
>   job ? job->base.id : -1);
>
> -/* even we skipped this reset, still need to set the job to guilty */
> -if (job)
> +if (job) {
> +/* re-insert node to avoid memory leak */
> +spin_lock(&job->base.sched->job_list_lock);
> +list_add(&job->base.node, &job->base.sched->pending_list);
> +spin_unlock(&job->base.sched->job_list_lock);
> +/* even we skipped this reset, still need to set the job to guilty
> +*/
>   drm_sched_increase_karma(&job->base);
> +}

I think this piece should be in a standalone patch as it comes to fix a bug of leaking jobs and not directly related to find actual guilty job. Also, this is not the only place where the problem arises - it also arises in other drivers where they check that guilty job's fence already signaled and bail out before reinsertion of bad job to mirror list. Seems to me better fix would be to handle this within scheduler code by adding specific return value to drm_sched_backend_ops.timedout_job marking that the code terminated early - before calling drm_sched_stop and so drm_sched_job_timedout would know this and then reinsert the job back to mirror_list itself.

>   goto skip_recovery;
>   }
>
> @@ -4788,6 +4794,7 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
>   }
>   }
>
> +tmp_vram_lost_counter = atomic_read(&((adev)->vram_lost_counter));
>   /* Actual ASIC resets if needed.*/
>   /* TODO Implement XGMI hive reset logic for SRIOV */
>   if (amdgpu_sriov_vf(adev)) {
> @@ -4805,6 +4812,67 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
>   /* Post ASIC reset for all devs .*/
>   list_for_each_entry(tmp_adev, device_list_handle, gmc.xgmi.head) {
>
> +if (amdgpu_gpu_recovery == 2 &&
> +!(tmp_vram_lost_counter < atomic_read(&adev->vram_lost_counter)))
> +{
> +
> +for (i = 0; i < AMDGPU_MAX_RINGS; ++i) {
> +struct amdgpu_ring *ring = tmp_adev->rings[i];
> +int ret = 0;
> +struct drm_sched_job *s_job;
> +
> +if (!ring || !ring->sched.thread)
> +continue;
> +
> +/* No point to resubmit jobs if we didn't HW reset*/
> +if (!tmp_adev->asic_reset_res && !job_signaled) {
> +
> +s_job = list_first_entry_or_null(&ring->sched.pending_list, struct drm_sched_job, list);
> +if (s_job == NULL)
> +continue;
> +
> +/* clear job's guilty and depend the folowing step to decide the real one */
> +drm_sched_reset_karma(s_job);
> +drm_sched_resubmit_jobs2(&ring->sched, 1);
> +ret = dma_fence_wait_timeout(s_job->s_fence->parent, false,
> +ring->sched.timeout);
> +
> +if (ret == 0) { /* timeout */
> +DRM_ERROR("Found the real bad job! ring:%s, job_id:%llx\n", ring->sched.name, s_job->id);
> +/* set guilty */
> +drm_sched_increase_karma(s_job);
> +
> +/* do hw reset */
> +if (amdgpu_sriov_vf(adev)) {
> +amdgpu_virt_fini_data_exchange(adev);
> +r = amdgpu_device_reset_sriov(adev, false);
> +if (r)
> +adev->asic_reset_res = r;
> +} else {
> +r  = amdgpu_do_asic_reset(hive, device_list_handle, &need_full_reset, false);
> +if (r && r == -EAGAIN)
> +goto retry;
> +}
> +
> +/* add reset counter so that the following resubmitted job could flush vmid */
> +atomic_inc(&tmp_adev->gpu_reset_counter);
> +continue;
> +}
> +
> +/* got the hw fence, signal finished fence */
> +atomic_dec(&ring->sched.num_jobs);
> +dma_fence_get(&s_job->s_fence->finished);
> +dma_fence_signal(&s_job->s_fence->finished);
> +dma_fence_put(&s_job->s_fence->finished);
> +
> +
> +/* remove node from list and free the job */
> +spin_lock(&ring->sched.job_list_lock);
> +list_del_init(&s_job->node);
> +spin_unlock(&ring->sched.job_list_lock);
> +ring->sched.ops->free_job(s_job);
> +}
> +}
> +}
> +
>   for (i = 0; i < AMDGPU_MAX_RINGS; ++i) {
>   struct amdgpu_ring *ring = tmp_adev->rings[i];
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 865f924772b0..9c3f4edb7532 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -509,7 +509,7 @@ module_param_named(compute_multipipe, amdgpu_compute_multipipe, int, 0444);
>    * DOC: gpu_recovery (int)
>    * Set to enable GPU recovery mechanism (1 = enable, 0 = disable). The default is -1 (auto, disabled except SRIOV).
>    */
> -MODULE_PARM_DESC(gpu_recovery, "Enable GPU recovery mechanism, (1 =
> enable, 0 = disable, -1 = auto)");
> +MODULE_PARM_DESC(gpu_recovery, "Enable GPU recovery mechanism, (2 =
> +advanced tdr mode, 1 = enable, 0 = disable, -1 = auto)");
>   module_param_named(gpu_recovery, amdgpu_gpu_recovery, int, 0444);
>
>   /**
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c
> b/drivers/gpu/drm/scheduler/sched_main.c
> index d82a7ebf6099..340a193b4fb9 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -395,6 +395,81 @@ void drm_sched_increase_karma(struct drm_sched_job *bad)
>   }
>   EXPORT_SYMBOL(drm_sched_increase_karma);
>
> +
> +void drm_sched_resubmit_jobs2(struct drm_gpu_scheduler *sched, int
> +max) {
> +struct drm_sched_job *s_job, *tmp;
> +uint64_t guilty_context;
> +bool found_guilty = false;
> +struct dma_fence *fence;
> +int i = 0;
> +
> +list_for_each_entry_safe(s_job, tmp, &sched->pending_list, list) {
> +struct drm_sched_fence *s_fence = s_job->s_fence;
> +
> +if (i >= max)
> +break;
> +
> +if (!found_guilty && atomic_read(&s_job->karma) > sched->hang_limit) {
> +found_guilty = true;
> +guilty_context = s_job->s_fence->scheduled.context;
> +}
> +
> +if (found_guilty && s_job->s_fence->scheduled.context == guilty_context)
> +dma_fence_set_error(&s_fence->finished, -ECANCELED);
> +
> +dma_fence_put(s_job->s_fence->parent);
> +fence = sched->ops->run_job(s_job);
> +i++;
> +
> +if (IS_ERR_OR_NULL(fence)) {
> +if (IS_ERR(fence))
> +dma_fence_set_error(&s_fence->finished, PTR_ERR(fence));
> +
> +s_job->s_fence->parent = NULL;
> +} else {
> +s_job->s_fence->parent = fence;
> +}
> +}
> +}
> +EXPORT_SYMBOL(drm_sched_resubmit_jobs2);

This is identical to drm_sched_resubmit_jobs in all but the 'int max' handling, can't you reuse drm_sched_resubmit_jobs by passing max argument==1 for the find guilty run and then, for the later normal run passing max==MAX_INT to avoid break the iteration to early ?

> +
> +
> +
> +void drm_sched_reset_karma(struct drm_sched_job *bad) {
> +int i;
> +struct drm_sched_entity *tmp;
> +struct drm_sched_entity *entity;
> +struct drm_gpu_scheduler *sched = bad->sched;
> +
> +/* don't reset @bad's karma if it's from KERNEL RQ,
> + * because sometimes GPU hang would cause kernel jobs (like VM updating jobs)
> + * corrupt but keep in mind that kernel jobs always considered good.
> + */
> +if (bad->s_priority != DRM_SCHED_PRIORITY_KERNEL) {
> +atomic_set(&bad->karma, 0);
> +for (i = DRM_SCHED_PRIORITY_MIN; i < DRM_SCHED_PRIORITY_KERNEL;
> +     i++) {
> +struct drm_sched_rq *rq = &sched->sched_rq[i];
> +
> +spin_lock(&rq->lock);
> +list_for_each_entry_safe(entity, tmp, &rq->entities, list) {
> +if (bad->s_fence->scheduled.context ==
> +entity->fence_context) {
> +if (entity->guilty)
> +atomic_set(entity->guilty, 0);
> +break;
> +}
> +}
> +spin_unlock(&rq->lock);
> +if (&entity->list != &rq->entities)
> +break;
> +}
> +}
> +}
> +EXPORT_SYMBOL(drm_sched_reset_karma);

Same as above, very similar drm_sched_increase_karma, I would add a flag and just reuse code instead of duplucation.

Andrey

> +
>   /**
>    * drm_sched_stop - stop the scheduler
>    *
> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> index 1c815e0a14ed..01c609149731 100644
> --- a/include/drm/gpu_scheduler.h
> +++ b/include/drm/gpu_scheduler.h
> @@ -321,7 +321,9 @@ void drm_sched_wakeup(struct drm_gpu_scheduler *sched);
>   void drm_sched_stop(struct drm_gpu_scheduler *sched, struct drm_sched_job *bad);
>   void drm_sched_start(struct drm_gpu_scheduler *sched, bool full_recovery);
>   void drm_sched_resubmit_jobs(struct drm_gpu_scheduler *sched);
> +void drm_sched_resubmit_jobs2(struct drm_gpu_scheduler *sched, int
> +max);
>   void drm_sched_increase_karma(struct drm_sched_job *bad);
> +void drm_sched_reset_karma(struct drm_sched_job *bad);
>   bool drm_sched_dependency_optimized(struct dma_fence* fence,
>       struct drm_sched_entity *entity);
>   void drm_sched_fault(struct drm_gpu_scheduler *sched);
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v6] drm/amd/amdgpu implement tdr advanced mode

[Grodzovsky, Andrey]

[-- Attachment #1.1: Type: text/plain, Size: 12599 bytes --]

You can just rename drm_sched_resubmit_jobs to drm_sched_resubmit_jobs_imp and create a wrapper function drm_sched_resubmit_jobs which will call that function with MAX_INT and so no need for code change in other driver will be needed. For amdgpu you call directly drm_sched_resubmit_jobs_imp with 1 and MAX_INT.

Andrey

________________________________
From: Zhang, Jack (Jian) <Jack.Zhang1@amd.com>
Sent: 10 March 2021 22:05
To: Grodzovsky, Andrey <Andrey.Grodzovsky@amd.com>; amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>; Koenig, Christian <Christian.Koenig@amd.com>; Liu, Monk <Monk.Liu@amd.com>; Deng, Emily <Emily.Deng@amd.com>
Subject: RE: [PATCH v6] drm/amd/amdgpu implement tdr advanced mode

[AMD Official Use Only - Internal Distribution Only]

Hi, Andrey,

Thank you for your review.

>> This is identical to drm_sched_resubmit_jobs in all but the 'int max' handling, can't you reuse drm_sched_resubmit_jobs by passing max argument==1 for the find guilty run and then, for the later normal run passing max==MAX_INT to avoid break the iteration to early ?

Due to C language doesn't support function overloading, we couldn’t define function like the following(compiler error):
void drm_sched_resubmit_jobs(struct drm_gpu_scheduler *sched, int max = INT_MAX);

We have to rewrite every vender's  code where call the drm_sched_resubmit_jobs if we defined the function like this.
void drm_sched_resubmit_jobs(struct drm_gpu_scheduler *sched, int max);

Meanwhile, I also explored the Variable Arguments in C, But it still cannot meet the needs very well.

Do you know some other methods that could help us to make this happen?
Otherwise, I'm afraid we have to define a separate ways as patch V6 did.

Thanks,
Jack

-----Original Message-----
From: Grodzovsky, Andrey <Andrey.Grodzovsky@amd.com>
Sent: Thursday, March 11, 2021 12:19 AM
To: Zhang, Jack (Jian) <Jack.Zhang1@amd.com>; amd-gfx@lists.freedesktop.org; Koenig, Christian <Christian.Koenig@amd.com>; Liu, Monk <Monk.Liu@amd.com>; Deng, Emily <Emily.Deng@amd.com>
Subject: Re: [PATCH v6] drm/amd/amdgpu implement tdr advanced mode



On 2021-03-10 8:33 a.m., Jack Zhang wrote:
> [Why]
> Previous tdr design treats the first job in job_timeout as the bad job.
> But sometimes a later bad compute job can block a good gfx job and
> cause an unexpected gfx job timeout because gfx and compute ring share
> internal GC HW mutually.
>
> [How]
> This patch implements an advanced tdr mode.It involves an additinal
> synchronous pre-resubmit step(Step0 Resubmit) before normal resubmit
> step in order to find the real bad job.
>
> 1. At Step0 Resubmit stage, it synchronously submits and pends for the
> first job being signaled. If it gets timeout, we identify it as guilty
> and do hw reset. After that, we would do the normal resubmit step to
> resubmit left jobs.
>
> 2. Re-insert Bailing job to mirror_list, and leave it to be handled by
> the main reset thread.
>
> 3. For whole gpu reset(vram lost), do resubmit as the old way.
>
> Signed-off-by: Jack Zhang <Jack.Zhang1@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 72 ++++++++++++++++++++-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c    |  2 +-
>   drivers/gpu/drm/scheduler/sched_main.c     | 75 ++++++++++++++++++++++
>   include/drm/gpu_scheduler.h                |  2 +
>   4 files changed, 148 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index e247c3a2ec08..dac0a242e5f5 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -4639,6 +4639,7 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
>   int i, r = 0;
>   bool need_emergency_restart = false;
>   bool audio_suspended = false;
> +int tmp_vram_lost_counter;
>
>   /*
>    * Special case: RAS triggered and full reset isn't supported @@
> -4689,9 +4690,14 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
>   dev_info(adev->dev, "Bailing on TDR for s_job:%llx, as another already in progress",
>   job ? job->base.id : -1);
>
> -/* even we skipped this reset, still need to set the job to guilty */
> -if (job)
> +if (job) {
> +/* re-insert node to avoid memory leak */
> +spin_lock(&job->base.sched->job_list_lock);
> +list_add(&job->base.node, &job->base.sched->pending_list);
> +spin_unlock(&job->base.sched->job_list_lock);
> +/* even we skipped this reset, still need to set the job to guilty
> +*/
>   drm_sched_increase_karma(&job->base);
> +}

I think this piece should be in a standalone patch as it comes to fix a bug of leaking jobs and not directly related to find actual guilty job. Also, this is not the only place where the problem arises - it also arises in other drivers where they check that guilty job's fence already signaled and bail out before reinsertion of bad job to mirror list. Seems to me better fix would be to handle this within scheduler code by adding specific return value to drm_sched_backend_ops.timedout_job marking that the code terminated early - before calling drm_sched_stop and so drm_sched_job_timedout would know this and then reinsert the job back to mirror_list itself.

>   goto skip_recovery;
>   }
>
> @@ -4788,6 +4794,7 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
>   }
>   }
>
> +tmp_vram_lost_counter = atomic_read(&((adev)->vram_lost_counter));
>   /* Actual ASIC resets if needed.*/
>   /* TODO Implement XGMI hive reset logic for SRIOV */
>   if (amdgpu_sriov_vf(adev)) {
> @@ -4805,6 +4812,67 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
>   /* Post ASIC reset for all devs .*/
>   list_for_each_entry(tmp_adev, device_list_handle, gmc.xgmi.head) {
>
> +if (amdgpu_gpu_recovery == 2 &&
> +!(tmp_vram_lost_counter < atomic_read(&adev->vram_lost_counter)))
> +{
> +
> +for (i = 0; i < AMDGPU_MAX_RINGS; ++i) {
> +struct amdgpu_ring *ring = tmp_adev->rings[i];
> +int ret = 0;
> +struct drm_sched_job *s_job;
> +
> +if (!ring || !ring->sched.thread)
> +continue;
> +
> +/* No point to resubmit jobs if we didn't HW reset*/
> +if (!tmp_adev->asic_reset_res && !job_signaled) {
> +
> +s_job = list_first_entry_or_null(&ring->sched.pending_list, struct drm_sched_job, list);
> +if (s_job == NULL)
> +continue;
> +
> +/* clear job's guilty and depend the folowing step to decide the real one */
> +drm_sched_reset_karma(s_job);
> +drm_sched_resubmit_jobs2(&ring->sched, 1);
> +ret = dma_fence_wait_timeout(s_job->s_fence->parent, false,
> +ring->sched.timeout);
> +
> +if (ret == 0) { /* timeout */
> +DRM_ERROR("Found the real bad job! ring:%s, job_id:%llx\n", ring->sched.name, s_job->id);
> +/* set guilty */
> +drm_sched_increase_karma(s_job);
> +
> +/* do hw reset */
> +if (amdgpu_sriov_vf(adev)) {
> +amdgpu_virt_fini_data_exchange(adev);
> +r = amdgpu_device_reset_sriov(adev, false);
> +if (r)
> +adev->asic_reset_res = r;
> +} else {
> +r  = amdgpu_do_asic_reset(hive, device_list_handle, &need_full_reset, false);
> +if (r && r == -EAGAIN)
> +goto retry;
> +}
> +
> +/* add reset counter so that the following resubmitted job could flush vmid */
> +atomic_inc(&tmp_adev->gpu_reset_counter);
> +continue;
> +}
> +
> +/* got the hw fence, signal finished fence */
> +atomic_dec(&ring->sched.num_jobs);
> +dma_fence_get(&s_job->s_fence->finished);
> +dma_fence_signal(&s_job->s_fence->finished);
> +dma_fence_put(&s_job->s_fence->finished);
> +
> +
> +/* remove node from list and free the job */
> +spin_lock(&ring->sched.job_list_lock);
> +list_del_init(&s_job->node);
> +spin_unlock(&ring->sched.job_list_lock);
> +ring->sched.ops->free_job(s_job);
> +}
> +}
> +}
> +
>   for (i = 0; i < AMDGPU_MAX_RINGS; ++i) {
>   struct amdgpu_ring *ring = tmp_adev->rings[i];
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 865f924772b0..9c3f4edb7532 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -509,7 +509,7 @@ module_param_named(compute_multipipe, amdgpu_compute_multipipe, int, 0444);
>    * DOC: gpu_recovery (int)
>    * Set to enable GPU recovery mechanism (1 = enable, 0 = disable). The default is -1 (auto, disabled except SRIOV).
>    */
> -MODULE_PARM_DESC(gpu_recovery, "Enable GPU recovery mechanism, (1 =
> enable, 0 = disable, -1 = auto)");
> +MODULE_PARM_DESC(gpu_recovery, "Enable GPU recovery mechanism, (2 =
> +advanced tdr mode, 1 = enable, 0 = disable, -1 = auto)");
>   module_param_named(gpu_recovery, amdgpu_gpu_recovery, int, 0444);
>
>   /**
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c
> b/drivers/gpu/drm/scheduler/sched_main.c
> index d82a7ebf6099..340a193b4fb9 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -395,6 +395,81 @@ void drm_sched_increase_karma(struct drm_sched_job *bad)
>   }
>   EXPORT_SYMBOL(drm_sched_increase_karma);
>
> +
> +void drm_sched_resubmit_jobs2(struct drm_gpu_scheduler *sched, int
> +max) {
> +struct drm_sched_job *s_job, *tmp;
> +uint64_t guilty_context;
> +bool found_guilty = false;
> +struct dma_fence *fence;
> +int i = 0;
> +
> +list_for_each_entry_safe(s_job, tmp, &sched->pending_list, list) {
> +struct drm_sched_fence *s_fence = s_job->s_fence;
> +
> +if (i >= max)
> +break;
> +
> +if (!found_guilty && atomic_read(&s_job->karma) > sched->hang_limit) {
> +found_guilty = true;
> +guilty_context = s_job->s_fence->scheduled.context;
> +}
> +
> +if (found_guilty && s_job->s_fence->scheduled.context == guilty_context)
> +dma_fence_set_error(&s_fence->finished, -ECANCELED);
> +
> +dma_fence_put(s_job->s_fence->parent);
> +fence = sched->ops->run_job(s_job);
> +i++;
> +
> +if (IS_ERR_OR_NULL(fence)) {
> +if (IS_ERR(fence))
> +dma_fence_set_error(&s_fence->finished, PTR_ERR(fence));
> +
> +s_job->s_fence->parent = NULL;
> +} else {
> +s_job->s_fence->parent = fence;
> +}
> +}
> +}
> +EXPORT_SYMBOL(drm_sched_resubmit_jobs2);

This is identical to drm_sched_resubmit_jobs in all but the 'int max' handling, can't you reuse drm_sched_resubmit_jobs by passing max argument==1 for the find guilty run and then, for the later normal run passing max==MAX_INT to avoid break the iteration to early ?

> +
> +
> +
> +void drm_sched_reset_karma(struct drm_sched_job *bad) {
> +int i;
> +struct drm_sched_entity *tmp;
> +struct drm_sched_entity *entity;
> +struct drm_gpu_scheduler *sched = bad->sched;
> +
> +/* don't reset @bad's karma if it's from KERNEL RQ,
> + * because sometimes GPU hang would cause kernel jobs (like VM updating jobs)
> + * corrupt but keep in mind that kernel jobs always considered good.
> + */
> +if (bad->s_priority != DRM_SCHED_PRIORITY_KERNEL) {
> +atomic_set(&bad->karma, 0);
> +for (i = DRM_SCHED_PRIORITY_MIN; i < DRM_SCHED_PRIORITY_KERNEL;
> +     i++) {
> +struct drm_sched_rq *rq = &sched->sched_rq[i];
> +
> +spin_lock(&rq->lock);
> +list_for_each_entry_safe(entity, tmp, &rq->entities, list) {
> +if (bad->s_fence->scheduled.context ==
> +entity->fence_context) {
> +if (entity->guilty)
> +atomic_set(entity->guilty, 0);
> +break;
> +}
> +}
> +spin_unlock(&rq->lock);
> +if (&entity->list != &rq->entities)
> +break;
> +}
> +}
> +}
> +EXPORT_SYMBOL(drm_sched_reset_karma);

Same as above, very similar drm_sched_increase_karma, I would add a flag and just reuse code instead of duplucation.

Andrey

> +
>   /**
>    * drm_sched_stop - stop the scheduler
>    *
> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> index 1c815e0a14ed..01c609149731 100644
> --- a/include/drm/gpu_scheduler.h
> +++ b/include/drm/gpu_scheduler.h
> @@ -321,7 +321,9 @@ void drm_sched_wakeup(struct drm_gpu_scheduler *sched);
>   void drm_sched_stop(struct drm_gpu_scheduler *sched, struct drm_sched_job *bad);
>   void drm_sched_start(struct drm_gpu_scheduler *sched, bool full_recovery);
>   void drm_sched_resubmit_jobs(struct drm_gpu_scheduler *sched);
> +void drm_sched_resubmit_jobs2(struct drm_gpu_scheduler *sched, int
> +max);
>   void drm_sched_increase_karma(struct drm_sched_job *bad);
> +void drm_sched_reset_karma(struct drm_sched_job *bad);
>   bool drm_sched_dependency_optimized(struct dma_fence* fence,
>       struct drm_sched_entity *entity);
>   void drm_sched_fault(struct drm_gpu_scheduler *sched);
>

[-- Attachment #1.2: Type: text/html, Size: 17749 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

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

RE: [PATCH v6] drm/amd/amdgpu implement tdr advanced mode

[Zhang, Jack (Jian)]

[-- Attachment #1.1: Type: text/plain, Size: 13493 bytes --]

[AMD Official Use Only - Internal Distribution Only]

Ok, that will do the trick. Thank you!

Jack
From: Grodzovsky, Andrey <Andrey.Grodzovsky@amd.com>
Sent: Thursday, March 11, 2021 11:24 AM
To: Zhang, Jack (Jian) <Jack.Zhang1@amd.com>; amd-gfx@lists.freedesktop.org; Koenig, Christian <Christian.Koenig@amd.com>; Liu, Monk <Monk.Liu@amd.com>; Deng, Emily <Emily.Deng@amd.com>
Subject: Re: [PATCH v6] drm/amd/amdgpu implement tdr advanced mode

You can just rename drm_sched_resubmit_jobs to drm_sched_resubmit_jobs_imp and create a wrapper function drm_sched_resubmit_jobs which will call that function with MAX_INT and so no need for code change in other driver will be needed. For amdgpu you call directly drm_sched_resubmit_jobs_imp with 1 and MAX_INT.

Andrey

________________________________
From: Zhang, Jack (Jian) <Jack.Zhang1@amd.com<mailto:Jack.Zhang1@amd.com>>
Sent: 10 March 2021 22:05
To: Grodzovsky, Andrey <Andrey.Grodzovsky@amd.com<mailto:Andrey.Grodzovsky@amd.com>>; amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org> <amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>>; Koenig, Christian <Christian.Koenig@amd.com<mailto:Christian.Koenig@amd.com>>; Liu, Monk <Monk.Liu@amd.com<mailto:Monk.Liu@amd.com>>; Deng, Emily <Emily.Deng@amd.com<mailto:Emily.Deng@amd.com>>
Subject: RE: [PATCH v6] drm/amd/amdgpu implement tdr advanced mode

[AMD Official Use Only - Internal Distribution Only]

Hi, Andrey,

Thank you for your review.

>> This is identical to drm_sched_resubmit_jobs in all but the 'int max' handling, can't you reuse drm_sched_resubmit_jobs by passing max argument==1 for the find guilty run and then, for the later normal run passing max==MAX_INT to avoid break the iteration to early ?

Due to C language doesn't support function overloading, we couldn’t define function like the following(compiler error):
void drm_sched_resubmit_jobs(struct drm_gpu_scheduler *sched, int max = INT_MAX);

We have to rewrite every vender's  code where call the drm_sched_resubmit_jobs if we defined the function like this.
void drm_sched_resubmit_jobs(struct drm_gpu_scheduler *sched, int max);

Meanwhile, I also explored the Variable Arguments in C, But it still cannot meet the needs very well.

Do you know some other methods that could help us to make this happen?
Otherwise, I'm afraid we have to define a separate ways as patch V6 did.

Thanks,
Jack

-----Original Message-----
From: Grodzovsky, Andrey <Andrey.Grodzovsky@amd.com<mailto:Andrey.Grodzovsky@amd.com>>
Sent: Thursday, March 11, 2021 12:19 AM
To: Zhang, Jack (Jian) <Jack.Zhang1@amd.com<mailto:Jack.Zhang1@amd.com>>; amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>; Koenig, Christian <Christian.Koenig@amd.com<mailto:Christian.Koenig@amd.com>>; Liu, Monk <Monk.Liu@amd.com<mailto:Monk.Liu@amd.com>>; Deng, Emily <Emily.Deng@amd.com<mailto:Emily.Deng@amd.com>>
Subject: Re: [PATCH v6] drm/amd/amdgpu implement tdr advanced mode



On 2021-03-10 8:33 a.m., Jack Zhang wrote:
> [Why]
> Previous tdr design treats the first job in job_timeout as the bad job.
> But sometimes a later bad compute job can block a good gfx job and
> cause an unexpected gfx job timeout because gfx and compute ring share
> internal GC HW mutually.
>
> [How]
> This patch implements an advanced tdr mode.It involves an additinal
> synchronous pre-resubmit step(Step0 Resubmit) before normal resubmit
> step in order to find the real bad job.
>
> 1. At Step0 Resubmit stage, it synchronously submits and pends for the
> first job being signaled. If it gets timeout, we identify it as guilty
> and do hw reset. After that, we would do the normal resubmit step to
> resubmit left jobs.
>
> 2. Re-insert Bailing job to mirror_list, and leave it to be handled by
> the main reset thread.
>
> 3. For whole gpu reset(vram lost), do resubmit as the old way.
>
> Signed-off-by: Jack Zhang <Jack.Zhang1@amd.com<mailto:Jack.Zhang1@amd.com>>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 72 ++++++++++++++++++++-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c    |  2 +-
>   drivers/gpu/drm/scheduler/sched_main.c     | 75 ++++++++++++++++++++++
>   include/drm/gpu_scheduler.h                |  2 +
>   4 files changed, 148 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index e247c3a2ec08..dac0a242e5f5 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -4639,6 +4639,7 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
>   int i, r = 0;
>   bool need_emergency_restart = false;
>   bool audio_suspended = false;
> +int tmp_vram_lost_counter;
>
>   /*
>    * Special case: RAS triggered and full reset isn't supported @@
> -4689,9 +4690,14 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
>   dev_info(adev->dev, "Bailing on TDR for s_job:%llx, as another already in progress",
>   job ? job->base.id : -1);
>
> -/* even we skipped this reset, still need to set the job to guilty */
> -if (job)
> +if (job) {
> +/* re-insert node to avoid memory leak */
> +spin_lock(&job->base.sched->job_list_lock);
> +list_add(&job->base.node, &job->base.sched->pending_list);
> +spin_unlock(&job->base.sched->job_list_lock);
> +/* even we skipped this reset, still need to set the job to guilty
> +*/
>   drm_sched_increase_karma(&job->base);
> +}

I think this piece should be in a standalone patch as it comes to fix a bug of leaking jobs and not directly related to find actual guilty job. Also, this is not the only place where the problem arises - it also arises in other drivers where they check that guilty job's fence already signaled and bail out before reinsertion of bad job to mirror list. Seems to me better fix would be to handle this within scheduler code by adding specific return value to drm_sched_backend_ops.timedout_job marking that the code terminated early - before calling drm_sched_stop and so drm_sched_job_timedout would know this and then reinsert the job back to mirror_list itself.

>   goto skip_recovery;
>   }
>
> @@ -4788,6 +4794,7 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
>   }
>   }
>
> +tmp_vram_lost_counter = atomic_read(&((adev)->vram_lost_counter));
>   /* Actual ASIC resets if needed.*/
>   /* TODO Implement XGMI hive reset logic for SRIOV */
>   if (amdgpu_sriov_vf(adev)) {
> @@ -4805,6 +4812,67 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
>   /* Post ASIC reset for all devs .*/
>   list_for_each_entry(tmp_adev, device_list_handle, gmc.xgmi.head) {
>
> +if (amdgpu_gpu_recovery == 2 &&
> +!(tmp_vram_lost_counter < atomic_read(&adev->vram_lost_counter)))
> +{
> +
> +for (i = 0; i < AMDGPU_MAX_RINGS; ++i) {
> +struct amdgpu_ring *ring = tmp_adev->rings[i];
> +int ret = 0;
> +struct drm_sched_job *s_job;
> +
> +if (!ring || !ring->sched.thread)
> +continue;
> +
> +/* No point to resubmit jobs if we didn't HW reset*/
> +if (!tmp_adev->asic_reset_res && !job_signaled) {
> +
> +s_job = list_first_entry_or_null(&ring->sched.pending_list, struct drm_sched_job, list);
> +if (s_job == NULL)
> +continue;
> +
> +/* clear job's guilty and depend the folowing step to decide the real one */
> +drm_sched_reset_karma(s_job);
> +drm_sched_resubmit_jobs2(&ring->sched, 1);
> +ret = dma_fence_wait_timeout(s_job->s_fence->parent, false,
> +ring->sched.timeout);
> +
> +if (ret == 0) { /* timeout */
> +DRM_ERROR("Found the real bad job! ring:%s, job_id:%llx\n", ring->sched.name, s_job->id);
> +/* set guilty */
> +drm_sched_increase_karma(s_job);
> +
> +/* do hw reset */
> +if (amdgpu_sriov_vf(adev)) {
> +amdgpu_virt_fini_data_exchange(adev);
> +r = amdgpu_device_reset_sriov(adev, false);
> +if (r)
> +adev->asic_reset_res = r;
> +} else {
> +r  = amdgpu_do_asic_reset(hive, device_list_handle, &need_full_reset, false);
> +if (r && r == -EAGAIN)
> +goto retry;
> +}
> +
> +/* add reset counter so that the following resubmitted job could flush vmid */
> +atomic_inc(&tmp_adev->gpu_reset_counter);
> +continue;
> +}
> +
> +/* got the hw fence, signal finished fence */
> +atomic_dec(&ring->sched.num_jobs);
> +dma_fence_get(&s_job->s_fence->finished);
> +dma_fence_signal(&s_job->s_fence->finished);
> +dma_fence_put(&s_job->s_fence->finished);
> +
> +
> +/* remove node from list and free the job */
> +spin_lock(&ring->sched.job_list_lock);
> +list_del_init(&s_job->node);
> +spin_unlock(&ring->sched.job_list_lock);
> +ring->sched.ops->free_job(s_job);
> +}
> +}
> +}
> +
>   for (i = 0; i < AMDGPU_MAX_RINGS; ++i) {
>   struct amdgpu_ring *ring = tmp_adev->rings[i];
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 865f924772b0..9c3f4edb7532 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -509,7 +509,7 @@ module_param_named(compute_multipipe, amdgpu_compute_multipipe, int, 0444);
>    * DOC: gpu_recovery (int)
>    * Set to enable GPU recovery mechanism (1 = enable, 0 = disable). The default is -1 (auto, disabled except SRIOV).
>    */
> -MODULE_PARM_DESC(gpu_recovery, "Enable GPU recovery mechanism, (1 =
> enable, 0 = disable, -1 = auto)");
> +MODULE_PARM_DESC(gpu_recovery, "Enable GPU recovery mechanism, (2 =
> +advanced tdr mode, 1 = enable, 0 = disable, -1 = auto)");
>   module_param_named(gpu_recovery, amdgpu_gpu_recovery, int, 0444);
>
>   /**
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c
> b/drivers/gpu/drm/scheduler/sched_main.c
> index d82a7ebf6099..340a193b4fb9 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -395,6 +395,81 @@ void drm_sched_increase_karma(struct drm_sched_job *bad)
>   }
>   EXPORT_SYMBOL(drm_sched_increase_karma);
>
> +
> +void drm_sched_resubmit_jobs2(struct drm_gpu_scheduler *sched, int
> +max) {
> +struct drm_sched_job *s_job, *tmp;
> +uint64_t guilty_context;
> +bool found_guilty = false;
> +struct dma_fence *fence;
> +int i = 0;
> +
> +list_for_each_entry_safe(s_job, tmp, &sched->pending_list, list) {
> +struct drm_sched_fence *s_fence = s_job->s_fence;
> +
> +if (i >= max)
> +break;
> +
> +if (!found_guilty && atomic_read(&s_job->karma) > sched->hang_limit) {
> +found_guilty = true;
> +guilty_context = s_job->s_fence->scheduled.context;
> +}
> +
> +if (found_guilty && s_job->s_fence->scheduled.context == guilty_context)
> +dma_fence_set_error(&s_fence->finished, -ECANCELED);
> +
> +dma_fence_put(s_job->s_fence->parent);
> +fence = sched->ops->run_job(s_job);
> +i++;
> +
> +if (IS_ERR_OR_NULL(fence)) {
> +if (IS_ERR(fence))
> +dma_fence_set_error(&s_fence->finished, PTR_ERR(fence));
> +
> +s_job->s_fence->parent = NULL;
> +} else {
> +s_job->s_fence->parent = fence;
> +}
> +}
> +}
> +EXPORT_SYMBOL(drm_sched_resubmit_jobs2);

This is identical to drm_sched_resubmit_jobs in all but the 'int max' handling, can't you reuse drm_sched_resubmit_jobs by passing max argument==1 for the find guilty run and then, for the later normal run passing max==MAX_INT to avoid break the iteration to early ?

> +
> +
> +
> +void drm_sched_reset_karma(struct drm_sched_job *bad) {
> +int i;
> +struct drm_sched_entity *tmp;
> +struct drm_sched_entity *entity;
> +struct drm_gpu_scheduler *sched = bad->sched;
> +
> +/* don't reset @bad's karma if it's from KERNEL RQ,
> + * because sometimes GPU hang would cause kernel jobs (like VM updating jobs)
> + * corrupt but keep in mind that kernel jobs always considered good.
> + */
> +if (bad->s_priority != DRM_SCHED_PRIORITY_KERNEL) {
> +atomic_set(&bad->karma, 0);
> +for (i = DRM_SCHED_PRIORITY_MIN; i < DRM_SCHED_PRIORITY_KERNEL;
> +     i++) {
> +struct drm_sched_rq *rq = &sched->sched_rq[i];
> +
> +spin_lock(&rq->lock);
> +list_for_each_entry_safe(entity, tmp, &rq->entities, list) {
> +if (bad->s_fence->scheduled.context ==
> +entity->fence_context) {
> +if (entity->guilty)
> +atomic_set(entity->guilty, 0);
> +break;
> +}
> +}
> +spin_unlock(&rq->lock);
> +if (&entity->list != &rq->entities)
> +break;
> +}
> +}
> +}
> +EXPORT_SYMBOL(drm_sched_reset_karma);

Same as above, very similar drm_sched_increase_karma, I would add a flag and just reuse code instead of duplucation.

Andrey

> +
>   /**
>    * drm_sched_stop - stop the scheduler
>    *
> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> index 1c815e0a14ed..01c609149731 100644
> --- a/include/drm/gpu_scheduler.h
> +++ b/include/drm/gpu_scheduler.h
> @@ -321,7 +321,9 @@ void drm_sched_wakeup(struct drm_gpu_scheduler *sched);
>   void drm_sched_stop(struct drm_gpu_scheduler *sched, struct drm_sched_job *bad);
>   void drm_sched_start(struct drm_gpu_scheduler *sched, bool full_recovery);
>   void drm_sched_resubmit_jobs(struct drm_gpu_scheduler *sched);
> +void drm_sched_resubmit_jobs2(struct drm_gpu_scheduler *sched, int
> +max);
>   void drm_sched_increase_karma(struct drm_sched_job *bad);
> +void drm_sched_reset_karma(struct drm_sched_job *bad);
>   bool drm_sched_dependency_optimized(struct dma_fence* fence,
>       struct drm_sched_entity *entity);
>   void drm_sched_fault(struct drm_gpu_scheduler *sched);
>

[-- Attachment #1.2: Type: text/html, Size: 20340 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

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

RE: [PATCH v6] drm/amd/amdgpu implement tdr advanced mode

[Zhang, Jack (Jian)]

[-- Attachment #1.1: Type: text/plain, Size: 14314 bytes --]

[AMD Official Use Only - Internal Distribution Only]

Hi, Andrey,

I just submitted the V7 patch in which I wrapped the increase_karma func and resubmit_job func.

Thanks,
Jack
From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Zhang, Jack (Jian)
Sent: Thursday, March 11, 2021 11:28 AM
To: Grodzovsky, Andrey <Andrey.Grodzovsky@amd.com>; amd-gfx@lists.freedesktop.org; Koenig, Christian <Christian.Koenig@amd.com>; Liu, Monk <Monk.Liu@amd.com>; Deng, Emily <Emily.Deng@amd.com>
Subject: RE: [PATCH v6] drm/amd/amdgpu implement tdr advanced mode


[AMD Official Use Only - Internal Distribution Only]


[AMD Official Use Only - Internal Distribution Only]

Ok, that will do the trick. Thank you!

Jack
From: Grodzovsky, Andrey <Andrey.Grodzovsky@amd.com<mailto:Andrey.Grodzovsky@amd.com>>
Sent: Thursday, March 11, 2021 11:24 AM
To: Zhang, Jack (Jian) <Jack.Zhang1@amd.com<mailto:Jack.Zhang1@amd.com>>; amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>; Koenig, Christian <Christian.Koenig@amd.com<mailto:Christian.Koenig@amd.com>>; Liu, Monk <Monk.Liu@amd.com<mailto:Monk.Liu@amd.com>>; Deng, Emily <Emily.Deng@amd.com<mailto:Emily.Deng@amd.com>>
Subject: Re: [PATCH v6] drm/amd/amdgpu implement tdr advanced mode

You can just rename drm_sched_resubmit_jobs to drm_sched_resubmit_jobs_imp and create a wrapper function drm_sched_resubmit_jobs which will call that function with MAX_INT and so no need for code change in other driver will be needed. For amdgpu you call directly drm_sched_resubmit_jobs_imp with 1 and MAX_INT.

Andrey

________________________________
From: Zhang, Jack (Jian) <Jack.Zhang1@amd.com<mailto:Jack.Zhang1@amd.com>>
Sent: 10 March 2021 22:05
To: Grodzovsky, Andrey <Andrey.Grodzovsky@amd.com<mailto:Andrey.Grodzovsky@amd.com>>; amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org> <amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>>; Koenig, Christian <Christian.Koenig@amd.com<mailto:Christian.Koenig@amd.com>>; Liu, Monk <Monk.Liu@amd.com<mailto:Monk.Liu@amd.com>>; Deng, Emily <Emily.Deng@amd.com<mailto:Emily.Deng@amd.com>>
Subject: RE: [PATCH v6] drm/amd/amdgpu implement tdr advanced mode

[AMD Official Use Only - Internal Distribution Only]

Hi, Andrey,

Thank you for your review.

>> This is identical to drm_sched_resubmit_jobs in all but the 'int max' handling, can't you reuse drm_sched_resubmit_jobs by passing max argument==1 for the find guilty run and then, for the later normal run passing max==MAX_INT to avoid break the iteration to early ?

Due to C language doesn't support function overloading, we couldn’t define function like the following(compiler error):
void drm_sched_resubmit_jobs(struct drm_gpu_scheduler *sched, int max = INT_MAX);

We have to rewrite every vender's  code where call the drm_sched_resubmit_jobs if we defined the function like this.
void drm_sched_resubmit_jobs(struct drm_gpu_scheduler *sched, int max);

Meanwhile, I also explored the Variable Arguments in C, But it still cannot meet the needs very well.

Do you know some other methods that could help us to make this happen?
Otherwise, I'm afraid we have to define a separate ways as patch V6 did.

Thanks,
Jack

-----Original Message-----
From: Grodzovsky, Andrey <Andrey.Grodzovsky@amd.com<mailto:Andrey.Grodzovsky@amd.com>>
Sent: Thursday, March 11, 2021 12:19 AM
To: Zhang, Jack (Jian) <Jack.Zhang1@amd.com<mailto:Jack.Zhang1@amd.com>>; amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>; Koenig, Christian <Christian.Koenig@amd.com<mailto:Christian.Koenig@amd.com>>; Liu, Monk <Monk.Liu@amd.com<mailto:Monk.Liu@amd.com>>; Deng, Emily <Emily.Deng@amd.com<mailto:Emily.Deng@amd.com>>
Subject: Re: [PATCH v6] drm/amd/amdgpu implement tdr advanced mode



On 2021-03-10 8:33 a.m., Jack Zhang wrote:
> [Why]
> Previous tdr design treats the first job in job_timeout as the bad job.
> But sometimes a later bad compute job can block a good gfx job and
> cause an unexpected gfx job timeout because gfx and compute ring share
> internal GC HW mutually.
>
> [How]
> This patch implements an advanced tdr mode.It involves an additinal
> synchronous pre-resubmit step(Step0 Resubmit) before normal resubmit
> step in order to find the real bad job.
>
> 1. At Step0 Resubmit stage, it synchronously submits and pends for the
> first job being signaled. If it gets timeout, we identify it as guilty
> and do hw reset. After that, we would do the normal resubmit step to
> resubmit left jobs.
>
> 2. Re-insert Bailing job to mirror_list, and leave it to be handled by
> the main reset thread.
>
> 3. For whole gpu reset(vram lost), do resubmit as the old way.
>
> Signed-off-by: Jack Zhang <Jack.Zhang1@amd.com<mailto:Jack.Zhang1@amd.com>>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 72 ++++++++++++++++++++-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c    |  2 +-
>   drivers/gpu/drm/scheduler/sched_main.c     | 75 ++++++++++++++++++++++
>   include/drm/gpu_scheduler.h                |  2 +
>   4 files changed, 148 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index e247c3a2ec08..dac0a242e5f5 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -4639,6 +4639,7 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
>   int i, r = 0;
>   bool need_emergency_restart = false;
>   bool audio_suspended = false;
> +int tmp_vram_lost_counter;
>
>   /*
>    * Special case: RAS triggered and full reset isn't supported @@
> -4689,9 +4690,14 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
>   dev_info(adev->dev, "Bailing on TDR for s_job:%llx, as another already in progress",
>   job ? job->base.id : -1);
>
> -/* even we skipped this reset, still need to set the job to guilty */
> -if (job)
> +if (job) {
> +/* re-insert node to avoid memory leak */
> +spin_lock(&job->base.sched->job_list_lock);
> +list_add(&job->base.node, &job->base.sched->pending_list);
> +spin_unlock(&job->base.sched->job_list_lock);
> +/* even we skipped this reset, still need to set the job to guilty
> +*/
>   drm_sched_increase_karma(&job->base);
> +}

I think this piece should be in a standalone patch as it comes to fix a bug of leaking jobs and not directly related to find actual guilty job. Also, this is not the only place where the problem arises - it also arises in other drivers where they check that guilty job's fence already signaled and bail out before reinsertion of bad job to mirror list. Seems to me better fix would be to handle this within scheduler code by adding specific return value to drm_sched_backend_ops.timedout_job marking that the code terminated early - before calling drm_sched_stop and so drm_sched_job_timedout would know this and then reinsert the job back to mirror_list itself.

>   goto skip_recovery;
>   }
>
> @@ -4788,6 +4794,7 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
>   }
>   }
>
> +tmp_vram_lost_counter = atomic_read(&((adev)->vram_lost_counter));
>   /* Actual ASIC resets if needed.*/
>   /* TODO Implement XGMI hive reset logic for SRIOV */
>   if (amdgpu_sriov_vf(adev)) {
> @@ -4805,6 +4812,67 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
>   /* Post ASIC reset for all devs .*/
>   list_for_each_entry(tmp_adev, device_list_handle, gmc.xgmi.head) {
>
> +if (amdgpu_gpu_recovery == 2 &&
> +!(tmp_vram_lost_counter < atomic_read(&adev->vram_lost_counter)))
> +{
> +
> +for (i = 0; i < AMDGPU_MAX_RINGS; ++i) {
> +struct amdgpu_ring *ring = tmp_adev->rings[i];
> +int ret = 0;
> +struct drm_sched_job *s_job;
> +
> +if (!ring || !ring->sched.thread)
> +continue;
> +
> +/* No point to resubmit jobs if we didn't HW reset*/
> +if (!tmp_adev->asic_reset_res && !job_signaled) {
> +
> +s_job = list_first_entry_or_null(&ring->sched.pending_list, struct drm_sched_job, list);
> +if (s_job == NULL)
> +continue;
> +
> +/* clear job's guilty and depend the folowing step to decide the real one */
> +drm_sched_reset_karma(s_job);
> +drm_sched_resubmit_jobs2(&ring->sched, 1);
> +ret = dma_fence_wait_timeout(s_job->s_fence->parent, false,
> +ring->sched.timeout);
> +
> +if (ret == 0) { /* timeout */
> +DRM_ERROR("Found the real bad job! ring:%s, job_id:%llx\n", ring->sched.name, s_job->id);
> +/* set guilty */
> +drm_sched_increase_karma(s_job);
> +
> +/* do hw reset */
> +if (amdgpu_sriov_vf(adev)) {
> +amdgpu_virt_fini_data_exchange(adev);
> +r = amdgpu_device_reset_sriov(adev, false);
> +if (r)
> +adev->asic_reset_res = r;
> +} else {
> +r  = amdgpu_do_asic_reset(hive, device_list_handle, &need_full_reset, false);
> +if (r && r == -EAGAIN)
> +goto retry;
> +}
> +
> +/* add reset counter so that the following resubmitted job could flush vmid */
> +atomic_inc(&tmp_adev->gpu_reset_counter);
> +continue;
> +}
> +
> +/* got the hw fence, signal finished fence */
> +atomic_dec(&ring->sched.num_jobs);
> +dma_fence_get(&s_job->s_fence->finished);
> +dma_fence_signal(&s_job->s_fence->finished);
> +dma_fence_put(&s_job->s_fence->finished);
> +
> +
> +/* remove node from list and free the job */
> +spin_lock(&ring->sched.job_list_lock);
> +list_del_init(&s_job->node);
> +spin_unlock(&ring->sched.job_list_lock);
> +ring->sched.ops->free_job(s_job);
> +}
> +}
> +}
> +
>   for (i = 0; i < AMDGPU_MAX_RINGS; ++i) {
>   struct amdgpu_ring *ring = tmp_adev->rings[i];
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 865f924772b0..9c3f4edb7532 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -509,7 +509,7 @@ module_param_named(compute_multipipe, amdgpu_compute_multipipe, int, 0444);
>    * DOC: gpu_recovery (int)
>    * Set to enable GPU recovery mechanism (1 = enable, 0 = disable). The default is -1 (auto, disabled except SRIOV).
>    */
> -MODULE_PARM_DESC(gpu_recovery, "Enable GPU recovery mechanism, (1 =
> enable, 0 = disable, -1 = auto)");
> +MODULE_PARM_DESC(gpu_recovery, "Enable GPU recovery mechanism, (2 =
> +advanced tdr mode, 1 = enable, 0 = disable, -1 = auto)");
>   module_param_named(gpu_recovery, amdgpu_gpu_recovery, int, 0444);
>
>   /**
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c
> b/drivers/gpu/drm/scheduler/sched_main.c
> index d82a7ebf6099..340a193b4fb9 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -395,6 +395,81 @@ void drm_sched_increase_karma(struct drm_sched_job *bad)
>   }
>   EXPORT_SYMBOL(drm_sched_increase_karma);
>
> +
> +void drm_sched_resubmit_jobs2(struct drm_gpu_scheduler *sched, int
> +max) {
> +struct drm_sched_job *s_job, *tmp;
> +uint64_t guilty_context;
> +bool found_guilty = false;
> +struct dma_fence *fence;
> +int i = 0;
> +
> +list_for_each_entry_safe(s_job, tmp, &sched->pending_list, list) {
> +struct drm_sched_fence *s_fence = s_job->s_fence;
> +
> +if (i >= max)
> +break;
> +
> +if (!found_guilty && atomic_read(&s_job->karma) > sched->hang_limit) {
> +found_guilty = true;
> +guilty_context = s_job->s_fence->scheduled.context;
> +}
> +
> +if (found_guilty && s_job->s_fence->scheduled.context == guilty_context)
> +dma_fence_set_error(&s_fence->finished, -ECANCELED);
> +
> +dma_fence_put(s_job->s_fence->parent);
> +fence = sched->ops->run_job(s_job);
> +i++;
> +
> +if (IS_ERR_OR_NULL(fence)) {
> +if (IS_ERR(fence))
> +dma_fence_set_error(&s_fence->finished, PTR_ERR(fence));
> +
> +s_job->s_fence->parent = NULL;
> +} else {
> +s_job->s_fence->parent = fence;
> +}
> +}
> +}
> +EXPORT_SYMBOL(drm_sched_resubmit_jobs2);

This is identical to drm_sched_resubmit_jobs in all but the 'int max' handling, can't you reuse drm_sched_resubmit_jobs by passing max argument==1 for the find guilty run and then, for the later normal run passing max==MAX_INT to avoid break the iteration to early ?

> +
> +
> +
> +void drm_sched_reset_karma(struct drm_sched_job *bad) {
> +int i;
> +struct drm_sched_entity *tmp;
> +struct drm_sched_entity *entity;
> +struct drm_gpu_scheduler *sched = bad->sched;
> +
> +/* don't reset @bad's karma if it's from KERNEL RQ,
> + * because sometimes GPU hang would cause kernel jobs (like VM updating jobs)
> + * corrupt but keep in mind that kernel jobs always considered good.
> + */
> +if (bad->s_priority != DRM_SCHED_PRIORITY_KERNEL) {
> +atomic_set(&bad->karma, 0);
> +for (i = DRM_SCHED_PRIORITY_MIN; i < DRM_SCHED_PRIORITY_KERNEL;
> +     i++) {
> +struct drm_sched_rq *rq = &sched->sched_rq[i];
> +
> +spin_lock(&rq->lock);
> +list_for_each_entry_safe(entity, tmp, &rq->entities, list) {
> +if (bad->s_fence->scheduled.context ==
> +entity->fence_context) {
> +if (entity->guilty)
> +atomic_set(entity->guilty, 0);
> +break;
> +}
> +}
> +spin_unlock(&rq->lock);
> +if (&entity->list != &rq->entities)
> +break;
> +}
> +}
> +}
> +EXPORT_SYMBOL(drm_sched_reset_karma);

Same as above, very similar drm_sched_increase_karma, I would add a flag and just reuse code instead of duplucation.

Andrey

> +
>   /**
>    * drm_sched_stop - stop the scheduler
>    *
> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> index 1c815e0a14ed..01c609149731 100644
> --- a/include/drm/gpu_scheduler.h
> +++ b/include/drm/gpu_scheduler.h
> @@ -321,7 +321,9 @@ void drm_sched_wakeup(struct drm_gpu_scheduler *sched);
>   void drm_sched_stop(struct drm_gpu_scheduler *sched, struct drm_sched_job *bad);
>   void drm_sched_start(struct drm_gpu_scheduler *sched, bool full_recovery);
>   void drm_sched_resubmit_jobs(struct drm_gpu_scheduler *sched);
> +void drm_sched_resubmit_jobs2(struct drm_gpu_scheduler *sched, int
> +max);
>   void drm_sched_increase_karma(struct drm_sched_job *bad);
> +void drm_sched_reset_karma(struct drm_sched_job *bad);
>   bool drm_sched_dependency_optimized(struct dma_fence* fence,
>       struct drm_sched_entity *entity);
>   void drm_sched_fault(struct drm_gpu_scheduler *sched);
>

[-- Attachment #1.2: Type: text/html, Size: 22072 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

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

Re: [PATCH v6] drm/amd/amdgpu implement tdr advanced mode

[Christian König]

[-- Attachment #1.1: Type: text/plain, Size: 13273 bytes --]

Alternatively changing all callers to give MAX_INT as parameter when 
they don't care is the preferred variant, but a bit more work.

Christian.

Am 11.03.21 um 04:24 schrieb Grodzovsky, Andrey:
> You can just rename drm_sched_resubmit_jobs to 
> drm_sched_resubmit_jobs_imp and create a wrapper 
> functiondrm_sched_resubmit_jobs which will call that function with 
> MAX_INT and so no need for code change in other driver will be needed. 
> For amdgpu you call directly drm_sched_resubmit_jobs_imp with 1 and 
> MAX_INT.
>
> Andrey
>
> ------------------------------------------------------------------------
> *From:* Zhang, Jack (Jian) <Jack.Zhang1@amd.com>
> *Sent:* 10 March 2021 22:05
> *To:* Grodzovsky, Andrey <Andrey.Grodzovsky@amd.com>; 
> amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>; Koenig, 
> Christian <Christian.Koenig@amd.com>; Liu, Monk <Monk.Liu@amd.com>; 
> Deng, Emily <Emily.Deng@amd.com>
> *Subject:* RE: [PATCH v6] drm/amd/amdgpu implement tdr advanced mode
> [AMD Official Use Only - Internal Distribution Only]
>
> Hi, Andrey,
>
> Thank you for your review.
>
> >> This is identical to drm_sched_resubmit_jobs in all but the 'int 
> max' handling, can't you reuse drm_sched_resubmit_jobs by passing max 
> argument==1 for the find guilty run and then, for the later normal run 
> passing max==MAX_INT to avoid break the iteration to early ?
>
> Due to C language doesn't support function overloading, we couldn’t 
> define function like the following(compiler error):
> void drm_sched_resubmit_jobs(struct drm_gpu_scheduler *sched, int max 
> = INT_MAX);
>
> We have to rewrite every vender's  code where call the 
> drm_sched_resubmit_jobs if we defined the function like this.
> void drm_sched_resubmit_jobs(struct drm_gpu_scheduler *sched, int max);
>
> Meanwhile, I also explored the Variable Arguments in C, But it still 
> cannot meet the needs very well.
>
> Do you know some other methods that could help us to make this happen?
> Otherwise, I'm afraid we have to define a separate ways as patch V6 did.
>
> Thanks,
> Jack
>
> -----Original Message-----
> From: Grodzovsky, Andrey <Andrey.Grodzovsky@amd.com>
> Sent: Thursday, March 11, 2021 12:19 AM
> To: Zhang, Jack (Jian) <Jack.Zhang1@amd.com>; 
> amd-gfx@lists.freedesktop.org; Koenig, Christian 
> <Christian.Koenig@amd.com>; Liu, Monk <Monk.Liu@amd.com>; Deng, Emily 
> <Emily.Deng@amd.com>
> Subject: Re: [PATCH v6] drm/amd/amdgpu implement tdr advanced mode
>
>
>
> On 2021-03-10 8:33 a.m., Jack Zhang wrote:
> > [Why]
> > Previous tdr design treats the first job in job_timeout as the bad job.
> > But sometimes a later bad compute job can block a good gfx job and
> > cause an unexpected gfx job timeout because gfx and compute ring share
> > internal GC HW mutually.
> >
> > [How]
> > This patch implements an advanced tdr mode.It involves an additinal
> > synchronous pre-resubmit step(Step0 Resubmit) before normal resubmit
> > step in order to find the real bad job.
> >
> > 1. At Step0 Resubmit stage, it synchronously submits and pends for the
> > first job being signaled. If it gets timeout, we identify it as guilty
> > and do hw reset. After that, we would do the normal resubmit step to
> > resubmit left jobs.
> >
> > 2. Re-insert Bailing job to mirror_list, and leave it to be handled by
> > the main reset thread.
> >
> > 3. For whole gpu reset(vram lost), do resubmit as the old way.
> >
> > Signed-off-by: Jack Zhang <Jack.Zhang1@amd.com>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 72 ++++++++++++++++++++-
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c    |  2 +-
> >   drivers/gpu/drm/scheduler/sched_main.c     | 75 ++++++++++++++++++++++
> >   include/drm/gpu_scheduler.h                |  2 +
> >   4 files changed, 148 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > index e247c3a2ec08..dac0a242e5f5 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > @@ -4639,6 +4639,7 @@ int amdgpu_device_gpu_recover(struct 
> amdgpu_device *adev,
> >   int i, r = 0;
> >   bool need_emergency_restart = false;
> >   bool audio_suspended = false;
> > +int tmp_vram_lost_counter;
> >
> >   /*
> >    * Special case: RAS triggered and full reset isn't supported @@
> > -4689,9 +4690,14 @@ int amdgpu_device_gpu_recover(struct 
> amdgpu_device *adev,
> >   dev_info(adev->dev, "Bailing on TDR for s_job:%llx, as another 
> already in progress",
> >   job ? job->base.id : -1);
> >
> > -/* even we skipped this reset, still need to set the job to guilty */
> > -if (job)
> > +if (job) {
> > +/* re-insert node to avoid memory leak */
> > +spin_lock(&job->base.sched->job_list_lock);
> > +list_add(&job->base.node, &job->base.sched->pending_list);
> > +spin_unlock(&job->base.sched->job_list_lock);
> > +/* even we skipped this reset, still need to set the job to guilty
> > +*/
> >   drm_sched_increase_karma(&job->base);
> > +}
>
> I think this piece should be in a standalone patch as it comes to fix 
> a bug of leaking jobs and not directly related to find actual guilty 
> job. Also, this is not the only place where the problem arises - it 
> also arises in other drivers where they check that guilty job's fence 
> already signaled and bail out before reinsertion of bad job to mirror 
> list. Seems to me better fix would be to handle this within scheduler 
> code by adding specific return value to 
> drm_sched_backend_ops.timedout_job marking that the code terminated 
> early - before calling drm_sched_stop and so drm_sched_job_timedout 
> would know this and then reinsert the job back to mirror_list itself.
>
> >   goto skip_recovery;
> >   }
> >
> > @@ -4788,6 +4794,7 @@ int amdgpu_device_gpu_recover(struct 
> amdgpu_device *adev,
> >   }
> >   }
> >
> > +tmp_vram_lost_counter = atomic_read(&((adev)->vram_lost_counter));
> >   /* Actual ASIC resets if needed.*/
> >   /* TODO Implement XGMI hive reset logic for SRIOV */
> >   if (amdgpu_sriov_vf(adev)) {
> > @@ -4805,6 +4812,67 @@ int amdgpu_device_gpu_recover(struct 
> amdgpu_device *adev,
> >   /* Post ASIC reset for all devs .*/
> >   list_for_each_entry(tmp_adev, device_list_handle, gmc.xgmi.head) {
> >
> > +if (amdgpu_gpu_recovery == 2 &&
> > +!(tmp_vram_lost_counter < atomic_read(&adev->vram_lost_counter)))
> > +{
> > +
> > +for (i = 0; i < AMDGPU_MAX_RINGS; ++i) {
> > +struct amdgpu_ring *ring = tmp_adev->rings[i];
> > +int ret = 0;
> > +struct drm_sched_job *s_job;
> > +
> > +if (!ring || !ring->sched.thread)
> > +continue;
> > +
> > +/* No point to resubmit jobs if we didn't HW reset*/
> > +if (!tmp_adev->asic_reset_res && !job_signaled) {
> > +
> > +s_job = list_first_entry_or_null(&ring->sched.pending_list, struct 
> drm_sched_job, list);
> > +if (s_job == NULL)
> > +continue;
> > +
> > +/* clear job's guilty and depend the folowing step to decide the 
> real one */
> > +drm_sched_reset_karma(s_job);
> > +drm_sched_resubmit_jobs2(&ring->sched, 1);
> > +ret = dma_fence_wait_timeout(s_job->s_fence->parent, false,
> > +ring->sched.timeout);
> > +
> > +if (ret == 0) { /* timeout */
> > +DRM_ERROR("Found the real bad job! ring:%s, job_id:%llx\n", 
> ring->sched.name, s_job->id);
> > +/* set guilty */
> > +drm_sched_increase_karma(s_job);
> > +
> > +/* do hw reset */
> > +if (amdgpu_sriov_vf(adev)) {
> > +amdgpu_virt_fini_data_exchange(adev);
> > +r = amdgpu_device_reset_sriov(adev, false);
> > +if (r)
> > +adev->asic_reset_res = r;
> > +} else {
> > +r  = amdgpu_do_asic_reset(hive, device_list_handle, 
> &need_full_reset, false);
> > +if (r && r == -EAGAIN)
> > +goto retry;
> > +}
> > +
> > +/* add reset counter so that the following resubmitted job could 
> flush vmid */
> > +atomic_inc(&tmp_adev->gpu_reset_counter);
> > +continue;
> > +}
> > +
> > +/* got the hw fence, signal finished fence */
> > +atomic_dec(&ring->sched.num_jobs);
> > +dma_fence_get(&s_job->s_fence->finished);
> > +dma_fence_signal(&s_job->s_fence->finished);
> > +dma_fence_put(&s_job->s_fence->finished);
> > +
> > +
> > +/* remove node from list and free the job */
> > +spin_lock(&ring->sched.job_list_lock);
> > +list_del_init(&s_job->node);
> > +spin_unlock(&ring->sched.job_list_lock);
> > +ring->sched.ops->free_job(s_job);
> > +}
> > +}
> > +}
> > +
> >   for (i = 0; i < AMDGPU_MAX_RINGS; ++i) {
> >   struct amdgpu_ring *ring = tmp_adev->rings[i];
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > index 865f924772b0..9c3f4edb7532 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > @@ -509,7 +509,7 @@ module_param_named(compute_multipipe, 
> amdgpu_compute_multipipe, int, 0444);
> >    * DOC: gpu_recovery (int)
> >    * Set to enable GPU recovery mechanism (1 = enable, 0 = disable). 
> The default is -1 (auto, disabled except SRIOV).
> >    */
> > -MODULE_PARM_DESC(gpu_recovery, "Enable GPU recovery mechanism, (1 =
> > enable, 0 = disable, -1 = auto)");
> > +MODULE_PARM_DESC(gpu_recovery, "Enable GPU recovery mechanism, (2 =
> > +advanced tdr mode, 1 = enable, 0 = disable, -1 = auto)");
> >   module_param_named(gpu_recovery, amdgpu_gpu_recovery, int, 0444);
> >
> >   /**
> > diff --git a/drivers/gpu/drm/scheduler/sched_main.c
> > b/drivers/gpu/drm/scheduler/sched_main.c
> > index d82a7ebf6099..340a193b4fb9 100644
> > --- a/drivers/gpu/drm/scheduler/sched_main.c
> > +++ b/drivers/gpu/drm/scheduler/sched_main.c
> > @@ -395,6 +395,81 @@ void drm_sched_increase_karma(struct 
> drm_sched_job *bad)
> >   }
> >   EXPORT_SYMBOL(drm_sched_increase_karma);
> >
> > +
> > +void drm_sched_resubmit_jobs2(struct drm_gpu_scheduler *sched, int
> > +max) {
> > +struct drm_sched_job *s_job, *tmp;
> > +uint64_t guilty_context;
> > +bool found_guilty = false;
> > +struct dma_fence *fence;
> > +int i = 0;
> > +
> > +list_for_each_entry_safe(s_job, tmp, &sched->pending_list, list) {
> > +struct drm_sched_fence *s_fence = s_job->s_fence;
> > +
> > +if (i >= max)
> > +break;
> > +
> > +if (!found_guilty && atomic_read(&s_job->karma) > sched->hang_limit) {
> > +found_guilty = true;
> > +guilty_context = s_job->s_fence->scheduled.context;
> > +}
> > +
> > +if (found_guilty && s_job->s_fence->scheduled.context == 
> guilty_context)
> > +dma_fence_set_error(&s_fence->finished, -ECANCELED);
> > +
> > +dma_fence_put(s_job->s_fence->parent);
> > +fence = sched->ops->run_job(s_job);
> > +i++;
> > +
> > +if (IS_ERR_OR_NULL(fence)) {
> > +if (IS_ERR(fence))
> > +dma_fence_set_error(&s_fence->finished, PTR_ERR(fence));
> > +
> > +s_job->s_fence->parent = NULL;
> > +} else {
> > +s_job->s_fence->parent = fence;
> > +}
> > +}
> > +}
> > +EXPORT_SYMBOL(drm_sched_resubmit_jobs2);
>
> This is identical to drm_sched_resubmit_jobs in all but the 'int max' 
> handling, can't you reuse drm_sched_resubmit_jobs by passing max 
> argument==1 for the find guilty run and then, for the later normal run 
> passing max==MAX_INT to avoid break the iteration to early ?
>
> > +
> > +
> > +
> > +void drm_sched_reset_karma(struct drm_sched_job *bad) {
> > +int i;
> > +struct drm_sched_entity *tmp;
> > +struct drm_sched_entity *entity;
> > +struct drm_gpu_scheduler *sched = bad->sched;
> > +
> > +/* don't reset @bad's karma if it's from KERNEL RQ,
> > + * because sometimes GPU hang would cause kernel jobs (like VM 
> updating jobs)
> > + * corrupt but keep in mind that kernel jobs always considered good.
> > + */
> > +if (bad->s_priority != DRM_SCHED_PRIORITY_KERNEL) {
> > +atomic_set(&bad->karma, 0);
> > +for (i = DRM_SCHED_PRIORITY_MIN; i < DRM_SCHED_PRIORITY_KERNEL;
> > +     i++) {
> > +struct drm_sched_rq *rq = &sched->sched_rq[i];
> > +
> > +spin_lock(&rq->lock);
> > +list_for_each_entry_safe(entity, tmp, &rq->entities, list) {
> > +if (bad->s_fence->scheduled.context ==
> > +entity->fence_context) {
> > +if (entity->guilty)
> > +atomic_set(entity->guilty, 0);
> > +break;
> > +}
> > +}
> > +spin_unlock(&rq->lock);
> > +if (&entity->list != &rq->entities)
> > +break;
> > +}
> > +}
> > +}
> > +EXPORT_SYMBOL(drm_sched_reset_karma);
>
> Same as above, very similar drm_sched_increase_karma, I would add a 
> flag and just reuse code instead of duplucation.
>
> Andrey
>
> > +
> >   /**
> >    * drm_sched_stop - stop the scheduler
> >    *
> > diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> > index 1c815e0a14ed..01c609149731 100644
> > --- a/include/drm/gpu_scheduler.h
> > +++ b/include/drm/gpu_scheduler.h
> > @@ -321,7 +321,9 @@ void drm_sched_wakeup(struct drm_gpu_scheduler 
> *sched);
> >   void drm_sched_stop(struct drm_gpu_scheduler *sched, struct 
> drm_sched_job *bad);
> >   void drm_sched_start(struct drm_gpu_scheduler *sched, bool 
> full_recovery);
> >   void drm_sched_resubmit_jobs(struct drm_gpu_scheduler *sched);
> > +void drm_sched_resubmit_jobs2(struct drm_gpu_scheduler *sched, int
> > +max);
> >   void drm_sched_increase_karma(struct drm_sched_job *bad);
> > +void drm_sched_reset_karma(struct drm_sched_job *bad);
> >   bool drm_sched_dependency_optimized(struct dma_fence* fence,
> >       struct drm_sched_entity *entity);
> >   void drm_sched_fault(struct drm_gpu_scheduler *sched);
> >


[-- Attachment #1.2: Type: text/html, Size: 27071 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

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