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

[Chris Wilson]

On Mon, Jun 23, 2014 at 01:36:07PM +0000, Mateo Lozano, Oscar wrote:
> > -----Original Message-----
> > From: Chris Wilson [mailto:chris at chris-wilson.co.uk]
> > Sent: Monday, June 23, 2014 2:27 PM
> > To: Mateo Lozano, Oscar
> > Cc: Volkin, Bradley D; intel-gfx at lists.freedesktop.org
> > Subject: Re: [Intel-gfx] [PATCH 26/53] drm/i915/bdw: New logical ring
> > submission mechanism
> > 
> > On Mon, Jun 23, 2014 at 01:18:35PM +0000, Mateo Lozano, Oscar wrote:
> > > > -----Original Message-----
> > > > From: Chris Wilson [mailto:chris at chris-wilson.co.uk]
> > > > Sent: Monday, June 23, 2014 2:14 PM
> > > > To: Mateo Lozano, Oscar
> > > > Cc: Volkin, Bradley D; intel-gfx at lists.freedesktop.org
> > > > Subject: Re: [Intel-gfx] [PATCH 26/53] drm/i915/bdw: New logical
> > > > ring submission mechanism
> > > >
> > > > 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.
> > > > -Chris
> > >
> > > Sorry, Chris, I obviously don´t have the same experience with 915 you have:
> > how do you propose to extract the right context from the ring?
> > 
> > The rings are a set of buffers and vfuncs that are associated with a context.
> > Before you can call intel_ring_begin() you must know what context you want
> > to operate on and therefore can pick the right logical/legacy ring and
> > interface for RCS/BCS/VCS/etc -Chris
> 
> Ok, but then you need to pass some extra stuff down together with the intel_engine_cs, either intel_context or intel_ringbuffer, right? Because that´s exactly what I did in previous versions, plumbing intel_context  everywhere where it was needed (I could have plumbed intel_ringbuffer instead, it really doesn´t matter). This was rejected for being too intrusive and not allowing easy maintenance in the future.

Nope. You didn't redesign the ringbuffers to function as we expected but
tacked on extra information and layering violations.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre