[Intel-gfx] [PATCH 5/8] drm/i915/icl: Add reset control register changes

[Chris Wilson]

Quoting Daniele Ceraolo Spurio (2018-03-16 20:28:25)
> 
> 
> On 16/03/18 05:14, Mika Kuoppala wrote:
> > From: Michel Thierry <michel.thierry at intel.com>
> > 
> > The bits used to reset the different engines/domains have changed in
> > GEN11, this patch maps the reset engine mask bits with the new bits
> > in the reset control register.
> > 
> > v2: Use shift-left instead of BIT macro to match the file style (Paulo).
> > v3: Reuse gen8_reset_engines (Daniele).
> > v4: Do not call intel_uncore_forcewake_reset after reset, we may be
> > using the forcewake to read protected registers elsewhere and those
> > results may be clobbered by the concurrent dropping of forcewake.
> > 
> > bspec: 19212
> > Cc: Oscar Mateo <oscar.mateo at intel.com>
> > Cc: Antonio Argenziano <antonio.argenziano at intel.com>
> > Cc: Paulo Zanoni <paulo.r.zanoni at intel.com>
> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> > Acked-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> > Signed-off-by: Michel Thierry <michel.thierry at intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_reg.h     | 11 ++++++++
> >   drivers/gpu/drm/i915/intel_uncore.c | 53 +++++++++++++++++++++++++++++++++++--
> >   2 files changed, 62 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 9eaaa96287ec..f3cc77690124 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -301,6 +301,17 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
> >   #define  GEN6_GRDOM_VECS            (1 << 4)
> >   #define  GEN9_GRDOM_GUC                     (1 << 5)
> >   #define  GEN8_GRDOM_MEDIA2          (1 << 7)
> > +/* GEN11 changed all bit defs except for FULL & RENDER */
> > +#define  GEN11_GRDOM_FULL            GEN6_GRDOM_FULL
> > +#define  GEN11_GRDOM_RENDER          GEN6_GRDOM_RENDER
> > +#define  GEN11_GRDOM_BLT             (1 << 2)
> > +#define  GEN11_GRDOM_GUC             (1 << 3)
> > +#define  GEN11_GRDOM_MEDIA           (1 << 5)
> > +#define  GEN11_GRDOM_MEDIA2          (1 << 6)
> > +#define  GEN11_GRDOM_MEDIA3          (1 << 7)
> > +#define  GEN11_GRDOM_MEDIA4          (1 << 8)
> > +#define  GEN11_GRDOM_VECS            (1 << 13)
> > +#define  GEN11_GRDOM_VECS2           (1 << 14)
> >   
> >   #define RING_PP_DIR_BASE(engine)    _MMIO((engine)->mmio_base+0x228)
> >   #define RING_PP_DIR_BASE_READ(engine)       _MMIO((engine)->mmio_base+0x518)
> > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> > index 4c616d074a97..cabbf0e682e7 100644
> > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > @@ -1909,6 +1909,50 @@ static int gen6_reset_engines(struct drm_i915_private *dev_priv,
> >       return gen6_hw_domain_reset(dev_priv, hw_mask);
> >   }
> >   
> > +/**
> > + * gen11_reset_engines - reset individual engines
> > + * @dev_priv: i915 device
> > + * @engine_mask: mask of intel_ring_flag() engines or ALL_ENGINES for full reset
> > + *
> > + * This function will reset the individual engines that are set in engine_mask.
> > + * If you provide ALL_ENGINES as mask, full global domain reset will be issued.
> > + *
> > + * Note: It is responsibility of the caller to handle the difference between
> > + * asking full domain reset versus reset for all available individual engines.
> > + *
> > + * Returns 0 on success, nonzero on error.
> > + */
> > +static int gen11_reset_engines(struct drm_i915_private *dev_priv,
> > +                            unsigned engine_mask)
> > +{
> > +     struct intel_engine_cs *engine;
> > +     const u32 hw_engine_mask[I915_NUM_ENGINES] = {
> > +             [RCS] = GEN11_GRDOM_RENDER,
> > +             [BCS] = GEN11_GRDOM_BLT,
> > +             [VCS] = GEN11_GRDOM_MEDIA,
> > +             [VCS2] = GEN11_GRDOM_MEDIA2,
> > +             [VCS3] = GEN11_GRDOM_MEDIA3,
> > +             [VCS4] = GEN11_GRDOM_MEDIA4,
> > +             [VECS] = GEN11_GRDOM_VECS,
> > +             [VECS2] = GEN11_GRDOM_VECS2,
> > +     };
> 
> Just a thought, but since this function is a copy of gen6_reset_engines 
> with the only difference being the array (GEN11_GRDOM_FULL is also the 
> same as GEN6_GRDOM_FULL), would it make sense to just add the array to 
> the gen6 function? e.g.:
> 
>         const u32 gen6_hw_engine_mask[] = {
>         ....
>         }
>         const u32 gen11_hw_engine_mask[] = {
>         ....
>         }
> 
>         const u32 *hw_engine_mask = INTEL_GEN(dev_priv) >= 11 ?
>                 gen11_hw_engine_mask : gen6_hw_engine_mask;
> 

Oh, and we are definitely in the territory where static const should
result in a smaller binary (.text + .data).
-Chris