| Message ID | 1524499966-24871-1-git-send-email-yunwei.zhang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Apr 23, 2018 at 09:12:46AM -0700, 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) > v10: > - Improved comments and replaced magical number. (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> I confess that I got lost on this thread, so please accept my apologies in advance if I'm missing something here. But I don't know anymore: - if this series has 2 or 3 patches. - which of the patches rv-b by Oscar are still valid - if they are passing cleaning on CI. So, my suggestion is to start a new series from scratch. (resend all without any in-reply-to) But please double check with Oscar if his rv-b should stay on the new series. Thanks, Rodrigo. > --- > drivers/gpu/drm/i915/i915_reg.h | 4 ++++ > drivers/gpu/drm/i915/intel_device_info.c | 34 ++++++++++++++++++++++++++++++++ > 2 files changed, 38 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 d917c9b..44ca90a 100644 > --- a/drivers/gpu/drm/i915/intel_device_info.c > +++ b/drivers/gpu/drm/i915/intel_device_info.c > @@ -729,6 +729,40 @@ 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. 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 (INTEL_GEN(dev_priv) >= 10 && > + 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 >> > + GEN10_L3BANK_PAIR_COUNT) & > + GEN10_L3BANK_MASK; > + u8 disabled_mask = fuse3 & GEN10_L3BANK_MASK; > + > + /* > + * 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
Sorry, I added a debug patch when submitting to trybot and forgot to remove that from my local branch. I will resubmit to a new series. Yunwei On 4/23/2018 12:55 PM, Rodrigo Vivi wrote: > On Mon, Apr 23, 2018 at 09:12:46AM -0700, 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) >> v10: >> - Improved comments and replaced magical number. (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> > I confess that I got lost on this thread, so please > accept my apologies in advance if I'm missing something here. > > But I don't know anymore: > > - if this series has 2 or 3 patches. > - which of the patches rv-b by Oscar are still valid > - if they are passing cleaning on CI. > > So, my suggestion is to start a new series from scratch. > (resend all without any in-reply-to) > > But please double check with Oscar if his rv-b should stay > on the new series. > > Thanks, > Rodrigo. > > >> --- >> drivers/gpu/drm/i915/i915_reg.h | 4 ++++ >> drivers/gpu/drm/i915/intel_device_info.c | 34 ++++++++++++++++++++++++++++++++ >> 2 files changed, 38 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 d917c9b..44ca90a 100644 >> --- a/drivers/gpu/drm/i915/intel_device_info.c >> +++ b/drivers/gpu/drm/i915/intel_device_info.c >> @@ -729,6 +729,40 @@ 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. 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 (INTEL_GEN(dev_priv) >= 10 && >> + 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 >> >> + GEN10_L3BANK_PAIR_COUNT) & >> + GEN10_L3BANK_MASK; >> + u8 disabled_mask = fuse3 & GEN10_L3BANK_MASK; >> + >> + /* >> + * 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
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 d917c9b..44ca90a 100644 --- a/drivers/gpu/drm/i915/intel_device_info.c +++ b/drivers/gpu/drm/i915/intel_device_info.c @@ -729,6 +729,40 @@ 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. 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 (INTEL_GEN(dev_priv) >= 10 && + 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 >> + GEN10_L3BANK_PAIR_COUNT) & + GEN10_L3BANK_MASK; + u8 disabled_mask = fuse3 & GEN10_L3BANK_MASK; + + /* + * 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;