[AMD Official Use Only - General]
-----Original Message-----
From: Lazar, Lijo <Lijo.Lazar@xxxxxxx>
Sent: Wednesday, March 27, 2024 2:22 PM
To: Wang, Yang(Kevin) <KevinYang.Wang@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
Cc: Zhang, Hawking <Hawking.Zhang@xxxxxxx>; Deucher, Alexander <Alexander.Deucher@xxxxxxx>
Subject: Re: [PATCH] drm/amdgpu: make amdgpu device attr_update() function more efficient
On 3/26/2024 2:32 PM, Yang Wang wrote:
> add a new enumeration type to identify device attribute node, this
> method is relatively more efficient compared with 'strcmp' in
> update_attr() function.
>
> Signed-off-by: Yang Wang <kevinyang.wang@xxxxxxx>
> ---
> drivers/gpu/drm/amd/pm/amdgpu_pm.c | 4 +--
> drivers/gpu/drm/amd/pm/inc/amdgpu_pm.h | 41
> ++++++++++++++++++++++++++
> 2 files changed, 43 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> index 85e935556d7d..04f53f2667fe 100644
> --- a/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> +++ b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> @@ -2226,16 +2226,16 @@ 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;
> + enum amdgpu_device_attr_type type = attr->type;
> uint32_t mp1_ver = amdgpu_ip_version(adev, MP1_HWIP, 0);
> uint32_t gc_ver = amdgpu_ip_version(adev, GC_HWIP, 0);
> - const char *attr_name = dev_attr->attr.name;
>
> if (!(attr->flags & mask)) {
> *states = ATTR_STATE_UNSUPPORTED;
> return 0;
> }
>
> -#define DEVICE_ATTR_IS(_name) (!strcmp(attr_name, #_name))
> +#define DEVICE_ATTR_IS(_name) (type == device_attr_type__##_name)
>
> if (DEVICE_ATTR_IS(pp_dpm_socclk)) {
> if (gc_ver < IP_VERSION(9, 0, 0))
> diff --git a/drivers/gpu/drm/amd/pm/inc/amdgpu_pm.h
> b/drivers/gpu/drm/amd/pm/inc/amdgpu_pm.h
> index eec816f0cbf9..157330c379be 100644
> --- a/drivers/gpu/drm/amd/pm/inc/amdgpu_pm.h
> +++ b/drivers/gpu/drm/amd/pm/inc/amdgpu_pm.h
> @@ -43,8 +43,48 @@ enum amdgpu_device_attr_states {
> ATTR_STATE_SUPPORTED,
> };
>
> +enum amdgpu_device_attr_type {
Prefer id instead of type. That aside,
Reviewed-by: Lijo Lazar <lijo.lazar@xxxxxxx>
Thanks,
Lijo
[kevin]:
Yes, agreed, the name 'attribute id' is better on this case.
Best Regards,
Kevin
> + device_attr_type__unknown = -1,
> + device_attr_type__power_dpm_state = 0,
> + device_attr_type__power_dpm_force_performance_level,
> + device_attr_type__pp_num_states,
> + device_attr_type__pp_cur_state,
> + device_attr_type__pp_force_state,
> + device_attr_type__pp_table,
> + device_attr_type__pp_dpm_sclk,
> + device_attr_type__pp_dpm_mclk,
> + device_attr_type__pp_dpm_socclk,
> + device_attr_type__pp_dpm_fclk,
> + device_attr_type__pp_dpm_vclk,
> + device_attr_type__pp_dpm_vclk1,
> + device_attr_type__pp_dpm_dclk,
> + device_attr_type__pp_dpm_dclk1,
> + device_attr_type__pp_dpm_dcefclk,
> + device_attr_type__pp_dpm_pcie,
> + device_attr_type__pp_sclk_od,
> + device_attr_type__pp_mclk_od,
> + device_attr_type__pp_power_profile_mode,
> + device_attr_type__pp_od_clk_voltage,
> + device_attr_type__gpu_busy_percent,
> + device_attr_type__mem_busy_percent,
> + device_attr_type__vcn_busy_percent,
> + device_attr_type__pcie_bw,
> + device_attr_type__pp_features,
> + device_attr_type__unique_id,
> + device_attr_type__thermal_throttling_logging,
> + device_attr_type__apu_thermal_cap,
> + device_attr_type__gpu_metrics,
> + device_attr_type__smartshift_apu_power,
> + device_attr_type__smartshift_dgpu_power,
> + device_attr_type__smartshift_bias,
> + device_attr_type__xgmi_plpd_policy,
> + device_attr_type__pm_metrics,
> + device_attr_type__count,
> +};
> +
> struct amdgpu_device_attr {
> struct device_attribute dev_attr;
> + enum amdgpu_device_attr_type type;
> enum amdgpu_device_attr_flags flags;
> int (*attr_update)(struct amdgpu_device *adev, struct amdgpu_device_attr *attr,
> uint32_t mask, enum amdgpu_device_attr_states *states); @@
> -61,6 +101,7 @@ struct amdgpu_device_attr_entry {
>
> #define __AMDGPU_DEVICE_ATTR(_name, _mode, _show, _store, _flags, ...) \
> { .dev_attr = __ATTR(_name, _mode, _show, _store), \
> + .type = device_attr_type__##_name, \
> .flags = _flags, \
> ##__VA_ARGS__, }
>
