All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH i-g-t 0/6] igt: Add support for testing writeback connectors
@ 2018-03-01 17:38 ` Liviu Dudau
  0 siblings, 0 replies; 27+ messages in thread
From: Liviu Dudau @ 2018-03-01 17:38 UTC (permalink / raw)
  To: i-g-t; +Cc: Intel GFX ML, Rob Clark

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.

Changelog:
 - 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 in lib/
  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           |  65 ++++++
 lib/igt_fb.h           |   5 +
 lib/igt_kms.c          | 178 +++++++++++++----
 lib/igt_kms.h          |  27 +++
 tests/Makefile.sources |   1 +
 tests/kms_writeback.c  | 531 +++++++++++++++++++++++++++++++++++++++++++++++++
 tests/meson.build      |   1 +
 7 files changed, 765 insertions(+), 43 deletions(-)
 create mode 100644 tests/kms_writeback.c

-- 
2.16.1

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Intel-gfx] [PATCH i-g-t 0/6] igt: Add support for testing writeback connectors
@ 2018-03-01 17:38 ` Liviu Dudau
  0 siblings, 0 replies; 27+ messages in thread
From: Liviu Dudau @ 2018-03-01 17:38 UTC (permalink / raw)
  To: i-g-t; +Cc: Intel GFX ML, Rob Clark

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.

Changelog:
 - 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 in lib/
  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           |  65 ++++++
 lib/igt_fb.h           |   5 +
 lib/igt_kms.c          | 178 +++++++++++++----
 lib/igt_kms.h          |  27 +++
 tests/Makefile.sources |   1 +
 tests/kms_writeback.c  | 531 +++++++++++++++++++++++++++++++++++++++++++++++++
 tests/meson.build      |   1 +
 7 files changed, 765 insertions(+), 43 deletions(-)
 create mode 100644 tests/kms_writeback.c

-- 
2.16.1

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH i-g-t 1/6] lib/igt_kms: Add writeback support in lib/
  2018-03-01 17:38 ` [Intel-gfx] " Liviu Dudau
@ 2018-03-01 17:38   ` Liviu Dudau
  -1 siblings, 0 replies; 27+ messages in thread
From: Liviu Dudau @ 2018-03-01 17:38 UTC (permalink / raw)
  To: i-g-t; +Cc: Intel GFX ML, Rob Clark

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

Add support in igt_kms for Writeback connectors, with the ability to
attach framebuffers and retrieve fences.

Signed-off-by: Brian Starkey <brian.starkey@arm.com>
---
 lib/igt_kms.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 lib/igt_kms.h | 14 ++++++++++++
 2 files changed, 85 insertions(+), 1 deletion(-)

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index 022abfe7..00d9d2e2 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -190,7 +190,10 @@ const char *igt_connector_prop_names[IGT_NUM_CONNECTOR_PROPS] = {
 	"scaling mode",
 	"CRTC_ID",
 	"DPMS",
-	"Broadcast RGB"
+	"Broadcast RGB",
+	"WRITEBACK_PIXEL_FORMATS",
+	"WRITEBACK_FB_ID",
+	"WRITEBACK_OUT_FENCE_PTR"
 };
 
 /*
@@ -452,6 +455,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" },
 	{}
 };
 
@@ -1947,6 +1951,7 @@ void igt_display_init(igt_display_t *display, int drm_fd)
 		output->force_reprobe = true;
 		output->id = resources->connectors[i];
 		output->display = display;
+		output->writeback_out_fence_fd = -1;
 
 		igt_output_refresh(output);
 	}
@@ -2040,6 +2045,43 @@ igt_output_t *igt_output_from_connector(igt_display_t *display,
 	return found;
 }
 
+void igt_output_set_writeback_fb(igt_output_t *output, struct igt_fb *fb)
+{
+	igt_display_t *display = output->display;
+	struct kmstest_connector_config *config = &output->config;
+
+	if (config->connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK)
+		return;
+
+	LOG(display, "%s: output_set_writeback_fb(%d)\n", output->name,
+	    fb ? fb->fb_id : 0);
+
+	output->writeback_fb = fb;
+}
+
+static void igt_output_reset_writeback_out_fence(igt_output_t *output)
+{
+	if (output->writeback_out_fence_fd >= 0) {
+		close(output->writeback_out_fence_fd);
+		output->writeback_out_fence_fd = -1;
+	}
+}
+
+void igt_output_request_writeback_out_fence(igt_output_t *output)
+{
+	igt_output_reset_writeback_out_fence(output);
+	igt_output_set_prop_value(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR,
+				  (ptrdiff_t)&output->writeback_out_fence_fd);
+}
+
+int igt_output_get_last_writeback_out_fence(igt_output_t *output)
+{
+	int fd = output->writeback_out_fence_fd;
+	output->writeback_out_fence_fd = -1;
+
+	return fd;
+}
+
 static void igt_pipe_fini(igt_pipe_t *pipe)
 {
 	int i;
@@ -2063,6 +2105,8 @@ static void igt_pipe_fini(igt_pipe_t *pipe)
 static void igt_output_fini(igt_output_t *output)
 {
 	kmstest_free_connector_config(&output->config);
+	if (output->writeback_out_fence_fd >= 0)
+		close(output->writeback_out_fence_fd);
 	free(output->name);
 	output->name = NULL;
 }
@@ -2879,6 +2923,31 @@ static void igt_atomic_prepare_connector_commit(igt_output_t *output, drmModeAto
 					  output->props[i],
 					  output->values[i]));
 	}
+
+	if (output->writeback_fb)
+		output->writeback_fb = NULL;
+
+	igt_output_reset_writeback_out_fence(output);
+}
+
+static void handle_writeback_out_fences(igt_display_t *display, uint32_t flags, int ret)
+{
+	int i;
+
+	for (i = 0; i < display->n_outputs; i++) {
+		igt_output_t *output = &display->outputs[i];
+
+		if (!output->config.connector)
+			continue;
+
+		if (!igt_output_is_prop_changed(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR))
+			continue;
+
+		igt_output_clear_prop_changed(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR);
+
+		if (ret || (flags & DRM_MODE_ATOMIC_TEST_ONLY))
+			igt_assert(output->writeback_out_fence_fd == -1);
+	}
 }
 
 /*
@@ -2929,6 +2998,7 @@ static int igt_atomic_commit(igt_display_t *display, uint32_t flags, void *user_
 	}
 
 	ret = drmModeAtomicCommit(display->drm_fd, req, flags, user_data);
+	handle_writeback_out_fences(display, flags, ret);
 
 	drmModeAtomicFree(req);
 	return ret;
diff --git a/lib/igt_kms.h b/lib/igt_kms.h
index 1c46186e..59ccc4c6 100644
--- a/lib/igt_kms.h
+++ b/lib/igt_kms.h
@@ -38,6 +38,10 @@
 #include "igt_fb.h"
 #include "ioctl_wrappers.h"
 
+#ifndef DRM_MODE_CONNECTOR_WRITEBACK
+#define DRM_MODE_CONNECTOR_WRITEBACK   18
+#endif
+
 /* Low-level helpers with kmstest_ prefix */
 
 /**
@@ -120,6 +124,9 @@ enum igt_atomic_connector_properties {
        IGT_CONNECTOR_CRTC_ID,
        IGT_CONNECTOR_DPMS,
        IGT_CONNECTOR_BROADCAST_RGB,
+       IGT_CONNECTOR_WRITEBACK_PIXEL_FORMATS,
+       IGT_CONNECTOR_WRITEBACK_FB_ID,
+       IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR,
        IGT_NUM_CONNECTOR_PROPS
 };
 
@@ -345,6 +352,9 @@ typedef struct {
 
 	uint32_t props[IGT_NUM_CONNECTOR_PROPS];
 	uint64_t values[IGT_NUM_CONNECTOR_PROPS];
+
+	struct igt_fb *writeback_fb;
+	int32_t writeback_out_fence_fd;
 } igt_output_t;
 
 struct igt_display {
@@ -380,6 +390,10 @@ 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);
+void igt_output_request_writeback_out_fence(igt_output_t *output);
+int igt_output_get_last_writeback_out_fence(igt_output_t *output);
+
 igt_plane_t *igt_pipe_get_plane_type(igt_pipe_t *pipe, int plane_type);
 
 void igt_pipe_request_out_fence(igt_pipe_t *pipe);
-- 
2.16.1

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

^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [igt-dev] [PATCH i-g-t 1/6] lib/igt_kms: Add writeback support in lib/
@ 2018-03-01 17:38   ` Liviu Dudau
  0 siblings, 0 replies; 27+ messages in thread
From: Liviu Dudau @ 2018-03-01 17:38 UTC (permalink / raw)
  To: i-g-t; +Cc: Intel GFX ML, Rob Clark, Brian Starkey

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

Add support in igt_kms for Writeback connectors, with the ability to
attach framebuffers and retrieve fences.

Signed-off-by: Brian Starkey <brian.starkey@arm.com>
---
 lib/igt_kms.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 lib/igt_kms.h | 14 ++++++++++++
 2 files changed, 85 insertions(+), 1 deletion(-)

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index 022abfe7..00d9d2e2 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -190,7 +190,10 @@ const char *igt_connector_prop_names[IGT_NUM_CONNECTOR_PROPS] = {
 	"scaling mode",
 	"CRTC_ID",
 	"DPMS",
-	"Broadcast RGB"
+	"Broadcast RGB",
+	"WRITEBACK_PIXEL_FORMATS",
+	"WRITEBACK_FB_ID",
+	"WRITEBACK_OUT_FENCE_PTR"
 };
 
 /*
@@ -452,6 +455,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" },
 	{}
 };
 
@@ -1947,6 +1951,7 @@ void igt_display_init(igt_display_t *display, int drm_fd)
 		output->force_reprobe = true;
 		output->id = resources->connectors[i];
 		output->display = display;
+		output->writeback_out_fence_fd = -1;
 
 		igt_output_refresh(output);
 	}
@@ -2040,6 +2045,43 @@ igt_output_t *igt_output_from_connector(igt_display_t *display,
 	return found;
 }
 
+void igt_output_set_writeback_fb(igt_output_t *output, struct igt_fb *fb)
+{
+	igt_display_t *display = output->display;
+	struct kmstest_connector_config *config = &output->config;
+
+	if (config->connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK)
+		return;
+
+	LOG(display, "%s: output_set_writeback_fb(%d)\n", output->name,
+	    fb ? fb->fb_id : 0);
+
+	output->writeback_fb = fb;
+}
+
+static void igt_output_reset_writeback_out_fence(igt_output_t *output)
+{
+	if (output->writeback_out_fence_fd >= 0) {
+		close(output->writeback_out_fence_fd);
+		output->writeback_out_fence_fd = -1;
+	}
+}
+
+void igt_output_request_writeback_out_fence(igt_output_t *output)
+{
+	igt_output_reset_writeback_out_fence(output);
+	igt_output_set_prop_value(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR,
+				  (ptrdiff_t)&output->writeback_out_fence_fd);
+}
+
+int igt_output_get_last_writeback_out_fence(igt_output_t *output)
+{
+	int fd = output->writeback_out_fence_fd;
+	output->writeback_out_fence_fd = -1;
+
+	return fd;
+}
+
 static void igt_pipe_fini(igt_pipe_t *pipe)
 {
 	int i;
@@ -2063,6 +2105,8 @@ static void igt_pipe_fini(igt_pipe_t *pipe)
 static void igt_output_fini(igt_output_t *output)
 {
 	kmstest_free_connector_config(&output->config);
+	if (output->writeback_out_fence_fd >= 0)
+		close(output->writeback_out_fence_fd);
 	free(output->name);
 	output->name = NULL;
 }
@@ -2879,6 +2923,31 @@ static void igt_atomic_prepare_connector_commit(igt_output_t *output, drmModeAto
 					  output->props[i],
 					  output->values[i]));
 	}
+
+	if (output->writeback_fb)
+		output->writeback_fb = NULL;
+
+	igt_output_reset_writeback_out_fence(output);
+}
+
+static void handle_writeback_out_fences(igt_display_t *display, uint32_t flags, int ret)
+{
+	int i;
+
+	for (i = 0; i < display->n_outputs; i++) {
+		igt_output_t *output = &display->outputs[i];
+
+		if (!output->config.connector)
+			continue;
+
+		if (!igt_output_is_prop_changed(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR))
+			continue;
+
+		igt_output_clear_prop_changed(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR);
+
+		if (ret || (flags & DRM_MODE_ATOMIC_TEST_ONLY))
+			igt_assert(output->writeback_out_fence_fd == -1);
+	}
 }
 
 /*
@@ -2929,6 +2998,7 @@ static int igt_atomic_commit(igt_display_t *display, uint32_t flags, void *user_
 	}
 
 	ret = drmModeAtomicCommit(display->drm_fd, req, flags, user_data);
+	handle_writeback_out_fences(display, flags, ret);
 
 	drmModeAtomicFree(req);
 	return ret;
diff --git a/lib/igt_kms.h b/lib/igt_kms.h
index 1c46186e..59ccc4c6 100644
--- a/lib/igt_kms.h
+++ b/lib/igt_kms.h
@@ -38,6 +38,10 @@
 #include "igt_fb.h"
 #include "ioctl_wrappers.h"
 
+#ifndef DRM_MODE_CONNECTOR_WRITEBACK
+#define DRM_MODE_CONNECTOR_WRITEBACK   18
+#endif
+
 /* Low-level helpers with kmstest_ prefix */
 
 /**
@@ -120,6 +124,9 @@ enum igt_atomic_connector_properties {
        IGT_CONNECTOR_CRTC_ID,
        IGT_CONNECTOR_DPMS,
        IGT_CONNECTOR_BROADCAST_RGB,
+       IGT_CONNECTOR_WRITEBACK_PIXEL_FORMATS,
+       IGT_CONNECTOR_WRITEBACK_FB_ID,
+       IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR,
        IGT_NUM_CONNECTOR_PROPS
 };
 
@@ -345,6 +352,9 @@ typedef struct {
 
 	uint32_t props[IGT_NUM_CONNECTOR_PROPS];
 	uint64_t values[IGT_NUM_CONNECTOR_PROPS];
+
+	struct igt_fb *writeback_fb;
+	int32_t writeback_out_fence_fd;
 } igt_output_t;
 
 struct igt_display {
@@ -380,6 +390,10 @@ 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);
+void igt_output_request_writeback_out_fence(igt_output_t *output);
+int igt_output_get_last_writeback_out_fence(igt_output_t *output);
+
 igt_plane_t *igt_pipe_get_plane_type(igt_pipe_t *pipe, int plane_type);
 
 void igt_pipe_request_out_fence(igt_pipe_t *pipe);
-- 
2.16.1

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

^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH i-g-t 2/6] kms_writeback: Add initial writeback tests
  2018-03-01 17:38 ` [Intel-gfx] " Liviu Dudau
@ 2018-03-01 17:38   ` Liviu Dudau
  -1 siblings, 0 replies; 27+ messages in thread
From: Liviu Dudau @ 2018-03-01 17:38 UTC (permalink / raw)
  To: i-g-t; +Cc: Intel GFX ML, Rob Clark

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>
---
 lib/igt_kms.c          |   7 +-
 lib/igt_kms.h          |   8 ++
 tests/Makefile.sources |   1 +
 tests/kms_writeback.c  | 352 +++++++++++++++++++++++++++++++++++++++++++++++++
 tests/meson.build      |   1 +
 5 files changed, 365 insertions(+), 4 deletions(-)
 create mode 100644 tests/kms_writeback.c

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index 00d9d2e2..d558c1b8 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -2267,8 +2267,7 @@ static uint32_t igt_plane_get_fb_id(igt_plane_t *plane)
 /*
  * Add position and fb changes of a plane to the atomic property set
  */
-static void
-igt_atomic_prepare_plane_commit(igt_plane_t *plane, igt_pipe_t *pipe,
+void igt_atomic_prepare_plane_commit(igt_plane_t *plane, igt_pipe_t *pipe,
 	drmModeAtomicReq *req)
 {
 	igt_display_t *display = pipe->display;
@@ -2878,7 +2877,7 @@ igt_pipe_obj_replace_prop_blob(igt_pipe_t *pipe, enum igt_atomic_crtc_properties
 /*
  * Add crtc property changes to the atomic property set
  */
-static void igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicReq *req)
+void igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicReq *req)
 {
 	int i;
 
@@ -2902,7 +2901,7 @@ static void igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicRe
 /*
  * Add connector property changes to the atomic property set
  */
-static void igt_atomic_prepare_connector_commit(igt_output_t *output, drmModeAtomicReq *req)
+void igt_atomic_prepare_connector_commit(igt_output_t *output, drmModeAtomicReq *req)
 {
 
 	int i;
diff --git a/lib/igt_kms.h b/lib/igt_kms.h
index 59ccc4c6..f38fd0a0 100644
--- a/lib/igt_kms.h
+++ b/lib/igt_kms.h
@@ -581,6 +581,12 @@ extern void igt_output_replace_prop_blob(igt_output_t *output,
 					 enum igt_atomic_connector_properties prop,
 					 const void *ptr, size_t length);
 
+void igt_atomic_prepare_plane_commit(igt_plane_t *plane, igt_pipe_t *pipe,
+	drmModeAtomicReq *req);
+
+
+void igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicReq *req);
+
 /**
  * igt_pipe_obj_has_prop:
  * @pipe: Pipe to check.
@@ -670,6 +676,8 @@ extern void igt_pipe_obj_replace_prop_blob(igt_pipe_t *pipe,
 
 void igt_pipe_refresh(igt_display_t *display, enum pipe pipe, bool force);
 
+void igt_atomic_prepare_connector_commit(igt_output_t *output, drmModeAtomicReq *req);
+
 void igt_enable_connectors(void);
 void igt_reset_connectors(void);
 
diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index 23f859be..9cfa474d 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -212,6 +212,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 00000000..d922213d
--- /dev/null
+++ b/tests/kms_writeback.c
@@ -0,0 +1,352 @@
+/*
+ * (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"
+
+/* We need to define these ourselves until we get an updated libdrm */
+#ifndef DRM_MODE_CONNECTOR_WRITEBACK
+#define DRM_MODE_CONNECTOR_WRITEBACK   18
+#endif
+
+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)
+{
+	bool found;
+	uint64_t check_fb_id;
+
+	found = kmstest_get_property(output->display->drm_fd, output->id,
+				     DRM_MODE_OBJECT_CONNECTOR,
+				     igt_connector_prop_names[IGT_CONNECTOR_WRITEBACK_FB_ID],
+				     NULL, &check_fb_id, NULL);
+	igt_assert(found && (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;
+	enum pipe pipe;
+	drmModeAtomicReq *req;
+	igt_display_t *display = output->display;
+	struct kmstest_connector_config *config = &output->config;
+
+	req = drmModeAtomicAlloc();
+	drmModeAtomicSetCursor(req, 0);
+
+	for_each_pipe(display, pipe) {
+		igt_pipe_t *pipe_obj = &display->pipes[pipe];
+		igt_plane_t *plane;
+
+		/*
+		 * Add CRTC Properties to the property set
+		 */
+		igt_atomic_prepare_crtc_commit(pipe_obj, req);
+
+		for_each_plane_on_pipe(display, pipe, plane) {
+			igt_atomic_prepare_plane_commit(plane, pipe_obj, req);
+		}
+	}
+
+	igt_assert_lt(0, drmModeAtomicAddProperty(req, output->config.connector->connector_id,
+						  output->props[IGT_CONNECTOR_CRTC_ID],
+						  config->crtc->crtc_id));
+	igt_assert_lt(0, drmModeAtomicAddProperty(req, output->config.connector->connector_id,
+						  output->props[IGT_CONNECTOR_WRITEBACK_FB_ID],
+						  fb_id));
+	igt_assert_lt(0, drmModeAtomicAddProperty(req, output->config.connector->connector_id,
+						  output->props[IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR],
+						  (uint64_t)out_fence_ptr));
+
+	if (ptr_valid)
+		*out_fence_ptr = 0;
+
+	ret = drmModeAtomicCommit(display->drm_fd, req, flags, NULL);
+
+	if (ptr_valid && (ret || (flags & DRM_MODE_ATOMIC_TEST_ONLY)))
+		igt_assert(*out_fence_ptr == -1);
+
+	drmModeAtomicFree(req);
+
+	/* 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_assert_fd(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 ABCGNRUXY";
+		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 2a1e6f19..a08e782b 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -188,6 +188,7 @@ test_progs = [
 	'kms_tv_load_detect',
 	'kms_universal_plane',
 	'kms_vblank',
+	'kms_writeback',
 	'meta_test',
 	'perf',
 	'pm_backlight',
-- 
2.16.1

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

^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [igt-dev] [PATCH i-g-t 2/6] kms_writeback: Add initial writeback tests
@ 2018-03-01 17:38   ` Liviu Dudau
  0 siblings, 0 replies; 27+ messages in thread
From: Liviu Dudau @ 2018-03-01 17:38 UTC (permalink / raw)
  To: i-g-t; +Cc: Intel GFX ML, Rob Clark, Brian Starkey

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>
---
 lib/igt_kms.c          |   7 +-
 lib/igt_kms.h          |   8 ++
 tests/Makefile.sources |   1 +
 tests/kms_writeback.c  | 352 +++++++++++++++++++++++++++++++++++++++++++++++++
 tests/meson.build      |   1 +
 5 files changed, 365 insertions(+), 4 deletions(-)
 create mode 100644 tests/kms_writeback.c

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index 00d9d2e2..d558c1b8 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -2267,8 +2267,7 @@ static uint32_t igt_plane_get_fb_id(igt_plane_t *plane)
 /*
  * Add position and fb changes of a plane to the atomic property set
  */
-static void
-igt_atomic_prepare_plane_commit(igt_plane_t *plane, igt_pipe_t *pipe,
+void igt_atomic_prepare_plane_commit(igt_plane_t *plane, igt_pipe_t *pipe,
 	drmModeAtomicReq *req)
 {
 	igt_display_t *display = pipe->display;
@@ -2878,7 +2877,7 @@ igt_pipe_obj_replace_prop_blob(igt_pipe_t *pipe, enum igt_atomic_crtc_properties
 /*
  * Add crtc property changes to the atomic property set
  */
-static void igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicReq *req)
+void igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicReq *req)
 {
 	int i;
 
@@ -2902,7 +2901,7 @@ static void igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicRe
 /*
  * Add connector property changes to the atomic property set
  */
-static void igt_atomic_prepare_connector_commit(igt_output_t *output, drmModeAtomicReq *req)
+void igt_atomic_prepare_connector_commit(igt_output_t *output, drmModeAtomicReq *req)
 {
 
 	int i;
diff --git a/lib/igt_kms.h b/lib/igt_kms.h
index 59ccc4c6..f38fd0a0 100644
--- a/lib/igt_kms.h
+++ b/lib/igt_kms.h
@@ -581,6 +581,12 @@ extern void igt_output_replace_prop_blob(igt_output_t *output,
 					 enum igt_atomic_connector_properties prop,
 					 const void *ptr, size_t length);
 
+void igt_atomic_prepare_plane_commit(igt_plane_t *plane, igt_pipe_t *pipe,
+	drmModeAtomicReq *req);
+
+
+void igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicReq *req);
+
 /**
  * igt_pipe_obj_has_prop:
  * @pipe: Pipe to check.
@@ -670,6 +676,8 @@ extern void igt_pipe_obj_replace_prop_blob(igt_pipe_t *pipe,
 
 void igt_pipe_refresh(igt_display_t *display, enum pipe pipe, bool force);
 
+void igt_atomic_prepare_connector_commit(igt_output_t *output, drmModeAtomicReq *req);
+
 void igt_enable_connectors(void);
 void igt_reset_connectors(void);
 
diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index 23f859be..9cfa474d 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -212,6 +212,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 00000000..d922213d
--- /dev/null
+++ b/tests/kms_writeback.c
@@ -0,0 +1,352 @@
+/*
+ * (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"
+
+/* We need to define these ourselves until we get an updated libdrm */
+#ifndef DRM_MODE_CONNECTOR_WRITEBACK
+#define DRM_MODE_CONNECTOR_WRITEBACK   18
+#endif
+
+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)
+{
+	bool found;
+	uint64_t check_fb_id;
+
+	found = kmstest_get_property(output->display->drm_fd, output->id,
+				     DRM_MODE_OBJECT_CONNECTOR,
+				     igt_connector_prop_names[IGT_CONNECTOR_WRITEBACK_FB_ID],
+				     NULL, &check_fb_id, NULL);
+	igt_assert(found && (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;
+	enum pipe pipe;
+	drmModeAtomicReq *req;
+	igt_display_t *display = output->display;
+	struct kmstest_connector_config *config = &output->config;
+
+	req = drmModeAtomicAlloc();
+	drmModeAtomicSetCursor(req, 0);
+
+	for_each_pipe(display, pipe) {
+		igt_pipe_t *pipe_obj = &display->pipes[pipe];
+		igt_plane_t *plane;
+
+		/*
+		 * Add CRTC Properties to the property set
+		 */
+		igt_atomic_prepare_crtc_commit(pipe_obj, req);
+
+		for_each_plane_on_pipe(display, pipe, plane) {
+			igt_atomic_prepare_plane_commit(plane, pipe_obj, req);
+		}
+	}
+
+	igt_assert_lt(0, drmModeAtomicAddProperty(req, output->config.connector->connector_id,
+						  output->props[IGT_CONNECTOR_CRTC_ID],
+						  config->crtc->crtc_id));
+	igt_assert_lt(0, drmModeAtomicAddProperty(req, output->config.connector->connector_id,
+						  output->props[IGT_CONNECTOR_WRITEBACK_FB_ID],
+						  fb_id));
+	igt_assert_lt(0, drmModeAtomicAddProperty(req, output->config.connector->connector_id,
+						  output->props[IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR],
+						  (uint64_t)out_fence_ptr));
+
+	if (ptr_valid)
+		*out_fence_ptr = 0;
+
+	ret = drmModeAtomicCommit(display->drm_fd, req, flags, NULL);
+
+	if (ptr_valid && (ret || (flags & DRM_MODE_ATOMIC_TEST_ONLY)))
+		igt_assert(*out_fence_ptr == -1);
+
+	drmModeAtomicFree(req);
+
+	/* 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_assert_fd(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 ABCGNRUXY";
+		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 2a1e6f19..a08e782b 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -188,6 +188,7 @@ test_progs = [
 	'kms_tv_load_detect',
 	'kms_universal_plane',
 	'kms_vblank',
+	'kms_writeback',
 	'meta_test',
 	'perf',
 	'pm_backlight',
-- 
2.16.1

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

^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH i-g-t 3/6] lib: Add function to hash a framebuffer
  2018-03-01 17:38 ` [Intel-gfx] " Liviu Dudau
@ 2018-03-01 17:38   ` Liviu Dudau
  -1 siblings, 0 replies; 27+ messages in thread
From: Liviu Dudau @ 2018-03-01 17:38 UTC (permalink / raw)
  To: i-g-t; +Cc: Intel GFX ML, Rob Clark

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>
---
 lib/igt_fb.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/igt_fb.h |  5 +++++
 2 files changed, 70 insertions(+)

diff --git a/lib/igt_fb.c b/lib/igt_fb.c
index 7404ba7c..1501006e 100644
--- a/lib/igt_fb.c
+++ b/lib/igt_fb.c
@@ -1705,3 +1705,68 @@ 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;
+
+	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(fb->stride);
+	if (!line) {
+		munmap(map, fb->size);
+		return -ENOMEM;
+	}
+
+	hash = FNV1a_OFFSET_BIAS;
+
+	for (y = 0; y < fb->height; y++, ptr += fb->stride) {
+
+		memcpy(line, ptr, fb->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
+}
diff --git a/lib/igt_fb.h b/lib/igt_fb.h
index 023b069d..bc615516 100644
--- a/lib/igt_fb.h
+++ b/lib/igt_fb.h
@@ -36,6 +36,8 @@
 
 #include <i915_drm.h>
 
+#include "igt_debugfs.h"
+
 /**
  * igt_fb_t:
  * @fb_id: KMS ID of the framebuffer
@@ -164,5 +166,8 @@ uint32_t igt_drm_format_to_bpp(uint32_t drm_format);
 const char *igt_format_str(uint32_t drm_format);
 bool igt_fb_supported_format(uint32_t drm_format);
 
+/* Get a hash for a framebuffer */
+int igt_fb_get_crc(struct igt_fb *fb, igt_crc_t *crc);
+
 #endif /* __IGT_FB_H__ */
 
-- 
2.16.1

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

^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [igt-dev] [PATCH i-g-t 3/6] lib: Add function to hash a framebuffer
@ 2018-03-01 17:38   ` Liviu Dudau
  0 siblings, 0 replies; 27+ messages in thread
From: Liviu Dudau @ 2018-03-01 17:38 UTC (permalink / raw)
  To: i-g-t; +Cc: Intel GFX ML, Rob Clark, Brian Starkey

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>
---
 lib/igt_fb.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/igt_fb.h |  5 +++++
 2 files changed, 70 insertions(+)

diff --git a/lib/igt_fb.c b/lib/igt_fb.c
index 7404ba7c..1501006e 100644
--- a/lib/igt_fb.c
+++ b/lib/igt_fb.c
@@ -1705,3 +1705,68 @@ 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;
+
+	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(fb->stride);
+	if (!line) {
+		munmap(map, fb->size);
+		return -ENOMEM;
+	}
+
+	hash = FNV1a_OFFSET_BIAS;
+
+	for (y = 0; y < fb->height; y++, ptr += fb->stride) {
+
+		memcpy(line, ptr, fb->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
+}
diff --git a/lib/igt_fb.h b/lib/igt_fb.h
index 023b069d..bc615516 100644
--- a/lib/igt_fb.h
+++ b/lib/igt_fb.h
@@ -36,6 +36,8 @@
 
 #include <i915_drm.h>
 
+#include "igt_debugfs.h"
+
 /**
  * igt_fb_t:
  * @fb_id: KMS ID of the framebuffer
@@ -164,5 +166,8 @@ uint32_t igt_drm_format_to_bpp(uint32_t drm_format);
 const char *igt_format_str(uint32_t drm_format);
 bool igt_fb_supported_format(uint32_t drm_format);
 
+/* Get a hash for a framebuffer */
+int igt_fb_get_crc(struct igt_fb *fb, igt_crc_t *crc);
+
 #endif /* __IGT_FB_H__ */
 
-- 
2.16.1

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

^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH i-g-t 4/6] kms_writeback: Add writeback-check-output
  2018-03-01 17:38 ` [Intel-gfx] " Liviu Dudau
@ 2018-03-01 17:38   ` Liviu Dudau
  -1 siblings, 0 replies; 27+ messages in thread
From: Liviu Dudau @ 2018-03-01 17:38 UTC (permalink / raw)
  To: i-g-t; +Cc: Intel GFX ML, Rob Clark

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 | 123 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 123 insertions(+)

diff --git a/tests/kms_writeback.c b/tests/kms_writeback.c
index d922213d..af896e05 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"
 
 /* We need to define these ourselves until we get an updated libdrm */
 #ifndef DRM_MODE_CONNECTOR_WRITEBACK
@@ -259,6 +260,115 @@ 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, out_fence = out_fence = igt_output_get_last_writeback_out_fence(output);
+	igt_assert(out_fence >= 0);
+
+	ret = sync_fence_wait(out_fence, 1000);
+	igt_assert(ret == 0);
+	close(out_fence);
+}
+
+static void writeback_seqence(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]);
+		if (out_fbs[i])
+			igt_output_request_writeback_out_fence(output);
+		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_seqence(output, plane, input_fb, &output_fb, 1);
+
+	/* Two commits, the second with no writeback */
+	out_fbs[0] = output_fb;
+	writeback_seqence(output, plane, input_fb, out_fbs, 2);
+
+	/* Two commits, both with writeback */
+	out_fbs[1] = output_fb;
+	writeback_seqence(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_seqence(output, plane, input_fb, out_fbs, 2);
+
+	igt_remove_fb(output_fb->fd, &second_out_fb);
+}
+
 igt_main
 {
 	igt_display_t display;
@@ -345,6 +455,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.16.1

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

^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [igt-dev] [PATCH i-g-t 4/6] kms_writeback: Add writeback-check-output
@ 2018-03-01 17:38   ` Liviu Dudau
  0 siblings, 0 replies; 27+ messages in thread
From: Liviu Dudau @ 2018-03-01 17:38 UTC (permalink / raw)
  To: i-g-t; +Cc: Intel GFX ML, Rob Clark, Brian Starkey

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 | 123 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 123 insertions(+)

diff --git a/tests/kms_writeback.c b/tests/kms_writeback.c
index d922213d..af896e05 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"
 
 /* We need to define these ourselves until we get an updated libdrm */
 #ifndef DRM_MODE_CONNECTOR_WRITEBACK
@@ -259,6 +260,115 @@ 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, out_fence = out_fence = igt_output_get_last_writeback_out_fence(output);
+	igt_assert(out_fence >= 0);
+
+	ret = sync_fence_wait(out_fence, 1000);
+	igt_assert(ret == 0);
+	close(out_fence);
+}
+
+static void writeback_seqence(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]);
+		if (out_fbs[i])
+			igt_output_request_writeback_out_fence(output);
+		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_seqence(output, plane, input_fb, &output_fb, 1);
+
+	/* Two commits, the second with no writeback */
+	out_fbs[0] = output_fb;
+	writeback_seqence(output, plane, input_fb, out_fbs, 2);
+
+	/* Two commits, both with writeback */
+	out_fbs[1] = output_fb;
+	writeback_seqence(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_seqence(output, plane, input_fb, out_fbs, 2);
+
+	igt_remove_fb(output_fb->fd, &second_out_fb);
+}
+
 igt_main
 {
 	igt_display_t display;
@@ -345,6 +455,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.16.1

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

^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH i-g-t 5/6] lib/igt_kms: Add igt_output_clone_pipe for cloning
  2018-03-01 17:38 ` [Intel-gfx] " Liviu Dudau
@ 2018-03-01 17:38   ` Liviu Dudau
  -1 siblings, 0 replies; 27+ messages in thread
From: Liviu Dudau @ 2018-03-01 17:38 UTC (permalink / raw)
  To: i-g-t; +Cc: Intel GFX ML, Rob Clark

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 d558c1b8..23fb064f 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -1646,6 +1646,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;
@@ -2133,42 +2144,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;
@@ -2192,6 +2167,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,
@@ -3344,6 +3352,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
@@ -3351,6 +3360,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
@@ -3367,11 +3386,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 f38fd0a0..6d13de21 100644
--- a/lib/igt_kms.h
+++ b/lib/igt_kms.h
@@ -334,6 +334,9 @@ struct igt_pipe {
 	uint32_t crtc_id;
 
 	int32_t out_fence_fd;
+	bool out_fence_requested;
+
+	uint32_t outputs;
 };
 
 typedef struct {
@@ -386,6 +389,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.16.1

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

^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [Intel-gfx] [PATCH i-g-t 5/6] lib/igt_kms: Add igt_output_clone_pipe for cloning
@ 2018-03-01 17:38   ` Liviu Dudau
  0 siblings, 0 replies; 27+ messages in thread
From: Liviu Dudau @ 2018-03-01 17:38 UTC (permalink / raw)
  To: i-g-t; +Cc: Intel GFX ML, Rob Clark

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 d558c1b8..23fb064f 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -1646,6 +1646,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;
@@ -2133,42 +2144,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;
@@ -2192,6 +2167,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,
@@ -3344,6 +3352,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
@@ -3351,6 +3360,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
@@ -3367,11 +3386,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 f38fd0a0..6d13de21 100644
--- a/lib/igt_kms.h
+++ b/lib/igt_kms.h
@@ -334,6 +334,9 @@ struct igt_pipe {
 	uint32_t crtc_id;
 
 	int32_t out_fence_fd;
+	bool out_fence_requested;
+
+	uint32_t outputs;
 };
 
 typedef struct {
@@ -386,6 +389,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.16.1

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

^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH i-g-t 6/6] kms_writeback: Add tests using a cloned output
  2018-03-01 17:38 ` [Intel-gfx] " Liviu Dudau
@ 2018-03-01 17:38   ` Liviu Dudau
  -1 siblings, 0 replies; 27+ messages in thread
From: Liviu Dudau @ 2018-03-01 17:38 UTC (permalink / raw)
  To: i-g-t; +Cc: Intel GFX ML, Rob Clark

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>
---
 tests/kms_writeback.c | 76 ++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 66 insertions(+), 10 deletions(-)

diff --git a/tests/kms_writeback.c b/tests/kms_writeback.c
index af896e05..5da7086f 100644
--- a/tests/kms_writeback.c
+++ b/tests/kms_writeback.c
@@ -56,7 +56,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;
@@ -98,6 +99,27 @@ 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);
+				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);
+				if (!newret) {
+					*clone = second_output;
+					break;
+				}
+			}
+		}
+	}
 	igt_plane_set_fb(plane, NULL);
 	igt_remove_fb(display->drm_fd, &input_fb);
 	igt_remove_fb(display->drm_fd, &output_fb);
@@ -105,7 +127,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;
 
@@ -121,10 +144,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;
 			}
 		}
@@ -165,9 +194,6 @@ static int do_writeback_test(igt_output_t *output, uint32_t flags,
 		igt_pipe_t *pipe_obj = &display->pipes[pipe];
 		igt_plane_t *plane;
 
-		/*
-		 * Add CRTC Properties to the property set
-		 */
 		igt_atomic_prepare_crtc_commit(pipe_obj, req);
 
 		for_each_plane_on_pipe(display, pipe, plane) {
@@ -311,6 +337,9 @@ static void writeback_seqence(igt_output_t *output, igt_plane_t *plane,
 		/* Commit */
 		igt_plane_set_fb(plane, in_fb);
 		igt_output_set_writeback_fb(output, out_fbs[i]);
+		igt_output_set_prop_value(output, IGT_CONNECTOR_WRITEBACK_FB_ID,
+					  out_fbs[i] ? out_fbs[i]->fb_id : 0);
+
 		if (out_fbs[i])
 			igt_output_request_writeback_out_fence(output);
 		igt_display_commit_atomic(output->display,
@@ -372,10 +401,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));
@@ -390,12 +420,13 @@ 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)
+		if (output->use_override_mode) {
 			memcpy(&mode, &output->override_mode, sizeof(mode));
-		else
+			igt_output_override_mode(output, &mode);
+		} else
 			memcpy(&mode, &output->config.default_mode, sizeof(mode));
 
 		plane = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY);
@@ -468,6 +499,31 @@ 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);
+
+		/*
+		 * ToDo: this should be forcing the other pipe to be a clone
+		 * of the writeback pipe, but it doesn't seem to happen.
+		 * Need to investigate the reason
+		 */
+		//igt_output_clone_pipe(clone, pipe);
+
+		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.16.1

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

^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [igt-dev] [PATCH i-g-t 6/6] kms_writeback: Add tests using a cloned output
@ 2018-03-01 17:38   ` Liviu Dudau
  0 siblings, 0 replies; 27+ messages in thread
From: Liviu Dudau @ 2018-03-01 17:38 UTC (permalink / raw)
  To: i-g-t; +Cc: Intel GFX ML, Rob Clark, Brian Starkey

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>
---
 tests/kms_writeback.c | 76 ++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 66 insertions(+), 10 deletions(-)

diff --git a/tests/kms_writeback.c b/tests/kms_writeback.c
index af896e05..5da7086f 100644
--- a/tests/kms_writeback.c
+++ b/tests/kms_writeback.c
@@ -56,7 +56,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;
@@ -98,6 +99,27 @@ 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);
+				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);
+				if (!newret) {
+					*clone = second_output;
+					break;
+				}
+			}
+		}
+	}
 	igt_plane_set_fb(plane, NULL);
 	igt_remove_fb(display->drm_fd, &input_fb);
 	igt_remove_fb(display->drm_fd, &output_fb);
@@ -105,7 +127,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;
 
@@ -121,10 +144,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;
 			}
 		}
@@ -165,9 +194,6 @@ static int do_writeback_test(igt_output_t *output, uint32_t flags,
 		igt_pipe_t *pipe_obj = &display->pipes[pipe];
 		igt_plane_t *plane;
 
-		/*
-		 * Add CRTC Properties to the property set
-		 */
 		igt_atomic_prepare_crtc_commit(pipe_obj, req);
 
 		for_each_plane_on_pipe(display, pipe, plane) {
@@ -311,6 +337,9 @@ static void writeback_seqence(igt_output_t *output, igt_plane_t *plane,
 		/* Commit */
 		igt_plane_set_fb(plane, in_fb);
 		igt_output_set_writeback_fb(output, out_fbs[i]);
+		igt_output_set_prop_value(output, IGT_CONNECTOR_WRITEBACK_FB_ID,
+					  out_fbs[i] ? out_fbs[i]->fb_id : 0);
+
 		if (out_fbs[i])
 			igt_output_request_writeback_out_fence(output);
 		igt_display_commit_atomic(output->display,
@@ -372,10 +401,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));
@@ -390,12 +420,13 @@ 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)
+		if (output->use_override_mode) {
 			memcpy(&mode, &output->override_mode, sizeof(mode));
-		else
+			igt_output_override_mode(output, &mode);
+		} else
 			memcpy(&mode, &output->config.default_mode, sizeof(mode));
 
 		plane = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY);
@@ -468,6 +499,31 @@ 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);
+
+		/*
+		 * ToDo: this should be forcing the other pipe to be a clone
+		 * of the writeback pipe, but it doesn't seem to happen.
+		 * Need to investigate the reason
+		 */
+		//igt_output_clone_pipe(clone, pipe);
+
+		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.16.1

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

^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [igt-dev] ✗ Fi.CI.BAT: failure for igt: Add support for testing writeback connectors
  2018-03-01 17:38 ` [Intel-gfx] " Liviu Dudau
                   ` (6 preceding siblings ...)
  (?)
@ 2018-03-01 18:48 ` Patchwork
  -1 siblings, 0 replies; 27+ messages in thread
From: Patchwork @ 2018-03-01 18:48 UTC (permalink / raw)
  To: Liviu Dudau; +Cc: igt-dev

== Series Details ==

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

== Summary ==

IGT patchset tested on top of latest successful build
a5d7e165fe5558487799ad75b9627f49947337af tools/intel_dp_compliance: Add missing GLIB_CFLAGS

with latest DRM-Tip kernel build CI_DRM_3860
7fe823aaeff7 drm-tip: 2018y-03m-01d-15h-58m-23s UTC integration manifest

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

---- Possible new issues:

Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                pass       -> INCOMPLETE (fi-hsw-4770)

---- Known issues:

Test debugfs_test:
        Subgroup read_all_entries:
                pass       -> INCOMPLETE (fi-snb-2520m) fdo#103713
Test gem_mmap_gtt:
        Subgroup basic-small-bo-tiledx:
                pass       -> FAIL       (fi-gdg-551) fdo#102575

fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
fdo#102575 https://bugs.freedesktop.org/show_bug.cgi?id=102575

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:416s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:425s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:373s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:486s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:279s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:482s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:470s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:470s
fi-cfl-8700k     total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:396s
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:566s
fi-cnl-y3        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:591s
fi-elk-e7500     total:288  pass:229  dwarn:0   dfail:0   fail:0   skip:59  time:417s
fi-gdg-551       total:288  pass:179  dwarn:0   dfail:0   fail:1   skip:108 time:288s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:505s
fi-hsw-4770      total:245  pass:221  dwarn:0   dfail:0   fail:0   skip:23 
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:416s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:454s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:410s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:451s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:489s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:447s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:494s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:589s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:425s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:501s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:519s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:486s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:487s
fi-skl-guc       total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:407s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:431s
fi-snb-2520m     total:3    pass:2    dwarn:0   dfail:0   fail:0   skip:0  
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:394s
Blacklisted hosts:
fi-cfl-u         total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:503s

== Logs ==

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [igt-dev] ✓ Fi.CI.BAT: success for igt: Add support for testing writeback connectors
  2018-03-01 17:38 ` [Intel-gfx] " Liviu Dudau
                   ` (7 preceding siblings ...)
  (?)
@ 2018-03-02 19:58 ` Patchwork
  -1 siblings, 0 replies; 27+ messages in thread
From: Patchwork @ 2018-03-02 19:58 UTC (permalink / raw)
  To: Liviu Dudau; +Cc: igt-dev

== Series Details ==

Series: igt: Add support for testing writeback connectors
URL   : https://patchwork.freedesktop.org/series/39229/
State : success

== Summary ==

IGT patchset tested on top of latest successful build
bddfb8dd3c1767f13d2af578d5c3d897fddf0dcd igt/gem_ctx_switch: Exercise all engines at once

with latest DRM-Tip kernel build CI_DRM_3867
4f4e4dd52a30 drm-tip: 2018y-03m-02d-16h-28m-21s UTC integration manifest

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

---- Known issues:

Test debugfs_test:
        Subgroup read_all_entries:
                pass       -> INCOMPLETE (fi-snb-2520m) fdo#103713

fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:416s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:423s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:372s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:491s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:279s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:481s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:483s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:470s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:456s
fi-cfl-8700k     total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:393s
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:559s
fi-cfl-u         total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:495s
fi-cnl-y3        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:580s
fi-elk-e7500     total:288  pass:229  dwarn:0   dfail:0   fail:0   skip:59  time:415s
fi-gdg-551       total:288  pass:179  dwarn:0   dfail:0   fail:1   skip:108 time:289s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:508s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:386s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:409s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:451s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:409s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:450s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:488s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:448s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:491s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:584s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:434s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:500s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:519s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:485s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:472s
fi-skl-guc       total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:406s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:431s
fi-snb-2520m     total:3    pass:2    dwarn:0   dfail:0   fail:0   skip:0  
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:393s

== Logs ==

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [igt-dev] ✗ Fi.CI.IGT: warning for igt: Add support for testing writeback connectors
  2018-03-01 17:38 ` [Intel-gfx] " Liviu Dudau
                   ` (8 preceding siblings ...)
  (?)
@ 2018-03-02 22:56 ` Patchwork
  -1 siblings, 0 replies; 27+ messages in thread
From: Patchwork @ 2018-03-02 22:56 UTC (permalink / raw)
  To: Liviu Dudau; +Cc: igt-dev

== Series Details ==

Series: igt: Add support for testing writeback connectors
URL   : https://patchwork.freedesktop.org/series/39229/
State : warning

== Summary ==

---- Possible new issues:

Test kms_frontbuffer_tracking:
        Subgroup fbc-1p-primscrn-cur-indfb-draw-mmap-cpu:
                pass       -> SKIP       (shard-snb)

---- Known issues:

Test gem_eio:
        Subgroup in-flight:
                incomplete -> PASS       (shard-apl) fdo#104945
Test kms_chv_cursor_fail:
        Subgroup pipe-b-256x256-left-edge:
                pass       -> DMESG-WARN (shard-snb) fdo#105185 +1
Test kms_fbcon_fbt:
        Subgroup fbc-suspend:
                incomplete -> PASS       (shard-hsw) fdo#105087
Test kms_flip:
        Subgroup 2x-modeset-vs-vblank-race-interruptible:
                pass       -> FAIL       (shard-hsw) fdo#103060
        Subgroup plain-flip-fb-recreate:
                pass       -> FAIL       (shard-hsw) fdo#100368
Test kms_rotation_crc:
        Subgroup primary-rotation-180:
                pass       -> FAIL       (shard-snb) fdo#103925
Test kms_sysfs_edid_timing:
                warn       -> PASS       (shard-apl) fdo#100047

fdo#104945 https://bugs.freedesktop.org/show_bug.cgi?id=104945
fdo#105185 https://bugs.freedesktop.org/show_bug.cgi?id=105185
fdo#105087 https://bugs.freedesktop.org/show_bug.cgi?id=105087
fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060
fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
fdo#103925 https://bugs.freedesktop.org/show_bug.cgi?id=103925
fdo#100047 https://bugs.freedesktop.org/show_bug.cgi?id=100047

shard-apl        total:3468 pass:1823 dwarn:1   dfail:0   fail:7   skip:1637 time:12588s
shard-hsw        total:3468 pass:1768 dwarn:1   dfail:0   fail:3   skip:1695 time:12284s
shard-snb        total:3468 pass:1359 dwarn:2   dfail:0   fail:2   skip:2105 time:7143s
Blacklisted hosts:
shard-kbl        total:3468 pass:1946 dwarn:1   dfail:0   fail:8   skip:1513 time:9906s

== Logs ==

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [igt-dev] [PATCH i-g-t 1/6] lib/igt_kms: Add writeback support in lib/
  2018-03-01 17:38   ` [igt-dev] " Liviu Dudau
@ 2018-06-25 12:19     ` Maarten Lankhorst
  -1 siblings, 0 replies; 27+ messages in thread
From: Maarten Lankhorst @ 2018-06-25 12:19 UTC (permalink / raw)
  To: Liviu Dudau, i-g-t; +Cc: Intel GFX ML, Rob Clark

Op 01-03-18 om 18:38 schreef Liviu Dudau:
> From: Brian Starkey <brian.starkey@arm.com>
>
> Add support in igt_kms for Writeback connectors, with the ability to
> attach framebuffers and retrieve fences.
>
> Signed-off-by: Brian Starkey <brian.starkey@arm.com>
> ---
>  lib/igt_kms.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  lib/igt_kms.h | 14 ++++++++++++
>  2 files changed, 85 insertions(+), 1 deletion(-)
>
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index 022abfe7..00d9d2e2 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -190,7 +190,10 @@ const char *igt_connector_prop_names[IGT_NUM_CONNECTOR_PROPS] = {
>  	"scaling mode",
>  	"CRTC_ID",
>  	"DPMS",
> -	"Broadcast RGB"
> +	"Broadcast RGB",
> +	"WRITEBACK_PIXEL_FORMATS",
> +	"WRITEBACK_FB_ID",
> +	"WRITEBACK_OUT_FENCE_PTR"
>  };
>  
>  /*
> @@ -452,6 +455,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" },
>  	{}
>  };
>  
> @@ -1947,6 +1951,7 @@ void igt_display_init(igt_display_t *display, int drm_fd)
>  		output->force_reprobe = true;
>  		output->id = resources->connectors[i];
>  		output->display = display;
> +		output->writeback_out_fence_fd = -1;
>  
>  		igt_output_refresh(output);
>  	}
> @@ -2040,6 +2045,43 @@ igt_output_t *igt_output_from_connector(igt_display_t *display,
>  	return found;
>  }
>  
> +void igt_output_set_writeback_fb(igt_output_t *output, struct igt_fb *fb)
> +{
> +	igt_display_t *display = output->display;
> +	struct kmstest_connector_config *config = &output->config;
> +
> +	if (config->connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK)
> +		return;

igt_output_set_prop_value(output, IGT_CONNECTOR_WRITEBACK_FB_ID, ...);

As a bonus you don't need to check for WRITEBACK type, because this property will be undefined and commit will fail.

> +	LOG(display, "%s: output_set_writeback_fb(%d)\n", output->name,
> +	    fb ? fb->fb_id : 0);
> +
> +	output->writeback_fb = fb;
> +}
Probably want to merge this with igt_output_request_writeback_out_fence(), else there's no point for the convenience function. :)

> +static void igt_output_reset_writeback_out_fence(igt_output_t *output)
> +{
> +	if (output->writeback_out_fence_fd >= 0) {
> +		close(output->writeback_out_fence_fd);
> +		output->writeback_out_fence_fd = -1;
> +	}
> +}
> +
> +void igt_output_request_writeback_out_fence(igt_output_t *output)
> +{
> +	igt_output_reset_writeback_out_fence(output);
> +	igt_output_set_prop_value(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR,
> +				  (ptrdiff_t)&output->writeback_out_fence_fd);
> +}
> +
> +int igt_output_get_last_writeback_out_fence(igt_output_t *output)
> +{
> +	int fd = output->writeback_out_fence_fd;
> +	output->writeback_out_fence_fd = -1;
> +
> +	return fd;
> +}
Please handle this the same we do for pipe_obj->out_fence_fd.. No need to do it differently.
> +
>  static void igt_pipe_fini(igt_pipe_t *pipe)
>  {
>  	int i;
> @@ -2063,6 +2105,8 @@ static void igt_pipe_fini(igt_pipe_t *pipe)
>  static void igt_output_fini(igt_output_t *output)
>  {
>  	kmstest_free_connector_config(&output->config);
> +	if (output->writeback_out_fence_fd >= 0)
> +		close(output->writeback_out_fence_fd);
>  	free(output->name);
>  	output->name = NULL;
>  }
> @@ -2879,6 +2923,31 @@ static void igt_atomic_prepare_connector_commit(igt_output_t *output, drmModeAto
>  					  output->props[i],
>  					  output->values[i]));
>  	}
> +
> +	if (output->writeback_fb)
> +		output->writeback_fb = NULL;
> +
> +	igt_output_reset_writeback_out_fence(output);
> +}
> +
> +static void handle_writeback_out_fences(igt_display_t *display, uint32_t flags, int ret)
> +{
> +	int i;
> +
> +	for (i = 0; i < display->n_outputs; i++) {
> +		igt_output_t *output = &display->outputs[i];
> +
> +		if (!output->config.connector)
> +			continue;
> +
> +		if (!igt_output_is_prop_changed(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR))
> +			continue;
> +
> +		igt_output_clear_prop_changed(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR);
> +
> +		if (ret || (flags & DRM_MODE_ATOMIC_TEST_ONLY))
> +			igt_assert(output->writeback_out_fence_fd == -1);
> +	}
>  }
>  
>  /*
> @@ -2929,6 +2998,7 @@ static int igt_atomic_commit(igt_display_t *display, uint32_t flags, void *user_
>  	}
>  
>  	ret = drmModeAtomicCommit(display->drm_fd, req, flags, user_data);
> +	handle_writeback_out_fences(display, flags, ret);
>  
>  	drmModeAtomicFree(req);
>  	return ret;
> diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> index 1c46186e..59ccc4c6 100644
> --- a/lib/igt_kms.h
> +++ b/lib/igt_kms.h
> @@ -38,6 +38,10 @@
>  #include "igt_fb.h"
>  #include "ioctl_wrappers.h"
>  
> +#ifndef DRM_MODE_CONNECTOR_WRITEBACK
> +#define DRM_MODE_CONNECTOR_WRITEBACK   18
> +#endif
> +
>  /* Low-level helpers with kmstest_ prefix */
>  
>  /**
> @@ -120,6 +124,9 @@ enum igt_atomic_connector_properties {
>         IGT_CONNECTOR_CRTC_ID,
>         IGT_CONNECTOR_DPMS,
>         IGT_CONNECTOR_BROADCAST_RGB,
> +       IGT_CONNECTOR_WRITEBACK_PIXEL_FORMATS,
> +       IGT_CONNECTOR_WRITEBACK_FB_ID,
> +       IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR,
>         IGT_NUM_CONNECTOR_PROPS
>  };
>  
> @@ -345,6 +352,9 @@ typedef struct {
>  
>  	uint32_t props[IGT_NUM_CONNECTOR_PROPS];
>  	uint64_t values[IGT_NUM_CONNECTOR_PROPS];
> +
> +	struct igt_fb *writeback_fb;
I've spent a lot of effort into making sure we can set any property directly. Please don't add this struct member here,
there's no need for it.
> +	int32_t writeback_out_fence_fd;
>  } igt_output_t;
>  
>  struct igt_display {
> @@ -380,6 +390,10 @@ 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);
> +void igt_output_request_writeback_out_fence(igt_output_t *output);
> +int igt_output_get_last_writeback_out_fence(igt_output_t *output);
> +
>  igt_plane_t *igt_pipe_get_plane_type(igt_pipe_t *pipe, int plane_type);
>  
>  void igt_pipe_request_out_fence(igt_pipe_t *pipe);


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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [igt-dev] [PATCH i-g-t 1/6] lib/igt_kms: Add writeback support in lib/
@ 2018-06-25 12:19     ` Maarten Lankhorst
  0 siblings, 0 replies; 27+ messages in thread
From: Maarten Lankhorst @ 2018-06-25 12:19 UTC (permalink / raw)
  To: Liviu Dudau, i-g-t; +Cc: Intel GFX ML, Rob Clark, Brian Starkey

Op 01-03-18 om 18:38 schreef Liviu Dudau:
> From: Brian Starkey <brian.starkey@arm.com>
>
> Add support in igt_kms for Writeback connectors, with the ability to
> attach framebuffers and retrieve fences.
>
> Signed-off-by: Brian Starkey <brian.starkey@arm.com>
> ---
>  lib/igt_kms.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  lib/igt_kms.h | 14 ++++++++++++
>  2 files changed, 85 insertions(+), 1 deletion(-)
>
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index 022abfe7..00d9d2e2 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -190,7 +190,10 @@ const char *igt_connector_prop_names[IGT_NUM_CONNECTOR_PROPS] = {
>  	"scaling mode",
>  	"CRTC_ID",
>  	"DPMS",
> -	"Broadcast RGB"
> +	"Broadcast RGB",
> +	"WRITEBACK_PIXEL_FORMATS",
> +	"WRITEBACK_FB_ID",
> +	"WRITEBACK_OUT_FENCE_PTR"
>  };
>  
>  /*
> @@ -452,6 +455,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" },
>  	{}
>  };
>  
> @@ -1947,6 +1951,7 @@ void igt_display_init(igt_display_t *display, int drm_fd)
>  		output->force_reprobe = true;
>  		output->id = resources->connectors[i];
>  		output->display = display;
> +		output->writeback_out_fence_fd = -1;
>  
>  		igt_output_refresh(output);
>  	}
> @@ -2040,6 +2045,43 @@ igt_output_t *igt_output_from_connector(igt_display_t *display,
>  	return found;
>  }
>  
> +void igt_output_set_writeback_fb(igt_output_t *output, struct igt_fb *fb)
> +{
> +	igt_display_t *display = output->display;
> +	struct kmstest_connector_config *config = &output->config;
> +
> +	if (config->connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK)
> +		return;

igt_output_set_prop_value(output, IGT_CONNECTOR_WRITEBACK_FB_ID, ...);

As a bonus you don't need to check for WRITEBACK type, because this property will be undefined and commit will fail.

> +	LOG(display, "%s: output_set_writeback_fb(%d)\n", output->name,
> +	    fb ? fb->fb_id : 0);
> +
> +	output->writeback_fb = fb;
> +}
Probably want to merge this with igt_output_request_writeback_out_fence(), else there's no point for the convenience function. :)

> +static void igt_output_reset_writeback_out_fence(igt_output_t *output)
> +{
> +	if (output->writeback_out_fence_fd >= 0) {
> +		close(output->writeback_out_fence_fd);
> +		output->writeback_out_fence_fd = -1;
> +	}
> +}
> +
> +void igt_output_request_writeback_out_fence(igt_output_t *output)
> +{
> +	igt_output_reset_writeback_out_fence(output);
> +	igt_output_set_prop_value(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR,
> +				  (ptrdiff_t)&output->writeback_out_fence_fd);
> +}
> +
> +int igt_output_get_last_writeback_out_fence(igt_output_t *output)
> +{
> +	int fd = output->writeback_out_fence_fd;
> +	output->writeback_out_fence_fd = -1;
> +
> +	return fd;
> +}
Please handle this the same we do for pipe_obj->out_fence_fd.. No need to do it differently.
> +
>  static void igt_pipe_fini(igt_pipe_t *pipe)
>  {
>  	int i;
> @@ -2063,6 +2105,8 @@ static void igt_pipe_fini(igt_pipe_t *pipe)
>  static void igt_output_fini(igt_output_t *output)
>  {
>  	kmstest_free_connector_config(&output->config);
> +	if (output->writeback_out_fence_fd >= 0)
> +		close(output->writeback_out_fence_fd);
>  	free(output->name);
>  	output->name = NULL;
>  }
> @@ -2879,6 +2923,31 @@ static void igt_atomic_prepare_connector_commit(igt_output_t *output, drmModeAto
>  					  output->props[i],
>  					  output->values[i]));
>  	}
> +
> +	if (output->writeback_fb)
> +		output->writeback_fb = NULL;
> +
> +	igt_output_reset_writeback_out_fence(output);
> +}
> +
> +static void handle_writeback_out_fences(igt_display_t *display, uint32_t flags, int ret)
> +{
> +	int i;
> +
> +	for (i = 0; i < display->n_outputs; i++) {
> +		igt_output_t *output = &display->outputs[i];
> +
> +		if (!output->config.connector)
> +			continue;
> +
> +		if (!igt_output_is_prop_changed(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR))
> +			continue;
> +
> +		igt_output_clear_prop_changed(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR);
> +
> +		if (ret || (flags & DRM_MODE_ATOMIC_TEST_ONLY))
> +			igt_assert(output->writeback_out_fence_fd == -1);
> +	}
>  }
>  
>  /*
> @@ -2929,6 +2998,7 @@ static int igt_atomic_commit(igt_display_t *display, uint32_t flags, void *user_
>  	}
>  
>  	ret = drmModeAtomicCommit(display->drm_fd, req, flags, user_data);
> +	handle_writeback_out_fences(display, flags, ret);
>  
>  	drmModeAtomicFree(req);
>  	return ret;
> diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> index 1c46186e..59ccc4c6 100644
> --- a/lib/igt_kms.h
> +++ b/lib/igt_kms.h
> @@ -38,6 +38,10 @@
>  #include "igt_fb.h"
>  #include "ioctl_wrappers.h"
>  
> +#ifndef DRM_MODE_CONNECTOR_WRITEBACK
> +#define DRM_MODE_CONNECTOR_WRITEBACK   18
> +#endif
> +
>  /* Low-level helpers with kmstest_ prefix */
>  
>  /**
> @@ -120,6 +124,9 @@ enum igt_atomic_connector_properties {
>         IGT_CONNECTOR_CRTC_ID,
>         IGT_CONNECTOR_DPMS,
>         IGT_CONNECTOR_BROADCAST_RGB,
> +       IGT_CONNECTOR_WRITEBACK_PIXEL_FORMATS,
> +       IGT_CONNECTOR_WRITEBACK_FB_ID,
> +       IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR,
>         IGT_NUM_CONNECTOR_PROPS
>  };
>  
> @@ -345,6 +352,9 @@ typedef struct {
>  
>  	uint32_t props[IGT_NUM_CONNECTOR_PROPS];
>  	uint64_t values[IGT_NUM_CONNECTOR_PROPS];
> +
> +	struct igt_fb *writeback_fb;
I've spent a lot of effort into making sure we can set any property directly. Please don't add this struct member here,
there's no need for it.
> +	int32_t writeback_out_fence_fd;
>  } igt_output_t;
>  
>  struct igt_display {
> @@ -380,6 +390,10 @@ 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);
> +void igt_output_request_writeback_out_fence(igt_output_t *output);
> +int igt_output_get_last_writeback_out_fence(igt_output_t *output);
> +
>  igt_plane_t *igt_pipe_get_plane_type(igt_pipe_t *pipe, int plane_type);
>  
>  void igt_pipe_request_out_fence(igt_pipe_t *pipe);


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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [igt-dev] [PATCH i-g-t 2/6] kms_writeback: Add initial writeback tests
  2018-03-01 17:38   ` [igt-dev] " Liviu Dudau
@ 2018-06-25 12:48     ` Maarten Lankhorst
  -1 siblings, 0 replies; 27+ messages in thread
From: Maarten Lankhorst @ 2018-06-25 12:48 UTC (permalink / raw)
  To: Liviu Dudau, i-g-t; +Cc: Intel GFX ML

Op 01-03-18 om 18:38 schreef 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>
> ---
>  lib/igt_kms.c          |   7 +-
>  lib/igt_kms.h          |   8 ++
>  tests/Makefile.sources |   1 +
>  tests/kms_writeback.c  | 352 +++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/meson.build      |   1 +
>  5 files changed, 365 insertions(+), 4 deletions(-)
>  create mode 100644 tests/kms_writeback.c
>
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index 00d9d2e2..d558c1b8 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -2267,8 +2267,7 @@ static uint32_t igt_plane_get_fb_id(igt_plane_t *plane)
>  /*
>   * Add position and fb changes of a plane to the atomic property set
>   */
> -static void
> -igt_atomic_prepare_plane_commit(igt_plane_t *plane, igt_pipe_t *pipe,
> +void igt_atomic_prepare_plane_commit(igt_plane_t *plane, igt_pipe_t *pipe,
>  	drmModeAtomicReq *req)
>  {
>  	igt_display_t *display = pipe->display;
> @@ -2878,7 +2877,7 @@ igt_pipe_obj_replace_prop_blob(igt_pipe_t *pipe, enum igt_atomic_crtc_properties
>  /*
>   * Add crtc property changes to the atomic property set
>   */
> -static void igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicReq *req)
> +void igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicReq *req)
>  {
>  	int i;
>  
> @@ -2902,7 +2901,7 @@ static void igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicRe
>  /*
>   * Add connector property changes to the atomic property set
>   */
> -static void igt_atomic_prepare_connector_commit(igt_output_t *output, drmModeAtomicReq *req)
> +void igt_atomic_prepare_connector_commit(igt_output_t *output, drmModeAtomicReq *req)
>  {
>  
>  	int i;
> diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> index 59ccc4c6..f38fd0a0 100644
> --- a/lib/igt_kms.h
> +++ b/lib/igt_kms.h
> @@ -581,6 +581,12 @@ extern void igt_output_replace_prop_blob(igt_output_t *output,
>  					 enum igt_atomic_connector_properties prop,
>  					 const void *ptr, size_t length);
>  
> +void igt_atomic_prepare_plane_commit(igt_plane_t *plane, igt_pipe_t *pipe,
> +	drmModeAtomicReq *req);
> +
> +
> +void igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicReq *req);
> +
>  /**
>   * igt_pipe_obj_has_prop:
>   * @pipe: Pipe to check.
> @@ -670,6 +676,8 @@ extern void igt_pipe_obj_replace_prop_blob(igt_pipe_t *pipe,
>  
>  void igt_pipe_refresh(igt_display_t *display, enum pipe pipe, bool force);
>  
> +void igt_atomic_prepare_connector_commit(igt_output_t *output, drmModeAtomicReq *req);
> +
>  void igt_enable_connectors(void);
>  void igt_reset_connectors(void);
Please don't..

> diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> index 23f859be..9cfa474d 100644
> --- a/tests/Makefile.sources
> +++ b/tests/Makefile.sources
> @@ -212,6 +212,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 00000000..d922213d
> --- /dev/null
> +++ b/tests/kms_writeback.c
> @@ -0,0 +1,352 @@
> +/*
> + * (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"
> +
> +/* We need to define these ourselves until we get an updated libdrm */
> +#ifndef DRM_MODE_CONNECTOR_WRITEBACK
> +#define DRM_MODE_CONNECTOR_WRITEBACK   18
> +#endif
> +
> +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);

Could we perhaps update igt_output_is_connected to always return true when mode override
is set, so we don't have to force enable the connector?

> +	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)
> +{
> +	bool found;
> +	uint64_t check_fb_id;
> +
> +	found = kmstest_get_property(output->display->drm_fd, output->id,
> +				     DRM_MODE_OBJECT_CONNECTOR,
> +				     igt_connector_prop_names[IGT_CONNECTOR_WRITEBACK_FB_ID],
> +				     NULL, &check_fb_id, NULL);
> +	igt_assert(found && (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;
> +	enum pipe pipe;
> +	drmModeAtomicReq *req;
> +	igt_display_t *display = output->display;
> +	struct kmstest_connector_config *config = &output->config;
> +
> +	req = drmModeAtomicAlloc();
> +	drmModeAtomicSetCursor(req, 0);
> +
> +	for_each_pipe(display, pipe) {
> +		igt_pipe_t *pipe_obj = &display->pipes[pipe];
> +		igt_plane_t *plane;
> +
> +		/*
> +		 * Add CRTC Properties to the property set
> +		 */
> +		igt_atomic_prepare_crtc_commit(pipe_obj, req);
> +
> +		for_each_plane_on_pipe(display, pipe, plane) {
> +			igt_atomic_prepare_plane_commit(plane, pipe_obj, req);
> +		}
> +	}
> +
> +	igt_assert_lt(0, drmModeAtomicAddProperty(req, output->config.connector->connector_id,
> +						  output->props[IGT_CONNECTOR_CRTC_ID],
> +						  config->crtc->crtc_id));
igt_output_set_pipe() ?
Or failing that, igt_output_set_property(output, IGT_CONNECTOR_CRTC_ID);

> +	igt_assert_lt(0, drmModeAtomicAddProperty(req, output->config.connector->connector_id,
> +						  output->props[IGT_CONNECTOR_WRITEBACK_FB_ID],
> +						  fb_id));
igt_output_set_property(output, IGT_CONNECTOR_FB_ID); or the convenience function from v1.. which also saves you from having to do the call below.

I would really keep all the logic for the atomic properties internal, please don't override igt_display_commit_atomic.. we've made it too easy to set properties to anything you want.

:)

~Maarten
> +	igt_assert_lt(0, drmModeAtomicAddProperty(req, output->config.connector->connector_id,
> +						  output->props[IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR],
> +						  (uint64_t)out_fence_ptr));
> +
> +	if (ptr_valid)
> +		*out_fence_ptr = 0;
> +
> +	ret = drmModeAtomicCommit(display->drm_fd, req, flags, NULL);
> +
> +	if (ptr_valid && (ret || (flags & DRM_MODE_ATOMIC_TEST_ONLY)))
> +		igt_assert(*out_fence_ptr == -1);
> +
> +	drmModeAtomicFree(req);
> +
> +	/* 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_assert_fd(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 ABCGNRUXY";
> +		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 2a1e6f19..a08e782b 100644
> --- a/tests/meson.build
> +++ b/tests/meson.build
> @@ -188,6 +188,7 @@ test_progs = [
>  	'kms_tv_load_detect',
>  	'kms_universal_plane',
>  	'kms_vblank',
> +	'kms_writeback',
>  	'meta_test',
>  	'perf',
>  	'pm_backlight',


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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 2/6] kms_writeback: Add initial writeback tests
@ 2018-06-25 12:48     ` Maarten Lankhorst
  0 siblings, 0 replies; 27+ messages in thread
From: Maarten Lankhorst @ 2018-06-25 12:48 UTC (permalink / raw)
  To: Liviu Dudau, i-g-t; +Cc: Intel GFX ML

Op 01-03-18 om 18:38 schreef 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>
> ---
>  lib/igt_kms.c          |   7 +-
>  lib/igt_kms.h          |   8 ++
>  tests/Makefile.sources |   1 +
>  tests/kms_writeback.c  | 352 +++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/meson.build      |   1 +
>  5 files changed, 365 insertions(+), 4 deletions(-)
>  create mode 100644 tests/kms_writeback.c
>
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index 00d9d2e2..d558c1b8 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -2267,8 +2267,7 @@ static uint32_t igt_plane_get_fb_id(igt_plane_t *plane)
>  /*
>   * Add position and fb changes of a plane to the atomic property set
>   */
> -static void
> -igt_atomic_prepare_plane_commit(igt_plane_t *plane, igt_pipe_t *pipe,
> +void igt_atomic_prepare_plane_commit(igt_plane_t *plane, igt_pipe_t *pipe,
>  	drmModeAtomicReq *req)
>  {
>  	igt_display_t *display = pipe->display;
> @@ -2878,7 +2877,7 @@ igt_pipe_obj_replace_prop_blob(igt_pipe_t *pipe, enum igt_atomic_crtc_properties
>  /*
>   * Add crtc property changes to the atomic property set
>   */
> -static void igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicReq *req)
> +void igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicReq *req)
>  {
>  	int i;
>  
> @@ -2902,7 +2901,7 @@ static void igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicRe
>  /*
>   * Add connector property changes to the atomic property set
>   */
> -static void igt_atomic_prepare_connector_commit(igt_output_t *output, drmModeAtomicReq *req)
> +void igt_atomic_prepare_connector_commit(igt_output_t *output, drmModeAtomicReq *req)
>  {
>  
>  	int i;
> diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> index 59ccc4c6..f38fd0a0 100644
> --- a/lib/igt_kms.h
> +++ b/lib/igt_kms.h
> @@ -581,6 +581,12 @@ extern void igt_output_replace_prop_blob(igt_output_t *output,
>  					 enum igt_atomic_connector_properties prop,
>  					 const void *ptr, size_t length);
>  
> +void igt_atomic_prepare_plane_commit(igt_plane_t *plane, igt_pipe_t *pipe,
> +	drmModeAtomicReq *req);
> +
> +
> +void igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicReq *req);
> +
>  /**
>   * igt_pipe_obj_has_prop:
>   * @pipe: Pipe to check.
> @@ -670,6 +676,8 @@ extern void igt_pipe_obj_replace_prop_blob(igt_pipe_t *pipe,
>  
>  void igt_pipe_refresh(igt_display_t *display, enum pipe pipe, bool force);
>  
> +void igt_atomic_prepare_connector_commit(igt_output_t *output, drmModeAtomicReq *req);
> +
>  void igt_enable_connectors(void);
>  void igt_reset_connectors(void);
Please don't..

> diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> index 23f859be..9cfa474d 100644
> --- a/tests/Makefile.sources
> +++ b/tests/Makefile.sources
> @@ -212,6 +212,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 00000000..d922213d
> --- /dev/null
> +++ b/tests/kms_writeback.c
> @@ -0,0 +1,352 @@
> +/*
> + * (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"
> +
> +/* We need to define these ourselves until we get an updated libdrm */
> +#ifndef DRM_MODE_CONNECTOR_WRITEBACK
> +#define DRM_MODE_CONNECTOR_WRITEBACK   18
> +#endif
> +
> +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);

Could we perhaps update igt_output_is_connected to always return true when mode override
is set, so we don't have to force enable the connector?

> +	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)
> +{
> +	bool found;
> +	uint64_t check_fb_id;
> +
> +	found = kmstest_get_property(output->display->drm_fd, output->id,
> +				     DRM_MODE_OBJECT_CONNECTOR,
> +				     igt_connector_prop_names[IGT_CONNECTOR_WRITEBACK_FB_ID],
> +				     NULL, &check_fb_id, NULL);
> +	igt_assert(found && (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;
> +	enum pipe pipe;
> +	drmModeAtomicReq *req;
> +	igt_display_t *display = output->display;
> +	struct kmstest_connector_config *config = &output->config;
> +
> +	req = drmModeAtomicAlloc();
> +	drmModeAtomicSetCursor(req, 0);
> +
> +	for_each_pipe(display, pipe) {
> +		igt_pipe_t *pipe_obj = &display->pipes[pipe];
> +		igt_plane_t *plane;
> +
> +		/*
> +		 * Add CRTC Properties to the property set
> +		 */
> +		igt_atomic_prepare_crtc_commit(pipe_obj, req);
> +
> +		for_each_plane_on_pipe(display, pipe, plane) {
> +			igt_atomic_prepare_plane_commit(plane, pipe_obj, req);
> +		}
> +	}
> +
> +	igt_assert_lt(0, drmModeAtomicAddProperty(req, output->config.connector->connector_id,
> +						  output->props[IGT_CONNECTOR_CRTC_ID],
> +						  config->crtc->crtc_id));
igt_output_set_pipe() ?
Or failing that, igt_output_set_property(output, IGT_CONNECTOR_CRTC_ID);

> +	igt_assert_lt(0, drmModeAtomicAddProperty(req, output->config.connector->connector_id,
> +						  output->props[IGT_CONNECTOR_WRITEBACK_FB_ID],
> +						  fb_id));
igt_output_set_property(output, IGT_CONNECTOR_FB_ID); or the convenience function from v1.. which also saves you from having to do the call below.

I would really keep all the logic for the atomic properties internal, please don't override igt_display_commit_atomic.. we've made it too easy to set properties to anything you want.

:)

~Maarten
> +	igt_assert_lt(0, drmModeAtomicAddProperty(req, output->config.connector->connector_id,
> +						  output->props[IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR],
> +						  (uint64_t)out_fence_ptr));
> +
> +	if (ptr_valid)
> +		*out_fence_ptr = 0;
> +
> +	ret = drmModeAtomicCommit(display->drm_fd, req, flags, NULL);
> +
> +	if (ptr_valid && (ret || (flags & DRM_MODE_ATOMIC_TEST_ONLY)))
> +		igt_assert(*out_fence_ptr == -1);
> +
> +	drmModeAtomicFree(req);
> +
> +	/* 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_assert_fd(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 ABCGNRUXY";
> +		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 2a1e6f19..a08e782b 100644
> --- a/tests/meson.build
> +++ b/tests/meson.build
> @@ -188,6 +188,7 @@ test_progs = [
>  	'kms_tv_load_detect',
>  	'kms_universal_plane',
>  	'kms_vblank',
> +	'kms_writeback',
>  	'meta_test',
>  	'perf',
>  	'pm_backlight',


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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [igt-dev] [PATCH i-g-t 1/6] lib/igt_kms: Add writeback support in lib/
  2018-06-25 12:19     ` Maarten Lankhorst
@ 2018-06-26  8:28       ` Liviu Dudau
  -1 siblings, 0 replies; 27+ messages in thread
From: Liviu Dudau @ 2018-06-26  8:28 UTC (permalink / raw)
  To: Maarten Lankhorst; +Cc: i-g-t, Intel GFX ML, Rob Clark

Hi Maarten,

Thanks for reviewing this! As this patch is quite old, I agree that it
needs a refresh in order to catch up with the latest igt. I will take
your feedback and come up with an update in the near future.

Best regards,
Liviu

On Mon, Jun 25, 2018 at 02:19:46PM +0200, Maarten Lankhorst wrote:
> Op 01-03-18 om 18:38 schreef Liviu Dudau:
> > From: Brian Starkey <brian.starkey@arm.com>
> >
> > Add support in igt_kms for Writeback connectors, with the ability to
> > attach framebuffers and retrieve fences.
> >
> > Signed-off-by: Brian Starkey <brian.starkey@arm.com>
> > ---
> >  lib/igt_kms.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  lib/igt_kms.h | 14 ++++++++++++
> >  2 files changed, 85 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> > index 022abfe7..00d9d2e2 100644
> > --- a/lib/igt_kms.c
> > +++ b/lib/igt_kms.c
> > @@ -190,7 +190,10 @@ const char *igt_connector_prop_names[IGT_NUM_CONNECTOR_PROPS] = {
> >  	"scaling mode",
> >  	"CRTC_ID",
> >  	"DPMS",
> > -	"Broadcast RGB"
> > +	"Broadcast RGB",
> > +	"WRITEBACK_PIXEL_FORMATS",
> > +	"WRITEBACK_FB_ID",
> > +	"WRITEBACK_OUT_FENCE_PTR"
> >  };
> >  
> >  /*
> > @@ -452,6 +455,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" },
> >  	{}
> >  };
> >  
> > @@ -1947,6 +1951,7 @@ void igt_display_init(igt_display_t *display, int drm_fd)
> >  		output->force_reprobe = true;
> >  		output->id = resources->connectors[i];
> >  		output->display = display;
> > +		output->writeback_out_fence_fd = -1;
> >  
> >  		igt_output_refresh(output);
> >  	}
> > @@ -2040,6 +2045,43 @@ igt_output_t *igt_output_from_connector(igt_display_t *display,
> >  	return found;
> >  }
> >  
> > +void igt_output_set_writeback_fb(igt_output_t *output, struct igt_fb *fb)
> > +{
> > +	igt_display_t *display = output->display;
> > +	struct kmstest_connector_config *config = &output->config;
> > +
> > +	if (config->connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK)
> > +		return;
> 
> igt_output_set_prop_value(output, IGT_CONNECTOR_WRITEBACK_FB_ID, ...);
> 
> As a bonus you don't need to check for WRITEBACK type, because this property will be undefined and commit will fail.
> 
> > +	LOG(display, "%s: output_set_writeback_fb(%d)\n", output->name,
> > +	    fb ? fb->fb_id : 0);
> > +
> > +	output->writeback_fb = fb;
> > +}
> Probably want to merge this with igt_output_request_writeback_out_fence(), else there's no point for the convenience function. :)
> 
> > +static void igt_output_reset_writeback_out_fence(igt_output_t *output)
> > +{
> > +	if (output->writeback_out_fence_fd >= 0) {
> > +		close(output->writeback_out_fence_fd);
> > +		output->writeback_out_fence_fd = -1;
> > +	}
> > +}
> > +
> > +void igt_output_request_writeback_out_fence(igt_output_t *output)
> > +{
> > +	igt_output_reset_writeback_out_fence(output);
> > +	igt_output_set_prop_value(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR,
> > +				  (ptrdiff_t)&output->writeback_out_fence_fd);
> > +}
> > +
> > +int igt_output_get_last_writeback_out_fence(igt_output_t *output)
> > +{
> > +	int fd = output->writeback_out_fence_fd;
> > +	output->writeback_out_fence_fd = -1;
> > +
> > +	return fd;
> > +}
> Please handle this the same we do for pipe_obj->out_fence_fd.. No need to do it differently.
> > +
> >  static void igt_pipe_fini(igt_pipe_t *pipe)
> >  {
> >  	int i;
> > @@ -2063,6 +2105,8 @@ static void igt_pipe_fini(igt_pipe_t *pipe)
> >  static void igt_output_fini(igt_output_t *output)
> >  {
> >  	kmstest_free_connector_config(&output->config);
> > +	if (output->writeback_out_fence_fd >= 0)
> > +		close(output->writeback_out_fence_fd);
> >  	free(output->name);
> >  	output->name = NULL;
> >  }
> > @@ -2879,6 +2923,31 @@ static void igt_atomic_prepare_connector_commit(igt_output_t *output, drmModeAto
> >  					  output->props[i],
> >  					  output->values[i]));
> >  	}
> > +
> > +	if (output->writeback_fb)
> > +		output->writeback_fb = NULL;
> > +
> > +	igt_output_reset_writeback_out_fence(output);
> > +}
> > +
> > +static void handle_writeback_out_fences(igt_display_t *display, uint32_t flags, int ret)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < display->n_outputs; i++) {
> > +		igt_output_t *output = &display->outputs[i];
> > +
> > +		if (!output->config.connector)
> > +			continue;
> > +
> > +		if (!igt_output_is_prop_changed(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR))
> > +			continue;
> > +
> > +		igt_output_clear_prop_changed(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR);
> > +
> > +		if (ret || (flags & DRM_MODE_ATOMIC_TEST_ONLY))
> > +			igt_assert(output->writeback_out_fence_fd == -1);
> > +	}
> >  }
> >  
> >  /*
> > @@ -2929,6 +2998,7 @@ static int igt_atomic_commit(igt_display_t *display, uint32_t flags, void *user_
> >  	}
> >  
> >  	ret = drmModeAtomicCommit(display->drm_fd, req, flags, user_data);
> > +	handle_writeback_out_fences(display, flags, ret);
> >  
> >  	drmModeAtomicFree(req);
> >  	return ret;
> > diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> > index 1c46186e..59ccc4c6 100644
> > --- a/lib/igt_kms.h
> > +++ b/lib/igt_kms.h
> > @@ -38,6 +38,10 @@
> >  #include "igt_fb.h"
> >  #include "ioctl_wrappers.h"
> >  
> > +#ifndef DRM_MODE_CONNECTOR_WRITEBACK
> > +#define DRM_MODE_CONNECTOR_WRITEBACK   18
> > +#endif
> > +
> >  /* Low-level helpers with kmstest_ prefix */
> >  
> >  /**
> > @@ -120,6 +124,9 @@ enum igt_atomic_connector_properties {
> >         IGT_CONNECTOR_CRTC_ID,
> >         IGT_CONNECTOR_DPMS,
> >         IGT_CONNECTOR_BROADCAST_RGB,
> > +       IGT_CONNECTOR_WRITEBACK_PIXEL_FORMATS,
> > +       IGT_CONNECTOR_WRITEBACK_FB_ID,
> > +       IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR,
> >         IGT_NUM_CONNECTOR_PROPS
> >  };
> >  
> > @@ -345,6 +352,9 @@ typedef struct {
> >  
> >  	uint32_t props[IGT_NUM_CONNECTOR_PROPS];
> >  	uint64_t values[IGT_NUM_CONNECTOR_PROPS];
> > +
> > +	struct igt_fb *writeback_fb;
> I've spent a lot of effort into making sure we can set any property directly. Please don't add this struct member here,
> there's no need for it.
> > +	int32_t writeback_out_fence_fd;
> >  } igt_output_t;
> >  
> >  struct igt_display {
> > @@ -380,6 +390,10 @@ 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);
> > +void igt_output_request_writeback_out_fence(igt_output_t *output);
> > +int igt_output_get_last_writeback_out_fence(igt_output_t *output);
> > +
> >  igt_plane_t *igt_pipe_get_plane_type(igt_pipe_t *pipe, int plane_type);
> >  
> >  void igt_pipe_request_out_fence(igt_pipe_t *pipe);
> 
> 

-- 
====================
| 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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [igt-dev] [PATCH i-g-t 1/6] lib/igt_kms: Add writeback support in lib/
@ 2018-06-26  8:28       ` Liviu Dudau
  0 siblings, 0 replies; 27+ messages in thread
From: Liviu Dudau @ 2018-06-26  8:28 UTC (permalink / raw)
  To: Maarten Lankhorst; +Cc: i-g-t, Intel GFX ML, Rob Clark, Brian Starkey

Hi Maarten,

Thanks for reviewing this! As this patch is quite old, I agree that it
needs a refresh in order to catch up with the latest igt. I will take
your feedback and come up with an update in the near future.

Best regards,
Liviu

On Mon, Jun 25, 2018 at 02:19:46PM +0200, Maarten Lankhorst wrote:
> Op 01-03-18 om 18:38 schreef Liviu Dudau:
> > From: Brian Starkey <brian.starkey@arm.com>
> >
> > Add support in igt_kms for Writeback connectors, with the ability to
> > attach framebuffers and retrieve fences.
> >
> > Signed-off-by: Brian Starkey <brian.starkey@arm.com>
> > ---
> >  lib/igt_kms.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  lib/igt_kms.h | 14 ++++++++++++
> >  2 files changed, 85 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> > index 022abfe7..00d9d2e2 100644
> > --- a/lib/igt_kms.c
> > +++ b/lib/igt_kms.c
> > @@ -190,7 +190,10 @@ const char *igt_connector_prop_names[IGT_NUM_CONNECTOR_PROPS] = {
> >  	"scaling mode",
> >  	"CRTC_ID",
> >  	"DPMS",
> > -	"Broadcast RGB"
> > +	"Broadcast RGB",
> > +	"WRITEBACK_PIXEL_FORMATS",
> > +	"WRITEBACK_FB_ID",
> > +	"WRITEBACK_OUT_FENCE_PTR"
> >  };
> >  
> >  /*
> > @@ -452,6 +455,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" },
> >  	{}
> >  };
> >  
> > @@ -1947,6 +1951,7 @@ void igt_display_init(igt_display_t *display, int drm_fd)
> >  		output->force_reprobe = true;
> >  		output->id = resources->connectors[i];
> >  		output->display = display;
> > +		output->writeback_out_fence_fd = -1;
> >  
> >  		igt_output_refresh(output);
> >  	}
> > @@ -2040,6 +2045,43 @@ igt_output_t *igt_output_from_connector(igt_display_t *display,
> >  	return found;
> >  }
> >  
> > +void igt_output_set_writeback_fb(igt_output_t *output, struct igt_fb *fb)
> > +{
> > +	igt_display_t *display = output->display;
> > +	struct kmstest_connector_config *config = &output->config;
> > +
> > +	if (config->connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK)
> > +		return;
> 
> igt_output_set_prop_value(output, IGT_CONNECTOR_WRITEBACK_FB_ID, ...);
> 
> As a bonus you don't need to check for WRITEBACK type, because this property will be undefined and commit will fail.
> 
> > +	LOG(display, "%s: output_set_writeback_fb(%d)\n", output->name,
> > +	    fb ? fb->fb_id : 0);
> > +
> > +	output->writeback_fb = fb;
> > +}
> Probably want to merge this with igt_output_request_writeback_out_fence(), else there's no point for the convenience function. :)
> 
> > +static void igt_output_reset_writeback_out_fence(igt_output_t *output)
> > +{
> > +	if (output->writeback_out_fence_fd >= 0) {
> > +		close(output->writeback_out_fence_fd);
> > +		output->writeback_out_fence_fd = -1;
> > +	}
> > +}
> > +
> > +void igt_output_request_writeback_out_fence(igt_output_t *output)
> > +{
> > +	igt_output_reset_writeback_out_fence(output);
> > +	igt_output_set_prop_value(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR,
> > +				  (ptrdiff_t)&output->writeback_out_fence_fd);
> > +}
> > +
> > +int igt_output_get_last_writeback_out_fence(igt_output_t *output)
> > +{
> > +	int fd = output->writeback_out_fence_fd;
> > +	output->writeback_out_fence_fd = -1;
> > +
> > +	return fd;
> > +}
> Please handle this the same we do for pipe_obj->out_fence_fd.. No need to do it differently.
> > +
> >  static void igt_pipe_fini(igt_pipe_t *pipe)
> >  {
> >  	int i;
> > @@ -2063,6 +2105,8 @@ static void igt_pipe_fini(igt_pipe_t *pipe)
> >  static void igt_output_fini(igt_output_t *output)
> >  {
> >  	kmstest_free_connector_config(&output->config);
> > +	if (output->writeback_out_fence_fd >= 0)
> > +		close(output->writeback_out_fence_fd);
> >  	free(output->name);
> >  	output->name = NULL;
> >  }
> > @@ -2879,6 +2923,31 @@ static void igt_atomic_prepare_connector_commit(igt_output_t *output, drmModeAto
> >  					  output->props[i],
> >  					  output->values[i]));
> >  	}
> > +
> > +	if (output->writeback_fb)
> > +		output->writeback_fb = NULL;
> > +
> > +	igt_output_reset_writeback_out_fence(output);
> > +}
> > +
> > +static void handle_writeback_out_fences(igt_display_t *display, uint32_t flags, int ret)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < display->n_outputs; i++) {
> > +		igt_output_t *output = &display->outputs[i];
> > +
> > +		if (!output->config.connector)
> > +			continue;
> > +
> > +		if (!igt_output_is_prop_changed(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR))
> > +			continue;
> > +
> > +		igt_output_clear_prop_changed(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR);
> > +
> > +		if (ret || (flags & DRM_MODE_ATOMIC_TEST_ONLY))
> > +			igt_assert(output->writeback_out_fence_fd == -1);
> > +	}
> >  }
> >  
> >  /*
> > @@ -2929,6 +2998,7 @@ static int igt_atomic_commit(igt_display_t *display, uint32_t flags, void *user_
> >  	}
> >  
> >  	ret = drmModeAtomicCommit(display->drm_fd, req, flags, user_data);
> > +	handle_writeback_out_fences(display, flags, ret);
> >  
> >  	drmModeAtomicFree(req);
> >  	return ret;
> > diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> > index 1c46186e..59ccc4c6 100644
> > --- a/lib/igt_kms.h
> > +++ b/lib/igt_kms.h
> > @@ -38,6 +38,10 @@
> >  #include "igt_fb.h"
> >  #include "ioctl_wrappers.h"
> >  
> > +#ifndef DRM_MODE_CONNECTOR_WRITEBACK
> > +#define DRM_MODE_CONNECTOR_WRITEBACK   18
> > +#endif
> > +
> >  /* Low-level helpers with kmstest_ prefix */
> >  
> >  /**
> > @@ -120,6 +124,9 @@ enum igt_atomic_connector_properties {
> >         IGT_CONNECTOR_CRTC_ID,
> >         IGT_CONNECTOR_DPMS,
> >         IGT_CONNECTOR_BROADCAST_RGB,
> > +       IGT_CONNECTOR_WRITEBACK_PIXEL_FORMATS,
> > +       IGT_CONNECTOR_WRITEBACK_FB_ID,
> > +       IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR,
> >         IGT_NUM_CONNECTOR_PROPS
> >  };
> >  
> > @@ -345,6 +352,9 @@ typedef struct {
> >  
> >  	uint32_t props[IGT_NUM_CONNECTOR_PROPS];
> >  	uint64_t values[IGT_NUM_CONNECTOR_PROPS];
> > +
> > +	struct igt_fb *writeback_fb;
> I've spent a lot of effort into making sure we can set any property directly. Please don't add this struct member here,
> there's no need for it.
> > +	int32_t writeback_out_fence_fd;
> >  } igt_output_t;
> >  
> >  struct igt_display {
> > @@ -380,6 +390,10 @@ 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);
> > +void igt_output_request_writeback_out_fence(igt_output_t *output);
> > +int igt_output_get_last_writeback_out_fence(igt_output_t *output);
> > +
> >  igt_plane_t *igt_pipe_get_plane_type(igt_pipe_t *pipe, int plane_type);
> >  
> >  void igt_pipe_request_out_fence(igt_pipe_t *pipe);
> 
> 

-- 
====================
| 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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [igt-dev] [PATCH i-g-t 2/6] kms_writeback: Add initial writeback tests
  2018-06-25 12:48     ` [Intel-gfx] " Maarten Lankhorst
@ 2018-06-26  8:38       ` Liviu Dudau
  -1 siblings, 0 replies; 27+ messages in thread
From: Liviu Dudau @ 2018-06-26  8:38 UTC (permalink / raw)
  To: Maarten Lankhorst; +Cc: i-g-t, Intel GFX ML

On Mon, Jun 25, 2018 at 02:48:47PM +0200, Maarten Lankhorst wrote:
> Op 01-03-18 om 18:38 schreef 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>
> > ---
> >  lib/igt_kms.c          |   7 +-
> >  lib/igt_kms.h          |   8 ++
> >  tests/Makefile.sources |   1 +
> >  tests/kms_writeback.c  | 352 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  tests/meson.build      |   1 +
> >  5 files changed, 365 insertions(+), 4 deletions(-)
> >  create mode 100644 tests/kms_writeback.c
> >
> > diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> > index 00d9d2e2..d558c1b8 100644
> > --- a/lib/igt_kms.c
> > +++ b/lib/igt_kms.c
> > @@ -2267,8 +2267,7 @@ static uint32_t igt_plane_get_fb_id(igt_plane_t *plane)
> >  /*
> >   * Add position and fb changes of a plane to the atomic property set
> >   */
> > -static void
> > -igt_atomic_prepare_plane_commit(igt_plane_t *plane, igt_pipe_t *pipe,
> > +void igt_atomic_prepare_plane_commit(igt_plane_t *plane, igt_pipe_t *pipe,
> >  	drmModeAtomicReq *req)
> >  {
> >  	igt_display_t *display = pipe->display;
> > @@ -2878,7 +2877,7 @@ igt_pipe_obj_replace_prop_blob(igt_pipe_t *pipe, enum igt_atomic_crtc_properties
> >  /*
> >   * Add crtc property changes to the atomic property set
> >   */
> > -static void igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicReq *req)
> > +void igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicReq *req)
> >  {
> >  	int i;
> >  
> > @@ -2902,7 +2901,7 @@ static void igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicRe
> >  /*
> >   * Add connector property changes to the atomic property set
> >   */
> > -static void igt_atomic_prepare_connector_commit(igt_output_t *output, drmModeAtomicReq *req)
> > +void igt_atomic_prepare_connector_commit(igt_output_t *output, drmModeAtomicReq *req)
> >  {
> >  
> >  	int i;
> > diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> > index 59ccc4c6..f38fd0a0 100644
> > --- a/lib/igt_kms.h
> > +++ b/lib/igt_kms.h
> > @@ -581,6 +581,12 @@ extern void igt_output_replace_prop_blob(igt_output_t *output,
> >  					 enum igt_atomic_connector_properties prop,
> >  					 const void *ptr, size_t length);
> >  
> > +void igt_atomic_prepare_plane_commit(igt_plane_t *plane, igt_pipe_t *pipe,
> > +	drmModeAtomicReq *req);
> > +
> > +
> > +void igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicReq *req);
> > +
> >  /**
> >   * igt_pipe_obj_has_prop:
> >   * @pipe: Pipe to check.
> > @@ -670,6 +676,8 @@ extern void igt_pipe_obj_replace_prop_blob(igt_pipe_t *pipe,
> >  
> >  void igt_pipe_refresh(igt_display_t *display, enum pipe pipe, bool force);
> >  
> > +void igt_atomic_prepare_connector_commit(igt_output_t *output, drmModeAtomicReq *req);
> > +
> >  void igt_enable_connectors(void);
> >  void igt_reset_connectors(void);
> Please don't..

More context on what I should not do/say/write would be helpful.

> 
> > diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> > index 23f859be..9cfa474d 100644
> > --- a/tests/Makefile.sources
> > +++ b/tests/Makefile.sources
> > @@ -212,6 +212,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 00000000..d922213d
> > --- /dev/null
> > +++ b/tests/kms_writeback.c
> > @@ -0,0 +1,352 @@
> > +/*
> > + * (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"
> > +
> > +/* We need to define these ourselves until we get an updated libdrm */
> > +#ifndef DRM_MODE_CONNECTOR_WRITEBACK
> > +#define DRM_MODE_CONNECTOR_WRITEBACK   18
> > +#endif
> > +
> > +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);
> 
> Could we perhaps update igt_output_is_connected to always return true when mode override
> is set, so we don't have to force enable the connector?

Yes, that is now obsolete as we have a client cap as well that will hide
the connector unless set, so this patch needs updating as well.

> 
> > +	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)
> > +{
> > +	bool found;
> > +	uint64_t check_fb_id;
> > +
> > +	found = kmstest_get_property(output->display->drm_fd, output->id,
> > +				     DRM_MODE_OBJECT_CONNECTOR,
> > +				     igt_connector_prop_names[IGT_CONNECTOR_WRITEBACK_FB_ID],
> > +				     NULL, &check_fb_id, NULL);
> > +	igt_assert(found && (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;
> > +	enum pipe pipe;
> > +	drmModeAtomicReq *req;
> > +	igt_display_t *display = output->display;
> > +	struct kmstest_connector_config *config = &output->config;
> > +
> > +	req = drmModeAtomicAlloc();
> > +	drmModeAtomicSetCursor(req, 0);
> > +
> > +	for_each_pipe(display, pipe) {
> > +		igt_pipe_t *pipe_obj = &display->pipes[pipe];
> > +		igt_plane_t *plane;
> > +
> > +		/*
> > +		 * Add CRTC Properties to the property set
> > +		 */
> > +		igt_atomic_prepare_crtc_commit(pipe_obj, req);
> > +
> > +		for_each_plane_on_pipe(display, pipe, plane) {
> > +			igt_atomic_prepare_plane_commit(plane, pipe_obj, req);
> > +		}
> > +	}
> > +
> > +	igt_assert_lt(0, drmModeAtomicAddProperty(req, output->config.connector->connector_id,
> > +						  output->props[IGT_CONNECTOR_CRTC_ID],
> > +						  config->crtc->crtc_id));
> igt_output_set_pipe() ?
> Or failing that, igt_output_set_property(output, IGT_CONNECTOR_CRTC_ID);

Thanks for the hint, will update the patch.

> 
> > +	igt_assert_lt(0, drmModeAtomicAddProperty(req, output->config.connector->connector_id,
> > +						  output->props[IGT_CONNECTOR_WRITEBACK_FB_ID],
> > +						  fb_id));
> igt_output_set_property(output, IGT_CONNECTOR_FB_ID); or the convenience function from v1.. which also saves you from having to do the call below.
> 
> I would really keep all the logic for the atomic properties internal, please don't override igt_display_commit_atomic.. we've made it too easy to set properties to anything you want.

Understood. On the other hand, some flexibility in the test framework
for whacky values in order to do adversarial testing could be useful.

Thanks for reviewing this!

Best regards,
Liviu

> 
> :)
> 
> ~Maarten
> > +	igt_assert_lt(0, drmModeAtomicAddProperty(req, output->config.connector->connector_id,
> > +						  output->props[IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR],
> > +						  (uint64_t)out_fence_ptr));
> > +
> > +	if (ptr_valid)
> > +		*out_fence_ptr = 0;
> > +
> > +	ret = drmModeAtomicCommit(display->drm_fd, req, flags, NULL);
> > +
> > +	if (ptr_valid && (ret || (flags & DRM_MODE_ATOMIC_TEST_ONLY)))
> > +		igt_assert(*out_fence_ptr == -1);
> > +
> > +	drmModeAtomicFree(req);
> > +
> > +	/* 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_assert_fd(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 ABCGNRUXY";
> > +		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 2a1e6f19..a08e782b 100644
> > --- a/tests/meson.build
> > +++ b/tests/meson.build
> > @@ -188,6 +188,7 @@ test_progs = [
> >  	'kms_tv_load_detect',
> >  	'kms_universal_plane',
> >  	'kms_vblank',
> > +	'kms_writeback',
> >  	'meta_test',
> >  	'perf',
> >  	'pm_backlight',
> 
> 

-- 
====================
| 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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [igt-dev] [PATCH i-g-t 2/6] kms_writeback: Add initial writeback tests
@ 2018-06-26  8:38       ` Liviu Dudau
  0 siblings, 0 replies; 27+ messages in thread
From: Liviu Dudau @ 2018-06-26  8:38 UTC (permalink / raw)
  To: Maarten Lankhorst; +Cc: i-g-t, Intel GFX ML, Rob Clark, Brian Starkey

On Mon, Jun 25, 2018 at 02:48:47PM +0200, Maarten Lankhorst wrote:
> Op 01-03-18 om 18:38 schreef 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>
> > ---
> >  lib/igt_kms.c          |   7 +-
> >  lib/igt_kms.h          |   8 ++
> >  tests/Makefile.sources |   1 +
> >  tests/kms_writeback.c  | 352 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  tests/meson.build      |   1 +
> >  5 files changed, 365 insertions(+), 4 deletions(-)
> >  create mode 100644 tests/kms_writeback.c
> >
> > diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> > index 00d9d2e2..d558c1b8 100644
> > --- a/lib/igt_kms.c
> > +++ b/lib/igt_kms.c
> > @@ -2267,8 +2267,7 @@ static uint32_t igt_plane_get_fb_id(igt_plane_t *plane)
> >  /*
> >   * Add position and fb changes of a plane to the atomic property set
> >   */
> > -static void
> > -igt_atomic_prepare_plane_commit(igt_plane_t *plane, igt_pipe_t *pipe,
> > +void igt_atomic_prepare_plane_commit(igt_plane_t *plane, igt_pipe_t *pipe,
> >  	drmModeAtomicReq *req)
> >  {
> >  	igt_display_t *display = pipe->display;
> > @@ -2878,7 +2877,7 @@ igt_pipe_obj_replace_prop_blob(igt_pipe_t *pipe, enum igt_atomic_crtc_properties
> >  /*
> >   * Add crtc property changes to the atomic property set
> >   */
> > -static void igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicReq *req)
> > +void igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicReq *req)
> >  {
> >  	int i;
> >  
> > @@ -2902,7 +2901,7 @@ static void igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicRe
> >  /*
> >   * Add connector property changes to the atomic property set
> >   */
> > -static void igt_atomic_prepare_connector_commit(igt_output_t *output, drmModeAtomicReq *req)
> > +void igt_atomic_prepare_connector_commit(igt_output_t *output, drmModeAtomicReq *req)
> >  {
> >  
> >  	int i;
> > diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> > index 59ccc4c6..f38fd0a0 100644
> > --- a/lib/igt_kms.h
> > +++ b/lib/igt_kms.h
> > @@ -581,6 +581,12 @@ extern void igt_output_replace_prop_blob(igt_output_t *output,
> >  					 enum igt_atomic_connector_properties prop,
> >  					 const void *ptr, size_t length);
> >  
> > +void igt_atomic_prepare_plane_commit(igt_plane_t *plane, igt_pipe_t *pipe,
> > +	drmModeAtomicReq *req);
> > +
> > +
> > +void igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicReq *req);
> > +
> >  /**
> >   * igt_pipe_obj_has_prop:
> >   * @pipe: Pipe to check.
> > @@ -670,6 +676,8 @@ extern void igt_pipe_obj_replace_prop_blob(igt_pipe_t *pipe,
> >  
> >  void igt_pipe_refresh(igt_display_t *display, enum pipe pipe, bool force);
> >  
> > +void igt_atomic_prepare_connector_commit(igt_output_t *output, drmModeAtomicReq *req);
> > +
> >  void igt_enable_connectors(void);
> >  void igt_reset_connectors(void);
> Please don't..

More context on what I should not do/say/write would be helpful.

> 
> > diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> > index 23f859be..9cfa474d 100644
> > --- a/tests/Makefile.sources
> > +++ b/tests/Makefile.sources
> > @@ -212,6 +212,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 00000000..d922213d
> > --- /dev/null
> > +++ b/tests/kms_writeback.c
> > @@ -0,0 +1,352 @@
> > +/*
> > + * (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"
> > +
> > +/* We need to define these ourselves until we get an updated libdrm */
> > +#ifndef DRM_MODE_CONNECTOR_WRITEBACK
> > +#define DRM_MODE_CONNECTOR_WRITEBACK   18
> > +#endif
> > +
> > +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);
> 
> Could we perhaps update igt_output_is_connected to always return true when mode override
> is set, so we don't have to force enable the connector?

Yes, that is now obsolete as we have a client cap as well that will hide
the connector unless set, so this patch needs updating as well.

> 
> > +	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)
> > +{
> > +	bool found;
> > +	uint64_t check_fb_id;
> > +
> > +	found = kmstest_get_property(output->display->drm_fd, output->id,
> > +				     DRM_MODE_OBJECT_CONNECTOR,
> > +				     igt_connector_prop_names[IGT_CONNECTOR_WRITEBACK_FB_ID],
> > +				     NULL, &check_fb_id, NULL);
> > +	igt_assert(found && (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;
> > +	enum pipe pipe;
> > +	drmModeAtomicReq *req;
> > +	igt_display_t *display = output->display;
> > +	struct kmstest_connector_config *config = &output->config;
> > +
> > +	req = drmModeAtomicAlloc();
> > +	drmModeAtomicSetCursor(req, 0);
> > +
> > +	for_each_pipe(display, pipe) {
> > +		igt_pipe_t *pipe_obj = &display->pipes[pipe];
> > +		igt_plane_t *plane;
> > +
> > +		/*
> > +		 * Add CRTC Properties to the property set
> > +		 */
> > +		igt_atomic_prepare_crtc_commit(pipe_obj, req);
> > +
> > +		for_each_plane_on_pipe(display, pipe, plane) {
> > +			igt_atomic_prepare_plane_commit(plane, pipe_obj, req);
> > +		}
> > +	}
> > +
> > +	igt_assert_lt(0, drmModeAtomicAddProperty(req, output->config.connector->connector_id,
> > +						  output->props[IGT_CONNECTOR_CRTC_ID],
> > +						  config->crtc->crtc_id));
> igt_output_set_pipe() ?
> Or failing that, igt_output_set_property(output, IGT_CONNECTOR_CRTC_ID);

Thanks for the hint, will update the patch.

> 
> > +	igt_assert_lt(0, drmModeAtomicAddProperty(req, output->config.connector->connector_id,
> > +						  output->props[IGT_CONNECTOR_WRITEBACK_FB_ID],
> > +						  fb_id));
> igt_output_set_property(output, IGT_CONNECTOR_FB_ID); or the convenience function from v1.. which also saves you from having to do the call below.
> 
> I would really keep all the logic for the atomic properties internal, please don't override igt_display_commit_atomic.. we've made it too easy to set properties to anything you want.

Understood. On the other hand, some flexibility in the test framework
for whacky values in order to do adversarial testing could be useful.

Thanks for reviewing this!

Best regards,
Liviu

> 
> :)
> 
> ~Maarten
> > +	igt_assert_lt(0, drmModeAtomicAddProperty(req, output->config.connector->connector_id,
> > +						  output->props[IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR],
> > +						  (uint64_t)out_fence_ptr));
> > +
> > +	if (ptr_valid)
> > +		*out_fence_ptr = 0;
> > +
> > +	ret = drmModeAtomicCommit(display->drm_fd, req, flags, NULL);
> > +
> > +	if (ptr_valid && (ret || (flags & DRM_MODE_ATOMIC_TEST_ONLY)))
> > +		igt_assert(*out_fence_ptr == -1);
> > +
> > +	drmModeAtomicFree(req);
> > +
> > +	/* 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_assert_fd(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 ABCGNRUXY";
> > +		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 2a1e6f19..a08e782b 100644
> > --- a/tests/meson.build
> > +++ b/tests/meson.build
> > @@ -188,6 +188,7 @@ test_progs = [
> >  	'kms_tv_load_detect',
> >  	'kms_universal_plane',
> >  	'kms_vblank',
> > +	'kms_writeback',
> >  	'meta_test',
> >  	'perf',
> >  	'pm_backlight',
> 
> 

-- 
====================
| 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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [igt-dev] [PATCH i-g-t 2/6] kms_writeback: Add initial writeback tests
  2018-06-26  8:38       ` Liviu Dudau
@ 2018-06-26 12:44         ` Maarten Lankhorst
  -1 siblings, 0 replies; 27+ messages in thread
From: Maarten Lankhorst @ 2018-06-26 12:44 UTC (permalink / raw)
  To: Liviu Dudau; +Cc: i-g-t, Intel GFX ML

Op 26-06-18 om 10:38 schreef Liviu Dudau:
> On Mon, Jun 25, 2018 at 02:48:47PM +0200, Maarten Lankhorst wrote:
>> Op 01-03-18 om 18:38 schreef 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>
>>> ---
>>>  lib/igt_kms.c          |   7 +-
>>>  lib/igt_kms.h          |   8 ++
>>>  tests/Makefile.sources |   1 +
>>>  tests/kms_writeback.c  | 352 +++++++++++++++++++++++++++++++++++++++++++++++++
>>>  tests/meson.build      |   1 +
>>>  5 files changed, 365 insertions(+), 4 deletions(-)
>>>  create mode 100644 tests/kms_writeback.c
>>>
>>> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
>>> index 00d9d2e2..d558c1b8 100644
>>> --- a/lib/igt_kms.c
>>> +++ b/lib/igt_kms.c
>>> @@ -2267,8 +2267,7 @@ static uint32_t igt_plane_get_fb_id(igt_plane_t *plane)
>>>  /*
>>>   * Add position and fb changes of a plane to the atomic property set
>>>   */
>>> -static void
>>> -igt_atomic_prepare_plane_commit(igt_plane_t *plane, igt_pipe_t *pipe,
>>> +void igt_atomic_prepare_plane_commit(igt_plane_t *plane, igt_pipe_t *pipe,
>>>  	drmModeAtomicReq *req)
>>>  {
>>>  	igt_display_t *display = pipe->display;
>>> @@ -2878,7 +2877,7 @@ igt_pipe_obj_replace_prop_blob(igt_pipe_t *pipe, enum igt_atomic_crtc_properties
>>>  /*
>>>   * Add crtc property changes to the atomic property set
>>>   */
>>> -static void igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicReq *req)
>>> +void igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicReq *req)
>>>  {
>>>  	int i;
>>>  
>>> @@ -2902,7 +2901,7 @@ static void igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicRe
>>>  /*
>>>   * Add connector property changes to the atomic property set
>>>   */
>>> -static void igt_atomic_prepare_connector_commit(igt_output_t *output, drmModeAtomicReq *req)
>>> +void igt_atomic_prepare_connector_commit(igt_output_t *output, drmModeAtomicReq *req)
>>>  {
>>>  
>>>  	int i;
>>> diff --git a/lib/igt_kms.h b/lib/igt_kms.h
>>> index 59ccc4c6..f38fd0a0 100644
>>> --- a/lib/igt_kms.h
>>> +++ b/lib/igt_kms.h
>>> @@ -581,6 +581,12 @@ extern void igt_output_replace_prop_blob(igt_output_t *output,
>>>  					 enum igt_atomic_connector_properties prop,
>>>  					 const void *ptr, size_t length);
>>>  
>>> +void igt_atomic_prepare_plane_commit(igt_plane_t *plane, igt_pipe_t *pipe,
>>> +	drmModeAtomicReq *req);
>>> +
>>> +
>>> +void igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicReq *req);
>>> +
>>>  /**
>>>   * igt_pipe_obj_has_prop:
>>>   * @pipe: Pipe to check.
>>> @@ -670,6 +676,8 @@ extern void igt_pipe_obj_replace_prop_blob(igt_pipe_t *pipe,
>>>  
>>>  void igt_pipe_refresh(igt_display_t *display, enum pipe pipe, bool force);
>>>  
>>> +void igt_atomic_prepare_connector_commit(igt_output_t *output, drmModeAtomicReq *req);
>>> +
>>>  void igt_enable_connectors(void);
>>>  void igt_reset_connectors(void);
>> Please don't..
> More context on what I should not do/say/write would be helpful.
>
>>> diff --git a/tests/Makefile.sources b/tests/Makefile.sources
>>> index 23f859be..9cfa474d 100644
>>> --- a/tests/Makefile.sources
>>> +++ b/tests/Makefile.sources
>>> @@ -212,6 +212,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 00000000..d922213d
>>> --- /dev/null
>>> +++ b/tests/kms_writeback.c
>>> @@ -0,0 +1,352 @@
>>> +/*
>>> + * (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"
>>> +
>>> +/* We need to define these ourselves until we get an updated libdrm */
>>> +#ifndef DRM_MODE_CONNECTOR_WRITEBACK
>>> +#define DRM_MODE_CONNECTOR_WRITEBACK   18
>>> +#endif
>>> +
>>> +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);
>> Could we perhaps update igt_output_is_connected to always return true when mode override
>> is set, so we don't have to force enable the connector?
> Yes, that is now obsolete as we have a client cap as well that will hide
> the connector unless set, so this patch needs updating as well.
>
>>> +	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)
>>> +{
>>> +	bool found;
>>> +	uint64_t check_fb_id;
>>> +
>>> +	found = kmstest_get_property(output->display->drm_fd, output->id,
>>> +				     DRM_MODE_OBJECT_CONNECTOR,
>>> +				     igt_connector_prop_names[IGT_CONNECTOR_WRITEBACK_FB_ID],
>>> +				     NULL, &check_fb_id, NULL);
>>> +	igt_assert(found && (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;
>>> +	enum pipe pipe;
>>> +	drmModeAtomicReq *req;
>>> +	igt_display_t *display = output->display;
>>> +	struct kmstest_connector_config *config = &output->config;
>>> +
>>> +	req = drmModeAtomicAlloc();
>>> +	drmModeAtomicSetCursor(req, 0);
>>> +
>>> +	for_each_pipe(display, pipe) {
>>> +		igt_pipe_t *pipe_obj = &display->pipes[pipe];
>>> +		igt_plane_t *plane;
>>> +
>>> +		/*
>>> +		 * Add CRTC Properties to the property set
>>> +		 */
>>> +		igt_atomic_prepare_crtc_commit(pipe_obj, req);
>>> +
>>> +		for_each_plane_on_pipe(display, pipe, plane) {
>>> +			igt_atomic_prepare_plane_commit(plane, pipe_obj, req);
>>> +		}
>>> +	}
>>> +
>>> +	igt_assert_lt(0, drmModeAtomicAddProperty(req, output->config.connector->connector_id,
>>> +						  output->props[IGT_CONNECTOR_CRTC_ID],
>>> +						  config->crtc->crtc_id));
>> igt_output_set_pipe() ?
>> Or failing that, igt_output_set_property(output, IGT_CONNECTOR_CRTC_ID);
> Thanks for the hint, will update the patch.
>
>>> +	igt_assert_lt(0, drmModeAtomicAddProperty(req, output->config.connector->connector_id,
>>> +						  output->props[IGT_CONNECTOR_WRITEBACK_FB_ID],
>>> +						  fb_id));
>> igt_output_set_property(output, IGT_CONNECTOR_FB_ID); or the convenience function from v1.. which also saves you from having to do the call below.
>>
>> I would really keep all the logic for the atomic properties internal, please don't override igt_display_commit_atomic.. we've made it too easy to set properties to anything you want.
> Understood. On the other hand, some flexibility in the test framework
> for whacky values in order to do adversarial testing could be useful.
We do allow this explicitly, see kms_atomic, crtc_invalid_params tries setting an invalid mode:

	/* Pass a series of invalid object IDs for the mode ID. */
	igt_pipe_obj_set_prop_value(pipe, IGT_CRTC_MODE_ID, plane->drm_plane->plane_id);
	crtc_commit_atomic_err(pipe, plane, ATOMIC_RELAX_NONE, EINVAL);

	igt_pipe_obj_set_prop_value(pipe, IGT_CRTC_MODE_ID, pipe->crtc_id);
	crtc_commit_atomic_err(pipe, plane, ATOMIC_RELAX_NONE, EINVAL);

	igt_pipe_obj_set_prop_value(pipe, IGT_CRTC_MODE_ID, output->id);
	crtc_commit_atomic_err(pipe, plane, ATOMIC_RELAX_NONE, EINVAL);

	igt_pipe_obj_set_prop_value(pipe, IGT_CRTC_MODE_ID, fb->fb_id);
	crtc_commit_atomic_err(pipe, plane, ATOMIC_RELAX_NONE, EINVAL);

I couldn't put in a pointer to the kitchen sink, else I would have. O:)
Same works for plane and connector too, this is why you don't need to
override atomic commit(). 

You can put together an atomic test without ever using anything but
igt_plane_set_prop_value, igt_output_set_prop_value and igt_pipe(,_obj)_set_prop_value.

The convenience functions are just for allowing legacy support to continue working.

~Maarten

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [igt-dev] [PATCH i-g-t 2/6] kms_writeback: Add initial writeback tests
@ 2018-06-26 12:44         ` Maarten Lankhorst
  0 siblings, 0 replies; 27+ messages in thread
From: Maarten Lankhorst @ 2018-06-26 12:44 UTC (permalink / raw)
  To: Liviu Dudau; +Cc: i-g-t, Intel GFX ML, Rob Clark, Brian Starkey

Op 26-06-18 om 10:38 schreef Liviu Dudau:
> On Mon, Jun 25, 2018 at 02:48:47PM +0200, Maarten Lankhorst wrote:
>> Op 01-03-18 om 18:38 schreef 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>
>>> ---
>>>  lib/igt_kms.c          |   7 +-
>>>  lib/igt_kms.h          |   8 ++
>>>  tests/Makefile.sources |   1 +
>>>  tests/kms_writeback.c  | 352 +++++++++++++++++++++++++++++++++++++++++++++++++
>>>  tests/meson.build      |   1 +
>>>  5 files changed, 365 insertions(+), 4 deletions(-)
>>>  create mode 100644 tests/kms_writeback.c
>>>
>>> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
>>> index 00d9d2e2..d558c1b8 100644
>>> --- a/lib/igt_kms.c
>>> +++ b/lib/igt_kms.c
>>> @@ -2267,8 +2267,7 @@ static uint32_t igt_plane_get_fb_id(igt_plane_t *plane)
>>>  /*
>>>   * Add position and fb changes of a plane to the atomic property set
>>>   */
>>> -static void
>>> -igt_atomic_prepare_plane_commit(igt_plane_t *plane, igt_pipe_t *pipe,
>>> +void igt_atomic_prepare_plane_commit(igt_plane_t *plane, igt_pipe_t *pipe,
>>>  	drmModeAtomicReq *req)
>>>  {
>>>  	igt_display_t *display = pipe->display;
>>> @@ -2878,7 +2877,7 @@ igt_pipe_obj_replace_prop_blob(igt_pipe_t *pipe, enum igt_atomic_crtc_properties
>>>  /*
>>>   * Add crtc property changes to the atomic property set
>>>   */
>>> -static void igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicReq *req)
>>> +void igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicReq *req)
>>>  {
>>>  	int i;
>>>  
>>> @@ -2902,7 +2901,7 @@ static void igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicRe
>>>  /*
>>>   * Add connector property changes to the atomic property set
>>>   */
>>> -static void igt_atomic_prepare_connector_commit(igt_output_t *output, drmModeAtomicReq *req)
>>> +void igt_atomic_prepare_connector_commit(igt_output_t *output, drmModeAtomicReq *req)
>>>  {
>>>  
>>>  	int i;
>>> diff --git a/lib/igt_kms.h b/lib/igt_kms.h
>>> index 59ccc4c6..f38fd0a0 100644
>>> --- a/lib/igt_kms.h
>>> +++ b/lib/igt_kms.h
>>> @@ -581,6 +581,12 @@ extern void igt_output_replace_prop_blob(igt_output_t *output,
>>>  					 enum igt_atomic_connector_properties prop,
>>>  					 const void *ptr, size_t length);
>>>  
>>> +void igt_atomic_prepare_plane_commit(igt_plane_t *plane, igt_pipe_t *pipe,
>>> +	drmModeAtomicReq *req);
>>> +
>>> +
>>> +void igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicReq *req);
>>> +
>>>  /**
>>>   * igt_pipe_obj_has_prop:
>>>   * @pipe: Pipe to check.
>>> @@ -670,6 +676,8 @@ extern void igt_pipe_obj_replace_prop_blob(igt_pipe_t *pipe,
>>>  
>>>  void igt_pipe_refresh(igt_display_t *display, enum pipe pipe, bool force);
>>>  
>>> +void igt_atomic_prepare_connector_commit(igt_output_t *output, drmModeAtomicReq *req);
>>> +
>>>  void igt_enable_connectors(void);
>>>  void igt_reset_connectors(void);
>> Please don't..
> More context on what I should not do/say/write would be helpful.
>
>>> diff --git a/tests/Makefile.sources b/tests/Makefile.sources
>>> index 23f859be..9cfa474d 100644
>>> --- a/tests/Makefile.sources
>>> +++ b/tests/Makefile.sources
>>> @@ -212,6 +212,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 00000000..d922213d
>>> --- /dev/null
>>> +++ b/tests/kms_writeback.c
>>> @@ -0,0 +1,352 @@
>>> +/*
>>> + * (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"
>>> +
>>> +/* We need to define these ourselves until we get an updated libdrm */
>>> +#ifndef DRM_MODE_CONNECTOR_WRITEBACK
>>> +#define DRM_MODE_CONNECTOR_WRITEBACK   18
>>> +#endif
>>> +
>>> +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);
>> Could we perhaps update igt_output_is_connected to always return true when mode override
>> is set, so we don't have to force enable the connector?
> Yes, that is now obsolete as we have a client cap as well that will hide
> the connector unless set, so this patch needs updating as well.
>
>>> +	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)
>>> +{
>>> +	bool found;
>>> +	uint64_t check_fb_id;
>>> +
>>> +	found = kmstest_get_property(output->display->drm_fd, output->id,
>>> +				     DRM_MODE_OBJECT_CONNECTOR,
>>> +				     igt_connector_prop_names[IGT_CONNECTOR_WRITEBACK_FB_ID],
>>> +				     NULL, &check_fb_id, NULL);
>>> +	igt_assert(found && (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;
>>> +	enum pipe pipe;
>>> +	drmModeAtomicReq *req;
>>> +	igt_display_t *display = output->display;
>>> +	struct kmstest_connector_config *config = &output->config;
>>> +
>>> +	req = drmModeAtomicAlloc();
>>> +	drmModeAtomicSetCursor(req, 0);
>>> +
>>> +	for_each_pipe(display, pipe) {
>>> +		igt_pipe_t *pipe_obj = &display->pipes[pipe];
>>> +		igt_plane_t *plane;
>>> +
>>> +		/*
>>> +		 * Add CRTC Properties to the property set
>>> +		 */
>>> +		igt_atomic_prepare_crtc_commit(pipe_obj, req);
>>> +
>>> +		for_each_plane_on_pipe(display, pipe, plane) {
>>> +			igt_atomic_prepare_plane_commit(plane, pipe_obj, req);
>>> +		}
>>> +	}
>>> +
>>> +	igt_assert_lt(0, drmModeAtomicAddProperty(req, output->config.connector->connector_id,
>>> +						  output->props[IGT_CONNECTOR_CRTC_ID],
>>> +						  config->crtc->crtc_id));
>> igt_output_set_pipe() ?
>> Or failing that, igt_output_set_property(output, IGT_CONNECTOR_CRTC_ID);
> Thanks for the hint, will update the patch.
>
>>> +	igt_assert_lt(0, drmModeAtomicAddProperty(req, output->config.connector->connector_id,
>>> +						  output->props[IGT_CONNECTOR_WRITEBACK_FB_ID],
>>> +						  fb_id));
>> igt_output_set_property(output, IGT_CONNECTOR_FB_ID); or the convenience function from v1.. which also saves you from having to do the call below.
>>
>> I would really keep all the logic for the atomic properties internal, please don't override igt_display_commit_atomic.. we've made it too easy to set properties to anything you want.
> Understood. On the other hand, some flexibility in the test framework
> for whacky values in order to do adversarial testing could be useful.
We do allow this explicitly, see kms_atomic, crtc_invalid_params tries setting an invalid mode:

	/* Pass a series of invalid object IDs for the mode ID. */
	igt_pipe_obj_set_prop_value(pipe, IGT_CRTC_MODE_ID, plane->drm_plane->plane_id);
	crtc_commit_atomic_err(pipe, plane, ATOMIC_RELAX_NONE, EINVAL);

	igt_pipe_obj_set_prop_value(pipe, IGT_CRTC_MODE_ID, pipe->crtc_id);
	crtc_commit_atomic_err(pipe, plane, ATOMIC_RELAX_NONE, EINVAL);

	igt_pipe_obj_set_prop_value(pipe, IGT_CRTC_MODE_ID, output->id);
	crtc_commit_atomic_err(pipe, plane, ATOMIC_RELAX_NONE, EINVAL);

	igt_pipe_obj_set_prop_value(pipe, IGT_CRTC_MODE_ID, fb->fb_id);
	crtc_commit_atomic_err(pipe, plane, ATOMIC_RELAX_NONE, EINVAL);

I couldn't put in a pointer to the kitchen sink, else I would have. O:)
Same works for plane and connector too, this is why you don't need to
override atomic commit(). 

You can put together an atomic test without ever using anything but
igt_plane_set_prop_value, igt_output_set_prop_value and igt_pipe(,_obj)_set_prop_value.

The convenience functions are just for allowing legacy support to continue working.

~Maarten

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

end of thread, other threads:[~2018-06-26 12:44 UTC | newest]

Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-01 17:38 [PATCH i-g-t 0/6] igt: Add support for testing writeback connectors Liviu Dudau
2018-03-01 17:38 ` [Intel-gfx] " Liviu Dudau
2018-03-01 17:38 ` [PATCH i-g-t 1/6] lib/igt_kms: Add writeback support in lib/ Liviu Dudau
2018-03-01 17:38   ` [igt-dev] " Liviu Dudau
2018-06-25 12:19   ` Maarten Lankhorst
2018-06-25 12:19     ` Maarten Lankhorst
2018-06-26  8:28     ` Liviu Dudau
2018-06-26  8:28       ` Liviu Dudau
2018-03-01 17:38 ` [PATCH i-g-t 2/6] kms_writeback: Add initial writeback tests Liviu Dudau
2018-03-01 17:38   ` [igt-dev] " Liviu Dudau
2018-06-25 12:48   ` Maarten Lankhorst
2018-06-25 12:48     ` [Intel-gfx] " Maarten Lankhorst
2018-06-26  8:38     ` Liviu Dudau
2018-06-26  8:38       ` Liviu Dudau
2018-06-26 12:44       ` Maarten Lankhorst
2018-06-26 12:44         ` Maarten Lankhorst
2018-03-01 17:38 ` [PATCH i-g-t 3/6] lib: Add function to hash a framebuffer Liviu Dudau
2018-03-01 17:38   ` [igt-dev] " Liviu Dudau
2018-03-01 17:38 ` [PATCH i-g-t 4/6] kms_writeback: Add writeback-check-output Liviu Dudau
2018-03-01 17:38   ` [igt-dev] " Liviu Dudau
2018-03-01 17:38 ` [PATCH i-g-t 5/6] lib/igt_kms: Add igt_output_clone_pipe for cloning Liviu Dudau
2018-03-01 17:38   ` [Intel-gfx] " Liviu Dudau
2018-03-01 17:38 ` [PATCH i-g-t 6/6] kms_writeback: Add tests using a cloned output Liviu Dudau
2018-03-01 17:38   ` [igt-dev] " Liviu Dudau
2018-03-01 18:48 ` [igt-dev] ✗ Fi.CI.BAT: failure for igt: Add support for testing writeback connectors Patchwork
2018-03-02 19:58 ` [igt-dev] ✓ Fi.CI.BAT: success " Patchwork
2018-03-02 22:56 ` [igt-dev] ✗ Fi.CI.IGT: warning " Patchwork

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.