Re: [igt-dev] [PATCH i-g-t] tests/kms_yuv: Add yuv specific tests, v6.

Juha-Pekka Heikkila
2019-06-19T08:09:25Z

On 13.6.2019 16.14, Maarten Lankhorst wrote:
> Op 12-06-2019 om 22:18 schreef Juha-Pekka Heikkila:
>> Sorry for delay Maarten, I've been reading this on several evenings but always was interrupted. I think this look all good.
>>
>> Reviewed-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
>>
>> On 13.5.2019 19.53, Maarten Lankhorst wrote:
>>> Add tests excercising switching between various scaled/unscaled
>>> transitions to and from various nv12 formats, since some of the
>>> workarounds mention this may fail.
>>>
>>> We also add NV12 specific clipping/scaling tests, to make sure
>>> that YUV src coordinates are always programmed as a multiple of 2
>>> correctly, and verify scaling/clipping works with CRC tests.
>>>
>>> crop-scale-bug shows a corruption when scaling and cropping at the
>>> same time.
>>>
>>> The subpixel clip test rotates and clips a rectangle on each side,
>>> which should produce the same CRC as when that part of the rectangle
>>> is hidden by a cursor and a whole 2x2 pixel is hidden.
>>>
>>> Changes since v1:
>>> - Reset plane position to 0,0 at the start of yuv_valid_width_plane().
>>> - Handle Swati's feedback.
>>> Changes since v2:
>>> - Skip 90°/270° rotation in tests when formats don't support it.
>>> - Add all new HDR formats to test.
>>> - Use igt_plane_has_format_mod.
>>> - Support tests on !i915 by checking if X/Y tiling modifiers are supported.
>>> - Do not enable untested planes to prevent exhausing memory bandwidth
>>>    and available NV12 Y planes in the rgb-switch-scaled test.
>>> Changes since v3:
>>> - Add a sanity test to sanity test YUV conversion paths, on all planes and pipes.
>>> - Dynamically generate list of YUV tests through igt_format_array_fill.
>>> Changes since v4:
>>> - Decrease size to 64x64 to reduce test execution time even more,
>>>    some YUV conversion routines are slow.
>>> - Don't clear degamma/ctm, this is done in core now.
>>> Changes since v5:
>>> - Fix minor compile error.
>>> - Add missing igt_subtest_group
>>>
>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>> ---
>>>   tests/Makefile.sources |   1 +
>>>   tests/kms_yuv.c        | 937 +++++++++++++++++++++++++++++++++++++++++
>>>   tests/meson.build      |   1 +
>>>   3 files changed, 939 insertions(+)
>>>   create mode 100644 tests/kms_yuv.c
>>>
>>> diff --git a/tests/Makefile.sources b/tests/Makefile.sources
>>> index 7f921f6c5988..574861b2e30f 100644
>>> --- a/tests/Makefile.sources
>>> +++ b/tests/Makefile.sources
>>> @@ -75,6 +75,7 @@ TESTS_progs = \
>>>       kms_universal_plane \
>>>       kms_vblank \
>>>       kms_vrr \
>>> +    kms_yuv \
>>>       meta_test \
>>>       perf \
>>>       perf_pmu \
>>> diff --git a/tests/kms_yuv.c b/tests/kms_yuv.c
>>> new file mode 100644
>>> index 000000000000..6cf69b0504f8
>>> --- /dev/null
>>> +++ b/tests/kms_yuv.c
>>> @@ -0,0 +1,937 @@
>>> +/*
>>> + * Copyright © 2019 Intel Corporation
>>> + *
>>> + * Permission is hereby granted, free of charge, to any person obtaining a
>>> + * copy of this software and associated documentation files (the "Software"),
>>> + * to deal in the Software without restriction, including without limitation
>>> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
>>> + * and/or sell copies of the Software, and to permit persons to whom the
>>> + * Software is furnished to do so, subject to the following conditions:
>>> + *
>>> + * The above copyright notice and this permission notice shall be included in
>>> + * all copies or substantial portions of the Software.
>>> + *
>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
>>> + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
>>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
>>> + * IN THE SOFTWARE.
>>> + *
>>> + * Authors:
>>> + *    Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>> + */
>>> +#include "config.h"
>>> +
>>> +#include "igt.h"
>>> +#include <cairo.h>
>>> +#include <errno.h>
>>> +#include <stdint.h>
>>> +#include <unistd.h>
>>> +#include <sys/time.h>
>>> +
>>> +typedef struct {
>>> +    igt_display_t display;
>>> +
>>> +    igt_pipe_crc_t *pipe_crc;
>>> +    struct igt_fb fb[6];
>>> +} data_t;
>>> +
>>> +static uint64_t x_modifier = LOCAL_DRM_FORMAT_MOD_NONE;
>>> +static uint64_t y_modifier = LOCAL_DRM_FORMAT_MOD_NONE;
>>> +static uint32_t *formats, count_formats;
>>> +
>>> +static bool pipe_supports_format(igt_display_t *display, enum pipe pipe,
>>> +                 uint32_t format, uint64_t tiling)
>>> +{
>>> +    igt_plane_t *plane;
>>> +
>>> +    for_each_plane_on_pipe(display, pipe, plane)
>>> +        if (igt_plane_has_format_mod(plane, format, tiling))
>>> +            return true;
>>> +
>>> +    return false;
>>> +}
>>> +
>>> +
>>> +static void remove_fbs(data_t *data)
>>> +{
>>> +    int i;
>>> +
>>> +    for (i = 0; i < ARRAY_SIZE(data->fb); i++)
>>> +        igt_remove_fb(data->display.drm_fd, &data->fb[i]);
>>> +}
>>> +
>>> +static void prepare_crtc(data_t *data, enum pipe pipe, igt_output_t *output)
>>> +{
>>> +    igt_display_t *display = &data->display;
>>> +
>>> +    remove_fbs(data);
>>> +    igt_display_reset(display);
>>> +    igt_output_set_pipe(output, pipe);
>>> +    igt_display_commit2(display, COMMIT_ATOMIC);
>>> +
>>> +    igt_pipe_crc_free(data->pipe_crc);
>>> +    data->pipe_crc = igt_pipe_crc_new(display->drm_fd, pipe,
>>> +                      INTEL_PIPE_CRC_SOURCE_AUTO);
>>> +}
>>> +
>>> +static void fudge_lut(data_t *data, enum pipe pipe, uint16_t mask)
>>> +{
>>> +    igt_pipe_t *pipe_obj = &data->display.pipes[pipe];
>>> +    uint16_t *lut;
>>> +    int i, lut_size;
>>> +
>>> +    if (!igt_pipe_obj_has_prop(pipe_obj, IGT_CRTC_GAMMA_LUT_SIZE))
>>> +        return;
>>> +
>>> +    lut_size = igt_pipe_obj_get_prop(pipe_obj, IGT_CRTC_GAMMA_LUT_SIZE);
>>> +
>>> +    lut = malloc(sizeof(uint16_t) * lut_size);
>>> +    for (i = 0; i < lut_size; i++)
>>> +        lut[i] = (i * 0xffff / (lut_size - 1)) & mask;
>>> +
>>> +    igt_pipe_obj_replace_prop_blob(pipe_obj, IGT_CRTC_GAMMA_LUT, lut, lut_size * sizeof(uint16_t));
>>> +    free(lut);
>>> +}
>>> +
>>> +static void unfudge_lut(data_t *data, enum pipe pipe)
>>> +{
>>> +    igt_pipe_t *pipe_obj = &data->display.pipes[pipe];
>>> +
>>> +    if (igt_pipe_obj_has_prop(pipe_obj, IGT_CRTC_GAMMA_LUT))
>>> +        igt_pipe_obj_replace_prop_blob(pipe_obj, IGT_CRTC_GAMMA_LUT, NULL, 0);
>>> +}
>>> +
>>> +/*
>>> + * Test that we initialize a YUV fb as black correctly,
>>> + * that when we write a black FB it will get the same CRC, and that
>>> + * all colors we test will give the same CRC on all formats.
>>> + */
>>> +static void yuv_sanity(data_t *data, enum pipe pipe, igt_output_t *output)
>>> +{
>>> +    igt_display_t *display = &data->display;
>>> +    igt_plane_t *plane;
>>> +    igt_crc_t black_crc, color_crc, crc;
>>> +    bool first = true;
>>> +
>>> +    prepare_crtc(data, pipe, output);
>>> +
>>> +    for_each_plane_on_pipe(display, pipe, plane) {
>>> +        for (int i = 0; i < count_formats; i++) {
>>> +            struct igt_fb *fb = &data->fb[0];
>>> +            cairo_t *cr;
>>> +
>>> +            if (!igt_format_is_yuv(formats[i]) ||
>>> +                !igt_plane_has_format_mod(plane, formats[i], x_modifier))
>>> +                continue;
>>> +
>>> +            igt_debug("Testing format %.4s on plane %i\n",
>>> +                 (char *)&formats[i], plane->index);
>>> +            igt_create_fb(display->drm_fd, 64, 64, formats[i],
>>> +                      x_modifier, fb);
>>> +
>>> +            igt_plane_set_fb(plane, fb);
>>> +            igt_display_commit2(display, COMMIT_ATOMIC);
>>> +            if (!first) {
>>> +                igt_pipe_crc_collect_crc(data->pipe_crc, &crc);
>>> +                igt_assert_crc_equal(&black_crc, &crc);
>>> +            } else {
>>> +                igt_pipe_crc_collect_crc(data->pipe_crc, &black_crc);
>>> +            }
>>> +
>>> +            /* Compare initial black fb with reference painted black color */
>>> +            cr = igt_get_cairo_ctx(display->drm_fd, fb);
>>> +            igt_paint_color(cr, 0, 0, fb->width, fb->height, 0., 0., 0.);
>>> +            igt_put_cairo_ctx(display->drm_fd, fb, cr);
>>> +            igt_dirty_fb(display->drm_fd, fb);
>>> +            igt_wait_for_vblank(display->drm_fd, pipe);
>>> +            igt_pipe_crc_collect_crc(data->pipe_crc, &crc);
>>> +            igt_assert_crc_equal(&black_crc, &crc);
>>> +
>>> +
>>> +            /* Write a color fb */
>>> +            cr = igt_get_cairo_ctx(display->drm_fd, fb);
>>> +            igt_paint_color(cr, 0, 0, fb->width, fb->height, 1., 1., 0.);
>>> +            igt_put_cairo_ctx(display->drm_fd, fb, cr);
>>> +            fudge_lut(data, pipe, 0xfc00);
>>> +            igt_plane_set_fb(plane, fb);
>>> +            igt_display_commit2(display, COMMIT_ATOMIC);
>>> +
>>> +            if (!first) {
>>> +                igt_pipe_crc_collect_crc(data->pipe_crc, &crc);
>>> +                igt_assert_crc_equal(&color_crc, &crc);
>>> +            } else {
>>> +                igt_pipe_crc_collect_crc(data->pipe_crc, &color_crc);
>>> +                first = false;
>>> +            }
>>> +
>>> +            /* Convert from YUV, back to YUV, to expose bugs in conversion routines */
>>> +            cr = igt_get_cairo_ctx(display->drm_fd, fb);
>>> +            igt_put_cairo_ctx(display->drm_fd, fb, cr);
>>> +            igt_dirty_fb(display->drm_fd, fb);
>>> +            igt_wait_for_vblank(display->drm_fd, pipe);
>>> +            igt_pipe_crc_collect_crc(data->pipe_crc, &crc);
>>> +            igt_assert_crc_equal(&color_crc, &crc);
>>> +
>>> +            unfudge_lut(data, pipe);
>>> +            igt_plane_set_fb(plane, NULL);
>>> +            igt_remove_fb(display->drm_fd, &data->fb[0]);
>>> +        }
>>> +    }
>>> +}
>>> +
>>> +static void set_fb(igt_plane_t *plane, struct igt_fb *fb,
>>> +           bool scaled, igt_rotation_t rot)
>>> +{
>>> +    igt_plane_set_fb(plane, fb);
>>> +
>>> +    if (scaled && fb)
>>> +        igt_fb_set_size(fb, plane, fb->width, 16);
>>> +
>>> +    igt_plane_set_rotation(plane, rot);
>>> +    if (fb && (rot == IGT_ROTATION_90 || rot == IGT_ROTATION_270))
>>> +        igt_plane_set_size(plane, fb->height, fb->width);
>>> +}
>>> +
>>> +static void yuv_rgb_switch(data_t *data, enum pipe pipe,
>>> +               igt_output_t *output, bool scaled, unsigned format)
>>> +{
>>> +    drmModeModeInfo *mode = igt_output_get_mode(output);
>>> +    igt_display_t *display = &data->display;
>>> +    igt_plane_t *plane;
>>> +    int i;
>>> +    igt_crc_t ref_crc[4], crc;
>>> +    bool valid[4] = {};
>>> +
>>> +    prepare_crtc(data, pipe, output);
>>> +
>>> +    igt_create_pattern_fb(display->drm_fd, mode->hdisplay, mode->vdisplay,
>>> +                  format, x_modifier, &data->fb[0]);
>>> +
>>> +    igt_create_pattern_fb(display->drm_fd, mode->vdisplay, mode->hdisplay,
>>> +                  format, y_modifier, &data->fb[1]);
>>> +
>>> +    igt_create_pattern_fb(display->drm_fd, mode->hdisplay, mode->vdisplay,
>>> +                  DRM_FORMAT_XRGB8888, x_modifier, &data->fb[2]);
>>> +
>>> +    igt_create_pattern_fb(display->drm_fd, mode->vdisplay, mode->hdisplay,
>>> +                  DRM_FORMAT_XRGB8888, y_modifier, &data->fb[3]);
>>> +
>>> +    for_each_plane_on_pipe(display, pipe, plane) {
>>> +        const int seq[] = {
>>> +            2, 0, 2, 1, 3, 1, 3
>>> +        };
>>> +
>>> +        if (!igt_plane_has_format_mod(plane, format, data->fb[0].modifier))
>>> +            continue;
>>> +
>>> +        /* Collect reference crc with toggle in between. */
>>> +        for (i = 0; i < ARRAY_SIZE(ref_crc); i++) {
>>> +            igt_rotation_t rot = (i == 1 || i == 3) ?
>>> +                IGT_ROTATION_90 : IGT_ROTATION_0;
>>> +
>>> +            set_fb(plane, &data->fb[i], scaled, rot);
>>> +
>>> +            igt_display_commit2(display, COMMIT_ATOMIC);
>>> +            igt_pipe_crc_collect_crc(data->pipe_crc, &ref_crc[i]);
>>> +            valid[i] = true;
>>> +
>>> +            set_fb(plane, NULL, scaled, rot);
>>> +            igt_display_commit2(display, COMMIT_ATOMIC);
>>> +        }
>>> +
>>> +        for (i = 0; i < ARRAY_SIZE(seq); i++) {
>>> +            int j = seq[i];
>>> +            igt_rotation_t rot = (j == 1 || j == 3) ?
>>> +                IGT_ROTATION_90 : IGT_ROTATION_0;
>>> +
>>> +            if (!valid[j])
>>> +                continue;
>>> +
>>> +            set_fb(plane, &data->fb[j], scaled, rot);
>>> +            igt_display_commit2(display, COMMIT_ATOMIC);
>>> +
>>> +            igt_pipe_crc_collect_crc(data->pipe_crc, &crc);
>>> +            igt_assert_crc_equal(&ref_crc[j], &crc);
>>> +        }
>>> +
>>> +        /* We only have few scalers, don't use 1 for unused planes */
>>> +        igt_plane_set_fb(plane, NULL);
>>> +    }
>>> +}
>>> +
>>> +#define assert_collected_crc_equal(pipe_crc, crc, ref_crc) \
>>> +    do { \
>>> +        igt_pipe_crc_get_current(display->drm_fd, pipe_crc, crc);    \
>>> +        igt_assert_crc_equal(ref_crc, crc);    \
>>> +    } while (0)
>>> +
>>> +static void set_src_coords(igt_plane_t *plane, struct igt_fb *fb,
>>> +               igt_rotation_t rot, int vis, int hidden)
>>> +{
>>> +    switch (rot) {
>>> +    case IGT_ROTATION_0:
>>> +        igt_fb_set_position(fb, plane, fb->width / 2 - vis, fb->height / 2 - vis);
>>> +        break;
>>> +    case IGT_ROTATION_90:
>>> +        igt_fb_set_position(fb, plane, fb->width / 2 - hidden, fb->height / 2 - vis);
>>> +        break;
>>> +    case IGT_ROTATION_180:
>>> +        igt_fb_set_position(fb, plane, fb->width / 2 - hidden, fb->height / 2 - hidden);
>>> +        break;
>>> +    case IGT_ROTATION_270:
>>> +        igt_fb_set_position(fb, plane, fb->width / 2 - vis, fb->height / 2 - hidden);
>>> +        break;
>>> +    default: igt_assert(0);
>> ^^
>> This was the only nit I noticed.
>
> What's wrong there? Or do you mean putting it on a newline?
yes, default case on its own line but its just minor type detail.

/Juha-Pekka