[AMD Official Use Only - AMD Internal Distribution Only]

> -----Original Message-----
> From: amd-gfx <amd-gfx-boun...@lists.freedesktop.org> On Behalf Of Lijo Lazar
> Sent: Tuesday, November 5, 2024 1:39 PM
> To: amd-gfx@lists.freedesktop.org
> Cc: Zhang, Hawking <hawking.zh...@amd.com>; Deucher, Alexander
> <alexander.deuc...@amd.com>; Koenig, Christian <christian.koe...@amd.com>
> Subject: [PATCH 1/2] drm/amdgpu: Fix unmap queue logic
>
> In current logic, it calls ring_alloc followed by a ring_test. ring_test in 
> turn will call
> another ring_alloc. This is illegal usage as a ring_alloc is expected to be 
> closed
> properly with a ring_commit. Change to commit the unmap queue packet first
> followed by a ring_test. Add a comment about the usage of ring_test.

Regarding the description " This is illegal usage as a ring_alloc is expected 
to be closed properly with a ring_commit ", does this only apply to unmap queue 
logic ?
The current logic to do map queue is also to commit once in ring_test but 
ring_alloc twice.

>
> Also, reorder the current pre-condition checks of job hang or kiq ring 
> scheduler not
> ready. Without them being met, it is not useful to attempt ring or memory 
> allocations.
>
> Fixes tag refers to the original patch which introduced this issue which then 
> got
> carried over into newer code.
>
> Signed-off-by: Lijo Lazar <lijo.la...@amd.com>
>
> Fixes: 6c10b5cc4eaa ("drm/amdgpu: Remove duplicate code in gfx_v8_0.c")
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c | 13 +++++-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c    | 47 ++++++++++++++--------
>  drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c      |  7 ++++
>  3 files changed, 49 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> index c218e8f117e4..2a40150ae10f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> @@ -844,6 +844,9 @@ int amdgpu_amdkfd_unmap_hiq(struct amdgpu_device
> *adev, u32 doorbell_off,
>       if (!kiq->pmf || !kiq->pmf->kiq_unmap_queues)
>               return -EINVAL;
>
> +     if (!kiq_ring->sched.ready || adev->job_hang)
> +             return 0;
> +
>       ring_funcs = kzalloc(sizeof(*ring_funcs), GFP_KERNEL);
>       if (!ring_funcs)
>               return -ENOMEM;
> @@ -868,8 +871,14 @@ int amdgpu_amdkfd_unmap_hiq(struct amdgpu_device
> *adev, u32 doorbell_off,
>
>       kiq->pmf->kiq_unmap_queues(kiq_ring, ring, RESET_QUEUES, 0, 0);
>
> -     if (kiq_ring->sched.ready && !adev->job_hang)
> -             r = amdgpu_ring_test_helper(kiq_ring);
> +     /* Submit unmap queue packet */
> +     amdgpu_ring_commit(kiq_ring);
> +     /*
> +      * Ring test will do a basic scratch register change check. Just run
> +      * this to ensure that unmap queues that is submitted before got
> +      * processed successfully before returning.
> +      */
> +     r = amdgpu_ring_test_helper(kiq_ring);
>
>       spin_unlock(&kiq->ring_lock);
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> index dabc01cf1fbb..6733ff5d9628 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> @@ -515,6 +515,17 @@ int amdgpu_gfx_disable_kcq(struct amdgpu_device *adev,
> int xcc_id)
>       if (!kiq->pmf || !kiq->pmf->kiq_unmap_queues)
>               return -EINVAL;
>
> +     if (!kiq_ring->sched.ready || adev->job_hang)
> +             return 0;
> +     /**
> +      * This is workaround: only skip kiq_ring test
> +      * during ras recovery in suspend stage for gfx9.4.3
> +      */
> +     if ((amdgpu_ip_version(adev, GC_HWIP, 0) == IP_VERSION(9, 4, 3) ||
> +          amdgpu_ip_version(adev, GC_HWIP, 0) == IP_VERSION(9, 4, 4)) &&
> +         amdgpu_ras_in_recovery(adev))
> +             return 0;
> +
>       spin_lock(&kiq->ring_lock);
>       if (amdgpu_ring_alloc(kiq_ring, kiq->pmf->unmap_queues_size *
>                                       adev->gfx.num_compute_rings)) {
> @@ -528,20 +539,15 @@ int amdgpu_gfx_disable_kcq(struct amdgpu_device
> *adev, int xcc_id)
>                                          &adev->gfx.compute_ring[j],
>                                          RESET_QUEUES, 0, 0);
>       }
> -
> -     /**
> -      * This is workaround: only skip kiq_ring test
> -      * during ras recovery in suspend stage for gfx9.4.3
> +     /* Submit unmap queue packet */
> +     amdgpu_ring_commit(kiq_ring);
> +     /*
> +      * Ring test will do a basic scratch register change check. Just run
> +      * this to ensure that unmap queues that is submitted before got
> +      * processed successfully before returning.
>        */
> -     if ((amdgpu_ip_version(adev, GC_HWIP, 0) == IP_VERSION(9, 4, 3) ||
> -         amdgpu_ip_version(adev, GC_HWIP, 0) == IP_VERSION(9, 4, 4)) &&
> -         amdgpu_ras_in_recovery(adev)) {
> -             spin_unlock(&kiq->ring_lock);
> -             return 0;
> -     }
> +     r = amdgpu_ring_test_helper(kiq_ring);
>
> -     if (kiq_ring->sched.ready && !adev->job_hang)
> -             r = amdgpu_ring_test_helper(kiq_ring);
>       spin_unlock(&kiq->ring_lock);
>
>       return r;
> @@ -569,8 +575,11 @@ int amdgpu_gfx_disable_kgq(struct amdgpu_device *adev,
> int xcc_id)
>       if (!kiq->pmf || !kiq->pmf->kiq_unmap_queues)
>               return -EINVAL;
>
> -     spin_lock(&kiq->ring_lock);
> +     if (!adev->gfx.kiq[0].ring.sched.ready || adev->job_hang)
> +             return 0;
> +
>       if (amdgpu_gfx_is_master_xcc(adev, xcc_id)) {
> +             spin_lock(&kiq->ring_lock);
>               if (amdgpu_ring_alloc(kiq_ring, kiq->pmf->unmap_queues_size *
>                                               adev->gfx.num_gfx_rings)) {
>                       spin_unlock(&kiq->ring_lock);
> @@ -583,11 +592,17 @@ int amdgpu_gfx_disable_kgq(struct amdgpu_device
> *adev, int xcc_id)
>                                                  &adev->gfx.gfx_ring[j],
>                                                  PREEMPT_QUEUES, 0, 0);
>               }
> -     }
> +             /* Submit unmap queue packet */
> +             amdgpu_ring_commit(kiq_ring);
>
> -     if (adev->gfx.kiq[0].ring.sched.ready && !adev->job_hang)
> +             /*
> +              * Ring test will do a basic scratch register change check.
> +              * Just run this to ensure that unmap queues that is submitted
> +              * before got processed successfully before returning.
> +              */
>               r = amdgpu_ring_test_helper(kiq_ring);
> -     spin_unlock(&kiq->ring_lock);
> +             spin_unlock(&kiq->ring_lock);
> +     }
>
>       return r;
>  }
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> index f85e545653c7..553a6113fa67 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> @@ -4823,6 +4823,13 @@ static int gfx_v8_0_kcq_disable(struct amdgpu_device
> *adev)
>               amdgpu_ring_write(kiq_ring, 0);
>               amdgpu_ring_write(kiq_ring, 0);
>       }
> +     /* Submit unmap queue packet */
> +     amdgpu_ring_commit(kiq_ring);
> +     /*
> +      * Ring test will do a basic scratch register change check. Just run
> +      * this to ensure that unmap queues that is submitted before got
> +      * processed successfully before returning.
> +      */
>       r = amdgpu_ring_test_helper(kiq_ring);
>       if (r)
>               DRM_ERROR("KCQ disable failed\n");
> --
> 2.25.1

Reply via email to