[Intel-gfx] [PATCH 5/8] drm/i915/icl: Add reset control register changes
Michel Thierry
michel.thierry at intel.com
Tue Mar 27 16:26:07 UTC 2018
On 3/16/2018 1:28 PM, Daniele Ceraolo Spurio wrote:
>
>
> 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.:
There are more changes for gen11 coming (locking/unlocking the shared
SFC units), so I don't think it's a good idea to combine them.
>
> 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;
>
>
> My Ack still stands regardless and I also agree with renaming the
> defines to be closer to the specs.
>
> Daniele
>
>> + u32 hw_mask;
>> +
>> + BUILD_BUG_ON(VECS2 + 1 != I915_NUM_ENGINES);
>> +
>> + if (engine_mask == ALL_ENGINES) {
>> + hw_mask = GEN11_GRDOM_FULL;
>> + } else {
>> + unsigned int tmp;
>> +
>> + hw_mask = 0;
>> + for_each_engine_masked(engine, dev_priv, engine_mask, tmp)
>> + hw_mask |= hw_engine_mask[engine->id];
>> + }
>> +
>> + return gen6_hw_domain_reset(dev_priv, hw_mask);
>> +}
>> +
>> /**
>> * __intel_wait_for_register_fw - wait until register matches
>> expected state
>> * @dev_priv: the i915 device
>> @@ -2056,7 +2100,10 @@ static int gen8_reset_engines(struct
>> drm_i915_private *dev_priv,
>> if (gen8_reset_engine_start(engine))
>> goto not_ready;
>> - return gen6_reset_engines(dev_priv, engine_mask);
>> + if (INTEL_GEN(dev_priv) >= 11)
>> + return gen11_reset_engines(dev_priv, engine_mask);
>> + else
>> + return gen6_reset_engines(dev_priv, engine_mask);
>> not_ready:
>> for_each_engine_masked(engine, dev_priv, engine_mask, tmp)
>> @@ -2141,12 +2188,14 @@ bool intel_has_reset_engine(struct
>> drm_i915_private *dev_priv)
>> int intel_reset_guc(struct drm_i915_private *dev_priv)
>> {
>> + u32 guc_domain = INTEL_GEN(dev_priv) >= 11 ? GEN11_GRDOM_GUC :
>> + GEN9_GRDOM_GUC;
>> int ret;
>> GEM_BUG_ON(!HAS_GUC(dev_priv));
>> intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
>> - ret = gen6_hw_domain_reset(dev_priv, GEN9_GRDOM_GUC);
>> + ret = gen6_hw_domain_reset(dev_priv, guc_domain);
>> intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
>> return ret;
>>
More information about the Intel-gfx
mailing list