[PATCH] drm/i915/uc: Include requested frequency in slow firmware load messages

[John Harrison]

On 1/16/2025 03:52, Krzysztof Karas wrote:
> Hi John,
>
>> From: John Harrison <John.C.Harrison at Intel.com>
>>
>> To aid debug of sporadic issues, include the requested frequency in
>> the debug message as well as the actual frequency. That way we know
>> for certain that the clamping is not because the driver forgot to ask.
>>
> ...
>>   	} else if (delta_ms > 200) {
>>   		guc_warn(guc, "excessive init time: %lldms! [status = 0x%08X, count = %d, ret = %d]\n",
>>   			 delta_ms, status, count, ret);
>> -		guc_warn(guc, "excessive init time: [freq = %dMHz, before = %dMHz, perf_limit_reasons = 0x%08X]\n",
>> -			 intel_rps_read_actual_frequency(&gt->rps), before_freq,
>> +		guc_warn(guc, "excessive init time: [freq = %dMHz -> %dMHz vs %dMHz, perf_limit_reasons = 0x%08X]\n",
>> +			 before_freq, intel_rps_read_actual_frequency(&gt->rps),
>> +			 intel_rps_get_requested_frequency(&gt->rps),
> While, <value> -> <value> is clear to me if I were to encounter
> this type of log, I wonder if it would be apparent what "vs"
> means here without having to look into the code. Maybe it would
> be worth to add "vs actual:" or ", actual:" instead?
Are you meaning to swap the actual and requested values around? Changing 
it to "<before> -> <requested>, actual <actual>" would make it more 
confusing, IMHO.

The print is '<before> -> <actual> vs <requested>', i.e. "it started 
<here> and went <there> compared to what we asked for which was <this>". 
That seems like the logical order and description to me. The line is 
already fairly long so the idea was not to make it any longer than 
necessary while still giving all the useful information in a sensible 
manner.

John.

>
> Krzysztof
>
>>   			 intel_uncore_read(uncore, intel_gt_perf_limit_reasons_reg(gt)));
>>   	} else {
>> -		guc_dbg(guc, "init took %lldms, freq = %dMHz, before = %dMHz, status = 0x%08X, count = %d, ret = %d\n",
>> -			delta_ms, intel_rps_read_actual_frequency(&gt->rps),
>> -			before_freq, status, count, ret);
>> +		guc_dbg(guc, "init took %lldms, freq = %dMHz -> %dMHz vs %dMHz, status = 0x%08X, count = %d, ret = %d\n",
>> +			delta_ms, before_freq, intel_rps_read_actual_frequency(&gt->rps),
>> +			intel_rps_get_requested_frequency(&gt->rps), status, count, ret);
> ...