Re: [Intel-gfx] [PATCH] drm/i915: Unbind objects in shrinker only if device is runtime active
Chris Wilsonwrites: > [ text/plain ] > On Wed, Mar 30, 2016 at 01:11:53PM +0300, Mika Kuoppala wrote: >> From: Praveen Paneri >> >> 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. >> >> v2: Using newly implemented intel_runtime_pm_get_if_in_use() >> >> Signed-off-by: Akash Goel >> Signed-off-by: Praveen Paneri >> Reviewed-by: Chris Wilson >> --- >> drivers/gpu/drm/i915/i915_gem_shrinker.c | 12 >> 1 file changed, 12 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c >> b/drivers/gpu/drm/i915/i915_gem_shrinker.c >> index d3c473ffb90a..3bc292d626ff 100644 >> --- a/drivers/gpu/drm/i915/i915_gem_shrinker.c >> +++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c >> @@ -129,6 +129,15 @@ i915_gem_shrink(struct drm_i915_private *dev_priv, >> i915_gem_retire_requests(dev_priv->dev); >> >> /* >> + * Unbinding of objects will require HW access; Let us not wake the >> + * device just to recover a little memory. If absolutely necessary, >> + * we will force the wake during oom-notifier. >> + */ > > The implication was that we would send the companion patch as well! Missed it, thanks for pointing out. Will send it as a separate one and combine when merging. Thanks, -Mika > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Unbind objects in shrinker only if device is runtime active
On Wed, Mar 30, 2016 at 01:11:53PM +0300, Mika Kuoppala wrote: > From: Praveen Paneri> > 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. > > v2: Using newly implemented intel_runtime_pm_get_if_in_use() > > Signed-off-by: Akash Goel > Signed-off-by: Praveen Paneri > Reviewed-by: Chris Wilson > --- > drivers/gpu/drm/i915/i915_gem_shrinker.c | 12 > 1 file changed, 12 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c > b/drivers/gpu/drm/i915/i915_gem_shrinker.c > index d3c473ffb90a..3bc292d626ff 100644 > --- a/drivers/gpu/drm/i915/i915_gem_shrinker.c > +++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c > @@ -129,6 +129,15 @@ i915_gem_shrink(struct drm_i915_private *dev_priv, > i915_gem_retire_requests(dev_priv->dev); > > /* > + * Unbinding of objects will require HW access; Let us not wake the > + * device just to recover a little memory. If absolutely necessary, > + * we will force the wake during oom-notifier. > + */ The implication was that we would send the companion patch as well! -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Unbind objects in shrinker only if device is runtime active
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system] Hi Praveen, [auto build test WARNING on drm-intel/for-linux-next] [also build test WARNING on v4.4-rc6 next-20151223] url: https://github.com/0day-ci/linux/commits/Praveen-Paneri/drm-i915-Unbind-objects-in-shrinker-only-if-device-is-runtime-active/20151224-183944 base: git://anongit.freedesktop.org/drm-intel for-linux-next config: x86_64-randconfig-x001-201551 (attached as .config) reproduce: # save the attached .config to linux build tree make ARCH=x86_64 All warnings (new ones prefixed by >>): In file included from include/uapi/linux/stddef.h:1:0, from include/linux/stddef.h:4, from include/uapi/linux/posix_types.h:4, from include/uapi/linux/types.h:13, from include/linux/types.h:5, from include/uapi/linux/capability.h:16, from include/linux/capability.h:15, from include/linux/sched.h:15, from include/linux/oom.h:5, from drivers/gpu/drm/i915/i915_gem_shrinker.c:25: drivers/gpu/drm/i915/i915_gem_shrinker.c: In function 'i915_gem_shrink': drivers/gpu/drm/i915/i915_gem_shrinker.c:97:5: error: implicit declaration of function 'intel_runtime_pm_get_noidle' [-Werror=implicit-function-declaration] !intel_runtime_pm_get_noidle(dev_priv)) ^ include/linux/compiler.h:147:28: note: in definition of macro '__trace_if' if (__builtin_constant_p((cond)) ? !!(cond) : \ ^ >> drivers/gpu/drm/i915/i915_gem_shrinker.c:96:2: note: in expansion of macro >> 'if' if ((flags & I915_SHRINK_BOUND) && ^ cc1: some warnings being treated as errors vim +/if +96 drivers/gpu/drm/i915/i915_gem_shrinker.c 19 * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 20 * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS 21 * IN THE SOFTWARE. 22 * 23 */ 24 > 25 #include 26 #include 27 #include 28 #include 29 #include 30 #include 31 #include 32 #include 33 34 #include "i915_drv.h" 35 #include "i915_trace.h" 36 37 static bool mutex_is_locked_by(struct mutex *mutex, struct task_struct *task) 38 { 39 if (!mutex_is_locked(mutex)) 40 return false; 41 42 #if defined(CONFIG_SMP) || defined(CONFIG_DEBUG_MUTEXES) 43 return mutex->owner == task; 44 #else 45 /* Since UP may be pre-empted, we cannot assume that we own the lock */ 46 return false; 47 #endif 48 } 49 50 /** 51 * i915_gem_shrink - Shrink buffer object caches 52 * @dev_priv: i915 device 53 * @target: amount of memory to make available, in pages 54 * @flags: control flags for selecting cache types 55 * 56 * This function is the main interface to the shrinker. It will try to release 57 * up to @target pages of main memory backing storage from buffer objects. 58 * Selection of the specific caches can be done with @flags. This is e.g. useful 59 * when purgeable objects should be removed from caches preferentially. 60 * 61 * Note that it's not guaranteed that released amount is actually available as 62 * free system memory - the pages might still be in-used to due to other reasons 63 * (like cpu mmaps) or the mm core has reused them before we could grab them. 64 * Therefore code that needs to explicitly shrink buffer objects caches (e.g. to 65 * avoid deadlocks in memory reclaim) must fall back to i915_gem_shrink_all(). 66 * 67 * Also note that any kind of pinning (both per-vma address space pins and 68 * backing storage pins at the buffer object level) result in the shrinker code 69 * having to skip the object. 70 * 71 * Returns: 72 * The number of pages of backing storage actually released. 73 */ 74 unsigned long 75 i915_gem_shrink(struct drm_i915_private *dev_priv, 76 unsigned long target, unsigned flags) 77 { 78 const struct { 79 struct list_head *list; 80 unsigned int bit; 81 } phases[] = { 82 { _priv->mm.unbound_list, I915_SHRINK_UNBOUND }, 83 { _priv->mm.bound_list, I915_SHRINK_BOUND }, 84 { NULL, 0 }, 85 }, *phase; 86 unsigned long count = 0; 87 88 trace_i915_gem_shrink(dev_priv, target, flags); 89 i915_gem_retire_requests(dev_priv->dev); 90 91 /* 92 * Unbinding of objects will require HW access. Lets not wake 93 * up gfx
Re: [Intel-gfx] [PATCH] drm/i915: Unbind objects in shrinker only if device is runtime active
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> Signed-off-by: Praveen Paneri > Cc: Chris Wilson 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 /* 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. */ Gives a better rationale, I think. And can you, whilst you are here, please put the intel_runtime_pm_get() into i915_gem_shrinker_oom() -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Unbind objects in shrinker only if device is runtime active
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> >>Signed-off-by: Praveen Paneri > >>Cc: Chris Wilson > > > >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 > > > >/* 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. > There is a runtime_pm_get protection in i915_gem_free_object, so > should not be a problem for ringbuffer & context image objects and > most probably the i915_gem_context_clean would get completed before > the device again goes into runtime suspend state. No the one in i915_gem_free_object is actually wrong (granularity), and hopefully will be fixed in the near future. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Unbind objects in shrinker only if device is runtime active
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 GoelSigned-off-by: Praveen Paneri Cc: Chris Wilson 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 /* 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 ? Actually retire_requests could also lead to a potential unbinding, if the last reference of a context goes away in that. There is a runtime_pm_get protection in i915_gem_free_object, so should not be a problem for ringbuffer & context image objects and most probably the i915_gem_context_clean would get completed before the device again goes into runtime suspend state. Best regards Akash Gives a better rationale, I think. And can you, whilst you are here, please put the intel_runtime_pm_get() into i915_gem_shrinker_oom() -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Unbind objects in shrinker only if device is runtime active
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 GoelSigned-off-by: Praveen Paneri Cc: Chris Wilson 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 /* 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. Best regards Akash There is a runtime_pm_get protection in i915_gem_free_object, so should not be a problem for ringbuffer & context image objects and most probably the i915_gem_context_clean would get completed before the device again goes into runtime suspend state. No the one in i915_gem_free_object is actually wrong (granularity), and hopefully will be fixed in the near future. -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Unbind objects in shrinker only if device is runtime active
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> Signed-off-by: Praveen Paneri > Cc: Chris Wilson > >>> > >>>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 > >>> > >>>/* 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@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx