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@xxxxxxxxx> > >>>>Signed-off-by: Praveen Paneri <praveen.paneri@xxxxxxxxx> > >>>>Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> > >>> > >>>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@xxxxxxxxxxxxxxxxxx> > >>> > >>>/* 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
