[PATCH 00/12] kms tests for the DRM fences interfaces

[PATCH 00/12] kms tests for the DRM fences interfaces

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Hi,

That is the first version of the igt tests for DRM fences[1]. The
first four patches are just fix/improvements on the kms_atomic
infrastructure.

These patches depends on Robert Foss tests for sw_sync and a branch
with those tests included can be seen here:

https://git.collabora.com/cgit/user/padovan/intel-gpu-tools.git/log/

Gustavo

---
[1] https://lkml.org/lkml/2016/11/13/324

Gustavo Padovan (11):
  tests/kms_atomic_transition: use select + read instead of blocking
    read
  tests/kms_atomic_transition: don't assume max pipes
  lib/igt_kms: move igt_kms_get_alt_edid() to the right place
  lib/igt_kms: export properties names
  tests/kms_atomic: use global atomic properties definitions
  lib/igt_kms: Add support for the OUT_FENCE_PTR property
  tests/kms_atomic: stress possible fence settings
  tests/kms_atomic_transition: add fencing parameter to
    run_transition_tests
  tests/kms_atomic_transition: add out_fences tests
  tests/kms_atomic_transition: add in_fences tests
  tests/kms_atomic_transition: set out_fence for all crtcs

Robert Foss (1):
  lib/igt_kms: Add support for the IN_FENCE_FD property

 lib/igt_kms.c                 | 102 +++++++++++++----
 lib/igt_kms.h                 |  13 +++
 tests/kms_atomic.c            | 247 ++++++++++++++++++++++++++----------------
 tests/kms_atomic_transition.c | 148 ++++++++++++++++++++++---
 4 files changed, 379 insertions(+), 131 deletions(-)

-- 
2.5.5

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH i-g-t 1/2] lib/drmtest: Fix igt_skip message

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Now other gpus are supported too.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 lib/drmtest.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/drmtest.c b/lib/drmtest.c
index 884fe7c..9f3ac7f 100644
--- a/lib/drmtest.c
+++ b/lib/drmtest.c
@@ -263,7 +263,7 @@ int __drm_open_driver(int chipset)
 		close(fd);
 	}
 
-	igt_skip("No intel gpu found\n");
+	igt_skip("No known gpu found\n");
 	return -1;
 }
 
-- 
2.5.5

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

[PATCH 01/12] tests/kms_atomic_transition: use select + read instead of blocking read

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

If the event never arrives we can timeout with select and end the test.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 tests/kms_atomic_transition.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tests/kms_atomic_transition.c b/tests/kms_atomic_transition.c
index 1977993..e693c88 100644
--- a/tests/kms_atomic_transition.c
+++ b/tests/kms_atomic_transition.c
@@ -296,6 +296,14 @@ static void commit_display(igt_display_t *display, unsigned event_mask, bool non
 		struct drm_event *e = (void *)buf;
 		struct drm_event_vblank *vblank = (void *)buf;
 		uint32_t crtc_id, pipe = I915_MAX_PIPES;
+		struct timeval timeout = { .tv_sec = 3, .tv_usec = 0 };
+		fd_set fds;
+
+		FD_ZERO(&fds);
+		FD_SET(0, &fds);
+		FD_SET(display->drm_fd, &fds);
+		ret = select(display->drm_fd + 1, &fds, NULL, NULL, &timeout);
+		igt_assert(ret > 0);
 
 		ret = read(display->drm_fd, buf, sizeof(buf));
 		if (ret < 0 && (errno == EINTR || errno == EAGAIN))
-- 
2.5.5

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH i-g-t 2/2] lib/drmtest: add virtio_gpu support

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Support the virtio GPU on drmtest.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 lib/drmtest.c | 9 +++++++++
 lib/drmtest.h | 1 +
 2 files changed, 10 insertions(+)

diff --git a/lib/drmtest.c b/lib/drmtest.c
index 9f3ac7f..b374006 100644
--- a/lib/drmtest.c
+++ b/lib/drmtest.c
@@ -115,6 +115,11 @@ static bool is_vgem_device(int fd)
 	return __is_device(fd, "vgem");
 }
 
+static bool is_virtio_device(int fd)
+{
+	return __is_device(fd, "virt");
+}
+
 static bool has_known_intel_chipset(int fd)
 {
 	struct drm_i915_getparam gp;
@@ -260,6 +265,10 @@ int __drm_open_driver(int chipset)
 		    is_vgem_device(fd))
 			return fd;
 
+		if (chipset & DRIVER_VIRTIO &&
+		    is_virtio_device(fd))
+			return fd;
+
 		close(fd);
 	}
 
diff --git a/lib/drmtest.h b/lib/drmtest.h
index 8ce32a6..19d4bd1 100644
--- a/lib/drmtest.h
+++ b/lib/drmtest.h
@@ -41,6 +41,7 @@
 #define DRIVER_INTEL	(1 << 0)
 #define DRIVER_VC4	(1 << 1)
 #define DRIVER_VGEM	(1 << 2)
+#define DRIVER_VIRTIO	(1 << 3)
 #define DRIVER_ANY 	~(DRIVER_VGEM)
 
 #ifdef ANDROID
-- 
2.5.5

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 02/12] tests/kms_atomic_transition: don't assume max pipes

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 tests/kms_atomic_transition.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/kms_atomic_transition.c b/tests/kms_atomic_transition.c
index e693c88..8b26b53 100644
--- a/tests/kms_atomic_transition.c
+++ b/tests/kms_atomic_transition.c
@@ -404,7 +404,7 @@ static void run_modeset_tests(igt_display_t *display, int howmany, bool nonblock
 {
 	struct igt_fb fbs[2];
 	int i, j;
-	unsigned iter_max = 1 << I915_MAX_PIPES;
+	unsigned iter_max = 1 << display->n_pipes;
 	igt_pipe_crc_t *pipe_crcs[I915_MAX_PIPES];
 	igt_output_t *output;
 	unsigned width = 0, height = 0;
-- 
2.5.5

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

[PATCH 03/12] lib/igt_kms: move igt_kms_get_alt_edid() to the right place

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 lib/igt_kms.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index 989704e..aa9fd16 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -153,23 +153,6 @@ const unsigned char* igt_kms_get_base_edid(void)
 #define EDID_NAME alt_edid
 #include "igt_edid_template.h"
 
-/**
- * igt_kms_get_alt_edid:
- *
- * Get an alternate edid block, which includes the following modes:
- *
- *  - 1400x1050 60Hz
- *  - 1920x1080 60Hz
- *  - 1280x720 60Hz
- *  - 1024x768 60Hz
- *  - 800x600 60Hz
- *  - 640x480 60Hz
- *
- * This can be extended with further features using functions such as
- * #kmstest_edid_add_3d.
- *
- * Returns: an alternate edid block
- */
 static const char *igt_plane_prop_names[IGT_NUM_PLANE_PROPS] = {
 	"SRC_X",
 	"SRC_Y",
@@ -297,6 +280,23 @@ igt_atomic_fill_pipe_props(igt_display_t *display, igt_pipe_t *pipe,
 	drmModeFreeObjectProperties(props);
 }
 
+/**
+ * igt_kms_get_alt_edid:
+ *
+ * Get an alternate edid block, which includes the following modes:
+ *
+ *  - 1400x1050 60Hz
+ *  - 1920x1080 60Hz
+ *  - 1280x720 60Hz
+ *  - 1024x768 60Hz
+ *  - 800x600 60Hz
+ *  - 640x480 60Hz
+ *
+ * This can be extended with further features using functions such as
+ * #kmstest_edid_add_3d.
+ *
+ * Returns: an alternate edid block
+ */
 const unsigned char* igt_kms_get_alt_edid(void)
 {
 	update_edid_csum(alt_edid);
-- 
2.5.5

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

[PATCH 04/12] lib/igt_kms: export properties names

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 lib/igt_kms.c | 6 +++---
 lib/igt_kms.h | 5 +++++
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index aa9fd16..8aaff5b 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -153,7 +153,7 @@ const unsigned char* igt_kms_get_base_edid(void)
 #define EDID_NAME alt_edid
 #include "igt_edid_template.h"
 
-static const char *igt_plane_prop_names[IGT_NUM_PLANE_PROPS] = {
+const char *igt_plane_prop_names[IGT_NUM_PLANE_PROPS] = {
 	"SRC_X",
 	"SRC_Y",
 	"SRC_W",
@@ -168,7 +168,7 @@ static const char *igt_plane_prop_names[IGT_NUM_PLANE_PROPS] = {
 	"rotation"
 };
 
-static const char *igt_crtc_prop_names[IGT_NUM_CRTC_PROPS] = {
+const char *igt_crtc_prop_names[IGT_NUM_CRTC_PROPS] = {
 	"background_color",
 	"CTM",
 	"DEGAMMA_LUT",
@@ -177,7 +177,7 @@ static const char *igt_crtc_prop_names[IGT_NUM_CRTC_PROPS] = {
 	"ACTIVE"
 };
 
-static const char *igt_connector_prop_names[IGT_NUM_CONNECTOR_PROPS] = {
+const char *igt_connector_prop_names[IGT_NUM_CONNECTOR_PROPS] = {
 	"scaling mode",
 	"CRTC_ID"
 };
diff --git a/lib/igt_kms.h b/lib/igt_kms.h
index 6422adc..ae2b505 100644
--- a/lib/igt_kms.h
+++ b/lib/igt_kms.h
@@ -113,12 +113,16 @@ enum igt_atomic_crtc_properties {
        IGT_NUM_CRTC_PROPS
 };
 
+extern const char *igt_crtc_prop_names[];
+
 enum igt_atomic_connector_properties {
        IGT_CONNECTOR_SCALING_MODE = 0,
        IGT_CONNECTOR_CRTC_ID,
        IGT_NUM_CONNECTOR_PROPS
 };
 
+extern const char *igt_connector_prop_names[];
+
 struct kmstest_connector_config {
 	drmModeCrtc *crtc;
 	drmModeConnector *connector;
@@ -214,6 +218,7 @@ enum igt_atomic_plane_properties {
        IGT_NUM_PLANE_PROPS
 };
 
+extern const char *igt_plane_prop_names[];
 
 typedef struct igt_display igt_display_t;
 typedef struct igt_pipe igt_pipe_t;
-- 
2.5.5

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 05/12] tests/kms_atomic: use global atomic properties definitions

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 tests/kms_atomic.c | 123 ++++++++++++++++-------------------------------------
 1 file changed, 37 insertions(+), 86 deletions(-)

diff --git a/tests/kms_atomic.c b/tests/kms_atomic.c
index 1441fdf..8b648eb 100644
--- a/tests/kms_atomic.c
+++ b/tests/kms_atomic.c
@@ -105,55 +105,6 @@ static const char *plane_type_prop_names[NUM_PLANE_TYPE_PROPS] = {
 	"Cursor"
 };
 
-enum plane_properties {
-	PLANE_SRC_X = 0,
-	PLANE_SRC_Y,
-	PLANE_SRC_W,
-	PLANE_SRC_H,
-	PLANE_CRTC_X,
-	PLANE_CRTC_Y,
-	PLANE_CRTC_W,
-	PLANE_CRTC_H,
-	PLANE_FB_ID,
-	PLANE_CRTC_ID,
-	PLANE_TYPE,
-	NUM_PLANE_PROPS
-};
-
-static const char *plane_prop_names[NUM_PLANE_PROPS] = {
-	"SRC_X",
-	"SRC_Y",
-	"SRC_W",
-	"SRC_H",
-	"CRTC_X",
-	"CRTC_Y",
-	"CRTC_W",
-	"CRTC_H",
-	"FB_ID",
-	"CRTC_ID",
-	"type"
-};
-
-enum crtc_properties {
-	CRTC_MODE_ID = 0,
-	CRTC_ACTIVE,
-	NUM_CRTC_PROPS
-};
-
-static const char *crtc_prop_names[NUM_CRTC_PROPS] = {
-	"MODE_ID",
-	"ACTIVE"
-};
-
-enum connector_properties {
-	CONNECTOR_CRTC_ID = 0,
-	NUM_CONNECTOR_PROPS
-};
-
-static const char *connector_prop_names[NUM_CONNECTOR_PROPS] = {
-	"CRTC_ID"
-};
-
 struct kms_atomic_blob {
 	uint32_t id; /* 0 if not already allocated */
 	size_t len;
@@ -197,9 +148,9 @@ struct kms_atomic_state {
 
 struct kms_atomic_desc {
 	int fd;
-	uint32_t props_connector[NUM_CONNECTOR_PROPS];
-	uint32_t props_crtc[NUM_CRTC_PROPS];
-	uint32_t props_plane[NUM_PLANE_PROPS];
+	uint32_t props_connector[IGT_NUM_CONNECTOR_PROPS];
+	uint32_t props_crtc[IGT_NUM_CRTC_PROPS];
+	uint32_t props_plane[IGT_NUM_PLANE_PROPS];
 	uint64_t props_plane_type[NUM_PLANE_TYPE_PROPS];
 };
 
@@ -280,7 +231,7 @@ connector_get_current_state(struct kms_atomic_connector_state *connector)
 	for (i = 0; i < props->count_props; i++) {
 		uint32_t *prop_ids = connector->state->desc->props_connector;
 
-		if (props->props[i] == prop_ids[CONNECTOR_CRTC_ID])
+		if (props->props[i] == prop_ids[IGT_CONNECTOR_CRTC_ID])
 			connector->crtc_id = props->prop_values[i];
 	}
 	drmModeFreeObjectProperties(props);
@@ -348,16 +299,16 @@ find_connector(struct kms_atomic_state *state,
 static void plane_populate_req(struct kms_atomic_plane_state *plane,
 			       drmModeAtomicReq *req)
 {
-	plane_set_prop(req, plane, PLANE_CRTC_ID, plane->crtc_id);
-	plane_set_prop(req, plane, PLANE_FB_ID, plane->fb_id);
-	plane_set_prop(req, plane, PLANE_SRC_X, plane->src_x);
-	plane_set_prop(req, plane, PLANE_SRC_Y, plane->src_y);
-	plane_set_prop(req, plane, PLANE_SRC_W, plane->src_w);
-	plane_set_prop(req, plane, PLANE_SRC_H, plane->src_h);
-	plane_set_prop(req, plane, PLANE_CRTC_X, plane->crtc_x);
-	plane_set_prop(req, plane, PLANE_CRTC_Y, plane->crtc_y);
-	plane_set_prop(req, plane, PLANE_CRTC_W, plane->crtc_w);
-	plane_set_prop(req, plane, PLANE_CRTC_H, plane->crtc_h);
+	plane_set_prop(req, plane, IGT_PLANE_CRTC_ID, plane->crtc_id);
+	plane_set_prop(req, plane, IGT_PLANE_FB_ID, plane->fb_id);
+	plane_set_prop(req, plane, IGT_PLANE_SRC_X, plane->src_x);
+	plane_set_prop(req, plane, IGT_PLANE_SRC_Y, plane->src_y);
+	plane_set_prop(req, plane, IGT_PLANE_SRC_W, plane->src_w);
+	plane_set_prop(req, plane, IGT_PLANE_SRC_H, plane->src_h);
+	plane_set_prop(req, plane, IGT_PLANE_CRTC_X, plane->crtc_x);
+	plane_set_prop(req, plane, IGT_PLANE_CRTC_Y, plane->crtc_y);
+	plane_set_prop(req, plane, IGT_PLANE_CRTC_W, plane->crtc_w);
+	plane_set_prop(req, plane, IGT_PLANE_CRTC_H, plane->crtc_h);
 }
 
 static void plane_get_current_state(struct kms_atomic_plane_state *plane)
@@ -373,27 +324,27 @@ static void plane_get_current_state(struct kms_atomic_plane_state *plane)
 	for (i = 0; i < props->count_props; i++) {
 		uint32_t *prop_ids = desc->props_plane;
 
-		if (props->props[i] == prop_ids[PLANE_CRTC_ID])
+		if (props->props[i] == prop_ids[IGT_PLANE_CRTC_ID])
 			plane->crtc_id = props->prop_values[i];
-		else if (props->props[i] == prop_ids[PLANE_FB_ID])
+		else if (props->props[i] == prop_ids[IGT_PLANE_FB_ID])
 			plane->fb_id = props->prop_values[i];
-		else if (props->props[i] == prop_ids[PLANE_CRTC_X])
+		else if (props->props[i] == prop_ids[IGT_PLANE_CRTC_X])
 			plane->crtc_x = props->prop_values[i];
-		else if (props->props[i] == prop_ids[PLANE_CRTC_Y])
+		else if (props->props[i] == prop_ids[IGT_PLANE_CRTC_Y])
 			plane->crtc_y = props->prop_values[i];
-		else if (props->props[i] == prop_ids[PLANE_CRTC_W])
+		else if (props->props[i] == prop_ids[IGT_PLANE_CRTC_W])
 			plane->crtc_w = props->prop_values[i];
-		else if (props->props[i] == prop_ids[PLANE_CRTC_H])
+		else if (props->props[i] == prop_ids[IGT_PLANE_CRTC_H])
 			plane->crtc_h = props->prop_values[i];
-		else if (props->props[i] == prop_ids[PLANE_SRC_X])
+		else if (props->props[i] == prop_ids[IGT_PLANE_SRC_X])
 			plane->src_x = props->prop_values[i];
-		else if (props->props[i] == prop_ids[PLANE_SRC_Y])
+		else if (props->props[i] == prop_ids[IGT_PLANE_SRC_Y])
 			plane->src_y = props->prop_values[i];
-		else if (props->props[i] == prop_ids[PLANE_SRC_W])
+		else if (props->props[i] == prop_ids[IGT_PLANE_SRC_W])
 			plane->src_w = props->prop_values[i];
-		else if (props->props[i] == prop_ids[PLANE_SRC_H])
+		else if (props->props[i] == prop_ids[IGT_PLANE_SRC_H])
 			plane->src_h = props->prop_values[i];
-		else if (props->props[i] == prop_ids[PLANE_TYPE]) {
+		else if (props->props[i] == prop_ids[IGT_PLANE_TYPE]) {
 			int j;
 
 			for (j = 0; j < ARRAY_SIZE(desc->props_plane_type); j++) {
@@ -473,8 +424,8 @@ find_plane(struct kms_atomic_state *state, enum plane_type type,
 static void crtc_populate_req(struct kms_atomic_crtc_state *crtc,
 			      drmModeAtomicReq *req)
 {
-	crtc_set_prop(req, crtc, CRTC_MODE_ID, crtc->mode.id);
-	crtc_set_prop(req, crtc, CRTC_ACTIVE, crtc->active);
+	crtc_set_prop(req, crtc, IGT_CRTC_MODE_ID, crtc->mode.id);
+	crtc_set_prop(req, crtc, IGT_CRTC_ACTIVE, crtc->active);
 }
 
 static void crtc_get_current_state(struct kms_atomic_crtc_state *crtc)
@@ -489,7 +440,7 @@ static void crtc_get_current_state(struct kms_atomic_crtc_state *crtc)
 	for (i = 0; i < props->count_props; i++) {
 		uint32_t *prop_ids = crtc->state->desc->props_crtc;
 
-		if (props->props[i] == prop_ids[CRTC_MODE_ID]) {
+		if (props->props[i] == prop_ids[IGT_CRTC_MODE_ID]) {
 			drmModePropertyBlobPtr blob;
 
 			crtc->mode.id = props->prop_values[i];
@@ -509,7 +460,7 @@ static void crtc_get_current_state(struct kms_atomic_crtc_state *crtc)
 				crtc->mode.data = blob->data;
 			crtc->mode.len = blob->length;
 		}
-		else if (props->props[i] == prop_ids[CRTC_ACTIVE]) {
+		else if (props->props[i] == prop_ids[IGT_CRTC_ACTIVE]) {
 			crtc->active = props->prop_values[i];
 		}
 	}
@@ -618,7 +569,7 @@ static void crtc_commit_legacy(struct kms_atomic_crtc_state *crtc,
 
 	for (i = 0; i < props->count_props; i++) {
 		if (props->props[i] !=
-		    crtc->state->desc->props_crtc[CRTC_MODE_ID])
+		    crtc->state->desc->props_crtc[IGT_CRTC_MODE_ID])
 			continue;
 		crtc->mode.id = props->prop_values[i];
 		break;
@@ -747,20 +698,20 @@ static void atomic_setup(struct kms_atomic_state *state)
 	igt_assert(state->connectors);
 
 	fill_obj_props(desc->fd, res->crtcs[0],
-		       DRM_MODE_OBJECT_CRTC, NUM_CRTC_PROPS,
-		       crtc_prop_names, desc->props_crtc);
+		       DRM_MODE_OBJECT_CRTC, IGT_NUM_CRTC_PROPS,
+		       igt_crtc_prop_names, desc->props_crtc);
 
 	fill_obj_props(desc->fd, res_plane->planes[0],
-		       DRM_MODE_OBJECT_PLANE, NUM_PLANE_PROPS,
-		       plane_prop_names, desc->props_plane);
+		       DRM_MODE_OBJECT_PLANE, IGT_NUM_PLANE_PROPS,
+		       igt_plane_prop_names, desc->props_plane);
 	fill_obj_prop_map(desc->fd, res_plane->planes[0],
 			  DRM_MODE_OBJECT_PLANE, "type",
 			  NUM_PLANE_TYPE_PROPS, plane_type_prop_names,
 			  desc->props_plane_type);
 
 	fill_obj_props(desc->fd, res->connectors[0],
-		       DRM_MODE_OBJECT_CONNECTOR, NUM_CONNECTOR_PROPS,
-		       connector_prop_names, desc->props_connector);
+		       DRM_MODE_OBJECT_CONNECTOR, IGT_NUM_CONNECTOR_PROPS,
+		       igt_connector_prop_names, desc->props_connector);
 
 	for (i = 0; i < state->num_crtcs; i++) {
 		struct kms_atomic_crtc_state *crtc = &state->crtcs[i];
@@ -1236,7 +1187,7 @@ static void atomic_invalid_params(struct kms_atomic_crtc_state *crtc,
 
 	/* Valid property, valid value. */
 	for (i = 0; i < ARRAY_SIZE(props_raw); i++) {
-		props_raw[i] = desc->props_crtc[CRTC_MODE_ID];
+		props_raw[i] = desc->props_crtc[IGT_CRTC_MODE_ID];
 		values_raw[i] = crtc->mode.id;
 	}
 	do_ioctl(desc->fd, DRM_IOCTL_MODE_ATOMIC, &ioc);
-- 
2.5.5

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

[PATCH 06/12] lib/igt_kms: Add support for the IN_FENCE_FD property

[Gustavo Padovan]

From: Robert Foss <robert.foss@collabora.com>

Add support dor the IN_FENCE_FD property to enable setting in fences for atomic
commits.

Signed-off-by: Robert Foss <robert.foss@collabora.com>
---
 lib/igt_kms.c | 20 ++++++++++++++++++++
 lib/igt_kms.h |  5 +++++
 2 files changed, 25 insertions(+)

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index 8aaff5b..4748c0a 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -164,6 +164,7 @@ const char *igt_plane_prop_names[IGT_NUM_PLANE_PROPS] = {
 	"CRTC_H",
 	"FB_ID",
 	"CRTC_ID",
+	"IN_FENCE_FD",
 	"type",
 	"rotation"
 };
@@ -1426,6 +1427,7 @@ void igt_display_init(igt_display_t *display, int drm_fd)
 			n_planes++;
 			plane->pipe = pipe;
 			plane->drm_plane = drm_plane;
+			plane->fence_fd = -1;
 
 			if (is_atomic == 0) {
 				display->is_atomic = 1;
@@ -1712,6 +1714,11 @@ igt_atomic_prepare_plane_commit(igt_plane_t *plane, igt_pipe_t *pipe,
 	    plane->index,
 	    fb_id);
 
+	if (plane->fence_fd >= 0) {
+		uint32_t fence_fd = plane->fence_fd;
+		igt_atomic_populate_plane_req(req, plane, IGT_PLANE_IN_FENCE_FD, fence_fd);
+	}
+
 	if (plane->fb_changed) {
 		igt_atomic_populate_plane_req(req, plane, IGT_PLANE_CRTC_ID, fb_id ? crtc_id : 0);
 		igt_atomic_populate_plane_req(req, plane, IGT_PLANE_FB_ID, fb_id);
@@ -2522,6 +2529,19 @@ void igt_plane_set_fb(igt_plane_t *plane, struct igt_fb *fb)
 	plane->size_changed = true;
 }
 
+/**
+ * igt_plane_set_fence_fd:
+ * @plane: plane
+ * @fence_fd: fence fd, disable fence_fd by setting it to 0
+ *
+ * This function sets a fence fd to enable a commit to wait for some event to
+ * occur before completing.
+ */
+void igt_plane_set_fence_fd(igt_plane_t *plane, uint32_t fence_fd)
+{
+	plane->fence_fd = fence_fd;
+}
+
 void igt_plane_set_position(igt_plane_t *plane, int x, int y)
 {
 	igt_pipe_t *pipe = plane->pipe;
diff --git a/lib/igt_kms.h b/lib/igt_kms.h
index ae2b505..344f931 100644
--- a/lib/igt_kms.h
+++ b/lib/igt_kms.h
@@ -213,6 +213,7 @@ enum igt_atomic_plane_properties {
 
        IGT_PLANE_FB_ID,
        IGT_PLANE_CRTC_ID,
+       IGT_PLANE_IN_FENCE_FD,
        IGT_PLANE_TYPE,
        IGT_PLANE_ROTATION,
        IGT_NUM_PLANE_PROPS
@@ -266,6 +267,9 @@ typedef struct {
 	uint32_t src_h;
 
 	igt_rotation_t rotation;
+
+	/* in fence fd */
+	int32_t fence_fd;
 	uint32_t atomic_props_plane[IGT_NUM_PLANE_PROPS];
 } igt_plane_t;
 
@@ -349,6 +353,7 @@ void igt_pipe_set_ctm_matrix(igt_pipe_t *pipe, void *ptr, size_t length);
 void igt_pipe_set_gamma_lut(igt_pipe_t *pipe, void *ptr, size_t length);
 
 void igt_plane_set_fb(igt_plane_t *plane, struct igt_fb *fb);
+void igt_plane_set_fence_fd(igt_plane_t *plane, uint32_t fence_fd);
 void igt_plane_set_position(igt_plane_t *plane, int x, int y);
 void igt_plane_set_size(igt_plane_t *plane, int w, int h);
 void igt_plane_set_rotation(igt_plane_t *plane, igt_rotation_t rotation);
-- 
2.5.5

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 07/12] lib/igt_kms: Add support for the OUT_FENCE_PTR property

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Add support for the OUT_FENCE_PTR property to enable setting out fences for
atomic commits.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 lib/igt_kms.c | 20 +++++++++++++++++++-
 lib/igt_kms.h |  3 +++
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index 4748c0a..f25e1eb 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -175,7 +175,8 @@ const char *igt_crtc_prop_names[IGT_NUM_CRTC_PROPS] = {
 	"DEGAMMA_LUT",
 	"GAMMA_LUT",
 	"MODE_ID",
-	"ACTIVE"
+	"ACTIVE",
+	"OUT_FENCE_PTR"
 };
 
 const char *igt_connector_prop_names[IGT_NUM_CONNECTOR_PROPS] = {
@@ -2103,6 +2104,9 @@ static void igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicRe
 		igt_atomic_populate_crtc_req(req, pipe_obj, IGT_CRTC_ACTIVE, !!output);
 	}
 
+	if (pipe_obj->out_fence_ptr)
+		igt_atomic_populate_crtc_req(req, pipe_obj, IGT_CRTC_OUT_FENCE_PTR, pipe_obj->out_fence_ptr);
+
 	/*
 	 *	TODO: Add all crtc level properties here
 	 */
@@ -2683,6 +2687,20 @@ igt_pipe_set_gamma_lut(igt_pipe_t *pipe, void *ptr, size_t length)
 }
 
 /**
+ * igt_pipe_set_out_fence_ptr:
+ * @pipe: pipe pointer to which background color to be set
+ * @fence_ptr: out fence pointer
+ *
+ * Sets the out fence pointer that will be passed to the kernel in
+ * the atomic ioctl. When the kernel returns the out fence pointer
+ * will contain the fd number of the out fence created by KMS.
+ */
+void igt_pipe_set_out_fence_ptr(igt_pipe_t *pipe, int *fence_ptr)
+{
+	pipe->out_fence_ptr = (uint64_t) fence_ptr;
+}
+
+/**
  * igt_crtc_set_background:
  * @pipe: pipe pointer to which background color to be set
  * @background: background color value in BGR 16bpc
diff --git a/lib/igt_kms.h b/lib/igt_kms.h
index 344f931..02d7bd1 100644
--- a/lib/igt_kms.h
+++ b/lib/igt_kms.h
@@ -110,6 +110,7 @@ enum igt_atomic_crtc_properties {
        IGT_CRTC_GAMMA_LUT,
        IGT_CRTC_MODE_ID,
        IGT_CRTC_ACTIVE,
+       IGT_CRTC_OUT_FENCE_PTR,
        IGT_NUM_CRTC_PROPS
 };
 
@@ -298,6 +299,7 @@ struct igt_pipe {
 
 	uint64_t mode_blob;
 	bool mode_changed;
+	uint64_t out_fence_ptr;
 };
 
 typedef struct {
@@ -351,6 +353,7 @@ static inline bool igt_plane_supports_rotation(igt_plane_t *plane)
 void igt_pipe_set_degamma_lut(igt_pipe_t *pipe, void *ptr, size_t length);
 void igt_pipe_set_ctm_matrix(igt_pipe_t *pipe, void *ptr, size_t length);
 void igt_pipe_set_gamma_lut(igt_pipe_t *pipe, void *ptr, size_t length);
+void igt_pipe_set_out_fence_ptr(igt_pipe_t *pipe, int *fence_ptr);
 
 void igt_plane_set_fb(igt_plane_t *plane, struct igt_fb *fb);
 void igt_plane_set_fence_fd(igt_plane_t *plane, uint32_t fence_fd);
-- 
2.5.5

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

[PATCH 08/12] tests/kms_atomic: stress possible fence settings

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 tests/kms_atomic.c | 124 +++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 115 insertions(+), 9 deletions(-)

diff --git a/tests/kms_atomic.c b/tests/kms_atomic.c
index 8b648eb..58fc0dd 100644
--- a/tests/kms_atomic.c
+++ b/tests/kms_atomic.c
@@ -44,6 +44,7 @@
 #include "drmtest.h"
 #include "igt.h"
 #include "igt_aux.h"
+#include "sw_sync.h"
 
 #ifndef DRM_CLIENT_CAP_ATOMIC
 #define DRM_CLIENT_CAP_ATOMIC 3
@@ -126,6 +127,7 @@ struct kms_atomic_plane_state {
 	uint32_t fb_id; /* 0 to disable */
 	uint32_t src_x, src_y, src_w, src_h; /* 16.16 fixed-point */
 	uint32_t crtc_x, crtc_y, crtc_w, crtc_h; /* normal integers */
+	int32_t fence_fd;
 };
 
 struct kms_atomic_crtc_state {
@@ -133,6 +135,7 @@ struct kms_atomic_crtc_state {
 	uint32_t obj;
 	int idx;
 	bool active;
+	uint64_t out_fence_ptr;
 	struct kms_atomic_blob mode;
 };
 
@@ -190,11 +193,13 @@ static uint32_t blob_duplicate(int fd, uint32_t id_orig)
 	crtc_populate_req(crtc, req); \
 	plane_populate_req(plane, req); \
 	do_atomic_commit((crtc)->state->desc->fd, req, flags); \
-	crtc_check_current_state(crtc, plane, relax); \
-	plane_check_current_state(plane, relax); \
+	if (!(flags & DRM_MODE_ATOMIC_TEST_ONLY)) { \
+		crtc_check_current_state(crtc, plane, relax); \
+		plane_check_current_state(plane, relax); \
+	} \
 }
 
-#define crtc_commit_atomic_err(crtc, plane, crtc_old, plane_old, req, relax, e) { \
+#define crtc_commit_atomic_err(crtc, plane, crtc_old, plane_old, req, flags, relax, e) { \
 	drmModeAtomicSetCursor(req, 0); \
 	crtc_populate_req(crtc, req); \
 	plane_populate_req(plane, req); \
@@ -299,6 +304,9 @@ find_connector(struct kms_atomic_state *state,
 static void plane_populate_req(struct kms_atomic_plane_state *plane,
 			       drmModeAtomicReq *req)
 {
+	if (plane->fence_fd)
+		plane_set_prop(req, plane, IGT_PLANE_IN_FENCE_FD, plane->fence_fd);
+
 	plane_set_prop(req, plane, IGT_PLANE_CRTC_ID, plane->crtc_id);
 	plane_set_prop(req, plane, IGT_PLANE_FB_ID, plane->fb_id);
 	plane_set_prop(req, plane, IGT_PLANE_SRC_X, plane->src_x);
@@ -424,6 +432,10 @@ find_plane(struct kms_atomic_state *state, enum plane_type type,
 static void crtc_populate_req(struct kms_atomic_crtc_state *crtc,
 			      drmModeAtomicReq *req)
 {
+	if (crtc->out_fence_ptr)
+		crtc_set_prop(req, crtc, IGT_CRTC_OUT_FENCE_PTR,
+			      crtc->out_fence_ptr);
+
 	crtc_set_prop(req, crtc, IGT_CRTC_MODE_ID, crtc->mode.id);
 	crtc_set_prop(req, crtc, IGT_CRTC_ACTIVE, crtc->active);
 }
@@ -986,6 +998,7 @@ static void plane_invalid_params(struct kms_atomic_crtc_state *crtc,
 	uint32_t format = plane_get_igt_format(&plane);
 	drmModeAtomicReq *req = drmModeAtomicAlloc();
 	struct igt_fb fb;
+	int timeline, fence_fd;
 
 	/* Pass a series of invalid object IDs for the FB ID. */
 	plane.fb_id = plane.obj;
@@ -1024,6 +1037,23 @@ static void plane_invalid_params(struct kms_atomic_crtc_state *crtc,
 	plane_commit_atomic_err(&plane, plane_old, req,
 	                        ATOMIC_RELAX_NONE, EINVAL);
 
+	/* invalid fence fd */
+	plane.fence_fd = plane.state->desc->fd;
+	plane.crtc_id = plane_old->crtc_id;
+	plane_commit_atomic_err(&plane, plane_old, req,
+	                        ATOMIC_RELAX_NONE, EINVAL);
+
+	/* Valid fence_fd but invalid CRTC */
+	timeline = sw_sync_timeline_create();
+	fence_fd =  sw_sync_fence_create(timeline, 1);
+	plane.fence_fd = fence_fd;
+	plane.crtc_id = ~0U;
+	plane_commit_atomic_err(&plane, plane_old, req,
+	                        ATOMIC_RELAX_NONE, EINVAL);
+	close(fence_fd);
+	close(timeline);
+
+	plane.fence_fd = -1;
 	plane.crtc_id = plane_old->crtc_id;
 	plane_commit_atomic(&plane, req, ATOMIC_RELAX_NONE);
 
@@ -1058,27 +1088,98 @@ static void crtc_invalid_params(struct kms_atomic_crtc_state *crtc_old,
 {
 	struct kms_atomic_crtc_state crtc = *crtc_old;
 	drmModeAtomicReq *req = drmModeAtomicAlloc();
+	int timeline, fence_fd, *out_fence;
 
 	igt_assert(req);
 
 	/* Pass a series of invalid object IDs for the mode ID. */
 	crtc.mode.id = plane->obj;
-	crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req,
+	crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req, 0,
 	                       ATOMIC_RELAX_NONE, EINVAL);
 
 	crtc.mode.id = crtc.obj;
-	crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req,
+	crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req, 0,
 	                       ATOMIC_RELAX_NONE, EINVAL);
 
 	crtc.mode.id = conn->obj;
-	crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req,
+	crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req, 0,
 	                       ATOMIC_RELAX_NONE, EINVAL);
 
 	crtc.mode.id = plane->fb_id;
-	crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req,
+	crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req, 0,
 	                       ATOMIC_RELAX_NONE, EINVAL);
 
+	/* invalid out_fence_ptr */
+	crtc.mode.id = crtc_old->mode.id;
+	crtc.out_fence_ptr = (uint64_t) crtc_invalid_params;
+	crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req, 0,
+	                       ATOMIC_RELAX_NONE, EFAULT);
+
+	/* invalid out_fence_ptr */
+	crtc.mode.id = crtc_old->mode.id;
+	crtc.out_fence_ptr = (uint64_t) 0x8;
+	crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req, 0,
+	                       ATOMIC_RELAX_NONE, EFAULT);
+	crtc.out_fence_ptr = (uint64_t) 0;
+
+	/* valid in fence but not allowed prop on crtc */
+	timeline = sw_sync_timeline_create();
+	fence_fd =  sw_sync_fence_create(timeline, 1);
+	plane->fence_fd = fence_fd;
+	crtc.active = false;
+	crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req, 0,
+			       ATOMIC_RELAX_NONE, EINVAL);
+
+	out_fence = malloc(sizeof(uint64_t));
+	igt_assert(out_fence);
+
+
+	/* valid out fence ptr and flip event but not allowed prop on crtc */
+	crtc.out_fence_ptr = (uint64_t) out_fence;
+	crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req, 0,
+			       ATOMIC_RELAX_NONE, EINVAL);
+
+	/* valid out fence ptr and flip event but not allowed prop on crtc */
+	crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req,
+			       DRM_MODE_PAGE_FLIP_EVENT,
+			       ATOMIC_RELAX_NONE, EINVAL);
+
+	/* valid page flip event but not allowed prop on crtc */
+	crtc.out_fence_ptr = (uint64_t) 0;
+	crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req,
+			       DRM_MODE_PAGE_FLIP_EVENT,
+			       ATOMIC_RELAX_NONE, EINVAL);
+	crtc.active = true;
+
+	/* valid out fence  ptr and flip event but invalid prop on crtc */
+	crtc.out_fence_ptr = (uint64_t) out_fence;
+	crtc.mode.id = plane->fb_id;
+	crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req, 0,
+			       ATOMIC_RELAX_NONE, EINVAL);
+
+	/* valid out fence ptr and flip event but invalid prop on crtc */
+	crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req,
+			       DRM_MODE_PAGE_FLIP_EVENT,
+			       ATOMIC_RELAX_NONE, EINVAL);
+
+	/* valid page flip event but invalid prop on crtc */
+	crtc.out_fence_ptr = (uint64_t) 0;
+	crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req,
+			       DRM_MODE_PAGE_FLIP_EVENT,
+			       ATOMIC_RELAX_NONE, EINVAL);
+
+	/* successful TEST_ONLY with fences set */
+	plane->fence_fd = fence_fd;
 	crtc.mode.id = crtc_old->mode.id;
+	crtc_commit_atomic(&crtc, plane, req, ATOMIC_RELAX_NONE,
+			   DRM_MODE_ATOMIC_TEST_ONLY);
+	igt_assert(*out_fence == -1);
+	close(fence_fd);
+	close(timeline);
+
+	/* reset fences */
+	plane->fence_fd = -1;
+	crtc.out_fence_ptr = (uint64_t) 0;
 	crtc_commit_atomic(&crtc, plane, req, ATOMIC_RELAX_NONE, 0);
 
 	/* Create a blob which is the wrong size to be a valid mode. */
@@ -1086,7 +1187,7 @@ static void crtc_invalid_params(struct kms_atomic_crtc_state *crtc_old,
 					    crtc.mode.data,
 					    sizeof(struct drm_mode_modeinfo) - 1,
 					    &crtc.mode.id));
-	crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req,
+	crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req, 0,
 	                       ATOMIC_RELAX_NONE, EINVAL);
 
 
@@ -1094,12 +1195,17 @@ static void crtc_invalid_params(struct kms_atomic_crtc_state *crtc_old,
 					    crtc.mode.data,
 					    sizeof(struct drm_mode_modeinfo) + 1,
 					    &crtc.mode.id));
-	crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req,
+	crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req, 0,
 	                       ATOMIC_RELAX_NONE, EINVAL);
 
+	/* out fence ptr but not page flip event */
+	crtc.out_fence_ptr = (uint64_t) out_fence;
+	crtc_commit_atomic(crtc_old, plane, req, ATOMIC_RELAX_NONE, 0);
+
 	/* Restore the CRTC and check the state matches the old. */
 	crtc_commit_atomic(crtc_old, plane, req, ATOMIC_RELAX_NONE, 0);
 
+	free(out_fence);
 	drmModeAtomicFree(req);
 }
 
-- 
2.5.5

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

[PATCH 09/12] tests/kms_atomic_transition: add fencing parameter to run_transition_tests

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 tests/kms_atomic_transition.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/tests/kms_atomic_transition.c b/tests/kms_atomic_transition.c
index 8b26b53..8bef85d 100644
--- a/tests/kms_atomic_transition.c
+++ b/tests/kms_atomic_transition.c
@@ -121,7 +121,7 @@ enum transition_type {
  * so test this and make sure it works.
  */
 static void
-run_transition_test(igt_display_t *display, enum pipe pipe, igt_output_t *output, enum transition_type type, bool nonblocking)
+run_transition_test(igt_display_t *display, enum pipe pipe, igt_output_t *output, enum transition_type type, bool nonblocking, bool fencing)
 {
 	struct igt_fb fb, argb_fb;
 	drmModeModeInfo *mode, override_mode;
@@ -592,19 +592,19 @@ igt_main
 
 	igt_subtest("plane-all-transition")
 		for_each_pipe_with_valid_output(&display, pipe, output)
-			run_transition_test(&display, pipe, output, TRANSITION_PLANES, false);
+			run_transition_test(&display, pipe, output, TRANSITION_PLANES, false, false);
 
 	igt_subtest("plane-all-transition-nonblocking")
 		for_each_pipe_with_valid_output(&display, pipe, output)
-			run_transition_test(&display, pipe, output, TRANSITION_PLANES, true);
+			run_transition_test(&display, pipe, output, TRANSITION_PLANES, true, false);
 
 	igt_subtest("plane-all-modeset-transition")
 		for_each_pipe_with_valid_output(&display, pipe, output)
-			run_transition_test(&display, pipe, output, TRANSITION_MODESET, false);
+			run_transition_test(&display, pipe, output, TRANSITION_MODESET, false, false);
 
 	igt_subtest("plane-toggle-modeset-transition")
 		for_each_pipe_with_valid_output(&display, pipe, output)
-			run_transition_test(&display, pipe, output, TRANSITION_MODESET_DISABLE, false);
+			run_transition_test(&display, pipe, output, TRANSITION_MODESET_DISABLE, false, false);
 
 	for (i = 1; i <= I915_MAX_PIPES; i++) {
 		igt_subtest_f("%ix-modeset-transitions", i)
-- 
2.5.5

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 10/12] tests/kms_atomic_transition: add out_fences tests

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 lib/igt_kms.c                 | 22 ++++++++++++++++++++++
 tests/kms_atomic_transition.c | 30 ++++++++++++++++++++++++++++--
 2 files changed, 50 insertions(+), 2 deletions(-)

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index f25e1eb..9271ab4 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -49,6 +49,7 @@
 #include "intel_chipset.h"
 #include "igt_debugfs.h"
 #include "igt_sysfs.h"
+#include "sw_sync.h"
 
 /**
  * SECTION:igt_kms
@@ -2183,6 +2184,27 @@ static int igt_atomic_commit(igt_display_t *display, uint32_t flags, void *user_
 	}
 
 	ret = drmModeAtomicCommit(display->drm_fd, req, flags, user_data);
+	if (!ret) {
+		uint64_t *fence_ptr;
+
+		for_each_pipe(display, pipe) {
+			igt_pipe_t *pipe_obj = &display->pipes[pipe];
+
+			fence_ptr = (uint64_t *)pipe_obj->out_fence_ptr;
+			if (!fence_ptr)
+				continue;
+
+			if (flags & DRM_MODE_ATOMIC_TEST_ONLY) {
+				igt_assert(*fence_ptr == -1);
+			} else {
+				igt_assert(*fence_ptr >= 0);
+				ret = sw_sync_wait(*fence_ptr, 1000);
+				igt_assert(ret == 0);
+				close(*fence_ptr);
+			}
+		}
+	}
+
 	drmModeAtomicFree(req);
 	return ret;
 
diff --git a/tests/kms_atomic_transition.c b/tests/kms_atomic_transition.c
index 8bef85d..507f526 100644
--- a/tests/kms_atomic_transition.c
+++ b/tests/kms_atomic_transition.c
@@ -132,6 +132,7 @@ run_transition_test(igt_display_t *display, enum pipe pipe, igt_output_t *output
 	struct plane_parms parms[IGT_MAX_PLANES];
 	bool skip_test = false;
 	unsigned flags = DRM_MODE_PAGE_FLIP_EVENT;
+	int out_fence, ret;
 
 	if (nonblocking)
 		flags |= DRM_MODE_ATOMIC_NONBLOCK;
@@ -198,9 +199,10 @@ run_transition_test(igt_display_t *display, enum pipe pipe, igt_output_t *output
 	 * planes to fix this
 	 */
 	while (1) {
-		int ret;
-
 		wm_setup_plane(display, pipe, iter_max - 1, parms);
+
+		if (fencing)
+			igt_pipe_set_out_fence_ptr(&display->pipes[pipe], &out_fence);
 		ret = igt_display_try_commit_atomic(display, DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
 
 		if (ret != -EINVAL || n_planes < 3)
@@ -231,6 +233,9 @@ run_transition_test(igt_display_t *display, enum pipe pipe, igt_output_t *output
 
 		wm_setup_plane(display, pipe, i, parms);
 
+		if (fencing)
+			igt_pipe_set_out_fence_ptr(&display->pipes[pipe], &out_fence);
+
 		igt_display_commit_atomic(display, flags, (void *)(unsigned long)i);
 		drmHandleEvent(display->drm_fd, &drm_events);
 
@@ -239,6 +244,9 @@ run_transition_test(igt_display_t *display, enum pipe pipe, igt_output_t *output
 
 			wm_setup_plane(display, pipe, 0, parms);
 
+			if (fencing)
+				igt_pipe_set_out_fence_ptr(&display->pipes[pipe], &out_fence);
+
 			igt_display_commit_atomic(display, flags, (void *)0UL);
 
 			drmHandleEvent(display->drm_fd, &drm_events);
@@ -252,6 +260,9 @@ run_transition_test(igt_display_t *display, enum pipe pipe, igt_output_t *output
 				if (type == TRANSITION_MODESET)
 					igt_output_override_mode(output, &override_mode);
 
+				if (fencing)
+					igt_pipe_set_out_fence_ptr(&display->pipes[pipe], &out_fence);
+
 				igt_display_commit_atomic(display, flags, (void *)(unsigned long)j);
 				drmHandleEvent(display->drm_fd, &drm_events);
 
@@ -259,6 +270,9 @@ run_transition_test(igt_display_t *display, enum pipe pipe, igt_output_t *output
 				if (type == TRANSITION_MODESET)
 					igt_output_override_mode(output, NULL);
 
+				if (fencing)
+					igt_pipe_set_out_fence_ptr(&display->pipes[pipe], &out_fence);
+
 				igt_display_commit_atomic(display, flags, (void *)(unsigned long)i);
 				drmHandleEvent(display->drm_fd, &drm_events);
 			}
@@ -594,14 +608,26 @@ igt_main
 		for_each_pipe_with_valid_output(&display, pipe, output)
 			run_transition_test(&display, pipe, output, TRANSITION_PLANES, false, false);
 
+	igt_subtest("plane-all-transition-fencing")
+		for_each_pipe_with_valid_output(&display, pipe, output)
+			run_transition_test(&display, pipe, output, TRANSITION_PLANES, false, true);
+
 	igt_subtest("plane-all-transition-nonblocking")
 		for_each_pipe_with_valid_output(&display, pipe, output)
 			run_transition_test(&display, pipe, output, TRANSITION_PLANES, true, false);
 
+	igt_subtest("plane-all-transition-nonblocking-fencing")
+		for_each_pipe_with_valid_output(&display, pipe, output)
+			run_transition_test(&display, pipe, output, TRANSITION_PLANES, true, true);
+
 	igt_subtest("plane-all-modeset-transition")
 		for_each_pipe_with_valid_output(&display, pipe, output)
 			run_transition_test(&display, pipe, output, TRANSITION_MODESET, false, false);
 
+	igt_subtest("plane-all-modeset-transition-fencing")
+		for_each_pipe_with_valid_output(&display, pipe, output)
+			run_transition_test(&display, pipe, output, TRANSITION_MODESET, false, true);
+
 	igt_subtest("plane-toggle-modeset-transition")
 		for_each_pipe_with_valid_output(&display, pipe, output)
 			run_transition_test(&display, pipe, output, TRANSITION_MODESET_DISABLE, false, false);
-- 
2.5.5

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 11/12] tests/kms_atomic_transition: add in_fences tests

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 tests/kms_atomic_transition.c | 92 ++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 82 insertions(+), 10 deletions(-)

diff --git a/tests/kms_atomic_transition.c b/tests/kms_atomic_transition.c
index 507f526..54fc9da 100644
--- a/tests/kms_atomic_transition.c
+++ b/tests/kms_atomic_transition.c
@@ -23,7 +23,9 @@
 
 #include "igt.h"
 #include "drmtest.h"
+#include "sw_sync.h"
 #include <errno.h>
+#include <pthread.h>
 #include <stdbool.h>
 #include <stdio.h>
 #include <string.h>
@@ -111,6 +113,78 @@ enum transition_type {
 	TRANSITION_MODESET_DISABLE,
 };
 
+int timeline[IGT_MAX_PLANES];
+pthread_t thread[IGT_MAX_PLANES];
+int seqno[IGT_MAX_PLANES];
+
+static void prepare_fencing(igt_display_t *display, enum pipe pipe)
+{
+	igt_plane_t *plane;
+
+	for_each_plane_on_pipe(display, pipe, plane)
+		timeline[plane->index] = sw_sync_timeline_create();
+}
+
+static void unprepare_fencing(igt_display_t *display, enum pipe pipe)
+{
+	igt_plane_t *plane;
+
+	for_each_plane_on_pipe(display, pipe, plane)
+		close(timeline[plane->index]);
+}
+
+static void *fence_inc_thread(void *arg)
+{
+	int t = *((int *) arg);
+
+	pthread_detach(pthread_self());
+
+	usleep(5000);
+	sw_sync_timeline_inc(t, 1);
+	return NULL;
+}
+
+static void configure_fencing(igt_display_t *display, enum pipe pipe)
+{
+	igt_plane_t *plane;
+	int i, fd, ret;
+
+	for_each_plane_on_pipe(display, pipe, plane) {
+
+		if (!plane->fb)
+			continue;
+
+		i = plane->index;
+
+		seqno[i]++;
+		fd = sw_sync_fence_create(timeline[i], seqno[i]);
+		igt_plane_set_fence_fd(plane, fd);
+		ret = pthread_create(&thread[i], NULL, fence_inc_thread, &timeline[i]);
+		igt_assert_eq(ret, 0);
+	}
+}
+
+static void clear_fencing(igt_display_t *display, enum pipe pipe)
+{
+	igt_plane_t *plane;
+
+	for_each_plane_on_pipe(display, pipe, plane)
+		igt_plane_set_fence_fd(plane, -1);
+}
+
+static void atomic_commit(igt_display_t *display, enum pipe pipe, unsigned int flags, void *data, bool fencing, int *out_fence)
+{
+	if (fencing) {
+		configure_fencing(display, pipe);
+		igt_pipe_set_out_fence_ptr(&display->pipes[pipe], out_fence);
+	}
+
+	igt_display_commit_atomic(display, flags, data);
+
+	if (fencing)
+		clear_fencing(display, pipe);
+}
+
 /*
  * 1. Set primary plane to a known fb.
  * 2. Make sure getcrtc returns the correct fb id.
@@ -134,6 +208,9 @@ run_transition_test(igt_display_t *display, enum pipe pipe, igt_output_t *output
 	unsigned flags = DRM_MODE_PAGE_FLIP_EVENT;
 	int out_fence, ret;
 
+	if (fencing)
+		prepare_fencing(display, pipe);
+
 	if (nonblocking)
 		flags |= DRM_MODE_ATOMIC_NONBLOCK;
 
@@ -233,10 +310,8 @@ run_transition_test(igt_display_t *display, enum pipe pipe, igt_output_t *output
 
 		wm_setup_plane(display, pipe, i, parms);
 
-		if (fencing)
-			igt_pipe_set_out_fence_ptr(&display->pipes[pipe], &out_fence);
+		atomic_commit(display, pipe, flags, (void *)(unsigned long)i, fencing, &out_fence);
 
-		igt_display_commit_atomic(display, flags, (void *)(unsigned long)i);
 		drmHandleEvent(display->drm_fd, &drm_events);
 
 		if (type == TRANSITION_MODESET_DISABLE) {
@@ -260,19 +335,14 @@ run_transition_test(igt_display_t *display, enum pipe pipe, igt_output_t *output
 				if (type == TRANSITION_MODESET)
 					igt_output_override_mode(output, &override_mode);
 
-				if (fencing)
-					igt_pipe_set_out_fence_ptr(&display->pipes[pipe], &out_fence);
-
-				igt_display_commit_atomic(display, flags, (void *)(unsigned long)j);
+				atomic_commit(display, pipe, flags, (void *)(unsigned long)i, fencing, &out_fence);
 				drmHandleEvent(display->drm_fd, &drm_events);
 
 				wm_setup_plane(display, pipe, i, parms);
 				if (type == TRANSITION_MODESET)
 					igt_output_override_mode(output, NULL);
 
-				if (fencing)
-					igt_pipe_set_out_fence_ptr(&display->pipes[pipe], &out_fence);
-
+				atomic_commit(display, pipe, flags, (void *)(unsigned long)i, fencing, &out_fence);
 				igt_display_commit_atomic(display, flags, (void *)(unsigned long)i);
 				drmHandleEvent(display->drm_fd, &drm_events);
 			}
@@ -280,6 +350,8 @@ run_transition_test(igt_display_t *display, enum pipe pipe, igt_output_t *output
 	}
 
 cleanup:
+	unprepare_fencing(display, pipe);
+
 	igt_output_set_pipe(output, PIPE_NONE);
 
 	for_each_plane_on_pipe(display, pipe, plane)
-- 
2.5.5

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

[PATCH 12/12] tests/kms_atomic_transition: set out_fence for all crtcs

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 tests/kms_atomic_transition.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/tests/kms_atomic_transition.c b/tests/kms_atomic_transition.c
index 54fc9da..3022e04 100644
--- a/tests/kms_atomic_transition.c
+++ b/tests/kms_atomic_transition.c
@@ -486,7 +486,7 @@ static void collect_crcs_mask(igt_pipe_crc_t **pipe_crcs, unsigned mask, igt_crc
 	}
 }
 
-static void run_modeset_tests(igt_display_t *display, int howmany, bool nonblocking)
+static void run_modeset_tests(igt_display_t *display, int howmany, bool nonblocking, bool fencing)
 {
 	struct igt_fb fbs[2];
 	int i, j;
@@ -495,6 +495,7 @@ static void run_modeset_tests(igt_display_t *display, int howmany, bool nonblock
 	igt_output_t *output;
 	unsigned width = 0, height = 0;
 	bool skip_test = false;
+	int out_fence[I915_MAX_PIPES];
 
 	for_each_connected_output(display, output) {
 		drmModeModeInfo *mode = igt_output_get_mode(output);
@@ -532,6 +533,11 @@ static void run_modeset_tests(igt_display_t *display, int howmany, bool nonblock
 			igt_plane_set_size(plane, mode->hdisplay, mode->vdisplay);
 		} else
 			igt_plane_set_fb(plane, NULL);
+
+		if(fencing)
+			igt_pipe_set_out_fence_ptr(&display->pipes[i],
+						   &out_fence[i]);
+
 	}
 
 	/*
@@ -619,7 +625,7 @@ cleanup:
 
 }
 
-static void run_modeset_transition(igt_display_t *display, int requested_outputs, bool nonblocking)
+static void run_modeset_transition(igt_display_t *display, int requested_outputs, bool nonblocking, bool fencing)
 {
 	igt_output_t *outputs[I915_MAX_PIPES] = {};
 	int num_outputs = 0;
@@ -647,7 +653,7 @@ static void run_modeset_transition(igt_display_t *display, int requested_outputs
 		      "Should have at least %i outputs, found %i\n",
 		      requested_outputs, num_outputs);
 
-	run_modeset_tests(display, requested_outputs, nonblocking);
+	run_modeset_tests(display, requested_outputs, nonblocking, fencing);
 }
 
 igt_main
@@ -706,10 +712,16 @@ igt_main
 
 	for (i = 1; i <= I915_MAX_PIPES; i++) {
 		igt_subtest_f("%ix-modeset-transitions", i)
-			run_modeset_transition(&display, i, false);
+			run_modeset_transition(&display, i, false, false);
 
 		igt_subtest_f("%ix-modeset-transitions-nonblocking", i)
-			run_modeset_transition(&display, i, true);
+			run_modeset_transition(&display, i, true, false);
+
+		igt_subtest_f("%ix-modeset-transitions-fencing", i)
+			run_modeset_transition(&display, i, false, true);
+
+		igt_subtest_f("%ix-modeset-transitions-nonblocking-fencing", i)
+			run_modeset_transition(&display, i, true, true);
 	}
 
 	igt_fixture {
-- 
2.5.5

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

Re: [PATCH 01/12] tests/kms_atomic_transition: use select + read instead of blocking read

[Daniel Vetter]

On Mon, Nov 14, 2016 at 06:59:14PM +0900, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> If the event never arrives we can timeout with select and end the test.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>  tests/kms_atomic_transition.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/tests/kms_atomic_transition.c b/tests/kms_atomic_transition.c
> index 1977993..e693c88 100644
> --- a/tests/kms_atomic_transition.c
> +++ b/tests/kms_atomic_transition.c
> @@ -296,6 +296,14 @@ static void commit_display(igt_display_t *display, unsigned event_mask, bool non
>  		struct drm_event *e = (void *)buf;
>  		struct drm_event_vblank *vblank = (void *)buf;
>  		uint32_t crtc_id, pipe = I915_MAX_PIPES;
> +		struct timeval timeout = { .tv_sec = 3, .tv_usec = 0 };
> +		fd_set fds;
> +
> +		FD_ZERO(&fds);
> +		FD_SET(0, &fds);
> +		FD_SET(display->drm_fd, &fds);
> +		ret = select(display->drm_fd + 1, &fds, NULL, NULL, &timeout);
> +		igt_assert(ret > 0);

Hm, we have igt_timeout which sends a signal and kills the test if the
timeout expires. And drm event reading should be interruptible. That might
be an even simpler way to implement this.
-Daniel

>  
>  		ret = read(display->drm_fd, buf, sizeof(buf));
>  		if (ret < 0 && (errno == EINTR || errno == EAGAIN))
> -- 
> 2.5.5
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 02/12] tests/kms_atomic_transition: don't assume max pipes

[Daniel Vetter]

On Mon, Nov 14, 2016 at 06:59:16PM +0900, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>  tests/kms_atomic_transition.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/kms_atomic_transition.c b/tests/kms_atomic_transition.c
> index e693c88..8b26b53 100644
> --- a/tests/kms_atomic_transition.c
> +++ b/tests/kms_atomic_transition.c
> @@ -404,7 +404,7 @@ static void run_modeset_tests(igt_display_t *display, int howmany, bool nonblock
>  {
>  	struct igt_fb fbs[2];
>  	int i, j;
> -	unsigned iter_max = 1 << I915_MAX_PIPES;
> +	unsigned iter_max = 1 << display->n_pipes;

Didn't Tomeu have some patch series to fix these all up?
-Daniel

>  	igt_pipe_crc_t *pipe_crcs[I915_MAX_PIPES];
>  	igt_output_t *output;
>  	unsigned width = 0, height = 0;
> -- 
> 2.5.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 04/12] lib/igt_kms: export properties names

[Daniel Vetter]

On Mon, Nov 14, 2016 at 06:59:18PM +0900, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

gtkdoc for anything exported would always be nice ...
-Daniel

> ---
>  lib/igt_kms.c | 6 +++---
>  lib/igt_kms.h | 5 +++++
>  2 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index aa9fd16..8aaff5b 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -153,7 +153,7 @@ const unsigned char* igt_kms_get_base_edid(void)
>  #define EDID_NAME alt_edid
>  #include "igt_edid_template.h"
>  
> -static const char *igt_plane_prop_names[IGT_NUM_PLANE_PROPS] = {
> +const char *igt_plane_prop_names[IGT_NUM_PLANE_PROPS] = {
>  	"SRC_X",
>  	"SRC_Y",
>  	"SRC_W",
> @@ -168,7 +168,7 @@ static const char *igt_plane_prop_names[IGT_NUM_PLANE_PROPS] = {
>  	"rotation"
>  };
>  
> -static const char *igt_crtc_prop_names[IGT_NUM_CRTC_PROPS] = {
> +const char *igt_crtc_prop_names[IGT_NUM_CRTC_PROPS] = {
>  	"background_color",
>  	"CTM",
>  	"DEGAMMA_LUT",
> @@ -177,7 +177,7 @@ static const char *igt_crtc_prop_names[IGT_NUM_CRTC_PROPS] = {
>  	"ACTIVE"
>  };
>  
> -static const char *igt_connector_prop_names[IGT_NUM_CONNECTOR_PROPS] = {
> +const char *igt_connector_prop_names[IGT_NUM_CONNECTOR_PROPS] = {
>  	"scaling mode",
>  	"CRTC_ID"
>  };
> diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> index 6422adc..ae2b505 100644
> --- a/lib/igt_kms.h
> +++ b/lib/igt_kms.h
> @@ -113,12 +113,16 @@ enum igt_atomic_crtc_properties {
>         IGT_NUM_CRTC_PROPS
>  };
>  
> +extern const char *igt_crtc_prop_names[];
> +
>  enum igt_atomic_connector_properties {
>         IGT_CONNECTOR_SCALING_MODE = 0,
>         IGT_CONNECTOR_CRTC_ID,
>         IGT_NUM_CONNECTOR_PROPS
>  };
>  
> +extern const char *igt_connector_prop_names[];
> +
>  struct kmstest_connector_config {
>  	drmModeCrtc *crtc;
>  	drmModeConnector *connector;
> @@ -214,6 +218,7 @@ enum igt_atomic_plane_properties {
>         IGT_NUM_PLANE_PROPS
>  };
>  
> +extern const char *igt_plane_prop_names[];
>  
>  typedef struct igt_display igt_display_t;
>  typedef struct igt_pipe igt_pipe_t;
> -- 
> 2.5.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 08/12] tests/kms_atomic: stress possible fence settings

[Daniel Vetter]

On Mon, Nov 14, 2016 at 06:59:22PM +0900, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>  tests/kms_atomic.c | 124 +++++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 115 insertions(+), 9 deletions(-)
> 
> diff --git a/tests/kms_atomic.c b/tests/kms_atomic.c
> index 8b648eb..58fc0dd 100644
> --- a/tests/kms_atomic.c
> +++ b/tests/kms_atomic.c
> @@ -44,6 +44,7 @@
>  #include "drmtest.h"
>  #include "igt.h"
>  #include "igt_aux.h"
> +#include "sw_sync.h"
>  
>  #ifndef DRM_CLIENT_CAP_ATOMIC
>  #define DRM_CLIENT_CAP_ATOMIC 3
> @@ -126,6 +127,7 @@ struct kms_atomic_plane_state {
>  	uint32_t fb_id; /* 0 to disable */
>  	uint32_t src_x, src_y, src_w, src_h; /* 16.16 fixed-point */
>  	uint32_t crtc_x, crtc_y, crtc_w, crtc_h; /* normal integers */
> +	int32_t fence_fd;
>  };
>  
>  struct kms_atomic_crtc_state {
> @@ -133,6 +135,7 @@ struct kms_atomic_crtc_state {
>  	uint32_t obj;
>  	int idx;
>  	bool active;
> +	uint64_t out_fence_ptr;
>  	struct kms_atomic_blob mode;
>  };
>  
> @@ -190,11 +193,13 @@ static uint32_t blob_duplicate(int fd, uint32_t id_orig)
>  	crtc_populate_req(crtc, req); \
>  	plane_populate_req(plane, req); \
>  	do_atomic_commit((crtc)->state->desc->fd, req, flags); \
> -	crtc_check_current_state(crtc, plane, relax); \
> -	plane_check_current_state(plane, relax); \
> +	if (!(flags & DRM_MODE_ATOMIC_TEST_ONLY)) { \
> +		crtc_check_current_state(crtc, plane, relax); \
> +		plane_check_current_state(plane, relax); \
> +	} \
>  }
>  
> -#define crtc_commit_atomic_err(crtc, plane, crtc_old, plane_old, req, relax, e) { \
> +#define crtc_commit_atomic_err(crtc, plane, crtc_old, plane_old, req, flags, relax, e) { \
>  	drmModeAtomicSetCursor(req, 0); \
>  	crtc_populate_req(crtc, req); \
>  	plane_populate_req(plane, req); \
> @@ -299,6 +304,9 @@ find_connector(struct kms_atomic_state *state,
>  static void plane_populate_req(struct kms_atomic_plane_state *plane,
>  			       drmModeAtomicReq *req)
>  {
> +	if (plane->fence_fd)
> +		plane_set_prop(req, plane, IGT_PLANE_IN_FENCE_FD, plane->fence_fd);
> +
>  	plane_set_prop(req, plane, IGT_PLANE_CRTC_ID, plane->crtc_id);
>  	plane_set_prop(req, plane, IGT_PLANE_FB_ID, plane->fb_id);
>  	plane_set_prop(req, plane, IGT_PLANE_SRC_X, plane->src_x);
> @@ -424,6 +432,10 @@ find_plane(struct kms_atomic_state *state, enum plane_type type,
>  static void crtc_populate_req(struct kms_atomic_crtc_state *crtc,
>  			      drmModeAtomicReq *req)
>  {
> +	if (crtc->out_fence_ptr)
> +		crtc_set_prop(req, crtc, IGT_CRTC_OUT_FENCE_PTR,
> +			      crtc->out_fence_ptr);
> +
>  	crtc_set_prop(req, crtc, IGT_CRTC_MODE_ID, crtc->mode.id);
>  	crtc_set_prop(req, crtc, IGT_CRTC_ACTIVE, crtc->active);
>  }
> @@ -986,6 +998,7 @@ static void plane_invalid_params(struct kms_atomic_crtc_state *crtc,
>  	uint32_t format = plane_get_igt_format(&plane);
>  	drmModeAtomicReq *req = drmModeAtomicAlloc();
>  	struct igt_fb fb;
> +	int timeline, fence_fd;
>  
>  	/* Pass a series of invalid object IDs for the FB ID. */
>  	plane.fb_id = plane.obj;
> @@ -1024,6 +1037,23 @@ static void plane_invalid_params(struct kms_atomic_crtc_state *crtc,
>  	plane_commit_atomic_err(&plane, plane_old, req,
>  	                        ATOMIC_RELAX_NONE, EINVAL);
>  
> +	/* invalid fence fd */
> +	plane.fence_fd = plane.state->desc->fd;
> +	plane.crtc_id = plane_old->crtc_id;
> +	plane_commit_atomic_err(&plane, plane_old, req,
> +	                        ATOMIC_RELAX_NONE, EINVAL);
> +
> +	/* Valid fence_fd but invalid CRTC */
> +	timeline = sw_sync_timeline_create();
> +	fence_fd =  sw_sync_fence_create(timeline, 1);
> +	plane.fence_fd = fence_fd;
> +	plane.crtc_id = ~0U;
> +	plane_commit_atomic_err(&plane, plane_old, req,
> +	                        ATOMIC_RELAX_NONE, EINVAL);
> +	close(fence_fd);
> +	close(timeline);
> +
> +	plane.fence_fd = -1;
>  	plane.crtc_id = plane_old->crtc_id;
>  	plane_commit_atomic(&plane, req, ATOMIC_RELAX_NONE);
>  
> @@ -1058,27 +1088,98 @@ static void crtc_invalid_params(struct kms_atomic_crtc_state *crtc_old,
>  {
>  	struct kms_atomic_crtc_state crtc = *crtc_old;
>  	drmModeAtomicReq *req = drmModeAtomicAlloc();
> +	int timeline, fence_fd, *out_fence;
>  
>  	igt_assert(req);
>  
>  	/* Pass a series of invalid object IDs for the mode ID. */
>  	crtc.mode.id = plane->obj;
> -	crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req,
> +	crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req, 0,
>  	                       ATOMIC_RELAX_NONE, EINVAL);
>  
>  	crtc.mode.id = crtc.obj;
> -	crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req,
> +	crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req, 0,
>  	                       ATOMIC_RELAX_NONE, EINVAL);
>  
>  	crtc.mode.id = conn->obj;
> -	crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req,
> +	crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req, 0,
>  	                       ATOMIC_RELAX_NONE, EINVAL);
>  
>  	crtc.mode.id = plane->fb_id;
> -	crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req,
> +	crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req, 0,
>  	                       ATOMIC_RELAX_NONE, EINVAL);
>  
> +	/* invalid out_fence_ptr */
> +	crtc.mode.id = crtc_old->mode.id;
> +	crtc.out_fence_ptr = (uint64_t) crtc_invalid_params;
> +	crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req, 0,
> +	                       ATOMIC_RELAX_NONE, EFAULT);
> +
> +	/* invalid out_fence_ptr */
> +	crtc.mode.id = crtc_old->mode.id;
> +	crtc.out_fence_ptr = (uint64_t) 0x8;
> +	crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req, 0,
> +	                       ATOMIC_RELAX_NONE, EFAULT);
> +	crtc.out_fence_ptr = (uint64_t) 0;
> +
> +	/* valid in fence but not allowed prop on crtc */
> +	timeline = sw_sync_timeline_create();
> +	fence_fd =  sw_sync_fence_create(timeline, 1);
> +	plane->fence_fd = fence_fd;
> +	crtc.active = false;
> +	crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req, 0,
> +			       ATOMIC_RELAX_NONE, EINVAL);
> +
> +	out_fence = malloc(sizeof(uint64_t));
> +	igt_assert(out_fence);
> +
> +
> +	/* valid out fence ptr and flip event but not allowed prop on crtc */
> +	crtc.out_fence_ptr = (uint64_t) out_fence;
> +	crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req, 0,
> +			       ATOMIC_RELAX_NONE, EINVAL);
> +
> +	/* valid out fence ptr and flip event but not allowed prop on crtc */
> +	crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req,
> +			       DRM_MODE_PAGE_FLIP_EVENT,
> +			       ATOMIC_RELAX_NONE, EINVAL);
> +
> +	/* valid page flip event but not allowed prop on crtc */
> +	crtc.out_fence_ptr = (uint64_t) 0;
> +	crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req,
> +			       DRM_MODE_PAGE_FLIP_EVENT,
> +			       ATOMIC_RELAX_NONE, EINVAL);
> +	crtc.active = true;
> +
> +	/* valid out fence  ptr and flip event but invalid prop on crtc */
> +	crtc.out_fence_ptr = (uint64_t) out_fence;
> +	crtc.mode.id = plane->fb_id;
> +	crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req, 0,
> +			       ATOMIC_RELAX_NONE, EINVAL);
> +
> +	/* valid out fence ptr and flip event but invalid prop on crtc */
> +	crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req,
> +			       DRM_MODE_PAGE_FLIP_EVENT,
> +			       ATOMIC_RELAX_NONE, EINVAL);
> +
> +	/* valid page flip event but invalid prop on crtc */
> +	crtc.out_fence_ptr = (uint64_t) 0;
> +	crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req,
> +			       DRM_MODE_PAGE_FLIP_EVENT,
> +			       ATOMIC_RELAX_NONE, EINVAL);
> +
> +	/* successful TEST_ONLY with fences set */
> +	plane->fence_fd = fence_fd;

Smashing all invalid tests into one function&subtest is imo bad style.
gem_exec_params.c is probably overkill in splitting stuff up, but for this
here at least making some groups of invalid tests as subgroups would be
good imo.

E.g. replacing all your comments with somewhat meaningful subtests names
also helps with making things self-documenting a bit more. Old rule of
thumb that if you write a comment explaining what the code does, it needs
to be refactored to make it obvious what it does.
-Daniel

>  	crtc.mode.id = crtc_old->mode.id;
> +	crtc_commit_atomic(&crtc, plane, req, ATOMIC_RELAX_NONE,
> +			   DRM_MODE_ATOMIC_TEST_ONLY);
> +	igt_assert(*out_fence == -1);
> +	close(fence_fd);
> +	close(timeline);
> +
> +	/* reset fences */
> +	plane->fence_fd = -1;
> +	crtc.out_fence_ptr = (uint64_t) 0;
>  	crtc_commit_atomic(&crtc, plane, req, ATOMIC_RELAX_NONE, 0);
>  
>  	/* Create a blob which is the wrong size to be a valid mode. */
> @@ -1086,7 +1187,7 @@ static void crtc_invalid_params(struct kms_atomic_crtc_state *crtc_old,
>  					    crtc.mode.data,
>  					    sizeof(struct drm_mode_modeinfo) - 1,
>  					    &crtc.mode.id));
> -	crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req,
> +	crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req, 0,
>  	                       ATOMIC_RELAX_NONE, EINVAL);
>  
>  
> @@ -1094,12 +1195,17 @@ static void crtc_invalid_params(struct kms_atomic_crtc_state *crtc_old,
>  					    crtc.mode.data,
>  					    sizeof(struct drm_mode_modeinfo) + 1,
>  					    &crtc.mode.id));
> -	crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req,
> +	crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req, 0,
>  	                       ATOMIC_RELAX_NONE, EINVAL);
>  
> +	/* out fence ptr but not page flip event */
> +	crtc.out_fence_ptr = (uint64_t) out_fence;
> +	crtc_commit_atomic(crtc_old, plane, req, ATOMIC_RELAX_NONE, 0);
> +
>  	/* Restore the CRTC and check the state matches the old. */
>  	crtc_commit_atomic(crtc_old, plane, req, ATOMIC_RELAX_NONE, 0);
>  
> +	free(out_fence);
>  	drmModeAtomicFree(req);
>  }
>  
> -- 
> 2.5.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/12] tests/kms_atomic_transition: don't assume max pipes

[Tomeu Vizoso]

On 15 November 2016 at 09:01, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, Nov 14, 2016 at 06:59:16PM +0900, Gustavo Padovan wrote:
>> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>>
>> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>> ---
>>  tests/kms_atomic_transition.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tests/kms_atomic_transition.c b/tests/kms_atomic_transition.c
>> index e693c88..8b26b53 100644
>> --- a/tests/kms_atomic_transition.c
>> +++ b/tests/kms_atomic_transition.c
>> @@ -404,7 +404,7 @@ static void run_modeset_tests(igt_display_t *display, int howmany, bool nonblock
>>  {
>>       struct igt_fb fbs[2];
>>       int i, j;
>> -     unsigned iter_max = 1 << I915_MAX_PIPES;
>> +     unsigned iter_max = 1 << display->n_pipes;
>
> Didn't Tomeu have some patch series to fix these all up?

Don't remember, and couldn't find any within my local branches. Maybe
Robert? But I'm adding it to my backlog anyway.

Regards,

Tomeu

> -Daniel
>
>>       igt_pipe_crc_t *pipe_crcs[I915_MAX_PIPES];
>>       igt_output_t *output;
>>       unsigned width = 0, height = 0;
>> --
>> 2.5.5
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 02/12] tests/kms_atomic_transition: don't assume max pipes

[Robert Foss]

On Tue, 2016-11-15 at 14:25 +0100, Tomeu Vizoso wrote:
> On 15 November 2016 at 09:01, Daniel Vetter <daniel@ffwll.ch> wrote:
> > 
> > On Mon, Nov 14, 2016 at 06:59:16PM +0900, Gustavo Padovan wrote:
> > > 
> > > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > > 
> > > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > > ---
> > >  tests/kms_atomic_transition.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/tests/kms_atomic_transition.c
> > > b/tests/kms_atomic_transition.c
> > > index e693c88..8b26b53 100644
> > > --- a/tests/kms_atomic_transition.c
> > > +++ b/tests/kms_atomic_transition.c
> > > @@ -404,7 +404,7 @@ static void run_modeset_tests(igt_display_t
> > > *display, int howmany, bool nonblock
> > >  {
> > >       struct igt_fb fbs[2];
> > >       int i, j;
> > > -     unsigned iter_max = 1 << I915_MAX_PIPES;
> > > +     unsigned iter_max = 1 << display->n_pipes;
> > Didn't Tomeu have some patch series to fix these all up?
> Don't remember, and couldn't find any within my local branches. Maybe
> Robert? But I'm adding it to my backlog anyway.
> 

I don't recognize it, but thanks tomeu!

Rob.

> 
> > 
> > -Daniel
> > 
> > > 
> > >       igt_pipe_crc_t *pipe_crcs[I915_MAX_PIPES];
> > >       igt_output_t *output;
> > >       unsigned width = 0, height = 0;
> > > --
> > > 2.5.5
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 07/12] lib/igt_kms: Add support for the OUT_FENCE_PTR property

[Brian Starkey]

Hi Gustavo,

A little late to the party here, but I was blocked by our internal
contributions process...

I didn't see these end up in my checkout yet though, so I guess they
aren't picked up yet.

On Mon, Nov 14, 2016 at 06:59:21PM +0900, Gustavo Padovan wrote:
>From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>
>Add support for the OUT_FENCE_PTR property to enable setting out fences for
>atomic commits.
>
>Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>---
> lib/igt_kms.c | 20 +++++++++++++++++++-
> lib/igt_kms.h |  3 +++
> 2 files changed, 22 insertions(+), 1 deletion(-)
>
>diff --git a/lib/igt_kms.c b/lib/igt_kms.c
>index 4748c0a..f25e1eb 100644
>--- a/lib/igt_kms.c
>+++ b/lib/igt_kms.c
>@@ -175,7 +175,8 @@ const char *igt_crtc_prop_names[IGT_NUM_CRTC_PROPS] = {
> 	"DEGAMMA_LUT",
> 	"GAMMA_LUT",
> 	"MODE_ID",
>-	"ACTIVE"
>+	"ACTIVE",
>+	"OUT_FENCE_PTR"
> };
>
> const char *igt_connector_prop_names[IGT_NUM_CONNECTOR_PROPS] = {
>@@ -2103,6 +2104,9 @@ static void igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicRe
> 		igt_atomic_populate_crtc_req(req, pipe_obj, IGT_CRTC_ACTIVE, !!output);
> 	}
>
>+	if (pipe_obj->out_fence_ptr)
>+		igt_atomic_populate_crtc_req(req, pipe_obj, IGT_CRTC_OUT_FENCE_PTR, pipe_obj->out_fence_ptr);
>+
> 	/*
> 	 *	TODO: Add all crtc level properties here
> 	 */
>@@ -2683,6 +2687,20 @@ igt_pipe_set_gamma_lut(igt_pipe_t *pipe, void *ptr, size_t length)
> }
>
> /**
>+ * igt_pipe_set_out_fence_ptr:
>+ * @pipe: pipe pointer to which background color to be set
>+ * @fence_ptr: out fence pointer

I don't think fence_ptr can be int *. It needs to be a pointer to a
64-bit type.

>+ *
>+ * Sets the out fence pointer that will be passed to the kernel in
>+ * the atomic ioctl. When the kernel returns the out fence pointer
>+ * will contain the fd number of the out fence created by KMS.
>+ */
>+void igt_pipe_set_out_fence_ptr(igt_pipe_t *pipe, int *fence_ptr)
>+{
>+	pipe->out_fence_ptr = (uint64_t) fence_ptr;
>+}
>+
>+/**
>  * igt_crtc_set_background:
>  * @pipe: pipe pointer to which background color to be set
>  * @background: background color value in BGR 16bpc
>diff --git a/lib/igt_kms.h b/lib/igt_kms.h
>index 344f931..02d7bd1 100644
>--- a/lib/igt_kms.h
>+++ b/lib/igt_kms.h
>@@ -110,6 +110,7 @@ enum igt_atomic_crtc_properties {
>        IGT_CRTC_GAMMA_LUT,
>        IGT_CRTC_MODE_ID,
>        IGT_CRTC_ACTIVE,
>+       IGT_CRTC_OUT_FENCE_PTR,
>        IGT_NUM_CRTC_PROPS
> };
>
>@@ -298,6 +299,7 @@ struct igt_pipe {
>
> 	uint64_t mode_blob;
> 	bool mode_changed;
>+	uint64_t out_fence_ptr;

IMO this should be:

	int64_t *out_fence_ptr;

That way there can be no confusion about the type. You can convert it
to u64 just before giving it to the kernel.

Thanks,
-Brian

> };
>
> typedef struct {
>@@ -351,6 +353,7 @@ static inline bool igt_plane_supports_rotation(igt_plane_t *plane)
> void igt_pipe_set_degamma_lut(igt_pipe_t *pipe, void *ptr, size_t length);
> void igt_pipe_set_ctm_matrix(igt_pipe_t *pipe, void *ptr, size_t length);
> void igt_pipe_set_gamma_lut(igt_pipe_t *pipe, void *ptr, size_t length);
>+void igt_pipe_set_out_fence_ptr(igt_pipe_t *pipe, int *fence_ptr);
>
> void igt_plane_set_fb(igt_plane_t *plane, struct igt_fb *fb);
> void igt_plane_set_fence_fd(igt_plane_t *plane, uint32_t fence_fd);
>-- 
>2.5.5
>
>_______________________________________________
>Intel-gfx mailing list
>Intel-gfx@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 07/12] lib/igt_kms: Add support for the OUT_FENCE_PTR property

[Chris Wilson]

On Tue, Nov 22, 2016 at 10:53:51AM +0000, Brian Starkey wrote:
> Hi Gustavo,
> 
> A little late to the party here, but I was blocked by our internal
> contributions process...
> 
> I didn't see these end up in my checkout yet though, so I guess they
> aren't picked up yet.
> 
> On Mon, Nov 14, 2016 at 06:59:21PM +0900, Gustavo Padovan wrote:
> >From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> >
> >Add support for the OUT_FENCE_PTR property to enable setting out fences for
> >atomic commits.
> >
> >Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> >---
> >lib/igt_kms.c | 20 +++++++++++++++++++-
> >lib/igt_kms.h |  3 +++
> >2 files changed, 22 insertions(+), 1 deletion(-)
> >
> >diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> >index 4748c0a..f25e1eb 100644
> >--- a/lib/igt_kms.c
> >+++ b/lib/igt_kms.c
> >@@ -175,7 +175,8 @@ const char *igt_crtc_prop_names[IGT_NUM_CRTC_PROPS] = {
> >	"DEGAMMA_LUT",
> >	"GAMMA_LUT",
> >	"MODE_ID",
> >-	"ACTIVE"
> >+	"ACTIVE",
> >+	"OUT_FENCE_PTR"
> >};
> >
> >const char *igt_connector_prop_names[IGT_NUM_CONNECTOR_PROPS] = {
> >@@ -2103,6 +2104,9 @@ static void igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicRe
> >		igt_atomic_populate_crtc_req(req, pipe_obj, IGT_CRTC_ACTIVE, !!output);
> >	}
> >
> >+	if (pipe_obj->out_fence_ptr)
> >+		igt_atomic_populate_crtc_req(req, pipe_obj, IGT_CRTC_OUT_FENCE_PTR, pipe_obj->out_fence_ptr);
> >+
> >	/*
> >	 *	TODO: Add all crtc level properties here
> >	 */
> >@@ -2683,6 +2687,20 @@ igt_pipe_set_gamma_lut(igt_pipe_t *pipe, void *ptr, size_t length)
> >}
> >
> >/**
> >+ * igt_pipe_set_out_fence_ptr:
> >+ * @pipe: pipe pointer to which background color to be set
> >+ * @fence_ptr: out fence pointer
> 
> I don't think fence_ptr can be int *. It needs to be a pointer to a
> 64-bit type.
> 
> >+ *
> >+ * Sets the out fence pointer that will be passed to the kernel in
> >+ * the atomic ioctl. When the kernel returns the out fence pointer
> >+ * will contain the fd number of the out fence created by KMS.
> >+ */
> >+void igt_pipe_set_out_fence_ptr(igt_pipe_t *pipe, int *fence_ptr)
> >+{
> >+	pipe->out_fence_ptr = (uint64_t) fence_ptr;
> >+}
> >+
> >+/**
> > * igt_crtc_set_background:
> > * @pipe: pipe pointer to which background color to be set
> > * @background: background color value in BGR 16bpc
> >diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> >index 344f931..02d7bd1 100644
> >--- a/lib/igt_kms.h
> >+++ b/lib/igt_kms.h
> >@@ -110,6 +110,7 @@ enum igt_atomic_crtc_properties {
> >       IGT_CRTC_GAMMA_LUT,
> >       IGT_CRTC_MODE_ID,
> >       IGT_CRTC_ACTIVE,
> >+       IGT_CRTC_OUT_FENCE_PTR,
> >       IGT_NUM_CRTC_PROPS
> >};
> >
> >@@ -298,6 +299,7 @@ struct igt_pipe {
> >
> >	uint64_t mode_blob;
> >	bool mode_changed;
> >+	uint64_t out_fence_ptr;
> 
> IMO this should be:
> 
> 	int64_t *out_fence_ptr;

In userspace, fences are *fd*, a plain int. It is only the uabi that we
pass pointers as u64 to the kernel, and indeed that should be limited to
the uabi wrapper.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 06/12] lib/igt_kms: Add support for the IN_FENCE_FD property

[Brian Starkey]

Hi,

On Mon, Nov 14, 2016 at 06:59:20PM +0900, Gustavo Padovan wrote:
>From: Robert Foss <robert.foss@collabora.com>
>
>Add support dor the IN_FENCE_FD property to enable setting in fences for atomic
>commits.
>
>Signed-off-by: Robert Foss <robert.foss@collabora.com>
>---
> lib/igt_kms.c | 20 ++++++++++++++++++++
> lib/igt_kms.h |  5 +++++
> 2 files changed, 25 insertions(+)
>
>diff --git a/lib/igt_kms.c b/lib/igt_kms.c
>index 8aaff5b..4748c0a 100644
>--- a/lib/igt_kms.c
>+++ b/lib/igt_kms.c
>@@ -164,6 +164,7 @@ const char *igt_plane_prop_names[IGT_NUM_PLANE_PROPS] = {
> 	"CRTC_H",
> 	"FB_ID",
> 	"CRTC_ID",
>+	"IN_FENCE_FD",
> 	"type",
> 	"rotation"
> };
>@@ -1426,6 +1427,7 @@ void igt_display_init(igt_display_t *display, int drm_fd)
> 			n_planes++;
> 			plane->pipe = pipe;
> 			plane->drm_plane = drm_plane;
>+			plane->fence_fd = -1;
>
> 			if (is_atomic == 0) {
> 				display->is_atomic = 1;
>@@ -1712,6 +1714,11 @@ igt_atomic_prepare_plane_commit(igt_plane_t *plane, igt_pipe_t *pipe,
> 	    plane->index,
> 	    fb_id);
>
>+	if (plane->fence_fd >= 0) {
>+		uint32_t fence_fd = plane->fence_fd;

Assigning to uint32_t here will make the top bytes zero when it gets
cast to uint64_t. I guess that works out fine because the cast back to
int in the kernel will be 32-bits, but IMO better to cast to int64_t
here to get proper sign-extension to 64-bits.

>+		igt_atomic_populate_plane_req(req, plane, IGT_PLANE_IN_FENCE_FD, fence_fd);
>+	}
>+
> 	if (plane->fb_changed) {
> 		igt_atomic_populate_plane_req(req, plane, IGT_PLANE_CRTC_ID, fb_id ? crtc_id : 0);
> 		igt_atomic_populate_plane_req(req, plane, IGT_PLANE_FB_ID, fb_id);
>@@ -2522,6 +2529,19 @@ void igt_plane_set_fb(igt_plane_t *plane, struct igt_fb *fb)
> 	plane->size_changed = true;
> }
>
>+/**
>+ * igt_plane_set_fence_fd:
>+ * @plane: plane
>+ * @fence_fd: fence fd, disable fence_fd by setting it to 0

Should be -1 to disable.

Cheers,
Brian

>+ *
>+ * This function sets a fence fd to enable a commit to wait for some event to
>+ * occur before completing.
>+ */
>+void igt_plane_set_fence_fd(igt_plane_t *plane, uint32_t fence_fd)
>+{
>+	plane->fence_fd = fence_fd;
>+}
>+
> void igt_plane_set_position(igt_plane_t *plane, int x, int y)
> {
> 	igt_pipe_t *pipe = plane->pipe;
>diff --git a/lib/igt_kms.h b/lib/igt_kms.h
>index ae2b505..344f931 100644
>--- a/lib/igt_kms.h
>+++ b/lib/igt_kms.h
>@@ -213,6 +213,7 @@ enum igt_atomic_plane_properties {
>
>        IGT_PLANE_FB_ID,
>        IGT_PLANE_CRTC_ID,
>+       IGT_PLANE_IN_FENCE_FD,
>        IGT_PLANE_TYPE,
>        IGT_PLANE_ROTATION,
>        IGT_NUM_PLANE_PROPS
>@@ -266,6 +267,9 @@ typedef struct {
> 	uint32_t src_h;
>
> 	igt_rotation_t rotation;
>+
>+	/* in fence fd */
>+	int32_t fence_fd;
> 	uint32_t atomic_props_plane[IGT_NUM_PLANE_PROPS];
> } igt_plane_t;
>
>@@ -349,6 +353,7 @@ void igt_pipe_set_ctm_matrix(igt_pipe_t *pipe, void *ptr, size_t length);
> void igt_pipe_set_gamma_lut(igt_pipe_t *pipe, void *ptr, size_t length);
>
> void igt_plane_set_fb(igt_plane_t *plane, struct igt_fb *fb);
>+void igt_plane_set_fence_fd(igt_plane_t *plane, uint32_t fence_fd);
> void igt_plane_set_position(igt_plane_t *plane, int x, int y);
> void igt_plane_set_size(igt_plane_t *plane, int w, int h);
> void igt_plane_set_rotation(igt_plane_t *plane, igt_rotation_t rotation);
>-- 
>2.5.5
>
>_______________________________________________
>dri-devel mailing list
>dri-devel@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 07/12] lib/igt_kms: Add support for the OUT_FENCE_PTR property

[Brian Starkey]

On Tue, Nov 22, 2016 at 11:06:00AM +0000, Chris Wilson wrote:
>On Tue, Nov 22, 2016 at 10:53:51AM +0000, Brian Starkey wrote:
>> Hi Gustavo,
>>
>> A little late to the party here, but I was blocked by our internal
>> contributions process...
>>
>> I didn't see these end up in my checkout yet though, so I guess they
>> aren't picked up yet.
>>
>> On Mon, Nov 14, 2016 at 06:59:21PM +0900, Gustavo Padovan wrote:
>> >From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>> >
>> >Add support for the OUT_FENCE_PTR property to enable setting out fences for
>> >atomic commits.
>> >
>> >Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>> >---
>> >lib/igt_kms.c | 20 +++++++++++++++++++-
>> >lib/igt_kms.h |  3 +++
>> >2 files changed, 22 insertions(+), 1 deletion(-)
>> >
>> >diff --git a/lib/igt_kms.c b/lib/igt_kms.c
>> >index 4748c0a..f25e1eb 100644
>> >--- a/lib/igt_kms.c
>> >+++ b/lib/igt_kms.c
>> >@@ -175,7 +175,8 @@ const char *igt_crtc_prop_names[IGT_NUM_CRTC_PROPS] = {
>> >	"DEGAMMA_LUT",
>> >	"GAMMA_LUT",
>> >	"MODE_ID",
>> >-	"ACTIVE"
>> >+	"ACTIVE",
>> >+	"OUT_FENCE_PTR"
>> >};
>> >
>> >const char *igt_connector_prop_names[IGT_NUM_CONNECTOR_PROPS] = {
>> >@@ -2103,6 +2104,9 @@ static void igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicRe
>> >		igt_atomic_populate_crtc_req(req, pipe_obj, IGT_CRTC_ACTIVE, !!output);
>> >	}
>> >
>> >+	if (pipe_obj->out_fence_ptr)
>> >+		igt_atomic_populate_crtc_req(req, pipe_obj, IGT_CRTC_OUT_FENCE_PTR, pipe_obj->out_fence_ptr);
>> >+
>> >	/*
>> >	 *	TODO: Add all crtc level properties here
>> >	 */
>> >@@ -2683,6 +2687,20 @@ igt_pipe_set_gamma_lut(igt_pipe_t *pipe, void *ptr, size_t length)
>> >}
>> >
>> >/**
>> >+ * igt_pipe_set_out_fence_ptr:
>> >+ * @pipe: pipe pointer to which background color to be set
>> >+ * @fence_ptr: out fence pointer
>>
>> I don't think fence_ptr can be int *. It needs to be a pointer to a
>> 64-bit type.
>>
>> >+ *
>> >+ * Sets the out fence pointer that will be passed to the kernel in
>> >+ * the atomic ioctl. When the kernel returns the out fence pointer
>> >+ * will contain the fd number of the out fence created by KMS.
>> >+ */
>> >+void igt_pipe_set_out_fence_ptr(igt_pipe_t *pipe, int *fence_ptr)
>> >+{
>> >+	pipe->out_fence_ptr = (uint64_t) fence_ptr;
>> >+}
>> >+
>> >+/**
>> > * igt_crtc_set_background:
>> > * @pipe: pipe pointer to which background color to be set
>> > * @background: background color value in BGR 16bpc
>> >diff --git a/lib/igt_kms.h b/lib/igt_kms.h
>> >index 344f931..02d7bd1 100644
>> >--- a/lib/igt_kms.h
>> >+++ b/lib/igt_kms.h
>> >@@ -110,6 +110,7 @@ enum igt_atomic_crtc_properties {
>> >       IGT_CRTC_GAMMA_LUT,
>> >       IGT_CRTC_MODE_ID,
>> >       IGT_CRTC_ACTIVE,
>> >+       IGT_CRTC_OUT_FENCE_PTR,
>> >       IGT_NUM_CRTC_PROPS
>> >};
>> >
>> >@@ -298,6 +299,7 @@ struct igt_pipe {
>> >
>> >	uint64_t mode_blob;
>> >	bool mode_changed;
>> >+	uint64_t out_fence_ptr;
>>
>> IMO this should be:
>>
>> 	int64_t *out_fence_ptr;
>
>In userspace, fences are *fd*, a plain int. It is only the uabi that we
>pass pointers as u64 to the kernel, and indeed that should be limited to
>the uabi wrapper.
>-Chris

Where's the uabi wrapper in this case?

Wherever it is, afaik someone needs to have 64-bit type for the kernel
to stash its fd in - on the kernel side out_fence_ptr is
(s64 __user *), so if there's not a 64-bit variable on the other end
of it then someone's going to have a bad day.

-Brian

>
>-- 
>Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 07/12] lib/igt_kms: Add support for the OUT_FENCE_PTR property

[Chris Wilson]

On Tue, Nov 22, 2016 at 11:54:57AM +0000, Brian Starkey wrote:
> On Tue, Nov 22, 2016 at 11:06:00AM +0000, Chris Wilson wrote:
> >On Tue, Nov 22, 2016 at 10:53:51AM +0000, Brian Starkey wrote:
> >>Hi Gustavo,
> >>
> >>A little late to the party here, but I was blocked by our internal
> >>contributions process...
> >>
> >>I didn't see these end up in my checkout yet though, so I guess they
> >>aren't picked up yet.
> >>
> >>On Mon, Nov 14, 2016 at 06:59:21PM +0900, Gustavo Padovan wrote:
> >>>From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> >>>
> >>>Add support for the OUT_FENCE_PTR property to enable setting out fences for
> >>>atomic commits.
> >>>
> >>>Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> >>>---
> >>>lib/igt_kms.c | 20 +++++++++++++++++++-
> >>>lib/igt_kms.h |  3 +++
> >>>2 files changed, 22 insertions(+), 1 deletion(-)
> >>>
> >>>diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> >>>index 4748c0a..f25e1eb 100644
> >>>--- a/lib/igt_kms.c
> >>>+++ b/lib/igt_kms.c
> >>>@@ -175,7 +175,8 @@ const char *igt_crtc_prop_names[IGT_NUM_CRTC_PROPS] = {
> >>>	"DEGAMMA_LUT",
> >>>	"GAMMA_LUT",
> >>>	"MODE_ID",
> >>>-	"ACTIVE"
> >>>+	"ACTIVE",
> >>>+	"OUT_FENCE_PTR"
> >>>};
> >>>
> >>>const char *igt_connector_prop_names[IGT_NUM_CONNECTOR_PROPS] = {
> >>>@@ -2103,6 +2104,9 @@ static void igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicRe
> >>>		igt_atomic_populate_crtc_req(req, pipe_obj, IGT_CRTC_ACTIVE, !!output);
> >>>	}
> >>>
> >>>+	if (pipe_obj->out_fence_ptr)
> >>>+		igt_atomic_populate_crtc_req(req, pipe_obj, IGT_CRTC_OUT_FENCE_PTR, pipe_obj->out_fence_ptr);
> >>>+
> >>>	/*
> >>>	 *	TODO: Add all crtc level properties here
> >>>	 */
> >>>@@ -2683,6 +2687,20 @@ igt_pipe_set_gamma_lut(igt_pipe_t *pipe, void *ptr, size_t length)
> >>>}
> >>>
> >>>/**
> >>>+ * igt_pipe_set_out_fence_ptr:
> >>>+ * @pipe: pipe pointer to which background color to be set
> >>>+ * @fence_ptr: out fence pointer
> >>
> >>I don't think fence_ptr can be int *. It needs to be a pointer to a
> >>64-bit type.
> >>
> >>>+ *
> >>>+ * Sets the out fence pointer that will be passed to the kernel in
> >>>+ * the atomic ioctl. When the kernel returns the out fence pointer
> >>>+ * will contain the fd number of the out fence created by KMS.
> >>>+ */
> >>>+void igt_pipe_set_out_fence_ptr(igt_pipe_t *pipe, int *fence_ptr)
> >>>+{
> >>>+	pipe->out_fence_ptr = (uint64_t) fence_ptr;
> >>>+}
> >>>+
> >>>+/**
> >>> * igt_crtc_set_background:
> >>> * @pipe: pipe pointer to which background color to be set
> >>> * @background: background color value in BGR 16bpc
> >>>diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> >>>index 344f931..02d7bd1 100644
> >>>--- a/lib/igt_kms.h
> >>>+++ b/lib/igt_kms.h
> >>>@@ -110,6 +110,7 @@ enum igt_atomic_crtc_properties {
> >>>       IGT_CRTC_GAMMA_LUT,
> >>>       IGT_CRTC_MODE_ID,
> >>>       IGT_CRTC_ACTIVE,
> >>>+       IGT_CRTC_OUT_FENCE_PTR,
> >>>       IGT_NUM_CRTC_PROPS
> >>>};
> >>>
> >>>@@ -298,6 +299,7 @@ struct igt_pipe {
> >>>
> >>>	uint64_t mode_blob;
> >>>	bool mode_changed;
> >>>+	uint64_t out_fence_ptr;
> >>
> >>IMO this should be:
> >>
> >>	int64_t *out_fence_ptr;
> >
> >In userspace, fences are *fd*, a plain int. It is only the uabi that we
> >pass pointers as u64 to the kernel, and indeed that should be limited to
> >the uabi wrapper.
> >-Chris
> 
> Where's the uabi wrapper in this case?
> 
> Wherever it is, afaik someone needs to have 64-bit type for the kernel
> to stash its fd in - on the kernel side out_fence_ptr is
> (s64 __user *), so if there's not a 64-bit variable on the other end
> of it then someone's going to have a bad day.

We do not have pointers in the uabi because they are different sizes on
different platforms. The uabi must be a u64 representation of a user
address to store the result - that is what we pass to the crtc set
property ioctl. That it has been futher managled not to pass around fd
is an interesting twist, but ideally that sillyness should not make
itself into our API.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 07/12] lib/igt_kms: Add support for the OUT_FENCE_PTR property

[Brian Starkey]

On Tue, Nov 22, 2016 at 12:10:52PM +0000, Chris Wilson wrote:
>On Tue, Nov 22, 2016 at 11:54:57AM +0000, Brian Starkey wrote:
>> On Tue, Nov 22, 2016 at 11:06:00AM +0000, Chris Wilson wrote:
>> >On Tue, Nov 22, 2016 at 10:53:51AM +0000, Brian Starkey wrote:
>> >>Hi Gustavo,
>> >>
>> >>A little late to the party here, but I was blocked by our internal
>> >>contributions process...
>> >>
>> >>I didn't see these end up in my checkout yet though, so I guess they
>> >>aren't picked up yet.
>> >>
>> >>On Mon, Nov 14, 2016 at 06:59:21PM +0900, Gustavo Padovan wrote:
>> >>>From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>> >>>
>> >>>Add support for the OUT_FENCE_PTR property to enable setting out fences for
>> >>>atomic commits.
>> >>>
>> >>>Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>> >>>---
>> >>>lib/igt_kms.c | 20 +++++++++++++++++++-
>> >>>lib/igt_kms.h |  3 +++
>> >>>2 files changed, 22 insertions(+), 1 deletion(-)
>> >>>
>> >>>diff --git a/lib/igt_kms.c b/lib/igt_kms.c
>> >>>index 4748c0a..f25e1eb 100644
>> >>>--- a/lib/igt_kms.c
>> >>>+++ b/lib/igt_kms.c
>> >>>@@ -175,7 +175,8 @@ const char *igt_crtc_prop_names[IGT_NUM_CRTC_PROPS] = {
>> >>>	"DEGAMMA_LUT",
>> >>>	"GAMMA_LUT",
>> >>>	"MODE_ID",
>> >>>-	"ACTIVE"
>> >>>+	"ACTIVE",
>> >>>+	"OUT_FENCE_PTR"
>> >>>};
>> >>>
>> >>>const char *igt_connector_prop_names[IGT_NUM_CONNECTOR_PROPS] = {
>> >>>@@ -2103,6 +2104,9 @@ static void igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicRe
>> >>>		igt_atomic_populate_crtc_req(req, pipe_obj, IGT_CRTC_ACTIVE, !!output);
>> >>>	}
>> >>>
>> >>>+	if (pipe_obj->out_fence_ptr)
>> >>>+		igt_atomic_populate_crtc_req(req, pipe_obj, IGT_CRTC_OUT_FENCE_PTR, pipe_obj->out_fence_ptr);
>> >>>+
>> >>>	/*
>> >>>	 *	TODO: Add all crtc level properties here
>> >>>	 */
>> >>>@@ -2683,6 +2687,20 @@ igt_pipe_set_gamma_lut(igt_pipe_t *pipe, void *ptr, size_t length)
>> >>>}
>> >>>
>> >>>/**
>> >>>+ * igt_pipe_set_out_fence_ptr:
>> >>>+ * @pipe: pipe pointer to which background color to be set
>> >>>+ * @fence_ptr: out fence pointer
>> >>
>> >>I don't think fence_ptr can be int *. It needs to be a pointer to a
>> >>64-bit type.
>> >>
>> >>>+ *
>> >>>+ * Sets the out fence pointer that will be passed to the kernel in
>> >>>+ * the atomic ioctl. When the kernel returns the out fence pointer
>> >>>+ * will contain the fd number of the out fence created by KMS.
>> >>>+ */
>> >>>+void igt_pipe_set_out_fence_ptr(igt_pipe_t *pipe, int *fence_ptr)
>> >>>+{
>> >>>+	pipe->out_fence_ptr = (uint64_t) fence_ptr;
>> >>>+}
>> >>>+
>> >>>+/**
>> >>> * igt_crtc_set_background:
>> >>> * @pipe: pipe pointer to which background color to be set
>> >>> * @background: background color value in BGR 16bpc
>> >>>diff --git a/lib/igt_kms.h b/lib/igt_kms.h
>> >>>index 344f931..02d7bd1 100644
>> >>>--- a/lib/igt_kms.h
>> >>>+++ b/lib/igt_kms.h
>> >>>@@ -110,6 +110,7 @@ enum igt_atomic_crtc_properties {
>> >>>       IGT_CRTC_GAMMA_LUT,
>> >>>       IGT_CRTC_MODE_ID,
>> >>>       IGT_CRTC_ACTIVE,
>> >>>+       IGT_CRTC_OUT_FENCE_PTR,
>> >>>       IGT_NUM_CRTC_PROPS
>> >>>};
>> >>>
>> >>>@@ -298,6 +299,7 @@ struct igt_pipe {
>> >>>
>> >>>	uint64_t mode_blob;
>> >>>	bool mode_changed;
>> >>>+	uint64_t out_fence_ptr;
>> >>
>> >>IMO this should be:
>> >>
>> >>	int64_t *out_fence_ptr;
>> >
>> >In userspace, fences are *fd*, a plain int. It is only the uabi that we
>> >pass pointers as u64 to the kernel, and indeed that should be limited to
>> >the uabi wrapper.
>> >-Chris
>>
>> Where's the uabi wrapper in this case?
>>
>> Wherever it is, afaik someone needs to have 64-bit type for the kernel
>> to stash its fd in - on the kernel side out_fence_ptr is
>> (s64 __user *), so if there's not a 64-bit variable on the other end
>> of it then someone's going to have a bad day.
>
>We do not have pointers in the uabi because they are different sizes on
>different platforms. The uabi must be a u64 representation of a user
>address to store the result - that is what we pass to the crtc set
>property ioctl.

Sure, but igt_pipe is not a uabi structure. By storing a uint64_t here
we're making it needlessly opaque what the value is actually meant to
be - which is the address of a 64-bit signed integer.

Regardless, tests cannot set out_fence_ptr to the address of an int, I
hope we can agree on that. Where that detail gets taken care of I
don't much mind - but this code as-is is incorrect.

By making igt_pipe.out_fence_ptr an (int64_t *) I thought we'd be
letting the compiler warn anyone else away from incorrect code.

>That it has been futher managled not to pass around fd
>is an interesting twist, but ideally that sillyness should not make
>itself into our API.

Allowing the kernel and userspace to have different ideas about how
big an int is doesn't sound so silly to me. It may not be a
theoretical problem forever.

-Brian

>-Chris
>
>-- 
>Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 07/12] lib/igt_kms: Add support for the OUT_FENCE_PTR property

[Daniel Vetter]

On Tue, Nov 22, 2016 at 12:37:47PM +0000, Brian Starkey wrote:
> On Tue, Nov 22, 2016 at 12:10:52PM +0000, Chris Wilson wrote:
> > On Tue, Nov 22, 2016 at 11:54:57AM +0000, Brian Starkey wrote:
> > > On Tue, Nov 22, 2016 at 11:06:00AM +0000, Chris Wilson wrote:
> > > >On Tue, Nov 22, 2016 at 10:53:51AM +0000, Brian Starkey wrote:
> > > >>Hi Gustavo,
> > > >>
> > > >>A little late to the party here, but I was blocked by our internal
> > > >>contributions process...
> > > >>
> > > >>I didn't see these end up in my checkout yet though, so I guess they
> > > >>aren't picked up yet.
> > > >>
> > > >>On Mon, Nov 14, 2016 at 06:59:21PM +0900, Gustavo Padovan wrote:
> > > >>>From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > > >>>
> > > >>>Add support for the OUT_FENCE_PTR property to enable setting out fences for
> > > >>>atomic commits.
> > > >>>
> > > >>>Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > > >>>---
> > > >>>lib/igt_kms.c | 20 +++++++++++++++++++-
> > > >>>lib/igt_kms.h |  3 +++
> > > >>>2 files changed, 22 insertions(+), 1 deletion(-)
> > > >>>
> > > >>>diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> > > >>>index 4748c0a..f25e1eb 100644
> > > >>>--- a/lib/igt_kms.c
> > > >>>+++ b/lib/igt_kms.c
> > > >>>@@ -175,7 +175,8 @@ const char *igt_crtc_prop_names[IGT_NUM_CRTC_PROPS] = {
> > > >>>	"DEGAMMA_LUT",
> > > >>>	"GAMMA_LUT",
> > > >>>	"MODE_ID",
> > > >>>-	"ACTIVE"
> > > >>>+	"ACTIVE",
> > > >>>+	"OUT_FENCE_PTR"
> > > >>>};
> > > >>>
> > > >>>const char *igt_connector_prop_names[IGT_NUM_CONNECTOR_PROPS] = {
> > > >>>@@ -2103,6 +2104,9 @@ static void igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicRe
> > > >>>		igt_atomic_populate_crtc_req(req, pipe_obj, IGT_CRTC_ACTIVE, !!output);
> > > >>>	}
> > > >>>
> > > >>>+	if (pipe_obj->out_fence_ptr)
> > > >>>+		igt_atomic_populate_crtc_req(req, pipe_obj, IGT_CRTC_OUT_FENCE_PTR, pipe_obj->out_fence_ptr);
> > > >>>+
> > > >>>	/*
> > > >>>	 *	TODO: Add all crtc level properties here
> > > >>>	 */
> > > >>>@@ -2683,6 +2687,20 @@ igt_pipe_set_gamma_lut(igt_pipe_t *pipe, void *ptr, size_t length)
> > > >>>}
> > > >>>
> > > >>>/**
> > > >>>+ * igt_pipe_set_out_fence_ptr:
> > > >>>+ * @pipe: pipe pointer to which background color to be set
> > > >>>+ * @fence_ptr: out fence pointer
> > > >>
> > > >>I don't think fence_ptr can be int *. It needs to be a pointer to a
> > > >>64-bit type.
> > > >>
> > > >>>+ *
> > > >>>+ * Sets the out fence pointer that will be passed to the kernel in
> > > >>>+ * the atomic ioctl. When the kernel returns the out fence pointer
> > > >>>+ * will contain the fd number of the out fence created by KMS.
> > > >>>+ */
> > > >>>+void igt_pipe_set_out_fence_ptr(igt_pipe_t *pipe, int *fence_ptr)
> > > >>>+{
> > > >>>+	pipe->out_fence_ptr = (uint64_t) fence_ptr;
> > > >>>+}
> > > >>>+
> > > >>>+/**
> > > >>> * igt_crtc_set_background:
> > > >>> * @pipe: pipe pointer to which background color to be set
> > > >>> * @background: background color value in BGR 16bpc
> > > >>>diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> > > >>>index 344f931..02d7bd1 100644
> > > >>>--- a/lib/igt_kms.h
> > > >>>+++ b/lib/igt_kms.h
> > > >>>@@ -110,6 +110,7 @@ enum igt_atomic_crtc_properties {
> > > >>>       IGT_CRTC_GAMMA_LUT,
> > > >>>       IGT_CRTC_MODE_ID,
> > > >>>       IGT_CRTC_ACTIVE,
> > > >>>+       IGT_CRTC_OUT_FENCE_PTR,
> > > >>>       IGT_NUM_CRTC_PROPS
> > > >>>};
> > > >>>
> > > >>>@@ -298,6 +299,7 @@ struct igt_pipe {
> > > >>>
> > > >>>	uint64_t mode_blob;
> > > >>>	bool mode_changed;
> > > >>>+	uint64_t out_fence_ptr;
> > > >>
> > > >>IMO this should be:
> > > >>
> > > >>	int64_t *out_fence_ptr;
> > > >
> > > >In userspace, fences are *fd*, a plain int. It is only the uabi that we
> > > >pass pointers as u64 to the kernel, and indeed that should be limited to
> > > >the uabi wrapper.
> > > >-Chris
> > > 
> > > Where's the uabi wrapper in this case?
> > > 
> > > Wherever it is, afaik someone needs to have 64-bit type for the kernel
> > > to stash its fd in - on the kernel side out_fence_ptr is
> > > (s64 __user *), so if there's not a 64-bit variable on the other end
> > > of it then someone's going to have a bad day.
> > 
> > We do not have pointers in the uabi because they are different sizes on
> > different platforms. The uabi must be a u64 representation of a user
> > address to store the result - that is what we pass to the crtc set
> > property ioctl.
> 
> Sure, but igt_pipe is not a uabi structure. By storing a uint64_t here
> we're making it needlessly opaque what the value is actually meant to
> be - which is the address of a 64-bit signed integer.
> 
> Regardless, tests cannot set out_fence_ptr to the address of an int, I
> hope we can agree on that. Where that detail gets taken care of I
> don't much mind - but this code as-is is incorrect.
> 
> By making igt_pipe.out_fence_ptr an (int64_t *) I thought we'd be
> letting the compiler warn anyone else away from incorrect code.
> 
> > That it has been futher managled not to pass around fd
> > is an interesting twist, but ideally that sillyness should not make
> > itself into our API.
> 
> Allowing the kernel and userspace to have different ideas about how
> big an int is doesn't sound so silly to me. It may not be a
> theoretical problem forever.

What Chris means is that you want to have an int out_fence in igt_pipe,
and just pass the address of that into the OUT_FENCE_PTR property. In
userspace we want to directly handle the fd, not a pointer to an fd. Like
Chris explained, the pointer-to-fd-cast-to-u64 is just to be able to reuse
the atomic ioctl as transport, it's not a reasonable interface within
userspace.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 07/12] lib/igt_kms: Add support for the OUT_FENCE_PTR property

[Brian Starkey]

On Tue, Nov 22, 2016 at 02:12:59PM +0100, Daniel Vetter wrote:
>On Tue, Nov 22, 2016 at 12:37:47PM +0000, Brian Starkey wrote:
>> On Tue, Nov 22, 2016 at 12:10:52PM +0000, Chris Wilson wrote:
>> > On Tue, Nov 22, 2016 at 11:54:57AM +0000, Brian Starkey wrote:
>> > > On Tue, Nov 22, 2016 at 11:06:00AM +0000, Chris Wilson wrote:
>> > > >On Tue, Nov 22, 2016 at 10:53:51AM +0000, Brian Starkey wrote:
>> > > >>Hi Gustavo,
>> > > >>
>> > > >>A little late to the party here, but I was blocked by our internal
>> > > >>contributions process...
>> > > >>
>> > > >>I didn't see these end up in my checkout yet though, so I guess they
>> > > >>aren't picked up yet.
>> > > >>
>> > > >>On Mon, Nov 14, 2016 at 06:59:21PM +0900, Gustavo Padovan wrote:
>> > > >>>From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>> > > >>>
>> > > >>>Add support for the OUT_FENCE_PTR property to enable setting out fences for
>> > > >>>atomic commits.
>> > > >>>
>> > > >>>Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>> > > >>>---
>> > > >>>lib/igt_kms.c | 20 +++++++++++++++++++-
>> > > >>>lib/igt_kms.h |  3 +++
>> > > >>>2 files changed, 22 insertions(+), 1 deletion(-)
>> > > >>>
>> > > >>>diff --git a/lib/igt_kms.c b/lib/igt_kms.c
>> > > >>>index 4748c0a..f25e1eb 100644
>> > > >>>--- a/lib/igt_kms.c
>> > > >>>+++ b/lib/igt_kms.c
>> > > >>>@@ -175,7 +175,8 @@ const char *igt_crtc_prop_names[IGT_NUM_CRTC_PROPS] = {
>> > > >>>	"DEGAMMA_LUT",
>> > > >>>	"GAMMA_LUT",
>> > > >>>	"MODE_ID",
>> > > >>>-	"ACTIVE"
>> > > >>>+	"ACTIVE",
>> > > >>>+	"OUT_FENCE_PTR"
>> > > >>>};
>> > > >>>
>> > > >>>const char *igt_connector_prop_names[IGT_NUM_CONNECTOR_PROPS] = {
>> > > >>>@@ -2103,6 +2104,9 @@ static void igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicRe
>> > > >>>		igt_atomic_populate_crtc_req(req, pipe_obj, IGT_CRTC_ACTIVE, !!output);
>> > > >>>	}
>> > > >>>
>> > > >>>+	if (pipe_obj->out_fence_ptr)
>> > > >>>+		igt_atomic_populate_crtc_req(req, pipe_obj, IGT_CRTC_OUT_FENCE_PTR, pipe_obj->out_fence_ptr);
>> > > >>>+
>> > > >>>	/*
>> > > >>>	 *	TODO: Add all crtc level properties here
>> > > >>>	 */
>> > > >>>@@ -2683,6 +2687,20 @@ igt_pipe_set_gamma_lut(igt_pipe_t *pipe, void *ptr, size_t length)
>> > > >>>}
>> > > >>>
>> > > >>>/**
>> > > >>>+ * igt_pipe_set_out_fence_ptr:
>> > > >>>+ * @pipe: pipe pointer to which background color to be set
>> > > >>>+ * @fence_ptr: out fence pointer
>> > > >>
>> > > >>I don't think fence_ptr can be int *. It needs to be a pointer to a
>> > > >>64-bit type.
>> > > >>
>> > > >>>+ *
>> > > >>>+ * Sets the out fence pointer that will be passed to the kernel in
>> > > >>>+ * the atomic ioctl. When the kernel returns the out fence pointer
>> > > >>>+ * will contain the fd number of the out fence created by KMS.
>> > > >>>+ */
>> > > >>>+void igt_pipe_set_out_fence_ptr(igt_pipe_t *pipe, int *fence_ptr)
>> > > >>>+{
>> > > >>>+	pipe->out_fence_ptr = (uint64_t) fence_ptr;
>> > > >>>+}
>> > > >>>+
>> > > >>>+/**
>> > > >>> * igt_crtc_set_background:
>> > > >>> * @pipe: pipe pointer to which background color to be set
>> > > >>> * @background: background color value in BGR 16bpc
>> > > >>>diff --git a/lib/igt_kms.h b/lib/igt_kms.h
>> > > >>>index 344f931..02d7bd1 100644
>> > > >>>--- a/lib/igt_kms.h
>> > > >>>+++ b/lib/igt_kms.h
>> > > >>>@@ -110,6 +110,7 @@ enum igt_atomic_crtc_properties {
>> > > >>>       IGT_CRTC_GAMMA_LUT,
>> > > >>>       IGT_CRTC_MODE_ID,
>> > > >>>       IGT_CRTC_ACTIVE,
>> > > >>>+       IGT_CRTC_OUT_FENCE_PTR,
>> > > >>>       IGT_NUM_CRTC_PROPS
>> > > >>>};
>> > > >>>
>> > > >>>@@ -298,6 +299,7 @@ struct igt_pipe {
>> > > >>>
>> > > >>>	uint64_t mode_blob;
>> > > >>>	bool mode_changed;
>> > > >>>+	uint64_t out_fence_ptr;
>> > > >>
>> > > >>IMO this should be:
>> > > >>
>> > > >>	int64_t *out_fence_ptr;
>> > > >
>> > > >In userspace, fences are *fd*, a plain int. It is only the uabi that we
>> > > >pass pointers as u64 to the kernel, and indeed that should be limited to
>> > > >the uabi wrapper.
>> > > >-Chris
>> > >
>> > > Where's the uabi wrapper in this case?
>> > >
>> > > Wherever it is, afaik someone needs to have 64-bit type for the kernel
>> > > to stash its fd in - on the kernel side out_fence_ptr is
>> > > (s64 __user *), so if there's not a 64-bit variable on the other end
>> > > of it then someone's going to have a bad day.
>> >
>> > We do not have pointers in the uabi because they are different sizes on
>> > different platforms. The uabi must be a u64 representation of a user
>> > address to store the result - that is what we pass to the crtc set
>> > property ioctl.
>>
>> Sure, but igt_pipe is not a uabi structure. By storing a uint64_t here
>> we're making it needlessly opaque what the value is actually meant to
>> be - which is the address of a 64-bit signed integer.
>>
>> Regardless, tests cannot set out_fence_ptr to the address of an int, I
>> hope we can agree on that. Where that detail gets taken care of I
>> don't much mind - but this code as-is is incorrect.
>>
>> By making igt_pipe.out_fence_ptr an (int64_t *) I thought we'd be
>> letting the compiler warn anyone else away from incorrect code.
>>
>> > That it has been futher managled not to pass around fd
>> > is an interesting twist, but ideally that sillyness should not make
>> > itself into our API.
>>
>> Allowing the kernel and userspace to have different ideas about how
>> big an int is doesn't sound so silly to me. It may not be a
>> theoretical problem forever.
>
>What Chris means is that you want to have an int out_fence in igt_pipe,
>and just pass the address of that into the OUT_FENCE_PTR property.

Storing the fence itself in igt_pipe instead of a pointer to it is a
different matter (and it isn't what's implemented in this patch).

It still doesn't change the fact that you can't do as you suggest -
you cannot just pass the address of an int in the OUT_FENCE_PTR
property.

In the kernel, put_user(fd, fence_state[i].out_fence_ptr); is going to
write 8 bytes. If out_fence_ptr is the address of a 4-byte variable,
then obviously that's not going to work out so well.

>In
>userspace we want to directly handle the fd, not a pointer to an fd. Like
>Chris explained, the pointer-to-fd-cast-to-u64 is just to be able to reuse
>the atomic ioctl as transport, it's not a reasonable interface within
>userspace.

I don't really follow this bit. At some point, something in userspace
is going to need to take care of the fact that the kernel needs to
have an 8-byte container to write into.

-Brian

>-Daniel
>-- 
>Daniel Vetter
>Software Engineer, Intel Corporation
>http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 07/12] lib/igt_kms: Add support for the OUT_FENCE_PTR property

[Daniel Vetter]

On Tue, Nov 22, 2016 at 01:50:53PM +0000, Brian Starkey wrote:
> On Tue, Nov 22, 2016 at 02:12:59PM +0100, Daniel Vetter wrote:
> > On Tue, Nov 22, 2016 at 12:37:47PM +0000, Brian Starkey wrote:
> > > On Tue, Nov 22, 2016 at 12:10:52PM +0000, Chris Wilson wrote:
> > > > On Tue, Nov 22, 2016 at 11:54:57AM +0000, Brian Starkey wrote:
> > > > > On Tue, Nov 22, 2016 at 11:06:00AM +0000, Chris Wilson wrote:
> > > > > >On Tue, Nov 22, 2016 at 10:53:51AM +0000, Brian Starkey wrote:
> > > > > >>Hi Gustavo,
> > > > > >>
> > > > > >>A little late to the party here, but I was blocked by our internal
> > > > > >>contributions process...
> > > > > >>
> > > > > >>I didn't see these end up in my checkout yet though, so I guess they
> > > > > >>aren't picked up yet.
> > > > > >>
> > > > > >>On Mon, Nov 14, 2016 at 06:59:21PM +0900, Gustavo Padovan wrote:
> > > > > >>>From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > > > > >>>
> > > > > >>>Add support for the OUT_FENCE_PTR property to enable setting out fences for
> > > > > >>>atomic commits.
> > > > > >>>
> > > > > >>>Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > > > > >>>---
> > > > > >>>lib/igt_kms.c | 20 +++++++++++++++++++-
> > > > > >>>lib/igt_kms.h |  3 +++
> > > > > >>>2 files changed, 22 insertions(+), 1 deletion(-)
> > > > > >>>
> > > > > >>>diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> > > > > >>>index 4748c0a..f25e1eb 100644
> > > > > >>>--- a/lib/igt_kms.c
> > > > > >>>+++ b/lib/igt_kms.c
> > > > > >>>@@ -175,7 +175,8 @@ const char *igt_crtc_prop_names[IGT_NUM_CRTC_PROPS] = {
> > > > > >>>	"DEGAMMA_LUT",
> > > > > >>>	"GAMMA_LUT",
> > > > > >>>	"MODE_ID",
> > > > > >>>-	"ACTIVE"
> > > > > >>>+	"ACTIVE",
> > > > > >>>+	"OUT_FENCE_PTR"
> > > > > >>>};
> > > > > >>>
> > > > > >>>const char *igt_connector_prop_names[IGT_NUM_CONNECTOR_PROPS] = {
> > > > > >>>@@ -2103,6 +2104,9 @@ static void igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicRe
> > > > > >>>		igt_atomic_populate_crtc_req(req, pipe_obj, IGT_CRTC_ACTIVE, !!output);
> > > > > >>>	}
> > > > > >>>
> > > > > >>>+	if (pipe_obj->out_fence_ptr)
> > > > > >>>+		igt_atomic_populate_crtc_req(req, pipe_obj, IGT_CRTC_OUT_FENCE_PTR, pipe_obj->out_fence_ptr);
> > > > > >>>+
> > > > > >>>	/*
> > > > > >>>	 *	TODO: Add all crtc level properties here
> > > > > >>>	 */
> > > > > >>>@@ -2683,6 +2687,20 @@ igt_pipe_set_gamma_lut(igt_pipe_t *pipe, void *ptr, size_t length)
> > > > > >>>}
> > > > > >>>
> > > > > >>>/**
> > > > > >>>+ * igt_pipe_set_out_fence_ptr:
> > > > > >>>+ * @pipe: pipe pointer to which background color to be set
> > > > > >>>+ * @fence_ptr: out fence pointer
> > > > > >>
> > > > > >>I don't think fence_ptr can be int *. It needs to be a pointer to a
> > > > > >>64-bit type.
> > > > > >>
> > > > > >>>+ *
> > > > > >>>+ * Sets the out fence pointer that will be passed to the kernel in
> > > > > >>>+ * the atomic ioctl. When the kernel returns the out fence pointer
> > > > > >>>+ * will contain the fd number of the out fence created by KMS.
> > > > > >>>+ */
> > > > > >>>+void igt_pipe_set_out_fence_ptr(igt_pipe_t *pipe, int *fence_ptr)
> > > > > >>>+{
> > > > > >>>+	pipe->out_fence_ptr = (uint64_t) fence_ptr;
> > > > > >>>+}
> > > > > >>>+
> > > > > >>>+/**
> > > > > >>> * igt_crtc_set_background:
> > > > > >>> * @pipe: pipe pointer to which background color to be set
> > > > > >>> * @background: background color value in BGR 16bpc
> > > > > >>>diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> > > > > >>>index 344f931..02d7bd1 100644
> > > > > >>>--- a/lib/igt_kms.h
> > > > > >>>+++ b/lib/igt_kms.h
> > > > > >>>@@ -110,6 +110,7 @@ enum igt_atomic_crtc_properties {
> > > > > >>>       IGT_CRTC_GAMMA_LUT,
> > > > > >>>       IGT_CRTC_MODE_ID,
> > > > > >>>       IGT_CRTC_ACTIVE,
> > > > > >>>+       IGT_CRTC_OUT_FENCE_PTR,
> > > > > >>>       IGT_NUM_CRTC_PROPS
> > > > > >>>};
> > > > > >>>
> > > > > >>>@@ -298,6 +299,7 @@ struct igt_pipe {
> > > > > >>>
> > > > > >>>	uint64_t mode_blob;
> > > > > >>>	bool mode_changed;
> > > > > >>>+	uint64_t out_fence_ptr;
> > > > > >>
> > > > > >>IMO this should be:
> > > > > >>
> > > > > >>	int64_t *out_fence_ptr;
> > > > > >
> > > > > >In userspace, fences are *fd*, a plain int. It is only the uabi that we
> > > > > >pass pointers as u64 to the kernel, and indeed that should be limited to
> > > > > >the uabi wrapper.
> > > > > >-Chris
> > > > >
> > > > > Where's the uabi wrapper in this case?
> > > > >
> > > > > Wherever it is, afaik someone needs to have 64-bit type for the kernel
> > > > > to stash its fd in - on the kernel side out_fence_ptr is
> > > > > (s64 __user *), so if there's not a 64-bit variable on the other end
> > > > > of it then someone's going to have a bad day.
> > > >
> > > > We do not have pointers in the uabi because they are different sizes on
> > > > different platforms. The uabi must be a u64 representation of a user
> > > > address to store the result - that is what we pass to the crtc set
> > > > property ioctl.
> > > 
> > > Sure, but igt_pipe is not a uabi structure. By storing a uint64_t here
> > > we're making it needlessly opaque what the value is actually meant to
> > > be - which is the address of a 64-bit signed integer.
> > > 
> > > Regardless, tests cannot set out_fence_ptr to the address of an int, I
> > > hope we can agree on that. Where that detail gets taken care of I
> > > don't much mind - but this code as-is is incorrect.
> > > 
> > > By making igt_pipe.out_fence_ptr an (int64_t *) I thought we'd be
> > > letting the compiler warn anyone else away from incorrect code.
> > > 
> > > > That it has been futher managled not to pass around fd
> > > > is an interesting twist, but ideally that sillyness should not make
> > > > itself into our API.
> > > 
> > > Allowing the kernel and userspace to have different ideas about how
> > > big an int is doesn't sound so silly to me. It may not be a
> > > theoretical problem forever.
> > 
> > What Chris means is that you want to have an int out_fence in igt_pipe,
> > and just pass the address of that into the OUT_FENCE_PTR property.
> 
> Storing the fence itself in igt_pipe instead of a pointer to it is a
> different matter (and it isn't what's implemented in this patch).
> 
> It still doesn't change the fact that you can't do as you suggest -
> you cannot just pass the address of an int in the OUT_FENCE_PTR
> property.
> 
> In the kernel, put_user(fd, fence_state[i].out_fence_ptr); is going to
> write 8 bytes. If out_fence_ptr is the address of a 4-byte variable,
> then obviously that's not going to work out so well.

Oh right, s/int/int64_t/, but otherwise that's imo what we should do.

> > In
> > userspace we want to directly handle the fd, not a pointer to an fd. Like
> > Chris explained, the pointer-to-fd-cast-to-u64 is just to be able to reuse
> > the atomic ioctl as transport, it's not a reasonable interface within
> > userspace.
> 
> I don't really follow this bit. At some point, something in userspace
> is going to need to take care of the fact that the kernel needs to
> have an 8-byte container to write into.

Hm, why can't we allow igt test to directly access igt_pipe->out_fence?
And switch your function to igt_pipe_request_out_fence?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 07/12] lib/igt_kms: Add support for the OUT_FENCE_PTR property

[Brian Starkey]

On Tue, Nov 22, 2016 at 02:56:49PM +0100, Daniel Vetter wrote:
>On Tue, Nov 22, 2016 at 01:50:53PM +0000, Brian Starkey wrote:
>> On Tue, Nov 22, 2016 at 02:12:59PM +0100, Daniel Vetter wrote:
>> > On Tue, Nov 22, 2016 at 12:37:47PM +0000, Brian Starkey wrote:
>> > > On Tue, Nov 22, 2016 at 12:10:52PM +0000, Chris Wilson wrote:
>> > > > On Tue, Nov 22, 2016 at 11:54:57AM +0000, Brian Starkey wrote:
>> > > > > On Tue, Nov 22, 2016 at 11:06:00AM +0000, Chris Wilson wrote:
>> > > > > >On Tue, Nov 22, 2016 at 10:53:51AM +0000, Brian Starkey wrote:
>> > > > > >>Hi Gustavo,
>> > > > > >>
>> > > > > >>A little late to the party here, but I was blocked by our internal
>> > > > > >>contributions process...
>> > > > > >>
>> > > > > >>I didn't see these end up in my checkout yet though, so I guess they
>> > > > > >>aren't picked up yet.
>> > > > > >>
>> > > > > >>On Mon, Nov 14, 2016 at 06:59:21PM +0900, Gustavo Padovan wrote:
>> > > > > >>>From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>> > > > > >>>
>> > > > > >>>Add support for the OUT_FENCE_PTR property to enable setting out fences for
>> > > > > >>>atomic commits.
>> > > > > >>>
>> > > > > >>>Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>> > > > > >>>---
>> > > > > >>>lib/igt_kms.c | 20 +++++++++++++++++++-
>> > > > > >>>lib/igt_kms.h |  3 +++
>> > > > > >>>2 files changed, 22 insertions(+), 1 deletion(-)
>> > > > > >>>
>> > > > > >>>diff --git a/lib/igt_kms.c b/lib/igt_kms.c
>> > > > > >>>index 4748c0a..f25e1eb 100644
>> > > > > >>>--- a/lib/igt_kms.c
>> > > > > >>>+++ b/lib/igt_kms.c
>> > > > > >>>@@ -175,7 +175,8 @@ const char *igt_crtc_prop_names[IGT_NUM_CRTC_PROPS] = {
>> > > > > >>>	"DEGAMMA_LUT",
>> > > > > >>>	"GAMMA_LUT",
>> > > > > >>>	"MODE_ID",
>> > > > > >>>-	"ACTIVE"
>> > > > > >>>+	"ACTIVE",
>> > > > > >>>+	"OUT_FENCE_PTR"
>> > > > > >>>};
>> > > > > >>>
>> > > > > >>>const char *igt_connector_prop_names[IGT_NUM_CONNECTOR_PROPS] = {
>> > > > > >>>@@ -2103,6 +2104,9 @@ static void igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicRe
>> > > > > >>>		igt_atomic_populate_crtc_req(req, pipe_obj, IGT_CRTC_ACTIVE, !!output);
>> > > > > >>>	}
>> > > > > >>>
>> > > > > >>>+	if (pipe_obj->out_fence_ptr)
>> > > > > >>>+		igt_atomic_populate_crtc_req(req, pipe_obj, IGT_CRTC_OUT_FENCE_PTR, pipe_obj->out_fence_ptr);
>> > > > > >>>+
>> > > > > >>>	/*
>> > > > > >>>	 *	TODO: Add all crtc level properties here
>> > > > > >>>	 */
>> > > > > >>>@@ -2683,6 +2687,20 @@ igt_pipe_set_gamma_lut(igt_pipe_t *pipe, void *ptr, size_t length)
>> > > > > >>>}
>> > > > > >>>
>> > > > > >>>/**
>> > > > > >>>+ * igt_pipe_set_out_fence_ptr:
>> > > > > >>>+ * @pipe: pipe pointer to which background color to be set
>> > > > > >>>+ * @fence_ptr: out fence pointer
>> > > > > >>
>> > > > > >>I don't think fence_ptr can be int *. It needs to be a pointer to a
>> > > > > >>64-bit type.
>> > > > > >>
>> > > > > >>>+ *
>> > > > > >>>+ * Sets the out fence pointer that will be passed to the kernel in
>> > > > > >>>+ * the atomic ioctl. When the kernel returns the out fence pointer
>> > > > > >>>+ * will contain the fd number of the out fence created by KMS.
>> > > > > >>>+ */
>> > > > > >>>+void igt_pipe_set_out_fence_ptr(igt_pipe_t *pipe, int *fence_ptr)
>> > > > > >>>+{
>> > > > > >>>+	pipe->out_fence_ptr = (uint64_t) fence_ptr;
>> > > > > >>>+}
>> > > > > >>>+
>> > > > > >>>+/**
>> > > > > >>> * igt_crtc_set_background:
>> > > > > >>> * @pipe: pipe pointer to which background color to be set
>> > > > > >>> * @background: background color value in BGR 16bpc
>> > > > > >>>diff --git a/lib/igt_kms.h b/lib/igt_kms.h
>> > > > > >>>index 344f931..02d7bd1 100644
>> > > > > >>>--- a/lib/igt_kms.h
>> > > > > >>>+++ b/lib/igt_kms.h
>> > > > > >>>@@ -110,6 +110,7 @@ enum igt_atomic_crtc_properties {
>> > > > > >>>       IGT_CRTC_GAMMA_LUT,
>> > > > > >>>       IGT_CRTC_MODE_ID,
>> > > > > >>>       IGT_CRTC_ACTIVE,
>> > > > > >>>+       IGT_CRTC_OUT_FENCE_PTR,
>> > > > > >>>       IGT_NUM_CRTC_PROPS
>> > > > > >>>};
>> > > > > >>>
>> > > > > >>>@@ -298,6 +299,7 @@ struct igt_pipe {
>> > > > > >>>
>> > > > > >>>	uint64_t mode_blob;
>> > > > > >>>	bool mode_changed;
>> > > > > >>>+	uint64_t out_fence_ptr;
>> > > > > >>
>> > > > > >>IMO this should be:
>> > > > > >>
>> > > > > >>	int64_t *out_fence_ptr;
>> > > > > >
>> > > > > >In userspace, fences are *fd*, a plain int. It is only the uabi that we
>> > > > > >pass pointers as u64 to the kernel, and indeed that should be limited to
>> > > > > >the uabi wrapper.
>> > > > > >-Chris
>> > > > >
>> > > > > Where's the uabi wrapper in this case?
>> > > > >
>> > > > > Wherever it is, afaik someone needs to have 64-bit type for the kernel
>> > > > > to stash its fd in - on the kernel side out_fence_ptr is
>> > > > > (s64 __user *), so if there's not a 64-bit variable on the other end
>> > > > > of it then someone's going to have a bad day.
>> > > >
>> > > > We do not have pointers in the uabi because they are different sizes on
>> > > > different platforms. The uabi must be a u64 representation of a user
>> > > > address to store the result - that is what we pass to the crtc set
>> > > > property ioctl.
>> > >
>> > > Sure, but igt_pipe is not a uabi structure. By storing a uint64_t here
>> > > we're making it needlessly opaque what the value is actually meant to
>> > > be - which is the address of a 64-bit signed integer.
>> > >
>> > > Regardless, tests cannot set out_fence_ptr to the address of an int, I
>> > > hope we can agree on that. Where that detail gets taken care of I
>> > > don't much mind - but this code as-is is incorrect.
>> > >
>> > > By making igt_pipe.out_fence_ptr an (int64_t *) I thought we'd be
>> > > letting the compiler warn anyone else away from incorrect code.
>> > >
>> > > > That it has been futher managled not to pass around fd
>> > > > is an interesting twist, but ideally that sillyness should not make
>> > > > itself into our API.
>> > >
>> > > Allowing the kernel and userspace to have different ideas about how
>> > > big an int is doesn't sound so silly to me. It may not be a
>> > > theoretical problem forever.
>> >
>> > What Chris means is that you want to have an int out_fence in igt_pipe,
>> > and just pass the address of that into the OUT_FENCE_PTR property.
>>
>> Storing the fence itself in igt_pipe instead of a pointer to it is a
>> different matter (and it isn't what's implemented in this patch).
>>
>> It still doesn't change the fact that you can't do as you suggest -
>> you cannot just pass the address of an int in the OUT_FENCE_PTR
>> property.
>>
>> In the kernel, put_user(fd, fence_state[i].out_fence_ptr); is going to
>> write 8 bytes. If out_fence_ptr is the address of a 4-byte variable,
>> then obviously that's not going to work out so well.
>
>Oh right, s/int/int64_t/, but otherwise that's imo what we should do.
>
>> > In
>> > userspace we want to directly handle the fd, not a pointer to an fd. Like
>> > Chris explained, the pointer-to-fd-cast-to-u64 is just to be able to reuse
>> > the atomic ioctl as transport, it's not a reasonable interface within
>> > userspace.
>>
>> I don't really follow this bit. At some point, something in userspace
>> is going to need to take care of the fact that the kernel needs to
>> have an 8-byte container to write into.
>
>Hm, why can't we allow igt test to directly access igt_pipe->out_fence?
>And switch your function to igt_pipe_request_out_fence?

Not my function, but yeah that sounds fine to me.

All I wanted was to make sure we weren't passing the address of an int
for the kernel to clobber whatever was nearby. My suggestion to store
an (int64_t *) was just to let the compiler prevent that clobbering
from ever happening by accident.

-Brian

>-Daniel
>-- 
>Daniel Vetter
>Software Engineer, Intel Corporation
>http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH i-g-t 1/2] lib/drmtest: Fix igt_skip message

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Now other gpus are supported too.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 lib/drmtest.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/drmtest.c b/lib/drmtest.c
index 884fe7c..9f3ac7f 100644
--- a/lib/drmtest.c
+++ b/lib/drmtest.c
@@ -263,7 +263,7 @@ int __drm_open_driver(int chipset)
 		close(fd);
 	}
 
-	igt_skip("No intel gpu found\n");
+	igt_skip("No known gpu found\n");
 	return -1;
 }
 
-- 
2.5.5

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel