| Message ID | 1450953968-27805-1-git-send-email-praveen.paneri@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
[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 <linux/oom.h>
26 #include <linux/shmem_fs.h>
27 #include <linux/slab.h>
28 #include <linux/swap.h>
29 #include <linux/pci.h>
30 #include <linux/dma-buf.h>
31 #include <drm/drmP.h>
32 #include <drm/i915_drm.h>
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 { &dev_priv->mm.unbound_list, I915_SHRINK_UNBOUND },
83 { &dev_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 device just for this. Do the unbinding only if gfx
94 * device is already active.
95 */
> 96 if ((flags & I915_SHRINK_BOUND) &&
97 !intel_runtime_pm_get_noidle(dev_priv))
98 flags &= ~I915_SHRINK_BOUND;
99
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
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@intel.com> > Signed-off-by: Praveen Paneri <praveen.paneri@intel.com> > Cc: Chris Wilson <chris@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@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. */ 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
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@intel.com> >> Signed-off-by: Praveen Paneri <praveen.paneri@intel.com> >> Cc: Chris Wilson <chris@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@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 ? 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 >
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@intel.com> > >>Signed-off-by: Praveen Paneri <praveen.paneri@intel.com> > >>Cc: Chris Wilson <chris@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@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. > 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
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@intel.com> >>>> Signed-off-by: Praveen Paneri <praveen.paneri@intel.com> >>>> Cc: Chris Wilson <chris@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@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. 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 >
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@intel.com> > >>>>Signed-off-by: Praveen Paneri <praveen.paneri@intel.com> > >>>>Cc: Chris Wilson <chris@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@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
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)) + flags &= ~I915_SHRINK_BOUND; + + /* * As we may completely rewrite the (un)bound list whilst unbinding * (due to retiring requests) we have to strictly process only * one element of the list at the time, and recheck the list @@ -144,6 +153,8 @@ i915_gem_shrink(struct drm_i915_private *dev_priv, } list_splice(&still_in_list, phase->list); } + if (flags & I915_SHRINK_BOUND) + intel_runtime_pm_put(dev_priv); i915_gem_retire_requests(dev_priv->dev);