| Message ID | 1524005962-23062-1-git-send-email-yunwei.zhang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <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> > Reviewed-by: Oscar Mateo <oscar.mateo@intel.com> > --- > 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 0xffffff > #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;
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 <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> >> Reviewed-by: Oscar Mateo <oscar.mateo@intel.com> >> --- >> 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 0xffffff >> #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; >
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 0xffffff #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)) { + /* + * 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;