| Message ID | 20191129105436.20100-1-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/i915/pmu: Report frequency as zero while GPU is sleeping | expand |
Quoting Tvrtko Ursulin (2019-11-29 10:54:36) > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > We used to report the minimum possible frequency as both requested and > active while GPU was in sleep state. This was a consequence of sampling > the value from the "current frequency" field in our software tracking. > > This was strictly speaking wrong, but given that until recently the > current frequency in sleeping state used to be equal to minimum, it did > not stand out sufficiently to be noticed as such. > > After some recent changes have made the current frequency be reported > as last active before GPU went to sleep, meaning both requested and active > frequencies could end up being reported at their maximum values for the > duration of the GPU idle state, it became much more obvious that this does > not make sense. > > To fix this we will now sample the frequency counters only when the GPU is > awake. As a consequence reported frequencies could be reported as below > the GPU reported minimum but that should be much less confusing that the > current situation. > > v2: > * Split out early exit conditions for readability. (Chris) > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> I'm happy if you are happy... -Chris
Quoting Tvrtko Ursulin (2019-11-29 10:54:36) > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > We used to report the minimum possible frequency as both requested and > active while GPU was in sleep state. This was a consequence of sampling > the value from the "current frequency" field in our software tracking. > > This was strictly speaking wrong, but given that until recently the > current frequency in sleeping state used to be equal to minimum, it did > not stand out sufficiently to be noticed as such. > > After some recent changes have made the current frequency be reported > as last active before GPU went to sleep, meaning both requested and active > frequencies could end up being reported at their maximum values for the > duration of the GPU idle state, it became much more obvious that this does > not make sense. > > To fix this we will now sample the frequency counters only when the GPU is > awake. As a consequence reported frequencies could be reported as below > the GPU reported minimum but that should be much less confusing that the > current situation. > > v2: > * Split out early exit conditions for readability. (Chris) > Closes: https://gitlab.freedesktop.org/drm/intel/issues/675 > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
On 06/12/2019 12:32, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2019-11-29 10:54:36) >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> We used to report the minimum possible frequency as both requested and >> active while GPU was in sleep state. This was a consequence of sampling >> the value from the "current frequency" field in our software tracking. >> >> This was strictly speaking wrong, but given that until recently the >> current frequency in sleeping state used to be equal to minimum, it did >> not stand out sufficiently to be noticed as such. >> >> After some recent changes have made the current frequency be reported >> as last active before GPU went to sleep, meaning both requested and active >> frequencies could end up being reported at their maximum values for the >> duration of the GPU idle state, it became much more obvious that this does >> not make sense. >> >> To fix this we will now sample the frequency counters only when the GPU is >> awake. As a consequence reported frequencies could be reported as below >> the GPU reported minimum but that should be much less confusing that the >> current situation. >> >> v2: >> * Split out early exit conditions for readability. (Chris) >> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > > I'm happy if you are happy... Okay pushed, thanks! Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c index 95e824a78d4d..eaad9c97d031 100644 --- a/drivers/gpu/drm/i915/i915_pmu.c +++ b/drivers/gpu/drm/i915/i915_pmu.c @@ -370,6 +370,13 @@ add_sample_mult(struct i915_pmu_sample *sample, u32 val, u32 mul) sample->cur += mul_u32_u32(val, mul); } +static bool frequency_sampling_enabled(struct i915_pmu *pmu) +{ + return pmu->enable & + (config_enabled_mask(I915_PMU_ACTUAL_FREQUENCY) | + config_enabled_mask(I915_PMU_REQUESTED_FREQUENCY)); +} + static void frequency_sample(struct intel_gt *gt, unsigned int period_ns) { @@ -378,32 +385,33 @@ frequency_sample(struct intel_gt *gt, unsigned int period_ns) struct i915_pmu *pmu = &i915->pmu; struct intel_rps *rps = >->rps; + if (!frequency_sampling_enabled(pmu)) + return; + + /* Report 0/0 (actual/requested) frequency while parked. */ + if (!intel_gt_pm_get_if_awake(gt)) + return; + if (pmu->enable & config_enabled_mask(I915_PMU_ACTUAL_FREQUENCY)) { u32 val; - val = rps->cur_freq; - if (intel_gt_pm_get_if_awake(gt)) { - u32 stat; - - /* - * We take a quick peek here without using forcewake - * so that we don't perturb the system under observation - * (forcewake => !rc6 => increased power use). We expect - * that if the read fails because it is outside of the - * mmio power well, then it will return 0 -- in which - * case we assume the system is running at the intended - * frequency. Fortunately, the read should rarely fail! - */ - stat = intel_uncore_read_fw(uncore, GEN6_RPSTAT1); - if (stat) - val = intel_get_cagf(rps, stat); - - intel_gt_pm_put_async(gt); - } + /* + * We take a quick peek here without using forcewake + * so that we don't perturb the system under observation + * (forcewake => !rc6 => increased power use). We expect + * that if the read fails because it is outside of the + * mmio power well, then it will return 0 -- in which + * case we assume the system is running at the intended + * frequency. Fortunately, the read should rarely fail! + */ + val = intel_uncore_read_fw(uncore, GEN6_RPSTAT1); + if (val) + val = intel_get_cagf(rps, val); + else + val = rps->cur_freq; add_sample_mult(&pmu->sample[__I915_SAMPLE_FREQ_ACT], - intel_gpu_freq(rps, val), - period_ns / 1000); + intel_gpu_freq(rps, val), period_ns / 1000); } if (pmu->enable & config_enabled_mask(I915_PMU_REQUESTED_FREQUENCY)) { @@ -411,6 +419,8 @@ frequency_sample(struct intel_gt *gt, unsigned int period_ns) intel_gpu_freq(rps, rps->cur_freq), period_ns / 1000); } + + intel_gt_pm_put_async(gt); } static enum hrtimer_restart i915_sample(struct hrtimer *hrtimer)