Re: [PATCH 08/31] drm/amdgpu: track ring state associated with a job

["Christian König" <christian.koenig@amd.com>]

On 6/5/25 03:45, Alex Deucher wrote:
> We need to know the wptr and sequence number associated
> with a job so that we can re-emit the unprocessed state
> after a ring reset.  Pre-allocate storage space for
> the ring buffer contents and add a helper to save off
> the unprocessed state so that it can be re-emitted
> after the queue is reset.
> 
> Add a helper that ring reset callbacks can use to verify
> that the ring has reset successfully and to reemit any
> unprocessed ring contents from subsequent jobs.
> 
> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c | 15 ++++
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c    | 13 +++-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_job.c   | 11 ++-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c  | 92 +++++++++++++++++++++++
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h  | 15 ++++
>  5 files changed, 143 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> index 569e0e5373927..25a664273bf0d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> @@ -141,6 +141,9 @@ int amdgpu_fence_emit(struct amdgpu_ring *ring, struct dma_fence **f, struct amd
>  	}
>  	fence = &am_fence->base;
>  	am_fence->ring = ring;
> +	am_fence->start_ring_wptr = 0;
> +	am_fence->end_ring_wptr = 0;

Why do we need the start here? I would just keep the end around and then jump from fence to fence while re-submitting them.

> +	am_fence->context = 0;
>  
>  	seq = ++ring->fence_drv.sync_seq;
>  	if (job && job->job_run_counter) {
> @@ -748,6 +751,18 @@ void amdgpu_fence_driver_force_completion(struct amdgpu_ring *ring)
>  	amdgpu_fence_process(ring);
>  }
>  
> +/**
> + * amdgpu_fence_driver_seq_force_completion - force signal of specified sequence
> + *
> + * @ring: fence of the ring to signal
> + *
> + */
> +void amdgpu_fence_driver_seq_force_completion(struct amdgpu_ring *ring, u32 seq)

Better give the full fence structure here.

> +{
> +	amdgpu_fence_write(ring, seq);
> +	amdgpu_fence_process(ring);
> +}
> +
>  /*
>   * Common fence implementation
>   */
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> index 802743efa3b39..636941697a740 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> @@ -126,7 +126,9 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, unsigned int num_ibs,
>  		       struct dma_fence **f)
>  {
>  	struct amdgpu_device *adev = ring->adev;
> +	u64 start_ring_wptr, end_ring_wptr;
>  	struct amdgpu_ib *ib = &ibs[0];
> +	struct amdgpu_fence *am_fence;
>  	struct dma_fence *tmp = NULL;
>  	bool need_ctx_switch;
>  	struct amdgpu_vm *vm;
> @@ -138,7 +140,6 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, unsigned int num_ibs,
>  	int vmid = AMDGPU_JOB_GET_VMID(job);
>  	bool need_pipe_sync = false;
>  	unsigned int cond_exec;
> -
>  	unsigned int i;
>  	int r = 0;
>  
> @@ -187,6 +188,7 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, unsigned int num_ibs,
>  		dev_err(adev->dev, "scheduling IB failed (%d).\n", r);
>  		return r;
>  	}
> +	start_ring_wptr = ring->wptr;
>  
>  	need_ctx_switch = ring->current_ctx != fence_ctx;
>  	if (ring->funcs->emit_pipeline_sync && job &&
> @@ -306,6 +308,15 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, unsigned int num_ibs,
>  
>  	amdgpu_ring_ib_end(ring);
>  	amdgpu_ring_commit(ring);
> +	/* This must be last for resets to work properly
> +	 * as we need to save the wptr associated with this
> +	 * fence.
> +	 */
> +	end_ring_wptr = ring->wptr;
> +	am_fence = container_of(*f, struct amdgpu_fence, base);
> +	am_fence->start_ring_wptr = start_ring_wptr;
> +	am_fence->end_ring_wptr = end_ring_wptr;

The end_ring_wptr variable is superflous and I would put assigning that into a helper in amdgpu_fence.c

> +
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> index 23b6a0fc0c691..73c26e2e01647 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> @@ -89,8 +89,9 @@ static enum drm_gpu_sched_stat amdgpu_job_timedout(struct drm_sched_job *s_job)
>  {
>  	struct amdgpu_ring *ring = to_amdgpu_ring(s_job->sched);
>  	struct amdgpu_job *job = to_amdgpu_job(s_job);
> -	struct amdgpu_task_info *ti;
> +	struct amdgpu_fence *am_fence = &job->hw_fence;
>  	struct amdgpu_device *adev = ring->adev;
> +	struct amdgpu_task_info *ti;
>  	bool set_error = false;
>  	int idx, r;
>  
> @@ -154,7 +155,8 @@ static enum drm_gpu_sched_stat amdgpu_job_timedout(struct drm_sched_job *s_job)
>  		else
>  			is_guilty = true;
>  
> -		if (is_guilty)
> +		amdgpu_ring_backup_unprocessed_jobs(ring, is_guilty, am_fence);
> +		if (is_guilty) {
>  			dma_fence_set_error(&s_job->s_fence->finished, -ETIME);
>  			set_error = true;
>  		}
> @@ -409,6 +411,7 @@ static struct dma_fence *amdgpu_job_run(struct drm_sched_job *sched_job)
>  	struct amdgpu_ring *ring = to_amdgpu_ring(sched_job->sched);
>  	struct amdgpu_device *adev = ring->adev;
>  	struct dma_fence *fence = NULL, *finished;
> +	struct amdgpu_fence *am_fence;
>  	struct amdgpu_job *job;
>  	int r = 0;
>  
> @@ -433,6 +436,10 @@ static struct dma_fence *amdgpu_job_run(struct drm_sched_job *sched_job)
>  				"Error scheduling IBs (%d) in ring(%s)", r,
>  				ring->name);
>  	}
> +	if (fence && finished) {
> +		am_fence = container_of(fence, struct amdgpu_fence, base);
> +		am_fence->context = finished->context;
> +	}

Better put that into amdgpu_fence_emit().

>  
>  	job->job_run_counter++;
>  	amdgpu_job_free_resources(job);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> index 426834806fbf2..3a0e0883bd8e7 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> @@ -333,6 +333,12 @@ int amdgpu_ring_init(struct amdgpu_device *adev, struct amdgpu_ring *ring,
>  	/*  Initialize cached_rptr to 0 */
>  	ring->cached_rptr = 0;
>  
> +	if (!ring->ring_backup) {
> +		ring->ring_backup = kvzalloc(ring->ring_size, GFP_KERNEL);
> +		if (!ring->ring_backup)
> +			return -ENOMEM;
> +	}
> +
>  	/* Allocate ring buffer */
>  	if (ring->ring_obj == NULL) {
>  		r = amdgpu_bo_create_kernel(adev, ring->ring_size + ring->funcs->extra_dw, PAGE_SIZE,
> @@ -342,6 +348,7 @@ int amdgpu_ring_init(struct amdgpu_device *adev, struct amdgpu_ring *ring,
>  					    (void **)&ring->ring);
>  		if (r) {
>  			dev_err(adev->dev, "(%d) ring create failed\n", r);
> +			kvfree(ring->ring_backup);
>  			return r;
>  		}
>  		amdgpu_ring_clear_ring(ring);
> @@ -385,6 +392,8 @@ void amdgpu_ring_fini(struct amdgpu_ring *ring)
>  	amdgpu_bo_free_kernel(&ring->ring_obj,
>  			      &ring->gpu_addr,
>  			      (void **)&ring->ring);
> +	kvfree(ring->ring_backup);
> +	ring->ring_backup = NULL;
>  
>  	dma_fence_put(ring->vmid_wait);
>  	ring->vmid_wait = NULL;
> @@ -753,3 +762,86 @@ bool amdgpu_ring_sched_ready(struct amdgpu_ring *ring)
>  
>  	return true;
>  }
> +
> +static void amdgpu_ring_backup_unprocessed_job(struct amdgpu_ring *ring,
> +					       unsigned int idx,
> +					       u64 start_wptr, u32 end_wptr)

Drop the job from all function names.

We are really re-emitting unprocessed commands.

> +{
> +	unsigned int first_idx = start_wptr & ring->buf_mask;
> +	unsigned int last_idx = end_wptr & ring->buf_mask;
> +	unsigned int i, j, entries_to_copy;
> +
> +	if (last_idx < first_idx) {
> +		entries_to_copy = ring->buf_mask + 1 - first_idx;
> +		for (i = 0; i < entries_to_copy; i++)
> +			ring->ring_backup[idx + i] = ring->ring[first_idx + i];
> +		ring->ring_backup_entries_to_copy += entries_to_copy;
> +		entries_to_copy = last_idx;
> +		for (j = 0; j < entries_to_copy; j++)
> +			ring->ring_backup[idx + i + j] = ring->ring[j];
> +		ring->ring_backup_entries_to_copy += entries_to_copy;
> +	} else {
> +		entries_to_copy = last_idx - first_idx;
> +		for (i = 0; i < entries_to_copy; i++)
> +			ring->ring_backup[idx + i] = ring->ring[first_idx + i];
> +		ring->ring_backup_entries_to_copy += entries_to_copy;
> +	}
> +}
> +
> +void amdgpu_ring_backup_unprocessed_jobs(struct amdgpu_ring *ring,
> +					 bool is_guilty,
> +					 struct amdgpu_fence *bad_fence)
> +{
> +	struct amdgpu_fence *fence;
> +	struct dma_fence *old, **ptr;
> +	int i;
> +
> +	ring->ring_backup_entries_to_copy = 0;
> +	for (i = 0; i <= ring->fence_drv.num_fences_mask; i++) {

That is the wrong order for the fences, you need to start/end at the last submitted one.

I strongly suggest to implement that in amdgpu_fence.c

Regards,
Christian.

> +		ptr = &ring->fence_drv.fences[i];
> +		rcu_read_lock();
> +		old = rcu_dereference(*ptr);
> +
> +		if (old && !dma_fence_is_signaled(old)) {
> +			fence = container_of(old, struct amdgpu_fence, base);
> +			/* save everything if the ring is not guilty, otherwise
> +			 * just save the content from other contexts.
> +			 */
> +			if (!is_guilty || (fence->context != bad_fence->context))
> +				amdgpu_ring_backup_unprocessed_job(ring,
> +								   ring->ring_backup_entries_to_copy,
> +								   fence->start_ring_wptr,
> +								   fence->end_ring_wptr);
> +		}
> +		rcu_read_unlock();
> +	}
> +
> +	ring->ring_bad_seq = bad_fence->base.seqno;
> +}
> +
> +int amdgpu_ring_reemit_unprocessed_jobs(struct amdgpu_ring *ring)
> +{
> +	unsigned int i;
> +	int r;
> +
> +	/* verify that the ring is functional */
> +	r = amdgpu_ring_test_ring(ring);
> +	if (r)
> +		return r;
> +	/* re-emit the unprocessed ring contents */
> +	if (ring->ring_backup_entries_to_copy) {
> +		r = amdgpu_ring_alloc(ring, ring->ring_backup_entries_to_copy);
> +		if (r)
> +			return r;
> +		/* signal the fence of the bad job */
> +		amdgpu_fence_driver_seq_force_completion(ring, ring->ring_bad_seq);
> +		for (i = 0; i < ring->ring_backup_entries_to_copy; i++)
> +			amdgpu_ring_write(ring, ring->ring_backup[i]);
> +		amdgpu_ring_commit(ring);
> +	} else {
> +		/* signal the fence of the bad job */
> +		amdgpu_fence_driver_seq_force_completion(ring, ring->ring_bad_seq);
> +	}
> +
> +	return 0;
> +}
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> index e1f25218943a4..69b71401adb7a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> @@ -141,6 +141,12 @@ struct amdgpu_fence {
>  	/* RB, DMA, etc. */
>  	struct amdgpu_ring		*ring;
>  	ktime_t				start_timestamp;
> +
> +	/* wptrs for the fence for resets */
> +	u64				start_ring_wptr;
> +	u64				end_ring_wptr;
> +	/* fence context for resets */
> +	u64				context;
>  };
>  
>  extern const struct drm_sched_backend_ops amdgpu_sched_ops;
> @@ -148,6 +154,8 @@ extern const struct drm_sched_backend_ops amdgpu_sched_ops;
>  void amdgpu_fence_driver_clear_job_fences(struct amdgpu_ring *ring);
>  void amdgpu_fence_driver_set_error(struct amdgpu_ring *ring, int error);
>  void amdgpu_fence_driver_force_completion(struct amdgpu_ring *ring);
> +void amdgpu_fence_driver_seq_force_completion(struct amdgpu_ring *ring,
> +					      u32 seq);
>  
>  int amdgpu_fence_driver_init_ring(struct amdgpu_ring *ring);
>  int amdgpu_fence_driver_start_ring(struct amdgpu_ring *ring,
> @@ -284,6 +292,9 @@ struct amdgpu_ring {
>  
>  	struct amdgpu_bo	*ring_obj;
>  	uint32_t		*ring;
> +	uint32_t		*ring_backup;
> +	unsigned int		ring_backup_entries_to_copy;
> +	uint64_t		ring_bad_seq;
>  	unsigned		rptr_offs;
>  	u64			rptr_gpu_addr;
>  	volatile u32		*rptr_cpu_addr;
> @@ -550,4 +561,8 @@ int amdgpu_ib_pool_init(struct amdgpu_device *adev);
>  void amdgpu_ib_pool_fini(struct amdgpu_device *adev);
>  int amdgpu_ib_ring_tests(struct amdgpu_device *adev);
>  bool amdgpu_ring_sched_ready(struct amdgpu_ring *ring);
> +void amdgpu_ring_backup_unprocessed_jobs(struct amdgpu_ring *ring,
> +					 bool is_guilty,
> +					 struct amdgpu_fence *bad_fence);
> +int amdgpu_ring_reemit_unprocessed_jobs(struct amdgpu_ring *ring);
>  #endif