Re: [igt-dev] [PATCH i-g-t] tests/kms_crtc_background_color: overhaul to match upstream ABI (v5.1)

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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-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
> _______________________________________________
> igt-dev mailing list
> igt-dev@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/igt-dev

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux