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

[Chris Wilson]

Quoting Chris Wilson (2019-04-03 08:54:41)
> 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.

The problem with that is no, we are not putting a __i915_gem_unpark()
into i915_request_create. At this point, we care about the gt.wakeref.

Maybe I should take another shot at splitting the GEM wakeref from GT.

i915_gem_runtime_pm_get()
  i915_gt_unpark()
  i915_gt_park()
i915_gem_runtime_pm_put()

-Chris