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

[Abhinav Kumar <quic_abhinavk@quicinc.com>]

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.

>>
>>> 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);