[igt-dev] [PATCH i-g-t] tests/kms_dither: Add new IGT test to validate crtc Dithering

Modem, Bhanuprakash bhanuprakash.modem at intel.com
Thu Jul 23 05:53:11 UTC 2020


> -----Original Message-----
> From: Sharma, Swati2 <swati2.sharma at intel.com>
> Sent: Wednesday, July 22, 2020 8:45 PM
> To: Modem, Bhanuprakash <bhanuprakash.modem at intel.com>; igt-
> dev at lists.freedesktop.org
> Cc: B S, Karthik <karthik.b.s at intel.com>; Shankar, Uma
> <uma.shankar at intel.com>; Laxminarayan Bharadiya, Pankaj
> <pankaj.laxminarayan.bharadiya at intel.com>
> Subject: Re: [PATCH i-g-t] tests/kms_dither: Add new IGT test to validate
> crtc Dithering
> 
> Hi,
> 
> Please find my review comments inline.
> 
> On 22-Jul-20 3:18 AM, Bhanuprakash Modem wrote:
> > Even though feature Dithering is there since legacy, there is no
> > specific IGT for this.
> 
> Can you please give little bit intro about dithering?
> 
> >
> > This patch will create New IGT for Dithering. And below is the
> > truth table for CRTC Dithering.
> >
> 
> It will be nice if you can explain logic behind following truth table
> here?
> Like you mentioned
> "If fb BPC is greater than the panel BPC, driver should enable dithering
> else disable."
> 
> > |----------------|---------|-------------|
> > |  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     |
> > |----------------|---------|-------------|
> 
> Also why are we missing 12BPC FB?
[Bhanu] I think 12 BPC format is not available for RGB. 
> 
> >
> > v2:
> > * Fix commit message (Karthik)
> > * Add file author (Karthik)
> > * Dynamic subtests for pipe-output combinations (Karthik)
> > * Update deprecated pipe name (Karthik)
> > * Remove redundant fb creation logic (Karthik)
> >
> > Cc: Swati Sharma <swati2.sharma at intel.com>
> > Cc: Karthik B S <karthik.b.s at intel.com>
> > Cc: Uma Shankar <uma.shankar at intel.com>
> > Cc: Pankaj Bharadiya <pankaj.laxminarayan.bharadiya at intel.com>
> > Signed-off-by: Bhanuprakash Modem <bhanuprakash.modem at intel.com>
> > ---
> >   tests/Makefile.sources |   1 +
> >   tests/kms_dither.c     | 257 +++++++++++++++++++++++++++++++++++++++++
> >   tests/meson.build      |   1 +
> >   3 files changed, 259 insertions(+)
> >   create mode 100644 tests/kms_dither.c
> >
> > diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> > index 0653c3d3..e816037c 100644
> > --- a/tests/Makefile.sources
> > +++ b/tests/Makefile.sources
> > @@ -43,6 +43,7 @@ TESTS_progs = \
> >   	kms_cursor_crc \
> >   	kms_cursor_edge_walk \
> >   	kms_cursor_legacy \
> > +	kms_dither \
> >   	kms_dp_aux_dev \
> >   	kms_dp_dsc \
> >   	kms_dp_tiled_display \
> > diff --git a/tests/kms_dither.c b/tests/kms_dither.c
> > new file mode 100644
> > index 00000000..0d517e74
> > --- /dev/null
> > +++ b/tests/kms_dither.c
> > @@ -0,0 +1,257 @@
> > +/*
> > + * 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>
> > + *
> 
> Extra \n?
> 
> > + */
> > +
> > +#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
> 
> Should 12 bpc FB be added?
[Bhanu] 12 bpc format is not there in RGB, shall we rename these macros like IGT_RGB_FRAME_BUFFER_BPC_8 etc..? 
> 
> > +
> > +/* Test flags. */
> > +enum {
> > +	TEST_NONE = 1 << 0,
> > +	TEST_SUSPEND = 1 << 1,
> > +};
> > +
> > +/* 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;
> > +
> > +/* 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 bool get_dither_state(data_t *data)
> > +{
> > +	char buf[256];
> > +	char crtc_name[7];
> > +	char *start_loc;
> > +	int fd, res;
> > +	unsigned int status;
> > +
> > +	snprintf(crtc_name, 7, "crtc-%d", data->pipe_id);
> > +	fd = igt_debugfs_open(data->drm_fd, crtc_name, O_RDONLY);
> > +	igt_assert(fd >= 0);
> > +
> > +	res = igt_debugfs_simple_read(fd, "dither", buf, sizeof(buf));
> > +	igt_require(res > 0);
> > +	close(fd);
> > +
> > +	igt_assert(start_loc = strstr(buf, "Dither: "));
> > +	igt_assert_eq(sscanf(start_loc, "Dither: %u", &status), 1);
> > +
> > +	return !!status;
> > +}
> > +
> > +/* Returns the current and maximum bpc from the connector debugfs. */
> 
> Func is not returning current bpc only max_bpc is getting returned.
> Correct comment.
> 
> > +static unsigned int get_output_bpc(data_t *data, igt_output_t *output)
> > +{
> > +	char buf[256];
> > +	char *start_loc;
> > +	int fd, res;
> > +	unsigned int max_bpc;
> > +
> > +	fd = igt_debugfs_connector_dir(data->drm_fd, output->name,
> O_RDONLY);
> > +	igt_assert(fd >= 0);
> > +
> > +	res = igt_debugfs_simple_read(fd, "output_bpc", buf, sizeof(buf));
> > +
> > +	igt_require(res > 0);
> > +
> > +	close(fd);
> > +
> > +	igt_assert(start_loc = strstr(buf, "Maximum: "));
> > +	igt_assert_eq(sscanf(start_loc, "Maximum: %u", &max_bpc), 1);
> > +
> > +	return max_bpc;
> > +}
> > +
> > +static void test_dithering(data_t *data, enum pipe pipe,
> > +			   igt_output_t *output, int fb_bpc,
> > +			   int output_bpc, uint32_t flags)
> > +{
> > +	igt_display_t *display = &data->display;
> > +	bool enabled;
> > +
> > +	igt_info("Dithering test execution on %s PIPE_%s\n",
> > +			output->name, kmstest_pipe_name(pipe));
> > +	prepare_test(data, output, pipe);
> > +
> > +	igt_plane_set_fb(data->primary, &data->fb);
> > +	igt_plane_set_size(data->primary, data->mode->hdisplay, data->mode-
> >vdisplay);
> > +	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
> > +	 * If fb_bpc is greater than output_bpc, Dithering should be Enabled
> > +	 * else Disabled
> > +	 */
> > +	enabled = get_dither_state(data);
> > +	if (fb_bpc > output_bpc)
> > +		igt_assert_f(enabled, "(fb_%dbpc > output_%dbpc): Dither should
> be enabled\n",
> > +				fb_bpc, output_bpc);
> > +	else
> > +		igt_assert_f(!enabled, "(fb_%dbpc <= output_%dbpc): Dither
> should be disabled\n",
> > +				fb_bpc, output_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);
> 
> Insert a \n here.
> 
> > +	return;
> > +}
> > +
> > +/* Returns true if an output supports max bpc property. */
> > +static bool has_max_bpc(igt_output_t *output)
> > +{
> > +        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,
> > +		uint32_t flags)
> > +{
> > +	igt_output_t *output;
> > +	igt_display_t *display = &data->display;
> > +	enum pipe pipe;
> > +	unsigned int max_bpc;
> > +	int valid_outputs = 0;
> > +
> > +	for_each_connected_output(display, output) {
> > +		if (!has_max_bpc(output))
> > +			continue;
> > +
> > +		max_bpc = get_output_bpc(data, output);
> > +		if(max_bpc < output_bpc)
> > +			continue;
> > +
> > +		igt_assert(igt_create_fb(data->drm_fd, 512, 512, fb_format,
> > +				LOCAL_DRM_FORMAT_MOD_NONE,
> > +				&data->fb)
> > +			  );
> > +		for_each_pipe(display, pipe) {
> > +			igt_dynamic_f("connector-%s-pipe-%s", output->name,
> kmstest_pipe_name(pipe)) {
> > +				if (!igt_pipe_connector_valid(pipe, output))
> > +					continue;
> > +				test_dithering(data, pipe, output, fb_bpc,
> output_bpc, flags);
> > +			}
> > +		}
> > +		igt_remove_fb(data->drm_fd, &data->fb);
> > +
> > +		valid_outputs++;
> > +	}
> > +
> > +	igt_require_f(valid_outputs, "No connector found with MAX BPC
> connector property. (or) "
> > +				     "No connector found with Max Panel BPC (%d) >=
> %d.\n",
> > +		      max_bpc, output_bpc);
> > +}
> > +
> > +igt_main
> > +{
> > +	struct {
> > +		int bpc;
> > +		int format;
> > +	} fb_formats[] = {
> > +		{ IGT_FRAME_BUFFER_BPC_8, DRM_FORMAT_RGB888 },
> > +		{ IGT_FRAME_BUFFER_BPC_10, DRM_FORMAT_XRGB2101010 },
> > +		{ IGT_FRAME_BUFFER_BPC_16, DRM_FORMAT_XRGB16161616F },
> > +	} > +	int output_bpc[] = {
> > +		IGT_CONNECTOR_BPC_6,
> > +		IGT_CONNECTOR_BPC_8,
> > +		IGT_CONNECTOR_BPC_10
> > +	};
> 
> Should these strucs be defined outside igt_main()?
[Bhanu] No other function is directly accessing these structs, I think this can be ok to keep inside the main().
> 
> > +	int i, j;
> > +	data_t data = { 0 };
> > +
> > +	igt_fixture {
> > +		data.drm_fd = drm_open_driver_master(DRIVER_INTEL);
> > +		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(fb_formats); i++) {
> > +		for (j = 0; j < ARRAY_SIZE(output_bpc); j++) {
> > +			igt_describe_f("Framebuffer BPC:%d, Panel BPC:%d,
> Expected Dither:%s\n",
> > +				       fb_formats[i].bpc, output_bpc[j],
> > +				       (fb_formats[i].bpc > output_bpc[j]) ?
> "Enable": "Disable"); > +
> > +			igt_subtest_with_dynamic_f("FB-%dBPC-Vs-Panel-%dBPC",
> fb_formats[i].bpc, output_bpc[j])
> > +				run_dither_test(&data,
> > +						fb_formats[i].bpc,
> > +						fb_formats[i].format,
> > +						output_bpc[j],
> > +						TEST_NONE);
> > +		}
> > +	}
> > +
> > +	igt_fixture {
> > +		igt_display_fini(&data.display);
> > +	}
> > +}
> > diff --git a/tests/meson.build b/tests/meson.build
> > index cfe508c4..05a9cfb8 100644
> > --- a/tests/meson.build
> > +++ b/tests/meson.build
> > @@ -27,6 +27,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',
> >
> Overall it looks good. However on CI tests are getting skipped.
> Can you please get IGT+Kernel patches tested together?
> 
> --
> ~Swati Sharma


More information about the igt-dev mailing list