[igt-dev] [i-g-t] tests: Remove kms_crtc_background_color test
Laxminarayan Bharadiya, Pankaj
pankaj.laxminarayan.bharadiya at intel.com
Wed Oct 7 08:56:08 UTC 2020
> -----Original Message-----
> From: Matt Roper <matthew.d.roper at intel.com>
> Sent: 03 September 2020 07:23
> To: Peres, Martin <martin.peres at intel.com>
> Cc: Laxminarayan Bharadiya, Pankaj
> <pankaj.laxminarayan.bharadiya at intel.com>; Sharma, Swati2
> <swati2.sharma at intel.com>; igt-dev at 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 at intel.com>
> > >> Sent: 02 September 2020 13:59
> > >> To: Laxminarayan Bharadiya, Pankaj
> > >> <pankaj.laxminarayan.bharadiya at intel.com>;
> > >> igt-dev at lists.freedesktop.org; Martin Peres
> > >> <martin.peres at 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 at intel.com>
Can this patch be merged please?
Thanks,
Pankaj
>
>
> Matt
>
> >
> > Acked-by: Martin Peres <martin.peres at linux.intel.com>
> >
> > Thanks,
> > Martin
> >
> > >
> > > Thanks,
> > > Pankaj
> > >
> > >>
> > >> Adding Martin Peres.
> > >>>
> > >>> Signed-off-by: Pankaj Bharadiya
> > >>> <pankaj.laxminarayan.bharadiya at 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
More information about the igt-dev
mailing list