[PATCH 2/8] drm/amd/pm: refine the checks for sysfs interfaces support
Lazar, Lijo
lijo.lazar at amd.com
Tue Jan 10 04:46:31 UTC 2023
On 1/10/2023 8:07 AM, Quan, Evan wrote:
> [AMD Official Use Only - General]
>
>
>
>> -----Original Message-----
>> From: Lazar, Lijo <Lijo.Lazar at amd.com>
>> Sent: Monday, January 9, 2023 6:52 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/9/2023 2:27 PM, Quan, Evan wrote:
>>> [AMD Official Use Only - General]
>>>
>>>
>>>
>>>> -----Original Message-----
>>>> From: Lazar, Lijo <Lijo.Lazar at amd.com>
>>>> Sent: Monday, January 9, 2023 11:28 AM
>>>> 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/9/2023 7:42 AM, Quan, Evan wrote:
>>>>> [AMD Official Use Only - General]
>>>>>
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Lazar, Lijo <Lijo.Lazar at amd.com>
>>>>>> Sent: Friday, January 6, 2023 6:01 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/6/2023 2:14 PM, Quan, Evan wrote:
>>>>>>> [AMD Official Use Only - General]
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: Lazar, Lijo <Lijo.Lazar at amd.com>
>>>>>>>> Sent: Friday, January 6, 2023 11:55 AM
>>>>>>>> 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/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
>>>>>>>>>>> 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).
Yes, the time is spent as the patch doesn't look fine.
> 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.
Sorry, I don't think this is the right approach. I suggest to drop this
for now and do it in the right way later rather than using fix-it-later
approach.
Thanks,
Lijo
> 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);
More information about the amd-gfx
mailing list