> -----Original Message----- > From: Daniel Vetter [mailto:daniel.vetter@xxxxxxxx] On Behalf Of Daniel > Vetter > Sent: Thursday, August 14, 2014 4:37 PM > To: Daniel, Thomas > Cc: Daniel Vetter; intel-gfx@xxxxxxxxxxxxxxxxxxxxx > Subject: Re: [PATCH 08/43] drm/i915/bdw: Add a context and an > engine pointers to the ringbuffer > > On Thu, Aug 14, 2014 at 05:32:28PM +0200, Daniel Vetter wrote: > > On Thu, Aug 14, 2014 at 03:09:45PM +0000, Daniel, Thomas wrote: > > > When it comes to the execlist submission (actually as early as the > > > execlist request queueing), the engine and context are indeed used and > required. > > > intel_logical_ring_advance_and_submit() is the lrc function > > > analogous to > > > __intel_ring_advance() and I believe the initial creation of > > > intel_lrc.c was actually done by copying intel_ringbuffer.c. This > > > explains why some of the lrc code is perhaps not as it would have > > > been if this had been designed from scratch, and there is room for future > improvement. > > > advance_and_submit therefore only gets the ringbuffer struct and > > > uses the context pointer in that struct to get the logical ring > > > context itself. At that point the engine, context and new tail > > > pointer are handed over to the execlist queue backend. > > > > I guess I need to clarify: Does it make sense to move the ELSP > > respectively the submission to the execlist scheduler queue out of > > there up a few levels into the execlist cmd submission function? Is it > > possible or is there some technical reason that I'm overlooking? Yes this would make sense, and we already have a separate emit_request vfunc which is only used in lrc mode so we can for example change the signature to accept a drm_i915_gem_request* directly and take the ctx and engine from there. > > > > I want to know what exactly I'm dealing with here before I sign up for > > it by merging the patches as-is and asking for a cleanup. I doesn't > > look bad really, but there's always a good chance that I've overlooked > > a bigger dragon. > > > > Since you have the patches and worked with them I'm asking you such > > explorative questions. Ofc I can do this checking myself, but that > > takes time ... This doesn't mean that you have to implement the > > changes, just be reasonable confident that it will work out as a cleanup on > top. Understood. > > To clarify more the context: Currently you're replies sound like "This is what it > looks like and I don't really know why nor whether we can change that". You're right, I don't know why it was done this way. But I can see that there is no problem to change it later - it's only a piece of code after all... > That's not confidence instilling and that makes maintainers reluctant to > merge patches for fear of needing to fix things themselves > ;-) If it helps, I can tell you that several guys in our team are working with this code and we have a vested interest in making sure the quality is as high as possible. Cheers, Thomas. > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
