[PATCH 3/4] drm/amdgpu: Don't modify grace_period in helper function

[Kim, Jonathan]

[Public]

> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces at lists.freedesktop.org> On Behalf Of Harish
> Kasiviswanathan
> Sent: Wednesday, February 12, 2025 5:04 PM
> To: amd-gfx at lists.freedesktop.org
> Cc: Kasiviswanathan, Harish <Harish.Kasiviswanathan at amd.com>
> Subject: [PATCH 3/4] drm/amdgpu: Don't modify grace_period in helper function
>
> build_grace_period_packet_info is asic helper function that fetches the
> correct format. It is the responsibility of the caller to validate the
> value.
>
> Signed-off-by: Harish Kasiviswanathan <Harish.Kasiviswanathan at amd.com>
> ---
>  .../drm/amd/amdgpu/amdgpu_amdkfd_gfx_v10.c    | 18 +++++----------
>  .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c | 17 +++++---------
>  .../drm/amd/amdkfd/kfd_packet_manager_v9.c    | 23 +++++++++++++------
>  3 files changed, 28 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v10.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v10.c
> index 62176d607bef..8e72dcff8867 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v10.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v10.c
> @@ -1029,18 +1029,12 @@ void
> kgd_gfx_v10_build_grace_period_packet_info(struct amdgpu_device *adev,
>  {
>       *reg_data = wait_times;
>
> -     /*
> -      * The CP cannont handle a 0 grace period input and will result in
> -      * an infinite grace period being set so set to 1 to prevent this.
> -      */
> -     if (grace_period == 0)
> -             grace_period = 1;
> -
> -     *reg_data = REG_SET_FIELD(*reg_data,
> -                     CP_IQ_WAIT_TIME2,
> -                     SCH_WAVE,
> -                     grace_period);
> -
> +     if (grace_period) {

Should we get rid of the notion of grace_period in the function name as well as the param since you're doing it for the rest of the patch?

> +             *reg_data = REG_SET_FIELD(*reg_data,
> +                             CP_IQ_WAIT_TIME2,
> +                             SCH_WAVE,
> +                             grace_period);
> +     }
>       *reg_offset = SOC15_REG_OFFSET(GC, 0, mmCP_IQ_WAIT_TIME2);
>  }
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
> index 441568163e20..04c86a229a23 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
> @@ -1085,17 +1085,12 @@ void
> kgd_gfx_v9_build_grace_period_packet_info(struct amdgpu_device *adev,
>  {
>       *reg_data = wait_times;
>
> -     /*
> -      * The CP cannot handle a 0 grace period input and will result in
> -      * an infinite grace period being set so set to 1 to prevent this.
> -      */
> -     if (grace_period == 0)
> -             grace_period = 1;
> -
> -     *reg_data = REG_SET_FIELD(*reg_data,
> -                     CP_IQ_WAIT_TIME2,
> -                     SCH_WAVE,
> -                     grace_period);
> +     if (grace_period) {
> +             *reg_data = REG_SET_FIELD(*reg_data,
> +                             CP_IQ_WAIT_TIME2,
> +                             SCH_WAVE,
> +                             grace_period);
> +     }
>
>       *reg_offset = SOC15_REG_OFFSET(GC, 0, mmCP_IQ_WAIT_TIME2);
>  }
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager_v9.c
> b/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager_v9.c
> index 6a5ddadec936..ecabf95d972f 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager_v9.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager_v9.c
> @@ -307,13 +307,22 @@ static int pm_set_compute_queue_wait_counts_v9(struct
> packet_manager *pm,
>       uint32_t reg_data = 0;
>
>       if (wait_counts_config == KFD_INIT_CP_QUEUE_WAIT_TIMES) {
> -                /* Set CWSR grace period to 1x1000 cycle for GFX9.4.3 APU */
> -                if (amdgpu_emu_mode == 0 && dqm->dev->adev->gmc.is_app_apu &&
> -                    KFD_GC_VERSION(dqm->dev) == IP_VERSION(9, 4, 3))
> -                        wait_counts_config = 1;
> -                else
> -                        return 0;
> -        }
> +             /* Set CWSR grace period to 1x1000 cycle for GFX9.4.3 APU */
> +             if (amdgpu_emu_mode == 0 && dqm->dev->adev->gmc.is_app_apu
> &&
> +                 KFD_GC_VERSION(dqm->dev) == IP_VERSION(9, 4, 3))
> +                     wait_counts_config = 1;
> +             else
> +                     return 0;
> +     } else if (!wait_counts_config)
> +             /*
> +              * NOTE: The CP cannot handle a 0 grace period input and will result
> in
> +              * an infinite grace period being. This is not an issue here since 0
> +              * would just set it to default value.
> +              * However, grace_period needs to be explicitly set to 1 for avoiding
> +              * any breakage for existing debugger user interface. Currently,
> +              * debugger interface expects value 1 when set to 0.
> +              */
> +             wait_counts_config = 1;

If you decide to follow my suggestions for patch 2, then you would only need the else-if condition.

Jon

>
>       pm->dqm->dev->kfd2kgd->build_grace_period_packet_info(
>                       pm->dqm->dev->adev,
> --
> 2.34.1