On 2/10/2025 1:01 PM, Jesse.zhang@xxxxxxx wrote:
> From: "Jesse.zhang@xxxxxxx" <Jesse.zhang@xxxxxxx>
>
> This commit introduces a caller parameter to the amdgpu_sdma_reset_instance function to differentiate
> between reset requests originating from the KGD and KFD.
> This change ensures proper synchronization between KGD and KFD during SDMA resets.
>
> If the caller is KFD, the function now acquires and releases the scheduler lock (ring->sched.job_list_lock)
> to protect the SDMA queue during the reset.
>
> These changes prevent race conditions and ensure safe SDMA reset operations
> when initiated by KFD, improving system stability and reliability.
>
> V2: replace the ring_lock with the existed the scheduler
> locks for the queues (ring->sched) on the sdma engine.(Alex)
>
> v3: call drm_sched_wqueue_stop() rather than job_list_lock.
> If a GPU ring reset was already initiated for one ring at amdgpu_job_timedout,
> skip resetting that ring and call drm_sched_wqueue_stop()
> for the other rings (Alex)
>
> replace the common lock (sdma_reset_lock) with DQM lock to
> to resolve reset races between the two driver sections during KFD eviction.(Jon)
>
> Rename the caller to Reset_src and
> Change AMDGPU_RESET_SRC_SDMA_KGD/KFD to AMDGPU_RESET_SRC_SDMA_HWS/RING (Jon)
> v4: restart the wqueue if the reset was successful,
> or fall back to a full adapter reset. (Alex)
>
> move definition of reset source to enumeration AMDGPU_RESET_SRCS, and
> check reset src in amdgpu_sdma_reset_instance (Jon)
>
> Suggested-by: Alex Deucher <alexander.deucher@xxxxxxx>
> Suggested-by: Jiadong Zhu <Jiadong.Zhu@xxxxxxx>
> Suggested-by: Jonathan Kim <Jonathan.Kim@xxxxxxx>
> Signed-off-by: Jesse Zhang <jesse.zhang@xxxxxxx>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_reset.h | 2 +
> drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c | 54 +++++++++++++++++++----
> drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h | 6 +--
> drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c | 8 ++--
> 4 files changed, 56 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_reset.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_reset.h
> index 4d9b9701139b..5b86e12ff9fe 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_reset.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_reset.h
> @@ -43,6 +43,8 @@ enum AMDGPU_RESET_SRCS {
> AMDGPU_RESET_SRC_MES,
> AMDGPU_RESET_SRC_HWS,
> AMDGPU_RESET_SRC_USER,
> + AMDGPU_RESET_SRC_SDMA_RING,
> + AMDGPU_RESET_SRC_SDMA_HWS,
Ok. Probably this is the reason it's only dealing with instance rather
than amdgpu_ring. Please ignore my comments about the ring args in patch 2.
Thanks,
Lijo
> };
>
> struct amdgpu_reset_context {
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c
> index 19c8be7d72e2..8864a9d7455b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c
> @@ -25,6 +25,7 @@
> #include "amdgpu.h"
> #include "amdgpu_sdma.h"
> #include "amdgpu_ras.h"
> +#include "amdgpu_reset.h"
>
> #define AMDGPU_CSA_SDMA_SIZE 64
> /* SDMA CSA reside in the 3rd page of CSA */
> @@ -484,6 +485,7 @@ void amdgpu_sdma_register_on_reset_callbacks(struct amdgpu_device *adev, struct
> * amdgpu_sdma_reset_instance - Reset a specific SDMA instance
> * @adev: Pointer to the AMDGPU device
> * @instance_id: ID of the SDMA engine instance to reset
> + * @src: The source of reset function (KGD or KFD)
> *
> * This function performs the following steps:
> * 1. Calls all registered pre_reset callbacks to allow KFD and AMDGPU to save their state.
> @@ -492,20 +494,42 @@ void amdgpu_sdma_register_on_reset_callbacks(struct amdgpu_device *adev, struct
> *
> * Returns: 0 on success, or a negative error code on failure.
> */
> -int amdgpu_sdma_reset_instance(struct amdgpu_device *adev, uint32_t instance_id)
> +int amdgpu_sdma_reset_instance(struct amdgpu_device *adev, uint32_t instance_id, int src)
> {
> struct sdma_on_reset_funcs *funcs;
> - int ret;
> + int ret = 0;
> + struct amdgpu_sdma_instance *sdma_instance = &adev->sdma.instance[instance_id];;
> + struct amdgpu_ring *gfx_ring = &sdma_instance->ring;
> + struct amdgpu_ring *page_ring = &sdma_instance->page;
> + bool gfx_sched_stopped = false, page_sched_stopped = false;
> +
> + /* Check if the reset source is valid for SDMA ring reset */
> + if (src != AMDGPU_RESET_SRC_SDMA_RING && src != AMDGPU_RESET_SRC_HWS)
> + return -EINVAL;
> +
> + /* 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_wqueue_stop(&gfx_ring->sched);
> + gfx_sched_stopped = true;;
> + }
> +
> + if (adev->sdma.has_page_queue && !amdgpu_ring_sched_ready(page_ring)) {
> + drm_sched_wqueue_stop(&page_ring->sched);
> + page_sched_stopped = true;
> + }
>
> /* Invoke all registered pre_reset callbacks */
> list_for_each_entry(funcs, &adev->sdma.reset_callback_list, list) {
> if (funcs->pre_reset) {
> - ret = funcs->pre_reset(adev, instance_id);
> + ret = funcs->pre_reset(adev, instance_id, src);
> if (ret) {
> dev_err(adev->dev,
> "beforeReset callback failed for instance %u: %d\n",
> instance_id, ret);
> - return ret;
> + goto exit;
> }
> }
> }
> @@ -514,21 +538,35 @@ int amdgpu_sdma_reset_instance(struct amdgpu_device *adev, uint32_t instance_id)
> ret = amdgpu_dpm_reset_sdma(adev, 1 << instance_id);
> if (ret) {
> dev_err(adev->dev, "Failed to reset SDMA instance %u\n", instance_id);
> - return ret;
> + goto exit;
> }
>
> /* Invoke all registered post_reset callbacks */
> list_for_each_entry(funcs, &adev->sdma.reset_callback_list, list) {
> if (funcs->post_reset) {
> - ret = funcs->post_reset(adev, instance_id);
> + ret = funcs->post_reset(adev, instance_id, src);
> if (ret) {
> dev_err(adev->dev,
> "afterReset callback failed for instance %u: %d\n",
> instance_id, ret);
> - return ret;
> + goto exit;
> }
> }
> }
>
> - return 0;
> +exit:
> + /* 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) {
> + if (gfx_sched_stopped && amdgpu_ring_sched_ready(gfx_ring)) {
> + drm_sched_wqueue_start(&gfx_ring->sched);
> + }
> + if (page_sched_stopped && amdgpu_ring_sched_ready(page_ring)) {
> + drm_sched_wqueue_start(&page_ring->sched);
> + }
> + }
> +
> + return ret;
> }
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h
> index fbb8b04ef2cb..df5c9f7a4374 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h
> @@ -99,8 +99,8 @@ struct amdgpu_sdma_ras {
> };
>
> struct sdma_on_reset_funcs {
> - int (*pre_reset)(struct amdgpu_device *adev, uint32_t instance_id);
> - int (*post_reset)(struct amdgpu_device *adev, uint32_t instance_id);
> + int (*pre_reset)(struct amdgpu_device *adev, uint32_t instance_id, int src);
> + int (*post_reset)(struct amdgpu_device *adev, uint32_t instance_id, int src);
> /* Linked list node to store this structure in a list; */
> struct list_head list;
> };
> @@ -166,7 +166,7 @@ struct amdgpu_buffer_funcs {
> };
>
> void amdgpu_sdma_register_on_reset_callbacks(struct amdgpu_device *adev, struct sdma_on_reset_funcs *funcs);
> -int amdgpu_sdma_reset_instance(struct amdgpu_device *adev, uint32_t instance_id);
> +int amdgpu_sdma_reset_instance(struct amdgpu_device *adev, uint32_t instance_id, int src);
>
> #define amdgpu_emit_copy_buffer(adev, ib, s, d, b, t) (adev)->mman.buffer_funcs->emit_copy_buffer((ib), (s), (d), (b), (t))
> #define amdgpu_emit_fill_buffer(adev, ib, s, d, b) (adev)->mman.buffer_funcs->emit_fill_buffer((ib), (s), (d), (b))
> 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 3e60456b0db0..c297b4a13680 100644
> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c
> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c
> @@ -30,6 +30,7 @@
> #include "amdgpu_xcp.h"
> #include "amdgpu_ucode.h"
> #include "amdgpu_trace.h"
> +#include "amdgpu_reset.h"
>
> #include "sdma/sdma_4_4_2_offset.h"
> #include "sdma/sdma_4_4_2_sh_mask.h"
> @@ -1480,6 +1481,7 @@ static int sdma_v4_4_2_sw_init(struct amdgpu_ip_block *ip_block)
> if (r)
> return r;
> INIT_LIST_HEAD(&adev->sdma.reset_callback_list);
> +
> return r;
> }
>
> @@ -1608,10 +1610,10 @@ static int sdma_v4_4_2_reset_queue(struct amdgpu_ring *ring, unsigned int vmid)
> {
> struct amdgpu_device *adev = ring->adev;
> u32 id = GET_INST(SDMA0, ring->me);
> - return amdgpu_sdma_reset_instance(adev, id);
> + return amdgpu_sdma_reset_instance(adev, id, AMDGPU_RESET_SRC_SDMA_RING);
> }
>
> -static int sdma_v4_4_2_stop_queue(struct amdgpu_device *adev, uint32_t instance_id)
> +static int sdma_v4_4_2_stop_queue(struct amdgpu_device *adev, uint32_t instance_id, int src)
> {
> u32 inst_mask;
> struct amdgpu_ring *ring = &adev->sdma.instance[instance_id].ring;
> @@ -1628,7 +1630,7 @@ static int sdma_v4_4_2_stop_queue(struct amdgpu_device *adev, uint32_t instance_
> return 0;
> }
>
> -static int sdma_v4_4_2_restore_queue(struct amdgpu_device *adev, uint32_t instance_id)
> +static int sdma_v4_4_2_restore_queue(struct amdgpu_device *adev, uint32_t instance_id, int src)
> {
> int i;
> u32 inst_mask;
