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

[Tvrtko Ursulin]

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:
>>> It turns out that we may want to restore the original
>>> engine->submit_request (and engine->schedule) callbacks from more than
>>> just the guc <-> execlists transition. Move this to a vfunc so we can
>>> have a common interface.
>>>
>>> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>> Cc: Mika Kuoppala <mika.kuoppala at intel.com>
>>> ---
>>> drivers/gpu/drm/i915/i915_guc_submission.c |  2 +-
>>> drivers/gpu/drm/i915/intel_engine_cs.c     | 10 ++++++++++
>>> drivers/gpu/drm/i915/intel_lrc.c           | 15 +++++----------
>>> drivers/gpu/drm/i915/intel_lrc.h           |  1 -
>>> drivers/gpu/drm/i915/intel_ringbuffer.c    | 15 +++++++++++++--
>>> drivers/gpu/drm/i915/intel_ringbuffer.h    |  4 ++++
>>> 6 files changed, 33 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
>>> index b4d24cd7639a..119b5c073833 100644
>>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>>> @@ -1051,7 +1051,7 @@ void i915_guc_submission_disable(struct drm_i915_private *dev_priv)
>>> 		return;
>>>
>>> 	/* Revert back to manual ELSP submission */
>>> -	intel_execlists_enable_submission(dev_priv);
>>> +	intel_engines_enable_submission(dev_priv);
>>
>> intel_engines_default_submission came to my mind but that will also
>> be misleading once the guc switch is toggled. But I think less
>> misleading than enable_submission. And vfunc name maybe as
>> assign_default_submission?
>
> intel_engines_reset_default_submission
> engine->set_default_submission
>
> Not overly enamoured, but the above seems like the best compromise so
> far.

Sounds good.

>>> }
>>>
>>> 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.

Legacy is a bit more uncleanly split now that I look at it, but putting 
it at intel_engine_init_common would be just more of the same.

Regards,

Tvrtko