[PATCH v2] drm/amd/pm: consistent approach for smartshift
Lazar, Lijo
lijo.lazar at amd.com
Thu May 12 12:16:01 UTC 2022
On 5/12/2022 5:42 PM, Sundararaju, Sathishkumar wrote:
>
> 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().
No, I didn't mean to combine all of them. Only the above logic (fetching
from dGPU, if not fetch from APU) may be kept in a single function. It's
the same for any device regardless of dGPU or APU.
Thanks,
Lijo
>
>>
>> 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