Re: [igt-dev] [PATCH i-g-t v2] tests/kms_writeback: Enhance kms_writeback for custom modes

[Rohith Iyer <quic_rohiiyer@quicinc.com>]

Including all original cc's - accidentally didn't reply to all.

On 7/6/2022 1:22 PM, Rohith Iyer wrote:
> 
> 
> On 7/6/2022 10:18 AM, Ville Syrjälä wrote:
>> On Wed, Jun 22, 2022 at 06:30:03PM -0700, Rohith Iyer wrote:
>>> Enhance kms_writeback to add support for the below options:
>>>
>>> - View all writeback modes
>>> Usage: "./kms_writeback --list-modes"
>>>
>>> - Test a standard mode from connector list
>>> Usage: "./kms_writeback --standard <mode_index>"
>>
>> "standard" makes me think of gtf/cvt/etc.
> 
> Acked.

We are planning to use "builtin" as the flag name instead of "standard".

> 
>>
>>>
>>> - Test a custom mode from user input
>>> Usage: "./kms_writeback --custom <mode_parameters>"
>>> Refer to --help for exact syntax
>>>
>>> - Dump the writeback output buffer to png file
>>> Usage: "./kms_writeback --dump"
>>
>> That should probably take a file name.
> 
> Currently, the dumped buffer will be saved in $IGT_FRAME_DUMP_PATH. If 
> the user wants to customize the file path, they can set that environment 
> variable. Is there any advantage of passing in a file name as a CLI 
> argument over the current implementation?

To make it more clear, I can indicate in the commit message that the 
file will be written to $IGT_FRAME_DUMP_PATH.

> 
>>
>>>
>>> Changes made in V2:
>>> - Removed variable redeclaration
>>> - Changed sscanf format types
>>>
>>> Signed-off-by: Rohith Iyer <quic_rohiiyer@quicinc.com>
>>> ---
>>>   tests/kms_writeback.c | 183 +++++++++++++++++++++++++++++++++++++-----
>>>   1 file changed, 161 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/tests/kms_writeback.c b/tests/kms_writeback.c
>>> index 6efc72df..4f343b48 100644
>>> --- a/tests/kms_writeback.c
>>> +++ b/tests/kms_writeback.c
>>> @@ -26,11 +26,13 @@
>>>   #include <stdbool.h>
>>>   #include <stdio.h>
>>>   #include <string.h>
>>> +#include <limits.h>
>>>   #include "igt.h"
>>>   #include "igt_core.h"
>>>   #include "igt_fb.h"
>>>   #include "sw_sync.h"
>>> +#include "igt_chamelium.h"
>>>   IGT_TEST_DESCRIPTION(
>>>      "This test validates the expected behavior of the writeback 
>>> connectors "
>>> @@ -39,6 +41,17 @@ IGT_TEST_DESCRIPTION(
>>>      "by using CRC."
>>>   );
>>> +typedef struct {
>>> +    bool standard_mode;
>>> +    bool custom_mode;
>>> +    bool list_modes;
>>> +    bool dump_check;
>>> +    int mode_index;
>>> +    drmModeModeInfo user_mode;
>>> +} data_t;
>>> +
>>> +static data_t data;
>>> +
>>>   static drmModePropertyBlobRes 
>>> *get_writeback_formats_blob(igt_output_t *output)
>>>   {
>>>       drmModePropertyBlobRes *blob = NULL;
>>> @@ -58,29 +71,15 @@ static drmModePropertyBlobRes 
>>> *get_writeback_formats_blob(igt_output_t *output)
>>>       return blob;
>>>   }
>>> -static bool check_writeback_config(igt_display_t *display, 
>>> igt_output_t *output)
>>> +static bool check_writeback_config(igt_display_t *display, 
>>> igt_output_t *output,
>>> +                drmModeModeInfo override_mode)
>>>   {
>>>       igt_fb_t input_fb, output_fb;
>>>       igt_plane_t *plane;
>>>       uint32_t writeback_format = DRM_FORMAT_XRGB8888;
>>>       uint64_t modifier = DRM_FORMAT_MOD_LINEAR;
>>>       int width, height, ret;
>>> -    drmModeModeInfo override_mode = {
>>> -        .clock = 25175,
>>> -        .hdisplay = 640,
>>> -        .hsync_start = 656,
>>> -        .hsync_end = 752,
>>> -        .htotal = 800,
>>> -        .hskew = 0,
>>> -        .vdisplay = 480,
>>> -        .vsync_start = 490,
>>> -        .vsync_end = 492,
>>> -        .vtotal = 525,
>>> -        .vscan = 0,
>>> -        .vrefresh = 60,
>>> -        .flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
>>> -        .name = {"640x480-60"},
>>> -    };
>>> +
>>>       igt_output_override_mode(output, &override_mode);
>>>       width = override_mode.hdisplay;
>>> @@ -109,8 +108,25 @@ static bool check_writeback_config(igt_display_t 
>>> *display, igt_output_t *output)
>>>   static igt_output_t *kms_writeback_get_output(igt_display_t *display)
>>>   {
>>> -    int i;
>>>       enum pipe pipe;
>>> +    int i;
>>> +
>>> +    drmModeModeInfo override_mode = {
>>> +        .clock = 25175,
>>> +        .hdisplay = 640,
>>> +        .hsync_start = 656,
>>> +        .hsync_end = 752,
>>> +        .htotal = 800,
>>> +        .hskew = 0,
>>> +        .vdisplay = 480,
>>> +        .vsync_start = 490,
>>> +        .vsync_end = 492,
>>> +        .vtotal = 525,
>>> +        .vscan = 0,
>>> +        .vrefresh = 60,
>>> +        .flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
>>> +        .name = {"640x480-60"},
>>> +    };
>>>       for (i = 0; i < display->n_outputs; i++) {
>>>           igt_output_t *output = &display->outputs[i];
>>> @@ -121,7 +137,12 @@ static igt_output_t 
>>> *kms_writeback_get_output(igt_display_t *display)
>>>           for_each_pipe(display, pipe) {
>>>               igt_output_set_pipe(output, pipe);
>>> -            if (check_writeback_config(display, output)) {
>>> +            if (data.custom_mode)
>>> +                override_mode = data.user_mode;
>>> +            if (data.standard_mode)
>>> +                override_mode = 
>>> output->config.connector->modes[data.mode_index];
>>> +
>>> +            if (check_writeback_config(display, output, 
>>> override_mode)) {
>>>                   igt_debug("Using connector %u:%s on pipe %d\n",
>>>                         output->config.connector->connector_id,
>>>                         output->name, pipe);
>>> @@ -347,7 +368,114 @@ static void writeback_check_output(igt_output_t 
>>> *output, igt_plane_t *plane,
>>>       igt_remove_fb(output_fb->fd, &second_out_fb);
>>>   }
>>> -igt_main
>>> +static void do_single_commit(igt_output_t *output, igt_plane_t 
>>> *plane, igt_fb_t *in_fb,
>>> +                    igt_fb_t *out_fb)
>>> +{
>>> +    uint32_t in_fb_color = 0xffff0000;
>>> +
>>> +    fill_fb(in_fb, in_fb_color);
>>> +
>>> +    igt_plane_set_fb(plane, in_fb);
>>> +    igt_output_set_writeback_fb(output, out_fb);
>>> +
>>> +    igt_display_commit_atomic(output->display, 
>>> DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
>>> +    if (out_fb)
>>> +        get_and_wait_out_fence(output);
>>> +}
>>> +
>>> +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)
>>> +{
>>> +    cairo_surface_t *fb_surface_out;
>>> +    char filename_out[PATH_MAX];
>>> +    cairo_status_t status;
>>> +    char *id;
>>> +    unsigned int fb_id;
>>> +    igt_fb_t output_fb;
>>> +
>>> +    id = getenv("IGT_FRAME_DUMP_PATH");
>>> +    fb_id = igt_create_fb(display->drm_fd, mode->hdisplay, 
>>> mode->vdisplay, DRM_FORMAT_XRGB8888,
>>> +                igt_fb_mod_to_tiling(0), &output_fb);
>>> +    igt_require(fb_id > 0);
>>> +
>>> +    do_single_commit(output, plane, input_fb, &output_fb);
>>> +
>>> +    fb_surface_out = igt_get_cairo_surface(display->drm_fd, 
>>> &output_fb);
>>> +    snprintf(filename_out, PATH_MAX, "%s/%s.png", id, "frame-out");
>>> +    status = cairo_surface_write_to_png(fb_surface_out, filename_out);
>>> +    igt_assert_eq(status, CAIRO_STATUS_SUCCESS);
>>> +
>>> +    igt_remove_fb(display->drm_fd, &output_fb);
>>> +}
>>> +
>>> +static igt_output_t *list_writeback_modes(igt_display_t *display)
>>> +{
>>> +    for (int i = 0; i < display->n_outputs; i++) {
>>> +        igt_output_t *output = &display->outputs[i];
>>> +
>>> +        if (output->config.connector->connector_type == 
>>> DRM_MODE_CONNECTOR_WRITEBACK) {
>>> +            for (int j = 0; j < 
>>> output->config.connector->count_modes; j++) {
>>> +                igt_info("[%d]", j);
>>> +                kmstest_dump_mode(&output->config.connector->modes[j]);
>>> +            }
>>> +            break;
>>> +        }
>>> +    }
>>> +    return NULL;
>>> +}
>>> +
>>> +static int parse_mode_string(char *optarg)
>>> +{
>>> +    return sscanf(optarg, 
>>> "%u:%hu:%hu:%hu:%hu:%hu:%hu:%hu:%hu:%hu:%hu:%u:%u:%u:%s", 
>>> &data.user_mode.clock,
>>> +            &data.user_mode.hdisplay, &data.user_mode.hsync_start,
>>> +            &data.user_mode.hsync_end, &data.user_mode.htotal,
>>> +            &data.user_mode.hskew, &data.user_mode.vdisplay,
>>> +            &data.user_mode.vsync_start, &data.user_mode.vsync_end,
>>> +            &data.user_mode.vtotal, &data.user_mode.vscan,
>>> +            &data.user_mode.vrefresh, &data.user_mode.type,
>>> +            &data.user_mode.flags, data.user_mode.name);
>>
>> We should use some standard syntax here. Some ideas:
>> - use the same syntax as testdisplay
>> - accept X modelines perhaps? Woud eg. let you just feed in stuff
>>    from 'gtf' or 'cvt' tools
>> - could also consider accepting some easier syntax eg.
>>    "cvt:640x480-60"/etc. so you would have to play around with external
>>    tools or type in the full modeline
>> - whatever option(s) we choose I'm thinking this whole thing should
>>    live in lib/ so each test wouldn't have to reinvent some new syntax
>>    for it
> 
> Acked.

Planning to make the testdisplay parsing a lib helper method, then 
planning to use that helper in kms_writeback.

> 
>>
>>> +}
>>> +
>>> +static int opt_handler(int option, int option_index, void *_data)
>>> +{
>>> +    switch (option) {
>>> +    case 'l':
>>> +        data.list_modes = true;
>>> +        break;
>>> +    case 's':
>>> +        data.standard_mode = true;
>>> +        data.mode_index = atoi(optarg);
>>> +        break;
>>> +    case 'c':
>>> +        data.custom_mode = true;
>>> +        igt_assert(parse_mode_string(optarg) > 0);
>>> +        break;
>>> +    case 'd':
>>> +        data.dump_check = true;
>>> +        break;
>>> +    default:
>>> +        return IGT_OPT_HANDLER_ERROR;
>>> +    }
>>> +    return IGT_OPT_HANDLER_SUCCESS;
>>> +}
>>> +
>>> +const char *help_str =
>>> +    " --list-modes | -l List of writeback connector modes\n"
>>> +    " --standard | -s Commits a standard mode\n"
>>> +    " --custom | -c Commits a custom mode inputted by user 
>>> <clock:hdisplay:hsyncstart:hsyncend:"
>>> +    
>>> "htotal:hskew:vdisplay:vsyncstart:vsyncend:vtotal:vscan:vrefresh:flags:name>\n" 
>>>
>>> +    " --dump | -d Prints buffer to file - Use command: export 
>>> IGT_FRAME_DUMP_PATH=\"<filepath>\""
>>> +    " before running dump. Will skip all other tests.\n";
>>> +
>>> +static const struct option long_options[] = {
>>> +    { .name = "list-modes", .has_arg = false, .val = 'l', },
>>> +    { .name = "standard", .has_arg = true, .val = 's', },
>>> +    { .name = "custom", .has_arg = true, .val = 'c', },
>>> +    { .name = "dump", .has_arg = false, .val = 'd', },
>>> +    {}
>>> +};
>>> +
>>> +igt_main_args("s:c:dl", long_options, help_str, opt_handler, NULL)
>>>   {
>>>       igt_display_t display;
>>>       igt_output_t *output;
>>> @@ -386,15 +514,23 @@ igt_main
>>>                         &input_fb);
>>>           igt_assert(fb_id >= 0);
>>>           igt_plane_set_fb(plane, &input_fb);
>>> -    }
>>> +        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 or list_modes flag is high, then the 
>>> following subtests will be skipped
>>> +      * as we do not want to do CRC validation.
>>> +      */
>>>       igt_describe("Check the writeback format");
>>>       igt_subtest("writeback-pixel-formats") {
>>> +        igt_skip_on(data.dump_check || data.list_modes);
>>>           drmModePropertyBlobRes *formats_blob = 
>>> get_writeback_formats_blob(output);
>>>           const char *valid_chars = "01234568 ABCGNRUVXY";
>>>           unsigned int i;
>>>           char *c;
>>> -
>>>           /*
>>>            * We don't have a comprehensive list of formats, so just 
>>> check
>>>            * that the blob length is sensible and that it doesn't 
>>> contain
>>> @@ -412,6 +548,7 @@ igt_main
>>>                "(output framebuffer and fence); this test goes through"
>>>                "the combination of possible bad options");
>>>       igt_subtest("writeback-invalid-parameters") {
>>> +        igt_skip_on(data.dump_check || data.list_modes);
>>>           igt_fb_t invalid_output_fb;
>>>           fb_id = igt_create_fb(display.drm_fd, mode.hdisplay / 2,
>>>                         mode.vdisplay / 2,
>>> @@ -427,6 +564,7 @@ igt_main
>>>       igt_describe("Validate WRITEBACK_FB_ID with valid and invalid 
>>> options");
>>>       igt_subtest("writeback-fb-id") {
>>> +        igt_skip_on(data.dump_check || data.list_modes);
>>>           igt_fb_t output_fb;
>>>           fb_id = igt_create_fb(display.drm_fd, mode.hdisplay, 
>>> mode.vdisplay,
>>>                         DRM_FORMAT_XRGB8888,
>>> @@ -441,6 +579,7 @@ igt_main
>>>       igt_describe("Check writeback output with CRC validation");
>>>       igt_subtest("writeback-check-output") {
>>> +        igt_skip_on(data.dump_check || data.list_modes);
>>>           igt_fb_t output_fb;
>>>           fb_id = igt_create_fb(display.drm_fd, mode.hdisplay, 
>>> mode.vdisplay,
>>>                         DRM_FORMAT_XRGB8888,
>>> -- 
>>> 2.17.1
>>