Re: [PATCH 3/7] drm/amd/pm: drop the redundant 'supported' member of smu_feature structure

["Lazar, Lijo" <lijo.lazar@xxxxxxx>]

On 1/21/2022 1:14 PM, Evan Quan wrote:
> As it has exactly the same value as the 'enabled' member and also do
> the same thing.

I believe the original intention is different. We need to cache the 
features which are really supported by PMFW on that device on init. When 
a request comes through sysfs node for enable/disable of a feature, we 
should check against this and disallow those which are not supported.

Thanks,
Lijo

> Signed-off-by: Evan Quan <evan.quan@xxxxxxx>
> Change-Id: I07c9a5ac5290cd0d88a40ce1768d393156419b5a
> ---
>   drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c     |  1 -
>   drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h |  1 -
>   .../gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c   |  8 ++++----
>   .../amd/pm/swsmu/smu11/sienna_cichlid_ppt.c   | 10 +++++-----
>   .../gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c    | 19 ++++++++-----------
>   .../gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c  |  5 +----
>   .../gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c    |  5 +----
>   .../drm/amd/pm/swsmu/smu13/yellow_carp_ppt.c  |  3 ---
>   drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c        | 17 -----------------
>   drivers/gpu/drm/amd/pm/swsmu/smu_cmn.h        |  3 ---
>   10 files changed, 19 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> index 5ace30434e60..d3237b89f2c5 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> @@ -949,7 +949,6 @@ static int smu_sw_init(void *handle)
>   
>   	smu->pool_size = adev->pm.smu_prv_buffer_size;
>   	smu->smu_feature.feature_num = SMU_FEATURE_MAX;
> -	bitmap_zero(smu->smu_feature.supported, SMU_FEATURE_MAX);
>   	bitmap_zero(smu->smu_feature.enabled, SMU_FEATURE_MAX);
>   	bitmap_zero(smu->smu_feature.allowed, SMU_FEATURE_MAX);
>   
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h b/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
> index 18f24db7d202..3c0360772822 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
> +++ b/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
> @@ -388,7 +388,6 @@ struct smu_power_context {
>   struct smu_feature
>   {
>   	uint32_t feature_num;
> -	DECLARE_BITMAP(supported, SMU_FEATURE_MAX);
>   	DECLARE_BITMAP(allowed, SMU_FEATURE_MAX);
>   	DECLARE_BITMAP(enabled, SMU_FEATURE_MAX);
>   };
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
> index 84834c24a7e9..9fb290f9aaeb 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
> @@ -1625,8 +1625,8 @@ static int navi10_display_config_changed(struct smu_context *smu)
>   	int ret = 0;
>   
>   	if ((smu->watermarks_bitmap & WATERMARKS_EXIST) &&
> -	    smu_cmn_feature_is_supported(smu, SMU_FEATURE_DPM_DCEFCLK_BIT) &&
> -	    smu_cmn_feature_is_supported(smu, SMU_FEATURE_DPM_SOCCLK_BIT)) {
> +	    smu_cmn_feature_is_enabled(smu, SMU_FEATURE_DPM_DCEFCLK_BIT) &&
> +	    smu_cmn_feature_is_enabled(smu, SMU_FEATURE_DPM_SOCCLK_BIT)) {
>   		ret = smu_cmn_send_smc_msg_with_param(smu, SMU_MSG_NumOfDisplays,
>   						  smu->display_config->num_display,
>   						  NULL);
> @@ -1864,13 +1864,13 @@ static int navi10_notify_smc_display_config(struct smu_context *smu)
>   	min_clocks.dcef_clock_in_sr = smu->display_config->min_dcef_deep_sleep_set_clk;
>   	min_clocks.memory_clock = smu->display_config->min_mem_set_clock;
>   
> -	if (smu_cmn_feature_is_supported(smu, SMU_FEATURE_DPM_DCEFCLK_BIT)) {
> +	if (smu_cmn_feature_is_enabled(smu, SMU_FEATURE_DPM_DCEFCLK_BIT)) {
>   		clock_req.clock_type = amd_pp_dcef_clock;
>   		clock_req.clock_freq_in_khz = min_clocks.dcef_clock * 10;
>   
>   		ret = smu_v11_0_display_clock_voltage_request(smu, &clock_req);
>   		if (!ret) {
> -			if (smu_cmn_feature_is_supported(smu, SMU_FEATURE_DS_DCEFCLK_BIT)) {
> +			if (smu_cmn_feature_is_enabled(smu, SMU_FEATURE_DS_DCEFCLK_BIT)) {
>   				ret = smu_cmn_send_smc_msg_with_param(smu,
>   								  SMU_MSG_SetMinDeepSleepDcefclk,
>   								  min_clocks.dcef_clock_in_sr/100,
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c
> index 651fe748e423..d568d6137a00 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c
> @@ -1281,8 +1281,8 @@ static int sienna_cichlid_display_config_changed(struct smu_context *smu)
>   	int ret = 0;
>   
>   	if ((smu->watermarks_bitmap & WATERMARKS_EXIST) &&
> -	    smu_cmn_feature_is_supported(smu, SMU_FEATURE_DPM_DCEFCLK_BIT) &&
> -	    smu_cmn_feature_is_supported(smu, SMU_FEATURE_DPM_SOCCLK_BIT)) {
> +	    smu_cmn_feature_is_enabled(smu, SMU_FEATURE_DPM_DCEFCLK_BIT) &&
> +	    smu_cmn_feature_is_enabled(smu, SMU_FEATURE_DPM_SOCCLK_BIT)) {
>   #if 0
>   		ret = smu_cmn_send_smc_msg_with_param(smu, SMU_MSG_NumOfDisplays,
>   						  smu->display_config->num_display,
> @@ -1521,13 +1521,13 @@ static int sienna_cichlid_notify_smc_display_config(struct smu_context *smu)
>   	min_clocks.dcef_clock_in_sr = smu->display_config->min_dcef_deep_sleep_set_clk;
>   	min_clocks.memory_clock = smu->display_config->min_mem_set_clock;
>   
> -	if (smu_cmn_feature_is_supported(smu, SMU_FEATURE_DPM_DCEFCLK_BIT)) {
> +	if (smu_cmn_feature_is_enabled(smu, SMU_FEATURE_DPM_DCEFCLK_BIT)) {
>   		clock_req.clock_type = amd_pp_dcef_clock;
>   		clock_req.clock_freq_in_khz = min_clocks.dcef_clock * 10;
>   
>   		ret = smu_v11_0_display_clock_voltage_request(smu, &clock_req);
>   		if (!ret) {
> -			if (smu_cmn_feature_is_supported(smu, SMU_FEATURE_DS_DCEFCLK_BIT)) {
> +			if (smu_cmn_feature_is_enabled(smu, SMU_FEATURE_DS_DCEFCLK_BIT)) {
>   				ret = smu_cmn_send_smc_msg_with_param(smu,
>   								  SMU_MSG_SetMinDeepSleepDcefclk,
>   								  min_clocks.dcef_clock_in_sr/100,
> @@ -3752,7 +3752,7 @@ static int sienna_cichlid_gpo_control(struct smu_context *smu,
>   	int ret = 0;
>   
>   
> -	if (smu_cmn_feature_is_supported(smu, SMU_FEATURE_DPM_GFX_GPO_BIT)) {
> +	if (smu_cmn_feature_is_enabled(smu, SMU_FEATURE_DPM_GFX_GPO_BIT)) {
>   		ret = smu_cmn_get_smc_version(smu, NULL, &smu_version);
>   		if (ret)
>   			return ret;
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
> index b58a4c2823c2..b69c2ecc8e25 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
> @@ -808,7 +808,6 @@ int smu_v11_0_system_features_control(struct smu_context *smu,
>   		return ret;
>   
>   	bitmap_zero(feature->enabled, feature->feature_num);
> -	bitmap_zero(feature->supported, feature->feature_num);
>   
>   	if (en) {
>   		ret = smu_cmn_get_enabled_mask(smu, feature_mask, 2);
> @@ -817,8 +816,6 @@ int smu_v11_0_system_features_control(struct smu_context *smu,
>   
>   		bitmap_copy(feature->enabled, (unsigned long *)&feature_mask,
>   			    feature->feature_num);
> -		bitmap_copy(feature->supported, (unsigned long *)&feature_mask,
> -			    feature->feature_num);
>   	}
>   
>   	return ret;
> @@ -1186,7 +1183,7 @@ smu_v11_0_auto_fan_control(struct smu_context *smu, bool auto_fan_control)
>   {
>   	int ret = 0;
>   
> -	if (!smu_cmn_feature_is_supported(smu, SMU_FEATURE_FAN_CONTROL_BIT))
> +	if (!smu_cmn_feature_is_enabled(smu, SMU_FEATURE_FAN_CONTROL_BIT))
>   		return 0;
>   
>   	ret = smu_cmn_feature_set_enabled(smu, SMU_FEATURE_FAN_CONTROL_BIT, auto_fan_control);
> @@ -1607,7 +1604,7 @@ bool smu_v11_0_baco_is_support(struct smu_context *smu)
>   		return false;
>   
>   	/* Arcturus does not support this bit mask */
> -	if (smu_cmn_feature_is_supported(smu, SMU_FEATURE_BACO_BIT) &&
> +	if (smu_cmn_feature_is_enabled(smu, SMU_FEATURE_BACO_BIT) &&
>   	   !smu_cmn_feature_is_enabled(smu, SMU_FEATURE_BACO_BIT))
>   		return false;
>   
> @@ -2150,7 +2147,7 @@ int smu_v11_0_gfx_ulv_control(struct smu_context *smu,
>   {
>   	int ret = 0;
>   
> -	if (smu_cmn_feature_is_supported(smu, SMU_FEATURE_GFX_ULV_BIT))
> +	if (smu_cmn_feature_is_enabled(smu, SMU_FEATURE_GFX_ULV_BIT))
>   		ret = smu_cmn_feature_set_enabled(smu, SMU_FEATURE_GFX_ULV_BIT, enablement);
>   
>   	return ret;
> @@ -2162,7 +2159,7 @@ int smu_v11_0_deep_sleep_control(struct smu_context *smu,
>   	struct amdgpu_device *adev = smu->adev;
>   	int ret = 0;
>   
> -	if (smu_cmn_feature_is_supported(smu, SMU_FEATURE_DS_GFXCLK_BIT)) {
> +	if (smu_cmn_feature_is_enabled(smu, SMU_FEATURE_DS_GFXCLK_BIT)) {
>   		ret = smu_cmn_feature_set_enabled(smu, SMU_FEATURE_DS_GFXCLK_BIT, enablement);
>   		if (ret) {
>   			dev_err(adev->dev, "Failed to %s GFXCLK DS!\n", enablement ? "enable" : "disable");
> @@ -2170,7 +2167,7 @@ int smu_v11_0_deep_sleep_control(struct smu_context *smu,
>   		}
>   	}
>   
> -	if (smu_cmn_feature_is_supported(smu, SMU_FEATURE_DS_UCLK_BIT)) {
> +	if (smu_cmn_feature_is_enabled(smu, SMU_FEATURE_DS_UCLK_BIT)) {
>   		ret = smu_cmn_feature_set_enabled(smu, SMU_FEATURE_DS_UCLK_BIT, enablement);
>   		if (ret) {
>   			dev_err(adev->dev, "Failed to %s UCLK DS!\n", enablement ? "enable" : "disable");
> @@ -2178,7 +2175,7 @@ int smu_v11_0_deep_sleep_control(struct smu_context *smu,
>   		}
>   	}
>   
> -	if (smu_cmn_feature_is_supported(smu, SMU_FEATURE_DS_FCLK_BIT)) {
> +	if (smu_cmn_feature_is_enabled(smu, SMU_FEATURE_DS_FCLK_BIT)) {
>   		ret = smu_cmn_feature_set_enabled(smu, SMU_FEATURE_DS_FCLK_BIT, enablement);
>   		if (ret) {
>   			dev_err(adev->dev, "Failed to %s FCLK DS!\n", enablement ? "enable" : "disable");
> @@ -2186,7 +2183,7 @@ int smu_v11_0_deep_sleep_control(struct smu_context *smu,
>   		}
>   	}
>   
> -	if (smu_cmn_feature_is_supported(smu, SMU_FEATURE_DS_SOCCLK_BIT)) {
> +	if (smu_cmn_feature_is_enabled(smu, SMU_FEATURE_DS_SOCCLK_BIT)) {
>   		ret = smu_cmn_feature_set_enabled(smu, SMU_FEATURE_DS_SOCCLK_BIT, enablement);
>   		if (ret) {
>   			dev_err(adev->dev, "Failed to %s SOCCLK DS!\n", enablement ? "enable" : "disable");
> @@ -2194,7 +2191,7 @@ int smu_v11_0_deep_sleep_control(struct smu_context *smu,
>   		}
>   	}
>   
> -	if (smu_cmn_feature_is_supported(smu, SMU_FEATURE_DS_LCLK_BIT)) {
> +	if (smu_cmn_feature_is_enabled(smu, SMU_FEATURE_DS_LCLK_BIT)) {
>   		ret = smu_cmn_feature_set_enabled(smu, SMU_FEATURE_DS_LCLK_BIT, enablement);
>   		if (ret) {
>   			dev_err(adev->dev, "Failed to %s LCLK DS!\n", enablement ? "enable" : "disable");
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
> index b4a3c9b8b54e..e72831aa4859 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
> @@ -1960,7 +1960,6 @@ static int vangogh_system_features_control(struct smu_context *smu, bool en)
>   						      RLC_STATUS_OFF, NULL);
>   
>   	bitmap_zero(feature->enabled, feature->feature_num);
> -	bitmap_zero(feature->supported, feature->feature_num);
>   
>   	if (!en)
>   		return ret;
> @@ -1971,8 +1970,6 @@ static int vangogh_system_features_control(struct smu_context *smu, bool en)
>   
>   	bitmap_copy(feature->enabled, (unsigned long *)&feature_mask,
>   		    feature->feature_num);
> -	bitmap_copy(feature->supported, (unsigned long *)&feature_mask,
> -		    feature->feature_num);
>   
>   	return 0;
>   }
> @@ -1989,7 +1986,7 @@ static int vangogh_post_smu_init(struct smu_context *smu)
>   		adev->gfx.config.max_sh_per_se * adev->gfx.config.max_shader_engines;
>   
>   	/* allow message will be sent after enable message on Vangogh*/
> -	if (smu_cmn_feature_is_supported(smu, SMU_FEATURE_DPM_GFXCLK_BIT) &&
> +	if (smu_cmn_feature_is_enabled(smu, SMU_FEATURE_DPM_GFXCLK_BIT) &&
>   			(adev->pg_flags & AMD_PG_SUPPORT_GFX_PG)) {
>   		ret = smu_cmn_send_smc_msg(smu, SMU_MSG_EnableGfxOff, NULL);
>   		if (ret) {
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
> index 1754a3720179..c5d354175675 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
> @@ -774,7 +774,6 @@ int smu_v13_0_system_features_control(struct smu_context *smu,
>   		return ret;
>   
>   	bitmap_zero(feature->enabled, feature->feature_num);
> -	bitmap_zero(feature->supported, feature->feature_num);
>   
>   	if (en) {
>   		ret = smu_cmn_get_enabled_mask(smu, feature_mask, 2);
> @@ -783,8 +782,6 @@ int smu_v13_0_system_features_control(struct smu_context *smu,
>   
>   		bitmap_copy(feature->enabled, (unsigned long *)&feature_mask,
>   			    feature->feature_num);
> -		bitmap_copy(feature->supported, (unsigned long *)&feature_mask,
> -			    feature->feature_num);
>   	}
>   
>   	return ret;
> @@ -1071,7 +1068,7 @@ smu_v13_0_auto_fan_control(struct smu_context *smu, bool auto_fan_control)
>   {
>   	int ret = 0;
>   
> -	if (!smu_cmn_feature_is_supported(smu, SMU_FEATURE_FAN_CONTROL_BIT))
> +	if (!smu_cmn_feature_is_enabled(smu, SMU_FEATURE_FAN_CONTROL_BIT))
>   		return 0;
>   
>   	ret = smu_cmn_feature_set_enabled(smu, SMU_FEATURE_FAN_CONTROL_BIT, auto_fan_control);
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/yellow_carp_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu13/yellow_carp_ppt.c
> index f425827e2361..e9172622c0c4 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/yellow_carp_ppt.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/yellow_carp_ppt.c
> @@ -204,7 +204,6 @@ static int yellow_carp_system_features_control(struct smu_context *smu, bool en)
>   		ret = smu_cmn_send_smc_msg(smu, SMU_MSG_PrepareMp1ForUnload, NULL);
>   
>   	bitmap_zero(feature->enabled, feature->feature_num);
> -	bitmap_zero(feature->supported, feature->feature_num);
>   
>   	if (!en)
>   		return ret;
> @@ -215,8 +214,6 @@ static int yellow_carp_system_features_control(struct smu_context *smu, bool en)
>   
>   	bitmap_copy(feature->enabled, (unsigned long *)&feature_mask,
>   		    feature->feature_num);
> -	bitmap_copy(feature->supported, (unsigned long *)&feature_mask,
> -		    feature->feature_num);
>   
>   	return 0;
>   }
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c b/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c
> index 50164ebed1cd..49bcabe9d708 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c
> @@ -476,23 +476,6 @@ int smu_cmn_to_asic_specific_index(struct smu_context *smu,
>   	}
>   }
>   
> -int smu_cmn_feature_is_supported(struct smu_context *smu,
> -				 enum smu_feature_mask mask)
> -{
> -	struct smu_feature *feature = &smu->smu_feature;
> -	int feature_id;
> -
> -	feature_id = smu_cmn_to_asic_specific_index(smu,
> -						    CMN2ASIC_MAPPING_FEATURE,
> -						    mask);
> -	if (feature_id < 0)
> -		return 0;
> -
> -	WARN_ON(feature_id > feature->feature_num);
> -
> -	return test_bit(feature_id, feature->supported);
> -}
> -
>   int smu_cmn_feature_is_enabled(struct smu_context *smu,
>   			       enum smu_feature_mask mask)
>   {
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.h b/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.h
> index 4e34c18c6063..19919182260a 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.h
> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.h
> @@ -48,9 +48,6 @@ int smu_cmn_to_asic_specific_index(struct smu_context *smu,
>   				   enum smu_cmn2asic_mapping_type type,
>   				   uint32_t index);
>   
> -int smu_cmn_feature_is_supported(struct smu_context *smu,
> -				 enum smu_feature_mask mask);
> -
>   int smu_cmn_feature_is_enabled(struct smu_context *smu,
>   			       enum smu_feature_mask mask);
>