[Intel-gfx] [PATCH 2/6] drm/i915/icl: Correctly initialize the Gen11 engines

[Daniele Ceraolo Spurio]

On 02/03/18 08:14, Mika Kuoppala wrote:
> From: Oscar Mateo <oscar.mateo at intel.com>
> 
> Gen11 has up to 4 VCS and up to 2 VECS engines, this patch adds mmio
> base definitions for all of them.
> 
> Bspec: 20944
> Bspec: 7021
> 
> v2: Set the correct mmio_base in intel_engines_init_mmio; updating the
> base mmio values any later would cause incorrect reads in
> i915_gem_sanitize (Michel).
> 
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> Cc: Ceraolo Spurio, Daniele <daniele.ceraolospurio at intel.com>
> Signed-off-by: Oscar Mateo <oscar.mateo at intel.com>
> Signed-off-by: Michel Thierry <michel.thierry at intel.com>
> ---
>   drivers/gpu/drm/i915/i915_reg.h        |  6 +++++
>   drivers/gpu/drm/i915/intel_engine_cs.c | 44 +++++++++++++++++++++++++++++++++-
>   2 files changed, 49 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 258e86eb37d5..45ae05d0fe78 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -2345,7 +2345,13 @@ enum i915_power_well_id {
>   #define BSD_RING_BASE		0x04000
>   #define GEN6_BSD_RING_BASE	0x12000
>   #define GEN8_BSD2_RING_BASE	0x1c000
> +#define GEN11_BSD_RING_BASE	0x1c0000
> +#define GEN11_BSD2_RING_BASE	0x1c4000
> +#define GEN11_BSD3_RING_BASE	0x1d0000
> +#define GEN11_BSD4_RING_BASE	0x1d4000
>   #define VEBOX_RING_BASE		0x1a000
> +#define GEN11_VEBOX_RING_BASE		0x1c8000
> +#define GEN11_VEBOX2_RING_BASE		0x1d8000
>   #define BLT_RING_BASE		0x22000
>   #define RING_TAIL(base)		_MMIO((base)+0x30)
>   #define RING_HEAD(base)		_MMIO((base)+0x34)
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index 3e1107ecb6ee..911fc08658c5 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -123,6 +123,22 @@ static const struct engine_info intel_engines[] = {
>   		.mmio_base = GEN8_BSD2_RING_BASE,
>   		.irq_shift = GEN8_VCS2_IRQ_SHIFT,
>   	},
> +	[VCS3] = {
> +		.hw_id = VCS3_HW,
> +		.uabi_id = I915_EXEC_BSD,
> +		.class = VIDEO_DECODE_CLASS,
> +		.instance = 2,
> +		.mmio_base = GEN11_BSD3_RING_BASE,
> +		.irq_shift = 0, /* not used */
> +	},
> +	[VCS4] = {
> +		.hw_id = VCS4_HW,
> +		.uabi_id = I915_EXEC_BSD,
> +		.class = VIDEO_DECODE_CLASS,
> +		.instance = 3,
> +		.mmio_base = GEN11_BSD4_RING_BASE,
> +		.irq_shift = 0, /* not used */
> +	},
>   	[VECS] = {
>   		.hw_id = VECS_HW,
>   		.uabi_id = I915_EXEC_VEBOX,
> @@ -131,6 +147,14 @@ static const struct engine_info intel_engines[] = {
>   		.mmio_base = VEBOX_RING_BASE,
>   		.irq_shift = GEN8_VECS_IRQ_SHIFT,
>   	},
> +	[VECS2] = {
> +		.hw_id = VECS2_HW,
> +		.uabi_id = I915_EXEC_VEBOX,
> +		.class = VIDEO_ENHANCEMENT_CLASS,
> +		.instance = 1,
> +		.mmio_base = GEN11_VEBOX2_RING_BASE,
> +		.irq_shift = 0, /* not used */
> +	},
>   };
>   
>   /**
> @@ -230,7 +254,25 @@ intel_engine_setup(struct drm_i915_private *dev_priv,
>   			 class_info->name, info->instance) >=
>   		sizeof(engine->name));
>   	engine->hw_id = engine->guc_id = info->hw_id;
> -	engine->mmio_base = info->mmio_base;
> +	if (INTEL_GEN(dev_priv) >= 11) {
> +		switch (engine->id) {
> +		case VCS:
> +			engine->mmio_base = GEN11_BSD_RING_BASE;
> +			break;
> +		case VCS2:
> +			engine->mmio_base = GEN11_BSD2_RING_BASE;
> +			break;
> +		case VECS:
> +			engine->mmio_base = GEN11_VEBOX_RING_BASE;
> +			break;
> +		default:
> +			/* take the original value for all other engines  */
> +			engine->mmio_base = info->mmio_base;
> +			break;
> +		}

I'm slightly unconvinced by the fact that we have a static table with 
some values and then we use other values here. Maybe we could instead 
use and array of [starting_gen, mmio_base] pairs in the table and derive 
the correct value here? Anyway, since this is not the only place where 
we don't use the mmio_base value from the table (same happens for 
pre-gen6 BSD in intel_init_bsd_ring_buffer) I don't consider this a 
blocking issue. All the values match the specs, so:

Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>

I'll probably check how using the table I mentioned above looks like 
after this is merged and I'll send an RFC if it seems nice.

Thanks,
Daniele

> +	} else {
> +		engine->mmio_base = info->mmio_base;
> +	}
>   	engine->irq_shift = info->irq_shift;
>   	engine->class = info->class;
>   	engine->instance = info->instance;
>