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

[Daniel, Thomas]

> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] On Behalf Of Daniel
> Vetter
> Sent: Wednesday, August 13, 2014 4:16 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 Wed, Aug 13, 2014 at 01:34:15PM +0000, Daniel, Thomas wrote:
> >
> >
> > > -----Original Message-----
> > > From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] On Behalf Of
> > > Daniel Vetter
> > > Sent: Monday, August 11, 2014 3:21 PM
> > > To: Daniel, Thomas
> > > Cc: 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 Mon, Aug 11, 2014 at 04:14:13PM +0200, Daniel Vetter wrote:
> > > > On Thu, Jul 24, 2014 at 05:04:16PM +0100, Thomas Daniel wrote:
> > > > > From: Oscar Mateo <oscar.mateo at intel.com>
> > > > >
> > > > > Any given ringbuffer is unequivocally tied to one context and one
> engine.
> > > > > By setting the appropriate pointers to them, the ringbuffer
> > > > > struct holds all the infromation you might need to submit a
> > > > > workload for processing, Execlists style.
> > > > >
> > > > > Signed-off-by: Oscar Mateo <oscar.mateo at intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_lrc.c        |    2 ++
> > > > >  drivers/gpu/drm/i915/intel_ringbuffer.c |    2 ++
> > > > >  drivers/gpu/drm/i915/intel_ringbuffer.h |    3 +++
> > > > >  3 files changed, 7 insertions(+)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c
> > > > > b/drivers/gpu/drm/i915/intel_lrc.c
> > > > > index 0a12b8c..2eb7db6 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_lrc.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > > > > @@ -132,6 +132,8 @@ int intel_lr_context_deferred_create(struct
> > > intel_context *ctx,
> > > > >  		return ret;
> > > > >  	}
> > > > >
> > > > > +	ringbuf->ring = ring;
> > > > > +	ringbuf->ctx = ctx;
> > > > >  	ringbuf->size = 32 * PAGE_SIZE;
> > > > >  	ringbuf->effective_size = ringbuf->size;
> > > > >  	ringbuf->head = 0;
> > > > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c
> > > > > b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > > > > index 01e9840..279dda4 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > > > > @@ -1570,6 +1570,8 @@ static int intel_init_ring_buffer(struct
> > > drm_device *dev,
> > > > >  	INIT_LIST_HEAD(&ring->active_list);
> > > > >  	INIT_LIST_HEAD(&ring->request_list);
> > > > >  	ringbuf->size = 32 * PAGE_SIZE;
> > > > > +	ringbuf->ring = ring;
> > > > > +	ringbuf->ctx = ring->default_context;
> > > >
> > > > That doesn't make a terribly lot of sense tbh. I fear it's one of
> > > > these slight confusions which will take tons of patches to clean up.
> > > > Why exactly do we need the ring->ctx pointer?
> > > >
> > > > If we only need this for lrc I want to name it accordingly, to
> > > > make sure legacy code doesn't grow stupid ideas. And also we
> > > > should only initialize this in the lrc ctx init then.
> > > >
> > > > All patches up to this one merged.
> > >
> > > Ok, I've discussed this quickly with Damien on irc. We decided to
> > > cut away the ring->ctx part of this patch for now to be able to move on.
> > > -Daniel
> > As you've seen, removing ringbuffer->ctx causes serious problems with
> > the plumbing later on.  This can be renamed (perhaps to lrc) and
> > removed from legacy init.
> >
> > Each ring buffer belongs to a specific context - it makes sense to me
> > to keep this information within the ringbuffer structure so that we
> > don't have to pass the context pointer around everywhere.
> 
> I agree that it causes trouble with the follow-up patches, but I'm not sold on
> this being a terrible good idea. After all for ELSP we don't want to submit a
> ring, we want to submit the full context. So if the code that's supposed to do
> the execlist ctx submission only has the pointer to the ring object, the layer
> looks a bit wrong.
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.

Thomas.

> 
> Same was iirc about the add_request part.
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch