[Intel-gfx] [PATCH v3 1/2] drm/i915: Move engine->submit_request selection to a vfunc

[Chris Wilson]

On Wed, Mar 15, 2017 at 08:14:04AM +0000, Tvrtko Ursulin wrote:
> 
> On 14/03/2017 21:33, Chris Wilson wrote:
> >On Tue, Mar 14, 2017 at 04:31:58PM +0000, Tvrtko Ursulin wrote:
> >>
> >>On 14/03/2017 09:34, Chris Wilson wrote:
> >>>}
> >>>
> >>>void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
> >>>diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> >>>index 73fe718516a5..5663ebab851f 100644
> >>>--- a/drivers/gpu/drm/i915/intel_engine_cs.c
> >>>+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> >>>@@ -191,6 +191,7 @@ int intel_engines_init(struct drm_i915_private *dev_priv)
> >>>			goto cleanup;
> >>>		}
> >>>
> >>>+		engine->enable_submission(engine);
> >>
> >>Could be moved to intel_engines_setup_common if the
> >>logical_ring_setup was re-arranged a bit so that the default_vfuncs
> >>are assigned before it. Legacy looks like it would be alright with
> >>that approach already.
> >>
> >>My thinking here is not to expose this vfunc so prominently in the
> >>code since it is a bit of a low level internal implementation thing.
> >
> >The concern is reasonable, but equally moving it to
> >intel_engine_setup_common() is hairy. Otoh, I think it is suitable for
> >intel_engine_init_common(). Happy?
> 
> Why do you think it is hairy for intel_engine_setup_common? It keeps
> the setup/init split for lrc, where all vfuncs are initialized in
> the setup phase which was the intention.

At the moment, setup_common() has no dependencies. Having the callback
for engine->set_default_submission inside init_common() doesn't break
the pattern of all vfuncs being in setup_common() - if you no longer
regard the two vfuncs set by the callback as being part of that set.

In init_common() we already utilize the state set on the engine, the
callback seems consistent there.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre