[Intel-gfx] [PATCH] drm/i915/guc: Decouple GuC engine id from ring id

[Chris Wilson]

On Fri, Jan 22, 2016 at 03:06:28PM -0800, yu.dai at intel.com wrote:
> From: Alex Dai <yu.dai at intel.com>
> 
> Previously GuC uses ring id as engine id because of same definition.
> But this is not true since this commit:
> 
> commit de1add360522c876c25ef2bbbbab1c94bdb509ab
> Author: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> Date:   Fri Jan 15 15:12:50 2016 +0000
> 
>     drm/i915: Decouple execbuf uAPI from internal implementation
> 
> Added GuC engine id into GuC interface to decouple it from ring id used
> by driver.
> 
> Signed-off-by: Alex Dai <yu.dai at intel.com>
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index c5db235..9a4e01e 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2446,7 +2446,6 @@ static void i915_guc_client_info(struct seq_file *m,
>  				 struct drm_i915_private *dev_priv,
>  				 struct i915_guc_client *client)
>  {
> -	struct intel_engine_cs *ring;
>  	uint64_t tot = 0;
>  	uint32_t i;
>  
> @@ -2461,10 +2460,9 @@ static void i915_guc_client_info(struct seq_file *m,
>  	seq_printf(m, "\tFailed doorbell: %u\n", client->b_fail);
>  	seq_printf(m, "\tLast submission result: %d\n", client->retcode);
>  
> -	for_each_ring(ring, dev_priv, i) {
> -		seq_printf(m, "\tSubmissions: %llu %s\n",
> -				client->submissions[i],
> -				ring->name);
> +	for (i = GUC_RENDER_ENGINE; i < GUC_MAX_ENGINES_NUM; i++) {
> +		seq_printf(m, "\tSubmissions: %llu, engine %d\n",
> +				client->submissions[i], i);
>  		tot += client->submissions[i];
>  	}
>  	seq_printf(m, "\tTotal: %llu\n", tot);
> @@ -2477,7 +2475,6 @@ static int i915_guc_info(struct seq_file *m, void *data)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_guc guc;
>  	struct i915_guc_client client = {};
> -	struct intel_engine_cs *ring;
>  	enum intel_ring_id i;
>  	u64 total = 0;
>  
> @@ -2501,9 +2498,9 @@ static int i915_guc_info(struct seq_file *m, void *data)
>  	seq_printf(m, "GuC last action error code: %d\n", guc.action_err);
>  
>  	seq_printf(m, "\nGuC submissions:\n");
> -	for_each_ring(ring, dev_priv, i) {
> -		seq_printf(m, "\t%-24s: %10llu, last seqno 0x%08x %9d\n",
> -			ring->name, guc.submissions[i],
> +	for (i = GUC_RENDER_ENGINE; i < GUC_MAX_ENGINES_NUM; i++) {
> +		seq_printf(m, "\tengine %d: %10llu, last seqno 0x%08x %9d\n",
> +			i, guc.submissions[i],
>  			guc.last_seqno[i], guc.last_seqno[i]);
>  		total += guc.submissions[i];

For debugfs, would it not be more convenient to use the ring->name and
show the corresponding guc engine id?

> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 51ae5c1..601e2c8 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -365,6 +365,14 @@ static void guc_init_proc_desc(struct intel_guc *guc,
>  	kunmap_atomic(base);
>  }
>  
> +static const enum intel_ring_id guc_engine_map[GUC_MAX_ENGINES_NUM] = {
> +	[GUC_RENDER_ENGINE] = RCS,
> +	[GUC_VIDEO_ENGINE] = VCS,
> +	[GUC_BLITTER_ENGINE] = BCS,
> +	[GUC_VIDEOENHANCE_ENGINE] = VECS,
> +	[GUC_VIDEO_ENGINE2] = VCS2
> +};
> +
>  /*
>   * Initialise/clear the context descriptor shared with the GuC firmware.
>   *
> @@ -388,9 +396,10 @@ static void guc_init_ctx_desc(struct intel_guc *guc,
>  	desc.priority = client->priority;
>  	desc.db_id = client->doorbell_id;
>  
> -	for (i = 0; i < I915_NUM_RINGS; i++) {
> +	for (i = GUC_RENDER_ENGINE; i < GUC_MAX_ENGINES_NUM; i++) {
>  		struct guc_execlist_context *lrc = &desc.lrc[i];

Again, would it not be more consistent to iterate over engines
(for_each_ring) and use struct guc_execlist_context *lrc =
&desc.lrc[ring->guc_id] ?

> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index b12f2aa..b69eadb 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -158,6 +158,7 @@ struct  intel_engine_cs {
>  #define I915_NUM_RINGS 5
>  #define _VCS(n) (VCS + (n))
>  	unsigned int exec_id;
> +	unsigned int guc_engine_id;

Is the tautology useful? It is an engine, so intel_engine_cs->guc_id
would mean the correspondance map between our engines and the guc's.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre