Re: [PATCH 3/5] drm/i915: Unify engine init loop

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

On 06/07/16 12:05, Chris Wilson wrote:
> On Wed, Jul 06, 2016 at 11:52:09AM +0100, Tvrtko Ursulin wrote:
>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
>> index fcff30998227..5bebcb16e488 100644
>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>> @@ -2115,6 +2115,7 @@ static const struct engine_info {
>>   	u32 mmio_base;
>>   	unsigned irq_shift;
>>   	int (*init)(struct intel_engine_cs *engine);
>> +	int (*init_ringbuf)(struct intel_engine_cs *engine);
>
> Blatant favouritisim!
>
> The consensus (all of 2, perhas 3, people) name was to call them
>
> 	intel_legacy_submission.c
> 	intel_execlists_submission.c
> 	intel_guc_submission.c
>
> from that we would then have
>
> 	int (*init_legacy_sumission)(struct intel_engine_cs *engine);
> 	int (*init_execlists_sumission)(struct intel_engine_cs *engine);
>
> init_ringbuf() means to me to initialise the ringbuffer structure
> associated both with execlists and legacy.

Agreed on the naming.

> What I tried a couple of years ago was doing all the immediate setup
> inside intel_ringbuffer.c and then dispatching to intel_execlists.c for
> it to override as needed.
>
>> @@ -2201,10 +2203,15 @@ int intel_logical_rings_init(struct drm_device *dev)
>>   		if (!HAS_ENGINE(dev_priv, i))
>>   			continue;
>>
>> -		if (!intel_engines[i].init)
>> +		if (i915.enable_execlists)
>> +			init = intel_engines[i].init;
>> +		else
>> +			init = intel_engines[i].init_ringbuf;
>
> I think I prefer your patch, but I'd like to some all the common setup in
> one place.  intel_engine_cs.c ?

Yes I was thinking that it needs a new home now. intel_engine_cs sounds OK.

Regards,

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