[PATCH] drm/i915: Skip shrinking of fenced objects if Gfx is suspended

[PATCH] drm/i915: Skip shrinking of fenced objects if Gfx is suspended

[Praveen Paneri]

When the system is running low on memory, gem shrinker is invoked.
In this process objects will be unbinded from GTT.
For tiled objects, access to fence registers could be required while
unbinding them. That requires a resume of gfx device, if suspended,
in the shrinker path. This intermediate resume could cause power
leakage.
To avoid this intermediate resume of gfx device, don't consider
tiled(fenced) objects for purge in the shrinker path.

Signed-off-by: Akash Goel <akash.goel@intel.com>
Signed-off-by: Praveen Paneri <praveen.paneri@intel.com>
---
 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..443432a 100644
--- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
+++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
@@ -129,6 +129,17 @@ i915_gem_shrink(struct drm_i915_private *dev_priv,
 			if ((flags & I915_SHRINK_ACTIVE) == 0 && obj->active)
 				continue;
 
+			/*
+			 * Skip the unbinding of objects, possessing a fence
+			 * register, if the device in the suspended state.
+			 * Otherwise device has to be resumed before an access
+			 * is made to the fence register on unbinding.
+			 */
+			if (HAS_RUNTIME_PM(dev_priv->dev) &&
+				dev_priv->pm.suspended &&
+				(obj->fence_reg != I915_FENCE_REG_NONE))
+				continue;
+
 			drm_gem_object_reference(&obj->base);
 
 			/* For the unbound phase, this should be a no-op! */
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Skip shrinking of fenced objects if Gfx is suspended

[Chris Wilson]

On Thu, Dec 17, 2015 at 10:59:13AM +0530, Praveen Paneri wrote:
> When the system is running low on memory, gem shrinker is invoked.
> In this process objects will be unbinded from GTT.
> For tiled objects, access to fence registers could be required while
> unbinding them. That requires a resume of gfx device, if suspended,
> in the shrinker path. This intermediate resume could cause power
> leakage.
> To avoid this intermediate resume of gfx device, don't consider
> tiled(fenced) objects for purge in the shrinker path.

Well, you can't unbind anything whilst the device is suspended (GSM is
also hardware access).

Please see the recent intel_runtime_pm_tryget() discussion.
-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

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

[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.

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>
---
 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))
+		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);
 
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 5233 bytes --]

[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

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 28518 bytes --]

[-- Attachment #3: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

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

-- 
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: [PATCH] drm/i915: Unbind objects in shrinker only if device is runtime active

[Goel, Akash]

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
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

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

-- 
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: [PATCH] drm/i915: Unbind objects in shrinker only if device is runtime active

[Goel, Akash]

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
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/2] drm/i915: Unbind objects in shrinker only if device is runtime active

[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.

Signed-off-by: Akash Goel <akash.goel@intel.com>
Signed-off-by: Praveen Paneri <praveen.paneri@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 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 f7df54a..6f91070 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; Let us not wake the
+	 * device just to recover a little memory. If absolutely necessary,
+	 * we will force the wake during oom-notifier.
+	 */
+	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
@@ -145,6 +154,9 @@ 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);
 
 	return count;
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/2] drm/i915: Add rpm get/put in i915_shrinker_oom

[Praveen Paneri]

i915_gem_shrink_all() will scan the bound list only if device is not
suspended but in OOM scenarios it becomes absolutely necessary to
release as much memory as possible. So, adding rpm get/put in
i915_shrinker_oom() to ensure shrinking of bound objects in OOM
scenario.

Signed-off-by: Praveen Paneri <praveen.paneri@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_shrinker.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
index 6f91070..267aa10 100644
--- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
+++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
@@ -297,7 +297,9 @@ i915_gem_shrinker_oom(struct notifier_block *nb, unsigned long event, void *ptr)
 	was_interruptible = dev_priv->mm.interruptible;
 	dev_priv->mm.interruptible = false;
 
+	intel_runtime_pm_get(dev_priv);
 	freed_pages = i915_gem_shrink_all(dev_priv);
+	intel_runtime_pm_put(dev_priv);
 
 	dev_priv->mm.interruptible = was_interruptible;
 
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Add rpm get/put in i915_shrinker_oom

[Chris Wilson]

On Tue, Dec 29, 2015 at 12:35:39PM +0530, Praveen Paneri wrote:
> i915_gem_shrink_all() will scan the bound list only if device is not
> suspended but in OOM scenarios it becomes absolutely necessary to
> release as much memory as possible. So, adding rpm get/put in
> i915_shrinker_oom() to ensure shrinking of bound objects in OOM
> scenario.
> 
> Signed-off-by: Praveen Paneri <praveen.paneri@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-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: [PATCH 2/2] drm/i915: Add rpm get/put in i915_shrinker_oom

[Daniel Vetter]

On Tue, Dec 29, 2015 at 11:58:26AM +0000, Chris Wilson wrote:
> On Tue, Dec 29, 2015 at 12:35:39PM +0530, Praveen Paneri wrote:
> > i915_gem_shrink_all() will scan the bound list only if device is not
> > suspended but in OOM scenarios it becomes absolutely necessary to
> > release as much memory as possible. So, adding rpm get/put in
> > i915_shrinker_oom() to ensure shrinking of bound objects in OOM
> > scenario.
> > 
> > Signed-off-by: Praveen Paneri <praveen.paneri@intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Both applied to dinq, thanks.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Add rpm get/put in i915_shrinker_oom

[Daniel Vetter]

On Tue, Jan 05, 2016 at 11:16:52AM +0100, Daniel Vetter wrote:
> On Tue, Dec 29, 2015 at 11:58:26AM +0000, Chris Wilson wrote:
> > On Tue, Dec 29, 2015 at 12:35:39PM +0530, Praveen Paneri wrote:
> > > i915_gem_shrink_all() will scan the bound list only if device is not
> > > suspended but in OOM scenarios it becomes absolutely necessary to
> > > release as much memory as possible. So, adding rpm get/put in
> > > i915_shrinker_oom() to ensure shrinking of bound objects in OOM
> > > scenario.
> > > 
> > > Signed-off-by: Praveen Paneri <praveen.paneri@intel.com>
> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> Both applied to dinq, thanks.

Well that didn't work since it doesn't compile. My tree lacks
intel_runtime_pm_get_noidle. Please resend with prerequisite patches
included in the patch series.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Unbind objects in shrinker only if device is runtime active

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1570 bytes --]

Hi Praveen,

[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on v4.4-rc7 next-20151223]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Praveen-Paneri/drm-i915-Unbind-objects-in-shrinker-only-if-device-is-runtime-active/20151229-145756
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: x86_64-rhel (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/gpu/drm/i915/i915_gem_shrinker.c: In function 'i915_gem_shrink':
>> drivers/gpu/drm/i915/i915_gem_shrinker.c:97:6: error: implicit declaration of function 'intel_runtime_pm_get_noidle' [-Werror=implicit-function-declaration]
         !intel_runtime_pm_get_noidle(dev_priv))
         ^
   cc1: some warnings being treated as errors

vim +/intel_runtime_pm_get_noidle +97 drivers/gpu/drm/i915/i915_gem_shrinker.c

    91		/*
    92		 * Unbinding of objects will require HW access; Let us not wake the
    93		 * device just to recover a little memory. If absolutely necessary,
    94		 * we will force the wake during oom-notifier.
    95		 */
    96		if ((flags & I915_SHRINK_BOUND) &&
  > 97		    !intel_runtime_pm_get_noidle(dev_priv))
    98			flags &= ~I915_SHRINK_BOUND;
    99	
   100		/*

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 35623 bytes --]

[-- Attachment #3: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 1/2] drm/i915: Unbind objects in shrinker only if device is runtime active

[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 <akash.goel@intel.com>
Signed-off-by: Praveen Paneri <praveen.paneri@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 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 d3c473f..3bc292d 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.
+	 */
+	if ((flags & I915_SHRINK_BOUND) &&
+	    !intel_runtime_pm_get_if_in_use(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
@@ -188,6 +197,9 @@ 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);
 
 	return count;
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[CI-RUN PATCH 0/2] ci-run of shrinker rpm fixes

[Mika Kuoppala]

Praveen Paneri (2):
  drm/i915: Unbind objects in shrinker only if device is runtime active
  drm/i915: Add rpm get/put in i915_shrinker_oom

 drivers/gpu/drm/i915/i915_gem_shrinker.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/2] drm/i915: Add rpm get/put in i915_shrinker_oom

[Mika Kuoppala]

From: Praveen Paneri <praveen.paneri@intel.com>

i915_gem_shrink_all() will scan the bound list only if device is not
suspended but in OOM scenarios it becomes absolutely necessary to
release as much memory as possible. So, adding rpm get/put in
i915_shrinker_oom() to ensure shrinking of bound objects in OOM
scenario.

Signed-off-by: Praveen Paneri <praveen.paneri@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem_shrinker.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
index dff6596a680f..206736673a52 100644
--- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
+++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
@@ -353,7 +353,9 @@ i915_gem_shrinker_oom(struct notifier_block *nb, unsigned long event, void *ptr)
 	if (!i915_gem_shrinker_lock_uninterruptible(dev_priv, &slu, 5000))
 		return NOTIFY_DONE;
 
+	intel_runtime_pm_get(dev_priv);
 	freed_pages = i915_gem_shrink_all(dev_priv);
+	intel_runtime_pm_put(dev_priv);
 
 	/* Because we may be allocating inside our own driver, we cannot
 	 * assert that there are no objects with pinned pages that are not
-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Add rpm get/put in i915_shrinker_oom

[Chris Wilson]

On Wed, Apr 13, 2016 at 04:54:07PM +0300, Mika Kuoppala wrote:
> From: Praveen Paneri <praveen.paneri@intel.com>
> 
> i915_gem_shrink_all() will scan the bound list only if device is not
> suspended but in OOM scenarios it becomes absolutely necessary to
> release as much memory as possible. So, adding rpm get/put in
> i915_shrinker_oom() to ensure shrinking of bound objects in OOM
> scenario.
> 
> Signed-off-by: Praveen Paneri <praveen.paneri@intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_gem_shrinker.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> index dff6596a680f..206736673a52 100644
> --- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
> +++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> @@ -353,7 +353,9 @@ i915_gem_shrinker_oom(struct notifier_block *nb, unsigned long event, void *ptr)
>  	if (!i915_gem_shrinker_lock_uninterruptible(dev_priv, &slu, 5000))
>  		return NOTIFY_DONE;
>  
> +	intel_runtime_pm_get(dev_priv);
>  	freed_pages = i915_gem_shrink_all(dev_priv);
> +	intel_runtime_pm_put(dev_priv);

Out of date, see also shrinker_vmap.
-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