[Intel-gfx] [PATCH 25/53] drm/i915/bdw: GEN-specific logical ring submit context (somewhat)

[Mateo Lozano, Oscar]

> -----Original Message-----
> From: Volkin, Bradley D
> Sent: Friday, June 20, 2014 9:28 PM
> To: Mateo Lozano, Oscar
> Cc: intel-gfx at lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 25/53] drm/i915/bdw: GEN-specific logical
> ring submit context (somewhat)
> 
> On Fri, Jun 13, 2014 at 08:37:43AM -0700, oscar.mateo at intel.com wrote:
> > From: Oscar Mateo <oscar.mateo at intel.com>
> >
> > For the moment, just mark the place (we still need to do a lot of
> > preparation before execlists are ready to start submitting things).
> >
> > Signed-off-by: Oscar Mateo <oscar.mateo at intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_lrc.c        | 11 +++++++++++
> >  drivers/gpu/drm/i915/intel_ringbuffer.h |  6 ++++++
> >  2 files changed, 17 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_lrc.c
> > b/drivers/gpu/drm/i915/intel_lrc.c
> > index 6c62ae5..02fc3d0 100644
> > --- a/drivers/gpu/drm/i915/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > @@ -139,6 +139,12 @@ static void gen8_set_seqno(struct intel_engine_cs
> *ring, u32 seqno)
> >  	intel_write_status_page(ring, I915_GEM_HWS_INDEX, seqno);  }
> >
> > +static void gen8_submit_ctx(struct intel_engine_cs *ring,
> > +			    struct intel_context *ctx, u32 value) {
> > +	DRM_ERROR("Execlists still not ready!\n"); }
> > +
> >  void intel_logical_ring_cleanup(struct intel_engine_cs *ring)  {
> >  	if (!intel_ring_initialized(ring))
> > @@ -213,6 +219,7 @@ static int logical_render_ring_init(struct
> drm_device *dev)
> >  	ring->cleanup = intel_fini_pipe_control;
> >  	ring->get_seqno = gen8_get_seqno;
> >  	ring->set_seqno = gen8_set_seqno;
> > +	ring->submit_ctx = gen8_submit_ctx;
> >
> >  	return logical_ring_init(dev, ring);  } @@ -231,6 +238,7 @@ static
> > int logical_bsd_ring_init(struct drm_device *dev)
> >  	ring->init = gen8_init_common_ring;
> >  	ring->get_seqno = gen8_get_seqno;
> >  	ring->set_seqno = gen8_set_seqno;
> > +	ring->submit_ctx = gen8_submit_ctx;
> >
> >  	return logical_ring_init(dev, ring);  } @@ -249,6 +257,7 @@ static
> > int logical_bsd2_ring_init(struct drm_device *dev)
> >  	ring->init = gen8_init_common_ring;
> >  	ring->get_seqno = gen8_get_seqno;
> >  	ring->set_seqno = gen8_set_seqno;
> > +	ring->submit_ctx = gen8_submit_ctx;
> >
> >  	return logical_ring_init(dev, ring);  } @@ -267,6 +276,7 @@ static
> > int logical_blt_ring_init(struct drm_device *dev)
> >  	ring->init = gen8_init_common_ring;
> >  	ring->get_seqno = gen8_get_seqno;
> >  	ring->set_seqno = gen8_set_seqno;
> > +	ring->submit_ctx = gen8_submit_ctx;
> >
> >  	return logical_ring_init(dev, ring);  } @@ -285,6 +295,7 @@ static
> > int logical_vebox_ring_init(struct drm_device *dev)
> >  	ring->init = gen8_init_common_ring;
> >  	ring->get_seqno = gen8_get_seqno;
> >  	ring->set_seqno = gen8_set_seqno;
> > +	ring->submit_ctx = gen8_submit_ctx;
> >
> >  	return logical_ring_init(dev, ring);  } diff --git
> > a/drivers/gpu/drm/i915/intel_ringbuffer.h
> > b/drivers/gpu/drm/i915/intel_ringbuffer.h
> > index ff8753c..1a6df42 100644
> > --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> > @@ -79,6 +79,8 @@ struct intel_ringbuffer {
> >  	u32 last_retired_head;
> >  };
> >
> > +struct intel_context;
> > +
> >  struct  intel_engine_cs {
> >  	const char	*name;
> >  	enum intel_ring_id {
> > @@ -146,6 +148,10 @@ struct  intel_engine_cs {
> >  				  unsigned int num_dwords);
> >  	} semaphore;
> >
> > +	/* Execlists */
> > +	void		(*submit_ctx)(struct intel_engine_cs *ring,
> > +				      struct intel_context *ctx, u32 value);
> > +
> 
> Is it worth making this a vfunc in the refactored codebase? It ends up as the
> same function for all engines...called in one place...the implementation of
> which is a single call to another function that takes the same arguments.
> Previously this was an implementation of the write_tail vfunc, so it made
> sense. I'm not so sure now.

Now that you say it, no, it´s probably not worth it. This stuff has changed so many times that sometimes it´s difficult to keep track :(

-- Oscar