[igt-dev] [PATCH i-g-t 1/2] tests/kms_dither: New IGT to validate crtc Dithering
Mark Yacoub
markyacoub at chromium.org
Thu Jul 15 14:50:29 UTC 2021
On Thu, Jul 15, 2021 at 12:00 AM Modem, Bhanuprakash
<bhanuprakash.modem at intel.com> wrote:
>
> > 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?
Hey Bhanu,
So I ran the test on a Zork (amdgpu) device and it's pretty gracefully
handled. The first subtest skips due to `eDP-1 don't support 6-bpc`
and the second one is pretty expected as you try to open
`i915_display_info` but it skips at `igt_require(res > 0);`.
Basically everything is gracefully handled if you do
`drm_open_driver_master(DRIVER_ANY);` and should be quite easy to
extend it to other platforms when needed.
I don't mind uploading the change so let me know :))
>
> - 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