[AMD Official Use Only - General]
> -----Original Message-----
> From: Lazar, Lijo <Lijo.Lazar@xxxxxxx>
> Sent: Thursday, January 5, 2023 9:58 PM
> To: Quan, Evan <Evan.Quan@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: Deucher, Alexander <Alexander.Deucher@xxxxxxx>
> 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@xxxxxxx>
> > 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?
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);
