Re: [igt-dev] [i-g-t] tests: Remove kms_crtc_background_color test

["Laxminarayan Bharadiya, Pankaj" <pankaj.laxminarayan.bharadiya@intel.com>]

> -----Original Message-----
> From: Matt Roper <matthew.d.roper@intel.com>
> Sent: 03 September 2020 07:23
> To: Peres, Martin <martin.peres@intel.com>
> Cc: Laxminarayan Bharadiya, Pankaj
> <pankaj.laxminarayan.bharadiya@intel.com>; Sharma, Swati2
> <swati2.sharma@intel.com>; igt-dev@lists.freedesktop.org
> Subject: Re: [igt-dev] [i-g-t] tests: Remove kms_crtc_background_color test
> 
> On Wed, Sep 02, 2020 at 04:58:35AM -0700, Peres, Martin wrote:
> > +Matt
> >
> > Hi everyone,
> >
> > On 2020-09-02 11:34, Laxminarayan Bharadiya, Pankaj wrote:
> > >> -----Original Message-----
> > >> From: Sharma, Swati2 <swati2.sharma@intel.com>
> > >> Sent: 02 September 2020 13:59
> > >> To: Laxminarayan Bharadiya, Pankaj
> > >> <pankaj.laxminarayan.bharadiya@intel.com>;
> > >> igt-dev@lists.freedesktop.org; Martin Peres
> > >> <martin.peres@linux.intel.com>
> > >> Subject: Re: [igt-dev] [i-g-t] tests: Remove
> > >> kms_crtc_background_color test
> > >>
> > >>
> > >>
> > >> On 28-Jul-20 8:53 PM, Pankaj Bharadiya wrote:
> > >>> BACKGROUND_COLOR property is not supported in kernel as of now.
> > >>> Following patch attempted to add support but never got merged due
> > >>> to lack of userspace support.
> > >>>
> > >>>
> https://patchwork.freedesktop.org/patch/333632/?series=67424&rev=1
> > >>>
> > >>> This test case is always skipped, as it does not find the
> > >>> BACKGROUND_COLOR prop support hence remove it.
> > >>
> > >> Why we have to remove the test? What if its supported in kernel in
> future?
> > >> If test is skipping; we can add it to blacklist (listing it as
> > >> expected
> > >> skip)
> > >
> > > IMO, first of all, this code should not have been added unless kernel
> supports it.
> > > It's a dead code.
> > >
> > > I don’t think there a plan to add BACKGROUND_COLOR property in
> > > kernel since we don't have any reference user space implantation which
> uses/need this property.
> >
> > I tend to agree with Pankaj here. The code won't be gone, and this
> > patch can be reverted when the property lands in at least one DRM driver.
> >
> > That being said, please make sure with Matt that no activity is going
> > on around this property before landing this patch. If so, then this patch is:
> 
> Yeah, go ahead and remove it.  There is software out there that uses the
> property, but it's all closed-source software, so the kernel support has never
> been upstreamable and only exists in product group kernels.
> 
> It seems every couple years someone comes along and shows interest in
> using the functionality in open source userspace, so we put effort into
> refreshing the patches and resubmitting them upstream, but ultimately the
> userspace efforts fizzle out and the kernel code goes back on ice.
> I'm not aware of any specific interest at the moment, and if this does get
> resurrected again in the future we can just write new IGT support for it then
> (my last couple upstream respins also included complete overhauls of the
> IGT support, so the old snapshot that's already present in upsteam IGT isn't
> even really useful if the kernel support does come back).
> 
> For the removal:
> Acked-by: Matt Roper <matthew.d.roper@intel.com>

Can this patch be merged please?

Thanks,
Pankaj
> 
> 
> Matt
> 
> >
> > Acked-by: Martin Peres <martin.peres@linux.intel.com>
> >
> > Thanks,
> > Martin
> >
> > >
> > > Thanks,
> > > Pankaj
> > >
> > >>
> > >> Adding Martin Peres.
> > >>>
> > >>> Signed-off-by: Pankaj Bharadiya
> > >>> <pankaj.laxminarayan.bharadiya@intel.com>
> > >>> ---
> > >>>   lib/igt_kms.c                     |   1 -
> > >>>   lib/igt_kms.h                     |   3 +-
> > >>>   tests/Makefile.sources            |   1 -
> > >>>   tests/kms_crtc_background_color.c | 186 ------------------------------
> > >>>   tests/meson.build                 |   1 -
> > >>>   5 files changed, 1 insertion(+), 191 deletions(-)
> > >>>   delete mode 100644 tests/kms_crtc_background_color.c
> > >>>
> > >>> diff --git a/lib/igt_kms.c b/lib/igt_kms.c index
> > >>> f57972f19..003f6af7b
> > >>> 100644
> > >>> --- a/lib/igt_kms.c
> > >>> +++ b/lib/igt_kms.c
> > >>> @@ -398,7 +398,6 @@ 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_CTM] = "CTM",
> > >>>   	[IGT_CRTC_GAMMA_LUT] = "GAMMA_LUT",
> > >>>   	[IGT_CRTC_GAMMA_LUT_SIZE] = "GAMMA_LUT_SIZE", diff --git
> > >>> a/lib/igt_kms.h b/lib/igt_kms.h index 26dc9f5fb..954f7be52 100644
> > >>> --- a/lib/igt_kms.h
> > >>> +++ b/lib/igt_kms.h
> > >>> @@ -96,8 +96,7 @@ void kmstest_restore_vt_mode(void);
> > >>>   void kmstest_set_vt_text_mode(void);
> > >>>
> > >>>   enum igt_atomic_crtc_properties {
> > >>> -       IGT_CRTC_BACKGROUND = 0,
> > >>> -       IGT_CRTC_CTM,
> > >>> +       IGT_CRTC_CTM = 0,
> > >>>          IGT_CRTC_GAMMA_LUT,
> > >>>          IGT_CRTC_GAMMA_LUT_SIZE,
> > >>>          IGT_CRTC_DEGAMMA_LUT,
> > >>> diff --git a/tests/Makefile.sources b/tests/Makefile.sources index
> > >>> 93d7768c4..994011700 100644
> > >>> --- a/tests/Makefile.sources
> > >>> +++ b/tests/Makefile.sources
> > >>> @@ -40,7 +40,6 @@ TESTS_progs = \
> > >>>   	kms_ccs \
> > >>>   	kms_concurrent \
> > >>>   	kms_content_protection\
> > >>> -	kms_crtc_background_color \
> > >>>   	kms_cursor_crc \
> > >>>   	kms_cursor_edge_walk \
> > >>>   	kms_cursor_legacy \
> > >>> diff --git a/tests/kms_crtc_background_color.c
> > >>> b/tests/kms_crtc_background_color.c
> > >>> deleted file mode 100644
> > >>> index b4141b0df..000000000
> > >>> --- a/tests/kms_crtc_background_color.c
> > >>> +++ /dev/null
> > >>> @@ -1,186 +0,0 @@
> > >>> -/*
> > >>> - * Copyright © 2013,2014 Intel Corporation
> > >>> - *
> > >>> - * Permission is hereby granted, free of charge, to any person
> > >>> obtaining a
> > >>> - * copy of this software and associated documentation files (the
> > >>> "Software"),
> > >>> - * to deal in the Software without restriction, including without
> > >>> limitation
> > >>> - * the rights to use, copy, modify, merge, publish, distribute,
> > >>> sublicense,
> > >>> - * and/or sell copies of the Software, and to permit persons to
> > >>> whom the
> > >>> - * Software is furnished to do so, subject to the following conditions:
> > >>> - *
> > >>> - * The above copyright notice and this permission notice
> > >>> (including the next
> > >>> - * paragraph) shall be included in all copies or substantial
> > >>> portions of the
> > >>> - * Software.
> > >>> - *
> > >>> - * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY
> > >>> KIND, EXPRESS OR
> > >>> - * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> > >>> MERCHANTABILITY,
> > >>> - * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.
> IN NO
> > >> EVENT
> > >>> SHALL
> > >>> - * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
> > >> DAMAGES
> > >>> OR OTHER
> > >>> - * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
> > >>> OTHERWISE, ARISING
> > >>> - * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
> USE OR
> > >>> OTHER DEALINGS
> > >>> - * IN THE SOFTWARE.
> > >>> - *
> > >>> - */
> > >>> -
> > >>> -#include "igt.h"
> > >>> -#include <math.h>
> > >>> -
> > >>> -
> > >>> -IGT_TEST_DESCRIPTION("Test crtc background color feature");
> > >>> -
> > >>> -typedef struct {
> > >>> -	int gfx_fd;
> > >>> -	igt_display_t display;
> > >>> -	struct igt_fb fb;
> > >>> -	igt_crc_t ref_crc;
> > >>> -	igt_pipe_crc_t *pipe_crc;
> > >>> -} 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)
> > >>> -{
> > >>> -	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);
> > >>> -
> > >>> -	/* 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(cr);
> > >>> -}
> > >>> -
> > >>> -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)
> > >>> -{
> > >>> -	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_plane_set_fb(plane, NULL);
> > >>> -	igt_output_set_pipe(output, PIPE_ANY);
> > >>> -
> > >>> -	igt_display_commit2(display, COMMIT_UNIVERSAL);
> > >>> -}
> > >>> -
> > >>> -static void test_crtc_background(data_t *data) -{
> > >>> -	igt_display_t *display = &data->display;
> > >>> -	igt_output_t *output;
> > >>> -	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_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_f(valid_tests, "no valid crtc/connector combinations
> > >> found\n");
> > >>> -}
> > >>> -
> > >>> -igt_simple_main
> > >>> -{
> > >>> -	data_t data = {};
> > >>> -
> > >>> -	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);
> > >>> -
> > >>> -	igt_display_fini(&data.display);
> > >>> -}
> > >>> diff --git a/tests/meson.build b/tests/meson.build index
> > >>> ca792ed86..a6e1b7fae 100644
> > >>> --- a/tests/meson.build
> > >>> +++ b/tests/meson.build
> > >>> @@ -24,7 +24,6 @@ test_progs = [
> > >>>   	'kms_ccs',
> > >>>   	'kms_concurrent',
> > >>>   	'kms_content_protection',
> > >>> -	'kms_crtc_background_color',
> > >>>   	'kms_cursor_crc',
> > >>>   	'kms_cursor_edge_walk',
> > >>>   	'kms_cursor_legacy',
> > >>>
> > >>
> > >> --
> > >> ~Swati Sharma
> >
> 
> 
> 
> --
> Matt Roper
> Graphics Software Engineer
> VTT-OSGC Platform Enablement
> Intel Corporation
> (916) 356-2795
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev