Re: [PATCH 5/8] drm/i915/icl: Add reset control register changes

Michel Thierry <michel.thierry@xxxxxxxxx> · Tue, 27 Mar 2018 09:26:07 -0700

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@xxxxxxxxx>

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@xxxxxxxxx>
Cc: Antonio Argenziano <antonio.argenziano@xxxxxxxxx>
Cc: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
Acked-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
Signed-off-by: Michel Thierry <michel.thierry@xxxxxxxxx>
---
  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;

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







