[Intel-gfx] [PATCH 06/22] drm/i915: Pass intel_context to i915_request_create()

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-04-02 14:17:30)
> 
> On 25/03/2019 09:03, Chris Wilson wrote:
> > @@ -727,17 +695,19 @@ i915_request_alloc(struct intel_engine_cs *engine, struct i915_gem_context *ctx)
> >       if (ret)
> >               goto err_unwind;
> >   
> > -     ret = engine->request_alloc(rq);
> > +     ret = rq->engine->request_alloc(rq);
> >       if (ret)
> >               goto err_unwind;
> >   
> > +     rq->infix = rq->ring->emit; /* end of header; start of user payload */
> > +
> >       /* Keep a second pin for the dual retirement along engine and ring */
> >       __intel_context_pin(ce);
> > -
> > -     rq->infix = rq->ring->emit; /* end of header; start of user payload */
> > +     atomic_inc(&i915->gt.active_requests);
> 
> Oh I hate this..
> 
> >   
> >       /* Check that we didn't interrupt ourselves with a new request */
> >       GEM_BUG_ON(rq->timeline->seqno != rq->fence.seqno);
> > +     lockdep_assert_held(&tl->mutex);
> >       return rq;

[snip]

> > +     /*
> > +      * Pinning the contexts may generate requests in order to acquire
> > +      * GGTT space, so do this first before we reserve a seqno for
> > +      * ourselves.
> > +      */
> > +     ce = intel_context_pin(ctx, engine);
> > +     if (IS_ERR(ce))
> > +             return ERR_CAST(ce);
> > +
> > +     i915_gem_unpark(i915);
> > +
> > +     rq = i915_request_create(ce);
> > +
> > +     i915_gem_park(i915);
> 
> ... because it is so anti-self-documenting.
> 
> Maybe we could have something like:
> 
> __i915_gem_unpark(...)
> {
>         GEM_BUG_ON(!atomic_read(active_requests));
>         atomic_inc(active_requests);
> }
> 
> Similar to __intel_context_pin, just so it is more obvious what the code 
> is doing.

But not here though. Since this is the path that has to wake up the
device itself, pin the context (eventually run retirement, handle
allocation fallback) and not presume the caller already has (or will).

Inside i915_require_create we do have the assert that the device is
awake, which should be has GT wakeref instead. (Note, not GEM wakeref at
this point, that's the troubling bit.)

Post snip: Saw the connected argument.
-Chris