Re: [PATCH 5/7] drm/amdgpu: Rework the amdgpu_job_stop_all_jobs_on_sched() hack

["Christian König" <ckoenig.leichtzumerken@gmail.com>]

On 11/6/26 15:57, Tvrtko Ursulin wrote:
> amdgpu_job_stop_all_jobs_on_sched is a semi broken hack used on only one
> platform which suffers from several problems as mentioned in
> https://lore.kernel.org/amd-gfx/44edde63-7181-44fb-a4f7-94e50514f539@amd.com/.
> 
> Until it can be fully removed, given that it directly acceses DRM scheduler
> internals which want to be changed, we need to rework it to allow for
> replacing the SPSC queue with a standard spin locked list.
> 
> Problem at hand is the lock inversion between the run-queue and entity
> lock which becomes apparent once locking is added to the job queue
> amdgpu_job_stop_all_jobs_on_sched().
> 
> To work around it we change the approach of how queued jobs are drained by
> using trylock and restarting on locked entities until all queued jobs from
> all active entities are removed.
> 
> This slightly widens the existing scope for a live lock in case of
> userspace actively submitting new jobs. Whereas the current code can keep
> processing the single busy entity, this new implementation can retry on the
> entities which it previously processed.
> 
> Since userspace submitting jobs so rapidly the GPU reset cannot finish and
> the problem already exists, plus the plan is to remove this code
> altogether, we can say this is acceptable in order to unblock the DRM
> scheduler rework.
> 
> While at it we add the assert scheduler workqueue has been stopped by the
> caller and also correct the wrong order of signalling already submitted
> and queued fences.

I think that is generally the wrong approach.

See function amdgpu_ras_need_emergency_restart() who triggers the behavior when returning true:

        if (adev->asic_type == CHIP_VEGA20 &&
            adev->pm.fw_version <= 0x283400) {
                return !(amdgpu_asic_reset_method(adev) == AMD_RESET_METHOD_BACO) &&
                                amdgpu_ras_intr_triggered();
        }
        return false;

So only on Vega 20 with an outdated power management FW version we use this hack to clean up the DRM scheduler jobs.

My educated guess is the code wasn't used in at least the last 5-6 years :)

I think we should rather completely remove the amdgpu_job_stop_all_jobs_on_sched() hack and just print a big warning in the system log when we detect an outdated firmware.

Hawking and Alex any objections to that?

Regards,
Christian.

> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> References: https://lore.kernel.org/amd-gfx/44edde63-7181-44fb-a4f7-94e50514f539@amd.com/
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Hawking Zhang <Hawking.Zhang@amd.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: Simona Vetter <simona@ffwll.ch>
> Cc: Danilo Krummrich <dakr@kernel.org>
> Cc: Philipp Stanner <phasta@kernel.org>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 86 ++++++++++++++++++++-----
>  1 file changed, 69 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> index 8689a3c933b2..1f4a4a5a978d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> @@ -457,13 +457,6 @@ static struct dma_fence *amdgpu_job_run(struct drm_sched_job *sched_job)
>  	return fence;
>  }
>  
> -/*
> - * This is a duplicate function from DRM scheduler sched_internal.h.
> - * Plan is to remove it when amdgpu_job_stop_all_jobs_on_sched is removed, due
> - * latter being incorrect and racy.
> - *
> - * See https://lore.kernel.org/amd-gfx/44edde63-7181-44fb-a4f7-94e50514f539@amd.com/
> - */
>  static struct drm_sched_job *
>  drm_sched_entity_queue_pop(struct drm_sched_entity *entity)
>  {
> @@ -476,28 +469,66 @@ drm_sched_entity_queue_pop(struct drm_sched_entity *entity)
>  	return container_of(node, struct drm_sched_job, queue_node);
>  }
>  
> -void amdgpu_job_stop_all_jobs_on_sched(struct drm_gpu_scheduler *sched)
> +static struct drm_sched_entity *
> +get_entity_with_jobs(struct drm_gpu_scheduler *sched) __acquires(&entity->lock)
>  {
> -	struct drm_sched_job *s_job;
> -	struct drm_sched_entity *s_entity = NULL;
> +	struct drm_sched_entity *entity;
>  	int i;
>  
> -	/* Signal all jobs not yet scheduled */
> +retry:
>  	for (i = DRM_SCHED_PRIORITY_KERNEL; i < sched->num_rqs; i++) {
>  		struct drm_sched_rq *rq = sched->sched_rq[i];
> +
>  		spin_lock(&rq->lock);
> -		list_for_each_entry(s_entity, &rq->entities, list) {
> -			while ((s_job = drm_sched_entity_queue_pop(s_entity))) {
> -				struct drm_sched_fence *s_fence = s_job->s_fence;
> +		list_for_each_entry(entity, &rq->entities, list) {
> +			/*
> +			 * This can false negative, but so could the previous
> +			 * approach of scanning the run-queues once when
> +			 * nothing prevents userspace to queue more jobs.
> +			 */
> +			if (!spsc_queue_count(&entity->job_queue))
> +				continue;
>  
> -				dma_fence_signal(&s_fence->scheduled);
> -				dma_fence_set_error(&s_fence->finished, -EHWPOISON);
> -				dma_fence_signal(&s_fence->finished);
> +			/*
> +			 * We must trylock because normal locking order is
> +			 * entity then rq and here we already have rq locked
> +			 * in order to be able to walk the entities.
> +			 */
> +			if (spin_trylock(&entity->lock)) {
> +				/* Returns with a locked entity. */
> +				spin_unlock(&rq->lock);
> +				return entity;
> +			} else {
> +				/*
> +				 * Busy entity - we will try again looking for a
> +				 * window where trylock will succeed.
> +				 */
> +				spin_unlock(&rq->lock);
> +				goto retry;
>  			}
>  		}
>  		spin_unlock(&rq->lock);
>  	}
>  
> +	return NULL;
> +}
> +
> +/*
> + * amdgpu_job_stop_all_jobs_on_sched has a single purpose which is an
> + * unreliable and racy, as described in:
> + *  https://lore.kernel.org/amd-gfx/44edde63-7181-44fb-a4f7-94e50514f539@amd.com/.
> + *
> + * Plan is to remove it, together with the above open coded DRM scheduler
> + * helpers, and replace with a proper solution.
> + */
> +void amdgpu_job_stop_all_jobs_on_sched(struct drm_gpu_scheduler *sched)
> +{
> +	struct drm_sched_job *s_job, *next;
> +	struct drm_sched_entity *s_entity;
> +	LIST_HEAD(queued);
> +
> +	WARN_ON_ONCE(!sched->pause_submit);
> +
>  	/* Signal all jobs already scheduled to HW */
>  	list_for_each_entry(s_job, &sched->pending_list, list) {
>  		struct drm_sched_fence *s_fence = s_job->s_fence;
> @@ -505,6 +536,27 @@ void amdgpu_job_stop_all_jobs_on_sched(struct drm_gpu_scheduler *sched)
>  		dma_fence_set_error(&s_fence->finished, -EHWPOISON);
>  		dma_fence_signal(&s_fence->finished);
>  	}
> +
> +	/*
> +	 * Signal all jobs not yet scheduled - note that the entity trylock loop
> +	 * can in theory livelock if userspace keeps submitting new jobs more
> +	 * rapidly than the loop can do a full pass over all run-queues and
> +	 * entities.
> +	 */
> +	while ((s_entity = get_entity_with_jobs(sched)) != NULL) {
> +		while ((s_job = drm_sched_entity_queue_pop(s_entity)))
> +			list_add_tail(&s_job->list, &queued);
> +		spin_unlock(&s_entity->lock);
> +	}
> +
> +	list_for_each_entry_safe(s_job, next, &queued, list) {
> +		struct drm_sched_fence *s_fence = s_job->s_fence;
> +
> +		list_del(&s_job->list);
> +		dma_fence_signal(&s_fence->scheduled);
> +		dma_fence_set_error(&s_fence->finished, -EHWPOISON);
> +		dma_fence_signal(&s_fence->finished);
> +	}
>  }
>  
>  const struct drm_sched_backend_ops amdgpu_sched_ops = {