[PATCH 2/8] drm/amd/pm: refine the checks for sysfs interfaces support
Lazar, Lijo
lijo.lazar at amd.com
Fri Jan 6 03:55:00 UTC 2023
On 1/6/2023 7:34 AM, Quan, Evan wrote:
> [AMD Official Use Only - General]
>
>
>
>> -----Original Message-----
>> From: Lazar, Lijo <Lijo.Lazar at amd.com>
>> Sent: Thursday, January 5, 2023 9:58 PM
>> To: Quan, Evan <Evan.Quan at amd.com>; amd-gfx at lists.freedesktop.org
>> Cc: Deucher, Alexander <Alexander.Deucher at amd.com>
>> Subject: Re: [PATCH 2/8] drm/amd/pm: refine the checks for sysfs interfaces
>> support
>>
>>
>>
>> On 1/5/2023 8:52 AM, Evan Quan wrote:
>>> Make the code more clean and readable with no real logics change.
>>>
>>> Signed-off-by: Evan Quan <evan.quan at amd.com>
>>> Change-Id: I21c879fa9abad9f6da3b5289adf3124950d2f4eb
>>> ---
>>> drivers/gpu/drm/amd/pm/amdgpu_pm.c | 200 ++++++++++++++---------
>> ------
>>> 1 file changed, 98 insertions(+), 102 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/pm/amdgpu_pm.c
>>> b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
>>> index fb6a7d45693a..c69db29eea24 100644
>>> --- a/drivers/gpu/drm/amd/pm/amdgpu_pm.c
>>> +++ b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
>>> @@ -2006,9 +2006,6 @@ static int default_attr_update(struct
>> amdgpu_device *adev, struct amdgpu_device_
>>> uint32_t mask, enum amdgpu_device_attr_states
>> *states)
>>> {
>>> struct device_attribute *dev_attr = &attr->dev_attr;
>>> - uint32_t mp1_ver = adev->ip_versions[MP1_HWIP][0];
>>> - uint32_t gc_ver = adev->ip_versions[GC_HWIP][0];
>>> - const char *attr_name = dev_attr->attr.name;
>>>
>>> if (!(attr->flags & mask) ||
>>> !(AMD_SYSFS_IF_BITMASK(attr->if_bit) &
>>> adev->pm.sysfs_if_supported)) { @@ -2016,112 +2013,14 @@ static int
>> default_attr_update(struct amdgpu_device *adev, struct amdgpu_device_
>>> return 0;
>>> }
>>>
>>> -#define DEVICE_ATTR_IS(_name) (!strcmp(attr_name, #_name))
>>> -
>>> - if (DEVICE_ATTR_IS(pp_dpm_socclk)) {
>>> - if (gc_ver < IP_VERSION(9, 0, 0))
>>> - *states = ATTR_STATE_UNSUPPORTED;
>>> - } else if (DEVICE_ATTR_IS(pp_dpm_dcefclk)) {
>>> - if (gc_ver < IP_VERSION(9, 0, 0) ||
>>> - gc_ver == IP_VERSION(9, 4, 1) ||
>>> - gc_ver == IP_VERSION(9, 4, 2))
>>> - *states = ATTR_STATE_UNSUPPORTED;
>>> - } else if (DEVICE_ATTR_IS(pp_dpm_fclk)) {
>>> - if (mp1_ver < IP_VERSION(10, 0, 0))
>>> - *states = ATTR_STATE_UNSUPPORTED;
>>> - } else if (DEVICE_ATTR_IS(pp_od_clk_voltage)) {
>>> - *states = ATTR_STATE_UNSUPPORTED;
>>> - if (amdgpu_dpm_is_overdrive_supported(adev))
>>> - *states = ATTR_STATE_SUPPORTED;
>>> - } else if (DEVICE_ATTR_IS(mem_busy_percent)) {
>>> - if (adev->flags & AMD_IS_APU || gc_ver == IP_VERSION(9, 0,
>> 1))
>>> - *states = ATTR_STATE_UNSUPPORTED;
>>> - } else if (DEVICE_ATTR_IS(pcie_bw)) {
>>> - /* PCIe Perf counters won't work on APU nodes */
>>> - if (adev->flags & AMD_IS_APU)
>>> - *states = ATTR_STATE_UNSUPPORTED;
>>> - } else if (DEVICE_ATTR_IS(unique_id)) {
>>> - switch (gc_ver) {
>>> - case IP_VERSION(9, 0, 1):
>>> - case IP_VERSION(9, 4, 0):
>>> - case IP_VERSION(9, 4, 1):
>>> - case IP_VERSION(9, 4, 2):
>>> - case IP_VERSION(10, 3, 0):
>>> - case IP_VERSION(11, 0, 0):
>>> - *states = ATTR_STATE_SUPPORTED;
>>> - break;
>>> - default:
>>> - *states = ATTR_STATE_UNSUPPORTED;
>>> - }
>>> - } else if (DEVICE_ATTR_IS(pp_features)) {
>>> - if (adev->flags & AMD_IS_APU || gc_ver < IP_VERSION(9, 0,
>> 0))
>>> - *states = ATTR_STATE_UNSUPPORTED;
>>> - } else if (DEVICE_ATTR_IS(gpu_metrics)) {
>>> - if (gc_ver < IP_VERSION(9, 1, 0))
>>> - *states = ATTR_STATE_UNSUPPORTED;
>>> - } else if (DEVICE_ATTR_IS(pp_dpm_vclk)) {
>>> - if (!(gc_ver == IP_VERSION(10, 3, 1) ||
>>> - gc_ver == IP_VERSION(10, 3, 0) ||
>>> - gc_ver == IP_VERSION(10, 1, 2) ||
>>> - gc_ver == IP_VERSION(11, 0, 0) ||
>>> - gc_ver == IP_VERSION(11, 0, 2)))
>>> - *states = ATTR_STATE_UNSUPPORTED;
>>> - } else if (DEVICE_ATTR_IS(pp_dpm_dclk)) {
>>> - if (!(gc_ver == IP_VERSION(10, 3, 1) ||
>>> - gc_ver == IP_VERSION(10, 3, 0) ||
>>> - gc_ver == IP_VERSION(10, 1, 2) ||
>>> - gc_ver == IP_VERSION(11, 0, 0) ||
>>> - gc_ver == IP_VERSION(11, 0, 2)))
>>> - *states = ATTR_STATE_UNSUPPORTED;
>>> - } else if (DEVICE_ATTR_IS(pp_power_profile_mode)) {
>>> - if (amdgpu_dpm_get_power_profile_mode(adev, NULL) ==
>> -EOPNOTSUPP)
>>> - *states = ATTR_STATE_UNSUPPORTED;
>>> - else if (gc_ver == IP_VERSION(10, 3, 0) &&
>> amdgpu_sriov_vf(adev))
>>> - *states = ATTR_STATE_UNSUPPORTED;
>>> - }
>>> -
>>> - switch (gc_ver) {
>>> - case IP_VERSION(9, 4, 1):
>>> - case IP_VERSION(9, 4, 2):
>>> - /* the Mi series card does not support standalone
>> mclk/socclk/fclk level setting */
>>> - if (DEVICE_ATTR_IS(pp_dpm_mclk) ||
>>> - DEVICE_ATTR_IS(pp_dpm_socclk) ||
>>> - DEVICE_ATTR_IS(pp_dpm_fclk)) {
>>> - dev_attr->attr.mode &= ~S_IWUGO;
>>> - dev_attr->store = NULL;
>>> - }
>>> - break;
>>> - case IP_VERSION(10, 3, 0):
>>> - if (DEVICE_ATTR_IS(power_dpm_force_performance_level)
>> &&
>>> - amdgpu_sriov_vf(adev)) {
>>> - dev_attr->attr.mode &= ~0222;
>>> - dev_attr->store = NULL;
>>> - }
>>> - break;
>>> - default:
>>> - break;
>>> - }
>>> -
>>> - if (DEVICE_ATTR_IS(pp_dpm_dcefclk)) {
>>> - /* SMU MP1 does not support dcefclk level setting */
>>> - if (gc_ver >= IP_VERSION(10, 0, 0)) {
>>> - dev_attr->attr.mode &= ~S_IWUGO;
>>> - dev_attr->store = NULL;
>>> - }
>>> - }
>>> -
>>> - /* setting should not be allowed from VF if not in one VF mode */
>>> - if (amdgpu_sriov_vf(adev) && !amdgpu_sriov_is_pp_one_vf(adev))
>> {
>>> + if (!(adev->pm.sysfs_if_attr_mode[attr->if_bit] & S_IWUGO)) {
>>> dev_attr->attr.mode &= ~S_IWUGO;
>>> dev_attr->store = NULL;
>>> }
>>>
>>> -#undef DEVICE_ATTR_IS
>>> -
>>> return 0;
>>> }
>>>
>>> -
>>> static int amdgpu_device_attr_create(struct amdgpu_device *adev,
>>> struct amdgpu_device_attr *attr,
>>> uint32_t mask, struct list_head *attr_list)
>> @@ -3411,6
>>> +3310,101 @@ static const struct attribute_group *hwmon_groups[] = {
>>> NULL
>>> };
>>>
>>> +static void amdgpu_sysfs_if_support_check(struct amdgpu_device *adev)
>>> +{
>>> + uint64_t *sysfs_if_supported = &adev->pm.sysfs_if_supported;
>>> + umode_t *sysfs_if_attr_mode = adev->pm.sysfs_if_attr_mode;
>>> + uint32_t mp1_ver = adev->ip_versions[MP1_HWIP][0];
>>> + uint32_t gc_ver = adev->ip_versions[GC_HWIP][0];
>>> + int i;
>>> +
>>> + /* All but those specific ASICs support these */
>>> + *sysfs_if_supported &= ~BIT_ULL(AMD_SYSFS_IF_UNIQUE_ID_BIT);
>>> + *sysfs_if_supported &=
>> ~(BIT_ULL(AMD_SYSFS_IF_PP_DPM_VCLK_BIT) |
>>> +
>> BIT_ULL(AMD_SYSFS_IF_PP_DPM_DCLK_BIT));
>>> +
>>> + if (gc_ver < IP_VERSION(9, 1, 0)) {
>>> + *sysfs_if_supported &=
>> ~BIT_ULL(AMD_SYSFS_IF_GPU_METRICS_BIT);
>>> +
>>> + if (gc_ver == IP_VERSION(9, 0, 1)) {
>>> + *sysfs_if_supported &=
>> ~BIT_ULL(AMD_SYSFS_IF_MEM_BUSY_PERCENT_BIT);
>>> + *sysfs_if_supported |=
>> BIT_ULL(AMD_SYSFS_IF_UNIQUE_ID_BIT);
>>> + }
>>> +
>>> + if (gc_ver < IP_VERSION(9, 0, 0))
>>> + *sysfs_if_supported &=
>> ~(BIT_ULL(AMD_SYSFS_IF_PP_DPM_SOCCLK_BIT) |
>>> +
>> BIT_ULL(AMD_SYSFS_IF_PP_DPM_DCEFCLK_BIT) |
>>> +
>> BIT_ULL(AMD_SYSFS_IF_PP_FEATURES_BIT));
>>> + } else {
>>> + switch (gc_ver) {
>>> + case IP_VERSION(9, 4, 0):
>>> + *sysfs_if_supported |=
>> BIT_ULL(AMD_SYSFS_IF_UNIQUE_ID_BIT);
>>> + break;
>>> + case IP_VERSION(9, 4, 1):
>>> + case IP_VERSION(9, 4, 2):
>>> + *sysfs_if_supported &=
>> ~BIT_ULL(AMD_SYSFS_IF_PP_DPM_DCEFCLK_BIT);
>>> + *sysfs_if_supported |=
>> BIT_ULL(AMD_SYSFS_IF_UNIQUE_ID_BIT);
>>> + /* the Mi series card does not support standalone
>> mclk/socclk/fclk level setting */
>>> +
>> sysfs_if_attr_mode[AMD_SYSFS_IF_PP_DPM_MCLK_BIT] &=
>> ~S_IWUGO;
>>> +
>> sysfs_if_attr_mode[AMD_SYSFS_IF_PP_DPM_SOCCLK_BIT] &=
>> ~S_IWUGO;
>>> +
>> sysfs_if_attr_mode[AMD_SYSFS_IF_PP_DPM_FCLK_BIT] &=
>> ~S_IWUGO;
>>> + break;
>>> + case IP_VERSION(10, 1, 2):
>>> + *sysfs_if_supported |=
>> BIT_ULL(AMD_SYSFS_IF_PP_DPM_VCLK_BIT) |
>>> +
>> BIT_ULL(AMD_SYSFS_IF_PP_DPM_DCLK_BIT);
>>> + break;
>>> + case IP_VERSION(10, 3, 0):
>>> + *sysfs_if_supported |=
>> BIT_ULL(AMD_SYSFS_IF_UNIQUE_ID_BIT);
>>> + *sysfs_if_supported |=
>> BIT_ULL(AMD_SYSFS_IF_PP_DPM_VCLK_BIT) |
>>> +
>> BIT_ULL(AMD_SYSFS_IF_PP_DPM_DCLK_BIT);
>>> + if (amdgpu_sriov_vf(adev)) {
>>> + *sysfs_if_supported &=
>> ~BIT_ULL(AMD_SYSFS_IF_PP_POWER_PROFILE_MODE_BIT);
>>> +
>> sysfs_if_attr_mode[AMD_SYSFS_IF_POWER_DPM_FORCE_PERFOR
>> MANCE_LEVEL_BIT] &= ~S_IWUGO;
>>> + }
>>> + break;
>>> + case IP_VERSION(10, 3, 1):
>>> + *sysfs_if_supported |=
>> BIT_ULL(AMD_SYSFS_IF_PP_DPM_VCLK_BIT) |
>>> +
>> BIT_ULL(AMD_SYSFS_IF_PP_DPM_DCLK_BIT);
>>> + break;
>>> + case IP_VERSION(11, 0, 0):
>>> + *sysfs_if_supported |=
>> BIT_ULL(AMD_SYSFS_IF_UNIQUE_ID_BIT);
>>> + *sysfs_if_supported |=
>> BIT_ULL(AMD_SYSFS_IF_PP_DPM_VCLK_BIT) |
>>> +
>> BIT_ULL(AMD_SYSFS_IF_PP_DPM_DCLK_BIT);
>>> + break;
>>> + case IP_VERSION(11, 0, 2):
>>> + *sysfs_if_supported |=
>> BIT_ULL(AMD_SYSFS_IF_PP_DPM_VCLK_BIT) |
>>> +
>> BIT_ULL(AMD_SYSFS_IF_PP_DPM_DCLK_BIT);
>>> + break;
>>> + default:
>>> + break;
>>> + }
>>> + }
>>> +
>>> + if (mp1_ver < IP_VERSION(10, 0, 0))
>>> + *sysfs_if_supported &=
>> ~BIT_ULL(AMD_SYSFS_IF_PP_DPM_FCLK_BIT);
>>> +
>>
>> With this change, the IP version based checks need to be moved to
>> respective smu_v* checks so that each IP version decides what is supported
>> at which level (R/W) rather than consolidating it here. Only generic checks
>> like amdgpu_sriov_is_pp_one_vf may be maintained here.
>> That way it really helps.
> [Quan, Evan] For some of them, they could be moved to respective smu_v* or gfx_v* checks.
> But for some of them, it will be difficult. For example, for "mp1_ver < IP_VERSION(10, 0, 0)" or " gc_ver >= IP_VERSION(10, 0, 0)", you need to figure out which asics it refers to first and then apply the same change to every of them. That seems more error prone.
> So, my thought is just left these old chunks as they were. And we just need to take care of the future/new asics. How do you think?
>
My preference is to clean up this as much as possible. Also, you may be
able to set some of them generically based on FEAT_DPM bits in
swsmu/powerplay.
Thanks,
Lijo
> Evan
>>
>> Thanks,
>> Lijo
>>
>>> + if (adev->flags & AMD_IS_APU)
>>> + *sysfs_if_supported &=
>> ~(BIT_ULL(AMD_SYSFS_IF_MEM_BUSY_PERCENT_BIT) |
>>> +
>> BIT_ULL(AMD_SYSFS_IF_PCIE_BW_BIT) |
>>> +
>> BIT_ULL(AMD_SYSFS_IF_PP_FEATURES_BIT));
>>> +
>>> + if (!amdgpu_dpm_is_overdrive_supported(adev))
>>> + *sysfs_if_supported &=
>>> +~BIT_ULL(AMD_SYSFS_IF_PP_OD_CLK_VOLTAGE_BIT);
>>> +
>>> + if (amdgpu_dpm_get_power_profile_mode(adev, NULL) == -
>> EOPNOTSUPP)
>>> + *sysfs_if_supported &=
>>> +~BIT_ULL(AMD_SYSFS_IF_PP_POWER_PROFILE_MODE_BIT);
>>> +
>>> + if (gc_ver >= IP_VERSION(10, 0, 0))
>>> + sysfs_if_attr_mode[AMD_SYSFS_IF_PP_DPM_DCEFCLK_BIT]
>> &= ~S_IWUGO;
>>> +
>>> + /* setting should not be allowed from VF if not in one VF mode */
>>> + if (amdgpu_sriov_vf(adev) &&
>>> + !amdgpu_sriov_is_pp_one_vf(adev)) {
>>> + for (i = 0; i <
>> AMD_MAX_NUMBER_OF_SYSFS_IF_SUPPORTED; i++)
>>> + sysfs_if_attr_mode[i] &= ~S_IWUGO;
>>> + }
>>> +}
>>> +
>>> int amdgpu_pm_sysfs_init(struct amdgpu_device *adev)
>>> {
>>> int ret;
>>> @@ -3424,6 +3418,8 @@ int amdgpu_pm_sysfs_init(struct amdgpu_device
>> *adev)
>>> if (adev->pm.dpm_enabled == 0)
>>> return 0;
>>>
>>> + amdgpu_sysfs_if_support_check(adev);
>>> +
>>> adev->pm.int_hwmon_dev =
>> hwmon_device_register_with_groups(adev->dev,
>>>
>> DRIVER_NAME, adev,
>>>
>> hwmon_groups);
More information about the amd-gfx
mailing list