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

["Goel, Akash" <akash.goel@intel.com>]

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