On 3/27/2018 4:13 PM, Chris Wilson wrote:
Quoting Zhang, Yunwei (2018-03-27 23:49:27)

On 3/27/2018 3:27 PM, Chris Wilson wrote:
Quoting Yunwei Zhang (2018-03-27 23:14:16)
WaProgramMgsrForCorrectSliceSpecificMmioReads dictate that before any MMIO
read into Slice/Subslice specific registers, MCR packet control
register(0xFDC) needs to be programmed to point to any enabled
slice/subslice pair. Otherwise, incorrect value will be returned.

However, that means each subsequent MMIO read will be forwarded to a
specific slice/subslice combination as read is unicast. This is OK since
slice/subslice specific register values are consistent in almost all cases
across slice/subslice. There are rare occasions such as INSTDONE that this
value will be dependent on slice/subslice combo, in such cases, we need to
program 0xFDC and recover this after. This is already covered by
read_subslice_reg.

Also, 0xFDC will lose its information after TDR/engine reset/power state
change.

References: HSD#1405586840, BSID#0575

v2:
   - use fls() instead of find_last_bit() (Chris)
   - added INTEL_SSEU to extract sseu from device info. (Chris)
v3:
   - rebase on latest tip
v5:
   - Added references (Mika)
   - Change the ordered of passing arguments and etc. (Ursulin)

Cc: Oscar Mateo <oscar.ma...@intel.com>
Cc: Michel Thierry <michel.thie...@intel.com>
Cc: Joonas Lahtinen <joonas.lahti...@linux.intel.com>
Cc: Chris Wilson <ch...@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuopp...@linux.intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursu...@linux.intel.com>
Signed-off-by: Yunwei Zhang <yunwei.zh...@intel.com>
---
   drivers/gpu/drm/i915/intel_engine_cs.c | 39 
++++++++++++++++++++++++++++++++--
   1 file changed, 37 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c 
b/drivers/gpu/drm/i915/intel_engine_cs.c
index de09fa4..4c78d1e 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -796,6 +796,27 @@ const char *i915_cache_level_str(struct drm_i915_private 
*i915, int type)
          }
   }
+static u32 calculate_mcr(struct drm_i915_private *dev_priv, u32 mcr)
+{
+       const struct sseu_dev_info *sseu = &(INTEL_INFO(dev_priv)->sseu);
+       u32 slice = fls(sseu->slice_mask);
+       u32 subslice = fls(sseu->subslice_mask[slice]);
+
+       mcr &= ~(GEN8_MCR_SLICE_MASK | GEN8_MCR_SUBSLICE_MASK);
+       mcr |= GEN8_MCR_SLICE(slice) | GEN8_MCR_SUBSLICE(subslice);
+
+       return mcr;
+}
+
+static void wa_init_mcr(struct drm_i915_private *dev_priv)
+{
+       u32 mcr;
+
+       mcr = I915_READ(GEN8_MCR_SELECTOR);
+       mcr = calculate_mcr(dev_priv, mcr);
+       I915_WRITE(GEN8_MCR_SELECTOR, mcr);
+}
+
   static inline uint32_t
   read_subslice_reg(struct drm_i915_private *dev_priv, int slice,
                    int subslice, i915_reg_t reg)
@@ -828,18 +849,29 @@ read_subslice_reg(struct drm_i915_private *dev_priv, int 
slice,
          intel_uncore_forcewake_get__locked(dev_priv, fw_domains);
mcr = I915_READ_FW(GEN8_MCR_SELECTOR);
+
          /*
           * The HW expects the slice and sublice selectors to be reset to 0
           * after reading out the registers.
           */
-       WARN_ON_ONCE(mcr & mcr_slice_subslice_mask);
+       WARN_ON_ONCE(INTEL_GEN(dev_priv) < 10 &&
+                    (mcr & mcr_slice_subslice_mask));
          mcr &= ~mcr_slice_subslice_mask;
          mcr |= mcr_slice_subslice_select;
          I915_WRITE_FW(GEN8_MCR_SELECTOR, mcr);
ret = I915_READ_FW(reg); - mcr &= ~mcr_slice_subslice_mask;
+       /*
+        * WaProgramMgsrForCorrectSliceSpecificMmioReads:cnl
+        * expects mcr to be programed to a enabled slice/subslice pair
+        * before any MMIO read into slice/subslice register
+        */
So the read was above, where we did set the subslice_select
appropriately. Here we are resetting back to 0 *after* the read, as the
comment before indicates.

So what are you trying to accomplish with this patch? Other than leaving
the code in conflict with itself.
-Chris
Hi Chris,

The comment mentioned 0xFDC needs to be reset to 0 was before this WA
was introduced, in later HW, this WA requires 0xFDC to be programmed to
a enabled slice/subslice.

What this patch does it to initialize 0xFDC once at the initialization
(also it will be called after engine reset/TDR/coming out of c6) and
make sure every time it is changed, it will be reprogrammed to a enabled
slice/subslice so that a MMIO
read will get the correct value. read_subslice_reg changes the 0xFDC
value and if it is set to 0, it will cause MMIO read to return invalid
value for s/ss specific registers.
What mmio read? The only accessor should be this function.

And still the two comments are in direct conflict with each other.
-Chris
This function is only used in INST_DONE case which you need to iterate through each slice/subslice to check and makes sense to program MCR for each s/ss combination. But there could be inadvertent read into this range without using this function, the value would be wrong without this WA.

Anyway, the comment is not right given the latest WA required, I will submit a new patchset to update this comment.

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

Reply via email to