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

[Luben Tuikov <luben.tuikov@amd.com>]

On 2021-10-19 00:38, Lazar, Lijo wrote:
>
> On 10/19/2021 9:45 AM, Luben Tuikov wrote:
>> On 2021-10-18 23:38, Lazar, Lijo wrote:
>>> 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@amd.com>
>>>> Cc: Lijo Lazar<Lijo.Lazar@amd.com>
>>>> Cc: Paul Menzel<pmenzel@molgen.mpg.de>
>>>> Signed-off-by: Luben Tuikov<luben.tuikov@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
>> Lijo just clarified to me that:
>>
>>> well, i had to look up as I haven't seen it before
>> I hope the following should make it clear about its usage:
>>
>> $cd linux/
>> $find . -name "*.[ch]" -exec grep -E "\?:" \{\} \+ | wc -l
>> 1042
>> $_
>>
> Thanks Luben!

You're welcome. I'm glad you're learning new things from my patches. Would've been easier if you'd just said in your email that you've never seen this ternary conditional shortcut before and that you've just learned of it from my patch. (Or not post anything at all in this very case and get in touch with me privately via email or Teams--I would've gladly clarified it there.)

I hope the find+egrep above is also edifying, so you can use it in the future in your learning process.

Regards,
Luben

>
> Thanks,
> Lijo
>
>> Regards,
>> Luben
>>
>>> 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:
>>>>