Re: [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 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

[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux