On Mon, Sep 30, 2019 at 10:18:17PM -0400, Martin Peres wrote:
> 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.
Just have to make sure that plane is the cursor since the blending
fail on the universal planes.
>
> - 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.
Third option would be to use the port crc instead.
>
> 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);
> > + }
> > }
> >
>
[-- Error: unable to create PGP subprocess! --]
> _______________________________________________
> igt-dev mailing list
> igt-dev@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/igt-dev
--
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
