[Intel-gfx] [PATCH 5/5] drm/i915: Flatten engine init control flow

[Daniel Vetter]

On Mon, Dec 01, 2014 at 04:11:15PM +0000, Dave Gordon wrote:
> On 20/11/14 09:19, Daniel Vetter wrote:
> > On Thu, Nov 20, 2014 at 08:10:27AM +0000, Chris Wilson wrote:
> >> On Thu, Nov 20, 2014 at 12:33:08AM +0100, Daniel Vetter wrote:
> >>> Now that sanity prevails and we have the clean split between software
> >>> init and starting the engines we can drop all the "have we allocate
> >>> this struct already?" nonsense.
> >>
> >> Actually, I was hoping for something completely different when you sad
> >> flattening the init...
> > 
> > I presume you've hoped for more ;-) Essentially this is just the legacy
> > ringbuffer blueprint, applying to execlist left as an exercise for the
> > reader. And it's just a very small patch.
> > 
> >> Compare with
> >> http://cgit.freedesktop.org/~ickle/linux-2.6/tree/drivers/gpu/drm/i915/intel_ringbuffer.c#n1983
> >>
> >> There is a lot of duplicated code in the ringbuffer setup, and the code
> >> movement here wants to be split differently so that the sequence is
> >> identical to execlists. (The only difference is that we only use one
> >> ring with legacy, rather than one ring in each context [per-engine].)
> >> Making sure the logic is foolproof for both is esssential.
> > 
> > Imo we first need to beat execlist into shape (with small patches, not by
> > rewriting it all). Once stuff settles we can take another look at whether
> > unification makes sense, but for now I want a big piece of insulation
> > between execlist and stuff we actually depend on. Or at least stuff Dave
> > might rip my head off over.
> > 
> > So for now I still want to embrace the duplication.
> > -Daniel
> 
> As a matter of policy: while we have the duplication and the split
> between legacy ringbuffer and lrc versions, should developers try to
> submit corresponding changes to both paths together (e.g. in the same
> patchset). Or is it OK to submit improvements only to one of the two
> paths, causing them to diverge and hence possibly making deduplication
> more difficult?

Yeah in this case I've figured I'll cause more trouble than benefit when I
trample all over intel_lrc.c. Also, I don't have a machine for testing
execlist ;-)
> 
> Anyway, this change looks OK in itself, so subject to the "exercise for
> the reader" approach being acceptable (on which point I defer to your
> and others judgement), then:
> 
> Reviewed-by: Dave Gordon <david.s.gordon at intel.com>

Queued for -next, thanks for the review.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch