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

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

On 10/13/2022 10:56 AM, Quan, Evan wrote:
> [AMD Official Use Only - General]
> 
> 
> 
>> -----Original Message-----
>> From: Lazar, Lijo <Lijo.Lazar@amd.com>
>> Sent: Thursday, October 13, 2022 12:14 PM
>> To: Quan, Evan <Evan.Quan@amd.com>; amd-gfx@lists.freedesktop.org
>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Zhang, Hawking
>> <Hawking.Zhang@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@amd.com>
>>> Reviewed-by: Hawking Zhang <Hawking.Zhang@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'.
>

Thanks for the clarification.

Series is
	Reviewed-by: Lijo Lazar <lijo.lazar@amd.com>

Thanks,
Lijo

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