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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
