Re: [PATCH 10/11] drm/i915: Mark the context and address space as closed

Chris Wilson
2015-12-17T14:26:50Z

On Thu, Dec 17, 2015 at 02:15:52PM +0000, Tvrtko Ursulin wrote:
> >+static void i915_ppgtt_close(struct i915_address_space *vm)
> >+{
> >+ struct list_head *phases[] = {
> >+ &vm->active_list,
> >+ &vm->inactive_list,
> >+ &vm->unbound_list,
> >+ NULL,
> >+ }, **phase;
> >+
> >+ RQ_BUG_ON(vm->is_ggtt);
> >+ RQ_BUG_ON(vm->closed);
> >+ vm->closed = true;
> >+
> >+ for (phase = phases; *phase; phase++) {
> >+ struct i915_vma *vma, *vn;
> >+
> >+ list_for_each_entry_safe(vma, vn, *phase, vm_link)
> >+ i915_vma_close(vma);
> >+ }
> >+}
>
> Hm so VMAs get unlinked from everywhere, but then on retire goes
> back to inactive. Is it not a bit weird?

Very weird. In the end, I have to stop unlinking in i915_vma_close()
from the vm lists.

> Why it is needed to unlink VMAs from everywhere when marking them as closed?

Indeed, it was just to try and keep this walk short. But I realised that
this would actually also foul up the evict/shrinker (by hiding objects
from them that should be thrown away).

> And actually on retire objects are ahead of VMAs in the
> req->active_list so the last object unreference happens before the
> last vma is retired, which is even weirder.
>
> Am I missing something?

That shouldn't happen. The i915_gem_object_retire_read is run after the
i915_vma_retire.

I had added some commentary to i915_vma_move_to_active() that hopefully
explains the interdependences between retire callbacks (mostly to try
and prevent breakage later).

@@ -1075,7 +1075,13 @@ void i915_vma_move_to_active(struct i915_vma *vma,

obj->dirty = 1; /* be paranoid */

- /* Add a reference if we're newly entering the active list. */
+ /* Add a reference if we're newly entering the active list.
+ * The order in which we add operations to the retirement queue is
+ * vital here: mark_active adds to the start of the callback list,
+ * such that subsequent callbacks are called first. Therefore we
+ * add the active reference first and queue for it to be dropped
+ * *last*.
+ */
-Chris

--
Chris Wilson, Intel Open Source Technology Centre