[igt-dev] [i-g-t] tests: Remove kms_crtc_background_color test
Matt Roper
matthew.d.roper at intel.com
Thu Sep 3 01:53:12 UTC 2020
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>
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