[AMD Official Use Only - General]
> -----Original Message-----
> From: Lazar, Lijo <Lijo.Lazar@xxxxxxx>
> Sent: Monday, January 9, 2023 6:52 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/9/2023 2:27 PM, Quan, Evan wrote:
> > [AMD Official Use Only - General]
> >
> >
> >
> >> -----Original Message-----
> >> From: Lazar, Lijo <Lijo.Lazar@xxxxxxx>
> >> Sent: Monday, January 9, 2023 11:28 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/9/2023 7:42 AM, Quan, Evan wrote:
> >>> [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.
> >>
> >> If you are moving to a different method, think in terms of the new
> >> method and not the legacy way. Otherwise, we can continue the legacy
> >> method as described above. Keeping two options open is not a choice.
> > [Quan, Evan] Sorry, I'm still not persuaded. It's not about legacy way.
>
> What is the reason for anyone not to maintain it in the legacy way for new
> ASICs also. This patch is at best half cooked. Either cook it fully or throw it out.
I think we spent too much time on those discussions about the proper designs(about where(which api/file) should some piece of code be placed).
If you do not see any issue that affects these patch function correctly, can we just land them first?
Please let us do the thing right first.
And i'm open with(and welcome) any further update to the logics introduced in those patches if you do see a better design.
BR
Evan
>
> Thanks,
> Lijo
>
> > In fact, all the code has been transformed into bitmask related.
> > I think your suggestions are about finding a new place for the code above.
> > I'm not convinced as i believe they served for legacy asics only and they
> cannot benefit for future asics.
> > So, it's not worth to take efforts to move them to new places and take the
> risks.
> >
> > BR
> > Evan
> >>
> >> Thanks,
> >> Lijo
> >>
> >>> 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);
