[PATCH v2] drm/amd/pm: consistent approach for smartshift
Sundararaju, Sathishkumar
sathishkumar.sundararaju at amd.com
Thu May 12 12:12:32 UTC 2022
On 5/12/2022 4:37 PM, Lazar, Lijo wrote:
>
>
> On 5/12/2022 4:26 PM, Sathishkumar S wrote:
>> create smartshift sysfs attributes from dGPU device even
>> on smartshift 1.0 platform to be consistent. Do not populate
>> the attributes on platforms that have APU only but not dGPU
>> or vice versa.
>>
>> V2: avoid checking for the number of VGA/DISPLAY devices (Lijo)
>>
>> Suggested-by: Alex Deucher <alexander.deucher at amd.com>
>> Signed-off-by: Sathishkumar S <sathishkumar.sundararaju at amd.com>
>> Acked-by: Alex Deucher <alexander.deucher at amd.com>
>> ---
>> drivers/gpu/drm/amd/pm/amdgpu_pm.c | 136 ++++++++++++++++-------------
>> 1 file changed, 74 insertions(+), 62 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/pm/amdgpu_pm.c
>> b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
>> index d3228216b2da..292e8c241597 100644
>> --- a/drivers/gpu/drm/amd/pm/amdgpu_pm.c
>> +++ b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
>> @@ -1734,22 +1734,11 @@ static ssize_t amdgpu_get_gpu_metrics(struct
>> device *dev,
>> return size;
>> }
>> -/**
>> - * DOC: smartshift_apu_power
>> - *
>> - * The amdgpu driver provides a sysfs API for reporting APU power
>> - * share if it supports smartshift. The value is expressed as
>> - * the proportion of stapm limit where stapm limit is the total APU
>> - * power limit. The result is in percentage. If APU power is 130% of
>> - * STAPM, then APU is using 30% of the dGPU's headroom.
>> - */
>> -
>> -static ssize_t amdgpu_get_smartshift_apu_power(struct device *dev,
>> struct device_attribute *attr,
>> - char *buf)
>> +static int amdgpu_read_powershift_percent(struct amdgpu_device *adev,
>> + uint32_t *ss_power, bool dgpu_share)
>> {
>> - struct drm_device *ddev = dev_get_drvdata(dev);
>> - struct amdgpu_device *adev = drm_to_adev(ddev);
>> - uint32_t ss_power, size;
>> + struct drm_device *ddev = adev_to_drm(adev);
>> + uint32_t size;
>> int r = 0;
>> if (amdgpu_in_reset(adev))
>> @@ -1763,28 +1752,65 @@ static ssize_t
>> amdgpu_get_smartshift_apu_power(struct device *dev, struct device
>> return r;
>> }
>> - r = amdgpu_dpm_read_sensor(adev, AMDGPU_PP_SENSOR_SS_APU_SHARE,
>> - (void *)&ss_power, &size);
>> - if (r)
>> - goto out;
>> -
>> - r = sysfs_emit(buf, "%u%%\n", ss_power);
>> + if (dgpu_share)
>> + r = amdgpu_dpm_read_sensor(adev,
>> AMDGPU_PP_SENSOR_SS_DGPU_SHARE,
>> + (void *)ss_power, &size);
>> + else
>> + r = amdgpu_dpm_read_sensor(adev, AMDGPU_PP_SENSOR_SS_APU_SHARE,
>> + (void *)ss_power, &size);
>> -out:
>> pm_runtime_mark_last_busy(ddev->dev);
>> pm_runtime_put_autosuspend(ddev->dev);
>> return r;
>> }
>> +/**
>> + * DOC: smartshift_apu_power
>> + *
>> + * The amdgpu driver provides a sysfs API for reporting APU power
>> + * shift in percentage if platform supports smartshift. Value 0
>> means that
>> + * there is no powershift and values between [1-100] means that the
>> power
>> + * is shifted to APU, the percentage of boost is with respect to APU
>> power
>> + * limit on the platform.
>> + */
>> +
>> +static ssize_t amdgpu_get_smartshift_apu_power(struct device *dev,
>> struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct drm_device *ddev = dev_get_drvdata(dev);
>> + struct amdgpu_device *adev = drm_to_adev(ddev);
>> + uint32_t ss_power = 0;
>> + int r = 0, i;
>> +
>> + r = amdgpu_read_powershift_percent(adev, &ss_power, false);
>> + if (r == -EOPNOTSUPP) {
>> + /* sensor not available on dGPU, try to read from APU */
>> + adev = NULL;
>> + mutex_lock(&mgpu_info.mutex);
>> + for (i = 0; i < mgpu_info.num_gpu; i++) {
>> + if (mgpu_info.gpu_ins[i].adev->flags & AMD_IS_APU) {
>> + adev = mgpu_info.gpu_ins[i].adev;
>> + break;
>> + }
>> + }
>> + mutex_unlock(&mgpu_info.mutex);
>> + if (adev)
>> + r = amdgpu_read_powershift_percent(adev, &ss_power, false);
>> + }
>> +
>> + if (!r)
>> + r = sysfs_emit(buf, "%u%%\n", ss_power);
>> +
>
> In v1 patch comment, I meant something like this
> return amdgpu_show_powershift_percent(adev, false);
>
> And the above logic is kept in amdgpu_show_powershift_percent();
>
> amdgpu_get_smartshift_dgpu_power():
> return amdgpu_show_powershift_percent(adev, true);
okay got it, will do that. But will retain
amdgpu_read_powershift_percent() as a separate function or rename it to
amdgpu_device_read_powershift().
I hope you also didn't mean to combine them all into
amdgpu_show_powershift_percent().
>
> Thanks,
> Lijo
>
>> + return r;
>> +}
>> /**
>> * DOC: smartshift_dgpu_power
>> *
>> - * The amdgpu driver provides a sysfs API for reporting the dGPU power
>> - * share if the device is in HG and supports smartshift. The value
>> - * is expressed as the proportion of stapm limit where stapm limit
>> - * is the total APU power limit. The value is in percentage. If dGPU
>> - * power is 20% higher than STAPM power(120%), it's using 20% of the
>> - * APU's power headroom.
>> + * The amdgpu driver provides a sysfs API for reporting dGPU power
>> + * shift in percentage if platform supports smartshift. Value 0
>> means that
>> + * there is no powershift and values between [1-100] means that the
>> power is
>> + * shifted to dGPU, the percentage of boost is with respect to dGPU
>> power
>> + * limit on the platform.
>> */
>> static ssize_t amdgpu_get_smartshift_dgpu_power(struct device
>> *dev, struct device_attribute *attr,
>> @@ -1792,31 +1818,28 @@ static ssize_t
>> amdgpu_get_smartshift_dgpu_power(struct device *dev, struct devic
>> {
>> struct drm_device *ddev = dev_get_drvdata(dev);
>> struct amdgpu_device *adev = drm_to_adev(ddev);
>> - uint32_t ss_power, size;
>> - int r = 0;
>> -
>> - if (amdgpu_in_reset(adev))
>> - return -EPERM;
>> - if (adev->in_suspend && !adev->in_runpm)
>> - return -EPERM;
>> -
>> - r = pm_runtime_get_sync(ddev->dev);
>> - if (r < 0) {
>> - pm_runtime_put_autosuspend(ddev->dev);
>> - return r;
>> + uint32_t ss_power = 0;
>> + int r = 0, i;
>> +
>> + r = amdgpu_read_powershift_percent(adev, &ss_power, true);
>> + if (r == -EOPNOTSUPP) {
>> + /* sensor not available on dGPU, try to read from APU */
>> + adev = NULL;
>> + mutex_lock(&mgpu_info.mutex);
>> + for (i = 0; i < mgpu_info.num_gpu; i++) {
>> + if (mgpu_info.gpu_ins[i].adev->flags & AMD_IS_APU) {
>> + adev = mgpu_info.gpu_ins[i].adev;
>> + break;
>> + }
>> + }
>> + mutex_unlock(&mgpu_info.mutex);
>> + if (adev)
>> + r = amdgpu_read_powershift_percent(adev, &ss_power, true);
>> }
>> - r = amdgpu_dpm_read_sensor(adev, AMDGPU_PP_SENSOR_SS_DGPU_SHARE,
>> - (void *)&ss_power, &size);
>> -
>> - if (r)
>> - goto out;
>> -
>> - r = sysfs_emit(buf, "%u%%\n", ss_power);
>> + if (!r)
>> + r = sysfs_emit(buf, "%u%%\n", ss_power);
>> -out:
>> - pm_runtime_mark_last_busy(ddev->dev);
>> - pm_runtime_put_autosuspend(ddev->dev);
>> return r;
>> }
>> @@ -1884,18 +1907,7 @@ static ssize_t
>> amdgpu_set_smartshift_bias(struct device *dev,
>> static int ss_power_attr_update(struct amdgpu_device *adev, struct
>> amdgpu_device_attr *attr,
>> uint32_t mask, enum amdgpu_device_attr_states *states)
>> {
>> - uint32_t ss_power, size;
>> -
>> - if (!amdgpu_acpi_is_power_shift_control_supported())
>> - *states = ATTR_STATE_UNSUPPORTED;
>> - else if ((adev->flags & AMD_IS_PX) &&
>> - !amdgpu_device_supports_smart_shift(adev_to_drm(adev)))
>> - *states = ATTR_STATE_UNSUPPORTED;
>> - else if (amdgpu_dpm_read_sensor(adev,
>> AMDGPU_PP_SENSOR_SS_APU_SHARE,
>> - (void *)&ss_power, &size))
>> - *states = ATTR_STATE_UNSUPPORTED;
>> - else if (amdgpu_dpm_read_sensor(adev,
>> AMDGPU_PP_SENSOR_SS_DGPU_SHARE,
>> - (void *)&ss_power, &size))
>> + if (!amdgpu_device_supports_smart_shift(adev_to_drm(adev)))
>> *states = ATTR_STATE_UNSUPPORTED;
>> return 0;
>>
More information about the amd-gfx
mailing list