drm/i915/gt: Treat idling as a RPS downclock event
diff mbox series

Message ID 20200322154018.15951-1-chris@chris-wilson.co.uk
State New
Headers show
Series
  • drm/i915/gt: Treat idling as a RPS downclock event
Related show

Commit Message

Chris Wilson March 22, 2020, 3:40 p.m. UTC
If we park/unpark faster than we can respond to RPS events, we never
will process a downlock event after expiring a waitboost, and thus we
will forever restart the GPU at max clocks even if the workload switches
and doesn't justify full power.

Closes: https://gitlab.freedesktop.org/drm/intel/issues/1500
Fixes: 3e7abf814193 ("drm/i915: Extract GT render power state management")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Andi Shyti <andi.shyti@intel.com>
Cc: Lyude Paul <lyude@redhat.com>
---
 drivers/gpu/drm/i915/gt/intel_rps.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

Comments

Lyude Paul April 3, 2020, 9:25 p.m. UTC | #1
Hey - almost forgot to reply to this, I actually probably won't be able to
test this properly for a while since my razer laptop is actually stuck in the
office I work at, and I legally can't get to it with COVID-19 shelter-in-
place. Sorry about that!

On Sun, 2020-03-22 at 15:40 +0000, Chris Wilson wrote:
> If we park/unpark faster than we can respond to RPS events, we never
> will process a downlock event after expiring a waitboost, and thus we
> will forever restart the GPU at max clocks even if the workload switches
> and doesn't justify full power.
> 
> Closes: https://gitlab.freedesktop.org/drm/intel/issues/1500
> Fixes: 3e7abf814193 ("drm/i915: Extract GT render power state management")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Andi Shyti <andi.shyti@intel.com>
> Cc: Lyude Paul <lyude@redhat.com>
> ---
>  drivers/gpu/drm/i915/gt/intel_rps.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c
> b/drivers/gpu/drm/i915/gt/intel_rps.c
> index 7bf631ca560b..50884aeac49c 100644
> --- a/drivers/gpu/drm/i915/gt/intel_rps.c
> +++ b/drivers/gpu/drm/i915/gt/intel_rps.c
> @@ -771,6 +771,19 @@ void intel_rps_park(struct intel_rps *rps)
>  	intel_uncore_forcewake_get(rps_to_uncore(rps), FORCEWAKE_MEDIA);
>  	rps_set(rps, rps->idle_freq, false);
>  	intel_uncore_forcewake_put(rps_to_uncore(rps), FORCEWAKE_MEDIA);
> +
> +	/*
> +	 * Since we will try and restart from the previously requested
> +	 * frequency on unparking, treat this idle point as a downlock
> +	 * interrupt and reduce the frequency for resume. If we park/unpark
> +	 * more frequently than the rps worker can run, we will not respond
> +	 * to any EI and never see a change in frequency.
> +	 *
> +	 * (Note the we accommodate Cherryview's limitation of only using
> +	 * an even bin by applying it to all.)
> +	 */
> +	rps->cur_freq =
> +		max_t(u8, round_down(rps->cur_freq - 1, 2), rps->min_freq);
>  }
>  
>  void intel_rps_boost(struct i915_request *rq)
Chris Wilson April 3, 2020, 9:29 p.m. UTC | #2
Quoting Lyude Paul (2020-04-03 22:25:43)
> Hey - almost forgot to reply to this, I actually probably won't be able to
> test this properly for a while since my razer laptop is actually stuck in the
> office I work at, and I legally can't get to it with COVID-19 shelter-in-
> place. Sorry about that!

No worries, but I'll probably forget to ping you when the world settles
down again. It's just something for you to keep in mind in case we get
some strange bug reports in 6 months time.
-Chris

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c b/drivers/gpu/drm/i915/gt/intel_rps.c
index 7bf631ca560b..50884aeac49c 100644
--- a/drivers/gpu/drm/i915/gt/intel_rps.c
+++ b/drivers/gpu/drm/i915/gt/intel_rps.c
@@ -771,6 +771,19 @@  void intel_rps_park(struct intel_rps *rps)
 	intel_uncore_forcewake_get(rps_to_uncore(rps), FORCEWAKE_MEDIA);
 	rps_set(rps, rps->idle_freq, false);
 	intel_uncore_forcewake_put(rps_to_uncore(rps), FORCEWAKE_MEDIA);
+
+	/*
+	 * Since we will try and restart from the previously requested
+	 * frequency on unparking, treat this idle point as a downlock
+	 * interrupt and reduce the frequency for resume. If we park/unpark
+	 * more frequently than the rps worker can run, we will not respond
+	 * to any EI and never see a change in frequency.
+	 *
+	 * (Note the we accommodate Cherryview's limitation of only using
+	 * an even bin by applying it to all.)
+	 */
+	rps->cur_freq =
+		max_t(u8, round_down(rps->cur_freq - 1, 2), rps->min_freq);
 }
 
 void intel_rps_boost(struct i915_request *rq)