[AMD Official Use Only] > -----Original Message----- > From: Lazar, Lijo <Lijo.Lazar@xxxxxxx> > Sent: Tuesday, December 14, 2021 2:13 PM > To: Quan, Evan <Evan.Quan@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx > Cc: Deucher, Alexander <Alexander.Deucher@xxxxxxx>; Limonciello, Mario > <Mario.Limonciello@xxxxxxx> > 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.quan@xxxxxxx> > > 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. BR Evan > > Thanks, > Lijo > > > break; > > case AMD_IP_BLOCK_TYPE_GFX: > > ret = pp_dpm_powergate_gfx(handle, gate); > >
