On 30/09/2019 19:13, Matt Roper wrote:
> CRTC background color kernel patches were written about 2.5 years ago
> and floated on the upstream mailing list, but since no opensource
> userspace materialized, we never actually merged them. However the
> corresponding IGT test did get merged and has basically been dead code
> ever since.
>
> A couple years later we finally have an open source userspace
> (ChromeOS), so lets update the IGT test to match the ABI that's actually
> going upstream and to remove some of the cruft from the original test
> that wouldn't actually work.
>
> It's worth noting that CRC's don't seem to work properly on Intel gen9.
> The bspec does tell us that we must have one plane enabled before taking
> a pipe-level ("dmux") CRC, so this test is violating that by disabling
> all planes; it's quite possible that this is the source of the CRC
> mismatch. If running on an Intel platform, you may want to run in
> interactive mode ("--interactive-debug=bgcolor --skip-crc-compare") to
> ensure that the colors being generated actually do match visually since
> the CRC's can't be trusted.
Hmm, landing a feature without automating testing for it is a bit too
much to ask IMO.
I have two proposals to make it happen:
- Could we add a workaround for the affected intel platforms? If the
problem really is because no planes are enabled, then surely a
fully-transparent plane would be a sufficient workaround.
- If CRCs really cannot be used for this, then we should use the
chamelium for it. The idea would be to detect if the connector is
connected to a chamelium, and if so use chamelium's CRC.
How does this sound?
Martin
>
> v2:
> - Swap red and blue ordering in property value to reflect change
> in v2 of kernel series.
>
> v3:
> - Minor updates to proposed uapi helpers (s/rgba/argb/).
>
> v4:
> - General restructuring into pipe/color subtests.
> - Use RGB2101010 framebuffers for comparison so that we match the bits
> of precision that Intel hardware background color accepts
>
> v5:
> - Re-enable CRC comparisons; just because these don't work on Intel
> doesn't mean we shouldn't use them on other vendors' platforms
> (Daniel).
> - Use DRIVER_ANY rather than DRIVER_INTEL. (Heiko Stuebner)
>
> v5.1:
> - Update commit message body discussion of CRC issues.
>
> Cc: igt-dev@xxxxxxxxxxxxxxxxxxxxx
> Cc: Daniel Vetter <daniel@xxxxxxxx>
> Cc: Heiko Stuebner <heiko@xxxxxxxxx>
> Signed-off-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
> ---
> lib/igt_kms.c | 2 +-
> tests/kms_crtc_background_color.c | 263 ++++++++++++++----------------
> 2 files changed, 127 insertions(+), 138 deletions(-)
>
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index e9b80b9b..9a7f0e23 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -391,7 +391,7 @@ const char * const igt_plane_prop_names[IGT_NUM_PLANE_PROPS] = {
> };
>
> const char * const igt_crtc_prop_names[IGT_NUM_CRTC_PROPS] = {
> - [IGT_CRTC_BACKGROUND] = "background_color",
> + [IGT_CRTC_BACKGROUND] = "BACKGROUND_COLOR",
> [IGT_CRTC_CTM] = "CTM",
> [IGT_CRTC_GAMMA_LUT] = "GAMMA_LUT",
> [IGT_CRTC_GAMMA_LUT_SIZE] = "GAMMA_LUT_SIZE",
> diff --git a/tests/kms_crtc_background_color.c b/tests/kms_crtc_background_color.c
> index 3df3401f..58cdd7a9 100644
> --- a/tests/kms_crtc_background_color.c
> +++ b/tests/kms_crtc_background_color.c
> @@ -25,164 +25,153 @@
> #include "igt.h"
> #include <math.h>
>
> -
> IGT_TEST_DESCRIPTION("Test crtc background color feature");
>
> +/*
> + * Paints a desired color into a full-screen primary plane and then compares
> + * that CRC with turning off all planes and setting the CRTC background to the
> + * same color.
> + *
> + * At least on current Intel platforms, the CRC tests appear to always fail,
> + * even though the resulting pipe output seems to be the same. The bspec tells
> + * us that we must have at least one plane turned on when taking a pipe-level
> + * CRC, so the fact that we're violating that may explain the failures. If
> + * your platform gives failures for all tests, you may want to run with
> + * --interactive-debug=bgcolor --skip-crc-compare to visually inspect that the
> + * background color matches the equivalent solid plane.
> + */
> +
> typedef struct {
> - int gfx_fd;
> igt_display_t display;
> - struct igt_fb fb;
> - igt_crc_t ref_crc;
> + int gfx_fd;
> + igt_output_t *output;
> igt_pipe_crc_t *pipe_crc;
> + drmModeModeInfo *mode;
> } data_t;
>
> -#define BLACK 0x000000 /* BGR 8bpc */
> -#define CYAN 0xFFFF00 /* BGR 8bpc */
> -#define PURPLE 0xFF00FF /* BGR 8bpc */
> -#define WHITE 0xFFFFFF /* BGR 8bpc */
> -
> -#define BLACK64 0x000000000000 /* BGR 16bpc */
> -#define CYAN64 0xFFFFFFFF0000 /* BGR 16bpc */
> -#define PURPLE64 0xFFFF0000FFFF /* BGR 16bpc */
> -#define YELLOW64 0x0000FFFFFFFF /* BGR 16bpc */
> -#define WHITE64 0xFFFFFFFFFFFF /* BGR 16bpc */
> -#define RED64 0x00000000FFFF /* BGR 16bpc */
> -#define GREEN64 0x0000FFFF0000 /* BGR 16bpc */
> -#define BLUE64 0xFFFF00000000 /* BGR 16bpc */
> -
> -static void
> -paint_background(data_t *data, struct igt_fb *fb, drmModeModeInfo *mode,
> - uint32_t background, double alpha)
> +/*
> + * Local copy of kernel uapi
> + */
> +static inline __u64
> +local_argb(__u8 bpc, __u16 alpha, __u16 red, __u16 green, __u16 blue)
> {
> - cairo_t *cr;
> - int w, h;
> - double r, g, b;
> -
> - w = mode->hdisplay;
> - h = mode->vdisplay;
> -
> - cr = igt_get_cairo_ctx(data->gfx_fd, &data->fb);
> + int msb_shift = 16 - bpc;
>
> - /* Paint with background color */
> - r = (double) (background & 0xFF) / 255.0;
> - g = (double) ((background & 0xFF00) >> 8) / 255.0;
> - b = (double) ((background & 0xFF0000) >> 16) / 255.0;
> - igt_paint_color_alpha(cr, 0, 0, w, h, r, g, b, alpha);
> -
> - igt_put_cairo_ctx(data->gfx_fd, &data->fb, cr);
> + return (__u64)alpha << msb_shift << 48 |
> + (__u64)red << msb_shift << 32 |
> + (__u64)green << msb_shift << 16 |
> + (__u64)blue << msb_shift;
> }
>
> -static void prepare_crtc(data_t *data, igt_output_t *output, enum pipe pipe,
> - igt_plane_t *plane, int opaque_buffer, int plane_color,
> - uint64_t pipe_background_color)
> +static void test_background(data_t *data, enum pipe pipe, int w, int h,
> + __u64 r, __u64 g, __u64 b)
> {
> - drmModeModeInfo *mode;
> - igt_display_t *display = &data->display;
> - int fb_id;
> - double alpha;
> -
> - igt_output_set_pipe(output, pipe);
> -
> - /* create the pipe_crc object for this pipe */
> - igt_pipe_crc_free(data->pipe_crc);
> - data->pipe_crc = igt_pipe_crc_new(data->gfx_fd, pipe, INTEL_PIPE_CRC_SOURCE_AUTO);
> -
> - mode = igt_output_get_mode(output);
> -
> - fb_id = igt_create_fb(data->gfx_fd,
> - mode->hdisplay, mode->vdisplay,
> - DRM_FORMAT_XRGB8888,
> - LOCAL_DRM_FORMAT_MOD_NONE, /* tiled */
> - &data->fb);
> - igt_assert(fb_id);
> -
> - /* To make FB pixel win with background color, set alpha as full opaque */
> - igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, pipe_background_color);
> - if (opaque_buffer)
> - alpha = 1.0; /* alpha 1 is fully opque */
> - else
> - alpha = 0.0; /* alpha 0 is fully transparent */
> - paint_background(data, &data->fb, mode, plane_color, alpha);
> -
> - igt_plane_set_fb(plane, &data->fb);
> - igt_display_commit2(display, COMMIT_UNIVERSAL);
> -}
> -
> -static void cleanup_crtc(data_t *data, igt_output_t *output, igt_plane_t *plane)
> -{
> - igt_display_t *display = &data->display;
> -
> - igt_pipe_crc_free(data->pipe_crc);
> - data->pipe_crc = NULL;
> -
> - igt_remove_fb(data->gfx_fd, &data->fb);
> -
> - igt_pipe_obj_set_prop_value(plane->pipe, IGT_CRTC_BACKGROUND, BLACK64);
> + igt_crc_t plane_crc, bg_crc;
> + struct igt_fb colorfb;
> + igt_plane_t *plane = igt_output_get_plane_type(data->output,
> + DRM_PLANE_TYPE_PRIMARY);
> +
> +
> + /* Fill the primary plane and set the background to the same color */
> + igt_create_color_fb(data->gfx_fd, w, h, DRM_FORMAT_XRGB2101010,
> + LOCAL_DRM_FORMAT_MOD_NONE,
> + (double)r / 0xFFFF,
> + (double)g / 0xFFFF,
> + (double)b / 0xFFFF,
> + &colorfb);
> + igt_plane_set_fb(plane, &colorfb);
> + igt_pipe_set_prop_value(&data->display, pipe, IGT_CRTC_BACKGROUND,
> + local_argb(16, 0xffff, r, g, b));
> + igt_display_commit2(&data->display, COMMIT_ATOMIC);
> + igt_pipe_crc_collect_crc(data->pipe_crc, &plane_crc);
> + igt_debug_wait_for_keypress("bgcolor");
> +
> + /* Turn off the primary plane so only bg shows */
> igt_plane_set_fb(plane, NULL);
> - igt_output_set_pipe(output, PIPE_ANY);
> -
> - igt_display_commit2(display, COMMIT_UNIVERSAL);
> + igt_display_commit2(&data->display, COMMIT_ATOMIC);
> + igt_pipe_crc_collect_crc(data->pipe_crc, &bg_crc);
> + igt_debug_wait_for_keypress("bgcolor");
> +
> + /*
> + * The following test relies on hardware that generates valid CRCs
> + * even when no planes are on. Sadly, this doesn't appear to be the
> + * case for current Intel platforms; pipe CRC's never match bgcolor
> + * CRC's, likely because we're violating the bspec's guidance that there
> + * must always be at least one real plane turned on when taking the
> + * pipe-level ("dmux") CRC.
> + */
> + igt_assert_crc_equal(&plane_crc, &bg_crc);
> }
>
> -static void test_crtc_background(data_t *data)
> +igt_main
> {
> - igt_display_t *display = &data->display;
> + data_t data = {};
> igt_output_t *output;
> + drmModeModeInfo *mode;
> + int w, h;
> enum pipe pipe;
> - int valid_tests = 0;
> -
> - for_each_pipe_with_valid_output(display, pipe, output) {
> - igt_plane_t *plane;
> -
> - igt_output_set_pipe(output, pipe);
> -
> - plane = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY);
> - igt_require(igt_pipe_has_prop(display, pipe, IGT_CRTC_BACKGROUND));
> -
> - prepare_crtc(data, output, pipe, plane, 1, PURPLE, BLACK64);
> -
> - /* Now set background without using a plane, i.e.,
> - * Disable the plane to let hw background color win blend. */
> - igt_plane_set_fb(plane, NULL);
> - igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, PURPLE64);
> - igt_display_commit2(display, COMMIT_UNIVERSAL);
> -
> - /* Try few other background colors */
> - igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, CYAN64);
> - igt_display_commit2(display, COMMIT_UNIVERSAL);
> -
> - igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, YELLOW64);
> - igt_display_commit2(display, COMMIT_UNIVERSAL);
>
> - igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, RED64);
> - igt_display_commit2(display, COMMIT_UNIVERSAL);
> + igt_fixture {
> + data.gfx_fd = drm_open_driver_master(DRIVER_ANY);
> + kmstest_set_vt_graphics_mode();
>
> - igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, GREEN64);
> - igt_display_commit2(display, COMMIT_UNIVERSAL);
> -
> - igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, BLUE64);
> - igt_display_commit2(display, COMMIT_UNIVERSAL);
> -
> - igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, WHITE64);
> - igt_display_commit2(display, COMMIT_UNIVERSAL);
> -
> - valid_tests++;
> - cleanup_crtc(data, output, plane);
> + igt_require_pipe_crc(data.gfx_fd);
> + igt_display_require(&data.display, data.gfx_fd);
> }
> - igt_require_f(valid_tests, "no valid crtc/connector combinations found\n");
> -}
>
> -igt_simple_main
> -{
> - data_t data = {};
> -
> - igt_skip_on_simulation();
> -
> - data.gfx_fd = drm_open_driver(DRIVER_INTEL);
> - igt_require_pipe_crc(data.gfx_fd);
> - igt_display_require(&data.display, data.gfx_fd);
> -
> - test_crtc_background(&data);
> + for_each_pipe_static(pipe) igt_subtest_group {
> + igt_fixture {
> + igt_display_require_output_on_pipe(&data.display, pipe);
> + igt_require(igt_pipe_has_prop(&data.display, pipe,
> + IGT_CRTC_BACKGROUND));
> +
> + output = igt_get_single_output_for_pipe(&data.display,
> + pipe);
> + igt_output_set_pipe(output, pipe);
> + data.output = output;
> +
> + mode = igt_output_get_mode(output);
> + w = mode->hdisplay;
> + h = mode->vdisplay;
> +
> + data.pipe_crc = igt_pipe_crc_new(data.gfx_fd, pipe,
> + INTEL_PIPE_CRC_SOURCE_AUTO);
> + }
> +
> + igt_subtest_f("background-color-pipe-%s-black",
> + kmstest_pipe_name(pipe))
> + test_background(&data, pipe, w, h,
> + 0, 0, 0);
> +
> + igt_subtest_f("background-color-pipe-%s-white",
> + kmstest_pipe_name(pipe))
> + test_background(&data, pipe, w, h,
> + 0xFFFF, 0xFFFF, 0xFFFF);
> +
> + igt_subtest_f("background-color-pipe-%s-red",
> + kmstest_pipe_name(pipe))
> + test_background(&data, pipe, w, h,
> + 0xFFFF, 0, 0);
> +
> + igt_subtest_f("background-color-pipe-%s-green",
> + kmstest_pipe_name(pipe))
> +
> + test_background(&data, pipe, w, h,
> + 0, 0xFFFF, 0);
> +
> + igt_subtest_f("background-color-pipe-%s-blue",
> + kmstest_pipe_name(pipe))
> + test_background(&data, pipe, w, h,
> + 0, 0, 0xFFFF);
> +
> + igt_fixture {
> + igt_display_reset(&data.display);
> + igt_pipe_crc_free(data.pipe_crc);
> + data.pipe_crc = NULL;
> + }
> + }
>
> - igt_display_fini(&data.display);
> + igt_fixture {
> + igt_display_fini(&data.display);
> + }
> }
>
Attachment:
pEpkey.asc
Description: application/pgp-keys
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
