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@xxxxxxxxxxxxxxxxxx>
Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
Cc: <drm-intel-fixes@xxxxxxxxxxxxxxxxxxxxx>
---
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@xxxxxxxxx>
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
