On 2026. szeptember 7., hétfő 9:17:12 közép-európai nyári idő Lazar, Lijo
wrote:
> On 04-Sep-26 12:58 PM, Timur Kristóf wrote:
> > Remove the caller_handles_kernel_queues argument from
> > the amdgpu_sdma_reset_engine() function and make it
> > always handle kernel queues.
> >
> > Now the SDMA recovery sequence is more consistent
> > between callers for the KFD as follows.
> > Before recovery: first the KFD is suspended,
> > then the SDMA queue contents are backed up.
> > After recovery: first the SDMA queue contents
> > are restored, then the KFD is resumed.
> >
> > Signed-off-by: Timur Kristóf <[email protected]>
> > ---
> >
> > drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c | 68 ++++++++++---------
> > drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h | 3 +-
> > drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c | 2 +-
> > .../drm/amd/amdkfd/kfd_device_queue_manager.c | 2 +-
> > 4 files changed, 38 insertions(+), 37 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c index
> > 9eebd8380834..07aac5b3ea92 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c
> > @@ -542,16 +542,14 @@ static int amdgpu_sdma_soft_reset(struct
> > amdgpu_device *adev, u32 instance_id)>
> > }
> >
> > /**
> >
> > - * amdgpu_sdma_reset_engine - Reset a specific SDMA engine
> > + * amdgpu_sdma_reset_engine() - Reset a specific SDMA engine instance.
> > + *
> >
> > * @adev: Pointer to the AMDGPU device
> > * @instance_id: Logical ID of the SDMA engine instance to reset
> >
> > - * @caller_handles_kernel_queues: Skip kernel queue processing. Caller
> > - * will handle it.
> >
> > *
> > * Returns: 0 on success, or a negative error code on failure.
> > */
> >
> > -int amdgpu_sdma_reset_engine(struct amdgpu_device *adev, uint32_t
> > instance_id, - bool
caller_handles_kernel_queues)
> > +int amdgpu_sdma_reset_engine(struct amdgpu_device *adev, uint32_t
> > instance_id)>
> > {
> >
> > struct amdgpu_sdma_instance *sdma_instance =
> > &adev->sdma.instance[instance_id]; struct amdgpu_ring *gfx_ring =
> > &sdma_instance->ring;
> >
> > @@ -564,20 +562,23 @@ int amdgpu_sdma_reset_engine(struct amdgpu_device
> > *adev, uint32_t instance_id,>
> > mutex_lock(&sdma_instance->engine_reset_mutex);
> >
> > - if (!caller_handles_kernel_queues) {
> > - /* Stop the scheduler's work queue for the GFX and page
rings if they
> > are running. - * This ensures that no new tasks are
submitted to the
> > queues while - * the reset is in progress.
> > - */
> > + /*
> > + * Stop the scheduler's work queue for the GFX and page rings if
they
> > are running. + * This ensures that no new tasks are submitted to
the
> > queues while + * the reset is in progress.
> > + */
> > + if (amdgpu_ring_sched_ready(gfx_ring) &&
> > !drm_sched_is_stopped(&gfx_ring->sched))>
> > drm_sched_wqueue_stop(&gfx_ring->sched);
> >
> > - gfx_fence = amdgpu_ring_find_guilty_fence(gfx_ring);
> > - amdgpu_ring_reset_helper_begin(gfx_ring, gfx_fence);
> >
> > - if (adev->sdma.has_page_queue) {
> > + gfx_fence = amdgpu_ring_find_guilty_fence(gfx_ring);
> > + amdgpu_ring_reset_helper_begin(gfx_ring, gfx_fence);
> > +
> > + if (adev->sdma.has_page_queue) {
> > + if (amdgpu_ring_sched_ready(page_ring) &&
> > !drm_sched_is_stopped(&page_ring->sched))>
> > drm_sched_wqueue_stop(&page_ring->sched);
> >
> > - page_fence =
amdgpu_ring_find_guilty_fence(page_ring);
> > - amdgpu_ring_reset_helper_begin(page_ring,
page_fence);
> > - }
> > +
> > + page_fence = amdgpu_ring_find_guilty_fence(page_ring);
> > + amdgpu_ring_reset_helper_begin(page_ring, page_fence);
> >
> > }
>
> Since this resets the engine, a different way may be to have something
> like below (similar to amdgpu_multi_ring_reset_helper_begin) which takes
> care of all rings in the engine instance.
>
> amdgpu_ring_engine_reset_helper_begin(guilty_ring, guilty_fence);
>
> amdgpu_ring_engine_reset_helper_end(guilty_ring, guilty_fence);
>
> ring_type = guilty_ring->funcs->type;
> eng_instance = guilty_ring->me
>
> Thanks,
> Lijo
I am planning to do exactly that, but I want to keep this series short and
focused on unifying the code paths for SDMA v4.4.2 and v5.x.
Is it OK if I do that in a follow-up series?
Thanks,
Timur
>
> > if (sdma_instance->funcs->stop_kernel_queue) {
> >
> > @@ -612,22 +613,25 @@ int amdgpu_sdma_reset_engine(struct amdgpu_device
> > *adev, uint32_t instance_id,>
> > }
> >
> > exit:
> > - if (!caller_handles_kernel_queues) {
> > - /* Restart the scheduler's work queue for the GFX and
page rings
> > - * if they were stopped by this function. This allows
new tasks
> > - * to be submitted to the queues after the reset is
complete.
> > - */
> > - if (!ret) {
> > - ret = amdgpu_ring_reset_helper_end(gfx_ring,
gfx_fence);
> > + /* Restart the scheduler's work queue for the GFX and page rings
> > + * if they were stopped by this function. This allows new tasks
> > + * to be submitted to the queues after the reset is complete.
> > + */
> > + if (!ret) {
> > + ret = amdgpu_ring_reset_helper_end(gfx_ring,
gfx_fence);
> > + if (ret)
> > + goto unlock;
> > +
> > + if (amdgpu_ring_sched_ready(gfx_ring))
> > + drm_sched_wqueue_start(&gfx_ring->sched);
> > +
> > + if (adev->sdma.has_page_queue) {
> > + ret =
amdgpu_ring_reset_helper_end(page_ring, page_fence);
> >
> > if (ret)
> >
> > goto unlock;
> >
> > - drm_sched_wqueue_start(&gfx_ring->sched);
> > - if (adev->sdma.has_page_queue) {
> > - ret =
amdgpu_ring_reset_helper_end(page_ring, page_fence);
> > - if (ret)
> > - goto unlock;
> > +
> > + if (amdgpu_ring_sched_ready(page_ring))
> >
> >
drm_sched_wqueue_start(&page_ring->sched);
> >
> > - }
> >
> > }
> >
> > }
> >
> > unlock:
> > @@ -662,13 +666,11 @@ int amdgpu_sdma_reset_queue_legacy(struct
> > amdgpu_ring *ring,>
> > return -EINVAL;
> >
> > }
> >
> > - amdgpu_ring_reset_helper_begin(ring, timedout_fence);
> > -
> >
> > amdgpu_amdkfd_suspend(adev, true);
> >
> > - r = amdgpu_sdma_reset_engine(adev, ring->me, true);
> > + r = amdgpu_sdma_reset_engine(adev, ring->me);
> >
> > amdgpu_amdkfd_resume(adev, true);
> > if (r)
> >
> > return r;
> >
> > - return amdgpu_ring_reset_helper_end(ring, timedout_fence);
> > + return 0;
> >
> > }
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h index
> > cb41453c1a19..5709d438e824 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h
> > @@ -153,8 +153,7 @@ struct amdgpu_buffer_funcs {
> >
> > uint32_t byte_count);
> >
> > };
> >
> > -int amdgpu_sdma_reset_engine(struct amdgpu_device *adev, uint32_t
> > instance_id, - bool
caller_handles_kernel_queues);
> > +int amdgpu_sdma_reset_engine(struct amdgpu_device *adev, uint32_t
> > instance_id);>
> > int amdgpu_sdma_reset_queue_legacy(struct amdgpu_ring *ring,
> >
> > unsigned int vmid,
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c
> > b/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c index
> > 77f385b9ef53..796ea9f74763 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c
> > @@ -1583,7 +1583,7 @@ static int sdma_v4_4_2_reset_queue(struct
> > amdgpu_ring *ring,>
> > int r;
> >
> > amdgpu_amdkfd_suspend(adev, true);
> >
> > - r = amdgpu_sdma_reset_engine(adev, id, false);
> > + r = amdgpu_sdma_reset_engine(adev, id);
> >
> > amdgpu_amdkfd_resume(adev, true);
> > return r;
> >
> > }
> >
> > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> > b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c index
> > a23384571193..f9fbc5628fb4 100644
> > --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> > @@ -2576,7 +2576,7 @@ static int reset_hung_queues_sdma(struct
> > device_queue_manager *dqm)>
> > continue;
> >
> > /* Reset engine and check. */
> >
> > - if (amdgpu_sdma_reset_engine(dqm->dev->adev,
i, false) ||
> > + if (amdgpu_sdma_reset_engine(dqm->dev->adev,
i) ||
> >
> > dqm->dev->kfd2kgd-
>hqd_sdma_get_doorbell(dqm->dev->adev, i, j) ||
> > !set_sdma_queue_as_reset(dqm,
doorbell_off)) {
> >
> > r = -ENOTRECOVERABLE;
