[PATCH] drm/amdgpu/pm: Fix the ppfeature value

[PATCH] drm/amdgpu/pm: Fix the ppfeature value

[Ma Jun]

Sometimes user may want to enable the od feature
by setting ppfeaturemask when loading amdgpu driver.
However,not all Asics support this feature.
So we need to restore the ppfeature value and print
a warning info.

Signed-off-by: Ma Jun <Jun.Ma2@amd.com>
---
 drivers/gpu/drm/amd/pm/amdgpu_dpm.c     | 15 ++++++++++++---
 drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h |  2 +-
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/pm/amdgpu_dpm.c b/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
index f84bfed50681..d777056b2f9d 100644
--- a/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
+++ b/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
@@ -1548,12 +1548,14 @@ int amdgpu_dpm_get_smu_prv_buf_details(struct amdgpu_device *adev,
 	return ret;
 }
 
-int amdgpu_dpm_is_overdrive_supported(struct amdgpu_device *adev)
+bool amdgpu_dpm_is_overdrive_supported(struct amdgpu_device *adev)
 {
+	bool od_support;
+
 	if (is_support_sw_smu(adev)) {
 		struct smu_context *smu = adev->powerplay.pp_handle;
 
-		return (smu->od_enabled || smu->is_apu);
+		od_support = (smu->od_enabled || smu->is_apu);
 	} else {
 		struct pp_hwmgr *hwmgr;
 
@@ -1566,8 +1568,15 @@ int amdgpu_dpm_is_overdrive_supported(struct amdgpu_device *adev)
 
 		hwmgr = (struct pp_hwmgr *)adev->powerplay.pp_handle;
 
-		return hwmgr->od_enabled;
+		od_support = hwmgr->od_enabled;
+	}
+
+	if (!od_support && (adev->pm.pp_feature & PP_OVERDRIVE_MASK)) {
+		adev->pm.pp_feature &= ~PP_OVERDRIVE_MASK;
+		DRM_WARN("overdrive feature is not supported\n");
 	}
+
+	return od_support;
 }
 
 int amdgpu_dpm_set_pp_table(struct amdgpu_device *adev,
diff --git a/drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h b/drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h
index 621200e0823f..0635f9d3a61a 100644
--- a/drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h
+++ b/drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h
@@ -551,7 +551,7 @@ int amdgpu_dpm_debugfs_print_current_performance_level(struct amdgpu_device *ade
 int amdgpu_dpm_get_smu_prv_buf_details(struct amdgpu_device *adev,
 				       void **addr,
 				       size_t *size);
-int amdgpu_dpm_is_overdrive_supported(struct amdgpu_device *adev);
+bool amdgpu_dpm_is_overdrive_supported(struct amdgpu_device *adev);
 int amdgpu_dpm_set_pp_table(struct amdgpu_device *adev,
 			    const char *buf,
 			    size_t size);
-- 
2.34.1

Re: [PATCH] drm/amdgpu/pm: Fix the ppfeature value

[Lazar, Lijo]

On 3/12/2024 4:29 PM, Ma Jun wrote:
> Sometimes user may want to enable the od feature
> by setting ppfeaturemask when loading amdgpu driver.
> However,not all Asics support this feature.
> So we need to restore the ppfeature value and print
> a warning info.
> 
> Signed-off-by: Ma Jun <Jun.Ma2@amd.com>
> ---
>  drivers/gpu/drm/amd/pm/amdgpu_dpm.c     | 15 ++++++++++++---
>  drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h |  2 +-
>  2 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/pm/amdgpu_dpm.c b/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
> index f84bfed50681..d777056b2f9d 100644
> --- a/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
> +++ b/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
> @@ -1548,12 +1548,14 @@ int amdgpu_dpm_get_smu_prv_buf_details(struct amdgpu_device *adev,
>  	return ret;
>  }
>  
> -int amdgpu_dpm_is_overdrive_supported(struct amdgpu_device *adev)
> +bool amdgpu_dpm_is_overdrive_supported(struct amdgpu_device *adev)
>  {
> +	bool od_support;
> +
>  	if (is_support_sw_smu(adev)) {
>  		struct smu_context *smu = adev->powerplay.pp_handle;
>  
> -		return (smu->od_enabled || smu->is_apu);
> +		od_support = (smu->od_enabled || smu->is_apu);
>  	} else {
>  		struct pp_hwmgr *hwmgr;
>  
> @@ -1566,8 +1568,15 @@ int amdgpu_dpm_is_overdrive_supported(struct amdgpu_device *adev)
>  
>  		hwmgr = (struct pp_hwmgr *)adev->powerplay.pp_handle;
>  
> -		return hwmgr->od_enabled;
> +		od_support = hwmgr->od_enabled;
> +	}
> +
> +	if (!od_support && (adev->pm.pp_feature & PP_OVERDRIVE_MASK)) {
> +		adev->pm.pp_feature &= ~PP_OVERDRIVE_MASK;
> +		DRM_WARN("overdrive feature is not supported\n");
>  	}
> +
> +	return od_support;

Instead of doing this inside DPM API, suggest to keep it outside towards
the end of initialization phase.

Thanks,
Lijo

>  }
>  
>  int amdgpu_dpm_set_pp_table(struct amdgpu_device *adev,
> diff --git a/drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h b/drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h
> index 621200e0823f..0635f9d3a61a 100644
> --- a/drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h
> +++ b/drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h
> @@ -551,7 +551,7 @@ int amdgpu_dpm_debugfs_print_current_performance_level(struct amdgpu_device *ade
>  int amdgpu_dpm_get_smu_prv_buf_details(struct amdgpu_device *adev,
>  				       void **addr,
>  				       size_t *size);
> -int amdgpu_dpm_is_overdrive_supported(struct amdgpu_device *adev);
> +bool amdgpu_dpm_is_overdrive_supported(struct amdgpu_device *adev);
>  int amdgpu_dpm_set_pp_table(struct amdgpu_device *adev,
>  			    const char *buf,
>  			    size_t size);

Re: [PATCH] drm/amdgpu/pm: Fix the ppfeature value

[Ma, Jun]

On 3/12/2024 8:57 PM, Lazar, Lijo wrote:
> 
> 
> On 3/12/2024 4:29 PM, Ma Jun wrote:
>> Sometimes user may want to enable the od feature
>> by setting ppfeaturemask when loading amdgpu driver.
>> However,not all Asics support this feature.
>> So we need to restore the ppfeature value and print
>> a warning info.
>>
>> Signed-off-by: Ma Jun <Jun.Ma2@amd.com>
>> ---
>>  drivers/gpu/drm/amd/pm/amdgpu_dpm.c     | 15 ++++++++++++---
>>  drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h |  2 +-
>>  2 files changed, 13 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/pm/amdgpu_dpm.c b/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
>> index f84bfed50681..d777056b2f9d 100644
>> --- a/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
>> +++ b/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
>> @@ -1548,12 +1548,14 @@ int amdgpu_dpm_get_smu_prv_buf_details(struct amdgpu_device *adev,
>>  	return ret;
>>  }
>>  
>> -int amdgpu_dpm_is_overdrive_supported(struct amdgpu_device *adev)
>> +bool amdgpu_dpm_is_overdrive_supported(struct amdgpu_device *adev)
>>  {
>> +	bool od_support;
>> +
>>  	if (is_support_sw_smu(adev)) {
>>  		struct smu_context *smu = adev->powerplay.pp_handle;
>>  
>> -		return (smu->od_enabled || smu->is_apu);
>> +		od_support = (smu->od_enabled || smu->is_apu);
>>  	} else {
>>  		struct pp_hwmgr *hwmgr;
>>  
>> @@ -1566,8 +1568,15 @@ int amdgpu_dpm_is_overdrive_supported(struct amdgpu_device *adev)
>>  
>>  		hwmgr = (struct pp_hwmgr *)adev->powerplay.pp_handle;
>>  
>> -		return hwmgr->od_enabled;
>> +		od_support = hwmgr->od_enabled;
>> +	}
>> +
>> +	if (!od_support && (adev->pm.pp_feature & PP_OVERDRIVE_MASK)) {
>> +		adev->pm.pp_feature &= ~PP_OVERDRIVE_MASK;
>> +		DRM_WARN("overdrive feature is not supported\n");
>>  	}
>> +
>> +	return od_support;
> 
> Instead of doing this inside DPM API, suggest to keep it outside towards
> the end of initialization phase.
> 

This function is called before the driver creates the OD related sys interface,
which is almost the last initialization phase. I think there's probably not much
need to break the whole logic and call this function again anywhere else.

Regards,
Ma Jun

> Thanks,
> Lijo
> 
>>  }
>>  
>>  int amdgpu_dpm_set_pp_table(struct amdgpu_device *adev,
>> diff --git a/drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h b/drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h
>> index 621200e0823f..0635f9d3a61a 100644
>> --- a/drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h
>> +++ b/drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h
>> @@ -551,7 +551,7 @@ int amdgpu_dpm_debugfs_print_current_performance_level(struct amdgpu_device *ade
>>  int amdgpu_dpm_get_smu_prv_buf_details(struct amdgpu_device *adev,
>>  				       void **addr,
>>  				       size_t *size);
>> -int amdgpu_dpm_is_overdrive_supported(struct amdgpu_device *adev);
>> +bool amdgpu_dpm_is_overdrive_supported(struct amdgpu_device *adev);
>>  int amdgpu_dpm_set_pp_table(struct amdgpu_device *adev,
>>  			    const char *buf,
>>  			    size_t size);

Re: [PATCH] drm/amdgpu/pm: Fix the ppfeature value

[Lazar, Lijo]

On 3/13/2024 8:15 AM, Ma, Jun wrote:
> 
> 
> On 3/12/2024 8:57 PM, Lazar, Lijo wrote:
>>
>>
>> On 3/12/2024 4:29 PM, Ma Jun wrote:
>>> Sometimes user may want to enable the od feature
>>> by setting ppfeaturemask when loading amdgpu driver.
>>> However,not all Asics support this feature.
>>> So we need to restore the ppfeature value and print
>>> a warning info.
>>>
>>> Signed-off-by: Ma Jun <Jun.Ma2@amd.com>
>>> ---
>>>  drivers/gpu/drm/amd/pm/amdgpu_dpm.c     | 15 ++++++++++++---
>>>  drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h |  2 +-
>>>  2 files changed, 13 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/pm/amdgpu_dpm.c b/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
>>> index f84bfed50681..d777056b2f9d 100644
>>> --- a/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
>>> +++ b/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
>>> @@ -1548,12 +1548,14 @@ int amdgpu_dpm_get_smu_prv_buf_details(struct amdgpu_device *adev,
>>>  	return ret;
>>>  }
>>>  
>>> -int amdgpu_dpm_is_overdrive_supported(struct amdgpu_device *adev)
>>> +bool amdgpu_dpm_is_overdrive_supported(struct amdgpu_device *adev)
>>>  {
>>> +	bool od_support;
>>> +
>>>  	if (is_support_sw_smu(adev)) {
>>>  		struct smu_context *smu = adev->powerplay.pp_handle;
>>>  
>>> -		return (smu->od_enabled || smu->is_apu);
>>> +		od_support = (smu->od_enabled || smu->is_apu);
>>>  	} else {
>>>  		struct pp_hwmgr *hwmgr;
>>>  
>>> @@ -1566,8 +1568,15 @@ int amdgpu_dpm_is_overdrive_supported(struct amdgpu_device *adev)
>>>  
>>>  		hwmgr = (struct pp_hwmgr *)adev->powerplay.pp_handle;
>>>  
>>> -		return hwmgr->od_enabled;
>>> +		od_support = hwmgr->od_enabled;
>>> +	}
>>> +
>>> +	if (!od_support && (adev->pm.pp_feature & PP_OVERDRIVE_MASK)) {
>>> +		adev->pm.pp_feature &= ~PP_OVERDRIVE_MASK;
>>> +		DRM_WARN("overdrive feature is not supported\n");
>>>  	}
>>> +
>>> +	return od_support;
>>
>> Instead of doing this inside DPM API, suggest to keep it outside towards
>> the end of initialization phase.
>>
> 
> This function is called before the driver creates the OD related sys interface,
> which is almost the last initialization phase. I think there's probably not much
> need to break the whole logic and call this function again anywhere else.
> 
amdgpu_dpm_* are meant as APIs and not meant to do internal manipulation
of pp_feature with every invocation.

This is indeed not the right way to do this. There is already another
place during driver initialization -
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/amd/pm/amdgpu_pm.c#L4277

This could be done even there.

Thanks,
Lijo

> Regards,
> Ma Jun
> 
>> Thanks,
>> Lijo
>>
>>>  }
>>>  
>>>  int amdgpu_dpm_set_pp_table(struct amdgpu_device *adev,
>>> diff --git a/drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h b/drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h
>>> index 621200e0823f..0635f9d3a61a 100644
>>> --- a/drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h
>>> +++ b/drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h
>>> @@ -551,7 +551,7 @@ int amdgpu_dpm_debugfs_print_current_performance_level(struct amdgpu_device *ade
>>>  int amdgpu_dpm_get_smu_prv_buf_details(struct amdgpu_device *adev,
>>>  				       void **addr,
>>>  				       size_t *size);
>>> -int amdgpu_dpm_is_overdrive_supported(struct amdgpu_device *adev);
>>> +bool amdgpu_dpm_is_overdrive_supported(struct amdgpu_device *adev);
>>>  int amdgpu_dpm_set_pp_table(struct amdgpu_device *adev,
>>>  			    const char *buf,
>>>  			    size_t size);