[PATCH v5,resend i-g-t 3/6] tests: Add kms_debugfs

[Peter Senna Tschudin]

Hi Karthik,

On 7/10/2025 9:30 PM, Karthik B S wrote:

[...]

>>>>> +
>>>>> +    if (ret) {
>>>>> +        igt_output_t *output;
>>>>> +
>>>>> +        for_each_connected_output(display, output)
>>>>> +            igt_output_set_pipe(output, PIPE_NONE);
>>>>> +
>>>> Please add some counter and abort on reaching max_retry
>>>> to prevent infinite loop here. Something like:
>>>>
>>>>          ++cnt_retry;
>>>>          igt_assert_f(cnt_retry < MAX_IGT_PIPES, "Too much retries
>>>> %d\n", cnt_retry);
>>>>
>>>> Karthik or Swati: I am not sure if this is needed, could you comment
>>>> on this here?
>>> |I think we don't need a retry at all here. If there is a fail with the
>>> first commit just try if we're able to go ahead after using
>>> 'igt_fit_modes_in_bw' and if it doesn't allow just skip the test here.
>>> That was my motivation to suggest to use the wrapper on the previous
>>> revision of the patch.|
>>>
>>> |The only issue currently I see(that I just observed) is in
>>> 'igt_fit_modes_in_bw' the try_commit we're doing is using atomic commit
>>> by default, although there is no reason to have this restriction. So
>>> ideally we would want to fix that in lib and then just have something
>>> like below in this test.|
>> This is getting a bit out of hand for me because I don't know much about
>> displays. First you asked me to use a wrapper, now you want me to fix
>> the wrapper. I will pass fixing the wrapper as this may lead to yet more
>> unrelated work.
>>
>> Should I revert back to not using igt_fit_modes_in_bw()?
> For the lib part I can send out a patch, should be a quick fix and be
> independent of this series. Would that be fine?

Yes! Please CC me in the patch if you want me to test it for you.

>>
>>> |if (atomic)|
>>>
>>> |    ret = igt_display_try_atomic_commit|
>>>
>>> |else|
>>>
>>> |    ret = igt_display_try_commit2(legacy)|
>>>
>>> |if (ret)|
>>>
>>> |    igt_require(||igt_fit_modes_in_bw(display))|
>>>
>>> |igt_display_commit2(display, display->is_atomic ? COMMIT_ATOMIC :
>>> COMMIT_;LEGACY);|
>> I am not sure I follow what you are suggesting here.
> 
> Ah my bad, if we actually fix/update the igt_fit_modes_in_bw even these
> changes are not required as these would be handled internally in the
> wrapper. So we would just need something like below.
> 
> igt_require_f(igt_fit_modes_in_bw,"No valid mode combo found.\n");
> 
> igt_display_commit2(display, display->is_atomic ? COMMIT_ATOMIC :
> COMMIT_LEGACY);

The test is going to be much cleaner after the change.

Thank you!

[...]