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

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Aug 13, 2014 at 01:34:15PM +0000, Daniel, Thomas wrote:
> 
> 
> > -----Original Message-----
> > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel
> > Vetter
> > Sent: Monday, August 11, 2014 3:21 PM
> > To: Daniel, Thomas
> > Cc: intel-gfx@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@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@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.

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