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

[Chris Wilson]

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