On Tue, Jun 3, 2025 at 11:01 AM Christian König
<christian.koe...@amd.com> wrote:
>
> On 6/3/25 16:27, Alex Deucher wrote:
> >>> I'm not quite sure I understand what you are proposing. Is the idea
> >>> to track all of the jobs associated with a particular process and then
> >>> when we reset a queue, skip all of the ring contents associated with
> >>> those jobs and signal and set the error on all of their job fences?
> >>
> >> More or less yes, I think that is what is needed here.
> >>
> >> A simple example: Unigine Heaven in window mode on an X server. Each frame
> >> usually results in 3 job submissions from unigine, plus one submission
> >> from X to copy the result it into the displayed frame.
> >>
> >> When we now assume that we can schedule 4 jobs at a time on the ring we
> >> get: U1, U2, U3, X1 | U4, U5, U6, X2 | U7, U8, U9, X3....
> >>
> >> Let's assume U4 hangs and we initiate a queue reset, in this case we
> >> definately need to skip U5 and U6 as well because they belonged to the
> >> same context and depend on each other. Only skipping U4 would certainly
> >> crash the GPU again.
> >
> > In that case, we'd just reset the queue again, but I agree it would be
> > a nicer experience to skip all of the jobs for that app.
>
> Well, that is a must have I think. Otherwise problems only get worse and
> worse after a while and we can't let submissions run into timeouts over and
> over again.
>
> >>
> >> X2 also dependet on U6, but that submission is from X and totally innocent
> >> and rendering garbage for the window content is probably ok considering
> >> that the application just crashed.
> >>
> >>> What about cross ring dependencies?
> >>
> >> For gang submission we would need to do a queue reset for both the gfx and
> >> compute queue to get out of this again. But that is probably ok since each
> >> queue can timeout on its own.
> >>
> >> We also don't need to track the jobs per process, just looking if
> >> job->base.sched_fence->finished.context changes should be sufficient.
> >
> > Something like this?
> >
> > for (i = 0; i <= ring->fence_drv.num_fences_mask; i++) {
> > ptr = &ring->fence_drv.fences[i];
> > old = rcu_dereference_protected(*ptr, 1);
> > if (old && old->ops == &amdgpu_job_fence_ops) {
> > struct amdgpu_job *other_job =
> > container_of(old, struct amdgpu_job, hw_fence.base);
> >
> > if (other_job->base.sched_fence->finished.context ==
> > job->base.sched_fence->finished.context) {
> > struct amdgpu_fence *am_fence =
> > &other_job->hw_fence;
> > // skip the ring contents associated
> > with this context
> > }
> > }
> > }
>
> I would copy job->base.sched_fence->finished.context into the HW fence when
> it is created, but apart from that yes that is exactly what I had in mind.
Where and when does finished.context get set?
Alex
>
> Christian.
>
>
> >
> > Thanks,
> >
> > Alex
> >
> >
> >>
> >> Regards,
> >> Christian.
> >>
> >>>
> >>> Alex
> >>>
> >>>>
> >>>> Regards,
> >>>> Christian.
> >>>>
> >>>>
> >>>>> return 0;
> >>>>> }
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> >>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> >>>>> index a0fab947143b5..f0f752284b925 100644
> >>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> >>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> >>>>> @@ -91,6 +91,7 @@ static enum drm_gpu_sched_stat
> >>>>> amdgpu_job_timedout(struct drm_sched_job *s_job)
> >>>>> struct amdgpu_job *job = to_amdgpu_job(s_job);
> >>>>> struct amdgpu_task_info *ti;
> >>>>> struct amdgpu_device *adev = ring->adev;
> >>>>> + struct dma_fence *fence = &job->hw_fence;
> >>>>> int idx;
> >>>>> int r;
> >>>>>
> >>>>> @@ -154,8 +155,10 @@ static enum drm_gpu_sched_stat
> >>>>> amdgpu_job_timedout(struct drm_sched_job *s_job)
> >>>>> else
> >>>>> is_guilty = true;
> >>>>>
> >>>>> - if (is_guilty)
> >>>>> + if (is_guilty) {
> >>>>> + amdgpu_ring_backup_unprocessed_jobs(ring,
> >>>>> job->ring_wptr, fence->seqno);
> >>>>> dma_fence_set_error(&s_job->s_fence->finished,
> >>>>> -ETIME);
> >>>>> + }
> >>>>>
> >>>>> r = amdgpu_ring_reset(ring, job->vmid);
> >>>>> if (!r) {
> >>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
> >>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
> >>>>> index f2c049129661f..c2ed0edb5179d 100644
> >>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
> >>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
> >>>>> @@ -79,6 +79,8 @@ struct amdgpu_job {
> >>>>> /* enforce isolation */
> >>>>> bool enforce_isolation;
> >>>>> bool run_cleaner_shader;
> >>>>> + /* wptr for the job for resets */
> >>>>> + uint32_t ring_wptr;
> >>>>>
> >>>>> uint32_t num_ibs;
> >>>>> struct amdgpu_ib ibs[];
> >>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> >>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> >>>>> index 426834806fbf2..909b121d432cb 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,40 @@ bool amdgpu_ring_sched_ready(struct amdgpu_ring
> >>>>> *ring)
> >>>>>
> >>>>> return true;
> >>>>> }
> >>>>> +
> >>>>> +void amdgpu_ring_backup_unprocessed_jobs(struct amdgpu_ring *ring,
> >>>>> + u64 bad_wptr, u32 bad_seq)
> >>>>> +{
> >>>>> + unsigned int entries_to_copy = ring->wptr - bad_wptr;
> >>>>> + unsigned int idx, i;
> >>>>> +
> >>>>> + for (i = 0; i < entries_to_copy; i++) {
> >>>>> + idx = (bad_wptr + i) & ring->buf_mask;
> >>>>> + ring->ring_backup[i] = ring->ring[idx];
> >>>>> + }
> >>>>> + ring->ring_backup_entries_to_copy = entries_to_copy;
> >>>>> + ring->ring_backup_seq = bad_seq;
> >>>>> +}
> >>>>> +
> >>>>> +int amdgpu_ring_reemit_unprocessed_jobs(struct amdgpu_ring *ring)
> >>>>> +{
> >>>>> + unsigned int i;
> >>>>> + int r;
> >>>>> +
> >>>>> + /* signal the fence of the bad job */
> >>>>> + amdgpu_fence_driver_seq_force_completion(ring,
> >>>>> ring->ring_backup_seq);
> >>>>> + /* 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) {
> >>>>> + if (amdgpu_ring_alloc(ring,
> >>>>> ring->ring_backup_entries_to_copy))
> >>>>> + return -ENOMEM;
> >>>>> + for (i = 0; i < ring->ring_backup_entries_to_copy; i++)
> >>>>> + amdgpu_ring_write(ring, ring->ring_backup[i]);
> >>>>> + amdgpu_ring_commit(ring);
> >>>>> + }
> >>>>> +
> >>>>> + return r;
> >>>>> +}
> >>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> >>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> >>>>> index b95b471107692..fd08449eee33f 100644
> >>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> >>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> >>>>> @@ -132,6 +132,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,
> >>>>> @@ -268,6 +270,9 @@ struct amdgpu_ring {
> >>>>>
> >>>>> struct amdgpu_bo *ring_obj;
> >>>>> uint32_t *ring;
> >>>>> + uint32_t *ring_backup;
> >>>>> + uint32_t ring_backup_seq;
> >>>>> + unsigned int ring_backup_entries_to_copy;
> >>>>> unsigned rptr_offs;
> >>>>> u64 rptr_gpu_addr;
> >>>>> volatile u32 *rptr_cpu_addr;
> >>>>> @@ -534,4 +539,7 @@ 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,
> >>>>> + u64 bad_wptr, u32 bad_seq);
> >>>>> +int amdgpu_ring_reemit_unprocessed_jobs(struct amdgpu_ring *ring);
> >>>>> #endif
> >>>>
> >>
>
