drm/i915/pmu: Report frequency as zero while GPU is sleeping
diff mbox series

Message ID 20191128161051.12753-1-tvrtko.ursulin@linux.intel.com
State New
Headers show
Series
  • drm/i915/pmu: Report frequency as zero while GPU is sleeping
Related show

Commit Message

Tvrtko Ursulin Nov. 28, 2019, 4:10 p.m. UTC
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.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_pmu.c | 44 +++++++++++++++++----------------
 1 file changed, 23 insertions(+), 21 deletions(-)

Comments

Chris Wilson Nov. 28, 2019, 4:19 p.m. UTC | #1
Quoting Tvrtko Ursulin (2019-11-28 16:10:51)
> 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.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Hmm. 0/0 while off, that will be a bit of a shock.

Idea/codewise
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

I'll dogfood it a bit to see how much getting used to it will take :)

> ---
>  drivers/gpu/drm/i915/i915_pmu.c | 44 +++++++++++++++++----------------
>  1 file changed, 23 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> index 95e824a78d4d..b576a2be9f81 100644
> --- a/drivers/gpu/drm/i915/i915_pmu.c
> +++ b/drivers/gpu/drm/i915/i915_pmu.c
> @@ -378,32 +378,32 @@ frequency_sample(struct intel_gt *gt, unsigned int period_ns)
>         struct i915_pmu *pmu = &i915->pmu;
>         struct intel_rps *rps = &gt->rps;
>  
> +       if (!(pmu->enable &
> +             (config_enabled_mask(I915_PMU_ACTUAL_FREQUENCY) |
> +              config_enabled_mask(I915_PMU_REQUESTED_FREQUENCY))) ||
> +           !intel_gt_pm_get_if_awake(gt))
> +               return;

Fwiw, I'd put this as two if()s since the multiline flags makes it
harder to read. Any chance we could do a

if (!frequency_sample_enabled(pmu))
	return;

/* Report 0/0 (actual/requested) frequency while parked */
if (!intel_gt_pm_get_if_awake(gt))
	return;

-Chris
Chris Wilson Nov. 28, 2019, 4:22 p.m. UTC | #2
Quoting Chris Wilson (2019-11-28 16:19:49)
> Quoting Tvrtko Ursulin (2019-11-28 16:10:51)
> > 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.
> > 
> > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Hmm. 0/0 while off, that will be a bit of a shock.

There is a consistency question of whether we should do the same through
the [second class ;] sysfs/ interface.
-Chris
Tvrtko Ursulin Nov. 28, 2019, 5:23 p.m. UTC | #3
On 28/11/2019 16:22, Chris Wilson wrote:
> Quoting Chris Wilson (2019-11-28 16:19:49)
>> Quoting Tvrtko Ursulin (2019-11-28 16:10:51)
>>> 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.
>>>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Hmm. 0/0 while off, that will be a bit of a shock.
> 
> There is a consistency question of whether we should do the same through
> the [second class ;] sysfs/ interface.

I am not sure cur_freq even belongs in sysfs. But IGT/i915_pm_rps seems 
to be depending on the current semantics.

Act_freq on the other hand is always reading the hw register. So should 
probably stay like that. Since it is in sysfs so ABI.

Where this leaves the PMU I don't know.

Regards,

Tvrtko

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index 95e824a78d4d..b576a2be9f81 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -378,32 +378,32 @@  frequency_sample(struct intel_gt *gt, unsigned int period_ns)
 	struct i915_pmu *pmu = &i915->pmu;
 	struct intel_rps *rps = &gt->rps;
 
+	if (!(pmu->enable &
+	      (config_enabled_mask(I915_PMU_ACTUAL_FREQUENCY) |
+	       config_enabled_mask(I915_PMU_REQUESTED_FREQUENCY))) ||
+	    !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 +411,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)