[Intel-gfx] [PATCH 7/7] drm/i915: Lazily unbind vma on close

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-04-27 16:38:47)
> 
> On 26/04/2018 18:49, Chris Wilson wrote:
> > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > index c74f5df3fb5a..f627a8c47c58 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > @@ -762,7 +762,8 @@ static int eb_lookup_vmas(struct i915_execbuffer *eb)
> >               }
> >   
> >               /* transfer ref to ctx */
> > -             vma->open_count++;
> > +             if (!vma->open_count++)
> > +                     i915_vma_reopen(vma);
> 
> So only execbuf path gets to be able to reopen the VMA? I assume this is 
> sufficient for the use case commit message describes? Other potential 
> use cases are not interesting?

It's the only generator/consumer of user vma. Everything else is the
global gtt. Think PIN_USER vs PIN_GLOBAL.

> > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > index e9d828324f67..272d6bb407cc 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > @@ -2218,6 +2218,12 @@ i915_ppgtt_create(struct drm_i915_private *dev_priv,
> >   }
> >   
> >   void i915_ppgtt_close(struct i915_address_space *vm)
> > +{
> > +     GEM_BUG_ON(vm->closed);
> > +     vm->closed = true;
> > +}
> > +
> > +static void ppgtt_destroy_vma(struct i915_address_space *vm)
> >   {
> >       struct list_head *phases[] = {
> >               &vm->active_list,
> > @@ -2226,15 +2232,12 @@ void i915_ppgtt_close(struct i915_address_space *vm)
> >               NULL,
> >       }, **phase;
> >   
> > -     GEM_BUG_ON(vm->closed);
> >       vm->closed = true;
> 
> There are no more references at this point so no need to mark it as 
> closed I think.

It's a trick for a quicker i915_vma_unbind that skips the PTE updates as
we know the ppgtt is being torn down.

> > -static void i915_vma_destroy(struct i915_vma *vma)
> > +void i915_vma_close(struct i915_vma *vma)
> > +{
> > +     lockdep_assert_held(&vma->vm->i915->drm.struct_mutex);
> > +
> > +     GEM_BUG_ON(i915_vma_is_closed(vma));
> 
> I think the VMA code has gotten pretty messy. For instance a couple of 
> external callers of is_vma_closed feel out of place. Like they should 
> try to do what ever they want with the VMA, say pin it, or close it, and 
> then that operation should either fail or handle the fact, respectively. 
> But just another grumble at this point.

But closing twice would be weird. The assert is here because the code as
is would be broken if called twice.

> > +     vma->flags |= I915_VMA_CLOSED;
> > +
> > +     list_add_tail(&vma->closed_link, &vma->vm->i915->gt.closed_vma);
> > +}
> 
> I think a comment next to this function might be good, doesn't have to 
> be long, just to mention the rationale behind lazy unbind/destroy. Just 
> because often after refactorings and code churn it is difficult to find 
> the commit associated with some logical piece of the code.
> 
> > +
> > +void i915_vma_reopen(struct i915_vma *vma)
> > +{
> > +     lockdep_assert_held(&vma->vm->i915->drm.struct_mutex);
> > +
> > +     if (vma->flags & I915_VMA_CLOSED) {
> > +             vma->flags &= ~I915_VMA_CLOSED;
> > +             list_del(&vma->closed_link);
> > +     }
> > +}
> 
> And then continuing the grumble, this helper wouldn't be needed. If 
> someone had a vlaid vma reference, and tried to do something meaningful 
> wiht it, the vma code would re-open it under the covers.

Oh no, no, no, no ;)

Magically reappearing vma reeks of uabi exposure.

> > diff --git a/drivers/gpu/drm/i915/selftests/mock_gem_device.c b/drivers/gpu/drm/i915/selftests/mock_gem_device.c
> > index a662c0450e77..4b6622c6986a 100644
> > --- a/drivers/gpu/drm/i915/selftests/mock_gem_device.c
> > +++ b/drivers/gpu/drm/i915/selftests/mock_gem_device.c
> > @@ -226,6 +226,7 @@ struct drm_i915_private *mock_gem_device(void)
> >   
> >       INIT_LIST_HEAD(&i915->gt.timelines);
> >       INIT_LIST_HEAD(&i915->gt.active_rings);
> > +     INIT_LIST_HEAD(&i915->gt.closed_vma);
> >   
> >       mutex_lock(&i915->drm.struct_mutex);
> >       mock_init_ggtt(i915);
> > 
> 
> Only two actionable things AFAIR. Then it looks OK to me. Although I 
> would a) see if you can get Joonas to read through it - perhaps he spots 
> something I missed, and b) ah no, won't mention any pencilling in of 
> looking at overall vma handling in the future.

You would only have to read it and execbuffer.c again :-p
-Chris