[Intel-gfx] [PATCH 32/38] drm/i915: Introduce intel_context.pin_mutex for pin management

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-03-06 09:45:27)
> 
> On 01/03/2019 14:03, Chris Wilson wrote:
> > Introduce a mutex to start locking the HW contexts independently of
> > struct_mutex, with a view to reducing the coarse struct_mutex. The
> > intel_context.pin_mutex is used to guard the transition to and from being
> > pinned on the gpu, and so is required before starting to build any
> > request, and released when the execution is completed.
> 
> execution is completed or construction/emission is completed?

We keep the context pinned until the end of emission, but we only need
to hold the lock while pinning the request. Yup, that last sentence
changed direction half-way through and is confused.

> >   static int
> > -__i915_gem_context_reconfigure_sseu(struct i915_gem_context *ctx,
> > -                                 struct intel_engine_cs *engine,
> > -                                 struct intel_sseu sseu)
> > +i915_gem_context_reconfigure_sseu(struct i915_gem_context *ctx,
> > +                               struct intel_engine_cs *engine,
> > +                               struct intel_sseu sseu)
> >   {
> >       struct intel_context *ce;
> >       int ret = 0;
> >   
> > -     ce = intel_context_instance(ctx, engine);
> > -     if (IS_ERR(ce))
> > -             return PTR_ERR(ce);
> > -
> >       GEM_BUG_ON(INTEL_GEN(ctx->i915) < 8);
> >       GEM_BUG_ON(engine->id != RCS);
> >   
> > +     ce = intel_context_pin_lock(ctx, engine);
> > +     if (IS_ERR(ce))
> > +             return PTR_ERR(ce);
> > +
> >       /* Nothing to do if unmodified. */
> >       if (!memcmp(&ce->sseu, &sseu, sizeof(sseu)))
> > -             return 0;
> > -
> > -     /*
> > -      * If context is not idle we have to submit an ordered request to modify
> > -      * its context image via the kernel context. Pristine and idle contexts
> > -      * will be configured on pinning.
> > -      */
> > -     if (ce->pin_count)
> > -             ret = gen8_modify_rpcs_gpu(ce, engine, sseu);
> > +             goto unlock;
> >   
> > +     ret = gen8_modify_rpcs_gpu(ce, sseu);
> 
> The _gpu suffix is not true any more so I'd drop it.

Ok.

> > @@ -134,16 +153,35 @@ intel_context_pin(struct i915_gem_context *ctx,
> >   
> >               i915_gem_context_get(ctx);
> >               GEM_BUG_ON(ce->gem_context != ctx);
> > +
> > +             smp_mb__before_atomic();
> 
> Why is this needed?

nop on intel, but because we are using the pin_count == 0 as the barrier
for others, before that is incremented we need to be sure that writes
into ce (for the pinned state) are visible.

> > @@ -50,18 +64,10 @@ intel_context_pin(struct i915_gem_context *ctx, struct intel_engine_cs *engine);
> >   
> >   static inline void __intel_context_pin(struct intel_context *ce)
> >   {
> > -     GEM_BUG_ON(!ce->pin_count);
> > -     ce->pin_count++;
> > +     GEM_BUG_ON(!intel_context_is_pinned(ce));
> > +     atomic_inc(&ce->pin_count);
> 
> This can be called without the mutex held but only if not zero, right?
> 
> BUG_ON(!atomic_inc_not_zero(&ce->pin_count)) ?

That's quite heavy (it's a locked cmpxchg), whereas a plain locked inc
doesn't involve a roundtrip.

In terms of this being a function that should only be used at controlled
points, and for that I think the GEM_BUG_ON(!intel_context_is_pinned())
is the right documentation (i.e. you should only be using this if you
know you have already pinned the ce yourself).

> >       u32 *lrc_reg_state;
> >       u64 lrc_desc;
> > -     int pin_count;
> > +
> > +     atomic_t pin_count;
> > +     struct mutex pin_mutex; /* guards pinning and associated on-gpuing */
> 
> Not pinning but transition from pinned to unpinned state AFAIU.

unpinned to pinned! A lot may happen the first time we pin, and every
user needs to be serialised across that.

pinned to unpinned is less tricky, but still requires serialisation with a
potential concurrent pinner.

The only cheap (as cheap as any cmpxchg operation may be) path is for
already pinned ce, which fortunately is our steady state.
-Chris