Re: [PATCH v5 1/2] drm/i915/cnl: Implement WaProgramMgsrForCorrectSliceSpecificMmioReads

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 3/28/2018 9:03 AM, Chris Wilson wrote:
Quoting Zhang, Yunwei (2018-03-28 16:54:26)

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.mateo@xxxxxxxxx>
Cc: Michel Thierry <michel.thierry@xxxxxxxxx>
Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Cc: Mika Kuoppala <mika.kuoppala@xxxxxxxxxxxxxxx>
Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>
Signed-off-by: Yunwei Zhang <yunwei.zhang@xxxxxxxxx>
---
    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.
Sure, but garbage in, garbage out. If we write an accessor for a
register because it requires a workaround, anyone who wants to access
the register should use the accessor. Not just leave HW in a random
state so that one particular selector works.
-Chris
But keep in mind this is not only *a* register, it is a range of registers, if we don't use this WA, developer needs to reminded if there is a MMIO read in slice/subslice/l3bank range, they will need to use read_subslice_reg instead of i915_READ.

To me, it is just safe to program MCR in the beginning and reprogram it every time we have to change like in INST_DONE case.

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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux