| Message ID | 1522188879-10779-1-git-send-email-yunwei.zhang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 27/03/2018 23:14, 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) > > Cc: Oscar Mateo <oscar.mateo@intel.com> > Cc: Michel Thierry <michel.thierry@intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > Signed-off-by: Yunwei Zhang <yunwei.zhang@intel.com> > --- > drivers/gpu/drm/i915/i915_reg.h | 4 ++++ > drivers/gpu/drm/i915/intel_engine_cs.c | 20 ++++++++++++++++++++ > 2 files changed, 24 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 1bca695f..4f2f5e1 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -2691,6 +2691,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 0xffffff > #define GEN8_EU_DIS0_S1_SHIFT 24 > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c > index 4c78d1e..7be7a75 100644 > --- a/drivers/gpu/drm/i915/intel_engine_cs.c > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c > @@ -811,6 +811,26 @@ static u32 calculate_mcr(struct drm_i915_private *dev_priv, u32 mcr) > static void wa_init_mcr(struct drm_i915_private *dev_priv) > { > u32 mcr; > + const struct sseu_dev_info *sseu = &(INTEL_INFO(dev_priv)->sseu); Another style nitpick - sorry I did not notice it before - we typically order assignments from functions arguments to locals first, then pure locals. Also we typically try to make the declaration block start wide and then narrow. > + > + /* If more than one slice are enabled, L3Banks should be all enabled */ L3Banks should be all enabled, or enabled for all enabled slices? (That comment below says "should have _matched_". > + if (is_power_of_2(sseu->slice_mask)) { > + /* > + * WaProgramMgsrForL3BankSpecificMmioReads: > + * read FUSE3 for enabled L3 Bank IDs, if L3 Bank matches > + * enabled subslice, no need to redirect MCR packet > + */ This comment implies there will be some action taken depending on this conditional relating to the MCR, but there is nothing there? It is not clear to me what should and whether perhaps this comment should be pulled up and merged with the one above the conditional. > + u32 slice = fls(sseu->slice_mask); > + u32 fuse3 = I915_READ(GEN10_MIRROR_FUSE3); > + u8 ss_mask = sseu->subslice_mask[slice]; Insert blank line after declarations. Also, is it correct to only consider the subslice mask of the last slice for this check? > + /* > + * Production silicon should have matched L3Bank and > + * subslice enabled > + */ > + WARN_ON(!((fuse3 & GEN10_L3BANK_MASK) & > + ((ss_mask | ss_mask >> GEN10_L3BANK_PAIR_COUNT) & > + GEN10_L3BANK_MASK))); Mask in fuse3 is the disabled mask right, since BSpec calls them "L3 Bank Disable Select"? Should you not be checking that none of the enabled slices have L3Bank disabled, while the above looks like it can miss a partial mismatch? Something like this: u8 enabled_mask = (ss_mask | ss_mask >> 4) & 0xf; u8 disabled_mask = fuse3 & 0xf; WARN_ON((enabled_mask & disabled_mask) != enabled_mask); > + } > > mcr = I915_READ(GEN8_MCR_SELECTOR); > mcr = calculate_mcr(dev_priv, mcr); > Regards, Tvrtko
On 28/03/2018 10:39, Tvrtko Ursulin wrote: > > On 27/03/2018 23:14, 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) >> >> Cc: Oscar Mateo <oscar.mateo@intel.com> >> Cc: Michel Thierry <michel.thierry@intel.com> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> >> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> >> Signed-off-by: Yunwei Zhang <yunwei.zhang@intel.com> >> --- >> drivers/gpu/drm/i915/i915_reg.h | 4 ++++ >> drivers/gpu/drm/i915/intel_engine_cs.c | 20 ++++++++++++++++++++ >> 2 files changed, 24 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/i915_reg.h >> b/drivers/gpu/drm/i915/i915_reg.h >> index 1bca695f..4f2f5e1 100644 >> --- a/drivers/gpu/drm/i915/i915_reg.h >> +++ b/drivers/gpu/drm/i915/i915_reg.h >> @@ -2691,6 +2691,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 0xffffff >> #define GEN8_EU_DIS0_S1_SHIFT 24 >> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c >> b/drivers/gpu/drm/i915/intel_engine_cs.c >> index 4c78d1e..7be7a75 100644 >> --- a/drivers/gpu/drm/i915/intel_engine_cs.c >> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c >> @@ -811,6 +811,26 @@ static u32 calculate_mcr(struct drm_i915_private >> *dev_priv, u32 mcr) >> static void wa_init_mcr(struct drm_i915_private *dev_priv) >> { >> u32 mcr; >> + const struct sseu_dev_info *sseu = &(INTEL_INFO(dev_priv)->sseu); > > Another style nitpick - sorry I did not notice it before - we typically > order assignments from functions arguments to locals first, then pure > locals. Also we typically try to make the declaration block start wide > and then narrow. > >> + >> + /* If more than one slice are enabled, L3Banks should be all >> enabled */ > > L3Banks should be all enabled, or enabled for all enabled slices? (That > comment below says "should have _matched_". > >> + if (is_power_of_2(sseu->slice_mask)) { >> + /* >> + * WaProgramMgsrForL3BankSpecificMmioReads: >> + * read FUSE3 for enabled L3 Bank IDs, if L3 Bank matches >> + * enabled subslice, no need to redirect MCR packet >> + */ > > This comment implies there will be some action taken depending on this > conditional relating to the MCR, but there is nothing there? > > It is not clear to me what should and whether perhaps this comment > should be pulled up and merged with the one above the conditional. > >> + u32 slice = fls(sseu->slice_mask); >> + u32 fuse3 = I915_READ(GEN10_MIRROR_FUSE3); >> + u8 ss_mask = sseu->subslice_mask[slice]; > > Insert blank line after declarations. > > Also, is it correct to only consider the subslice mask of the last slice > for this check? > >> + /* >> + * Production silicon should have matched L3Bank and >> + * subslice enabled >> + */ >> + WARN_ON(!((fuse3 & GEN10_L3BANK_MASK) & >> + ((ss_mask | ss_mask >> GEN10_L3BANK_PAIR_COUNT) & > >> + GEN10_L3BANK_MASK))); > > Mask in fuse3 is the disabled mask right, since BSpec calls them "L3 > Bank Disable Select"? > > Should you not be checking that none of the enabled slices have L3Bank > disabled, while the above looks like it can miss a partial mismatch? > Something like this: > > u8 enabled_mask = (ss_mask | ss_mask >> 4) & 0xf; > u8 disabled_mask = fuse3 & 0xf; > > WARN_ON((enabled_mask & disabled_mask) != enabled_mask); Oops no, that's wrong, should be "(enabled_mask & ~disabled_mask) != enabled_mask)" I think. Which is then the same as WARN_ON(enabled_mask & disabled_mask) - aka same bit is both enabled and disabled - so maybe I got the meaning of "L3 Bank Disable Select" wrong. Regards, Tvrtko > >> + } >> mcr = I915_READ(GEN8_MCR_SELECTOR); >> mcr = calculate_mcr(dev_priv, mcr); >> > > Regards, > > Tvrtko
On 3/28/2018 2:39 AM, Tvrtko Ursulin wrote: > > On 27/03/2018 23:14, 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) >> >> Cc: Oscar Mateo <oscar.mateo@intel.com> >> Cc: Michel Thierry <michel.thierry@intel.com> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> >> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> >> Signed-off-by: Yunwei Zhang <yunwei.zhang@intel.com> >> --- >> drivers/gpu/drm/i915/i915_reg.h | 4 ++++ >> drivers/gpu/drm/i915/intel_engine_cs.c | 20 ++++++++++++++++++++ >> 2 files changed, 24 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/i915_reg.h >> b/drivers/gpu/drm/i915/i915_reg.h >> index 1bca695f..4f2f5e1 100644 >> --- a/drivers/gpu/drm/i915/i915_reg.h >> +++ b/drivers/gpu/drm/i915/i915_reg.h >> @@ -2691,6 +2691,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 0xffffff >> #define GEN8_EU_DIS0_S1_SHIFT 24 >> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c >> b/drivers/gpu/drm/i915/intel_engine_cs.c >> index 4c78d1e..7be7a75 100644 >> --- a/drivers/gpu/drm/i915/intel_engine_cs.c >> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c >> @@ -811,6 +811,26 @@ static u32 calculate_mcr(struct drm_i915_private >> *dev_priv, u32 mcr) >> static void wa_init_mcr(struct drm_i915_private *dev_priv) >> { >> u32 mcr; >> + const struct sseu_dev_info *sseu = &(INTEL_INFO(dev_priv)->sseu); > > Another style nitpick - sorry I did not notice it before - we > typically order assignments from functions arguments to locals first, > then pure locals. Also we typically try to make the declaration block > start wide and then narrow. > >> + >> + /* If more than one slice are enabled, L3Banks should be all >> enabled */ > > L3Banks should be all enabled, or enabled for all enabled slices? > (That comment below says "should have _matched_". See comment below > >> + if (is_power_of_2(sseu->slice_mask)) { >> + /* >> + * WaProgramMgsrForL3BankSpecificMmioReads: >> + * read FUSE3 for enabled L3 Bank IDs, if L3 Bank matches >> + * enabled subslice, no need to redirect MCR packet >> + */ > > This comment implies there will be some action taken depending on this > conditional relating to the MCR, but there is nothing there? > > It is not clear to me what should and whether perhaps this comment > should be pulled up and merged with the one above the conditional. This WA(L3Bank but not the slice/subslice) is not meant to exist on production silicon, I am not sure in this case whether we should implement/upstream the WA. So we did this also to solicit suggestions. That is why in case of L3Banks somehow do get fused off, we issue a warning instead of programming MCR register. > >> + u32 slice = fls(sseu->slice_mask); >> + u32 fuse3 = I915_READ(GEN10_MIRROR_FUSE3); >> + u8 ss_mask = sseu->subslice_mask[slice]; > > Insert blank line after declarations. > > Also, is it correct to only consider the subslice mask of the last > slice for this check? The case only exists on 1 enabled slice scenario, if there are two or more slices enabled, no subslice will be fused off. > >> + /* >> + * Production silicon should have matched L3Bank and >> + * subslice enabled >> + */ >> + WARN_ON(!((fuse3 & GEN10_L3BANK_MASK) & >> + ((ss_mask | ss_mask >> GEN10_L3BANK_PAIR_COUNT) & > + >> GEN10_L3BANK_MASK))); > > Mask in fuse3 is the disabled mask right, since BSpec calls them "L3 > Bank Disable Select"? > > Should you not be checking that none of the enabled slices have L3Bank > disabled, while the above looks like it can miss a partial mismatch? > Something like this: > > u8 enabled_mask = (ss_mask | ss_mask >> 4) & 0xf; > u8 disabled_mask = fuse3 & 0xf; > > WARN_ON((enabled_mask & disabled_mask) != enabled_mask); > >> + } >> mcr = I915_READ(GEN8_MCR_SELECTOR); >> mcr = calculate_mcr(dev_priv, mcr); >> > > Regards, > > Tvrtko Thanks, Yunwei
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 1bca695f..4f2f5e1 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -2691,6 +2691,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 0xffffff #define GEN8_EU_DIS0_S1_SHIFT 24 diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c index 4c78d1e..7be7a75 100644 --- a/drivers/gpu/drm/i915/intel_engine_cs.c +++ b/drivers/gpu/drm/i915/intel_engine_cs.c @@ -811,6 +811,26 @@ static u32 calculate_mcr(struct drm_i915_private *dev_priv, u32 mcr) static void wa_init_mcr(struct drm_i915_private *dev_priv) { u32 mcr; + const struct sseu_dev_info *sseu = &(INTEL_INFO(dev_priv)->sseu); + + /* If more than one slice are enabled, L3Banks should be all enabled */ + if (is_power_of_2(sseu->slice_mask)) { + /* + * WaProgramMgsrForL3BankSpecificMmioReads: + * read FUSE3 for enabled L3 Bank IDs, if L3 Bank matches + * enabled subslice, no need to redirect MCR packet + */ + u32 slice = fls(sseu->slice_mask); + u32 fuse3 = I915_READ(GEN10_MIRROR_FUSE3); + u8 ss_mask = sseu->subslice_mask[slice]; + /* + * Production silicon should have matched L3Bank and + * subslice enabled + */ + WARN_ON(!((fuse3 & GEN10_L3BANK_MASK) & + ((ss_mask | ss_mask >> GEN10_L3BANK_PAIR_COUNT) & + GEN10_L3BANK_MASK))); + } mcr = I915_READ(GEN8_MCR_SELECTOR); mcr = calculate_mcr(dev_priv, mcr);
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) Cc: Oscar Mateo <oscar.mateo@intel.com> Cc: Michel Thierry <michel.thierry@intel.com> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> Signed-off-by: Yunwei Zhang <yunwei.zhang@intel.com> --- drivers/gpu/drm/i915/i915_reg.h | 4 ++++ drivers/gpu/drm/i915/intel_engine_cs.c | 20 ++++++++++++++++++++ 2 files changed, 24 insertions(+)