[Intel-gfx] [PATCH 32/46] drm/i915: Pull VM lists under the VM mutex.

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-01-17 16:23:48)
> 
> On 16/01/2019 17:01, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2019-01-16 16:47:43)
> >>
> >> On 07/01/2019 11:54, Chris Wilson wrote:
> >>> @@ -1530,20 +1531,21 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
> >>>    
> >>>    static void i915_gem_object_bump_inactive_ggtt(struct drm_i915_gem_object *obj)
> >>>    {
> >>> -     struct drm_i915_private *i915;
> >>> +     struct drm_i915_private *i915 = to_i915(obj->base.dev);
> >>>        struct list_head *list;
> >>>        struct i915_vma *vma;
> >>>    
> >>>        GEM_BUG_ON(!i915_gem_object_has_pinned_pages(obj));
> >>>    
> >>> +     mutex_lock(&i915->ggtt.vm.mutex);
> >>>        for_each_ggtt_vma(vma, obj) {
> >>>                if (!drm_mm_node_allocated(&vma->node))
> >>>                        continue;
> >>>    
> >>>                list_move_tail(&vma->vm_link, &vma->vm->bound_list);
> >>>        }
> >>> +     mutex_unlock(&i915->ggtt.vm.mutex);
> >>
> >> This is now struct_mutex -> vm->mutex nesting, which we would preferably
> >> want to avoid? There only two callers for the function.
> >>
> >> It looks we could remove nesting from i915_gem_set_domain_ioctl by just
> >> moving the call to after mutex unlock.
> >>
> >> i915_gem_object_unpin_from_display_plane callers are not as easy so
> >> maybe at least do the one above?
> > 
> > unpin_from_display_plane is the goal here tbh.
> > 
> >>> -     i915 = to_i915(obj->base.dev);
> >>>        spin_lock(&i915->mm.obj_lock);
> >>>        list = obj->bind_count ? &i915->mm.bound_list : &i915->mm.unbound_list;
> >>>        list_move_tail(&obj->mm.link, list);
> >>> diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c
> >>> index a76f65fe86be..4a0c6830659d 100644
> >>> --- a/drivers/gpu/drm/i915/i915_gem_evict.c
> >>> +++ b/drivers/gpu/drm/i915/i915_gem_evict.c
> >>> @@ -433,6 +433,7 @@ int i915_gem_evict_vm(struct i915_address_space *vm)
> >>>        }
> >>>    
> >>>        INIT_LIST_HEAD(&eviction_list);
> >>> +     mutex_lock(&vm->mutex);
> >>>        list_for_each_entry(vma, &vm->bound_list, vm_link) {
> >>>                if (i915_vma_is_pinned(vma))
> >>>                        continue;
> >>> @@ -440,6 +441,7 @@ int i915_gem_evict_vm(struct i915_address_space *vm)
> >>>                __i915_vma_pin(vma);
> >>>                list_add(&vma->evict_link, &eviction_list);
> >>>        }
> >>> +     mutex_unlock(&vm->mutex);
> >>
> >> This is another nesting so I suppose you leave all this fun for later?

Yes, I remember some more of the fun that made me defer this task -- and
that was the random waits we could hit, requiring the GPU reset dilemma
be resolved (i.e. reworking reset to avoid taking any these resets,
which also prevents us from hitting these from the shrinker).

> > Yes, the intent was to put the locks in place (gradually) then peel back
> > struct_mutex (gradually).
> > 
> >>> @@ -689,8 +694,10 @@ i915_vma_remove(struct i915_vma *vma)
> >>>    
> >>>        vma->ops->clear_pages(vma);
> >>>    
> >>> +     mutex_lock(&vma->vm->mutex);
> >>>        drm_mm_remove_node(&vma->node);
> >>
> >> This is by design also protected by the vm->mutex? But insertion is not
> >> AFAICT.
> > 
> > Not yet. Can you guess which bit proved tricky? ;) Getting the right
> > point to lock for execbuf, and eviction. At the same time over there is
> > the fuss with ww_mutex, as well as contexts et al, and it all gets
> > confusing quickly.
> > 
> > ...(tries to remember why this patch is actually here; this set was
> > picked so that I could do obj->vma_list without struct_mutex (which
> > was used for timeline allocation) and I pulled in anything required to
> > resolve conflicts, but why this one)...
> 
> Have you figured it out in the meantime?

The patch does at it says and protects the vma->vm_link/vm->*_list. You
start to look at trying to decide if i915_vma_pin() does atomic magic or
if it should require the caller lock(vm->mutex), and I quickly descend
into wanting to do the domain+fence management of
ww_mutex_lock(obj->resv.lock) instead.

Bah, make the caller take vm->mutex and then we can see if that is
better than atomic magic later.
-Chris