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

[Karthik B S <karthik.b.s@intel.com>]

Hi,

On 7/20/2020 10:20 PM, Bhanuprakash Modem wrote:
> 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     |
> |----------------|---------|-------------|
>
Add the test name in the subject.
tests/kms_dither: <explanation>
> Cc: Swati Sharma <swati2.sharma@intel.com>
> Cc: Karthik B S <karthik.b.s@intel.com>
> Cc: Uma Shankar <uma.shankar@intel.com>
> Cc: Pankaj Bharadiya <pankaj.laxminarayan.bharadiya@intel.com>
> Signed-off-by: Bhanuprakash Modem <bhanuprakash.modem@intel.com>
> ---
>   tests/Makefile.sources |   1 +
>   tests/kms_dither.c     | 252 +++++++++++++++++++++++++++++++++++++++++
>   tests/meson.build      |   1 +
>   3 files changed, 254 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..f4b143bb
> --- /dev/null
> +++ b/tests/kms_dither.c
> @@ -0,0 +1,252 @@
> +/*
> + * 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.
You can mention the author here.
> + */
> +
> +#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
> +
> +/* 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;
> +} 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. */
> +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_on_output(data_t *data, igt_output_t *output,
> +		int fb_bpc, int fb_format, int output_bpc,
> +		uint32_t flags)
> +{
> +	igt_display_t *display = &data->display;
> +	enum pipe pipe;
> +	igt_fb_t fb;
> +	int ret;
> +	bool enabled;
> +
> +	for_each_pipe(display, pipe) {
> +		if (!igt_pipe_connector_valid(pipe, output))
> +			continue;
> +
> +		igt_info("Dithering test execution on %s PIPE_%s\n",
> +				output->name, kmstest_pipe_name(pipe));
> +		prepare_test(data, output, pipe);
> +
Can we move the igt_create_fb outside the loop?
Seems redundant inside the loop as we're not changing anything here.

Any particular reason we're using 512x512 for fb?
If so, should it be a macro?
> +		ret = igt_create_fb(data->drm_fd, 512, 512, fb_format,
> +				    LOCAL_DRM_FORMAT_MOD_NONE,
> +				    &fb);
> +		igt_assert(ret);
> +
> +		igt_plane_set_fb(data->primary, &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);
Sorry for the nitpick, but I would prefer to use PIPE_NONE here than 
PIPE_ANY, as it looks like PIPE_ANY is deprecated.
Shouldn't matter either way :)
> +		igt_output_set_pipe(output, PIPE_ANY);
> +		igt_display_commit2(display, display->is_atomic ? COMMIT_ATOMIC : COMMIT_LEGACY);
> +		igt_remove_fb(data->drm_fd, &fb);
> +	}
> +	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;
> +	unsigned int max_bpc;
> +	int valid_outputs = 0;
> +
It would be better if we can use dynamic subtests here for pipe-output 
combinations.
> +	for_each_connected_output(&data->display, output) {
> +		if (!has_max_bpc(output))
> +			continue;
> +
> +		max_bpc = get_output_bpc(data, output);
> +		if(max_bpc < output_bpc)
> +			continue;
> +
> +		test_dithering_on_output(data, output, fb_bpc, fb_format, output_bpc, flags);
> +		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
> +	};
> +	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_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_SUSPEND looks like is unused? Do we plan to add subtests for this?

Thanks,
Karthik.B.S
> +						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',
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev