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

Tvrtko Ursulin
2015-12-17T14:35:04Z

On 17/12/15 14:26, Chris Wilson wrote:
> 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*.
> + */

I don't know how I concluded active VMA is after the active object, and
I specifically saw the order and list_move.

Again, very good to document that, so something good at least came out
of it. :)

Regards,

Tvrtko