diff mbox

drm/i915: Add background commentary to "waitboosting"

Message ID 1449743856-30487-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive)
State New, archived
Headers show

Commit Message

Chris Wilson Dec. 10, 2015, 10:37 a.m. UTC
Describe the intent of boosting the GPU frequency to maximum before
waiting on the GPU.

RPS waitboosting was introduced with

commit b29c19b645287f7062e17d70fa4e9781a01a5d88
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Wed Sep 25 17:34:56 2013 +0100

    drm/i915: Boost RPS frequency for CPU stalls

but lacked a concise comment in the code to explain itself.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Comments

Daniel Vetter Dec. 11, 2015, 6:24 p.m. UTC | #1
On Thu, Dec 10, 2015 at 10:37:36AM +0000, Chris Wilson wrote:
> Describe the intent of boosting the GPU frequency to maximum before
> waiting on the GPU.
> 
> RPS waitboosting was introduced with
> 
> commit b29c19b645287f7062e17d70fa4e9781a01a5d88
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date:   Wed Sep 25 17:34:56 2013 +0100
> 
>     drm/i915: Boost RPS frequency for CPU stalls
> 
> but lacked a concise comment in the code to explain itself.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

We might want to consolidate this with the kerneldoc for gen6_rps_boost,
if that ever happens. But for now this is a good place.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/i915/i915_gem.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index ece7e2908fa3..40a7ade4eafe 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1248,6 +1248,22 @@ int __i915_wait_request(struct drm_i915_gem_request *req,
>  	}
>  
>  	trace_i915_gem_request_wait_begin(req);
> +
> +	/* This client is about to stall waiting for the GPU. In many cases
> +	 * this is undesirable and limits the throughput of the system, as
> +	 * many clients cannot continue processing user input/output whilst
> +	 * asleep. RPS autotuning may take tens of milliseconds to respond
> +	 * to the GPU load and thus incurs additional latency for the client.
> +	 * We can circumvent that promoting the GPU frequency to maximum
> +	 * before we wait. This makes GPU throttle up much more quickly
> +	 * (good for benchmarks), but at a cost of spending more power
> +	 * processing the workload (bad for battery). Not all clients even
> +	 * want their results immediately and for them we should just let
> +	 * the GPU select its own frequency to maximise efficiency.
> +	 * To prevent a single client from forcing the clocks too high for
> +	 * the whole system, we only allow each client to waitboost once
> +	 * in a busy period.
> +	 */
>  	if (INTEL_INFO(req->i915)->gen >= 6)
>  		gen6_rps_boost(req->i915, rps, req->emitted_jiffies);
>  
> -- 
> 2.6.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Chris Wilson Dec. 11, 2015, 9:03 p.m. UTC | #2
On Fri, Dec 11, 2015 at 07:24:29PM +0100, Daniel Vetter wrote:
> On Thu, Dec 10, 2015 at 10:37:36AM +0000, Chris Wilson wrote:
> > Describe the intent of boosting the GPU frequency to maximum before
> > waiting on the GPU.
> > 
> > RPS waitboosting was introduced with
> > 
> > commit b29c19b645287f7062e17d70fa4e9781a01a5d88
> > Author: Chris Wilson <chris@chris-wilson.co.uk>
> > Date:   Wed Sep 25 17:34:56 2013 +0100
> > 
> >     drm/i915: Boost RPS frequency for CPU stalls
> > 
> > but lacked a concise comment in the code to explain itself.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> We might want to consolidate this with the kerneldoc for gen6_rps_boost,
> if that ever happens. But for now this is a good place.

You should end up with both. The function comment will explain what it
does and give reasons why you might want to call it (clients stalls,
detecting missed pageflips, etc), but this code comment should explain
the intent of calling it now. I was tempted to try and put forward some
more objective figures for the benefits of waitboosting, but they grow
stale very quickly so I went with the long winded commentary instead.
-Chris
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index ece7e2908fa3..40a7ade4eafe 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1248,6 +1248,22 @@  int __i915_wait_request(struct drm_i915_gem_request *req,
 	}
 
 	trace_i915_gem_request_wait_begin(req);
+
+	/* This client is about to stall waiting for the GPU. In many cases
+	 * this is undesirable and limits the throughput of the system, as
+	 * many clients cannot continue processing user input/output whilst
+	 * asleep. RPS autotuning may take tens of milliseconds to respond
+	 * to the GPU load and thus incurs additional latency for the client.
+	 * We can circumvent that promoting the GPU frequency to maximum
+	 * before we wait. This makes GPU throttle up much more quickly
+	 * (good for benchmarks), but at a cost of spending more power
+	 * processing the workload (bad for battery). Not all clients even
+	 * want their results immediately and for them we should just let
+	 * the GPU select its own frequency to maximise efficiency.
+	 * To prevent a single client from forcing the clocks too high for
+	 * the whole system, we only allow each client to waitboost once
+	 * in a busy period.
+	 */
 	if (INTEL_INFO(req->i915)->gen >= 6)
 		gen6_rps_boost(req->i915, rps, req->emitted_jiffies);