[Intel-gfx] [PATCH 08/43] drm/i915/bdw: Add a context and an engine pointers to the ringbuffer

[Daniel Vetter]

On Thu, Aug 14, 2014 at 03:56:20PM +0000, Daniel, Thomas wrote:
> 
> 
> > -----Original Message-----
> > From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] On Behalf Of Daniel
> > Vetter
> > Sent: Thursday, August 14, 2014 4:37 PM
> > To: Daniel, Thomas
> > Cc: Daniel Vetter; intel-gfx at lists.freedesktop.org
> > Subject: Re: [Intel-gfx] [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.

Hm, I didn't spot the emit_request vfunc yet. Probably another one that
I'll ask you to fold in ;-)

> > > 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.

Ok, I think you get the hang of how this "guide your maintainer into
accepting stuff" game works ;-) I'll take your word for it that there's no
dragon hiding and that you'll bravely fight it anyway and will merge in a
few more patches. And I'll do a JIRA to jot down the restructuring we need
to do.

Merging might be a bit slower since I'm heading to Chicago on Saturday
already, so I might ask you to resend the remaining patches.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch