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

["Lazar, Lijo" <lijo.lazar@xxxxxxx>]

On 12/14/2021 1:16 PM, Quan, Evan wrote:
> [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.

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);