[Intel-gfx] [PATCH 12/16] drm/i915: Swap if(enable_execlists) in i915_gem_request_alloc for a vfunc

[Chris Wilson]

On Fri, Dec 09, 2016 at 03:16:33PM +0000, Tvrtko Ursulin wrote:
> 
> On 07/12/2016 13:58, Chris Wilson wrote:
> >A fairly trivial move of a matching pair of routines (for preparing a
> >request for construction) onto an engine vfunc. The ulterior motive is
> >to be able to create a mock request implementation.
> >
> >Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> >---
> > drivers/gpu/drm/i915/i915_gem_request.c | 5 +----
> > drivers/gpu/drm/i915/intel_lrc.c        | 4 +++-
> > drivers/gpu/drm/i915/intel_lrc.h        | 2 --
> > drivers/gpu/drm/i915/intel_ringbuffer.c | 4 +++-
> > drivers/gpu/drm/i915/intel_ringbuffer.h | 3 +--
> > 5 files changed, 8 insertions(+), 10 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c
> >index 06e9a607d934..881bed5347fb 100644
> >--- a/drivers/gpu/drm/i915/i915_gem_request.c
> >+++ b/drivers/gpu/drm/i915/i915_gem_request.c
> >@@ -622,10 +622,7 @@ i915_gem_request_alloc(struct intel_engine_cs *engine,
> > 	req->reserved_space = MIN_SPACE_FOR_ADD_REQUEST;
> > 	GEM_BUG_ON(req->reserved_space < engine->emit_breadcrumb_sz);
> >
> >-	if (i915.enable_execlists)
> >-		ret = intel_logical_ring_alloc_request_extras(req);
> >-	else
> >-		ret = intel_ring_alloc_request_extras(req);
> >+	ret = engine->request_alloc(req);
> > 	if (ret)
> > 		goto err_ctx;
> >
> >diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> >index 22ded92d0346..3f536ef37968 100644
> >--- a/drivers/gpu/drm/i915/intel_lrc.c
> >+++ b/drivers/gpu/drm/i915/intel_lrc.c
> >@@ -845,7 +845,7 @@ static void execlists_context_unpin(struct intel_engine_cs *engine,
> > 	i915_gem_context_put(ctx);
> > }
> >
> >-int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request)
> >+static int execlists_request_alloc(struct drm_i915_gem_request *request)
> > {
> > 	struct intel_engine_cs *engine = request->engine;
> > 	struct intel_context *ce = &request->ctx->engine[engine->id];
> >@@ -1816,6 +1816,8 @@ logical_ring_default_vfuncs(struct intel_engine_cs *engine)
> > 	engine->context_pin = execlists_context_pin;
> > 	engine->context_unpin = execlists_context_unpin;
> >
> >+	engine->request_alloc = execlists_request_alloc;
> >+
> > 	engine->emit_flush = gen8_emit_flush;
> > 	engine->emit_breadcrumb = gen8_emit_breadcrumb;
> > 	engine->emit_breadcrumb_sz = gen8_emit_breadcrumb_sz;
> >diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
> >index b5630331086a..01ba36ea125e 100644
> >--- a/drivers/gpu/drm/i915/intel_lrc.h
> >+++ b/drivers/gpu/drm/i915/intel_lrc.h
> >@@ -63,8 +63,6 @@ enum {
> > };
> >
> > /* Logical Rings */
> >-int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request);
> >-int intel_logical_ring_reserve_space(struct drm_i915_gem_request *request);
> > void intel_logical_ring_stop(struct intel_engine_cs *engine);
> > void intel_logical_ring_cleanup(struct intel_engine_cs *engine);
> > int logical_render_ring_init(struct intel_engine_cs *engine);
> >diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> >index a57eb5dec991..a42b9bf45b42 100644
> >--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> >+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> >@@ -2102,7 +2102,7 @@ void intel_legacy_submission_resume(struct drm_i915_private *dev_priv)
> > 	}
> > }
> >
> >-int intel_ring_alloc_request_extras(struct drm_i915_gem_request *request)
> >+static int ring_request_alloc(struct drm_i915_gem_request *request)
> > {
> > 	int ret;
> >
> >@@ -2597,6 +2597,8 @@ static void intel_ring_default_vfuncs(struct drm_i915_private *dev_priv,
> > 	engine->context_pin = intel_ring_context_pin;
> > 	engine->context_unpin = intel_ring_context_unpin;
> >
> >+	engine->request_alloc = ring_request_alloc;
> >+
> > 	engine->emit_breadcrumb = i9xx_emit_breadcrumb;
> > 	engine->emit_breadcrumb_sz = i9xx_emit_breadcrumb_sz;
> > 	if (i915.semaphores) {
> >diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> >index 0b69a50ab833..12f4dd1cbf9c 100644
> >--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> >+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> >@@ -272,6 +272,7 @@ struct intel_engine_cs {
> > 				       struct i915_gem_context *ctx);
> > 	void		(*context_unpin)(struct intel_engine_cs *engine,
> > 					 struct i915_gem_context *ctx);
> >+	int		(*request_alloc)(struct drm_i915_gem_request *req);
> > 	int		(*init_context)(struct drm_i915_gem_request *req);
> >
> > 	int		(*emit_flush)(struct drm_i915_gem_request *request,
> >@@ -476,8 +477,6 @@ void intel_engine_cleanup(struct intel_engine_cs *engine);
> >
> > void intel_legacy_submission_resume(struct drm_i915_private *dev_priv);
> >
> >-int intel_ring_alloc_request_extras(struct drm_i915_gem_request *request);
> >-
> > int __must_check intel_ring_begin(struct drm_i915_gem_request *req, int n);
> > int __must_check intel_ring_cacheline_align(struct drm_i915_gem_request *req);
> >
> >
> 
> In dev_priv->gt and not per engine would be better. I think we
> talked about this in some other context as well.
>
> Blast, the same then applies to engine->context_(un)pin. :)
> 
I've toyed with the idea of splitting context_pin for RCS/xCS on legacy
since only RCS has a context, and before Ironlake we have no contexts at
all.

->request_alloc() could specialise itself per engine, in theory at
least, don't know if we ever would want to do so in practice.

> What do you think?

Quite happy to pencil the idea in for later under sorting out a new split
for GT vs engine [vs scheduler] ;)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre