[Intel-gfx] [PATCH 06/50] drm/i915: s/intel_ring_buffer/intel_engine
Mateo Lozano, Oscar
oscar.mateo at intel.com
Mon May 19 12:02:07 CEST 2014
Hi Daniel,
> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter at 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 at 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 at 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 at intel.com
> wrote:
> > > > > From: Oscar Mateo <oscar.mateo at 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 at 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
More information about the Intel-gfx
mailing list