[Intel-gfx] [PATCH] drm/i915: Mark up i915_vma_unbind() as a potential sleeper

[Chris Wilson]

Quoting Joonas Lahtinen (2017-11-09 11:57:15)
> On Thu, 2017-11-09 at 10:59 +0000, Chris Wilson wrote:
> > Whenever we want to unbind a vma, we must wait on all GPU activity to
> > complete first. (This is what gives us the ability to do fine grained
> > eviction and purging by only having to wait on the VMA that we need to
> > unbind to proceed; though if pushed we can make it a rule that we are
> > only allowed to unbind already idle VMA and move the burden of the work
> > and organising the sleep onto the caller.) Currently, we might only
> > sleep if the vma is still active on the GPU, but in principle
> > i915_vma_unbind() always implies a sleep, so mark it up with a
> > might_sleep().
> > 
> > Suggested-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> > Cc: Matthew Auld <matthew.william.auld at gmail.com>
> > Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> 
> <SNIP>
> 
> > +++ b/drivers/gpu/drm/i915/i915_vma.c
> > @@ -743,6 +743,7 @@ int i915_vma_unbind(struct i915_vma *vma)
> >       /* First wait upon any activity as retiring the request may
> >        * have side-effects such as unpinning or even unbinding this vma.
> >        */
> > +     might_sleep();
> 
> Please lift this up, even before lockdep_assert_held, for more clarity.

The comment was explaining why we might_sleep(), so I liked it here. The
pattern that's kind of been established is that we document the locks we
expect the caller to be holding first, but that pattern is easy to break.

But the comment doesn't belong before lockdep_assert_held() as it is
currently written. :|
-Chris