[Intel-gfx] [PATCH v6] drm/i915/icl: Enhanced execution list support

[Daniele Ceraolo Spurio]

On 24/01/18 09:46, Chris Wilson wrote:
> Quoting Daniele Ceraolo Spurio (2018-01-24 17:30:07)
>> From: Thomas Daniel <thomas.daniel at intel.com>
>>
>> Enhanced Execlists is an upgraded version of execlists which supports
>> up to 8 ports. The lrcs to be submitted are written to a submit queue
>> (the ExecLists Submission Queue - ELSQ), which is then loaded on the
>> HW. When writing to the ELSP register, the lrcs are written cyclically
>> in the queue from position 0 to position 7. Alternatively, it is
>> possible to write directly in the individual positions of the queue
>> using the ELSQC registers. To be able to re-use all the existing code
>> we're using the latter method and we're currently limiting ourself to
>> only using 2 elements.
>>
>> The preemption flow is sligthly different with enhanced execlists, so
>> this patch turns preemption off temporarily for platforms with ELSQ
>> while we wait for the new mechanism to land.
>>
>> v2: Rebase.
>> v3: Switch from !IS_GEN11 to GEN < 11 (Daniele Ceraolo Spurio).
>> v4: Use the elsq registers instead of elsp. (Daniele Ceraolo Spurio)
>> v5: Reword commit, rename regs to be closer to specs, turn off
>>      preemption (Daniele), reuse engine->execlists.elsp (Chris)
>> v6: use has_logical_ring_elsq to differentiate the new paths
>>
>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>> Cc: Mika Kuoppala <mika.kuoppala at linux.intel.com>
>> Signed-off-by: Thomas Daniel <thomas.daniel at intel.com>
>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.h          |  7 ++++++-
>>   drivers/gpu/drm/i915/i915_pci.c          |  3 ++-
>>   drivers/gpu/drm/i915/intel_device_info.h |  1 +
>>   drivers/gpu/drm/i915/intel_lrc.c         | 35 +++++++++++++++++++++++++++-----
>>   drivers/gpu/drm/i915/intel_lrc.h         |  3 +++
>>   drivers/gpu/drm/i915/intel_ringbuffer.h  |  6 ++++--
>>   6 files changed, 46 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 8333692..346209a 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2741,8 +2741,13 @@ static inline unsigned int i915_sg_segment_size(void)
>>   
>>   #define HAS_LOGICAL_RING_CONTEXTS(dev_priv) \
>>                  ((dev_priv)->info.has_logical_ring_contexts)
>> +#define HAS_LOGICAL_RING_ELSQ(dev_priv) \
>> +               ((dev_priv)->info.has_logical_ring_elsq)
>> +
>> +/* XXX: Preemption disabled for ELSQ until support for new flow lands */
>>   #define HAS_LOGICAL_RING_PREEMPTION(dev_priv) \
>> -               ((dev_priv)->info.has_logical_ring_preemption)
>> +               ((dev_priv)->info.has_logical_ring_preemption && \
>> +                !HAS_LOGICAL_RING_ELSQ(dev_priv))
> 
> It's in the intel_device_info for a reason. I knew I should not have let
> Michal turn this into a macro.
> 

You mean setting has_logical_ring_preemption to zero directly? I thought 
the policy was to avoid setting things in device_info to values that 
don't reflect real HW capabilities and to do the hacks elsewhere.

> I still do not see any reason why you don't just make the current
> preemption work (it will) and then you can refine it if you prove it
> worthwhile.
> -Chris
> 

Just didn't see the worth of it ;). It's not a lot of code but it's in 
an hot path and we're most likely going to get rid of it soon as the new 
stuff is simpler. I'll put the change together and send it out so we can 
evaluate that and see what works better with code at hand.

Thanks,
Daniele