[igt-dev] [PATCH i-g-t v2] tests/kms_writeback: Enhance kms_writeback for custom modes
Rohith Iyer
quic_rohiiyer at quicinc.com
Wed Jul 6 20:22:34 UTC 2022
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.
>
>>
>> - 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?
>
>>
>> Changes made in V2:
>> - Removed variable redeclaration
>> - Changed sscanf format types
>>
>> Signed-off-by: Rohith Iyer <quic_rohiiyer at 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.
>
>> +}
>> +
>> +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
>
More information about the igt-dev
mailing list