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

[Lucas De Marchi]

On Tue, Apr 30, 2024 at 04:33:36PM GMT, Francois Dugast wrote:
>On Tue, Apr 30, 2024 at 08:43:15AM -0500, Lucas De Marchi wrote:
>> 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);
>
>The array overflow check is a great addition, but the other assertion
>would fail if the pattern is 0 as find_last_bit would return
>XE_MAX_L3_BANK_MASK_BITS which is greater than patternbits.

does it make sense to have pattern == 0? What would we even be repeating
according to a mask? failing the assert would be desirable I think.

Lucas De Marchi

>
>What about this:
>
>	xe_assert(xe, find_last_bit(pattern, XE_MAX_L3_BANK_MASK_BITS) < patternbits || bitmap_empty(pattern, XE_MAX_L3_BANK_MASK_BITS));
>	xe_assert(xe, patternbits * fls(mask) <= XE_MAX_L3_BANK_MASK_BITS);
>
>Francois
>
>>
>>
>> Lucas De Marchi
>>
>> > 	for_each_set_bit(bit, &mask, 32) {
>> > 		xe_l3_bank_mask_t shifted_pattern = {};
>> >
>> > --
>> > 2.34.1
>> >