Re: [PATCH] drm/amdgpu: correct the wrong cached state for GMC on PICASSO

[Lazar, Lijo]

On 12/14/2021 1:16 PM, Quan, Evan wrote:
[AMD Official Use Only]



-----Original Message-----
From: Lazar, Lijo <lijo.la...@amd.com>
Sent: Tuesday, December 14, 2021 2:13 PM
To: Quan, Evan <evan.q...@amd.com>; amd-gfx@lists.freedesktop.org
Cc: Deucher, Alexander <alexander.deuc...@amd.com>; Limonciello, Mario
<mario.limoncie...@amd.com>
Subject: Re: [PATCH] drm/amdgpu: correct the wrong cached state for GMC
on PICASSO



On 12/14/2021 7:04 AM, Evan Quan wrote:
Pair the operations did in GMC ->hw_init and ->hw_fini. That can help
to maintain correct cached state for GMC and avoid unintention gate
operation dropping due to wrong cached state.

BUG: https://gitlab.freedesktop.org/drm/amd/-/issues/1828

Signed-off-by: Evan Quan <evan.q...@amd.com>
Change-Id: I9976672a64464b86bb45eed0c25c9599d3bb4c06
---
   drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c            | 8 ++++++++
   drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c          | 8 ++++----
   drivers/gpu/drm/amd/pm/powerplay/amd_powerplay.c | 7 ++++++-
   3 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
index db2ec84f7237..c7492db3e189 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
@@ -1809,6 +1809,14 @@ static int gmc_v9_0_hw_fini(void *handle)
                return 0;
        }

+       /*
+        * Pair the operations did in gmc_v9_0_hw_init and thus maintain
+        * a correct cached state for GMC. Otherwise, the "gate" again
+        * operation on S3 resuming will fail due to wrong cached state.
+        */
+       if (adev->mmhub.funcs->update_power_gating)
+               adev->mmhub.funcs->update_power_gating(adev, false);
+
        amdgpu_irq_put(adev, &adev->gmc.ecc_irq, 0);
        amdgpu_irq_put(adev, &adev->gmc.vm_fault, 0);

diff --git a/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c
b/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c
index b3bede1dc41d..1da2ec692057 100644
--- a/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c
@@ -301,10 +301,10 @@ static void
mmhub_v1_0_update_power_gating(struct amdgpu_device *adev,
        if (amdgpu_sriov_vf(adev))
                return;

-       if (enable && adev->pg_flags & AMD_PG_SUPPORT_MMHUB) {
-               amdgpu_dpm_set_powergating_by_smu(adev,
AMD_IP_BLOCK_TYPE_GMC, true);
-
-       }
+       if (adev->pg_flags & AMD_PG_SUPPORT_MMHUB)
+               amdgpu_dpm_set_powergating_by_smu(adev,
+                                                 AMD_IP_BLOCK_TYPE_GMC,
+                                                 enable);
   }

   static int mmhub_v1_0_gart_enable(struct amdgpu_device *adev) diff
--git a/drivers/gpu/drm/amd/pm/powerplay/amd_powerplay.c
b/drivers/gpu/drm/amd/pm/powerplay/amd_powerplay.c
index 3656a77baea4..9953a77cb987 100644
--- a/drivers/gpu/drm/amd/pm/powerplay/amd_powerplay.c
+++ b/drivers/gpu/drm/amd/pm/powerplay/amd_powerplay.c
@@ -1167,7 +1167,12 @@ static int pp_set_powergating_by_smu(void
*handle,
                pp_dpm_powergate_vce(handle, gate);
                break;
        case AMD_IP_BLOCK_TYPE_GMC:
-               pp_dpm_powergate_mmhub(handle);
+               /*
+                * For now, this is only used on PICASSO.
+                * And only "gate" operation is supported.
+                */
+               if (gate)
+                       pp_dpm_powergate_mmhub(handle);

This is a generic entry point. Would rather keep PG enable or disable
supported in mmhub_v1_0 rather than than here. And this comment also
should be in mmhub_v1_0.
[Quan, Evan] pp_dpm_powergate_mmhub is not a generic interface. It's customized for 
PICASSO which accepts no parameter("bool gate") as other interfaces.
Thus some comments which explain why the interface is so special are proper I 
think.


amd_powerplay is the generic entry point and we should avoid ASIC 
specific things as much as possible. Ideally this should be handled in 
mmhub v1/smu10 hwmgr and not here.

Thanks,
Lijo

BR
Evan

Thanks,
Lijo

                break;
        case AMD_IP_BLOCK_TYPE_GFX:
                ret = pp_dpm_powergate_gfx(handle, gate);