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

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

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