[Intel-gfx] [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 at intel.com>
>>>> Signed-off-by: Praveen Paneri <praveen.paneri at intel.com>
>>>> Cc: Chris Wilson <chris at 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 at 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
>