[igt-dev] [PATCH i-g-t v4 0/6] igt: Add support for testing writeback connectors

[igt-dev] [PATCH i-g-t v4 0/6] igt: Add support for testing writeback connectors

[Liviu Dudau]

We're trying to introduce support for writeback connectors, a way to
expose in DRM the hardware functionality from display engines that
allows to write back into memory the result of the DE's composition
of supported planes.

Although this is a rebase of v3 with all the comments addressed, I'm not
expecting people to remember any of the previous versions, please review
this as if it is a new series.

Patches have been originally implemented by Brian, I've done the v3 and v4
updates to them.

Best regards,
Liviu

Changelog:
 - v4: Rebased on the latest i-g-t and switched to the igt_output_xxx()
   call as suggested by Maarten. v3 is here:
   https://lists.freedesktop.org/archives/intel-gfx/2018-March/157394.html
   Maarten's comments came a couple of months later :)
   https://lists.freedesktop.org/archives/intel-gfx/2018-June/169027.html
 - This is basically a v3 of the i-g-t tests, except I've now dropped
   all the changes that were trying to split the CRC functionality out
   of lib/igt_debugfs.c. v2 is here:
   https://lists.freedesktop.org/archives/intel-gfx/2017-July/133154.html
 - Added meson support for builting the kms_writeback test

Brian Starkey (6):
  lib/igt_kms: Add writeback support
  kms_writeback: Add initial writeback tests
  lib: Add function to hash a framebuffer
  kms_writeback: Add writeback-check-output
  lib/igt_kms: Add igt_output_clone_pipe for cloning
  kms_writeback: Add tests using a cloned output

 include/drm-uapi/drm.h      |  16 ++
 include/drm-uapi/drm_mode.h |   1 +
 lib/igt_fb.c                |  66 +++++
 lib/igt_fb.h                |   3 +
 lib/igt_kms.c               | 149 ++++++++---
 lib/igt_kms.h               |  11 +
 tests/Makefile.sources      |   1 +
 tests/kms_writeback.c       | 495 ++++++++++++++++++++++++++++++++++++
 tests/meson.build           |   1 +
 9 files changed, 705 insertions(+), 38 deletions(-)
 create mode 100644 tests/kms_writeback.c

-- 
2.19.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH i-g-t v4 1/6] lib/igt_kms: Add writeback support

[Liviu Dudau]

From: Brian Starkey <brian.starkey@arm.com>

Add support in igt_kms for writeback connectors, with the ability
to attach framebuffers.

Signed-off-by: Brian Starkey <brian.starkey@arm.com>
[rebased and updated to the latest igt style]
Signed-off-by: Liviu Dudau <liviu.dudau@arm.com>
---
 include/drm-uapi/drm.h      | 16 ++++++++++++
 include/drm-uapi/drm_mode.h |  1 +
 lib/igt_kms.c               | 50 +++++++++++++++++++++++++++++++++++++
 lib/igt_kms.h               |  6 +++++
 4 files changed, 73 insertions(+)

diff --git a/include/drm-uapi/drm.h b/include/drm-uapi/drm.h
index f0bd91de0..85c685a20 100644
--- a/include/drm-uapi/drm.h
+++ b/include/drm-uapi/drm.h
@@ -674,6 +674,22 @@ struct drm_get_cap {
  */
 #define DRM_CLIENT_CAP_ATOMIC	3
 
+/**
+ * DRM_CLIENT_CAP_ASPECT_RATIO
+ *
+ * If set to 1, the DRM core will provide aspect ratio information in modes.
+ */
+#define DRM_CLIENT_CAP_ASPECT_RATIO    4
+
+/**
+ * DRM_CLIENT_CAP_WRITEBACK_CONNECTORS
+ *
+ * If set to 1, the DRM core will expose special connectors to be used for
+ * writing back to memory the scene setup in the commit. Depends on client
+ * also supporting DRM_CLIENT_CAP_ATOMIC
+ */
+#define DRM_CLIENT_CAP_WRITEBACK_CONNECTORS	5
+
 /** DRM_IOCTL_SET_CLIENT_CAP ioctl argument type */
 struct drm_set_client_cap {
 	__u64 capability;
diff --git a/include/drm-uapi/drm_mode.h b/include/drm-uapi/drm_mode.h
index 2c575794f..7b47e184e 100644
--- a/include/drm-uapi/drm_mode.h
+++ b/include/drm-uapi/drm_mode.h
@@ -338,6 +338,7 @@ enum drm_mode_subconnector {
 #define DRM_MODE_CONNECTOR_VIRTUAL      15
 #define DRM_MODE_CONNECTOR_DSI		16
 #define DRM_MODE_CONNECTOR_DPI		17
+#define DRM_MODE_CONNECTOR_WRITEBACK	18
 
 struct drm_mode_get_connector {
 
diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index d806ccc1e..00d356fe0 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -197,6 +197,9 @@ const char * const igt_connector_prop_names[IGT_NUM_CONNECTOR_PROPS] = {
 	[IGT_CONNECTOR_DPMS] = "DPMS",
 	[IGT_CONNECTOR_BROADCAST_RGB] = "Broadcast RGB",
 	[IGT_CONNECTOR_CONTENT_PROTECTION] = "Content Protection",
+	[IGT_CONNECTOR_WRITEBACK_PIXEL_FORMATS] = "WRITEBACK_PIXEL_FORMATS",
+	[IGT_CONNECTOR_WRITEBACK_FB_ID] = "WRITEBACK_FB_ID",
+	[IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR] = "WRITEBACK_OUT_FENCE_PTR",
 };
 
 /*
@@ -453,6 +456,7 @@ static const struct type_name connector_type_names[] = {
 	{ DRM_MODE_CONNECTOR_VIRTUAL, "Virtual" },
 	{ DRM_MODE_CONNECTOR_DSI, "DSI" },
 	{ DRM_MODE_CONNECTOR_DPI, "DPI" },
+	{ DRM_MODE_CONNECTOR_WRITEBACK, "Writeback" },
 	{}
 };
 
@@ -1802,6 +1806,12 @@ static void igt_output_reset(igt_output_t *output)
 	if (igt_output_has_prop(output, IGT_CONNECTOR_BROADCAST_RGB))
 		igt_output_set_prop_value(output, IGT_CONNECTOR_BROADCAST_RGB,
 					  BROADCAST_RGB_FULL);
+	if (igt_output_has_prop(output, IGT_CONNECTOR_WRITEBACK_FB_ID))
+		igt_output_set_prop_value(output, IGT_CONNECTOR_WRITEBACK_FB_ID, 0);
+	if (igt_output_has_prop(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR)) {
+		igt_output_clear_prop_changed(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR);
+		output->writeback_out_fence_fd = -1;
+	}
 }
 
 /**
@@ -1814,6 +1824,8 @@ static void igt_output_reset(igt_output_t *output)
  * For outputs:
  * - %IGT_CONNECTOR_CRTC_ID
  * - %IGT_CONNECTOR_BROADCAST_RGB (if applicable)
+ * - %IGT_CONNECTOR_WRITEBACK_FB_ID
+ * - %IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR
  * - igt_output_override_mode() to default.
  *
  * For pipes:
@@ -1898,6 +1910,8 @@ bool igt_display_init(igt_display_t *display, int drm_fd)
 	if (drmSetClientCap(drm_fd, DRM_CLIENT_CAP_ATOMIC, 1) == 0)
 		display->is_atomic = 1;
 
+	drmSetClientCap(drm_fd, DRM_CLIENT_CAP_WRITEBACK_CONNECTORS, 1);
+
 	plane_resources = drmModeGetPlaneResources(display->drm_fd);
 	igt_assert(plane_resources);
 
@@ -2147,6 +2161,9 @@ static void igt_output_fini(igt_output_t *output)
 	kmstest_free_connector_config(&output->config);
 	free(output->name);
 	output->name = NULL;
+
+	if (output->writeback_out_fence_fd != -1)
+		close(output->writeback_out_fence_fd);
 }
 
 /**
@@ -3144,6 +3161,11 @@ static void igt_atomic_prepare_connector_commit(igt_output_t *output, drmModeAto
 					  output->props[i],
 					  output->values[i]));
 	}
+
+	if (output->writeback_out_fence_fd != -1) {
+		close(output->writeback_out_fence_fd);
+		output->writeback_out_fence_fd = -1;
+	}
 }
 
 /*
@@ -3264,6 +3286,14 @@ display_commit_changed(igt_display_t *display, enum igt_commit_style s)
 		else
 			/* no modeset in universal commit, no change to crtc. */
 			output->changed &= 1 << IGT_CONNECTOR_CRTC_ID;
+
+		if (s == COMMIT_ATOMIC) {
+			if (igt_output_is_prop_changed(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR))
+				igt_assert(output->writeback_out_fence_fd >= 0);
+
+			output->values[IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR] = 0;
+			igt_output_clear_prop_changed(output, IGT_CONNECTOR_WRITEBACK_FB_ID);
+		}
 	}
 
 	if (display->first_commit) {
@@ -3884,6 +3914,26 @@ void igt_pipe_request_out_fence(igt_pipe_t *pipe)
 	igt_pipe_obj_set_prop_value(pipe, IGT_CRTC_OUT_FENCE_PTR, (ptrdiff_t)&pipe->out_fence_fd);
 }
 
+/**
+ * igt_output_set_writeback_fb:
+ * @output: Target output
+ * @fb: Target framebuffer
+ *
+ * This function sets the given @fb to be used as the target framebuffer for the
+ * writeback engine at the next atomic commit. It will also request a writeback
+ * out fence that will contain the fd number of the out fence created by KMS.
+ */
+void igt_output_set_writeback_fb(igt_output_t *output, struct igt_fb *fb)
+{
+	igt_display_t *display = output->display;
+
+	LOG(display, "%s: output_set_writeback_fb(%d)\n", output->name, fb ? fb->fb_id : 0);
+
+	igt_output_set_prop_value(output, IGT_CONNECTOR_WRITEBACK_FB_ID, fb ? fb->fb_id : 0);
+	igt_output_set_prop_value(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR,
+				  (ptrdiff_t)&output->writeback_out_fence_fd);
+}
+
 /**
  * igt_wait_for_vblank_count:
  * @drm_fd: A drm file descriptor
diff --git a/lib/igt_kms.h b/lib/igt_kms.h
index e6aff339a..a84f9ac42 100644
--- a/lib/igt_kms.h
+++ b/lib/igt_kms.h
@@ -121,6 +121,9 @@ enum igt_atomic_connector_properties {
        IGT_CONNECTOR_DPMS,
        IGT_CONNECTOR_BROADCAST_RGB,
        IGT_CONNECTOR_CONTENT_PROTECTION,
+       IGT_CONNECTOR_WRITEBACK_PIXEL_FORMATS,
+       IGT_CONNECTOR_WRITEBACK_FB_ID,
+       IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR,
        IGT_NUM_CONNECTOR_PROPS
 };
 
@@ -359,6 +362,8 @@ typedef struct {
 	bool use_override_mode;
 	drmModeModeInfo override_mode;
 
+	int32_t writeback_out_fence_fd;
+
 	/* bitmask of changed properties */
 	uint64_t changed;
 
@@ -404,6 +409,7 @@ igt_plane_t *igt_output_get_plane(igt_output_t *output, int plane_idx);
 igt_plane_t *igt_output_get_plane_type(igt_output_t *output, int plane_type);
 igt_output_t *igt_output_from_connector(igt_display_t *display,
     drmModeConnector *connector);
+void igt_output_set_writeback_fb(igt_output_t *output, struct igt_fb *fb);
 
 igt_plane_t *igt_pipe_get_plane_type(igt_pipe_t *pipe, int plane_type);
 igt_output_t *igt_get_single_output_for_pipe(igt_display_t *display, enum pipe pipe);
-- 
2.19.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH i-g-t v4 2/6] kms_writeback: Add initial writeback tests

[Liviu Dudau]

From: Brian Starkey <brian.starkey@arm.com>

Add tests for the WRITEBACK_PIXEL_FORMATS, WRITEBACK_OUT_FENCE_PTR and
WRITEBACK_FB_ID properties on writeback connectors, ensuring their
behaviour is correct.

Signed-off-by: Brian Starkey <brian.starkey@arm.com>
[rebased and updated do_writeback_test() function to address feedback]
Signed-off-by: Liviu Dudau <liviu.dudau@arm.com>
---
 tests/Makefile.sources |   1 +
 tests/kms_writeback.c  | 314 +++++++++++++++++++++++++++++++++++++++++
 tests/meson.build      |   1 +
 3 files changed, 316 insertions(+)
 create mode 100644 tests/kms_writeback.c

diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index c910210b9..317032e32 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -86,6 +86,7 @@ TESTS_progs = \
 	kms_tv_load_detect \
 	kms_universal_plane \
 	kms_vblank \
+	kms_writeback \
 	meta_test \
 	perf \
 	perf_pmu \
diff --git a/tests/kms_writeback.c b/tests/kms_writeback.c
new file mode 100644
index 000000000..636777d0b
--- /dev/null
+++ b/tests/kms_writeback.c
@@ -0,0 +1,314 @@
+/*
+ * (C) COPYRIGHT 2017 ARM Limited. All rights reserved.
+ *
+ * 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 <errno.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <string.h>
+
+#include "igt.h"
+#include "igt_core.h"
+#include "igt_fb.h"
+
+static drmModePropertyBlobRes *get_writeback_formats_blob(igt_output_t *output)
+{
+	drmModePropertyBlobRes *blob = NULL;
+	uint64_t blob_id;
+	int ret;
+
+	ret = kmstest_get_property(output->display->drm_fd,
+				   output->config.connector->connector_id,
+				   DRM_MODE_OBJECT_CONNECTOR,
+				   igt_connector_prop_names[IGT_CONNECTOR_WRITEBACK_PIXEL_FORMATS],
+				   NULL, &blob_id, NULL);
+	if (ret)
+		blob = drmModeGetPropertyBlob(output->display->drm_fd, blob_id);
+
+	igt_assert(blob);
+
+	return blob;
+}
+
+static bool check_writeback_config(igt_display_t *display, igt_output_t *output)
+{
+	igt_fb_t input_fb, output_fb;
+	igt_plane_t *plane;
+	uint32_t writeback_format = DRM_FORMAT_XRGB8888;
+	uint64_t tiling = igt_fb_mod_to_tiling(0);
+	int width, height, ret;
+	drmModeModeInfo override_mode = {
+		.clock = 25175,
+		.hdisplay = 640,
+		.hsync_start = 656,
+		.hsync_end = 752,
+		.htotal = 800,
+		.hskew = 0,
+		.vdisplay = 480,
+		.vsync_start = 490,
+		.vsync_end = 492,
+		.vtotal = 525,
+		.vscan = 0,
+		.vrefresh = 60,
+		.flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
+		.name = {"640x480-60"},
+	};
+	igt_output_override_mode(output, &override_mode);
+
+	width = override_mode.hdisplay;
+	height = override_mode.vdisplay;
+
+	ret = igt_create_fb(display->drm_fd, width, height, DRM_FORMAT_XRGB8888, tiling, &input_fb);
+	igt_assert(ret >= 0);
+
+	ret = igt_create_fb(display->drm_fd, width, height, writeback_format, tiling, &output_fb);
+	igt_assert(ret >= 0);
+
+	plane = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY);
+	igt_plane_set_fb(plane, &input_fb);
+	igt_output_set_writeback_fb(output, &output_fb);
+
+	ret = igt_display_try_commit_atomic(display, DRM_MODE_ATOMIC_TEST_ONLY |
+					    DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
+	igt_plane_set_fb(plane, NULL);
+	igt_remove_fb(display->drm_fd, &input_fb);
+	igt_remove_fb(display->drm_fd, &output_fb);
+
+	return !ret;
+}
+
+static igt_output_t *kms_writeback_get_output(igt_display_t *display)
+{
+	int i;
+
+	for (i = 0; i < display->n_outputs; i++) {
+		igt_output_t *output = &display->outputs[i];
+		int j;
+
+		if (output->config.connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK)
+			continue;
+
+		kmstest_force_connector(display->drm_fd, output->config.connector, FORCE_CONNECTOR_ON);
+
+		for (j = 0; j < igt_display_get_n_pipes(display); j++) {
+			igt_output_set_pipe(output, j);
+
+			if (check_writeback_config(display, output)) {
+				igt_debug("Using connector %u:%s on pipe %d\n",
+					  output->config.connector->connector_id,
+					  output->name, j);
+				return output;
+			}
+		}
+
+		/* Restore any connectors we don't use, so we don't trip on them later */
+		kmstest_force_connector(display->drm_fd, output->config.connector, FORCE_CONNECTOR_UNSPECIFIED);
+	}
+
+	return NULL;
+}
+
+static void check_writeback_fb_id(igt_output_t *output)
+{
+	uint64_t check_fb_id;
+
+	check_fb_id = igt_output_get_prop(output, IGT_CONNECTOR_WRITEBACK_FB_ID);
+	igt_assert(check_fb_id == 0);
+}
+
+static int do_writeback_test(igt_output_t *output, uint32_t flags,
+			      uint32_t fb_id, int32_t *out_fence_ptr,
+			      bool ptr_valid)
+{
+	int ret;
+	igt_display_t *display = output->display;
+	struct kmstest_connector_config *config = &output->config;
+
+	igt_output_set_prop_value(output, IGT_CONNECTOR_CRTC_ID, config->crtc->crtc_id);
+	igt_output_set_prop_value(output, IGT_CONNECTOR_WRITEBACK_FB_ID, fb_id);
+	igt_output_set_prop_value(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR, (uint64_t)out_fence_ptr);
+
+	if (ptr_valid)
+		*out_fence_ptr = 0;
+
+	ret = igt_display_try_commit_atomic(display, flags, NULL);
+
+	if (ptr_valid)
+		igt_assert(*out_fence_ptr == -1);
+
+	/* WRITEBACK_FB_ID must always read as zero */
+	check_writeback_fb_id(output);
+
+	return ret;
+}
+
+static void invalid_out_fence(igt_output_t *output, igt_fb_t *valid_fb, igt_fb_t *invalid_fb)
+{
+	int i, ret;
+	int32_t out_fence;
+	struct {
+		uint32_t fb_id;
+		bool ptr_valid;
+		int32_t *out_fence_ptr;
+	} invalid_tests[] = {
+		{
+			/* No output buffer, but the WRITEBACK_OUT_FENCE_PTR set. */
+			.fb_id = 0,
+			.ptr_valid = true,
+			.out_fence_ptr = &out_fence,
+		},
+		{
+			/* Invalid output buffer. */
+			.fb_id = invalid_fb->fb_id,
+			.ptr_valid = true,
+			.out_fence_ptr = &out_fence,
+		},
+		{
+			/* Invalid WRITEBACK_OUT_FENCE_PTR. */
+			.fb_id = valid_fb->fb_id,
+			.ptr_valid = false,
+			.out_fence_ptr = (int32_t *)0x8,
+		},
+	};
+
+	for (i = 0; i < ARRAY_SIZE(invalid_tests); i++) {
+		ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET,
+					invalid_tests[i].fb_id,
+					invalid_tests[i].out_fence_ptr,
+					invalid_tests[i].ptr_valid);
+		igt_assert(ret != 0);
+	}
+}
+
+static void writeback_fb_id(igt_output_t *output, igt_fb_t *valid_fb, igt_fb_t *invalid_fb)
+{
+
+	int ret;
+
+	/* Valid output buffer */
+	ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET,
+				valid_fb->fb_id, NULL, false);
+	igt_assert(ret == 0);
+
+	/* Invalid object for WRITEBACK_FB_ID */
+	ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET,
+				output->id, NULL, false);
+	igt_assert(ret == -EINVAL);
+
+	/* Zero WRITEBACK_FB_ID */
+	ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET,
+				0, NULL, false);
+	igt_assert(ret == 0);
+}
+
+igt_main
+{
+	igt_display_t display;
+	igt_output_t *output;
+	igt_plane_t *plane;
+	igt_fb_t input_fb;
+	drmModeModeInfo mode;
+	int ret;
+
+	memset(&display, 0, sizeof(display));
+
+	igt_fixture {
+		display.drm_fd = drm_open_driver_master(DRIVER_ANY);
+		igt_display_require(&display, display.drm_fd);
+
+		kmstest_set_vt_graphics_mode();
+
+		igt_display_init(&display, display.drm_fd);
+
+		igt_require(display.is_atomic);
+
+		output = kms_writeback_get_output(&display);
+		igt_require(output);
+
+		if (output->use_override_mode)
+			memcpy(&mode, &output->override_mode, sizeof(mode));
+		else
+			memcpy(&mode, &output->config.default_mode, sizeof(mode));
+
+		plane = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY);
+		igt_require(plane);
+
+		ret = igt_create_fb(display.drm_fd, mode.hdisplay,
+				    mode.vdisplay,
+				    DRM_FORMAT_XRGB8888,
+				    igt_fb_mod_to_tiling(0),
+				    &input_fb);
+		igt_assert(ret >= 0);
+		igt_plane_set_fb(plane, &input_fb);
+	}
+
+	igt_subtest("writeback-pixel-formats") {
+		drmModePropertyBlobRes *formats_blob = get_writeback_formats_blob(output);
+		const char *valid_chars = "0123456 ABCGNRUVXY";
+		unsigned int i;
+		char *c;
+
+		/*
+		 * We don't have a comprehensive list of formats, so just check
+		 * that the blob length is sensible and that it doesn't contain
+		 * any outlandish characters
+		 */
+		igt_assert(!(formats_blob->length % 4));
+		c = formats_blob->data;
+		for (i = 0; i < formats_blob->length; i++)
+			igt_assert_f(strchr(valid_chars, c[i]),
+				     "Unexpected character %c\n", c[i]);
+	}
+
+	igt_subtest("writeback-invalid-out-fence") {
+		igt_fb_t invalid_fb;
+		ret = igt_create_fb(display.drm_fd, mode.hdisplay / 2,
+				    mode.vdisplay / 2,
+				    DRM_FORMAT_XRGB8888,
+				    igt_fb_mod_to_tiling(0),
+				    &invalid_fb);
+		igt_require(ret > 0);
+
+		invalid_out_fence(output, &input_fb, &invalid_fb);
+
+		igt_remove_fb(display.drm_fd, &invalid_fb);
+	}
+
+	igt_subtest("writeback-fb-id") {
+		igt_fb_t output_fb;
+		ret = igt_create_fb(display.drm_fd, mode.hdisplay, mode.vdisplay,
+				    DRM_FORMAT_XRGB8888,
+				    igt_fb_mod_to_tiling(0),
+				    &output_fb);
+		igt_require(ret > 0);
+
+		writeback_fb_id(output, &input_fb, &output_fb);
+
+		igt_remove_fb(display.drm_fd, &output_fb);
+	}
+
+	igt_fixture {
+		igt_remove_fb(display.drm_fd, &input_fb);
+		igt_display_fini(&display);
+	}
+}
diff --git a/tests/meson.build b/tests/meson.build
index eacdc1a71..78df4db86 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -63,6 +63,7 @@ test_progs = [
 	'kms_tv_load_detect',
 	'kms_universal_plane',
 	'kms_vblank',
+	'kms_writeback',
 	'meta_test',
 	'perf',
 	'pm_backlight',
-- 
2.19.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH i-g-t v4 3/6] lib: Add function to hash a framebuffer

[Liviu Dudau]

From: Brian Starkey <brian.starkey@arm.com>

To use writeback buffers as a CRC source, we need to be able to hash
them. Implement a simple FVA-1a hashing routine for this purpose.

Doing a bytewise hash on the framebuffer directly can be very slow if
the memory is noncached. By making a copy of each line in the FB first
(which can take advantage of word-access speedup), we can do the hash
on a cached copy, which is much faster (10x speedup on my platform).

Signed-off-by: Brian Starkey <brian.starkey@arm.com>
[rebased and updated to the most recent API]
Signed-off-by: Liviu Dudau <liviu.dudau@arm.com>
---
 lib/igt_fb.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/igt_fb.h |  3 +++
 2 files changed, 69 insertions(+)

diff --git a/lib/igt_fb.c b/lib/igt_fb.c
index fe0c99b9f..715de3e28 100644
--- a/lib/igt_fb.c
+++ b/lib/igt_fb.c
@@ -2254,6 +2254,72 @@ bool igt_fb_supported_format(uint32_t drm_format)
 	return false;
 }
 
+/*
+ * This implements the FNV-1a hashing algorithm instead of CRC, for
+ * simplicity
+ * http://www.isthe.com/chongo/tech/comp/fnv/index.html
+ *
+ * hash = offset_basis
+ * for each octet_of_data to be hashed
+ *         hash = hash xor octet_of_data
+ *         hash = hash * FNV_prime
+ * return hash
+ *
+ * 32 bit offset_basis = 2166136261
+ * 32 bit FNV_prime = 224 + 28 + 0x93 = 16777619
+ */
+int igt_fb_get_crc(struct igt_fb *fb, igt_crc_t *crc)
+{
+#define FNV1a_OFFSET_BIAS 2166136261
+#define FNV1a_PRIME 16777619
+	uint32_t hash;
+	void *map;
+	char *ptr, *line = NULL;
+	int x, y, cpp = igt_drm_format_to_bpp(fb->drm_format) / 8;
+	uint32_t stride = calc_plane_stride(fb, 0);
+
+	if (fb->is_dumb)
+		map = kmstest_dumb_map_buffer(fb->fd, fb->gem_handle, fb->size,
+					      PROT_READ);
+	else
+		map = gem_mmap__gtt(fb->fd, fb->gem_handle, fb->size,
+				    PROT_READ);
+	ptr = map;
+
+	/*
+	 * Framebuffers are often uncached, which can make byte-wise accesses
+	 * very slow. We copy each line of the FB into a local buffer to speed
+	 * up the hashing.
+	 */
+	line = malloc(stride);
+	if (!line) {
+		munmap(map, fb->size);
+		return -ENOMEM;
+	}
+
+	hash = FNV1a_OFFSET_BIAS;
+
+	for (y = 0; y < fb->height; y++, ptr += stride) {
+
+		memcpy(line, ptr, stride);
+
+		for (x = 0; x < fb->width * cpp; x++) {
+			hash ^= line[x];
+			hash *= FNV1a_PRIME;
+		}
+	}
+
+	crc->n_words = 1;
+	crc->crc[0] = hash;
+
+	free(line);
+	munmap(map, fb->size);
+
+	return 0;
+#undef FNV1a_OFFSET_BIAS
+#undef FNV1a_PRIME
+}
+
 /**
  * igt_format_is_yuv:
  * @drm_format: drm fourcc
diff --git a/lib/igt_fb.h b/lib/igt_fb.h
index 9f027deba..948c5380c 100644
--- a/lib/igt_fb.h
+++ b/lib/igt_fb.h
@@ -37,6 +37,7 @@
 #include <i915_drm.h>
 
 #include "igt_color_encoding.h"
+#include "igt_debugfs.h"
 
 /**
  * igt_fb_t:
@@ -173,5 +174,7 @@ const char *igt_format_str(uint32_t drm_format);
 bool igt_fb_supported_format(uint32_t drm_format);
 bool igt_format_is_yuv(uint32_t drm_format);
 
+int igt_fb_get_crc(struct igt_fb *fb, igt_crc_t *crc);
+
 #endif /* __IGT_FB_H__ */
 
-- 
2.19.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH i-g-t v4 4/6] kms_writeback: Add writeback-check-output

[Liviu Dudau]

From: Brian Starkey <brian.starkey@arm.com>

Add a test which makes commits using the writeback connector, and
checks the output buffer hash to make sure it is/isn't written as
appropriate.

Signed-off-by: Brian Starkey <brian.starkey@arm.com>
[rebased and addressed comments from Maarten]
Signed-off-by: Liviu Dudau <liviu.dudau@arm.com>

---
 tests/kms_writeback.c | 127 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 127 insertions(+)

diff --git a/tests/kms_writeback.c b/tests/kms_writeback.c
index 636777d0b..7cb65207e 100644
--- a/tests/kms_writeback.c
+++ b/tests/kms_writeback.c
@@ -30,6 +30,7 @@
 #include "igt.h"
 #include "igt_core.h"
 #include "igt_fb.h"
+#include "sw_sync.h"
 
 static drmModePropertyBlobRes *get_writeback_formats_blob(igt_output_t *output)
 {
@@ -221,6 +222,119 @@ static void writeback_fb_id(igt_output_t *output, igt_fb_t *valid_fb, igt_fb_t *
 	igt_assert(ret == 0);
 }
 
+static void fill_fb(igt_fb_t *fb, double color[3])
+{
+	cairo_t *cr = igt_get_cairo_ctx(fb->fd, fb);
+	igt_assert(cr);
+
+	igt_paint_color(cr, 0, 0, fb->width, fb->height,
+			color[0], color[1], color[2]);
+}
+
+static void get_and_wait_out_fence(igt_output_t *output)
+{
+	int ret;
+
+	igt_assert(output->writeback_out_fence_fd >= 0);
+
+	ret = sync_fence_wait(output->writeback_out_fence_fd, 1000);
+	igt_assert(ret == 0);
+	close(output->writeback_out_fence_fd);
+}
+
+static void writeback_sequence(igt_output_t *output, igt_plane_t *plane,
+				igt_fb_t *in_fb, igt_fb_t *out_fbs[], int n_commits)
+{
+	int i, color_idx = 0;
+	double in_fb_colors[2][3] = {
+		{ 1.0, 0.0, 0.0 },
+		{ 0.0, 1.0, 0.0 },
+	};
+	double clear_color[3] = { 1.0, 1.0, 1.0 };
+	igt_crc_t cleared_crc, out_expected;
+
+	for (i = 0; i < n_commits; i++, color_idx++) {
+		/* Change the input color each time */
+		fill_fb(in_fb, in_fb_colors[color_idx % 2]);
+
+		if (out_fbs[i]) {
+			igt_crc_t out_before;
+
+			/* Get the expected CRC */
+			fill_fb(out_fbs[i], in_fb_colors[color_idx % 2]);
+			igt_fb_get_crc(out_fbs[i], &out_expected);
+
+			fill_fb(out_fbs[i], clear_color);
+			if (i == 0)
+				igt_fb_get_crc(out_fbs[i], &cleared_crc);
+			igt_fb_get_crc(out_fbs[i], &out_before);
+			igt_assert_crc_equal(&cleared_crc, &out_before);
+		}
+
+		/* Commit */
+		igt_plane_set_fb(plane, in_fb);
+		if (out_fbs[i])
+			igt_output_set_writeback_fb(output, out_fbs[i]);
+		else
+			/* avoid setting the out fence ptr that is done via igt_output_set_writeback_fb() */
+			igt_output_set_prop_value(output, IGT_CONNECTOR_WRITEBACK_FB_ID, 0);
+
+		igt_display_commit_atomic(output->display,
+					  DRM_MODE_ATOMIC_ALLOW_MODESET,
+					  NULL);
+		if (out_fbs[i])
+			get_and_wait_out_fence(output);
+
+		/* Make sure the old output buffer is untouched */
+		if (i > 0 && out_fbs[i - 1] && (out_fbs[i] != out_fbs[i - 1])) {
+			igt_crc_t out_prev;
+			igt_fb_get_crc(out_fbs[i - 1], &out_prev);
+			igt_assert_crc_equal(&cleared_crc, &out_prev);
+		}
+
+		/* Make sure this output buffer is written */
+		if (out_fbs[i]) {
+			igt_crc_t out_after;
+			igt_fb_get_crc(out_fbs[i], &out_after);
+			igt_assert_crc_equal(&out_expected, &out_after);
+
+			/* And clear it, for the next time */
+			fill_fb(out_fbs[i], clear_color);
+		}
+	}
+}
+
+static void writeback_check_output(igt_output_t *output, igt_plane_t *plane,
+				   igt_fb_t *input_fb, igt_fb_t *output_fb)
+{
+	igt_fb_t *out_fbs[2] = { 0 };
+	igt_fb_t second_out_fb;
+	int ret;
+
+	/* One commit, with a writeback. */
+	writeback_sequence(output, plane, input_fb, &output_fb, 1);
+
+	/* Two commits, the second with no writeback */
+	out_fbs[0] = output_fb;
+	writeback_sequence(output, plane, input_fb, out_fbs, 2);
+
+	/* Two commits, both with writeback */
+	out_fbs[1] = output_fb;
+	writeback_sequence(output, plane, input_fb, out_fbs, 2);
+
+	ret = igt_create_fb(output_fb->fd, output_fb->width, output_fb->height,
+			    DRM_FORMAT_XRGB8888,
+			    igt_fb_mod_to_tiling(0),
+			    &second_out_fb);
+	igt_require(ret > 0);
+
+	/* Two commits, with different writeback buffers */
+	out_fbs[1] = &second_out_fb;
+	writeback_sequence(output, plane, input_fb, out_fbs, 2);
+
+	igt_remove_fb(output_fb->fd, &second_out_fb);
+}
+
 igt_main
 {
 	igt_display_t display;
@@ -307,6 +421,19 @@ igt_main
 		igt_remove_fb(display.drm_fd, &output_fb);
 	}
 
+	igt_subtest("writeback-check-output") {
+		igt_fb_t output_fb;
+		ret = igt_create_fb(display.drm_fd, mode.hdisplay, mode.vdisplay,
+				    DRM_FORMAT_XRGB8888,
+				    igt_fb_mod_to_tiling(0),
+				    &output_fb);
+		igt_require(ret > 0);
+
+		writeback_check_output(output, plane, &input_fb, &output_fb);
+
+		igt_remove_fb(display.drm_fd, &output_fb);
+	}
+
 	igt_fixture {
 		igt_remove_fb(display.drm_fd, &input_fb);
 		igt_display_fini(&display);
-- 
2.19.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH i-g-t v4 5/6] lib/igt_kms: Add igt_output_clone_pipe for cloning

[Liviu Dudau]

From: Brian Starkey <brian.starkey@arm.com>

An output can be added as a clone of any other output(s) attached to a
pipe using igt_output_clone_pipe()

Signed-off-by: Brian Starkey <brian.starkey@arm.com>
---
 lib/igt_kms.c | 99 +++++++++++++++++++++++++++++++--------------------
 lib/igt_kms.h |  5 +++
 2 files changed, 66 insertions(+), 38 deletions(-)

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index 00d356fe0..0a826718e 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -1694,6 +1694,17 @@ static void igt_display_log_shift(igt_display_t *display, int shift)
 	igt_assert(display->log_shift >= 0);
 }
 
+static int igt_output_idx(igt_output_t *output)
+{
+	int i;
+
+	for (i = 0; i < output->display->n_outputs; i++)
+		if (&output->display->outputs[i] == output)
+			return i;
+
+	return -1;
+}
+
 static void igt_output_refresh(igt_output_t *output)
 {
 	igt_display_t *display = output->display;
@@ -2188,42 +2199,6 @@ void igt_display_fini(igt_display_t *display)
 	display->pipes = NULL;
 }
 
-static void igt_display_refresh(igt_display_t *display)
-{
-	igt_output_t *output;
-	int i;
-
-	unsigned long pipes_in_use = 0;
-
-       /* Check that two outputs aren't trying to use the same pipe */
-	for (i = 0; i < display->n_outputs; i++) {
-		output = &display->outputs[i];
-
-		if (output->pending_pipe != PIPE_NONE) {
-			if (pipes_in_use & (1 << output->pending_pipe))
-				goto report_dup;
-
-			pipes_in_use |= 1 << output->pending_pipe;
-		}
-
-		if (output->force_reprobe)
-			igt_output_refresh(output);
-	}
-
-	return;
-
-report_dup:
-	for (; i > 0; i--) {
-		igt_output_t *b = &display->outputs[i - 1];
-
-		igt_assert_f(output->pending_pipe !=
-			     b->pending_pipe,
-			     "%s and %s are both trying to use pipe %s\n",
-			     igt_output_name(output), igt_output_name(b),
-			     kmstest_pipe_name(output->pending_pipe));
-	}
-}
-
 static igt_pipe_t *igt_output_get_driving_pipe(igt_output_t *output)
 {
 	igt_display_t *display = output->display;
@@ -2247,6 +2222,39 @@ static igt_pipe_t *igt_output_get_driving_pipe(igt_output_t *output)
 	return &display->pipes[pipe];
 }
 
+static void igt_display_refresh(igt_display_t *display)
+{
+	igt_output_t *output;
+	igt_pipe_t *pipe;
+	int i;
+
+	unsigned long pipes_in_use = 0;
+
+	/* Check that outputs and pipes agree wrt. cloning */
+	for (i = 0; i < display->n_outputs; i++) {
+		output = &display->outputs[i];
+		unsigned long pending_crtc_idx_mask = 1 << output->pending_pipe;
+
+		pipe = igt_output_get_driving_pipe(output);
+		if (pipe) {
+			igt_assert_f(pipe->outputs & (1 << igt_output_idx(output)),
+				     "Output %s not expected to be using pipe %s\n",
+				     igt_output_name(output),
+				     kmstest_pipe_name(pipe->pipe));
+
+			if (pipes_in_use & pending_crtc_idx_mask)
+				LOG(display, "Output %s clones pipe %s\n",
+				    igt_output_name(output),
+				    kmstest_pipe_name(pipe->pipe));
+		}
+
+		pipes_in_use |= pending_crtc_idx_mask;
+
+		if (output->force_reprobe)
+			igt_output_refresh(output);
+	}
+}
+
 static igt_plane_t *igt_pipe_get_plane(igt_pipe_t *pipe, int plane_idx)
 {
 	igt_require_f(plane_idx >= 0 && plane_idx < pipe->n_planes,
@@ -3583,6 +3591,7 @@ void igt_output_override_mode(igt_output_t *output, drmModeModeInfo *mode)
 	output->use_override_mode = !!mode;
 
 	if (pipe) {
+		igt_debug("overriding pipe mode in %s way\n", output->display->is_atomic ? "atomic" : "legacy");
 		if (output->display->is_atomic)
 			igt_pipe_obj_replace_prop_blob(pipe, IGT_CRTC_MODE_ID, igt_output_get_mode(output), sizeof(*mode));
 		else
@@ -3590,6 +3599,16 @@ void igt_output_override_mode(igt_output_t *output, drmModeModeInfo *mode)
 	}
 }
 
+void igt_output_clone_pipe(igt_output_t *output, enum pipe pipe)
+{
+	igt_display_t *display = output->display;
+	uint32_t current_clones = display->pipes[pipe].outputs;
+
+	igt_output_set_pipe(output, pipe);
+
+	display->pipes[pipe].outputs |= current_clones;
+}
+
 /*
  * igt_output_set_pipe:
  * @output: Target output for which the pipe is being set to
@@ -3606,11 +3625,15 @@ void igt_output_set_pipe(igt_output_t *output, enum pipe pipe)
 
 	igt_assert(output->name);
 
-	if (output->pending_pipe != PIPE_NONE)
+	if (output->pending_pipe != PIPE_NONE) {
 		old_pipe = igt_output_get_driving_pipe(output);
+		old_pipe->outputs &= ~(1 << igt_output_idx(output));
+	}
 
-	if (pipe != PIPE_NONE)
+	if (pipe != PIPE_NONE) {
 		pipe_obj = &display->pipes[pipe];
+		pipe_obj->outputs = (1 << igt_output_idx(output));
+	}
 
 	LOG(display, "%s: set_pipe(%s)\n", igt_output_name(output),
 	    kmstest_pipe_name(pipe));
diff --git a/lib/igt_kms.h b/lib/igt_kms.h
index a84f9ac42..4aeca22e9 100644
--- a/lib/igt_kms.h
+++ b/lib/igt_kms.h
@@ -349,6 +349,9 @@ struct igt_pipe {
 	uint32_t crtc_id;
 
 	int32_t out_fence_fd;
+	bool out_fence_requested;
+
+	uint32_t outputs;
 };
 
 typedef struct {
@@ -405,6 +408,8 @@ const char *igt_output_name(igt_output_t *output);
 drmModeModeInfo *igt_output_get_mode(igt_output_t *output);
 void igt_output_override_mode(igt_output_t *output, drmModeModeInfo *mode);
 void igt_output_set_pipe(igt_output_t *output, enum pipe pipe);
+void igt_output_clone_pipe(igt_output_t *output, enum pipe pipe);
+
 igt_plane_t *igt_output_get_plane(igt_output_t *output, int plane_idx);
 igt_plane_t *igt_output_get_plane_type(igt_output_t *output, int plane_type);
 igt_output_t *igt_output_from_connector(igt_display_t *display,
-- 
2.19.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH i-g-t v4 6/6] kms_writeback: Add tests using a cloned output

[Liviu Dudau]

From: Brian Starkey <brian.starkey@arm.com>

Update the connector search to also optionally attempt to find a
non-writeback connector to clone to.

Add a subtest which is the same as writeback-check-output, but also
clones to the second connector.

Signed-off-by: Brian Starkey <brian.starkey@arm.com>
[rebased and addressed comments by Maarten]
Signed-off-by: Liviu Dudau <liviu.dudau@arm.com>
---
 tests/kms_writeback.c | 64 +++++++++++++++++++++++++++++++++++++++----
 1 file changed, 59 insertions(+), 5 deletions(-)

diff --git a/tests/kms_writeback.c b/tests/kms_writeback.c
index 7cb65207e..290c9f9ee 100644
--- a/tests/kms_writeback.c
+++ b/tests/kms_writeback.c
@@ -51,7 +51,8 @@ static drmModePropertyBlobRes *get_writeback_formats_blob(igt_output_t *output)
 	return blob;
 }
 
-static bool check_writeback_config(igt_display_t *display, igt_output_t *output)
+static bool check_writeback_config(igt_display_t *display, igt_output_t *output,
+				   int pipe, igt_output_t **clone)
 {
 	igt_fb_t input_fb, output_fb;
 	igt_plane_t *plane;
@@ -91,6 +92,30 @@ static bool check_writeback_config(igt_display_t *display, igt_output_t *output)
 
 	ret = igt_display_try_commit_atomic(display, DRM_MODE_ATOMIC_TEST_ONLY |
 					    DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
+	if (!ret && clone) {
+		/* Try and find a clone */
+		int i, newret;
+		*clone = NULL;
+
+		for (i = 0; i < display->n_outputs; i++) {
+			igt_output_t *second_output = &display->outputs[i];
+			if (output != second_output &&
+			    igt_pipe_connector_valid(pipe, second_output)) {
+
+				igt_output_clone_pipe(second_output, pipe);
+				igt_output_override_mode(output, &override_mode);
+				newret = igt_display_try_commit_atomic(display, DRM_MODE_ATOMIC_TEST_ONLY |
+								       DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
+				igt_output_set_pipe(second_output, PIPE_NONE);
+				igt_debug("try_commit_atomic clone returned %d\n", newret);
+				if (!newret) {
+					*clone = second_output;
+					igt_debug("Selected clone %s\n", (*clone)->name);
+					break;
+				}
+			}
+		}
+	}
 	igt_plane_set_fb(plane, NULL);
 	igt_remove_fb(display->drm_fd, &input_fb);
 	igt_remove_fb(display->drm_fd, &output_fb);
@@ -98,7 +123,8 @@ static bool check_writeback_config(igt_display_t *display, igt_output_t *output)
 	return !ret;
 }
 
-static igt_output_t *kms_writeback_get_output(igt_display_t *display)
+static igt_output_t *kms_writeback_get_output(igt_display_t *display, enum pipe *pipe,
+					      igt_output_t **clone)
 {
 	int i;
 
@@ -114,10 +140,16 @@ static igt_output_t *kms_writeback_get_output(igt_display_t *display)
 		for (j = 0; j < igt_display_get_n_pipes(display); j++) {
 			igt_output_set_pipe(output, j);
 
-			if (check_writeback_config(display, output)) {
+			if (check_writeback_config(display, output, j, clone)) {
 				igt_debug("Using connector %u:%s on pipe %d\n",
 					  output->config.connector->connector_id,
 					  output->name, j);
+				if (clone && *clone)
+					igt_debug("Cloning to connector %u:%s\n",
+						  (*clone)->config.connector->connector_id,
+						  (*clone)->name);
+				if (pipe)
+					*pipe = j;
 				return output;
 			}
 		}
@@ -338,10 +370,11 @@ static void writeback_check_output(igt_output_t *output, igt_plane_t *plane,
 igt_main
 {
 	igt_display_t display;
-	igt_output_t *output;
+	igt_output_t *output, *clone;
 	igt_plane_t *plane;
 	igt_fb_t input_fb;
 	drmModeModeInfo mode;
+	enum pipe pipe;
 	int ret;
 
 	memset(&display, 0, sizeof(display));
@@ -356,7 +389,7 @@ igt_main
 
 		igt_require(display.is_atomic);
 
-		output = kms_writeback_get_output(&display);
+		output = kms_writeback_get_output(&display, &pipe, &clone);
 		igt_require(output);
 
 		if (output->use_override_mode)
@@ -434,6 +467,27 @@ igt_main
 		igt_remove_fb(display.drm_fd, &output_fb);
 	}
 
+	igt_subtest("writeback-check-output-clone") {
+		igt_fb_t output_fb;
+
+		igt_require(clone);
+
+		ret = igt_create_fb(display.drm_fd, mode.hdisplay, mode.vdisplay,
+				    DRM_FORMAT_XRGB8888,
+				    igt_fb_mod_to_tiling(0),
+				    &output_fb);
+		igt_require(ret > 0);
+
+		igt_output_clone_pipe(clone, pipe);
+		igt_output_override_mode(clone, &mode);
+
+		writeback_check_output(output, plane, &input_fb, &output_fb);
+
+		igt_output_set_pipe(clone, PIPE_NONE);
+
+		igt_remove_fb(display.drm_fd, &output_fb);
+	}
+
 	igt_fixture {
 		igt_remove_fb(display.drm_fd, &input_fb);
 		igt_display_fini(&display);
-- 
2.19.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] ✗ Fi.CI.BAT: failure for igt: Add support for testing writeback connectors (rev2)

[Patchwork]

== Series Details ==

Series: igt: Add support for testing writeback connectors (rev2)
URL   : https://patchwork.freedesktop.org/series/39229/
State : failure

== Summary ==

= CI Bug Log - changes from CI_DRM_5081 -> IGTPW_2037 =

== Summary - FAILURE ==

  Serious unknown changes coming with IGTPW_2037 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in IGTPW_2037, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/39229/revisions/2/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in IGTPW_2037:

  === IGT changes ===

    ==== Possible regressions ====

    igt@debugfs_test@read_all_entries:
      fi-skl-iommu:       PASS -> FAIL
      fi-glk-dsi:         PASS -> FAIL
      fi-ivb-3770:        PASS -> FAIL
      fi-cfl-s3:          PASS -> FAIL
      fi-hsw-peppy:       PASS -> FAIL
      fi-bdw-gvtdvm:      PASS -> FAIL
      fi-skl-6700hq:      PASS -> FAIL
      fi-bxt-j4205:       PASS -> FAIL
      fi-kbl-7500u:       PASS -> FAIL
      fi-cfl-8109u:       PASS -> FAIL
      fi-ivb-3520m:       PASS -> FAIL
      fi-skl-6770hq:      PASS -> FAIL
      fi-skl-6260u:       PASS -> FAIL
      fi-snb-2600:        PASS -> FAIL
      fi-hsw-4770r:       PASS -> FAIL
      fi-skl-gvtdvm:      PASS -> FAIL
      fi-kbl-guc:         PASS -> FAIL
      fi-bsw-kefka:       PASS -> FAIL
      fi-kbl-x1275:       PASS -> FAIL
      fi-bdw-5557u:       PASS -> FAIL
      fi-kbl-r:           PASS -> FAIL
      fi-skl-guc:         PASS -> FAIL
      fi-kbl-7567u:       PASS -> FAIL
      fi-apl-guc:         PASS -> FAIL
      fi-cnl-u:           PASS -> FAIL
      fi-glk-j4005:       PASS -> FAIL
      fi-byt-clapper:     PASS -> FAIL
      fi-skl-6600u:       PASS -> FAIL
      fi-byt-j1900:       PASS -> FAIL
      fi-bxt-dsi:         PASS -> FAIL
      fi-kbl-7560u:       PASS -> FAIL
      fi-cfl-8700k:       PASS -> FAIL
      fi-whl-u:           PASS -> FAIL
      fi-bsw-n3050:       PASS -> FAIL
      fi-skl-6700k2:      PASS -> FAIL
      fi-hsw-4770:        PASS -> FAIL
      fi-cfl-guc:         PASS -> FAIL

    
== Known issues ==

  Here are the changes found in IGTPW_2037 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_exec_suspend@basic-s3:
      fi-cfl-8109u:       PASS -> FAIL (fdo#103375)
      fi-blb-e6850:       PASS -> INCOMPLETE (fdo#107718)

    igt@kms_frontbuffer_tracking@basic:
      fi-byt-clapper:     PASS -> FAIL (fdo#103167)

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
      fi-byt-clapper:     PASS -> FAIL (fdo#107362, fdo#103191) +1

    
    ==== Possible fixes ====

    igt@gem_cpu_reloc@basic:
      fi-kbl-7560u:       INCOMPLETE (fdo#103665) -> PASS

    igt@kms_flip@basic-flip-vs-modeset:
      fi-skl-6700hq:      DMESG-WARN (fdo#105998) -> PASS +1

    
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#103375 https://bugs.freedesktop.org/show_bug.cgi?id=103375
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#105998 https://bugs.freedesktop.org/show_bug.cgi?id=105998
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
  fdo#107718 https://bugs.freedesktop.org/show_bug.cgi?id=107718


== Participating hosts (50 -> 43) ==

  Missing    (7): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-icl-u2 fi-bsw-cyan fi-ctg-p8600 fi-icl-u 


== Build changes ==

    * IGT: IGT_4705 -> IGTPW_2037

  CI_DRM_5081: f5e16acf6c85d38756c3efdb77ec6aede55df0ba @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_2037: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2037/
  IGT_4705: 7983e19ed62ec8db1884f55e07e458a62cc51e37 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools



== Testlist changes ==

+igt@kms_writeback@writeback-check-output
+igt@kms_writeback@writeback-check-output-clone
+igt@kms_writeback@writeback-fb-id
+igt@kms_writeback@writeback-invalid-out-fence
+igt@kms_writeback@writeback-pixel-formats

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2037/issues.html
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t v4 4/6] kms_writeback: Add writeback-check-output

[Maarten Lankhorst]

Op 05-11-18 om 11:16 schreef Liviu Dudau:
> From: Brian Starkey <brian.starkey@arm.com>
>
> Add a test which makes commits using the writeback connector, and
> checks the output buffer hash to make sure it is/isn't written as
> appropriate.
>
> Signed-off-by: Brian Starkey <brian.starkey@arm.com>
> [rebased and addressed comments from Maarten]
> Signed-off-by: Liviu Dudau <liviu.dudau@arm.com>
>
> ---
>  tests/kms_writeback.c | 127 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 127 insertions(+)
>
> diff --git a/tests/kms_writeback.c b/tests/kms_writeback.c
> index 636777d0b..7cb65207e 100644
> --- a/tests/kms_writeback.c
> +++ b/tests/kms_writeback.c
> @@ -30,6 +30,7 @@
>  #include "igt.h"
>  #include "igt_core.h"
>  #include "igt_fb.h"
> +#include "sw_sync.h"
>  
>  static drmModePropertyBlobRes *get_writeback_formats_blob(igt_output_t *output)
>  {
> @@ -221,6 +222,119 @@ static void writeback_fb_id(igt_output_t *output, igt_fb_t *valid_fb, igt_fb_t *
>  	igt_assert(ret == 0);
>  }
>  
> +static void fill_fb(igt_fb_t *fb, double color[3])
> +{
> +	cairo_t *cr = igt_get_cairo_ctx(fb->fd, fb);
> +	igt_assert(cr);
> +
> +	igt_paint_color(cr, 0, 0, fb->width, fb->height,
> +			color[0], color[1], color[2]);
Missing a igt_put_cairo_ctx here :)
> +}
> +
> +static void get_and_wait_out_fence(igt_output_t *output)
> +{
> +	int ret;
> +
> +	igt_assert(output->writeback_out_fence_fd >= 0);
This is already in atomic commit?
> +	ret = sync_fence_wait(output->writeback_out_fence_fd, 1000);
> +	igt_assert(ret == 0);
> +	close(output->writeback_out_fence_fd);
I think this would cause a double close, best just leave it open.
> +}
> +
> +static void writeback_sequence(igt_output_t *output, igt_plane_t *plane,
> +				igt_fb_t *in_fb, igt_fb_t *out_fbs[], int n_commits)
> +{
> +	int i, color_idx = 0;
> +	double in_fb_colors[2][3] = {
> +		{ 1.0, 0.0, 0.0 },
> +		{ 0.0, 1.0, 0.0 },
> +	};
> +	double clear_color[3] = { 1.0, 1.0, 1.0 };
> +	igt_crc_t cleared_crc, out_expected;
> +
> +	for (i = 0; i < n_commits; i++, color_idx++) {
> +		/* Change the input color each time */
> +		fill_fb(in_fb, in_fb_colors[color_idx % 2]);
> +
> +		if (out_fbs[i]) {
> +			igt_crc_t out_before;
> +
> +			/* Get the expected CRC */
> +			fill_fb(out_fbs[i], in_fb_colors[color_idx % 2]);
> +			igt_fb_get_crc(out_fbs[i], &out_expected);
> +
> +			fill_fb(out_fbs[i], clear_color);
> +			if (i == 0)
> +				igt_fb_get_crc(out_fbs[i], &cleared_crc);
> +			igt_fb_get_crc(out_fbs[i], &out_before);
> +			igt_assert_crc_equal(&cleared_crc, &out_before);
> +		}
> +
> +		/* Commit */
> +		igt_plane_set_fb(plane, in_fb);
> +		if (out_fbs[i])
> +			igt_output_set_writeback_fb(output, out_fbs[i]);
> +		else
> +			/* avoid setting the out fence ptr that is done via igt_output_set_writeback_fb() */
> +			igt_output_set_prop_value(output, IGT_CONNECTOR_WRITEBACK_FB_ID, 0);
Seems like we should handle the null fb case in igt_output_set_writeback_fb ?
> +
> +		igt_display_commit_atomic(output->display,
> +					  DRM_MODE_ATOMIC_ALLOW_MODESET,
> +					  NULL);
> +		if (out_fbs[i])
> +			get_and_wait_out_fence(output);

> +		/* Make sure the old output buffer is untouched */
> +		if (i > 0 && out_fbs[i - 1] && (out_fbs[i] != out_fbs[i - 1])) {
> +			igt_crc_t out_prev;
> +			igt_fb_get_crc(out_fbs[i - 1], &out_prev);
Maybe make the wait part of the API?

igt_output_fb_get_crc () ?
> +			igt_assert_crc_equal(&cleared_crc, &out_prev);
> +		}
> +
> +		/* Make sure this output buffer is written */
> +		if (out_fbs[i]) {
> +			igt_crc_t out_after;
> +			igt_fb_get_crc(out_fbs[i], &out_after);
> +			igt_assert_crc_equal(&out_expected, &out_after);
> +
> +			/* And clear it, for the next time */
> +			fill_fb(out_fbs[i], clear_color);
> +		}
> +	}
> +}
> +
> +static void writeback_check_output(igt_output_t *output, igt_plane_t *plane,
> +				   igt_fb_t *input_fb, igt_fb_t *output_fb)
> +{
> +	igt_fb_t *out_fbs[2] = { 0 };
> +	igt_fb_t second_out_fb;
> +	int ret;
> +
> +	/* One commit, with a writeback. */
> +	writeback_sequence(output, plane, input_fb, &output_fb, 1);
> +
> +	/* Two commits, the second with no writeback */
> +	out_fbs[0] = output_fb;
> +	writeback_sequence(output, plane, input_fb, out_fbs, 2);
> +
> +	/* Two commits, both with writeback */
> +	out_fbs[1] = output_fb;
> +	writeback_sequence(output, plane, input_fb, out_fbs, 2);
> +
> +	ret = igt_create_fb(output_fb->fd, output_fb->width, output_fb->height,
> +			    DRM_FORMAT_XRGB8888,
> +			    igt_fb_mod_to_tiling(0),
> +			    &second_out_fb);
> +	igt_require(ret > 0);
> +
> +	/* Two commits, with different writeback buffers */
> +	out_fbs[1] = &second_out_fb;
> +	writeback_sequence(output, plane, input_fb, out_fbs, 2);
> +
> +	igt_remove_fb(output_fb->fd, &second_out_fb);
> +}
> +
>  igt_main
>  {
>  	igt_display_t display;
> @@ -307,6 +421,19 @@ igt_main
>  		igt_remove_fb(display.drm_fd, &output_fb);
>  	}
>  
> +	igt_subtest("writeback-check-output") {
> +		igt_fb_t output_fb;
> +		ret = igt_create_fb(display.drm_fd, mode.hdisplay, mode.vdisplay,
> +				    DRM_FORMAT_XRGB8888,
> +				    igt_fb_mod_to_tiling(0),
> +				    &output_fb);
> +		igt_require(ret > 0);
> +
> +		writeback_check_output(output, plane, &input_fb, &output_fb);
> +
> +		igt_remove_fb(display.drm_fd, &output_fb);
> +	}
> +
>  	igt_fixture {
>  		igt_remove_fb(display.drm_fd, &input_fb);
>  		igt_display_fini(&display);


_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t v4 1/6] lib/igt_kms: Add writeback support

[Brian Starkey]

Hi Liviu,

Thanks for resurrecting this.

On Mon, Nov 05, 2018 at 10:16:11AM +0000, Liviu Dudau wrote:
>From: Brian Starkey <brian.starkey@arm.com>
>
>Add support in igt_kms for writeback connectors, with the ability
>to attach framebuffers.
>
>Signed-off-by: Brian Starkey <brian.starkey@arm.com>
>[rebased and updated to the latest igt style]
>Signed-off-by: Liviu Dudau <liviu.dudau@arm.com>
>---
> include/drm-uapi/drm.h      | 16 ++++++++++++
> include/drm-uapi/drm_mode.h |  1 +
> lib/igt_kms.c               | 50 +++++++++++++++++++++++++++++++++++++
> lib/igt_kms.h               |  6 +++++
> 4 files changed, 73 insertions(+)
>
>diff --git a/include/drm-uapi/drm.h b/include/drm-uapi/drm.h
>index f0bd91de0..85c685a20 100644
>--- a/include/drm-uapi/drm.h
>+++ b/include/drm-uapi/drm.h
>@@ -674,6 +674,22 @@ struct drm_get_cap {
>  */
> #define DRM_CLIENT_CAP_ATOMIC	3
>
>+/**
>+ * DRM_CLIENT_CAP_ASPECT_RATIO
>+ *
>+ * If set to 1, the DRM core will provide aspect ratio information in modes.
>+ */
>+#define DRM_CLIENT_CAP_ASPECT_RATIO    4
>+
>+/**
>+ * DRM_CLIENT_CAP_WRITEBACK_CONNECTORS
>+ *
>+ * If set to 1, the DRM core will expose special connectors to be used for
>+ * writing back to memory the scene setup in the commit. Depends on client
>+ * also supporting DRM_CLIENT_CAP_ATOMIC
>+ */
>+#define DRM_CLIENT_CAP_WRITEBACK_CONNECTORS	5
>+
> /** DRM_IOCTL_SET_CLIENT_CAP ioctl argument type */
> struct drm_set_client_cap {
> 	__u64 capability;
>diff --git a/include/drm-uapi/drm_mode.h b/include/drm-uapi/drm_mode.h
>index 2c575794f..7b47e184e 100644
>--- a/include/drm-uapi/drm_mode.h
>+++ b/include/drm-uapi/drm_mode.h
>@@ -338,6 +338,7 @@ enum drm_mode_subconnector {
> #define DRM_MODE_CONNECTOR_VIRTUAL      15
> #define DRM_MODE_CONNECTOR_DSI		16
> #define DRM_MODE_CONNECTOR_DPI		17
>+#define DRM_MODE_CONNECTOR_WRITEBACK	18
>
> struct drm_mode_get_connector {
>
>diff --git a/lib/igt_kms.c b/lib/igt_kms.c
>index d806ccc1e..00d356fe0 100644
>--- a/lib/igt_kms.c
>+++ b/lib/igt_kms.c
>@@ -197,6 +197,9 @@ const char * const igt_connector_prop_names[IGT_NUM_CONNECTOR_PROPS] = {
> 	[IGT_CONNECTOR_DPMS] = "DPMS",
> 	[IGT_CONNECTOR_BROADCAST_RGB] = "Broadcast RGB",
> 	[IGT_CONNECTOR_CONTENT_PROTECTION] = "Content Protection",
>+	[IGT_CONNECTOR_WRITEBACK_PIXEL_FORMATS] = "WRITEBACK_PIXEL_FORMATS",
>+	[IGT_CONNECTOR_WRITEBACK_FB_ID] = "WRITEBACK_FB_ID",
>+	[IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR] = "WRITEBACK_OUT_FENCE_PTR",
> };
>
> /*
>@@ -453,6 +456,7 @@ static const struct type_name connector_type_names[] = {
> 	{ DRM_MODE_CONNECTOR_VIRTUAL, "Virtual" },
> 	{ DRM_MODE_CONNECTOR_DSI, "DSI" },
> 	{ DRM_MODE_CONNECTOR_DPI, "DPI" },
>+	{ DRM_MODE_CONNECTOR_WRITEBACK, "Writeback" },
> 	{}
> };
>
>@@ -1802,6 +1806,12 @@ static void igt_output_reset(igt_output_t *output)
> 	if (igt_output_has_prop(output, IGT_CONNECTOR_BROADCAST_RGB))
> 		igt_output_set_prop_value(output, IGT_CONNECTOR_BROADCAST_RGB,
> 					  BROADCAST_RGB_FULL);
>+	if (igt_output_has_prop(output, IGT_CONNECTOR_WRITEBACK_FB_ID))
>+		igt_output_set_prop_value(output, IGT_CONNECTOR_WRITEBACK_FB_ID, 0);
>+	if (igt_output_has_prop(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR)) {
>+		igt_output_clear_prop_changed(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR);
>+		output->writeback_out_fence_fd = -1;
>+	}
> }
>
> /**
>@@ -1814,6 +1824,8 @@ static void igt_output_reset(igt_output_t *output)
>  * For outputs:
>  * - %IGT_CONNECTOR_CRTC_ID
>  * - %IGT_CONNECTOR_BROADCAST_RGB (if applicable)
>+ * - %IGT_CONNECTOR_WRITEBACK_FB_ID
>+ * - %IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR
>  * - igt_output_override_mode() to default.
>  *
>  * For pipes:
>@@ -1898,6 +1910,8 @@ bool igt_display_init(igt_display_t *display, int drm_fd)
> 	if (drmSetClientCap(drm_fd, DRM_CLIENT_CAP_ATOMIC, 1) == 0)
> 		display->is_atomic = 1;
>
>+	drmSetClientCap(drm_fd, DRM_CLIENT_CAP_WRITEBACK_CONNECTORS, 1);
>+
> 	plane_resources = drmModeGetPlaneResources(display->drm_fd);
> 	igt_assert(plane_resources);
>
>@@ -2147,6 +2161,9 @@ static void igt_output_fini(igt_output_t *output)
> 	kmstest_free_connector_config(&output->config);
> 	free(output->name);
> 	output->name = NULL;
>+
>+	if (output->writeback_out_fence_fd != -1)
>+		close(output->writeback_out_fence_fd);

Is it worth setting it to -1 here too? output->name looks like it's
set to NULL defensively.

> }
>
> /**
>@@ -3144,6 +3161,11 @@ static void igt_atomic_prepare_connector_commit(igt_output_t *output, drmModeAto
> 					  output->props[i],
> 					  output->values[i]));
> 	}
>+
>+	if (output->writeback_out_fence_fd != -1) {
>+		close(output->writeback_out_fence_fd);
>+		output->writeback_out_fence_fd = -1;
>+	}
> }
>
> /*
>@@ -3264,6 +3286,14 @@ display_commit_changed(igt_display_t *display, enum igt_commit_style s)
> 		else
> 			/* no modeset in universal commit, no change to crtc. */
> 			output->changed &= 1 << IGT_CONNECTOR_CRTC_ID;
>+
>+		if (s == COMMIT_ATOMIC) {
>+			if (igt_output_is_prop_changed(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR))
>+				igt_assert(output->writeback_out_fence_fd >= 0);
>+
>+			output->values[IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR] = 0;
>+			igt_output_clear_prop_changed(output, IGT_CONNECTOR_WRITEBACK_FB_ID);

The WRITEBACK_FB_ID property is auto-clearing, so does that need
special handling here? Might need to set
output->values[IGT_CONNECTOR_WRITEBACK_FB_ID] = 0 and mark it as
changed.

>+		}
> 	}
>
> 	if (display->first_commit) {
>@@ -3884,6 +3914,26 @@ void igt_pipe_request_out_fence(igt_pipe_t *pipe)
> 	igt_pipe_obj_set_prop_value(pipe, IGT_CRTC_OUT_FENCE_PTR, (ptrdiff_t)&pipe->out_fence_fd);
> }
>
>+/**
>+ * igt_output_set_writeback_fb:
>+ * @output: Target output
>+ * @fb: Target framebuffer
>+ *
>+ * This function sets the given @fb to be used as the target framebuffer for the
>+ * writeback engine at the next atomic commit. It will also request a writeback
>+ * out fence that will contain the fd number of the out fence created by KMS.
>+ */
>+void igt_output_set_writeback_fb(igt_output_t *output, struct igt_fb *fb)
>+{
>+	igt_display_t *display = output->display;
>+
>+	LOG(display, "%s: output_set_writeback_fb(%d)\n", output->name, fb ? fb->fb_id : 0);
>+
>+	igt_output_set_prop_value(output, IGT_CONNECTOR_WRITEBACK_FB_ID, fb ? fb->fb_id : 0);
>+	igt_output_set_prop_value(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR,
>+				  (ptrdiff_t)&output->writeback_out_fence_fd);

Did you mean ptrdiff_t? Might have been a typo in the
CRTC_OUT_FENCE_PTR

Also commented on a later patch - but requesting the out-fence could
be dependent on fb_id != 0, then you wouldn't need to open-code that
condition in the sequence test.

Cheers,
-Brian

>+}
>+
> /**
>  * igt_wait_for_vblank_count:
>  * @drm_fd: A drm file descriptor
>diff --git a/lib/igt_kms.h b/lib/igt_kms.h
>index e6aff339a..a84f9ac42 100644
>--- a/lib/igt_kms.h
>+++ b/lib/igt_kms.h
>@@ -121,6 +121,9 @@ enum igt_atomic_connector_properties {
>        IGT_CONNECTOR_DPMS,
>        IGT_CONNECTOR_BROADCAST_RGB,
>        IGT_CONNECTOR_CONTENT_PROTECTION,
>+       IGT_CONNECTOR_WRITEBACK_PIXEL_FORMATS,
>+       IGT_CONNECTOR_WRITEBACK_FB_ID,
>+       IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR,
>        IGT_NUM_CONNECTOR_PROPS
> };
>
>@@ -359,6 +362,8 @@ typedef struct {
> 	bool use_override_mode;
> 	drmModeModeInfo override_mode;
>
>+	int32_t writeback_out_fence_fd;
>+
> 	/* bitmask of changed properties */
> 	uint64_t changed;
>
>@@ -404,6 +409,7 @@ igt_plane_t *igt_output_get_plane(igt_output_t *output, int plane_idx);
> igt_plane_t *igt_output_get_plane_type(igt_output_t *output, int plane_type);
> igt_output_t *igt_output_from_connector(igt_display_t *display,
>     drmModeConnector *connector);
>+void igt_output_set_writeback_fb(igt_output_t *output, struct igt_fb *fb);
>
> igt_plane_t *igt_pipe_get_plane_type(igt_pipe_t *pipe, int plane_type);
> igt_output_t *igt_get_single_output_for_pipe(igt_display_t *display, enum pipe pipe);
>-- 
>2.19.1
>
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t v4 4/6] kms_writeback: Add writeback-check-output

[Brian Starkey]

Hi,

On Mon, Nov 05, 2018 at 10:16:14AM +0000, Liviu Dudau wrote:
>From: Brian Starkey <brian.starkey@arm.com>
>
>Add a test which makes commits using the writeback connector, and
>checks the output buffer hash to make sure it is/isn't written as
>appropriate.
>
>Signed-off-by: Brian Starkey <brian.starkey@arm.com>
>[rebased and addressed comments from Maarten]
>Signed-off-by: Liviu Dudau <liviu.dudau@arm.com>
>
>---
> tests/kms_writeback.c | 127 ++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 127 insertions(+)
>
>diff --git a/tests/kms_writeback.c b/tests/kms_writeback.c
>index 636777d0b..7cb65207e 100644
>--- a/tests/kms_writeback.c
>+++ b/tests/kms_writeback.c
>@@ -30,6 +30,7 @@
> #include "igt.h"
> #include "igt_core.h"
> #include "igt_fb.h"
>+#include "sw_sync.h"
>
> static drmModePropertyBlobRes *get_writeback_formats_blob(igt_output_t *output)
> {
>@@ -221,6 +222,119 @@ static void writeback_fb_id(igt_output_t *output, igt_fb_t *valid_fb, igt_fb_t *
> 	igt_assert(ret == 0);
> }
>
>+static void fill_fb(igt_fb_t *fb, double color[3])
>+{
>+	cairo_t *cr = igt_get_cairo_ctx(fb->fd, fb);
>+	igt_assert(cr);
>+
>+	igt_paint_color(cr, 0, 0, fb->width, fb->height,
>+			color[0], color[1], color[2]);
>+}
>+
>+static void get_and_wait_out_fence(igt_output_t *output)
>+{
>+	int ret;
>+
>+	igt_assert(output->writeback_out_fence_fd >= 0);
>+
>+	ret = sync_fence_wait(output->writeback_out_fence_fd, 1000);
>+	igt_assert(ret == 0);
>+	close(output->writeback_out_fence_fd);

Setting to -1 could be helpful here too. Tracking down double-closed
fds is always a pain.

>+}
>+
>+static void writeback_sequence(igt_output_t *output, igt_plane_t *plane,
>+				igt_fb_t *in_fb, igt_fb_t *out_fbs[], int n_commits)
>+{
>+	int i, color_idx = 0;
>+	double in_fb_colors[2][3] = {
>+		{ 1.0, 0.0, 0.0 },
>+		{ 0.0, 1.0, 0.0 },
>+	};
>+	double clear_color[3] = { 1.0, 1.0, 1.0 };
>+	igt_crc_t cleared_crc, out_expected;
>+
>+	for (i = 0; i < n_commits; i++, color_idx++) {
>+		/* Change the input color each time */
>+		fill_fb(in_fb, in_fb_colors[color_idx % 2]);
>+
>+		if (out_fbs[i]) {
>+			igt_crc_t out_before;
>+
>+			/* Get the expected CRC */
>+			fill_fb(out_fbs[i], in_fb_colors[color_idx % 2]);
>+			igt_fb_get_crc(out_fbs[i], &out_expected);
>+
>+			fill_fb(out_fbs[i], clear_color);
>+			if (i == 0)
>+				igt_fb_get_crc(out_fbs[i], &cleared_crc);
>+			igt_fb_get_crc(out_fbs[i], &out_before);
>+			igt_assert_crc_equal(&cleared_crc, &out_before);
>+		}
>+
>+		/* Commit */
>+		igt_plane_set_fb(plane, in_fb);
>+		if (out_fbs[i])
>+			igt_output_set_writeback_fb(output, out_fbs[i]);
>+		else
>+			/* avoid setting the out fence ptr that is done via igt_output_set_writeback_fb() */
>+			igt_output_set_prop_value(output, IGT_CONNECTOR_WRITEBACK_FB_ID, 0);

Would it work to put this check in igt_output_set_writeback_fb() so
that it only requests an out-fence if fb_id != 0 ?

Cheers,
-Brian

>+
>+		igt_display_commit_atomic(output->display,
>+					  DRM_MODE_ATOMIC_ALLOW_MODESET,
>+					  NULL);
>+		if (out_fbs[i])
>+			get_and_wait_out_fence(output);
>+
>+		/* Make sure the old output buffer is untouched */
>+		if (i > 0 && out_fbs[i - 1] && (out_fbs[i] != out_fbs[i - 1])) {
>+			igt_crc_t out_prev;
>+			igt_fb_get_crc(out_fbs[i - 1], &out_prev);
>+			igt_assert_crc_equal(&cleared_crc, &out_prev);
>+		}
>+
>+		/* Make sure this output buffer is written */
>+		if (out_fbs[i]) {
>+			igt_crc_t out_after;
>+			igt_fb_get_crc(out_fbs[i], &out_after);
>+			igt_assert_crc_equal(&out_expected, &out_after);
>+
>+			/* And clear it, for the next time */
>+			fill_fb(out_fbs[i], clear_color);
>+		}
>+	}
>+}
>+
>+static void writeback_check_output(igt_output_t *output, igt_plane_t *plane,
>+				   igt_fb_t *input_fb, igt_fb_t *output_fb)
>+{
>+	igt_fb_t *out_fbs[2] = { 0 };
>+	igt_fb_t second_out_fb;
>+	int ret;
>+
>+	/* One commit, with a writeback. */
>+	writeback_sequence(output, plane, input_fb, &output_fb, 1);
>+
>+	/* Two commits, the second with no writeback */
>+	out_fbs[0] = output_fb;
>+	writeback_sequence(output, plane, input_fb, out_fbs, 2);
>+
>+	/* Two commits, both with writeback */
>+	out_fbs[1] = output_fb;
>+	writeback_sequence(output, plane, input_fb, out_fbs, 2);
>+
>+	ret = igt_create_fb(output_fb->fd, output_fb->width, output_fb->height,
>+			    DRM_FORMAT_XRGB8888,
>+			    igt_fb_mod_to_tiling(0),
>+			    &second_out_fb);
>+	igt_require(ret > 0);
>+
>+	/* Two commits, with different writeback buffers */
>+	out_fbs[1] = &second_out_fb;
>+	writeback_sequence(output, plane, input_fb, out_fbs, 2);
>+
>+	igt_remove_fb(output_fb->fd, &second_out_fb);
>+}
>+
> igt_main
> {
> 	igt_display_t display;
>@@ -307,6 +421,19 @@ igt_main
> 		igt_remove_fb(display.drm_fd, &output_fb);
> 	}
>
>+	igt_subtest("writeback-check-output") {
>+		igt_fb_t output_fb;
>+		ret = igt_create_fb(display.drm_fd, mode.hdisplay, mode.vdisplay,
>+				    DRM_FORMAT_XRGB8888,
>+				    igt_fb_mod_to_tiling(0),
>+				    &output_fb);
>+		igt_require(ret > 0);
>+
>+		writeback_check_output(output, plane, &input_fb, &output_fb);
>+
>+		igt_remove_fb(display.drm_fd, &output_fb);
>+	}
>+
> 	igt_fixture {
> 		igt_remove_fb(display.drm_fd, &input_fb);
> 		igt_display_fini(&display);
>-- 
>2.19.1
>
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t v4 4/6] kms_writeback: Add writeback-check-output

[Maarten Lankhorst]

Op 09-11-18 om 16:09 schreef Brian Starkey:
> Hi,
>
> On Mon, Nov 05, 2018 at 10:16:14AM +0000, Liviu Dudau wrote:
>> From: Brian Starkey <brian.starkey@arm.com>
>>
>> Add a test which makes commits using the writeback connector, and
>> checks the output buffer hash to make sure it is/isn't written as
>> appropriate.
>>
>> Signed-off-by: Brian Starkey <brian.starkey@arm.com>
>> [rebased and addressed comments from Maarten]
>> Signed-off-by: Liviu Dudau <liviu.dudau@arm.com>
>>
>> ---
>> tests/kms_writeback.c | 127 ++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 127 insertions(+)
>>
>> diff --git a/tests/kms_writeback.c b/tests/kms_writeback.c
>> index 636777d0b..7cb65207e 100644
>> --- a/tests/kms_writeback.c
>> +++ b/tests/kms_writeback.c
>> @@ -30,6 +30,7 @@
>> #include "igt.h"
>> #include "igt_core.h"
>> #include "igt_fb.h"
>> +#include "sw_sync.h"
>>
>> static drmModePropertyBlobRes *get_writeback_formats_blob(igt_output_t *output)
>> {
>> @@ -221,6 +222,119 @@ static void writeback_fb_id(igt_output_t *output, igt_fb_t *valid_fb, igt_fb_t *
>> 	igt_assert(ret == 0);
>> }
>>
>> +static void fill_fb(igt_fb_t *fb, double color[3])
>> +{
>> +	cairo_t *cr = igt_get_cairo_ctx(fb->fd, fb);
>> +	igt_assert(cr);
>> +
>> +	igt_paint_color(cr, 0, 0, fb->width, fb->height,
>> +			color[0], color[1], color[2]);
>> +}
>> +
>> +static void get_and_wait_out_fence(igt_output_t *output)
>> +{
>> +	int ret;
>> +
>> +	igt_assert(output->writeback_out_fence_fd >= 0);
>> +
>> +	ret = sync_fence_wait(output->writeback_out_fence_fd, 1000);
>> +	igt_assert(ret == 0);
>> +	close(output->writeback_out_fence_fd);
> Setting to -1 could be helpful here too. Tracking down double-closed
> fds is always a pain.

I'm advertising keeping it open to keep the lifetime tracked by igt_display,
there's no point in handling it in the test unnecessarily. :)

We don't close them for crtc out fences either.

kms_atomic_transition.c:wait_for_transition()

In the same test, run_transition_test() TRANSITION_AFTER_FREE
does close it manaully, but it has to keep the fd after completion, to test
the fence is signaled as intended.

~Maarten

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t v4 5/6] lib/igt_kms: Add igt_output_clone_pipe for cloning

[Maarten Lankhorst]

Op 05-11-18 om 11:16 schreef Liviu Dudau:
> From: Brian Starkey <brian.starkey@arm.com>
>
> An output can be added as a clone of any other output(s) attached to a
> pipe using igt_output_clone_pipe()
>
> Signed-off-by: Brian Starkey <brian.starkey@arm.com>
> ---
>  lib/igt_kms.c | 99 +++++++++++++++++++++++++++++++--------------------
>  lib/igt_kms.h |  5 +++
>  2 files changed, 66 insertions(+), 38 deletions(-)
>
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index 00d356fe0..0a826718e 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -1694,6 +1694,17 @@ static void igt_display_log_shift(igt_display_t *display, int shift)
>  	igt_assert(display->log_shift >= 0);
>  }
>  
> +static int igt_output_idx(igt_output_t *output)
> +{
> +	int i;
> +
> +	for (i = 0; i < output->display->n_outputs; i++)
> +		if (&output->display->outputs[i] == output)
> +			return i;
> +
> +	return -1;
> +}
> +
>  static void igt_output_refresh(igt_output_t *output)
>  {
>  	igt_display_t *display = output->display;
> @@ -2188,42 +2199,6 @@ void igt_display_fini(igt_display_t *display)
>  	display->pipes = NULL;
>  }
>  
> -static void igt_display_refresh(igt_display_t *display)
> -{
> -	igt_output_t *output;
> -	int i;
> -
> -	unsigned long pipes_in_use = 0;
> -
> -       /* Check that two outputs aren't trying to use the same pipe */
> -	for (i = 0; i < display->n_outputs; i++) {
> -		output = &display->outputs[i];
> -
> -		if (output->pending_pipe != PIPE_NONE) {
> -			if (pipes_in_use & (1 << output->pending_pipe))
> -				goto report_dup;
> -
> -			pipes_in_use |= 1 << output->pending_pipe;
> -		}
> -
> -		if (output->force_reprobe)
> -			igt_output_refresh(output);
> -	}
> -
> -	return;
> -
> -report_dup:
> -	for (; i > 0; i--) {
> -		igt_output_t *b = &display->outputs[i - 1];
> -
> -		igt_assert_f(output->pending_pipe !=
> -			     b->pending_pipe,
> -			     "%s and %s are both trying to use pipe %s\n",
> -			     igt_output_name(output), igt_output_name(b),
> -			     kmstest_pipe_name(output->pending_pipe));
> -	}
> -}
> -
>  static igt_pipe_t *igt_output_get_driving_pipe(igt_output_t *output)
>  {
>  	igt_display_t *display = output->display;
> @@ -2247,6 +2222,39 @@ static igt_pipe_t *igt_output_get_driving_pipe(igt_output_t *output)
>  	return &display->pipes[pipe];
>  }
>  
> +static void igt_display_refresh(igt_display_t *display)
> +{
> +	igt_output_t *output;
> +	igt_pipe_t *pipe;
> +	int i;
> +
> +	unsigned long pipes_in_use = 0;
> +
> +	/* Check that outputs and pipes agree wrt. cloning */
> +	for (i = 0; i < display->n_outputs; i++) {
> +		output = &display->outputs[i];
> +		unsigned long pending_crtc_idx_mask = 1 << output->pending_pipe;
> +
> +		pipe = igt_output_get_driving_pipe(output);
> +		if (pipe) {
> +			igt_assert_f(pipe->outputs & (1 << igt_output_idx(output)),
> +				     "Output %s not expected to be using pipe %s\n",
> +				     igt_output_name(output),
> +				     kmstest_pipe_name(pipe->pipe));
> +
> +			if (pipes_in_use & pending_crtc_idx_mask)
> +				LOG(display, "Output %s clones pipe %s\n",
> +				    igt_output_name(output),
> +				    kmstest_pipe_name(pipe->pipe));
> +		}
> +
> +		pipes_in_use |= pending_crtc_idx_mask;
> +
> +		if (output->force_reprobe)
> +			igt_output_refresh(output);
> +	}
> +}
> +
>  static igt_plane_t *igt_pipe_get_plane(igt_pipe_t *pipe, int plane_idx)
>  {
>  	igt_require_f(plane_idx >= 0 && plane_idx < pipe->n_planes,
> @@ -3583,6 +3591,7 @@ void igt_output_override_mode(igt_output_t *output, drmModeModeInfo *mode)
>  	output->use_override_mode = !!mode;
>  
>  	if (pipe) {
> +		igt_debug("overriding pipe mode in %s way\n", output->display->is_atomic ? "atomic" : "legacy");
>  		if (output->display->is_atomic)
>  			igt_pipe_obj_replace_prop_blob(pipe, IGT_CRTC_MODE_ID, igt_output_get_mode(output), sizeof(*mode));
>  		else

I would move the mode being set to igt_pipe, kill off the internal
igt_pipe_get_output function; replace igt_output_override_mode()
with igt_pipe(,_obj)_override_mode functions, and fix
igt_primary_commit_legacy to use more than 1 output.

These are all the places why we assume only 1 pipe exists.

Looks also like igt_pipe_refresh might be replaced with igt_display_reset,
or at least igt_pipe_obj_set_prop_changed must be aclled unconditionally,
but that's not necessarily a big change.

~Maarten

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t v4 1/6] lib/igt_kms: Add writeback support

[Liviu Dudau]

On Fri, Nov 09, 2018 at 03:09:41PM +0000, Brian Starkey wrote:
> Hi Liviu,
> 
> Thanks for resurrecting this.
> 
> On Mon, Nov 05, 2018 at 10:16:11AM +0000, Liviu Dudau wrote:
> >From: Brian Starkey <brian.starkey@arm.com>
> >
> >Add support in igt_kms for writeback connectors, with the ability
> >to attach framebuffers.
> >
> >Signed-off-by: Brian Starkey <brian.starkey@arm.com>
> >[rebased and updated to the latest igt style]
> >Signed-off-by: Liviu Dudau <liviu.dudau@arm.com>
> >---
> > include/drm-uapi/drm.h      | 16 ++++++++++++
> > include/drm-uapi/drm_mode.h |  1 +
> > lib/igt_kms.c               | 50 +++++++++++++++++++++++++++++++++++++
> > lib/igt_kms.h               |  6 +++++
> > 4 files changed, 73 insertions(+)
> >
> >diff --git a/include/drm-uapi/drm.h b/include/drm-uapi/drm.h
> >index f0bd91de0..85c685a20 100644
> >--- a/include/drm-uapi/drm.h
> >+++ b/include/drm-uapi/drm.h
> >@@ -674,6 +674,22 @@ struct drm_get_cap {
> >  */
> > #define DRM_CLIENT_CAP_ATOMIC	3
> >
> >+/**
> >+ * DRM_CLIENT_CAP_ASPECT_RATIO
> >+ *
> >+ * If set to 1, the DRM core will provide aspect ratio information in modes.
> >+ */
> >+#define DRM_CLIENT_CAP_ASPECT_RATIO    4
> >+
> >+/**
> >+ * DRM_CLIENT_CAP_WRITEBACK_CONNECTORS
> >+ *
> >+ * If set to 1, the DRM core will expose special connectors to be used for
> >+ * writing back to memory the scene setup in the commit. Depends on client
> >+ * also supporting DRM_CLIENT_CAP_ATOMIC
> >+ */
> >+#define DRM_CLIENT_CAP_WRITEBACK_CONNECTORS	5
> >+
> > /** DRM_IOCTL_SET_CLIENT_CAP ioctl argument type */
> > struct drm_set_client_cap {
> > 	__u64 capability;
> >diff --git a/include/drm-uapi/drm_mode.h b/include/drm-uapi/drm_mode.h
> >index 2c575794f..7b47e184e 100644
> >--- a/include/drm-uapi/drm_mode.h
> >+++ b/include/drm-uapi/drm_mode.h
> >@@ -338,6 +338,7 @@ enum drm_mode_subconnector {
> > #define DRM_MODE_CONNECTOR_VIRTUAL      15
> > #define DRM_MODE_CONNECTOR_DSI		16
> > #define DRM_MODE_CONNECTOR_DPI		17
> >+#define DRM_MODE_CONNECTOR_WRITEBACK	18
> >
> > struct drm_mode_get_connector {
> >
> >diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> >index d806ccc1e..00d356fe0 100644
> >--- a/lib/igt_kms.c
> >+++ b/lib/igt_kms.c
> >@@ -197,6 +197,9 @@ const char * const igt_connector_prop_names[IGT_NUM_CONNECTOR_PROPS] = {
> > 	[IGT_CONNECTOR_DPMS] = "DPMS",
> > 	[IGT_CONNECTOR_BROADCAST_RGB] = "Broadcast RGB",
> > 	[IGT_CONNECTOR_CONTENT_PROTECTION] = "Content Protection",
> >+	[IGT_CONNECTOR_WRITEBACK_PIXEL_FORMATS] = "WRITEBACK_PIXEL_FORMATS",
> >+	[IGT_CONNECTOR_WRITEBACK_FB_ID] = "WRITEBACK_FB_ID",
> >+	[IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR] = "WRITEBACK_OUT_FENCE_PTR",
> > };
> >
> > /*
> >@@ -453,6 +456,7 @@ static const struct type_name connector_type_names[] = {
> > 	{ DRM_MODE_CONNECTOR_VIRTUAL, "Virtual" },
> > 	{ DRM_MODE_CONNECTOR_DSI, "DSI" },
> > 	{ DRM_MODE_CONNECTOR_DPI, "DPI" },
> >+	{ DRM_MODE_CONNECTOR_WRITEBACK, "Writeback" },
> > 	{}
> > };
> >
> >@@ -1802,6 +1806,12 @@ static void igt_output_reset(igt_output_t *output)
> > 	if (igt_output_has_prop(output, IGT_CONNECTOR_BROADCAST_RGB))
> > 		igt_output_set_prop_value(output, IGT_CONNECTOR_BROADCAST_RGB,
> > 					  BROADCAST_RGB_FULL);
> >+	if (igt_output_has_prop(output, IGT_CONNECTOR_WRITEBACK_FB_ID))
> >+		igt_output_set_prop_value(output, IGT_CONNECTOR_WRITEBACK_FB_ID, 0);
> >+	if (igt_output_has_prop(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR)) {
> >+		igt_output_clear_prop_changed(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR);
> >+		output->writeback_out_fence_fd = -1;
> >+	}
> > }
> >
> > /**
> >@@ -1814,6 +1824,8 @@ static void igt_output_reset(igt_output_t *output)
> >  * For outputs:
> >  * - %IGT_CONNECTOR_CRTC_ID
> >  * - %IGT_CONNECTOR_BROADCAST_RGB (if applicable)
> >+ * - %IGT_CONNECTOR_WRITEBACK_FB_ID
> >+ * - %IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR
> >  * - igt_output_override_mode() to default.
> >  *
> >  * For pipes:
> >@@ -1898,6 +1910,8 @@ bool igt_display_init(igt_display_t *display, int drm_fd)
> > 	if (drmSetClientCap(drm_fd, DRM_CLIENT_CAP_ATOMIC, 1) == 0)
> > 		display->is_atomic = 1;
> >
> >+	drmSetClientCap(drm_fd, DRM_CLIENT_CAP_WRITEBACK_CONNECTORS, 1);
> >+
> > 	plane_resources = drmModeGetPlaneResources(display->drm_fd);
> > 	igt_assert(plane_resources);
> >
> >@@ -2147,6 +2161,9 @@ static void igt_output_fini(igt_output_t *output)
> > 	kmstest_free_connector_config(&output->config);
> > 	free(output->name);
> > 	output->name = NULL;
> >+
> >+	if (output->writeback_out_fence_fd != -1)
> >+		close(output->writeback_out_fence_fd);
> 
> Is it worth setting it to -1 here too? output->name looks like it's
> set to NULL defensively.

I was following the style of igt_pipe_fini() where the out_fence_fd was
not set to -1 after closing, but I agree, it is probably safer to set it so.

> 
> > }
> >
> > /**
> >@@ -3144,6 +3161,11 @@ static void igt_atomic_prepare_connector_commit(igt_output_t *output, drmModeAto
> > 					  output->props[i],
> > 					  output->values[i]));
> > 	}
> >+
> >+	if (output->writeback_out_fence_fd != -1) {
> >+		close(output->writeback_out_fence_fd);
> >+		output->writeback_out_fence_fd = -1;
> >+	}
> > }
> >
> > /*
> >@@ -3264,6 +3286,14 @@ display_commit_changed(igt_display_t *display, enum igt_commit_style s)
> > 		else
> > 			/* no modeset in universal commit, no change to crtc. */
> > 			output->changed &= 1 << IGT_CONNECTOR_CRTC_ID;
> >+
> >+		if (s == COMMIT_ATOMIC) {
> >+			if (igt_output_is_prop_changed(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR))
> >+				igt_assert(output->writeback_out_fence_fd >= 0);
> >+
> >+			output->values[IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR] = 0;
> >+			igt_output_clear_prop_changed(output, IGT_CONNECTOR_WRITEBACK_FB_ID);
> 
> The WRITEBACK_FB_ID property is auto-clearing, so does that need
> special handling here? Might need to set
> output->values[IGT_CONNECTOR_WRITEBACK_FB_ID] = 0 and mark it as
> changed.

Will do.

> 
> >+		}
> > 	}
> >
> > 	if (display->first_commit) {
> >@@ -3884,6 +3914,26 @@ void igt_pipe_request_out_fence(igt_pipe_t *pipe)
> > 	igt_pipe_obj_set_prop_value(pipe, IGT_CRTC_OUT_FENCE_PTR, (ptrdiff_t)&pipe->out_fence_fd);
> > }
> >
> >+/**
> >+ * igt_output_set_writeback_fb:
> >+ * @output: Target output
> >+ * @fb: Target framebuffer
> >+ *
> >+ * This function sets the given @fb to be used as the target framebuffer for the
> >+ * writeback engine at the next atomic commit. It will also request a writeback
> >+ * out fence that will contain the fd number of the out fence created by KMS.
> >+ */
> >+void igt_output_set_writeback_fb(igt_output_t *output, struct igt_fb *fb)
> >+{
> >+	igt_display_t *display = output->display;
> >+
> >+	LOG(display, "%s: output_set_writeback_fb(%d)\n", output->name, fb ? fb->fb_id : 0);
> >+
> >+	igt_output_set_prop_value(output, IGT_CONNECTOR_WRITEBACK_FB_ID, fb ? fb->fb_id : 0);
> >+	igt_output_set_prop_value(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR,
> >+				  (ptrdiff_t)&output->writeback_out_fence_fd);
> 
> Did you mean ptrdiff_t? Might have been a typo in the
> CRTC_OUT_FENCE_PTR

No, again I was following the style of out_fence_fd variable where it
does use ptrdiff_t. Maybe that one is wrong too?

> 
> Also commented on a later patch - but requesting the out-fence could
> be dependent on fb_id != 0, then you wouldn't need to open-code that
> condition in the sequence test.

Where would the dependency be checked?


Best regards,
Liviu

> 
> Cheers,
> -Brian
> 
> >+}
> >+
> > /**
> >  * igt_wait_for_vblank_count:
> >  * @drm_fd: A drm file descriptor
> >diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> >index e6aff339a..a84f9ac42 100644
> >--- a/lib/igt_kms.h
> >+++ b/lib/igt_kms.h
> >@@ -121,6 +121,9 @@ enum igt_atomic_connector_properties {
> >        IGT_CONNECTOR_DPMS,
> >        IGT_CONNECTOR_BROADCAST_RGB,
> >        IGT_CONNECTOR_CONTENT_PROTECTION,
> >+       IGT_CONNECTOR_WRITEBACK_PIXEL_FORMATS,
> >+       IGT_CONNECTOR_WRITEBACK_FB_ID,
> >+       IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR,
> >        IGT_NUM_CONNECTOR_PROPS
> > };
> >
> >@@ -359,6 +362,8 @@ typedef struct {
> > 	bool use_override_mode;
> > 	drmModeModeInfo override_mode;
> >
> >+	int32_t writeback_out_fence_fd;
> >+
> > 	/* bitmask of changed properties */
> > 	uint64_t changed;
> >
> >@@ -404,6 +409,7 @@ igt_plane_t *igt_output_get_plane(igt_output_t *output, int plane_idx);
> > igt_plane_t *igt_output_get_plane_type(igt_output_t *output, int plane_type);
> > igt_output_t *igt_output_from_connector(igt_display_t *display,
> >     drmModeConnector *connector);
> >+void igt_output_set_writeback_fb(igt_output_t *output, struct igt_fb *fb);
> >
> > igt_plane_t *igt_pipe_get_plane_type(igt_pipe_t *pipe, int plane_type);
> > igt_output_t *igt_get_single_output_for_pipe(igt_display_t *display, enum pipe pipe);
> >-- 
> >2.19.1
> >

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t v4 4/6] kms_writeback: Add writeback-check-output

[Liviu Dudau]

On Fri, Nov 09, 2018 at 03:09:54PM +0000, Brian Starkey wrote:
> Hi,
> 
> On Mon, Nov 05, 2018 at 10:16:14AM +0000, Liviu Dudau wrote:
> >From: Brian Starkey <brian.starkey@arm.com>
> >
> >Add a test which makes commits using the writeback connector, and
> >checks the output buffer hash to make sure it is/isn't written as
> >appropriate.
> >
> >Signed-off-by: Brian Starkey <brian.starkey@arm.com>
> >[rebased and addressed comments from Maarten]
> >Signed-off-by: Liviu Dudau <liviu.dudau@arm.com>
> >
> >---
> > tests/kms_writeback.c | 127 ++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 127 insertions(+)
> >
> >diff --git a/tests/kms_writeback.c b/tests/kms_writeback.c
> >index 636777d0b..7cb65207e 100644
> >--- a/tests/kms_writeback.c
> >+++ b/tests/kms_writeback.c
> >@@ -30,6 +30,7 @@
> > #include "igt.h"
> > #include "igt_core.h"
> > #include "igt_fb.h"
> >+#include "sw_sync.h"
> >
> > static drmModePropertyBlobRes *get_writeback_formats_blob(igt_output_t *output)
> > {
> >@@ -221,6 +222,119 @@ static void writeback_fb_id(igt_output_t *output, igt_fb_t *valid_fb, igt_fb_t *
> > 	igt_assert(ret == 0);
> > }
> >
> >+static void fill_fb(igt_fb_t *fb, double color[3])
> >+{
> >+	cairo_t *cr = igt_get_cairo_ctx(fb->fd, fb);
> >+	igt_assert(cr);
> >+
> >+	igt_paint_color(cr, 0, 0, fb->width, fb->height,
> >+			color[0], color[1], color[2]);
> >+}
> >+
> >+static void get_and_wait_out_fence(igt_output_t *output)
> >+{
> >+	int ret;
> >+
> >+	igt_assert(output->writeback_out_fence_fd >= 0);
> >+
> >+	ret = sync_fence_wait(output->writeback_out_fence_fd, 1000);
> >+	igt_assert(ret == 0);
> >+	close(output->writeback_out_fence_fd);
> 
> Setting to -1 could be helpful here too. Tracking down double-closed
> fds is always a pain.

Will do.

> 
> >+}
> >+
> >+static void writeback_sequence(igt_output_t *output, igt_plane_t *plane,
> >+				igt_fb_t *in_fb, igt_fb_t *out_fbs[], int n_commits)
> >+{
> >+	int i, color_idx = 0;
> >+	double in_fb_colors[2][3] = {
> >+		{ 1.0, 0.0, 0.0 },
> >+		{ 0.0, 1.0, 0.0 },
> >+	};
> >+	double clear_color[3] = { 1.0, 1.0, 1.0 };
> >+	igt_crc_t cleared_crc, out_expected;
> >+
> >+	for (i = 0; i < n_commits; i++, color_idx++) {
> >+		/* Change the input color each time */
> >+		fill_fb(in_fb, in_fb_colors[color_idx % 2]);
> >+
> >+		if (out_fbs[i]) {
> >+			igt_crc_t out_before;
> >+
> >+			/* Get the expected CRC */
> >+			fill_fb(out_fbs[i], in_fb_colors[color_idx % 2]);
> >+			igt_fb_get_crc(out_fbs[i], &out_expected);
> >+
> >+			fill_fb(out_fbs[i], clear_color);
> >+			if (i == 0)
> >+				igt_fb_get_crc(out_fbs[i], &cleared_crc);
> >+			igt_fb_get_crc(out_fbs[i], &out_before);
> >+			igt_assert_crc_equal(&cleared_crc, &out_before);
> >+		}
> >+
> >+		/* Commit */
> >+		igt_plane_set_fb(plane, in_fb);
> >+		if (out_fbs[i])
> >+			igt_output_set_writeback_fb(output, out_fbs[i]);
> >+		else
> >+			/* avoid setting the out fence ptr that is done via igt_output_set_writeback_fb() */
> >+			igt_output_set_prop_value(output, IGT_CONNECTOR_WRITEBACK_FB_ID, 0);
> 
> Would it work to put this check in igt_output_set_writeback_fb() so
> that it only requests an out-fence if fb_id != 0 ?

It would work, but then we should drop or modify the first invalid_out_fence test
as the commit will fail, rather than set the out_fence_ptr to -1.

Best regards,
Liviu

> 
> Cheers,
> -Brian
> 
> >+
> >+		igt_display_commit_atomic(output->display,
> >+					  DRM_MODE_ATOMIC_ALLOW_MODESET,
> >+					  NULL);
> >+		if (out_fbs[i])
> >+			get_and_wait_out_fence(output);
> >+
> >+		/* Make sure the old output buffer is untouched */
> >+		if (i > 0 && out_fbs[i - 1] && (out_fbs[i] != out_fbs[i - 1])) {
> >+			igt_crc_t out_prev;
> >+			igt_fb_get_crc(out_fbs[i - 1], &out_prev);
> >+			igt_assert_crc_equal(&cleared_crc, &out_prev);
> >+		}
> >+
> >+		/* Make sure this output buffer is written */
> >+		if (out_fbs[i]) {
> >+			igt_crc_t out_after;
> >+			igt_fb_get_crc(out_fbs[i], &out_after);
> >+			igt_assert_crc_equal(&out_expected, &out_after);
> >+
> >+			/* And clear it, for the next time */
> >+			fill_fb(out_fbs[i], clear_color);
> >+		}
> >+	}
> >+}
> >+
> >+static void writeback_check_output(igt_output_t *output, igt_plane_t *plane,
> >+				   igt_fb_t *input_fb, igt_fb_t *output_fb)
> >+{
> >+	igt_fb_t *out_fbs[2] = { 0 };
> >+	igt_fb_t second_out_fb;
> >+	int ret;
> >+
> >+	/* One commit, with a writeback. */
> >+	writeback_sequence(output, plane, input_fb, &output_fb, 1);
> >+
> >+	/* Two commits, the second with no writeback */
> >+	out_fbs[0] = output_fb;
> >+	writeback_sequence(output, plane, input_fb, out_fbs, 2);
> >+
> >+	/* Two commits, both with writeback */
> >+	out_fbs[1] = output_fb;
> >+	writeback_sequence(output, plane, input_fb, out_fbs, 2);
> >+
> >+	ret = igt_create_fb(output_fb->fd, output_fb->width, output_fb->height,
> >+			    DRM_FORMAT_XRGB8888,
> >+			    igt_fb_mod_to_tiling(0),
> >+			    &second_out_fb);
> >+	igt_require(ret > 0);
> >+
> >+	/* Two commits, with different writeback buffers */
> >+	out_fbs[1] = &second_out_fb;
> >+	writeback_sequence(output, plane, input_fb, out_fbs, 2);
> >+
> >+	igt_remove_fb(output_fb->fd, &second_out_fb);
> >+}
> >+
> > igt_main
> > {
> > 	igt_display_t display;
> >@@ -307,6 +421,19 @@ igt_main
> > 		igt_remove_fb(display.drm_fd, &output_fb);
> > 	}
> >
> >+	igt_subtest("writeback-check-output") {
> >+		igt_fb_t output_fb;
> >+		ret = igt_create_fb(display.drm_fd, mode.hdisplay, mode.vdisplay,
> >+				    DRM_FORMAT_XRGB8888,
> >+				    igt_fb_mod_to_tiling(0),
> >+				    &output_fb);
> >+		igt_require(ret > 0);
> >+
> >+		writeback_check_output(output, plane, &input_fb, &output_fb);
> >+
> >+		igt_remove_fb(display.drm_fd, &output_fb);
> >+	}
> >+
> > 	igt_fixture {
> > 		igt_remove_fb(display.drm_fd, &input_fb);
> > 		igt_display_fini(&display);
> >-- 
> >2.19.1
> >

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t v4 4/6] kms_writeback: Add writeback-check-output

[Brian Starkey]

Hi Liviu,

On Mon, Dec 10, 2018 at 01:54:05PM +0000, Liviu Dudau wrote:
> On Fri, Nov 09, 2018 at 03:09:54PM +0000, Brian Starkey wrote:
> > >+static void writeback_sequence(igt_output_t *output, igt_plane_t *plane,
> > >+				igt_fb_t *in_fb, igt_fb_t *out_fbs[], int n_commits)
> > >+{
> > >+	int i, color_idx = 0;
> > >+	double in_fb_colors[2][3] = {
> > >+		{ 1.0, 0.0, 0.0 },
> > >+		{ 0.0, 1.0, 0.0 },
> > >+	};
> > >+	double clear_color[3] = { 1.0, 1.0, 1.0 };
> > >+	igt_crc_t cleared_crc, out_expected;
> > >+
> > >+	for (i = 0; i < n_commits; i++, color_idx++) {
> > >+		/* Change the input color each time */
> > >+		fill_fb(in_fb, in_fb_colors[color_idx % 2]);
> > >+
> > >+		if (out_fbs[i]) {
> > >+			igt_crc_t out_before;
> > >+
> > >+			/* Get the expected CRC */
> > >+			fill_fb(out_fbs[i], in_fb_colors[color_idx % 2]);
> > >+			igt_fb_get_crc(out_fbs[i], &out_expected);
> > >+
> > >+			fill_fb(out_fbs[i], clear_color);
> > >+			if (i == 0)
> > >+				igt_fb_get_crc(out_fbs[i], &cleared_crc);
> > >+			igt_fb_get_crc(out_fbs[i], &out_before);
> > >+			igt_assert_crc_equal(&cleared_crc, &out_before);
> > >+		}
> > >+
> > >+		/* Commit */
> > >+		igt_plane_set_fb(plane, in_fb);
> > >+		if (out_fbs[i])
> > >+			igt_output_set_writeback_fb(output, out_fbs[i]);
> > >+		else
> > >+			/* avoid setting the out fence ptr that is done via igt_output_set_writeback_fb() */
> > >+			igt_output_set_prop_value(output, IGT_CONNECTOR_WRITEBACK_FB_ID, 0);
> > 
> > Would it work to put this check in igt_output_set_writeback_fb() so
> > that it only requests an out-fence if fb_id != 0 ?
> 
> It would work, but then we should drop or modify the first invalid_out_fence test
> as the commit will fail, rather than set the out_fence_ptr to -1.

It looks like invalid_out_fence is explicitly calling
igt_output_set_prop_value() rather than the
igt_output_set_writeback_fb() helper, so we should be able to test
whatever we want there.

My theory is that given this fb_id != 0 check is the "correct" thing
in the normal case, we could put it in the helper. Any tests which
need to explicitly do the incorrect thing can open-code whatever
nastiness they're trying to exercise.

Cheers,
-Brian
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev