Re: [PATCH 26/53] drm/i915/bdw: New logical ring submission mechanism

[Ben Widawsky <ben@bwidawsk.net>]

On Mon, Jun 23, 2014 at 02:35:38PM +0000, Mateo Lozano, Oscar wrote:
> > -----Original Message-----
> > From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> > Sent: Monday, June 23, 2014 2:42 PM
> > To: Mateo Lozano, Oscar
> > Cc: Volkin, Bradley D; intel-gfx@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:36:07PM +0000, Mateo Lozano, Oscar wrote:
> > > > -----Original Message-----
> > > > From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> > > > Sent: Monday, June 23, 2014 2:27 PM
> > > > To: Mateo Lozano, Oscar
> > > > Cc: Volkin, Bradley D; intel-gfx@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@chris-wilson.co.uk]
> > > > > > Sent: Monday, June 23, 2014 2:14 PM
> > > > > > To: Mateo Lozano, Oscar
> > > > > > Cc: Volkin, Bradley D; intel-gfx@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
> 
> I know is no excuse, but as I said, I don´t know the code as well as you do. Let me explain to you the history of this one and maybe you can help me out discovering where I got it all wrong:
> 
> - The original design I inherited from Ben and Jesse created a completely new "struct intel_ring_buffer" per context, and passed that one on instead of passing one of the &dev_priv->ring[i] ones. When it was time to submit a context to the ELSP, they simply took it from ring->context. The problem with that was that creating an unbound number of "struct intel_ring_buffer" meant there were also an unbound number of things like "struct list_head active_list" or "u32 irq_enable_mask", which made little sense.
> - What we really needed, I naively thought, is to get rid of the concept of ring: there are no rings, there are engines (like the render engine, the vebox, etc...) and there are ringbuffers (as in circular buffer with a head offset, tail offset, and control information). So I went on and renamed the old "intel_ring_buffer" to "intel_engine_cs", then extracting a few things into a new "intel_ringbuffer" struct. Pre-Execlists, there was a 1:1 relationship between the ringbuffers and the engines. With Execlists, however, this 1:1 relationship is between the ringbuffers and the contexts.
> - I remember explaining this problem in a face-to-face meeting in the UK with some OTC people back in February (I think they tried to get you on the phone but they didn´t manage. I do remember they got Daniel though). Here, I proposed that an easy solution (easy, but maybe not right) was to plumb the context down: in Execlists mode, we would retrieve the ringbuffer from the context while, in legacy mode, we would get it from the engine. Everybody seemed to agree with this.
> - I worked on this premise for several versions that I sent to the mailing list for review (first the internal, then intel-gfx). Daniel only complained last month, when he pointed out that he asked, a long long time ago, for a completely separate execution path for Execlists. And this is where we are now...
> 
> And now back to the problem at hand: what do you suggest I do now (other than ritual seppuku, I mean)? I need to know the engine (for a lot of reasons), the ringbuffer (to know where to place commands) and the context (to know what to submit to the ELSP). I can spin it a thousand different ways, but I still need to know those three things. At the same time, I cannot reuse the old intel_ringbuffer code because Daniel won´t approve. What would you do?

I think what Chris is trying to say is that all of your operations
should be context driven. The ringbuffer is always a derivative of the
context. If I understand Chris correctly, I agree with him, but he is
being characteristically terse.

There should be two data structures:
intel_engine_cs, formerly intel_ringbuffer - for legacy
intel_context, formerly intel_hw_context - for execlist

You did that.

Then there should be a data structure to represent the ringbuffer within
the execlist context.

You did that, too.

I don't think the fact that there is a separate execbuf path makes any
difference in this conversation, but perhaps I missed some detail.

So at least from what I can tell, the data structures are right. The
problem is that we're mixing and matching intel_engine_cs with the new
[and I wish we could have used a different name] intel_ringbuffer. As
an example from near the top of the patch:
+       intel_logical_ring_advance(ringbuf);
+
+       if (intel_ring_stopped(ring))
+               return;

You're advancing the ringbuf, but checking the ring? That's confusing to
me.

I think the only solution for what Chris is asking for is to implement
this as 1 context per engine, as opposed to 1 context with a context
object per engine. As you correctly stated, I think we all agreed the
latter was fine when we met. Functionally, I see no difference, but it
does allow you to always use a context as the sole mechanism for making
any decisions and performing any operations. Now without writing all the
code, I can't promise it actually will look better, but I think it's
likely going to be a lot cleaner. Before you do any changes though...

On to what I see as the real problem: fundamentally, if Daniel put Chris
in charge of giving the thumbs or down, then you should get Daniel to
agree that he will defer to Chris, and you should do whatever Chris
says. You need not be caught in the middle of Daniel and Chris - it is a
bad place to be (I know from much experience). If Daniel is not okay
with that, then he needs to find a different reviewer.

-- 
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx