[Intel-gfx] [PATCH 10/29] drm/i915: Introduce context->enter() and context->exit()

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-04-10 11:05:13)
> 
> On 08/04/2019 10:17, Chris Wilson wrote:
> > +void __intel_context_enter(struct intel_context *ce)
> > +{
> > +     struct drm_i915_private *i915 = ce->gem_context->i915;
> > +
> > +     if (!i915->gt.active_requests++)
> > +             i915_gem_unpark(i915);
> > +}
> > +
> > +void __intel_context_exit(struct intel_context *ce)
> > +{
> > +     struct drm_i915_private *i915 = ce->gem_context->i915;
> > +
> > +     GEM_BUG_ON(!i915->gt.active_requests);
> > +     if (!--i915->gt.active_requests)
> > +             i915_gem_park(i915);
> > +}
> 
> In our normal nomenclature __intel_context_enter would be expected to be 
> directly called by intel_context_enter on the 0 -> 1 refcnt transition. 
> Here they are in fact common helpers, so one level of indirection, via a 
> different layer. I found this a bit confusing at first. My usual remedy 
> here is to prescribe a better name. __intel_gt_context_enter/exit? To 
> designate the glue between backend and GT.

The use of gt/gem here is a placeholder. Here we will be iterating over
the engines (so just the one for the usual contexts, and num_siblings for
virtual).

intel_context_enter_engine() ?

And virtual_context_enter remains distinct.

> > diff --git a/drivers/gpu/drm/i915/gt/intel_context.h b/drivers/gpu/drm/i915/gt/intel_context.h
> > index ebc861b1a49e..95b1fdc5826a 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_context.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_context.h
> > @@ -73,6 +73,26 @@ static inline void __intel_context_pin(struct intel_context *ce)
> >   
> >   void intel_context_unpin(struct intel_context *ce);
> >   
> > +void __intel_context_enter(struct intel_context *ce);
> > +void __intel_context_exit(struct intel_context *ce);
> > +
> > +static inline void intel_context_enter(struct intel_context *ce)
> > +{
> > +     if (!ce->active_count++)
> > +             ce->ops->enter(ce);
> > +}
> > +
> > +static inline void intel_context_mark_active(struct intel_context *ce)
> > +{
> 
> GEM_BUG_ON(ce->active_count == 0) ?

We're no longer in Kansas. I was wondering if I could start GT_BUG_ON
and include "intel_gt.h" with a goal of doing a search and replace within
gt/. That gives us some separation from GEM, and maybe we could even
start filtering based on need.
 
> And some lockdep_assert_held in all three?

Read on :(

The plan is for intel_context_enter/_exit to be under the
timeline->mutex, but that isn't realised for about another 30 patches.

mark_active has special protection because it gets used from the
serialised portion of intel_engine_park.
-Chris