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

[Daniele Ceraolo Spurio]

On 26/01/18 00:47, Chris Wilson wrote:
> Quoting Daniele Ceraolo Spurio (2018-01-26 00:10:09)
>>
>>
>> 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.
> 
> No, data driven code. intel_device_info was introduced to remove having
> heavy predicates so that we could see what will be enabled and what not
> in one place.
>   
>>> 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.
>>>
>>
>> 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.
> 
> Is the new stuff going to be any simpler? You still need a preemption
> point, so a special submission followed by detecting that in the CS
> handler to do the unwind.
> 
> And whilst I am here, els is awful. Either stick with elsp and note that
> they changed the name (+layout) on icl, or replace it with a generic
> name. Spelling it out completely as execlists->execlists_submission is
> still better than els, but submit[_reg] (or submit_hw) would be clearer.
> -Chris
> 

The elsp still exists on gen11, with a slightly different behavior as 
noted in the commit message, that's why I wanted to change the name. 
execlists->submit_reg sounds good.

Daniele