Re: [PATCH 06/50] drm/i915: s/intel_ring_buffer/intel_engine

["Mateo Lozano, Oscar" <oscar.mateo@intel.com>]

Hi Daniel,

> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
> Sent: Thursday, May 15, 2014 9:52 PM
> To: Mateo Lozano, Oscar
> Cc: Lespiau, Damien; Daniel Vetter; intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 06/50] drm/i915:
> s/intel_ring_buffer/intel_engine
> 
> On Thu, May 15, 2014 at 02:17:23PM +0000, Mateo Lozano, Oscar wrote:
> > > -----Original Message-----
> > > From: Lespiau, Damien
> > > Sent: Wednesday, May 14, 2014 2:26 PM
> > > To: Daniel Vetter
> > > Cc: Mateo Lozano, Oscar; intel-gfx@lists.freedesktop.org
> > > Subject: Re: [Intel-gfx] [PATCH 06/50] drm/i915:
> > > s/intel_ring_buffer/intel_engine
> > >
> > > On Tue, May 13, 2014 at 03:28:27PM +0200, Daniel Vetter wrote:
> > > > On Fri, May 09, 2014 at 01:08:36PM +0100, oscar.mateo@intel.com
> wrote:
> > > > > From: Oscar Mateo <oscar.mateo@intel.com>
> > > > >
> > > > > In the upcoming patches, we plan to break the correlation
> > > > > between engines (a.k.a. rings) and ringbuffers, so it makes
> > > > > sense to refactor the code and make the change obvious.
> > > > >
> > > > > No functional changes.
> > > > >
> > > > > Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> > > >
> > > > If we rename stuff I'd vote for something close to Bspec language,
> > > > like CS. So maybe intel_cs_engine?
> >
> > Bikeshedding much, are we? :)
> > If we want to get closer to bspecish, intel_engine_cs would be better.
> 
> I'm ok with that too ;-)
> 
> > > Also, can we have such patches (and the like of "drm/i915:
> > > for_each_ring") pushed early when everyone is happy with them, they
> > > cause constant rebasing pain.
> >
> > I second that motion!
> 
> Fully agreed - as soon as we have a rough sketch of where we want to go to I'll
> pull in the rename. Aside I highly suggest to do the rename with coccinelle and
> regerate it on rebases - that's much less error-prone than doing it by hand.
> -Daniel

I propose the following code refactoring at a minimum. Even if I abstract away all the "i915_gem_context.c" and "intel_ringbuffer.c" functionality, and part of "i915_gem_execbuffer.c", to keep changes to legacy code to a minimum, I still think the following changes are good for the overall code:

1)	s/intel_ring_buffer/intel_engine_cs

Straight renaming: if the actual ring buffers can live either inside the engine/ring (legacy ringbuffer submission) or inside the context (execlists), it doesn´t make sense that the engine/ring is called "intel_ring_buffer".

2)	Split the ringbuffers and the rings

New struct:

+struct intel_ringbuffer {
+       struct drm_i915_gem_object *obj;
+       void __iomem *virtual_start;
+
+       u32 head;
+       u32 tail;
+       int space;
+       int size;
+       int effective_size;
+
+       /** We track the position of the requests in the ring buffer, and
+        * when each is retired we increment last_retired_head as the GPU
+        * must have finished processing the request and so we know we
+        * can advance the ringbuffer up to that position.
+        *
+        * last_retired_head is set to -1 after the value is consumed so
+        * we can detect new retirements.
+        */
+       u32 last_retired_head;
+};

And "struct intel_engine_cs" now groups all these elements into "buffer":

-       void            __iomem *virtual_start;
-       struct          drm_i915_gem_object *obj;
-       u32             head;
-       u32             tail;
-       int             space;
-       int             size;
-       int             effective_size;
-       u32             last_retired_head;
+       struct intel_ringbuffer buffer;

3)	Introduce one context backing object per engine

-       struct drm_i915_gem_object *obj;
+       struct {
+               struct drm_i915_gem_object *obj;
+       } engine[I915_NUM_RINGS];

Legacy code only ever uses engine[RCS], so I can use it everywhere in the existing code.

If we agree on this minimum set, I´ll send the patches right away.

Cheers,
Oscar