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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 15/03/2017 09:41, Chris Wilson wrote:
> 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.

Okay, you convinced me. :)

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx