Re: [PATCH] tests/kms_writeback: refresh to expose writeback output

[Alex Hung <alex.hung@amd.com>]

On 2024-05-13 16:20, Abhinav Kumar wrote:
> 
> 
> On 5/13/2024 3:06 PM, Alex Hung wrote:
>>
>>
>> On 2024-05-13 15:48, Abhinav Kumar wrote:
>>>
>>>
>>> On 5/13/2024 2:30 PM, Alex Hung wrote:
>>>> With dc2d7fb4f978, all kms_writeback subtests are skipped.
>>>>
>>>> Refresh the outputs to grab all supported connectors,
>>>> the writeback output that is just enabled.
>>>>
>>>
>>> Can you please help to explain how the tests are getting skipped due 
>>> to setting the client_cap?
>>>
>>> And also why calling igt_display_reset_outputs() is fixing this issue?
>>
>> Without the igt_display_reset_outputs(), the result is
>>
>> IGT-Version: 1.28-gbe9b99928 (x86_64) (Linux: 6.7.0-rc5-99-custom x86_64)
>> Using IGT_SRANDOM=1715637543 for randomisation
>> Opened device: /dev/dri/card0
>> Test requirement not met in function __igt_unique____real_main553, 
>> file ../tests/kms_writeback.c:579:
>> Test requirement: output
>> Last errno: 95, Operation not supported
>> Subtest writeback-pixel-formats: SKIP (0.000s)
>> Subtest writeback-invalid-parameters: SKIP (0.000s)
>> Subtest writeback-fb-id: SKIP (0.000s)
>> Subtest writeback-fb-id-XRGB2101010: SKIP (0.000s)
>> Subtest writeback-check-output: SKIP (0.000s)
>> Subtest writeback-check-output-XRGB2101010: SKIP (0.000s)
>>
>>
>> It looks like the below "output = kms_writeback_get_output(&display)" 
>> will return null as it cannot find any connector type with 
>> DRM_MODE_CONNECTOR_WRITEBACK.
>>
>> The igt_display_reset_outputs() is able to re-initialize the output 
>> and therefore includes the writeback output.
>>
> 
> Thanks for the details, I understand the issue now.
> 
> But rather than calling igt_display_reset_outputs(), I think better way 
> will be to set the cap before calling igt_display_require()
> 
> diff --git a/tests/kms_writeback.c b/tests/kms_writeback.c
> index f89b2d62fcc9..6ecab46e440b 100644
> --- a/tests/kms_writeback.c
> +++ b/tests/kms_writeback.c
> @@ -567,14 +567,13 @@ igt_main_args("b:c:f:dl", long_options, help_str, 
> opt_handler, NULL)
> 
>                  kmstest_set_vt_graphics_mode();
> 
> +               ret = drmSetClientCap(display.drm_fd, 
> DRM_CLIENT_CAP_WRITEBACK_CONNECTORS, 1);
> +               igt_require_f(!ret, "error setting 
> DRM_CLIENT_CAP_WRITEBACK_CONNECTORS\n");
> +
>                  igt_display_require(&display, display.drm_fd);
> 
> Let me know if that works for you.
> 

This works if DRM_CLIENT_CAP_ATOMIC is also set as below:

Is this preferred?

--- a/tests/kms_writeback.c
+++ b/tests/kms_writeback.c
@@ -567,14 +567,18 @@ igt_main_args("b:c:f:dl", long_options, help_str, 
opt_handler, NULL)

                 kmstest_set_vt_graphics_mode();

-               igt_display_require(&display, display.drm_fd);
-
-               igt_require(display.is_atomic);
+               if (drmSetClientCap(display.drm_fd, 
DRM_CLIENT_CAP_ATOMIC, 1) == 0)
+                       display.is_atomic = 1;

                 ret = drmSetClientCap(display.drm_fd, 
DRM_CLIENT_CAP_WRITEBACK_CONNECTORS, 1);

                 igt_require_f(!ret, "error setting 
DRM_CLIENT_CAP_WRITEBACK_CONNECTORS\n");

+               igt_display_require(&display, display.drm_fd);
+
+               igt_require(display.is_atomic);
+
+




>>>
>>>> Fixes: dc2d7fb4f978 ("lib/igt_kms: move setting 
>>>> DRM_CLIENT_CAP_WRITEBACK_CONNECTORS to kms_writeback")
>>>> Signed-off-by: Alex Hung <alex.hung@amd.com>
>>>> ---
>>>>   tests/kms_writeback.c | 4 +++-
>>>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/tests/kms_writeback.c b/tests/kms_writeback.c
>>>> index f89b2d62f..4c54f3b89 100644
>>>> --- a/tests/kms_writeback.c
>>>> +++ b/tests/kms_writeback.c
>>>> @@ -572,9 +572,11 @@ igt_main_args("b:c:f:dl", long_options, 
>>>> help_str, opt_handler, NULL)
>>>>           igt_require(display.is_atomic);
>>>>           ret = drmSetClientCap(display.drm_fd, 
>>>> DRM_CLIENT_CAP_WRITEBACK_CONNECTORS, 1);
>>>> -
>>>
>>> Is this part needed for this patch?
>>
>> No, but I think they are related and thus grouping them together is 
>> easier to read.
>>
>>>
>>>>           igt_require_f(!ret, "error setting 
>>>> DRM_CLIENT_CAP_WRITEBACK_CONNECTORS\n");
>>>> +        /* Refresh the outputs to grab all supported connectors.*/
>>>> +        igt_display_reset_outputs(&display);
>>>> +
>>>>           output = kms_writeback_get_output(&display);
>>>>           igt_require(output);