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

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre