Re: [PATCH 2/6] drm/i915/icl: Correctly initialize the Gen11 engines

[Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>]

On 06/03/2018 22:07, Daniele Ceraolo Spurio wrote:
> On 02/03/18 08:14, Mika Kuoppala wrote:
> > From: Oscar Mateo <oscar.mateo@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > Cc: Ceraolo Spurio, Daniele <daniele.ceraolospurio@xxxxxxxxx>
> > Signed-off-by: Oscar Mateo <oscar.mateo@xxxxxxxxx>
> > Signed-off-by: Michel Thierry <michel.thierry@xxxxxxxxx>
> > ---
> >   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@xxxxxxxxx>
>
> 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.

Agreed it is inelegant to diverge. Your idea sounds potentially OK. Or 
even just duplicate the whole table for < gen6 and >= gen11 - they are 
not too big.

Regards,

Tvrtko

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx