[igt-dev] [PATCH i-g-t 1/2] tests/kms_dither: New IGT to validate crtc Dithering
Modem, Bhanuprakash
bhanuprakash.modem at intel.com
Thu Jul 15 04:00:08 UTC 2021
> From: Mark Yacoub <markyacoub at chromium.org>
> Sent: Monday, July 12, 2021 8:27 PM
> To: Harry Wentland <harry.wentland at amd.com>
> Cc: Varide, Nischal <nischal.varide at intel.com>; Modem, Bhanuprakash
> <bhanuprakash.modem at intel.com>; igt-dev at lists.freedesktop.org; Arkadiusz Hiler
> <arek at hiler.eu>; Sean Paul <seanpaul at chromium.org>; Siqueira, Rodrigo
> <Rodrigo.Siqueira at amd.com>
> Subject: Re: [igt-dev] [PATCH i-g-t 1/2] tests/kms_dither: New IGT to validate
> crtc Dithering
>
> On Mon, Jul 12, 2021 at 9:35 AM Harry Wentland <harry.wentland at amd.com> wrote:
> >
> >
> >
> > On 2021-07-08 12:05 p.m., Varide, Nischal wrote:
> > >
> > >
> > > -----Original Message-----
> > > From: Modem, Bhanuprakash <bhanuprakash.modem at intel.com>
> > > Sent: Friday, July 9, 2021 2:04 AM
> > > To: igt-dev at lists.freedesktop.org
> > > Cc: Modem, Bhanuprakash <bhanuprakash.modem at intel.com>; Sharma, Swati2
> <swati2.sharma at intel.com>; Shankar, Uma <uma.shankar at intel.com>; Varide,
> Nischal <nischal.varide at intel.com>
> > > Subject: [PATCH i-g-t 1/2] tests/kms_dither: New IGT to validate crtc
> Dithering
> > >
> > > Many of the display devices have color format support less than the color
> format of the input data (Ex: 8-bit input data and 6-bit display color depth).
> Then the input data will be either truncated or rounded, but this approach
> usually causes loss of detail and also produce large banded areas of a single
> color that significantly differs from the original image.
> > >
> > > Dithering is a technique used to enhance these colors by creating the
> illusion of smoothness by adjusting the nearby pixel color.
> > > For Eg: Converting 8-bit to 6-bit by modifying the pixel information.
> > >
> > > Dithering should be enabled when a panel color depth is lower than the
> color depth of the framebuffer.
> > >
> > > Even though feature Dithering is there since legacy, there is no specific
> IGT for this. This patch will create New IGT for Dithering.
> > > And below is the truth table for CRTC Dithering.
> > >
> > > |----------------|---------|-------------|
> > > | Frame buffer | Panel | Dithering |
> > > |----------------|---------|-------------|
> > > | 8 BPC | 6 BPC | Yes |
> > > |----------------|---------|-------------|
> > > | 8 BPC | 8 BPC | No |
> > > |----------------|---------|-------------|
> > > | 8 BPC | 10 BPC | No |
> > > |----------------|---------|-------------|
> > > | 10 BPC | 6 BPC | Yes |
> > > |----------------|---------|-------------|
> > > | 10 BPC | 8 BPC | Yes |
> > > |----------------|---------|-------------|
> > > | 10 BPC | 10 BPC | No |
> > > |----------------|---------|-------------|
> > > | 16 BPC | 6 BPC | Yes |
> > > |----------------|---------|-------------|
> > > | 16 BPC | 8 BPC | Yes |
> > > |----------------|---------|-------------|
> > > | 16 BPC | 10 BPC | Yes |
> > > |----------------|---------|-------------|
> > >
> > > As i915 enables dithering for 6 bpc panels only, we need to limit the
> execution to 6 & 8 bpc panels (Negative testing) only.
> > >
> > > Cc: Swati Sharma <swati2.sharma at intel.com>
> > > Cc: Uma Shankar <uma.shankar at intel.com>
> > > Cc: Nischal Varide <nischal.varide at intel.com>
> > > Signed-off-by: Bhanuprakash Modem <bhanuprakash.modem at intel.com>
> > > Reviewed-by: Nischal Varide <nischal.varide at intel.com>
> > > ---
> > > tests/kms_dither.c | 253 +++++++++++++++++++++++++++++++++++++++++++++
> > > tests/meson.build | 1 +
> > > 2 files changed, 254 insertions(+)
> > > create mode 100644 tests/kms_dither.c
> > >
> > > diff --git a/tests/kms_dither.c b/tests/kms_dither.c new file mode 100644
> index 0000000000..0e0c61dea2
> > > --- /dev/null
> > > +++ b/tests/kms_dither.c
> > > @@ -0,0 +1,253 @@
> > > +/*
> > > + * Copyright © 2020 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 COPYRIGHT HOLDER(S) OR AUTHOR(S) 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.
> > > + *
> > > + * Authors:
> > > + * Bhanuprakash Modem <bhanuprakash.modem at intel.com>
> > > + *
> > > + */
> > > +
> > > +#include "igt.h"
> > > +#include <fcntl.h>
> > > +#include <termios.h>
> > > +#include <unistd.h>
> > > +
> > > +IGT_TEST_DESCRIPTION("Test Dithering block status");
> > > +
> > > +/* Connector BPC */
> > > +#define IGT_CONNECTOR_BPC_6 6
> > > +#define IGT_CONNECTOR_BPC_8 8
> > > +#define IGT_CONNECTOR_BPC_10 10
> > > +#define IGT_CONNECTOR_BPC_12 12
> > > +
> > > +/* Framebuffer BPC */
> > > +#define IGT_FRAME_BUFFER_BPC_8 8
> > > +#define IGT_FRAME_BUFFER_BPC_10 10
> > > +#define IGT_FRAME_BUFFER_BPC_16 16
> > > +
> > > +/* Common test data. */
> > > +typedef struct data {
> > > + igt_display_t display;
> > > + igt_plane_t *primary;
> > > + igt_output_t *output;
> > > + igt_pipe_t *pipe;
> > > + drmModeModeInfo *mode;
> > > + enum pipe pipe_id;
> > > + int drm_fd;
> > > + igt_fb_t fb;
> > > +} data_t;
> > > +
> > > +typedef struct {
> > > + unsigned int bpc;
> > > + unsigned int dither;
> > > +} dither_status_t;
> > > +
> > > +/* Prepare test data. */
> > > +static void prepare_test(data_t *data, igt_output_t *output, enum pipe
> > > +pipe) {
> > > + igt_display_t *display = &data->display;
> > > +
> > > + data->pipe_id = pipe;
> > > + data->pipe = &data->display.pipes[data->pipe_id];
> > > + igt_assert(data->pipe);
> > > +
> > > + igt_display_reset(display);
> > > +
> > > + data->output = output;
> > > + igt_assert(data->output);
> > > +
> > > + data->mode = igt_output_get_mode(data->output);
> > > + igt_assert(data->mode);
> > > +
> > > + data->primary =
> > > + igt_pipe_get_plane_type(data->pipe, DRM_PLANE_TYPE_PRIMARY);
> > > +
> > > + igt_output_set_pipe(data->output, data->pipe_id); }
> > > +
> > > +/* Returns the current state of dithering from the crtc debugfs. */
> > > +static dither_status_t get_dither_state(data_t *data) {
> > > + char buf[512], tmp[5];
> > > + char *start_loc;
> > > + int dir, res;
> > > + dither_status_t status;
> > > +
> > > + dir = igt_debugfs_dir(data->drm_fd);
> > > + igt_assert(dir >= 0);
> > > +
> > > + res = igt_debugfs_simple_read(dir, "i915_display_info", buf,
> sizeof(buf));
> >
> > This seems to be an i915-specific test. Do those belong in the kms_
> namespace?
> >
> > Could this be made into a generic test?
> >
> > Harry
> >
> > > + igt_require(res > 0);
> > > + close(dir);
> > > +
> > > + igt_assert(start_loc = strstr(buf, ", bpp="));
> > > + igt_assert_eq(sscanf(start_loc, ", bpp=%u", &status.bpc), 1);
> > > + status.bpc /= 3;
> > > +
> > > + igt_assert(start_loc = strstr(buf, ", dither="));
> > > + igt_assert_eq(sscanf(start_loc, ", dither=%s", tmp), 1);
> > > + status.dither = !strcmp(tmp, "yes,");
> > > +
> > > + return status;
> > > +}
> > > +
> > > +static void test_dithering(data_t *data, enum pipe pipe,
> > > + igt_output_t *output,
> > > + int fb_bpc, int fb_format,
> > > + int output_bpc)
> > > +{
> > > + igt_display_t *display = &data->display;
> > > + dither_status_t status;
> > > + int bpc;
> > > +
> > > + igt_info("Dithering test execution on %s PIPE_%s\n",
> > > + output->name, kmstest_pipe_name(pipe));
> > > + prepare_test(data, output, pipe);
> > > +
> > > + igt_assert(igt_create_fb(data->drm_fd, data->mode->hdisplay,
> > > + data->mode->vdisplay, fb_format,
> > > + LOCAL_DRM_FORMAT_MOD_NONE, &data->fb));
> > > + igt_plane_set_fb(data->primary, &data->fb);
> > > + igt_plane_set_size(data->primary, data->mode->hdisplay,
> > > +data->mode->vdisplay);
> > > +
> > > + bpc = igt_output_get_prop(output, IGT_CONNECTOR_MAX_BPC);
> > > + igt_output_set_prop_value(data->output, IGT_CONNECTOR_MAX_BPC,
> output_bpc);
> > > + igt_display_commit2(display, display->is_atomic ? COMMIT_ATOMIC :
> > > +COMMIT_LEGACY);
> > > +
> > > + /*
> > > + * Check the status of Dithering block:
> > > + *
> > > + * Preserve the result & compute later (after clean-up).
> > > + * If fb_bpc is greater than output_bpc, Dithering should be enabled
> > > + * Else disabled
> > > + */
> > > + status = get_dither_state(data);
> > > +
> > > + igt_debug("FB BPC:%d, Panel BPC:%d, Pipe BPC:%d, Expected Dither:%s,
> Actual result:%s\n",
> > > + fb_bpc, output_bpc, status.bpc,
> > > + (fb_bpc > output_bpc) ? "Enable": "Disable",
> > > + status.dither ? "Enable": "Disable");
> > > +
> > > + /*
> > > + * We must update the Connector max_bpc property back
> > > + * Otherwise, previously updated value will stay forever and
> > > + * may cause the failures for next/other subtests.
> > > + */
> > > + igt_output_set_prop_value(data->output, IGT_CONNECTOR_MAX_BPC, bpc);
> > > + igt_plane_set_fb(data->primary, NULL);
> > > + igt_output_set_pipe(output, PIPE_NONE);
> > > + igt_display_commit2(display, display->is_atomic ? COMMIT_ATOMIC :
> COMMIT_LEGACY);
> > > + igt_remove_fb(data->drm_fd, &data->fb);
> > > +
> > > + /* Check if crtc bpc is updated with requested one. */
> > > + igt_require_f((status.bpc == output_bpc),
> > > + "%s can support max %u-bpc, but requested %d-bpc\n",
> > > + output->name, status.bpc, output_bpc);
> > > +
> > > + /* Compute the result. */
> > > + if (fb_bpc > output_bpc)
> > > + igt_assert_f(status.dither, "(fb_%dbpc > output_%dbpc):
> Dither should be enabled\n",
> > > + fb_bpc, output_bpc);
> > > + else
> > > + igt_assert_f(!status.dither, "(fb_%dbpc <= output_%dbpc):
> Dither should be disabled\n",
> > > + fb_bpc, output_bpc);
> > > +
> > > + return;
> > > +}
> > > +
> > > +/* Returns true if an output supports max bpc property & max_bpc >=
> > > +requested. */ static bool is_supported(igt_output_t *output, int bpc) {
> > > + return igt_output_has_prop(output, IGT_CONNECTOR_MAX_BPC) &&
> > > + igt_output_get_prop(output, IGT_CONNECTOR_MAX_BPC); }
> > > +
> > > +static void
> > > +run_dither_test(data_t *data, int fb_bpc, int fb_format, int
> > > +output_bpc) {
> > > + igt_output_t *output;
> > > + igt_display_t *display = &data->display;
> > > +
> > > + for_each_connected_output(display, output) {
> > > + enum pipe pipe;
> > > + drmModeConnector *connector = output->config.connector;
> > > +
> > > + if (!is_supported(output, output_bpc))
> > > + continue;
> > > +
> > > + /* HDMI won't support 6 BPC */
> > > + if (output_bpc == IGT_CONNECTOR_BPC_6 &&
> > > + (connector->connector_type == DRM_MODE_CONNECTOR_HDMIA ||
> > > + connector->connector_type == DRM_MODE_CONNECTOR_HDMIB))
> > > + continue;
> > > +
> > > + for_each_pipe(display, pipe) {
> > > + if (igt_pipe_connector_valid(pipe, output)) {
> > > + igt_dynamic_f("%s-pipe-%s", output->name,
> kmstest_pipe_name(pipe))
> > > + test_dithering(data, pipe, output,
> fb_bpc,
> > > + fb_format,
> output_bpc);
> > > +
> > > + /* One pipe is enough */
> > > + break;
> > > + }
> > > + }
> > > + }
> > > +}
> > > +
> > > +igt_main
> > > +{
> > > + struct {
> > > + int fb_bpc;
> > > + int format;
> > > + int output_bpc;
> > > + } tests[] = {
> > > + { IGT_FRAME_BUFFER_BPC_8, DRM_FORMAT_XRGB8888,
> IGT_CONNECTOR_BPC_6 },
> > > + { IGT_FRAME_BUFFER_BPC_8, DRM_FORMAT_XRGB8888,
> IGT_CONNECTOR_BPC_8 },
> > > + };
> > > + int i;
> > > + data_t data = { 0 };
> > > +
> > > + igt_fixture {
> > > + data.drm_fd = drm_open_driver_master(DRIVER_INTEL);
> I agree with Harry.
> For context: KMS tests on IGT are officially supported on multiple
> SoCs and there is an active development on it along with Intel's.
> With that being said, KMS tests are meant to be generic. I understand
> if sometime you need to test little things specific for intel so we
> use igt_require_[intel | otherdriver], but if the whole test is intel
> specific, it doesn't make sense to be part of kms, the best place for
> it would be tests/i915.
> So basically, we shouldn't have any `
> drm_open_driver_master(DRIVER_INTEL);` in any kms_* tests.
> Alternatively, this test can be made generic and everyone is happy :))
Agreed, Dithering is not intel specific. As this test is already merged,
for now maybe we can add a check intel_require(intel) in igt_fixture, and
start working to make this test generic.
Does this make sense?
- Bhanu
> > > + kmstest_set_vt_graphics_mode();
> > > +
> > > + igt_display_require(&data.display, data.drm_fd);
> > > + igt_display_require_output(&data.display);
> > > + }
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(tests); i++) {
> > > + igt_describe_f("Framebuffer BPC:%d, Panel BPC:%d, Expected
> Dither:%s\n",
> > > + tests[i].fb_bpc, tests[i].output_bpc,
> > > + (tests[i].fb_bpc > tests[i].output_bpc) ?
> > > + "Enable":
> "Disable");
> > > +
> > > + igt_subtest_with_dynamic_f("FB-%dBPC-Vs-Panel-%dBPC",
> > > + tests[i].fb_bpc, tests[i].output_bpc)
> > > + run_dither_test(&data,
> > > + tests[i].fb_bpc,
> > > + tests[i].format,
> > > + tests[i].output_bpc);
> > > + }
> > > +
> > > + igt_fixture {
> > > + igt_display_fini(&data.display);
> > > + }
> > > +}
> > > diff --git a/tests/meson.build b/tests/meson.build index
> 01911c4579..d3de40be07 100644
> > > --- a/tests/meson.build
> > > +++ b/tests/meson.build
> > > @@ -29,6 +29,7 @@ test_progs = [
> > > 'kms_cursor_crc',
> > > 'kms_cursor_edge_walk',
> > > 'kms_cursor_legacy',
> > > + 'kms_dither',
> > > 'kms_dp_aux_dev',
> > > 'kms_dp_dsc',
> > > 'kms_dp_tiled_display',
> > > --
> > > 2.20.1
> > >
> > > _______________________________________________
> > > igt-dev mailing list
> > > igt-dev at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/igt-dev>>
> >
> > _______________________________________________
> > igt-dev mailing list
> > igt-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/igt-dev
More information about the igt-dev
mailing list