[Intel-gfx] [PATCH 3/3] drm/i915: Drain freed objects for mmap space exhaustion

[Tvrtko Ursulin]

On 06/01/2017 15:44, Chris Wilson wrote:
> On Fri, Jan 06, 2017 at 03:33:10PM +0000, Tvrtko Ursulin wrote:
>>
>> On 06/01/2017 15:22, Chris Wilson wrote:
>>> As we now use a deferred free queue for objects, simply retiring the
>>> active objects is not enough to immediately free them and recover their
>>> mmap space - we must now also drain the freed object list.
>>>
>>> Fixes: fbbd37b36fa5 ("drm/i915: Move object release to a freelist + worker"
>>> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
>>> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
>>> Cc: <drm-intel-fixes at lists.freedesktop.org>
>>> ---
>>> drivers/gpu/drm/i915/i915_gem.c | 24 +++++++++++-------------
>>> 1 file changed, 11 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>>> index 94958437252a..f6f4ec894a7f 100644
>>> --- a/drivers/gpu/drm/i915/i915_gem.c
>>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>>> @@ -2090,23 +2090,21 @@ static int i915_gem_object_create_mmap_offset(struct drm_i915_gem_object *obj)
>>> 	int err;
>>>
>>> 	err = drm_gem_create_mmap_offset(&obj->base);
>>> -	if (!err)
>>> +	if (likely(!err))
>>> 		return 0;
>>>
>>> -	/* We can idle the GPU locklessly to flush stale objects, but in order
>>> -	 * to claim that space for ourselves, we need to take the big
>>> -	 * struct_mutex to free the requests+objects and allocate our slot.
>>> -	 */
>>> -	err = i915_gem_wait_for_idle(dev_priv, I915_WAIT_INTERRUPTIBLE);
>>> -	if (err)
>>> -		return err;
>>> +	/* Attempt to reap some mmap space from dead objects */
>>> +	do {
>>> +		err = i915_gem_wait_for_idle(dev_priv, I915_WAIT_INTERRUPTIBLE);
>>> +		if (err)
>>> +			break;
>>>
>>> -	err = i915_mutex_lock_interruptible(&dev_priv->drm);
>>> -	if (!err) {
>>> -		i915_gem_retire_requests(dev_priv);
>>> +		i915_gem_drain_freed_objects(dev_priv);
>>> 		err = drm_gem_create_mmap_offset(&obj->base);
>>> -		mutex_unlock(&dev_priv->drm.struct_mutex);
>>> -	}
>>> +		if (!err)
>>> +			break;
>>> +
>>> +	} while (flush_delayed_work(&dev_priv->gt.retire_work));
>>
>> Would it be better to keep a direct retire in here, since while it
>> waits for the retire worker to run, someone might grab the mutex and
>> then the retire worker won't do anything? Unless flush_delayed_work
>> would not return in case work re-queues itself?
>
> It's a wait either way. I favoured avoiding the struct_mutex here (one
> less to worry about) and waiting on active requests as that's secondary
> to the freed work which is how we expect to make forward progress. The
> loop on retire_work makes sure that any active dead objects will be reaped
> as soon as possible, and we keep on checking until there are no more dead
> objects available atm.

Ok I think I understand it now. I missed that i915_gem_wait_for_idle is 
getting repeat in every iteration so it could indeed wait for the retire 
worker multiple times.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>

Regards,

Tvrtko