[PATCH V3 3/3] drm/amd/pm: disable cstate feature for gpu reset scenario

[Quan, Evan]

[AMD Official Use Only - General]



> -----Original Message-----
> From: Lazar, Lijo <Lijo.Lazar at amd.com>
> Sent: Thursday, October 13, 2022 12:14 PM
> To: Quan, Evan <Evan.Quan at amd.com>; amd-gfx at lists.freedesktop.org
> Cc: Deucher, Alexander <Alexander.Deucher at amd.com>; Zhang, Hawking
> <Hawking.Zhang at amd.com>
> Subject: Re: [PATCH V3 3/3] drm/amd/pm: disable cstate feature for gpu
> reset scenario
> 
> 
> 
> On 10/13/2022 7:01 AM, Evan Quan wrote:
> > Suggested by PMFW team and same as what did for gfxoff feature.
> > This can address some Mode1Reset failures observed on SMU13.0.0.
> >
> > Signed-off-by: Evan Quan <evan.quan at amd.com>
> > Reviewed-by: Hawking Zhang <Hawking.Zhang at amd.com>
> > Change-Id: Ieb4e204c49abd405b1dce559c2ff75bb3887b6f9
> > --
> > v1->v2:
> >    - revise the code comments(Alex)
> >    - limit this to SMU13.0.0 and 13.0.7
> > v2->v3:
> >    - make this happen before display suspending
> 
> A better thing would be do
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c         | 8 ++++++++
> >   drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c  | 7 +++++++
> >   drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c | 7 +++++++
> >   3 files changed, 22 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > index ab8f970b2849..874bf623f394 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > @@ -2928,6 +2928,14 @@ static int
> amdgpu_device_ip_suspend_phase1(struct amdgpu_device *adev)
> >   	amdgpu_device_set_pg_state(adev, AMD_PG_STATE_UNGATE);
> >   	amdgpu_device_set_cg_state(adev, AMD_CG_STATE_UNGATE);
> >
> > +	/*
> > +	 * Per PMFW team's suggestion, driver needs to handle gfxoff
> > +	 * and df cstate features disablement for gpu reset(e.g. Mode1Reset)
> > +	 * scenario. Add the missing df cstate disablement here.
> > +	 */
> > +	if (amdgpu_dpm_set_df_cstate(adev, DF_CSTATE_DISALLOW))
> > +		dev_warn(adev->dev, "Failed to disallow df cstate");
> > +
> 
> If it's only related to display, you could move this right after below line so that
> headless systems don't need to take care of this. That will avoid any special
> handling needed for Aldebaran/Arcuturus also.
[Quan, Evan] Not quite sure. I know df cstate affects DAL a lot(MALL related features).
But I'm not sure whether there is other clients/IPs which are affected by df cstate.
I want this(cstate disablement) performed before all 'consumers'.

BR
Evan
> 
>                 if (adev->ip_blocks[i].version->type !=
> AMD_IP_BLOCK_TYPE_DCE)
>                          continue;
> 
> Thanks,
> Lijo
> 
> >   	for (i = adev->num_ip_blocks - 1; i >= 0; i--) {
> >   		if (!adev->ip_blocks[i].status.valid)
> >   			continue;
> > diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
> > b/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
> > index 445005571f76..7d34f40460eb 100644
> > --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
> > +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
> > @@ -2245,6 +2245,13 @@ static int arcturus_set_df_cstate(struct
> smu_context *smu,
> >   	uint32_t smu_version;
> >   	int ret;
> >
> > +	/*
> > +	 * Arcturus does not need the cstate disablement
> > +	 * prerequisite for gpu reset.
> > +	 */
> > +	if (amdgpu_in_reset(adev) || adev->in_suspend)
> > +		return 0;
> > +
> >   	ret = smu_cmn_get_smc_version(smu, NULL, &smu_version);
> >   	if (ret) {
> >   		dev_err(smu->adev->dev, "Failed to get smu version!\n");
> diff
> > --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
> > b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
> > index 619aee51b123..93a0f7f6a34e 100644
> > --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
> > +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
> > @@ -1640,6 +1640,13 @@ static bool aldebaran_is_baco_supported(struct
> smu_context *smu)
> >   static int aldebaran_set_df_cstate(struct smu_context *smu,
> >   				   enum pp_df_cstate state)
> >   {
> > +	/*
> > +	 * Aldebaran does not need the cstate disablement
> > +	 * prerequisite for gpu reset.
> > +	 */
> > +	if (amdgpu_in_reset(adev) || adev->in_suspend)
> > +		return 0;
> > +
> >   	return smu_cmn_send_smc_msg_with_param(smu,
> SMU_MSG_DFCstateControl, state, NULL);
> >   }
> >