[Intel-gfx] [PATCH 4/5] drm/i915/gem: Pin gen6_ppgtt prior to constructing the request

[Chris Wilson]

Quoting Andi Shyti (2019-12-06 23:31:26)
> Hi Chris,
> 
> > All pinning must be done prior to i915_request_create, to avoid
> > timeline->mutex inversions.
> > 
> > Here we slightly abuse the context_barrier_task stages to utilise the
> > 'skip' decision as an opportunity to acquire the pin on the new ppgtt.
> > Consider it s/skip/prepare/. At the moment, we only have on user of
> > context_barrier_task, so it might be worth breaking it down for the
> > specific task of set-vm and refactor it later if we find a second
> > purpose.
> 
> [...]
> 
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > index 9f1dc96b10a6..9d8d75765ee4 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > @@ -1141,8 +1141,6 @@ static int emit_ppgtt_update(struct i915_request *rq, void *data)
> >               *cs++ = MI_NOOP;
> >               intel_ring_advance(rq, cs);
> >       } else {
> > -             /* ppGTT is not part of the legacy context image */
> > -             gen6_ppgtt_pin(i915_vm_to_ppgtt(vm));
> >       }
> 
> mh? Am I not seeing something obvious? Can we remove the else?

Sure, I just have this thing about if() else if() that feels unbalanced
Just feels odd not to have something there.  :)

> 
> >  
> >       return 0;
> > @@ -1150,10 +1148,20 @@ static int emit_ppgtt_update(struct i915_request *rq, void *data)
> >  
> >  static bool skip_ppgtt_update(struct intel_context *ce, void *data)
> >  {
> > +     if (!test_bit(CONTEXT_ALLOC_BIT, &ce->flags))
> > +             return true;
> > +
> >       if (HAS_LOGICAL_RING_CONTEXTS(ce->engine->i915))
> > -             return !ce->state;
> > -     else
> > -             return !atomic_read(&ce->pin_count);
> > +             return false;
> > +
> > +     if (!atomic_read(&ce->pin_count))
> > +             return true;
> > +
> > +     /* ppGTT is not part of the legacy context image */
> > +     if (gen6_ppgtt_pin(i915_vm_to_ppgtt(ce->vm)))
> > +             return true;
> > +
> > +     return false;
> 
> looks correct, a bit tricky, but I don't see any issue.

The issue is the code is creaking beyond its design tolerances. :)
-Chris