| Message ID | 20250109093010.3879245-1-sk.anirban@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] drm/i915/selftests: Correct frequency handling in RPS power measurement | expand |
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. 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; > } >
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 > 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; >> } >>
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); 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; }