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