Re: [PATCH v2] drm/i915/selftests: Correct frequency handling in RPS power measurement

[Riana Tauro <riana.tauro@intel.com>]

Hi Anirban

On 1/10/2025 8:42 PM, Anirban, Sk wrote:
> 
> 
> 
> On 09-01-2025 16:45, Nilawar, Badal wrote:
>>
>>
>> On 09-01-2025 15:50, Nilawar, Badal wrote:
>>>
>>>
>>> On 09-01-2025 15:00, sk.anirban@intel.com wrote:
>>>> From: Sk Anirban<sk.anirban@intel.com>
>>>>
>>>> Fix the frequency calculation by ensuring it is adjusted
>>>> only once during power measurement. Update live_rps_power test
>>>> to use the correct frequency values for logging and comparison.
>>>>
>>>> v2:
>>>>    - Improved frequency logging (Riana)
>>>>
>>>> Signed-off-by: Sk Anirban<sk.anirban@intel.com>
>>>> Reviewed-by: Riana Tauro<riana.tauro@intel.com>
>>>> ---
>>>>   drivers/gpu/drm/i915/gt/selftest_rps.c | 11 ++++++-----
>>>>   1 file changed, 6 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/gt/selftest_rps.c b/drivers/gpu/ 
>>>> drm/i915/gt/selftest_rps.c
>>>> index c207a4fb03bf..e515d7eb628a 100644
>>>> --- a/drivers/gpu/drm/i915/gt/selftest_rps.c
>>>> +++ b/drivers/gpu/drm/i915/gt/selftest_rps.c
>>>> @@ -1126,6 +1126,7 @@ static u64 measure_power_at(struct intel_rps 
>>>> *rps, int *freq)
>>>>   {
>>>>       *freq = rps_set_check(rps, *freq);
>>>>       msleep(100);
>>>> +    *freq = intel_gpu_freq(rps, *freq);
>>> I am seeingrps_set_check will wait till act freq become desired freq, 
>>> in case of timeout act freq could be different. I think it would be 
>>> good to check freq returned by rps_set_check is expected freq if not 
>>> then read freq again after msleep.
>>
>> Please ignore above comments, I got your code. You are applying freq 
>> multiplier before passing to measure_power. While this approach works 
>> fine, I recommend fixing measure_power() by using read_cagf() instead 
>> of intel_rps_read_actual_frequency().
>> Add Fixes: ac4e8560248f ("drm/i915/selftests: Add helper function 
>> measure_power") in commit message.
>>
>> Regards,
>> Badal
>>
> The measure_power() function is being used by slpc also, as slpc is not 
> passing the raw frequency it may cause issue. So the plan is to create 
> independent function to measure power for slpc, and for rps I will be 
> using read_cagf() to calculate the avg.

Are you planning to re-work this patch? In that case use read_cagf for 
SLPC as well, wouldn't that work?
> 
> Regards,
> Anirban
>>
>>> Regards, Badal
>>>
>>>>       return measure_power(rps, freq);
>>>>   }
>>>> @@ -1202,13 +1203,13 @@ int live_rps_power(void *arg)
>>>>           pr_info("%s: min:%llumW @ %uMHz, max:%llumW @ %uMHz\n",
>>>>               engine->name,
>>>> -            min.power, intel_gpu_freq(rps, min.freq),
>>>> -            max.power, intel_gpu_freq(rps, max.freq));
>>>> +            min.power, min.freq,
>>>> +            max.power, max.freq);
>>>>           if (10 * min.freq >= 9 * max.freq) {
>>>> -            pr_notice("Could not control frequency, ran at [%d: 
>>>> %uMHz, %d:%uMhz]\n",
>>>> -                  min.freq, intel_gpu_freq(rps, min.freq),
>>>> -                  max.freq, intel_gpu_freq(rps, max.freq));
>>>> +            pr_notice("Could not control frequency, ran at [%uMHz, 
>>>> %uMhz]\n",
>>>> +                  min.freq,
>>>> +                  max.freq);
>>>>               continue;
>>>>           }
>