[AMD Official Use Only - General]
> -----Original Message-----
> From: Lazar, Lijo <Lijo.Lazar@xxxxxxx>
> Sent: Friday, January 6, 2023 6:01 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/6/2023 2:14 PM, Quan, Evan wrote:
> > [AMD Official Use Only - General]
> >
> >
> >
> >> -----Original Message-----
> >> From: Lazar, Lijo <Lijo.Lazar@xxxxxxx>
> >> Sent: Friday, January 6, 2023 11:55 AM
> >> 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/6/2023 7:34 AM, Quan, Evan wrote:
> >>> [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
> >>>>> adev->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.
> > I see. But I would expect the future ASICs take the way used in
> patch3(more straightforward instead of implicit checking for some APIs or
> DPM features).
> > That's also the reason why I do not want to cleanup those old chunks. As I
> do not expect them to serve for future ASICs.
> > Then it's not worth to take the efforts(and risk) to do the cleanup.
> Thoughts?
> >
>
> It's to make sure consistency. I don't think leaving two options to do the
> same thing is a good idea. Otherwise older will continue and cause confusion.
> Changing to newer one for all is the preferred method and handling common
> things in smu_common/powerplay is better rather than having to do
> everything in individual versions.
Those old bunches left in amdgpu_sysfs_if_support_check() can be divided into three types:
1. those which check for the existence for some API(like amdgpu_dpm_is_overdrive_supported/ amdgpu_dpm_get_power_profile_mode). The better choice for them is still left in amdgpu_pm.c I believe.
2. those which check for some common flags(like sriov or apu). The better choice for them is also left in amdgpu_pm.c
3. those which check for gc ip version or smu ip version. A better choice for them might be to move to amdgpu_discovery.c I think. But as mentioned before, I do not think it's worth to take the efforts(and risk) to do so.
So, as you can see, I do not think there is anything we can move to smu_common/powerplay part.
But if you were talking about splitting some changes in patch3 into smu_common/powerplay part, that will be a different story.
BR
Evan
>
> Thanks,
> Lijo
>
> > BR
> > Evan
> >>
> >> 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);
