[PATCH 2/2] drm/amdgpu: allow APU to send power gate message when dpm is disabled

[Lazar, Lijo]

On 12/6/2021 2:14 PM, Lang Yu wrote:
> On 12/06/ , Lazar, Lijo wrote:
>>
>>
>> On 12/6/2021 12:18 PM, Yu, Lang wrote:
>>> [Public]
>>>
>>> A typo.
>>>
>>>> -----Original Message-----
>>>> From: Yu, Lang
>>>> Sent: Monday, December 6, 2021 2:47 PM
>>>> To: Lazar, Lijo <Lijo.Lazar at amd.com>; amd-gfx at lists.freedesktop.org
>>>> Cc: Deucher, Alexander <Alexander.Deucher at amd.com>; Huang, Ray
>>>> <Ray.Huang at amd.com>
>>>> Subject: RE: [PATCH 2/2] drm/amdgpu: allow APU to send power gate message
>>>> when dpm is disabled
>>>>
>>>> [Public]
>>>>
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: Lazar, Lijo <Lijo.Lazar at amd.com>
>>>>> Sent: Monday, December 6, 2021 11:41 AM
>>>>> To: Yu, Lang <Lang.Yu at amd.com>; amd-gfx at lists.freedesktop.org
>>>>> Cc: Deucher, Alexander <Alexander.Deucher at amd.com>; Huang, Ray
>>>>> <Ray.Huang at amd.com>
>>>>> Subject: Re: [PATCH 2/2] drm/amdgpu: allow APU to send power gate
>>>>> message when dpm is disabled
>>>>>
>>>>>
>>>>>
>>>>> On 12/6/2021 8:19 AM, Yu, Lang wrote:
>>>>>> [Public]
>>>>>>
>>>>>>
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Lazar, Lijo <Lijo.Lazar at amd.com>
>>>>>>> Sent: Friday, December 3, 2021 5:52 PM
>>>>>>> To: Yu, Lang <Lang.Yu at amd.com>; amd-gfx at lists.freedesktop.org
>>>>>>> Cc: Deucher, Alexander <Alexander.Deucher at amd.com>; Huang, Ray
>>>>>>> <Ray.Huang at amd.com>
>>>>>>> Subject: Re: [PATCH 2/2] drm/amdgpu: allow APU to send power gate
>>>>>>> message when dpm is disabled
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 12/3/2021 12:24 PM, Lang Yu wrote:
>>>>>>>> The general hw fini sequence is SMU-> ... ->SDMA-> ...
>>>>>>>> We need to send power gate message to power off SDMA(in SDMA
>>>>>>>> hw_fini()) afer dpm is disabled(in SMU hw_fini()). Allow that for APU.
>>>>>>>
>>>>>>> This message is not right. In APUs there is no message provided by
>>>>>>> FW to enable/disable DPM, it is done in BIOS. Rephrase to something
>>>>>>> like after smu hw_fini is completed.
>>>>>>
>>>>>> It is power on/off SDMA message. Not enable/disable DPM.
>>>>>>
>>>>> Bad choice of word :) I didn't mean FW message, it was about this line
>>>>> in "commit message" - "afer dpm is disabled".
>>>>
>>>> Ok. I got it.
>>>>
>>>>>
>>>>>>>>
>>>>>>>> Signed-off-by: Lang Yu <lang.yu at amd.com>
>>>>>>>> ---
>>>>>>>>      drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 2 +-
>>>>>>>>      1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
>>>>>>>> b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
>>>>>>>> index 2d718c30c8eb..285a237f3605 100644
>>>>>>>> --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
>>>>>>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
>>>>>>>> @@ -277,7 +277,7 @@ static int smu_dpm_set_power_gate(void *handle,
>>>>>>>>      	struct smu_context *smu = handle;
>>>>>>>>      	int ret = 0;
>>>>>>>>
>>>>>>>> -	if (!smu->pm_enabled || !smu->adev->pm.dpm_enabled) {
>>>>>>>> +	if (!smu->pm_enabled || (!smu->is_apu &&
>>>>>>>> +!smu->adev->pm.dpm_enabled)) {
>>>>>>>
>>>>>>>
>>>>>>> This check was there before also, only the WARN is added. That means
>>>>>>> it was skipping sending messages in APUs also and so far this was
>>>>>>> working fine (until this gets noticed because of the warning).
>>>>>>>
>>>>>>> Now this would try to send the message to APU without any check.
>>>>>>> That doesn't look good. Ideal way should be to fix the sequence.
>>>>>>> Otherwise, suggest to do something like below as the last step of
>>>>>>> smu hw cleanup rather than sending the message blindly.
>>>>>>>
>>>>>>> 	if (smu->is_apu)
>>>>>>> 		smu->pm.dpm_enabled = smu_is_dpm_running(smu);
>>>>>>
>>>>>> smu_is_dpm_running(smu) will cause errors in suspend.
>>>>>>
>>>>> That is interesting. What is the error you get?
>>>>
>>>> [drm:amdgpu_dpm_enable_uvd [amdgpu]] *ERROR* Dpm enable uvd failed, ret =
>>>> -95 That means EOPNOTSUPP.
>>>>
>>>> Actually, in resume process, but adev->in_suspend  is still true.
>>>> For Renoir series APU, smu_is_dpm_running is hardcoded as following,
>>>>
>>>> static bool renoir_is_dpm_running(struct smu_context *smu) {
>>>> 	struct amdgpu_device *adev = smu->adev;
>>>>
>>>> 	/*
>>>> 	 * Until now, the pmfw hasn't exported the interface of SMU
>>>> 	 * feature mask to APU SKU so just force on all the feature
>>>> 	 * at early initial stage.
>>>> 	 */
>>>> 	if (adev->in_suspend)
>>>> 		return false;
>>>> 	else
>>
>> Renoir suspend shouldn't be a special case. FW should keep running with
>> features enabled after driver suspend. Could you try with a return true all
>> the time for this?
> 
> That worked.
> 
> But we just send an IP power on/off message here.
> 
> Do we really need dpm running?
> 

Yes, but it is a power management message. From a FW perspective, power 
management starts when DPM is enabled. Without that, it's not bothered 
about any power management features. Only a few non-PM related messages 
like i2c table transfer etc. work when it is not enabled. Usually for 
APUs, DPM gets enabled early through BIOS and driver doesn't have much 
control.

If dpm_enabled is not causing any SW logical errors, better to keep it 
coherent with FW DPM for swsmu based ASICs.

Thanks,
Lijo

> Regards,
> Lang
> 
>> Thanks,
>> Lijo
>>
>>>> 		return true;
>>>>
>>>> }
>>>>
>>>> So we got such an error.
>>>>
>>>> Regards,
>>>> Lang
>>>>
>>>>> Thanks,
>>>>> Lijo
>>>>>
>>>>>> Here we just  send some IP power on/off messages.
>>>>>> Is it necessary to enable DPM to send such messages?
>>>>>>
>>>>>> Regards,
>>>>>> Lang
>>>>>>
>>>>>>> Thanks,
>>>>>>> Lijo
>>>>>>>
>>>>>>>>      		dev_WARN(smu->adev->dev,
>>>>>>>>      			 "SMU uninitialized but power %s requested for %u!\n",
>>>>>>>>      			 gate ? "gate" : "ungate", block_type);
>>>>>>>>