Re: [Intel-gfx] [PATCH v9 2/2] drm/i915: Implement WaProgramMgsrForL3BankSpecificMmioReads

2018-04-18 Thread Oscar Mateo 



On 4/18/2018 9:40 AM, Oscar Mateo wrote:



On 4/17/2018 3:59 PM, Yunwei Zhang wrote:

L3Bank could be fused off in hardware for debug purpose, and it
is possible that subslice is enabled while its corresponding L3Bank 
pairs

are disabled. In such case, if MCR packet control register(0xFDC) is
programed to point to a disabled bank pair, a MMIO read into L3Bank 
range

will return 0 instead of correct values.

However, this is not going to be the case in any production silicon.
Therefore, we only check at initialization and issue a warning should
this really happen.

References: HSDES#1405586840

v2:
  - use fls instead of find_last_bit (Chris)
  - use is_power_of_2() instead of counting bit set (Chris)
v3:
  - rebase on latest tip
v5:
  - Added references (Mika)
  - Move local variable into scope where they are used (Ursulin)
  - use a new local variable to reduce long line of code (Ursulin)
v6:
  - Some coding style and use more local variables for clearer
    logic (Ursulin)
v7:
  - Rebased.
v8:
  - Reviewed by Oscar.
v9:
  - Fixed label location. (Oscar)

Cc: Oscar Mateo 
Cc: Michel Thierry 
Cc: Joonas Lahtinen 
Cc: Chris Wilson 
Cc: Mika Kuoppala 
Cc: Tvrtko Ursulin 
Signed-off-by: Yunwei Zhang 
Reviewed-by: Oscar Mateo 
---
  drivers/gpu/drm/i915/i915_reg.h  |  4 
  drivers/gpu/drm/i915/intel_device_info.c | 23 +++
  2 files changed, 27 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h 
b/drivers/gpu/drm/i915/i915_reg.h

index fb10602..6c9c01b 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -2709,6 +2709,10 @@ enum i915_power_well_id {
  #define   GEN10_F2_SS_DIS_SHIFT    18
  #define   GEN10_F2_SS_DIS_MASK    (0xf << GEN10_F2_SS_DIS_SHIFT)
  +#define    GEN10_MIRROR_FUSE3    _MMIO(0x9118)
+#define GEN10_L3BANK_PAIR_COUNT 4
+#define GEN10_L3BANK_MASK   0x0F
+
  #define GEN8_EU_DISABLE0    _MMIO(0x9134)
  #define   GEN8_EU_DIS0_S0_MASK    0xff
  #define   GEN8_EU_DIS0_S1_SHIFT    24
diff --git a/drivers/gpu/drm/i915/intel_device_info.c 
b/drivers/gpu/drm/i915/intel_device_info.c

index 1a4288f..530b6ba 100644
--- a/drivers/gpu/drm/i915/intel_device_info.c
+++ b/drivers/gpu/drm/i915/intel_device_info.c
@@ -729,6 +729,29 @@ static void sanitize_mcr(struct 
intel_device_info *info)

  u32 slice = fls(info->sseu.slice_mask);
  u32 subslice = fls(info->sseu.subslice_mask[slice]);
  +    /*
+ * WaProgramMgsrForL3BankSpecificMmioReads: cnl,icl
+ * L3Banks could be fused off in single slice scenario, however, if
+ * more than one slice is enabled, this should not happen.
+ */


Maybe a better explanation is warranted:

/*
 * WaProgramMgsrForL3BankSpecificMmioReads: cnl,icl
 * L3Banks could be fused off in single slice scenario. If that is the 
case,

 * we might need to program MCR select to a valid L3Bank by default,
 * to make sure we correctly read certain registers later on
* (in the range 0xB100 - 0xB3FF).
 * This might be incompatible with 
WaProgramMgsrForCorrectSliceSpecificMmioReads.

 * Fortunately, this should not happen in production hardware, so we only
 * assert that this is the case (instead of implementing something more 
complex

 * that requires checking the range of every MMIO read).
 */


+    if (is_power_of_2(info->sseu.slice_mask)) {


This WA is only required for GEN >= 10. In other GENs, 
GEN10_MIRROR_FUSE3 does not even exist!



+    /*
+ * read FUSE3 for enabled L3 Bank IDs, if L3 Bank matches
+ * enabled subslice, no need to redirect MCR packet
+ */
+    u32 fuse3 = I915_READ(GEN10_MIRROR_FUSE3);
+    u8 ss_mask = info->sseu.subslice_mask[slice];
+
+    u8 enabled_mask = (ss_mask | ss_mask >> 4) & 0xf;
+    u8 disabled_mask = fuse3 & 0xf;
+


You defined GEN10_L3BANK_MASK. Might as well use it :)


+    /*
+ * Production silicon should have matched L3Bank and
+ * subslice enabled
+ */
+    WARN_ON((enabled_mask & disabled_mask) != enabled_mask);
+    }
+
  if (INTEL_GEN(dev_priv) >= 11) {
  mcr_slice_subslice_mask = GEN11_MCR_SLICE_MASK |
    GEN11_MCR_SUBSLICE_MASK;




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

Re: [Intel-gfx] [PATCH v9 2/2] drm/i915: Implement WaProgramMgsrForL3BankSpecificMmioReads

2018-04-18 Thread Oscar Mateo 



On 4/17/2018 3:59 PM, Yunwei Zhang wrote:

L3Bank could be fused off in hardware for debug purpose, and it
is possible that subslice is enabled while its corresponding L3Bank pairs
are disabled. In such case, if MCR packet control register(0xFDC) is
programed to point to a disabled bank pair, a MMIO read into L3Bank range
will return 0 instead of correct values.

However, this is not going to be the case in any production silicon.
Therefore, we only check at initialization and issue a warning should
this really happen.

References: HSDES#1405586840

v2:
  - use fls instead of find_last_bit (Chris)
  - use is_power_of_2() instead of counting bit set (Chris)
v3:
  - rebase on latest tip
v5:
  - Added references (Mika)
  - Move local variable into scope where they are used (Ursulin)
  - use a new local variable to reduce long line of code (Ursulin)
v6:
  - Some coding style and use more local variables for clearer
logic (Ursulin)
v7:
  - Rebased.
v8:
  - Reviewed by Oscar.
v9:
  - Fixed label location. (Oscar)

Cc: Oscar Mateo 
Cc: Michel Thierry 
Cc: Joonas Lahtinen 
Cc: Chris Wilson 
Cc: Mika Kuoppala 
Cc: Tvrtko Ursulin 
Signed-off-by: Yunwei Zhang 
Reviewed-by: Oscar Mateo 
---
  drivers/gpu/drm/i915/i915_reg.h  |  4 
  drivers/gpu/drm/i915/intel_device_info.c | 23 +++
  2 files changed, 27 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index fb10602..6c9c01b 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -2709,6 +2709,10 @@ enum i915_power_well_id {
  #define   GEN10_F2_SS_DIS_SHIFT   18
  #define   GEN10_F2_SS_DIS_MASK(0xf << GEN10_F2_SS_DIS_SHIFT)
  
+#define	GEN10_MIRROR_FUSE3		_MMIO(0x9118)

+#define GEN10_L3BANK_PAIR_COUNT 4
+#define GEN10_L3BANK_MASK   0x0F
+
  #define GEN8_EU_DISABLE0  _MMIO(0x9134)
  #define   GEN8_EU_DIS0_S0_MASK0xff
  #define   GEN8_EU_DIS0_S1_SHIFT   24
diff --git a/drivers/gpu/drm/i915/intel_device_info.c 
b/drivers/gpu/drm/i915/intel_device_info.c
index 1a4288f..530b6ba 100644
--- a/drivers/gpu/drm/i915/intel_device_info.c
+++ b/drivers/gpu/drm/i915/intel_device_info.c
@@ -729,6 +729,29 @@ static void sanitize_mcr(struct intel_device_info *info)
u32 slice = fls(info->sseu.slice_mask);
u32 subslice = fls(info->sseu.subslice_mask[slice]);
  
+	/*

+* WaProgramMgsrForL3BankSpecificMmioReads: cnl,icl
+* L3Banks could be fused off in single slice scenario, however, if
+* more than one slice is enabled, this should not happen.
+*/
+   if (is_power_of_2(info->sseu.slice_mask)) {


This WA is only required for GEN >= 10. In other GENs, 
GEN10_MIRROR_FUSE3 does not even exist!



+   /*
+* read FUSE3 for enabled L3 Bank IDs, if L3 Bank matches
+* enabled subslice, no need to redirect MCR packet
+*/
+   u32 fuse3 = I915_READ(GEN10_MIRROR_FUSE3);
+   u8 ss_mask = info->sseu.subslice_mask[slice];
+
+   u8 enabled_mask = (ss_mask | ss_mask >> 4) & 0xf;
+   u8 disabled_mask = fuse3 & 0xf;
+


You defined GEN10_L3BANK_MASK. Might as well use it :)


+   /*
+* Production silicon should have matched L3Bank and
+* subslice enabled
+*/
+   WARN_ON((enabled_mask & disabled_mask) != enabled_mask);
+   }
+
if (INTEL_GEN(dev_priv) >= 11) {
mcr_slice_subslice_mask = GEN11_MCR_SLICE_MASK |
  GEN11_MCR_SUBSLICE_MASK;


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

[Intel-gfx] [PATCH v9 2/2] drm/i915: Implement WaProgramMgsrForL3BankSpecificMmioReads

2018-04-17 Thread Yunwei Zhang 
L3Bank could be fused off in hardware for debug purpose, and it
is possible that subslice is enabled while its corresponding L3Bank pairs
are disabled. In such case, if MCR packet control register(0xFDC) is
programed to point to a disabled bank pair, a MMIO read into L3Bank range
will return 0 instead of correct values.

However, this is not going to be the case in any production silicon.
Therefore, we only check at initialization and issue a warning should
this really happen.

References: HSDES#1405586840

v2:
 - use fls instead of find_last_bit (Chris)
 - use is_power_of_2() instead of counting bit set (Chris)
v3:
 - rebase on latest tip
v5:
 - Added references (Mika)
 - Move local variable into scope where they are used (Ursulin)
 - use a new local variable to reduce long line of code (Ursulin)
v6:
 - Some coding style and use more local variables for clearer
   logic (Ursulin)
v7:
 - Rebased.
v8:
 - Reviewed by Oscar.
v9:
 - Fixed label location. (Oscar)

Cc: Oscar Mateo 
Cc: Michel Thierry 
Cc: Joonas Lahtinen 
Cc: Chris Wilson 
Cc: Mika Kuoppala 
Cc: Tvrtko Ursulin 
Signed-off-by: Yunwei Zhang 
Reviewed-by: Oscar Mateo 
---
 drivers/gpu/drm/i915/i915_reg.h  |  4 
 drivers/gpu/drm/i915/intel_device_info.c | 23 +++
 2 files changed, 27 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index fb10602..6c9c01b 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -2709,6 +2709,10 @@ enum i915_power_well_id {
 #define   GEN10_F2_SS_DIS_SHIFT18
 #define   GEN10_F2_SS_DIS_MASK (0xf << GEN10_F2_SS_DIS_SHIFT)
 
+#defineGEN10_MIRROR_FUSE3  _MMIO(0x9118)
+#define GEN10_L3BANK_PAIR_COUNT 4
+#define GEN10_L3BANK_MASK   0x0F
+
 #define GEN8_EU_DISABLE0   _MMIO(0x9134)
 #define   GEN8_EU_DIS0_S0_MASK 0xff
 #define   GEN8_EU_DIS0_S1_SHIFT24
diff --git a/drivers/gpu/drm/i915/intel_device_info.c 
b/drivers/gpu/drm/i915/intel_device_info.c
index 1a4288f..530b6ba 100644
--- a/drivers/gpu/drm/i915/intel_device_info.c
+++ b/drivers/gpu/drm/i915/intel_device_info.c
@@ -729,6 +729,29 @@ static void sanitize_mcr(struct intel_device_info *info)
u32 slice = fls(info->sseu.slice_mask);
u32 subslice = fls(info->sseu.subslice_mask[slice]);
 
+   /*
+* WaProgramMgsrForL3BankSpecificMmioReads: cnl,icl
+* L3Banks could be fused off in single slice scenario, however, if
+* more than one slice is enabled, this should not happen.
+*/
+   if (is_power_of_2(info->sseu.slice_mask)) {
+   /*
+* read FUSE3 for enabled L3 Bank IDs, if L3 Bank matches
+* enabled subslice, no need to redirect MCR packet
+*/
+   u32 fuse3 = I915_READ(GEN10_MIRROR_FUSE3);
+   u8 ss_mask = info->sseu.subslice_mask[slice];
+
+   u8 enabled_mask = (ss_mask | ss_mask >> 4) & 0xf;
+   u8 disabled_mask = fuse3 & 0xf;
+
+   /*
+* Production silicon should have matched L3Bank and
+* subslice enabled
+*/
+   WARN_ON((enabled_mask & disabled_mask) != enabled_mask);
+   }
+
if (INTEL_GEN(dev_priv) >= 11) {
mcr_slice_subslice_mask = GEN11_MCR_SLICE_MASK |
  GEN11_MCR_SUBSLICE_MASK;
-- 
2.7.4

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