Re: [PATCH i-g-t v2] tests/kms_atomic_transition: Fix dynamic subtest

[Gustavo Sousa <gustavo.sousa@intel.com>]

Quoting Ngai-Mint Kwan (2025-10-30 17:02:10-03:00)
>
>On 2025-10-30 13:00, Ngai-Mint Kwan wrote:
>> Hi Gustavo,
>>
>> On 2025-10-30 12:33, Gustavo Sousa wrote:
>>> Quoting Ngai-Mint Kwan (2025-10-30 16:15:50-03:00)
>>>> Fix Segmentation Fault when calling igt_pipe_crc_free() due to 
>>>> double-freeing a
>>>> dangling pointer.
>>>>
>>>> This occurs when a system connected to multiple displays executes a 
>>>> test with
>>>> "1x-outputs" dynamic subtest.
>>>>
>>>> Example:
>>>> kms_atomic_transition --run-subtest modeset-transition 
>>>> --dynamic-subtest 1x-outputs
>>>>
>>>> v2:
>>>> - Move test cleanup inside run_modeset_tests().
>>>>
>>>> Signed-off-by: Ngai-Mint Kwan <ngai-mint.kwan@linux.intel.com>
>>>> ---
>>>> tests/kms_atomic_transition.c | 25 +++++++++++++------------
>>>> 1 file changed, 13 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/tests/kms_atomic_transition.c 
>>>> b/tests/kms_atomic_transition.c
>>>> index e1387bd89..419afe4dd 100644
>>>> --- a/tests/kms_atomic_transition.c
>>>> +++ b/tests/kms_atomic_transition.c
>>>> @@ -899,6 +899,7 @@ static void run_modeset_tests(data_t *data, int 
>>>> howmany, bool nonblocking, bool
>>>>          unsigned iter_max;
>>>>          igt_output_t *output;
>>>>          uint16_t width = 0, height = 0;
>>>> +        enum pipe pipe_enum;
>>> Nit: I would have just gone with 'enum pipe pipe', which seems to be 
>>> the most
>>> common way of doing it (according to
>>>
>>>      git grep -oh 'enum pipe \w\+' | sort | uniq -c | sort -g
>>>
>>> ).
>>>
>>> The change itself looks logically right, so:
>>>
>>> Reviewed-by: Gustavo Sousa <gustavo.sousa@intel.com>
>> That would have been my first choice too!
>> Unfortunately, the following warning appears on compile:
>>
>> ../tests/kms_atomic_transition.c: In function ‘run_modeset_tests’:
>> ../tests/kms_atomic_transition.c:921:29: warning: declaration of 
>> ‘pipe’ shadows a previous local [-Wshadow]
>>   921 |                 igt_pipe_t *pipe = &data->display.pipes[i];
>>       |                             ^~~~
>> ../tests/kms_atomic_transition.c:902:19: note: shadowed declaration is 
>> here
>>   902 |         enum pipe pipe;
>>       |                   ^~~~
>>
>> This is why I settled with "enum_pipe", but I am open to other 
>> suggestions.
>Correction: "pipe_enum"

Yep.  I did not see that.  Thanks.

I think we can go ahead with the patch as is.

--
Gustavo Sousa

>
>Kwan
>>
>> Kwan
>>>
>>>> retry:
>>>>          unset_output_pipe(&data->display);
>>>> @@ -1033,6 +1034,18 @@ retry:
>>>>                          }
>>>>                  }
>>>>          }
>>>> +
>>>> +        /* Cleanup */
>>>> +        unset_output_pipe(&data->display);
>>>> +        igt_display_commit2(&data->display, COMMIT_ATOMIC);
>>>> +
>>>> +        if (is_intel_device(data->drm_fd)) {
>>>> +                for_each_pipe(&data->display, pipe_enum)
>>>> + igt_pipe_crc_free(data->pipe_crcs[pipe_enum]);
>>>> +        }
>>>> +
>>>> +        igt_remove_fb(data->drm_fd, &data->fbs[0]);
>>>> +        igt_remove_fb(data->drm_fd, &data->fbs[1]);
>>>> }
>>>>
>>>> static void run_modeset_transition(data_t *data, int 
>>>> requested_outputs, bool nonblocking, bool fencing)
>>>> @@ -1067,18 +1080,6 @@ static void run_modeset_transition(data_t 
>>>> *data, int requested_outputs, bool non
>>>>
>>>>          igt_dynamic_f("%ix-outputs", requested_outputs)
>>>>                  run_modeset_tests(data, requested_outputs, 
>>>> nonblocking, fencing);
>>>> -
>>>> -        /* Cleanup */
>>>> -        unset_output_pipe(&data->display);
>>>> -        igt_display_commit2(&data->display, COMMIT_ATOMIC);
>>>> -
>>>> -        if (is_intel_device(data->drm_fd)) {
>>>> -                for_each_pipe(&data->display, pipe)
>>>> - igt_pipe_crc_free(data->pipe_crcs[pipe]);
>>>> -        }
>>>> -
>>>> -        igt_remove_fb(data->drm_fd, &data->fbs[0]);
>>>> -        igt_remove_fb(data->drm_fd, &data->fbs[1]);
>>>> }
>>>>
>>>> static bool pipe_output_combo_valid(igt_display_t *display,
>>>> -- 
>>>> 2.43.0
>>>>
>>
>