Re: [Intel-gfx] [PATCH] drm/i915/mtl: avoid stringop-overflow warning

2023-10-26 Thread Jani Nikula 
On Mon, 23 Oct 2023, Jani Nikula  wrote:
> On Mon, 16 Oct 2023, Arnd Bergmann  wrote:
>> From: Arnd Bergmann 
>>
>> The newly added memset() causes a warning for some reason I could not figure 
>> out:
>>
>> In file included from arch/x86/include/asm/string.h:3,
>>  from drivers/gpu/drm/i915/gt/intel_rc6.c:6:
>> In function 'rc6_res_reg_init',
>> inlined from 'intel_rc6_init' at 
>> drivers/gpu/drm/i915/gt/intel_rc6.c:610:2:
>> arch/x86/include/asm/string_32.h:195:29: error: '__builtin_memset' writing 
>> 16 bytes into a region of size 0 overflows the destination 
>> [-Werror=stringop-overflow=]
>>   195 | #define memset(s, c, count) __builtin_memset(s, c, count)
>>   | ^
>> drivers/gpu/drm/i915/gt/intel_rc6.c:584:9: note: in expansion of macro 
>> 'memset'
>>   584 | memset(rc6->res_reg, INVALID_MMIO_REG.reg, 
>> sizeof(rc6->res_reg));
>>   | ^~
>> In function 'intel_rc6_init':
>>
>> Change it to an normal initializer and an added memcpy() that does not have
>> this problem.
>>
>> Fixes: 4bb9ca7ee0745 ("drm/i915/mtl: C6 residency and C state type for MTL 
>> SAMedia")
>> Signed-off-by: Arnd Bergmann 
>> ---
>>  drivers/gpu/drm/i915/gt/intel_rc6.c | 16 ++--
>>  1 file changed, 10 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/intel_rc6.c 
>> b/drivers/gpu/drm/i915/gt/intel_rc6.c
>> index 8b67abd720be8..7090e4be29cb6 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_rc6.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_rc6.c
>> @@ -581,19 +581,23 @@ static void __intel_rc6_disable(struct intel_rc6 *rc6)
>>  
>>  static void rc6_res_reg_init(struct intel_rc6 *rc6)
>>  {
>> -memset(rc6->res_reg, INVALID_MMIO_REG.reg, sizeof(rc6->res_reg));
>
> That's just bollocks. memset() is byte granularity, while
> INVALID_MMIO_REG.reg is u32. If the value was anything other than 0,
> this would break.
>
> And you're not supposed to look at the guts of i915_reg_t to begin with,
> that's why it's a typedef. Basically any code that accesses the members
> of i915_reg_t outside of its implementation are doing it wrong.
>
> Reviewed-by: Jani Nikula 

Thanks for the patch, pushed to drm-intel-gt-next.

BR,
Jani.

>
>
>> +i915_reg_t res_reg[INTEL_RC6_RES_MAX] = {
>> +[0 ... INTEL_RC6_RES_MAX - 1] = INVALID_MMIO_REG,
>> +};
>>  
>>  switch (rc6_to_gt(rc6)->type) {
>>  case GT_MEDIA:
>> -rc6->res_reg[INTEL_RC6_RES_RC6] = MTL_MEDIA_MC6;
>> +res_reg[INTEL_RC6_RES_RC6] = MTL_MEDIA_MC6;
>>  break;
>>  default:
>> -rc6->res_reg[INTEL_RC6_RES_RC6_LOCKED] = GEN6_GT_GFX_RC6_LOCKED;
>> -rc6->res_reg[INTEL_RC6_RES_RC6] = GEN6_GT_GFX_RC6;
>> -rc6->res_reg[INTEL_RC6_RES_RC6p] = GEN6_GT_GFX_RC6p;
>> -rc6->res_reg[INTEL_RC6_RES_RC6pp] = GEN6_GT_GFX_RC6pp;
>> +res_reg[INTEL_RC6_RES_RC6_LOCKED] = GEN6_GT_GFX_RC6_LOCKED;
>> +res_reg[INTEL_RC6_RES_RC6] = GEN6_GT_GFX_RC6;
>> +res_reg[INTEL_RC6_RES_RC6p] = GEN6_GT_GFX_RC6p;
>> +res_reg[INTEL_RC6_RES_RC6pp] = GEN6_GT_GFX_RC6pp;
>>  break;
>>  }
>> +
>> +memcpy(rc6->res_reg, res_reg, sizeof(res_reg));
>>  }
>>  
>>  void intel_rc6_init(struct intel_rc6 *rc6)

-- 
Jani Nikula, Intel

Re: [Intel-gfx] [PATCH] drm/i915/mtl: avoid stringop-overflow warning

2023-10-16 Thread Arnd Bergmann 
On Tue, Oct 17, 2023, at 00:10, Andi Shyti wrote:
> Hi Arnd,
>
>>  static void rc6_res_reg_init(struct intel_rc6 *rc6)
>>  {
>> -memset(rc6->res_reg, INVALID_MMIO_REG.reg, sizeof(rc6->res_reg));
>
> This is a complex initialization, indeed... how about just
>
>memset(rc6->res_reg, 0, sizeof(rc6->res_reg));
>
>> +i915_reg_t res_reg[INTEL_RC6_RES_MAX] = {
>> +[0 ... INTEL_RC6_RES_MAX - 1] = INVALID_MMIO_REG,
>> +};
>
> This is basically a
>
>i915_reg_t res_reg[INTEL_RC6_RES_MAX] = { };
>
> Don't know which one is clearer.

Right, the original code went out of its way to use INVALID_MMIO_REG
instead of assuming it is zero, so I tried to preserve that for
consistency.

Arnd

Re: [Intel-gfx] [PATCH] drm/i915/mtl: avoid stringop-overflow warning

2023-10-16 Thread Andi Shyti 
Hi Arnd,

>  static void rc6_res_reg_init(struct intel_rc6 *rc6)
>  {
> - memset(rc6->res_reg, INVALID_MMIO_REG.reg, sizeof(rc6->res_reg));

This is a complex initialization, indeed... how about just

   memset(rc6->res_reg, 0, sizeof(rc6->res_reg));

> + i915_reg_t res_reg[INTEL_RC6_RES_MAX] = {
> + [0 ... INTEL_RC6_RES_MAX - 1] = INVALID_MMIO_REG,
> + };

This is basically a

   i915_reg_t res_reg[INTEL_RC6_RES_MAX] = { };

Don't know which one is clearer.

Andi