Re: [igt-dev] [PATCH i-g-t 1/3 v5] lib/igt_kms: Fix memory corruption

[Vignesh Raman <vignesh.raman@collabora.com>]

Hi Bhanu,

On 10/11/23 15:21, Modem, Bhanuprakash wrote:
>> @@ -2770,6 +2770,10 @@ void igt_display_require(igt_display_t 
>> *display, int drm_fd)
>>       }
>>   #endif
>> +    igt_require_f(resources->count_crtcs <= IGT_MAX_PIPES,
> ---------------------------------------------^
> As pipe index starts from 0, we must use '<' not '<='.

igt_require_f - Skip a (sub-)test if a condition is not met

igt_require_f(resources->count_crtcs <= IGT_MAX_PIPES,
                      "count_crtcs exceeds IGT_MAX_PIPES, 
resources->count_crtcs=%d, IGT_MAX_PIPES=%d\n",
                      resources->count_crtcs, IGT_MAX_PIPES);

If count_crtcs=16 and IGT_MAX_PIPES=16 (In the current case with 
virtio-gpu),

count_crtcs <= IGT_MAX_PIPES will be true and test requirement is passed 
and following statement will be executed.
display->n_pipes = IGT_MAX_PIPES;

In another case,
count_crtcs < IGT_MAX_PIPES will be false and test will be skipped,

So we need to use <= right?

>>   enum pipe {
>> @@ -70,7 +80,15 @@ enum pipe {
>>           PIPE_F,
>>       PIPE_G,
>>       PIPE_H,
>> -        IGT_MAX_PIPES
>> +    PIPE_I,
>> +    PIPE_J,
>> +    PIPE_K,
>> +    PIPE_L,
>> +    PIPE_M,
>> +    PIPE_N,
>> +    PIPE_O,
>> +    PIPE_P,
>> +    IGT_MAX_PIPES
> 
> Please don't mix tabs & spaces, and try to align with the declaration of 
> previous pipes.

The previous ones G and H used tabs and the rest were spaces. I will use 
spaces for all now.

> 
> Apart from these minor fixes, this patch LGTM. With above comments 
> addressed, this patch is
> 
> Reviewed-by: Bhanuprakash Modem <bhanuprakash.modem@intel.com>

Thank you.

Regards,
Vignesh