[PATCH v2] drm/xe/gt: Fix assert in L3 bank mask generation

[Lucas De Marchi]

On Tue, Apr 30, 2024 at 11:40:45AM GMT, Francois Dugast wrote:
>What needs to be asserted is that the pattern fits in the number
>of bits provided by the user in patternbits, otherwise it would
>be truncated when replicated according to the mask, which is
>likely not the intended use of this function.
>The pattern argument is a bitmap so use find_last_bit() instead
>of fls(). The bit position starts at index 0 so remove "or equal"
>from the comparison, and XE_MAX_L3_BANK_MASK_BITS would be the
>returned value if the pattern is 0.
>
>Cc: Matt Roper <matthew.d.roper at intel.com>
>Cc: Lucas De Marchi <lucas.demarchi at intel.com>
>Signed-off-by: Francois Dugast <francois.dugast at intel.com>
>---
> drivers/gpu/drm/xe/xe_gt_topology.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/gpu/drm/xe/xe_gt_topology.c b/drivers/gpu/drm/xe/xe_gt_topology.c
>index af841d801a8f..efe9e5d6fd52 100644
>--- a/drivers/gpu/drm/xe/xe_gt_topology.c
>+++ b/drivers/gpu/drm/xe/xe_gt_topology.c
>@@ -107,8 +107,11 @@ gen_l3_mask_from_pattern(struct xe_device *xe, xe_l3_bank_mask_t dst,
> 			 unsigned long mask)
> {
> 	unsigned long bit;
>+	unsigned long last_bit = find_last_bit(pattern,
>+					       XE_MAX_L3_BANK_MASK_BITS);
>
>-	xe_assert(xe, fls(mask) <= patternbits);
>+	xe_assert(xe, last_bit < patternbits ||
>+		  last_bit == XE_MAX_L3_BANK_MASK_BITS);

then if xe_assert() compiles to nothing we may be left with "unused
last_bit" variable" :(.  I think it would be simpler if pattern was
unsigned long long as apparently it would be sufficient for our needs
and we could simply do

	xe_assert(xe, pattern <= GENMASK_ULL(patternbits, 0));

As for XE_MAX_L3_BANK_MASK_BITS, the assert here doesn't seem sufficient
as you want to check if the final result doesn't overflow the array.


Wihout changing the pattern type, I think what we want is:

	xe_assert(xe, find_last_bit(pattern, XE_MAX_L3_BANK_MASK_BITS) < patternbits);
	xe_assert(xe, patternbits * fls(mask) <= XE_MAX_L3_BANK_MASK_BITS);


Lucas De Marchi

> 	for_each_set_bit(bit, &mask, 32) {
> 		xe_l3_bank_mask_t shifted_pattern = {};
>
>-- 
>2.34.1
>