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

[PATCH i-g-t v5 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 v4 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:
 - v5: Addressed comments from Brian Starkey. Old v4 changes are here:
   https://lists.freedesktop.org/archives/igt-dev/2018-November/006806.html
 
 - 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
   
 - v3: 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

 lib/igt_fb.c           |  66 ++++++
 lib/igt_fb.h           |   3 +
 lib/igt_kms.c          | 157 +++++++++----
 lib/igt_kms.h          |  11 +
 tests/Makefile.sources |   1 +
 tests/kms_writeback.c  | 492 +++++++++++++++++++++++++++++++++++++++++
 tests/meson.build      |   1 +
 7 files changed, 693 insertions(+), 38 deletions(-)
 create mode 100644 tests/kms_writeback.c

-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH i-g-t v5 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>
---
 lib/igt_kms.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/igt_kms.h |  6 ++++++
 2 files changed, 63 insertions(+)

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index 684a599ca..0bc2996cd 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:
@@ -1899,6 +1911,8 @@ void igt_display_require(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,11 @@ 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);
+		output->writeback_out_fence_fd = -1;
+	}
 }
 
 /**
@@ -3144,6 +3163,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 +3288,16 @@ 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;
+			output->values[IGT_CONNECTOR_WRITEBACK_FB_ID] = 0;
+			igt_output_clear_prop_changed(output, IGT_CONNECTOR_WRITEBACK_FB_ID);
+			igt_output_clear_prop_changed(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR);
+		}
 	}
 
 	if (display->first_commit) {
@@ -3884,6 +3918,29 @@ 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 if
+ * the given @fb is valid.
+ */
+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);
+	/* only request a writeback out fence if the framebuffer is valid */
+	if (fb)
+		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 4a7c3c979..13d3a9ceb 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;
 
@@ -403,6 +408,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.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH i-g-t v5 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 519eac792..4438d2165 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -90,6 +90,7 @@ TESTS_progs = \
 	kms_universal_plane \
 	kms_vblank \
 	kms_sequence \
+	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..66ef48a6c
--- /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_require(&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 e14ab2b45..41e78b8ee 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -60,6 +60,7 @@ test_progs = [
 	'kms_tv_load_detect',
 	'kms_universal_plane',
 	'kms_vblank',
+	'kms_writeback',
 	'meta_test',
 	'perf',
 	'pm_backlight',
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH i-g-t v5 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 5cd1829a3..11e200b53 100644
--- a/lib/igt_fb.c
+++ b/lib/igt_fb.c
@@ -2379,6 +2379,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.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH i-g-t v5 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>
---
 tests/kms_writeback.c | 124 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 124 insertions(+)

diff --git a/tests/kms_writeback.c b/tests/kms_writeback.c
index 66ef48a6c..0f20dadd2 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,116 @@ 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);
+	output->writeback_out_fence_fd = -1;
+}
+
+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);
+		igt_output_set_writeback_fb(output, out_fbs[i]);
+
+		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 +418,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.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH i-g-t v5 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 | 100 +++++++++++++++++++++++++++++++-------------------
 lib/igt_kms.h |   5 +++
 2 files changed, 67 insertions(+), 38 deletions(-)

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index 0bc2996cd..fe51a6c69 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;
@@ -2190,42 +2201,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;
@@ -2249,6 +2224,40 @@ 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;
+	unsigned long pending_crtc_idx_mask;
+
+	/* Check that outputs and pipes agree wrt. cloning */
+	for (i = 0; i < display->n_outputs; i++) {
+		output = &display->outputs[i];
+		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,
@@ -3587,6 +3596,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
@@ -3594,6 +3604,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
@@ -3610,11 +3630,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 13d3a9ceb..acb27982b 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 {
@@ -404,6 +407,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.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH i-g-t v5 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 0f20dadd2..ae536bbfa 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;
 			}
 		}
@@ -335,10 +367,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));
@@ -353,7 +386,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)
@@ -431,6 +464,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.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t v5 3/6] lib: Add function to hash a framebuffer

[Chris Wilson]

Quoting Liviu Dudau (2019-01-15 17:47:44)
> +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);

igt_memcpy_from_wc() for the reasons cited above.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t v5 3/6] lib: Add function to hash a framebuffer

[Liviu Dudau]

On Tue, Jan 15, 2019 at 06:47:47PM +0000, Chris Wilson wrote:
> Quoting Liviu Dudau (2019-01-15 17:47:44)
> > +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);
> 
> igt_memcpy_from_wc() for the reasons cited above.
> -Chris

Hi Chris,

Thanks for pointing out the function, I was not aware of it!

Now, looking at the implementation, and ignoring the fact that it is in a
file called igt_x86.c, it looks to me that it will end up calling memcpy
anyway for Arm drivers. Not being a GCC expert, I am wondering if the
ifunc() wrapper around resolve_memcpy_from_wc() will not actually
prevent the compiler from choosing an optimised version of memcpy for Arm.

I can refresh the patch if you think it is safe to use igt_memcpy_from_wc() for
non-x86 architectures.

Best regards,
Liviu

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

Re: [PATCH i-g-t v5 3/6] lib: Add function to hash a framebuffer

[Chris Wilson]

Quoting Liviu Dudau (2019-01-16 11:20:09)
> On Tue, Jan 15, 2019 at 06:47:47PM +0000, Chris Wilson wrote:
> > Quoting Liviu Dudau (2019-01-15 17:47:44)
> > > +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);
> > 
> > igt_memcpy_from_wc() for the reasons cited above.
> > -Chris
> 
> Hi Chris,
> 
> Thanks for pointing out the function, I was not aware of it!
> 
> Now, looking at the implementation, and ignoring the fact that it is in a
> file called igt_x86.c, it looks to me that it will end up calling memcpy
> anyway for Arm drivers. Not being a GCC expert, I am wondering if the
> ifunc() wrapper around resolve_memcpy_from_wc() will not actually
> prevent the compiler from choosing an optimised version of memcpy for Arm.
> 
> I can refresh the patch if you think it is safe to use igt_memcpy_from_wc() for
> non-x86 architectures.

You are memcpy'ing from uncached, there is no general purpose optimised
memcpy! :) The compiler builtin (e.g. rep movb for x86) may be the worst
thing you could do ;) For Arm, it will fallback to the general memcpy
routine which will do it own ifunc, just will not be inlined.

It should be safe, and it should be no worse than a call to memcpy with
variable arguments.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH i-g-t v6] 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).

v6: use igt_memcpy_from_wc() instead of plain memcpy, as suggested by
    Chris Wilson

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 5cd1829a3..044fd57de 100644
--- a/lib/igt_fb.c
+++ b/lib/igt_fb.c
@@ -2379,6 +2379,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) {
+
+		igt_memcpy_from_wc(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.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Brian Starkey]

Hi,

On Tue, Jan 15, 2019 at 05:47:46PM +0000, Liviu Dudau wrote:
> From: Brian Starkey <brian.starkey@arm.com>
> diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> index 13d3a9ceb..acb27982b 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;

I think it looks like this ended up being unused.

Thanks,
-Brian

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Brian Starkey]

Hi Liviu,

On Tue, Jan 15, 2019 at 05:47:42PM +0000, Liviu Dudau wrote:
> From: Brian Starkey <brian.starkey@arm.com>

[snip]

>  
> +/**
> + * 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 if
> + * the given @fb is valid.
> + */
> +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);
> +	/* only request a writeback out fence if the framebuffer is valid */
> +	if (fb)
> +		igt_output_set_prop_value(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR,
> +					  (ptrdiff_t)&output->writeback_out_fence_fd);

I'm still not sure (ptrdiff_t) is the right type here, (uintptr_t)
seems better.

Thanks,
-Brian
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Brian Starkey]

Hi Liviu,

On Tue, Jan 15, 2019 at 05:47:41PM +0000, Liviu Dudau wrote:
> 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 v4 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:
>  - v5: Addressed comments from Brian Starkey. Old v4 changes are here:
>    https://lists.freedesktop.org/archives/igt-dev/2018-November/006806.html

I just replied with two super minor things, but otherwise this looks
fine to me.

As we were just discussing, strangely it seems like we both never
received Maarten's v4 emails. His proposed changes sounded fine to me,
though, if you decide to roll them in.

Thanks,
-Brian

>  
>  - 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
>    
>  - v3: 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
> 
>  lib/igt_fb.c           |  66 ++++++
>  lib/igt_fb.h           |   3 +
>  lib/igt_kms.c          | 157 +++++++++----
>  lib/igt_kms.h          |  11 +
>  tests/Makefile.sources |   1 +
>  tests/kms_writeback.c  | 492 +++++++++++++++++++++++++++++++++++++++++
>  tests/meson.build      |   1 +
>  7 files changed, 693 insertions(+), 38 deletions(-)
>  create mode 100644 tests/kms_writeback.c
> 
> -- 
> 2.20.1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Rodrigo Siqueira]

[-- Attachment #1.1: Type: text/plain, Size: 3483 bytes --]

Hi Liviu,

I’m using your patchset to guide my implementation of writeback in the
VKMS, so, first of all, thanks :)

During my work, I noticed that you’re setting the drmSetClientCap()
before drmModeGetResources() which make the writeback capability
‘invisible’ for drmModeGetResources(). I made the following change, and
I could pass the igt_display_require():

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index f4ff3a96..bb2d8e06 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -1912,6 +1912,12 @@ void igt_display_require(igt_display_t *display, int drm_fd)
 
        display->drm_fd = drm_fd;
 
+       drmSetClientCap(drm_fd, DRM_CLIENT_CAP_UNIVERSAL_PLANES, 1);
+       if (drmSetClientCap(drm_fd, DRM_CLIENT_CAP_ATOMIC, 1) == 0)
+               display->is_atomic = 1;
+
+       drmSetClientCap(drm_fd, DRM_CLIENT_CAP_WRITEBACK_CONNECTORS, 1);
+
        resources = drmModeGetResources(display->drm_fd);
        if (!resources)
                goto out;
@@ -1924,12 +1930,6 @@ void igt_display_require(igt_display_t *display, int drm_fd)
        display->pipes = calloc(sizeof(igt_pipe_t), display->n_pipes);
        igt_assert_f(display->pipes, "Failed to allocate memory for %d pipes\n", display->n_pipes);
 
-       drmSetClientCap(drm_fd, DRM_CLIENT_CAP_UNIVERSAL_PLANES, 1);
-       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);

I'm not 100% confident about this issue, because of this I will send an
RFC and see if I can get more details about this issue.

Additionally, if you need any help with this patchset I will be glad to
help since I’m using it \o/

Best Regards
Rodrigo Siqueira

On 02/04, Brian Starkey wrote:
> Hi Liviu,
> 
> On Tue, Jan 15, 2019 at 05:47:42PM +0000, Liviu Dudau wrote:
> > From: Brian Starkey <brian.starkey@arm.com>
> 
> [snip]
> 
> >  
> > +/**
> > + * 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 if
> > + * the given @fb is valid.
> > + */
> > +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);
> > +	/* only request a writeback out fence if the framebuffer is valid */
> > +	if (fb)
> > +		igt_output_set_prop_value(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR,
> > +					  (ptrdiff_t)&output->writeback_out_fence_fd);
> 
> I'm still not sure (ptrdiff_t) is the right type here, (uintptr_t)
> seems better.
> 
> Thanks,
> -Brian
> _______________________________________________
> igt-dev mailing list
> igt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev

-- 
Rodrigo Siqueira
https://siqueira.tech
Graduate Student
Department of Computer Science
University of São Paulo

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Liviu Dudau]

On Wed, Mar 06, 2019 at 06:30:05PM -0300, Rodrigo Siqueira wrote:
> Hi Liviu,
> 
> I’m using your patchset to guide my implementation of writeback in the
> VKMS, so, first of all, thanks :)
> 
> During my work, I noticed that you’re setting the drmSetClientCap()
> before drmModeGetResources() which make the writeback capability

before? I believe your patchset moves it to be before, which makes
sense, as the writeback applies to connectors, not planes (sorry for the
confusion, I think I wrote the code when we were toying with the idea
that writeback was going to be a plane property).

> ‘invisible’ for drmModeGetResources(). I made the following change, and
> I could pass the igt_display_require():
> 
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index f4ff3a96..bb2d8e06 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -1912,6 +1912,12 @@ void igt_display_require(igt_display_t *display, int drm_fd)
>  
>         display->drm_fd = drm_fd;
>  
> +       drmSetClientCap(drm_fd, DRM_CLIENT_CAP_UNIVERSAL_PLANES, 1);
> +       if (drmSetClientCap(drm_fd, DRM_CLIENT_CAP_ATOMIC, 1) == 0)
> +               display->is_atomic = 1;
> +
> +       drmSetClientCap(drm_fd, DRM_CLIENT_CAP_WRITEBACK_CONNECTORS, 1);
> +
>         resources = drmModeGetResources(display->drm_fd);
>         if (!resources)
>                 goto out;
> @@ -1924,12 +1930,6 @@ void igt_display_require(igt_display_t *display, int drm_fd)
>         display->pipes = calloc(sizeof(igt_pipe_t), display->n_pipes);
>         igt_assert_f(display->pipes, "Failed to allocate memory for %d pipes\n", display->n_pipes);
>  
> -       drmSetClientCap(drm_fd, DRM_CLIENT_CAP_UNIVERSAL_PLANES, 1);
> -       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);
> 
> I'm not 100% confident about this issue, because of this I will send an
> RFC and see if I can get more details about this issue.

It actually looks correct to me, so 

Acked-by: Liviu Dudau <liviu.dudau@arm.com>

> 
> Additionally, if you need any help with this patchset I will be glad to
> help since I’m using it \o/

Well, I need to send an updated version and if you could review that
then I can merge it into igt.

Best regards,
Liviu

> 
> Best Regards
> Rodrigo Siqueira
> 
> On 02/04, Brian Starkey wrote:
> > Hi Liviu,
> > 
> > On Tue, Jan 15, 2019 at 05:47:42PM +0000, Liviu Dudau wrote:
> > > From: Brian Starkey <brian.starkey@arm.com>
> > 
> > [snip]
> > 
> > >  
> > > +/**
> > > + * 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 if
> > > + * the given @fb is valid.
> > > + */
> > > +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);
> > > +	/* only request a writeback out fence if the framebuffer is valid */
> > > +	if (fb)
> > > +		igt_output_set_prop_value(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR,
> > > +					  (ptrdiff_t)&output->writeback_out_fence_fd);
> > 
> > I'm still not sure (ptrdiff_t) is the right type here, (uintptr_t)
> > seems better.
> > 
> > Thanks,
> > -Brian
> > _______________________________________________
> > igt-dev mailing list
> > igt-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/igt-dev
> 
> -- 
> Rodrigo Siqueira
> https://siqueira.tech
> Graduate Student
> Department of Computer Science
> University of São Paulo



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

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

[Rodrigo Siqueira]

[-- Attachment #1.1: Type: text/plain, Size: 5204 bytes --]

On 03/18, Liviu Dudau wrote:
> On Wed, Mar 06, 2019 at 06:30:05PM -0300, Rodrigo Siqueira wrote:
> > Hi Liviu,
> > 
> > I’m using your patchset to guide my implementation of writeback in the
> > VKMS, so, first of all, thanks :)
> > 
> > During my work, I noticed that you’re setting the drmSetClientCap()
> > before drmModeGetResources() which make the writeback capability
> 
> before? I believe your patchset moves it to be before, which makes
> sense, as the writeback applies to connectors, not planes (sorry for the
> confusion, I think I wrote the code when we were toying with the idea
> that writeback was going to be a plane property).
> 
> > ‘invisible’ for drmModeGetResources(). I made the following change, and
> > I could pass the igt_display_require():
> > 
> > diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> > index f4ff3a96..bb2d8e06 100644
> > --- a/lib/igt_kms.c
> > +++ b/lib/igt_kms.c
> > @@ -1912,6 +1912,12 @@ void igt_display_require(igt_display_t *display, int drm_fd)
> >  
> >         display->drm_fd = drm_fd;
> >  
> > +       drmSetClientCap(drm_fd, DRM_CLIENT_CAP_UNIVERSAL_PLANES, 1);
> > +       if (drmSetClientCap(drm_fd, DRM_CLIENT_CAP_ATOMIC, 1) == 0)
> > +               display->is_atomic = 1;
> > +
> > +       drmSetClientCap(drm_fd, DRM_CLIENT_CAP_WRITEBACK_CONNECTORS, 1);
> > +
> >         resources = drmModeGetResources(display->drm_fd);
> >         if (!resources)
> >                 goto out;
> > @@ -1924,12 +1930,6 @@ void igt_display_require(igt_display_t *display, int drm_fd)
> >         display->pipes = calloc(sizeof(igt_pipe_t), display->n_pipes);
> >         igt_assert_f(display->pipes, "Failed to allocate memory for %d pipes\n", display->n_pipes);
> >  
> > -       drmSetClientCap(drm_fd, DRM_CLIENT_CAP_UNIVERSAL_PLANES, 1);
> > -       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);
> > 
> > I'm not 100% confident about this issue, because of this I will send an
> > RFC and see if I can get more details about this issue.
> 
> It actually looks correct to me, so 
> 
> Acked-by: Liviu Dudau <liviu.dudau@arm.com>
> 
> > 
> > Additionally, if you need any help with this patchset I will be glad to
> > help since I’m using it \o/
> 
> Well, I need to send an updated version and if you could review that
> then I can merge it into igt.

Sure! I will be glad to take some time to review your code :)

I can also try to test it on my current implementation of VKMS.
One question, I have a NanoPc-T1 device [1] which has a Mali GPU, can I
test the writeback feature in this device? All kind of Mali GPU has
support for the writeback feature?

1. http://wiki.friendlyarm.com/wiki/index.php/NanoPC-T1

Thanks!
Best Regards
 
> Best regards,
> Liviu
> 
> > 
> > Best Regards
> > Rodrigo Siqueira
> > 
> > On 02/04, Brian Starkey wrote:
> > > Hi Liviu,
> > > 
> > > On Tue, Jan 15, 2019 at 05:47:42PM +0000, Liviu Dudau wrote:
> > > > From: Brian Starkey <brian.starkey@arm.com>
> > > 
> > > [snip]
> > > 
> > > >  
> > > > +/**
> > > > + * 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 if
> > > > + * the given @fb is valid.
> > > > + */
> > > > +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);
> > > > +	/* only request a writeback out fence if the framebuffer is valid */
> > > > +	if (fb)
> > > > +		igt_output_set_prop_value(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR,
> > > > +					  (ptrdiff_t)&output->writeback_out_fence_fd);
> > > 
> > > I'm still not sure (ptrdiff_t) is the right type here, (uintptr_t)
> > > seems better.
> > > 
> > > Thanks,
> > > -Brian
> > > _______________________________________________
> > > igt-dev mailing list
> > > igt-dev@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/igt-dev
> > 
> > -- 
> > Rodrigo Siqueira
> > https://siqueira.tech
> > Graduate Student
> > Department of Computer Science
> > University of São Paulo
> 
> 
> 
> -- 
> ====================
> | I would like to |
> | fix the world,  |
> | but they're not |
> | giving me the   |
>  \ source code!  /
>   ---------------
>     ¯\_(ツ)_/¯

-- 
Rodrigo Siqueira
https://siqueira.tech
Graduate Student
Department of Computer Science
University of São Paulo

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Liviu Dudau]

On Mon, Mar 18, 2019 at 07:05:29PM -0300, Rodrigo Siqueira wrote:
> On 03/18, Liviu Dudau wrote:
> > On Wed, Mar 06, 2019 at 06:30:05PM -0300, Rodrigo Siqueira wrote:
> > > Hi Liviu,
> > > 
> > > I’m using your patchset to guide my implementation of writeback in the
> > > VKMS, so, first of all, thanks :)
> > > 
> > > During my work, I noticed that you’re setting the drmSetClientCap()
> > > before drmModeGetResources() which make the writeback capability
> > 
> > before? I believe your patchset moves it to be before, which makes
> > sense, as the writeback applies to connectors, not planes (sorry for the
> > confusion, I think I wrote the code when we were toying with the idea
> > that writeback was going to be a plane property).
> > 
> > > ‘invisible’ for drmModeGetResources(). I made the following change, and
> > > I could pass the igt_display_require():
> > > 
> > > diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> > > index f4ff3a96..bb2d8e06 100644
> > > --- a/lib/igt_kms.c
> > > +++ b/lib/igt_kms.c
> > > @@ -1912,6 +1912,12 @@ void igt_display_require(igt_display_t *display, int drm_fd)
> > >  
> > >         display->drm_fd = drm_fd;
> > >  
> > > +       drmSetClientCap(drm_fd, DRM_CLIENT_CAP_UNIVERSAL_PLANES, 1);
> > > +       if (drmSetClientCap(drm_fd, DRM_CLIENT_CAP_ATOMIC, 1) == 0)
> > > +               display->is_atomic = 1;
> > > +
> > > +       drmSetClientCap(drm_fd, DRM_CLIENT_CAP_WRITEBACK_CONNECTORS, 1);
> > > +
> > >         resources = drmModeGetResources(display->drm_fd);
> > >         if (!resources)
> > >                 goto out;
> > > @@ -1924,12 +1930,6 @@ void igt_display_require(igt_display_t *display, int drm_fd)
> > >         display->pipes = calloc(sizeof(igt_pipe_t), display->n_pipes);
> > >         igt_assert_f(display->pipes, "Failed to allocate memory for %d pipes\n", display->n_pipes);
> > >  
> > > -       drmSetClientCap(drm_fd, DRM_CLIENT_CAP_UNIVERSAL_PLANES, 1);
> > > -       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);
> > > 
> > > I'm not 100% confident about this issue, because of this I will send an
> > > RFC and see if I can get more details about this issue.
> > 
> > It actually looks correct to me, so 
> > 
> > Acked-by: Liviu Dudau <liviu.dudau@arm.com>
> > 
> > > 
> > > Additionally, if you need any help with this patchset I will be glad to
> > > help since I’m using it \o/
> > 
> > Well, I need to send an updated version and if you could review that
> > then I can merge it into igt.
> 
> Sure! I will be glad to take some time to review your code :)
> 
> I can also try to test it on my current implementation of VKMS.
> One question, I have a NanoPc-T1 device [1] which has a Mali GPU, can I
> test the writeback feature in this device? All kind of Mali GPU has
> support for the writeback feature?
> 
> 1. http://wiki.friendlyarm.com/wiki/index.php/NanoPC-T1

(sorry, email fell through the cracks)

The writeback is a Mali Display Processor feature, not GPU. As such you
need a device that has Mali DP in it. Commercially there will be a
platform later this year, but until it is announced I cannot say more
about it.

Best regards,
Liviu

> 
> Thanks!
> Best Regards
>  
> > Best regards,
> > Liviu
> > 
> > > 
> > > Best Regards
> > > Rodrigo Siqueira
> > > 
> > > On 02/04, Brian Starkey wrote:
> > > > Hi Liviu,
> > > > 
> > > > On Tue, Jan 15, 2019 at 05:47:42PM +0000, Liviu Dudau wrote:
> > > > > From: Brian Starkey <brian.starkey@arm.com>
> > > > 
> > > > [snip]
> > > > 
> > > > >  
> > > > > +/**
> > > > > + * 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 if
> > > > > + * the given @fb is valid.
> > > > > + */
> > > > > +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);
> > > > > +	/* only request a writeback out fence if the framebuffer is valid */
> > > > > +	if (fb)
> > > > > +		igt_output_set_prop_value(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR,
> > > > > +					  (ptrdiff_t)&output->writeback_out_fence_fd);
> > > > 
> > > > I'm still not sure (ptrdiff_t) is the right type here, (uintptr_t)
> > > > seems better.
> > > > 
> > > > Thanks,
> > > > -Brian
> > > > _______________________________________________
> > > > igt-dev mailing list
> > > > igt-dev@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/igt-dev
> > > 
> > > -- 
> > > Rodrigo Siqueira
> > > https://siqueira.tech
> > > Graduate Student
> > > Department of Computer Science
> > > University of São Paulo
> > 
> > 
> > 
> > -- 
> > ====================
> > | I would like to |
> > | fix the world,  |
> > | but they're not |
> > | giving me the   |
> >  \ source code!  /
> >   ---------------
> >     ¯\_(ツ)_/¯
> 
> -- 
> Rodrigo Siqueira
> https://siqueira.tech
> Graduate Student
> Department of Computer Science
> University of São Paulo



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