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

[Kasiviswanathan, Harish]

[Public]

From: Chen, Xiaogang <Xiaogang.Chen at amd.com>
Sent: Thursday, February 6, 2025 4:57 PM
To: Sakhnovitch, Elena (Elen) <Elena.Sakhnovitch at amd.com>; amd-gfx at lists.freedesktop.org
Cc: Kasiviswanathan, Harish <Harish.Kasiviswanathan at amd.com>
Subject: Re: [PATCH 2/3] drm/amdgpu: Don't modify grace_period in helper function


On 1/14/2025 1:52 PM, Elena Sakhnovitch wrote:
From: Harish Kasiviswanathan mailto:Harish.Kasiviswanathan at amd.com

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.
but what is hurt to valid it at asic function? each asic may has its own requirement on grace_period, so has its own checking.

[HK]:  build_grace_period_packet_info is a helper function to build the packet and that should be it. It need not check or update what value goes inside the packet.

Signed-off-by: Harish Kasiviswanathan mailto:Harish.Kasiviswanathan at amd.com
Signed-off-by: Elena Sakhnovitch mailto:Elena.Sakhnovitch at amd.com
---
 .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v10.c | 18 ++++++------------
 .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c  | 17 ++++++-----------
 .../gpu/drm/amd/amdkfd/kfd_packet_manager_v9.c | 12 ++++++++++++
 3 files changed, 24 insertions(+), 23 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) {
+               *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 fde212242129..adc7f7c78a18 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager_v9.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager_v9.c
@@ -306,6 +306,18 @@ static int pm_set_grace_period_v9(struct packet_manager *pm,
        uint32_t reg_offset = 0;
        uint32_t reg_data = 0;

+       /*
+        * 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) {
+               pr_debug("Invalid grace_period. Setting default value 0x%x\n",
+                        pm->dqm->wait_times);
+               if (WARN_ON((pm->dqm->wait_times & CP_IQ_WAIT_TIME2__SCH_WAVE_MASK)
+                       == 0))
+                       return -EINVAL;
should set grace_period to 1 here?
[HK]:  Previously grace_period==1 was chosen randomly to guard against invalid value 0. Instead of that better to set it to default value.


Regards
Xiaogang

+       }
+
        if (grace_period == SET_ASIC_OPTIMIZED_GRACE_PERIOD) {
                /* Set CWSR grace period to 1x1000 cycle for GFX9.4.3 APU */
                if (amdgpu_emu_mode == 0 && dqm->dev->adev->gmc.is_app_apu &&