Re: [CI 13/25] drm/i915: Remove the identical implementations of request space reservation

From: Daniel Vetter <daniel@ffwll.ch>
To: Chris Wilson <chris@chris-wilson.co.uk>,
	Dave Gordon <david.s.gordon@intel.com>,
	intel-gfx@lists.freedesktop.org
Subject: Re: [CI 13/25] drm/i915: Remove the identical implementations of request space reservation
Date: Thu, 28 Apr 2016 16:51:05 +0200	[thread overview]
Message-ID: <20160428145105.GG5784@phenom.ffwll.local> (raw)
In-Reply-To: <20160428143100.GP27856@nuc-i3427.alporthouse.com>

On Thu, Apr 28, 2016 at 03:31:00PM +0100, Chris Wilson wrote:
> On Thu, Apr 28, 2016 at 03:02:18PM +0100, Dave Gordon wrote:
> > On 28/04/16 09:56, Chris Wilson wrote:
> > >Now that we share intel_ring_begin(), reserving space for the tail of
> > >the request is identical between legacy/execlists and so the tautology
> > >can be removed. In the process, we move the reserved space tracking
> > >from the ringbuffer on to the request. This is to enable us to reorder
> > >the reserved space allocation in the next patch.
> > >
> > >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > >Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > >Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> > >Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > >---
> > >  drivers/gpu/drm/i915/i915_drv.h         |  3 +++
> > >  drivers/gpu/drm/i915/i915_gem.c         | 23 ++++++++++-------
> > >  drivers/gpu/drm/i915/intel_lrc.c        | 15 -----------
> > >  drivers/gpu/drm/i915/intel_ringbuffer.c | 44 +++------------------------------
> > >  drivers/gpu/drm/i915/intel_ringbuffer.h | 17 -------------
> > >  5 files changed, 20 insertions(+), 82 deletions(-)
> > 
> > While you're doing all this reconvergence of the different
> > submission mechanisms, how about splitting intel_ringbuffer.c into
> > one file concerned with operations on actual ringbuffers (e.g. all
> > the reserve, wrap, fill, emit stuff) independent of the submission
> > code, and a separate one for the legacy ringbuffer submission
> > mechanism.
> > 
> > Ideally, we could also do the same to intel_lrc.c, with only those
> > operations independent of submission mechanism but unique to Logical
> > Ring Contexts (as opposed to just ringbuffers) remaining in that
> > file, with a separate file again for the execlists submission code.
> > 
> > That would give us five files in total, split like this:
> > * ringbuffer.c           common to *all* ring manipulation
> > * lrc.c                  common code for logical contexts
> > 
> > * legacy_submission.c    TAIL, UHPTR, MI_SWITCH_CONTEXT, etc
> > * execlist_submission.c  ELSP, CSB interrupts, etc
> > * guc_submission.c       GuC WQ, doorbells, etc
> > 
> > Or would this just be too disruptive?
> 
> That split matches the organisation and flow of the code quite well. You
> don't mind if I do this towards the end of a long series of patches?
> 
> I am in favour. Any one else?

+1 Bonus points if it comes with a bit of kerneldoc explaining the
concepts and main interfaces ...

-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx