[PATCH 1/2] drm/amdgpu: remove power on/off SDMA in SMU hw_init/fini()

[PATCH 1/2] drm/amdgpu: remove power on/off SDMA in SMU hw_init/fini()

[Lang Yu]

Currently, we don't find some neccesities to power on/off
SDMA in SMU hw_init/fini(). It makes more sense in SDMA
hw_init/fini().

Signed-off-by: Lang Yu <lang.yu@amd.com>
---
 drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
index 5839918cb574..2d718c30c8eb 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
@@ -1350,7 +1350,6 @@ static int smu_hw_init(void *handle)
 	}
 
 	if (smu->is_apu) {
-		smu_powergate_sdma(&adev->smu, false);
 		smu_dpm_set_vcn_enable(smu, true);
 		smu_dpm_set_jpeg_enable(smu, true);
 		smu_set_gfx_cgpg(&adev->smu, true);
@@ -1512,10 +1511,6 @@ static int smu_hw_fini(void *handle)
 	if (amdgpu_sriov_vf(adev)&& !amdgpu_sriov_is_pp_one_vf(adev))
 		return 0;
 
-	if (smu->is_apu) {
-		smu_powergate_sdma(&adev->smu, true);
-	}
-
 	smu_dpm_set_vcn_enable(smu, false);
 	smu_dpm_set_jpeg_enable(smu, false);
 
-- 
2.25.1

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

[Lang Yu]

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.

Signed-off-by: Lang Yu <lang.yu@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)) {
 		dev_WARN(smu->adev->dev,
 			 "SMU uninitialized but power %s requested for %u!\n",
 			 gate ? "gate" : "ungate", block_type);
-- 
2.25.1

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

[Lazar, Lijo]

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.

> 
> Signed-off-by: Lang Yu <lang.yu@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);

Thanks,
Lijo

>   		dev_WARN(smu->adev->dev,
>   			 "SMU uninitialized but power %s requested for %u!\n",
>   			 gate ? "gate" : "ungate", block_type);
> 

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

[Yu, Lang]

[Public]



>-----Original Message-----
>From: Lazar, Lijo <Lijo.Lazar@amd.com>
>Sent: Friday, December 3, 2021 5:52 PM
>To: Yu, Lang <Lang.Yu@amd.com>; amd-gfx@lists.freedesktop.org
>Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Ray
><Ray.Huang@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.

>>
>> Signed-off-by: Lang Yu <lang.yu@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.

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

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

[Lazar, Lijo]

On 12/6/2021 8:19 AM, Yu, Lang wrote:
> [Public]
> 
> 
> 
>> -----Original Message-----
>> From: Lazar, Lijo <Lijo.Lazar@amd.com>
>> Sent: Friday, December 3, 2021 5:52 PM
>> To: Yu, Lang <Lang.Yu@amd.com>; amd-gfx@lists.freedesktop.org
>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Ray
>> <Ray.Huang@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".

>>>
>>> Signed-off-by: Lang Yu <lang.yu@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?

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

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

[Yu, Lang]

[Public]



>-----Original Message-----
>From: Lazar, Lijo <Lijo.Lazar@amd.com>
>Sent: Monday, December 6, 2021 11:41 AM
>To: Yu, Lang <Lang.Yu@amd.com>; amd-gfx@lists.freedesktop.org
>Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Ray
><Ray.Huang@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@amd.com>
>>> Sent: Friday, December 3, 2021 5:52 PM
>>> To: Yu, Lang <Lang.Yu@amd.com>; amd-gfx@lists.freedesktop.org
>>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Ray
>>> <Ray.Huang@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@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 false.
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
		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);
>>>>

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

[Yu, Lang]

[Public]

A typo.

>-----Original Message-----
>From: Yu, Lang
>Sent: Monday, December 6, 2021 2:47 PM
>To: Lazar, Lijo <Lijo.Lazar@amd.com>; amd-gfx@lists.freedesktop.org
>Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Ray
><Ray.Huang@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@amd.com>
>>Sent: Monday, December 6, 2021 11:41 AM
>>To: Yu, Lang <Lang.Yu@amd.com>; amd-gfx@lists.freedesktop.org
>>Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Ray
>><Ray.Huang@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@amd.com>
>>>> Sent: Friday, December 3, 2021 5:52 PM
>>>> To: Yu, Lang <Lang.Yu@amd.com>; amd-gfx@lists.freedesktop.org
>>>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Ray
>>>> <Ray.Huang@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@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
>		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);
>>>>>

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

[Lazar, Lijo]

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@amd.com>; amd-gfx@lists.freedesktop.org
>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Ray
>> <Ray.Huang@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@amd.com>
>>> Sent: Monday, December 6, 2021 11:41 AM
>>> To: Yu, Lang <Lang.Yu@amd.com>; amd-gfx@lists.freedesktop.org
>>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Ray
>>> <Ray.Huang@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@amd.com>
>>>>> Sent: Friday, December 3, 2021 5:52 PM
>>>>> To: Yu, Lang <Lang.Yu@amd.com>; amd-gfx@lists.freedesktop.org
>>>>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Ray
>>>>> <Ray.Huang@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@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?

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

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

[Lang Yu]

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@amd.com>; amd-gfx@lists.freedesktop.org
> > > Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Ray
> > > <Ray.Huang@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@amd.com>
> > > > Sent: Monday, December 6, 2021 11:41 AM
> > > > To: Yu, Lang <Lang.Yu@amd.com>; amd-gfx@lists.freedesktop.org
> > > > Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Ray
> > > > <Ray.Huang@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@amd.com>
> > > > > > Sent: Friday, December 3, 2021 5:52 PM
> > > > > > To: Yu, Lang <Lang.Yu@amd.com>; amd-gfx@lists.freedesktop.org
> > > > > > Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Ray
> > > > > > <Ray.Huang@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@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?

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

Re: [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@amd.com>; amd-gfx@lists.freedesktop.org
>>>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Ray
>>>> <Ray.Huang@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@amd.com>
>>>>> Sent: Monday, December 6, 2021 11:41 AM
>>>>> To: Yu, Lang <Lang.Yu@amd.com>; amd-gfx@lists.freedesktop.org
>>>>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Ray
>>>>> <Ray.Huang@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@amd.com>
>>>>>>> Sent: Friday, December 3, 2021 5:52 PM
>>>>>>> To: Yu, Lang <Lang.Yu@amd.com>; amd-gfx@lists.freedesktop.org
>>>>>>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Ray
>>>>>>> <Ray.Huang@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@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);
>>>>>>>>

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

[Lang Yu]

On 12/06/ , Lazar, Lijo wrote:
> 
> 
> 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@amd.com>; amd-gfx@lists.freedesktop.org
> > > > > Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Ray
> > > > > <Ray.Huang@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@amd.com>
> > > > > > Sent: Monday, December 6, 2021 11:41 AM
> > > > > > To: Yu, Lang <Lang.Yu@amd.com>; amd-gfx@lists.freedesktop.org
> > > > > > Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Ray
> > > > > > <Ray.Huang@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@amd.com>
> > > > > > > > Sent: Friday, December 3, 2021 5:52 PM
> > > > > > > > To: Yu, Lang <Lang.Yu@amd.com>; amd-gfx@lists.freedesktop.org
> > > > > > > > Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Ray
> > > > > > > > <Ray.Huang@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@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.

As you said for APUs, DPM gets enabled early through BIOS.
Are there any cases it is disabled by diver or BIOS or itself after that?

1, If not, why we query if it is running for APUs.
2, If yes, who enable it again for APus?

Regards,
Lang

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