Hi Daniel, > -----Original Message----- > From: Daniel Vetter [mailto:daniel.vetter@xxxxxxxx] On Behalf Of Daniel Vetter > Sent: Thursday, May 15, 2014 9:52 PM > To: Mateo Lozano, Oscar > Cc: Lespiau, Damien; Daniel Vetter; intel-gfx@xxxxxxxxxxxxxxxxxxxxx > Subject: Re: [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@xxxxxxxxxxxxxxxxxxxxx > > > Subject: Re: [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@xxxxxxxxx > wrote: > > > > > From: Oscar Mateo <oscar.mateo@xxxxxxxxx> > > > > > > > > > > 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@xxxxxxxxx> > > > > > > > > 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
