Re: [PATCH v3 1/2] drm/i915: Move engine->submit_request selection to a vfunc

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 14/03/2017 09:34, Chris Wilson wrote:
> It turns out that we may want to restore the original
> engine->submit_request (and engine->schedule) callbacks from more than
> just the guc <-> execlists transition. Move this to a vfunc so we can
> have a common interface.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_guc_submission.c |  2 +-
>  drivers/gpu/drm/i915/intel_engine_cs.c     | 10 ++++++++++
>  drivers/gpu/drm/i915/intel_lrc.c           | 15 +++++----------
>  drivers/gpu/drm/i915/intel_lrc.h           |  1 -
>  drivers/gpu/drm/i915/intel_ringbuffer.c    | 15 +++++++++++++--
>  drivers/gpu/drm/i915/intel_ringbuffer.h    |  4 ++++
>  6 files changed, 33 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index b4d24cd7639a..119b5c073833 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -1051,7 +1051,7 @@ void i915_guc_submission_disable(struct drm_i915_private *dev_priv)
>  		return;
>
>  	/* Revert back to manual ELSP submission */
> -	intel_execlists_enable_submission(dev_priv);
> +	intel_engines_enable_submission(dev_priv);

intel_engines_default_submission came to my mind but that will also be 
misleading once the guc switch is toggled. But I think less misleading 
than enable_submission. And vfunc name maybe as assign_default_submission?

>  }
>
>  void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index 73fe718516a5..5663ebab851f 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -191,6 +191,7 @@ int intel_engines_init(struct drm_i915_private *dev_priv)
>  			goto cleanup;
>  		}
>
> +		engine->enable_submission(engine);

Could be moved to intel_engines_setup_common if the logical_ring_setup 
was re-arranged a bit so that the default_vfuncs are assigned before it. 
Legacy looks like it would be alright with that approach already.

My thinking here is not to expose this vfunc so prominently in the code 
since it is a bit of a low level internal implementation thing.

Thoughts?

Regards,

Tvrtko


>  		mask |= ENGINE_MASK(id);
>  	}
>
> @@ -1115,6 +1116,15 @@ bool intel_engines_are_idle(struct drm_i915_private *dev_priv)
>  	return true;
>  }
>
> +void intel_engines_enable_submission(struct drm_i915_private *i915)
> +{
> +	struct intel_engine_cs *engine;
> +	enum intel_engine_id id;
> +
> +	for_each_engine(engine, i915, id)
> +		engine->enable_submission(engine);
> +}
> +
>  #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
>  #include "selftests/mock_engine.c"
>  #endif
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 89f38e7def9f..f79df7a51e60 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1560,15 +1560,10 @@ void intel_logical_ring_cleanup(struct intel_engine_cs *engine)
>  	kfree(engine);
>  }
>
> -void intel_execlists_enable_submission(struct drm_i915_private *dev_priv)
> +static void logical_ring_enable_submission(struct intel_engine_cs *engine)
>  {
> -	struct intel_engine_cs *engine;
> -	enum intel_engine_id id;
> -
> -	for_each_engine(engine, dev_priv, id) {
> -		engine->submit_request = execlists_submit_request;
> -		engine->schedule = execlists_schedule;
> -	}
> +	engine->submit_request = execlists_submit_request;
> +	engine->schedule = execlists_schedule;
>  }
>
>  static void
> @@ -1586,8 +1581,8 @@ logical_ring_default_vfuncs(struct intel_engine_cs *engine)
>  	engine->emit_flush = gen8_emit_flush;
>  	engine->emit_breadcrumb = gen8_emit_breadcrumb;
>  	engine->emit_breadcrumb_sz = gen8_emit_breadcrumb_sz;
> -	engine->submit_request = execlists_submit_request;
> -	engine->schedule = execlists_schedule;
> +
> +	engine->enable_submission = logical_ring_enable_submission;
>
>  	engine->irq_enable = gen8_logical_ring_enable_irq;
>  	engine->irq_disable = gen8_logical_ring_disable_irq;
> diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
> index 5fc07761caff..e8015e7bf4e9 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.h
> +++ b/drivers/gpu/drm/i915/intel_lrc.h
> @@ -87,6 +87,5 @@ uint64_t intel_lr_context_descriptor(struct i915_gem_context *ctx,
>  /* Execlists */
>  int intel_sanitize_enable_execlists(struct drm_i915_private *dev_priv,
>  				    int enable_execlists);
> -void intel_execlists_enable_submission(struct drm_i915_private *dev_priv);
>
>  #endif /* _INTEL_LRC_H_ */
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 4a864f8c9387..5b141f6639b6 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -2050,6 +2050,16 @@ static void intel_ring_init_irq(struct drm_i915_private *dev_priv,
>  	}
>  }
>
> +static void i9xx_enable_submission(struct intel_engine_cs *engine)
> +{
> +	engine->submit_request = i9xx_submit_request;
> +}
> +
> +static void gen6_bsd_enable_submission(struct intel_engine_cs *engine)
> +{
> +	engine->submit_request = gen6_bsd_submit_request;
> +}
> +
>  static void intel_ring_default_vfuncs(struct drm_i915_private *dev_priv,
>  				      struct intel_engine_cs *engine)
>  {
> @@ -2080,7 +2090,8 @@ static void intel_ring_default_vfuncs(struct drm_i915_private *dev_priv,
>  				engine->emit_breadcrumb_sz++;
>  		}
>  	}
> -	engine->submit_request = i9xx_submit_request;
> +
> +	engine->enable_submission = i9xx_enable_submission;
>
>  	if (INTEL_GEN(dev_priv) >= 8)
>  		engine->emit_bb_start = gen8_emit_bb_start;
> @@ -2165,7 +2176,7 @@ int intel_init_bsd_ring_buffer(struct intel_engine_cs *engine)
>  	if (INTEL_GEN(dev_priv) >= 6) {
>  		/* gen6 bsd needs a special wa for tail updates */
>  		if (IS_GEN6(dev_priv))
> -			engine->submit_request = gen6_bsd_submit_request;
> +			engine->enable_submission = gen6_bsd_enable_submission;
>  		engine->emit_flush = gen6_bsd_ring_flush;
>  		if (INTEL_GEN(dev_priv) < 8)
>  			engine->irq_enable_mask = GT_BSD_USER_INTERRUPT;
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 0ef491df5b4e..30d9820d978c 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -273,6 +273,8 @@ struct intel_engine_cs {
>  	void		(*reset_hw)(struct intel_engine_cs *engine,
>  				    struct drm_i915_gem_request *req);
>
> +	void		(*enable_submission)(struct intel_engine_cs *engine);
> +
>  	int		(*context_pin)(struct intel_engine_cs *engine,
>  				       struct i915_gem_context *ctx);
>  	void		(*context_unpin)(struct intel_engine_cs *engine,
> @@ -669,4 +671,6 @@ static inline u32 *gen8_emit_pipe_control(u32 *batch, u32 flags, u32 offset)
>  bool intel_engine_is_idle(struct intel_engine_cs *engine);
>  bool intel_engines_are_idle(struct drm_i915_private *dev_priv);
>
> +void intel_engines_enable_submission(struct drm_i915_private *i915);
> +
>  #endif /* _INTEL_RINGBUFFER_H_ */
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx