On Tue, Oct 01, 2019 at 03:27:41PM +0300, Ville Syrjälä wrote:
> 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.
Yeah that might be a w/a, we're using that on gen4 already for some ports.
We'd have to use these port crcs always though, since we can't tell
upfront whether the testcase is testing the background color.
-Daniel
>
> >
> > 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-...@lists.freedesktop.org
> > > Cc: Daniel Vetter <dan...@ffwll.ch>
> > > Cc: Heiko Stuebner <he...@sntech.de>
> > > Signed-off-by: Matt Roper <matthew.d.ro...@intel.com>
> > > ---
> > > 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-...@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/igt-dev
>
>
> --
> Ville Syrjälä
> Intel
> _______________________________________________
> igt-dev mailing list
> igt-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
