[PATCH 4/5] dpm/amd/pm: Sienna: Remove 0 MHz as a current clock frequency (v3)

[Lazar, Lijo]

On 10/19/2021 5:19 AM, Luben Tuikov wrote:
> A current value of a clock frequency of 0, means
> that the IP block is in some kind of low power
> state. Ignore it and don't report it here. Here we
> only report the possible operating (non-zero)
> frequencies of the block requested. So, if the
> current clock value is 0, then print the DPM
> frequencies, but don't report a current value.
> 
> v2: Don't report the minimum one as the current
> one when reported one is 0, i.e. don't add an
> asterisk (Lijo). LT: It is conceivable that this
> may confuse user-mode tools if they scan and look
> for a current one, i.e. look for an asterisk, but
> they'll have to adapt and use other methods for
> finding power states of the chip--we can't report
> 0 as current.
> v3: Start the subject title with a verb. (PaulM)
> 
> Cc: Alex Deucher <Alexander.Deucher at amd.com>
> Cc: Lijo Lazar <Lijo.Lazar at amd.com>
> Cc: Paul Menzel <pmenzel at molgen.mpg.de>
> Signed-off-by: Luben Tuikov <luben.tuikov at amd.com>
> ---
>   .../amd/pm/swsmu/smu11/sienna_cichlid_ppt.c   | 60 ++++++++++++-------
>   1 file changed, 40 insertions(+), 20 deletions(-)
> 
> 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 f630d5e928ccfe..6fe792be77dbbb 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
> @@ -1040,7 +1040,8 @@ static void sienna_cichlid_get_od_setting_range(struct smu_11_0_7_overdrive_tabl
>   }
>   
>   static int sienna_cichlid_print_clk_levels(struct smu_context *smu,
> -			enum smu_clk_type clk_type, char *buf)
> +					   enum smu_clk_type clk_type,
> +					   char *buf)
>   {
>   	struct amdgpu_device *adev = smu->adev;
>   	struct smu_table_context *table_context = &smu->smu_table;
> @@ -1052,12 +1053,12 @@ static int sienna_cichlid_print_clk_levels(struct smu_context *smu,
>   	OverDriveTable_t *od_table =
>   		(OverDriveTable_t *)table_context->overdrive_table;
>   	int i, size = 0, ret = 0;
> -	uint32_t curr_value = 0, value = 0, count = 0;
> +	uint32_t curr_value, value, count;
>   	uint32_t freq_value[3] = {0, 0, 0};
> -	uint32_t mark_index = 0;
>   	uint32_t gen_speed, lane_width;
>   	uint32_t min_value, max_value;
>   	uint32_t smu_version;
> +	bool     fine_grained;
>   
>   	smu_cmn_get_sysfs_buf(&buf, &size);
>   
> @@ -1077,6 +1078,20 @@ static int sienna_cichlid_print_clk_levels(struct smu_context *smu,
>   		if (ret)
>   			goto print_clk_out;
>   
> +		ret = smu_v11_0_get_dpm_freq_by_index(smu, clk_type, 0,
> +						      &freq_value[0]);
> +		if (ret)
> +			goto print_clk_out;
> +
> +		/* A current value of a clock frequency of 0, means
> +		 * that the IP block is in some kind of low power
> +		 * state. Ignore it and don't report it here. Here we
> +		 * only report the possible operating (non-zero)
> +		 * frequencies of the block requested. So, if the
> +		 * current clock value is 0, then we don't report a
> +		 * "current" value from the DPM states, i.e. we don't
> +		 * add an asterisk.
> +		 */
>   
>   		/* no need to disable gfxoff when retrieving the current gfxclk */
>   		if ((clk_type == SMU_GFXCLK) || (clk_type == SMU_SCLK))
> @@ -1086,38 +1101,43 @@ static int sienna_cichlid_print_clk_levels(struct smu_context *smu,
>   		if (ret)
>   			goto print_clk_out;
>   
> -		if (!sienna_cichlid_supports_fine_grained_dpm(smu, clk_type)) {
> -			for (i = 0; i < count; i++) {
> +		fine_grained = sienna_cichlid_supports_fine_grained_dpm(smu, clk_type);
> +		if (!fine_grained) {
> +			/* We already got the 0-th index--print it
> +			 * here and continue thereafter.
> +			 */
> +			size += sysfs_emit_at(buf, size, "%d: %uMhz %s\n", 0, freq_value[0],
> +					      curr_value == freq_value[0] ? "*" : "");
> +			for (i = 1; i < count; i++) {
>   				ret = smu_v11_0_get_dpm_freq_by_index(smu, clk_type, i, &value);
>   				if (ret)
>   					goto print_clk_out;
> -
>   				size += sysfs_emit_at(buf, size, "%d: %uMhz %s\n", i, value,
>   						curr_value == value ? "*" : "");
>   			}
>   		} else {
> -			ret = smu_v11_0_get_dpm_freq_by_index(smu, clk_type, 0, &freq_value[0]);
> -			if (ret)
> -				goto print_clk_out;
> +			freq_value[1] = curr_value ?: freq_value[0];

Omitting second expression is not standard C - 
https://gcc.gnu.org/onlinedocs/gcc/Conditionals.html

Thanks,
Lijo
>   			ret = smu_v11_0_get_dpm_freq_by_index(smu, clk_type, count - 1, &freq_value[2]);
>   			if (ret)
>   				goto print_clk_out;
>   
> -			freq_value[1] = curr_value;
> -			mark_index = curr_value == freq_value[0] ? 0 :
> -				     curr_value == freq_value[2] ? 2 : 1;
> -
> -			count = 3;
> -			if (mark_index != 1) {
> +			if (freq_value[1] == freq_value[0]) {
> +				i = 1;
> +				count = 3;
> +			} else if (freq_value[1] == freq_value[2]) {
> +				i = 0;
>   				count = 2;
> -				freq_value[1] = freq_value[2];
> +			} else {
> +				i = 0;
> +				count = 3;
>   			}
>   
> -			for (i = 0; i < count; i++) {
> -				size += sysfs_emit_at(buf, size, "%d: %uMhz %s\n", i, freq_value[i],
> -						curr_value  == freq_value[i] ? "*" : "");
> +			for ( ; i < count; i++) {
> +				size += sysfs_emit_at(buf, size,
> +						      "%d: %uMhz %s\n",
> +						      i, freq_value[i],
> +						      curr_value == freq_value[i] ? "*" : "");
>   			}
> -
>   		}
>   		break;
>   	case SMU_PCIE:
>