[Intel-gfx] [PATCH 26/53] drm/i915/bdw: New logical ring submission mechanism

[Jesse Barnes]

On Mon, 23 Jun 2014 14:13:55 +0100
Chris Wilson <chris at chris-wilson.co.uk> wrote:

> On Mon, Jun 23, 2014 at 01:09:37PM +0000, Mateo Lozano, Oscar wrote:
> > So far, yes, but that´s only because I artificially made
> > intel_lrc.c self-contained, as Daniel requested. What if we need to
> > execute commands from somewhere else, like in
> > intel_gen7_queue_flip()?
> > 
> > And this takes me to another discussion: this logical ring vs
> > legacy ring split is probably a good idea (time will tell), but we
> > should provide a way of sending commands for execution without
> > knowing if Execlists are enabled or not. In the early series that
> > was easy because we reused the ring_begin, ring_emit & ring_advance
> > functions, but this is not the case anymore. And without this,
> > sooner or later somebody will break legacy or execlists (this
> > already happened last week, when somebody here was implementing
> > native sync without knowing about Execlists).
> > 
> > So, the questions is: how do you feel about a dev_priv.gt vfunc
> > that takes a context, a ring, an array of DWORDS and a BB length
> > and does the intel_(logical)_ring_begin/emit/advance based on
> > i915.enable_execlists?
> 
> I'm still baffled by the design. intel_ring_begin() and friends should
> be able to find their context (logical or legacy) from the ring and
> dtrt.

To me, given that the LRC contains the ring, the right thing to do
would be to do what Oscar did in the first place: pass the context
around everywhere.  If there were cases where that wasn't ideal (the
layering violations you mention later?) we should fix them up instead.

But given that this is a standalone file, it's easy to fix up however
we want incrementally, as long as things work well to begin with and
it's reasonably easy to review it...

Jesse