Quoting Tvrtko Ursulin (2019-07-09 22:06:18)
> From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
>
> Two issues in this code:
>
> 1.
> fls() usage is incorrect causing off by one in subslice mask lookup,
> which in other words means subslice mask of all zeroes is always used
> (subslice mask of a slice which is not present, or even out of bounds
> array access), rendering the checks in wa_init_mcr either futile or
> random.
>
> 2.
> Condition in WARN_ON is not correct. It is doing a bitwise and operation
> between a positive (present subslices) and negative mask (disabled L3
> banks).
>
> This means that with corrected fls() usage the assert would always
> incorrectly fail.
>
> We can fix this by invereting the fuse bits in the check.
s/invereting/inverting/
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> Fixes: fe864b76c2ab ("drm/i915: Implement WaProgramMgsrForL3BankSpecificMmioReads")
> ---
> drivers/gpu/drm/i915/gt/intel_workarounds.c | 26 ++++++++++-----------
> 1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> index 9e069286d3ce..b5f19ad48d22 100644
> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> @@ -776,26 +776,26 @@ wa_init_mcr(struct drm_i915_private *i915, struct i915_wa_list *wal)
> * something more complex that requires checking the range of every
> * MMIO read).
> */
> - if (INTEL_GEN(i915) >= 10 &&
> - is_power_of_2(sseu->slice_mask)) {
> + if (INTEL_GEN(i915) >= 10 && is_power_of_2(sseu->slice_mask)) {
> /*
> - * read FUSE3 for enabled L3 Bank IDs, if L3 Bank matches
> - * enabled subslice, no need to redirect MCR packet
> + * 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 =
> - intel_uncore_read(&i915->uncore, GEN10_MIRROR_FUSE3);
> - u8 ss_mask = sseu->subslice_mask[slice];
> + unsigned int slice = fls(sseu->slice_mask) - 1;
> + u8 ss, en, dis;
>
> - u8 enabled_mask = (ss_mask | ss_mask >>
> - GEN10_L3BANK_PAIR_COUNT) & GEN10_L3BANK_MASK;
> - u8 disabled_mask = fuse3 & GEN10_L3BANK_MASK;
> + GEM_BUG_ON(slice >= ARRAY_SIZE(sseu->subslice_mask));
> + ss = sseu->subslice_mask[slice];
> +
> + en = (ss | ss >> GEN10_L3BANK_PAIR_COUNT) & GEN10_L3BANK_MASK;
> + dis = intel_uncore_read(&i915->uncore, GEN10_MIRROR_FUSE3) &
> + GEN10_L3BANK_MASK;
Ok.
> /*
> * Production silicon should have matched L3Bank and
> - * subslice enabled
> + * subslice enabled.
> */
> - WARN_ON((enabled_mask & disabled_mask) != enabled_mask);
> + WARN_ON((en & ~dis) != en);
That certainly makes more sense. I always feared that was some deep
magic to reflect the underlying HW.
Reviewed-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
