[Intel-gfx] [PATCH] drm/i915: Unbind objects in shrinker only if device is runtime active

[Chris Wilson]

On Thu, Dec 24, 2015 at 08:12:46PM +0530, Goel, Akash wrote:
> 
> 
> On 12/24/2015 8:02 PM, Chris Wilson wrote:
> >On Thu, Dec 24, 2015 at 07:54:09PM +0530, Goel, Akash wrote:
> >>
> >>
> >>On 12/24/2015 5:52 PM, Chris Wilson wrote:
> >>>On Thu, Dec 24, 2015 at 04:16:08PM +0530, Praveen Paneri wrote:
> >>>>When the system is running low on memory, gem shrinker is invoked.
> >>>>In this process objects will be unbounded from GTT and unbinding process
> >>>>will require access to GTT(GTTADR) and also to fence register potentially.
> >>>>That requires a resume of gfx device, if suspended, in the shrinker path.
> >>>>Considering the power leakage due to intermediate resume, perform unbinding
> >>>>operation only if device is already runtime active.
> >>>>
> >>>>Signed-off-by: Akash Goel <akash.goel at intel.com>
> >>>>Signed-off-by: Praveen Paneri <praveen.paneri at intel.com>
> >>>>Cc: Chris Wilson <chris at chris-wilson.co.uk>
> >>>
> >>>Lgtm, the only complication is that we over report the number of
> >>>shrinkable objects. But that isn't such a big issue with the current
> >>>incarnation of the shrinker.
> >>>
> >>>>---
> >>>>  drivers/gpu/drm/i915/i915_gem_shrinker.c | 11 +++++++++++
> >>>>  1 file changed, 11 insertions(+)
> >>>>
> >>>>diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> >>>>index f7df54a..89350f4 100644
> >>>>--- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
> >>>>+++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> >>>>@@ -89,6 +89,15 @@ i915_gem_shrink(struct drm_i915_private *dev_priv,
> >>>>  	i915_gem_retire_requests(dev_priv->dev);
> >>>>
> >>>>  	/*
> >>>>+	 * Unbinding of objects will require HW access. Lets not wake
> >>>>+	 * up gfx device just for this. Do the unbinding only if gfx
> >>>>+	 * device is already active.
> >>>>+	 */
> >>>>+	if ((flags & I915_SHRINK_BOUND) &&
> >>>>+			!intel_runtime_pm_get_noidle(dev_priv))
> >>>
> >>>Please line up contnuation lines with the opening bracking, hint cino=:0,(0 for vim.
> >>>
> >>>With the whitespace fixed,
> >>>Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
> >>>
> >>>/* Unbinding of objects will require HW access; let us not wake up
> >>>  * the device just to recover a little memory. If absolutely necessary,
> >>>  * we will force the wake during oom-notifier.
> >>>  */
> >>
> >>Sorry not fully sure but do we need to cover
> >>i915_gem_retire_requests() also ?
> >
> >No. That is covered by the dev_priv->mm.busy wakeref.
> >
> >>Actually retire_requests could also lead to a potential unbinding,
> >>if the last reference of a context goes away in that.
> >
> >Indeed, also last object unreference could trigger an unbinding, and
> >even last vma use. All covered by the dev_priv->mm.busy wakeref held
> >whilst there are any requests in flight.
> >
> Thank you so much for the clarification.
> So if the device is in a runtime suspended state, the call to
> i915_gem_retire_requests() should almost be a NOOP.

Yes. The list should be empty (and even execlists!). I should sprinkle
around a few assert_rpm_wakelock_held() around GEM to better indicate the
extents of that wakeref we take when submitting requests.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre