[Intel-gfx] [CI 2/4] drm/i915: Serialise with engine-pm around requests on the kernel_context

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-11-25 10:44:15)
> 
> On 24/11/2019 17:05, Chris Wilson wrote:
> > +static inline struct i915_request *
> > +intel_engine_create_kernel_request(struct intel_engine_cs *engine)
> > +{
> > +     struct i915_request *rq;
> > +
> > +     /*
> > +      * The engine->kernel_context is special as it is used inside
> > +      * the engine-pm barrier (see __engine_park()), circumventing
> > +      * the usual mutexes and relying on the engine-pm barrier
> > +      * instead. So whenever we use the engine->kernel_context
> > +      * outside of the barrier, we must manually handle the
> > +      * engine wakeref to serialise with the use inside.
> > +      */
> > +     intel_engine_pm_get(engine);
> > +     rq = i915_request_create(engine->kernel_context);
> > +     intel_engine_pm_put(engine);
> 
> i915_request_add does not have to be covered by the pm ref?

No, the normal course of action is:
i915_request_create:
	mutex_lock(timeline->mutex)
	intel_context_enter:
		if (!ce->active_count++) {
			intel_engine_pm_get();
			intel_timeline_enter();
		}

With the primary purpose of reducing atomics behind a simple usage
counter covered by the timeline->mutex.

Of course, engine-pm tries to be clever because it has to be called
under any timeline->mutex from retire, and so tries to fake
timeline->mutex itself.

> I am slightly confused by how patch converts some to this helper and at 
> some places it open codes it.

There were a few perf benchmarks, that I thought better to not add the
extra pair of atomics, and intel_engine_heartbeat.c is supposed to
cognisant of the rules and only operates on an active engine-pm.

The intent of intel_engine_create_kernel_request() was to apply a simple
fixup with a comment as to what is going on with the kernel_context.
-Chris