Re: [PATCH i-g-t v4 4/4] tests/kms_writeback: Add dump for valid clone mode

[Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>]

Hi Abhinav,

on Intel i915 and Xe drivers writeback is not implemented and I don't 
know if anyone is working on it. In those ways no need to care about 
Intel drivers.

/Juha-Pekka

On 26.9.2024 0.55, Abhinav Kumar wrote:
> Hi IGT maintainers
> 
> I have given some comments below so that we can skip the test if no 
> valid clones of WB output are found  but at a top level, I wanted to get 
> your feedback that if we validate every clone of WB output with it in a 
> pair-wise fashion, thats something which works for all vendors?
> 
> This will not test the case where lets say WB, output A, output B are 
> all clones of each other and we want to do a 3-way clone mode as it will 
> only test,
> 
> 1) WB with output A
> 2) WB with output B
> 
> But atleast it validates all of WB's clones one pair at a time.
> 
> So wanted to know if we can make this a generic test-case of 
> kms_writeback OR we need to protect this with some cmdline option.
> 
> Thanks
> 
> Abhinav
> On 9/25/2024 1:37 PM, Jessica Zhang wrote:
>> From: Esha Bharadwaj <quic_ebharadw@quicinc.com>
>>
>> Move the regular writeback dump into its own subtest and add a subtest to
>> dump pairs of cloned non-writeback and writeback encoders
>>
>> Signed-off-by: Esha Bharadwaj <quic_ebharadw@quicinc.com>
>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>> ---
>>   tests/kms_writeback.c | 86 +++++++++++++++++++++++++++++++++++++++++ 
>> +++++++---
>>   1 file changed, 82 insertions(+), 4 deletions(-)
>>
>> diff --git a/tests/kms_writeback.c b/tests/kms_writeback.c
>> index 4155961a0..cdbb9e436 100644
>> --- a/tests/kms_writeback.c
>> +++ b/tests/kms_writeback.c
>> @@ -47,6 +47,13 @@
>>   #include "sw_sync.h"
>>   /**
>> + * SUBTEST: dump-writeback
>> + * Description: Dump a non-cloned writeback buffer
>> + *
>> + * SUBTEST: dump-valid-clones
>> + * Description: Dump all valid clone pairs of writeback and non- 
>> writeback
>> + *              connectors
>> + *
>>    * SUBTEST: writeback-check-output-XRGB2101010
>>    * Description: Check XRGB2101010 writeback output with CRC validation
>>    *
>> @@ -450,7 +457,7 @@ static void do_single_commit(igt_output_t *output, 
>> igt_plane_t *plane, igt_fb_t
>>   }
>>   static void commit_and_dump_fb(igt_display_t *display, igt_output_t 
>> *output, igt_plane_t *plane,
>> -                    igt_fb_t *input_fb, drmModeModeInfo *mode)
>> +                   igt_fb_t *input_fb, drmModeModeInfo *mode, char 
>> *suffix)
>>   {
>>       cairo_surface_t *fb_surface_out;
>>       char filepath_out[PATH_MAX];
>> @@ -471,7 +478,8 @@ static void commit_and_dump_fb(igt_display_t 
>> *display, igt_output_t *output, igt
>>       do_single_commit(output, plane, input_fb, &output_fb);
>>       fb_surface_out = igt_get_cairo_surface(display->drm_fd, 
>> &output_fb);
>> -    snprintf(filepath_out, PATH_MAX, "%s/%s.png", path_name, file_name);
>> +    snprintf(filepath_out, PATH_MAX, "%s/%s-%s.png", path_name, 
>> file_name, suffix);
>> +
>>       status = cairo_surface_write_to_png(fb_surface_out, filepath_out);
>>       igt_assert_eq(status, CAIRO_STATUS_SUCCESS);
>>       cairo_surface_destroy(fb_surface_out);
>> @@ -555,6 +563,7 @@ igt_main_args("b:c:f:dl", long_options, help_str, 
>> opt_handler, NULL)
>>       drmModeModeInfo mode;
>>       unsigned int fb_id;
>>       int ret;
>> +    char dump_suffix[PATH_MAX];
>>       memset(&display, 0, sizeof(display));
>> @@ -603,9 +612,78 @@ igt_main_args("b:c:f:dl", long_options, help_str, 
>> opt_handler, NULL)
>>           if (data.list_modes)
>>               list_writeback_modes(&display);
>> -        if (data.dump_check)
>> -            commit_and_dump_fb(&display, output, plane, &input_fb, 
>> &mode);
>>       }
>> +
>> +    /*
>> +     * When dump_check is high, the following subtests will be run.
>> +     * These tests will be skipped if list_modes flag is high.
>> +     */
>> +    igt_describe("Dump a non-cloned writeback buffer");
>> +    igt_subtest("dump-writeback") {
>> +        igt_skip_on(!data.dump_check || data.list_modes);
>> +        snprintf(dump_suffix, PATH_MAX, "encoder%d", output->id);
>> +        commit_and_dump_fb(&display, output, plane, &input_fb, &mode, 
>> dump_suffix);
>> +    }
>> +
>> +    igt_describe("Dump valid clone pairs of writeback and non- 
>> writeback connectors");
>> +    igt_subtest("dump-valid-clones") {
>> +        igt_output_t *nonwb_output;
>> +        enum pipe curr_pipe;
>> +        drmModeModeInfo *nonwb_mode;
>> +        igt_fb_t clone_fb;
>> +
>> +        /*
>> +         * Drop the dump flag check once calling writeback_sequence() is
>> +         * supported
>> +         */
>> +        igt_skip_on(!data.dump_check || data.list_modes);
>> +        igt_skip_on_f(!(data.supported_colors & XRGB8888), 
>> "DRM_FORMAT_XRGB8888 is unsupported\n");
>> +        igt_require(fb_id > 0);
>> +        for_each_output(&display, nonwb_output) {
>> +            if ((nonwb_output->config.connector->connector_type == 
>> DRM_MODE_CONNECTOR_WRITEBACK)
>> +                    || igt_output_clone_valid(display.drm_fd, output, 
>> nonwb_output))
>> +                continue;
>> +
>> +            /* Set up non-writeback cloned output */
>> +            curr_pipe = output->pending_pipe;
>> +            nonwb_mode = igt_output_get_mode(nonwb_output);
>> +
>> +            igt_output_set_pipe(nonwb_output, curr_pipe);
>> +            igt_output_override_mode(output, nonwb_mode);
>> +
>> +            fb_id = igt_create_fb(display.drm_fd, nonwb_mode->hdisplay,
>> +                      nonwb_mode->vdisplay,
>> +                      DRM_FORMAT_XRGB8888,
>> +                      DRM_FORMAT_MOD_LINEAR,
>> +                      &clone_fb);
>> +            igt_assert(fb_id >= 0);
>> +
>> +            /*
>> +             * Currently, only dump the resulting frame. This is
>> +             * because the igt_fb_get_fnv1a_crc() is unable to
>> +             * calculate CRCs for non-32 bit aligned resolutions.
>> +             *
>> +             * Once there is a workaround for this issue, this can
>> +             * be changed to:
>> +             *    if (dump_check flag set)
>> +             *        commit_and_dump_fb()
>> +             *    else
>> +             *        writeback_sequence()
>> +             */
> 
> With https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/ 
> commit/27ba1f4756f12a3694dce6d456aed947f22a8e34 merged, this comment is 
> no longer true.
> 
> We should drop the dump_check restriction for clones test.
> 
> We should also skip this test if there were no possible_clones found for 
> the WB output.
>