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

[Mateo Lozano, Oscar]

> -----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.

-- Oscar