[igt-dev] [PATCH i-g-t v2] lib/igt_kms: Fix commits for planes with multiple possible CRTCs

* [igt-dev] [PATCH i-g-t v2] lib/igt_kms: Fix commits for planes with multiple possible CRTCs
@ 2019-02-22 17:04 Nicholas Kazlauskas
  2019-02-22 17:29 ` [igt-dev] ✗ Fi.CI.BAT: failure for lib/igt_kms: Fix commits for planes with multiple possible CRTCs (rev2) Patchwork
  0 siblings, 1 reply; 2+ messages in thread
From: Nicholas Kazlauskas @ 2019-02-22 17:04 UTC (permalink / raw)
  To: igt-dev; +Cc: Daniel Vetter

An igt_plane_t is defined per igt_pipe_t. It is treated as its
own independent resource but DRM planes can be exposed to being used on
a number of possible CRTCs - making it a shared resource.

In IGT planes with multiple possible CRTCs are added to the plane list
for each pipe that the plane supports. The internal state remains
independent in IGT so when the same plane is modified for multiple
pipes in a single commit the last pipe to modify it is the one whose
state gets fully applied.

This situation happens fairly often in practice - resetting the display
at the start of the test before a commit will reset the CRTC ID and FB
ID for each plane.

For an example, consider the
igt@kms_plane_alpha_blend@pipe-a-constant-alpha-max test.

This test will fail for any overlay plane exposed for multiple CRTCs.

The test tries to set a framebuffer for pipe A but has all the other
pipes reset the plane state in the same commit. If there are multiple
pipes on the hardware then the last pipe will be the one to set all the
plane properties. The driver will receive a commit with no overlay
plane enabled since the last pipe set CRTC ID and FB ID to 0, disabling
the plane. The reference CRC capture will be incorrect since not all
the planes have been enabled and the subsequent CRC captures will
not match, failing the test.

The simplest (but hacky) fix to this problem is to only set the
properties for the plane for the pipe that last had it bound.

This patch introduces a global plane list on igt_display_t that keeps
track of the pipe that pipe that last called igt_plane_set_fb. The
properties for the plane will only be applied from that single pipe
when commiting the state to DRM.

No behavioral changes should be introduced by this patch for hardware
whose planes are only ever exposed one CRTC.

It would likely be best to eventually modify the igt_pipe_t plane list
to be a list of pointers to planes instead (igt_plane_t**)
instead of being the actual plane objects, but that can come later.

Many areas of the code like to make use of the backpointer to the pipe
on the plane which makes refactoring the code in that manner a little
trickier.

v2: Add igt_plane_set_fb, use igt_plane_t for global plane list (Daniel)

Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
---
 lib/igt_kms.c | 102 ++++++++++++++++++++++++++++++++++++--------------
 lib/igt_kms.h |   6 ++-
 2 files changed, 78 insertions(+), 30 deletions(-)

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index 761bb193..810b8fe0 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -1905,6 +1905,25 @@ void igt_display_require(igt_display_t *display, int drm_fd)
 	plane_resources = drmModeGetPlaneResources(display->drm_fd);
 	igt_assert(plane_resources);
 
+	display->n_planes = plane_resources->count_planes;
+	display->planes = calloc(sizeof(igt_plane_t), display->n_planes);
+	igt_assert_f(display->planes, "Failed to allocate memory for %d planes\n", display->n_planes);
+
+	for (i = 0; i < plane_resources->count_planes; ++i) {
+		igt_plane_t *plane = &display->planes[i];
+		uint32_t id = plane_resources->planes[i];
+
+		plane->drm_plane = drmModeGetPlane(display->drm_fd, id);
+		igt_assert(plane->drm_plane);
+
+		plane->type = get_drm_plane_type(display->drm_fd, id);
+		plane->values[IGT_PLANE_IN_FENCE_FD] = ~0ULL;
+
+		igt_fill_plane_props(display, plane, IGT_NUM_PLANE_PROPS,
+				     igt_plane_prop_names);
+		igt_fill_plane_format_mod(display, plane);
+	}
+
 	for_each_pipe(display, i) {
 		igt_pipe_t *pipe = &display->pipes[i];
 		igt_plane_t *plane;
@@ -1922,17 +1941,12 @@ void igt_display_require(igt_display_t *display, int drm_fd)
 		igt_fill_pipe_props(display, pipe, IGT_NUM_CRTC_PROPS, igt_crtc_prop_names);
 
 		/* count number of valid planes */
-		for (j = 0; j < plane_resources->count_planes; j++) {
-			drmModePlane *drm_plane;
-
-			drm_plane = drmModeGetPlane(display->drm_fd,
-						    plane_resources->planes[j]);
+		for (j = 0; j < display->n_planes; j++) {
+			drmModePlane *drm_plane = display->planes[j].drm_plane;
 			igt_assert(drm_plane);
 
 			if (drm_plane->possible_crtcs & (1 << i))
 				n_planes++;
-
-			drmModeFreePlane(drm_plane);
 		}
 
 		igt_assert_lt(0, n_planes);
@@ -1941,20 +1955,14 @@ void igt_display_require(igt_display_t *display, int drm_fd)
 		last_plane = n_planes - 1;
 
 		/* add the planes that can be used with that pipe */
-		for (j = 0; j < plane_resources->count_planes; j++) {
-			drmModePlane *drm_plane;
+		for (j = 0; j < display->n_planes; j++) {
+			igt_plane_t *global_plane = &display->planes[j];
+			drmModePlane *drm_plane = global_plane->drm_plane;
 
-			drm_plane = drmModeGetPlane(display->drm_fd,
-						    plane_resources->planes[j]);
-			igt_assert(drm_plane);
-
-			if (!(drm_plane->possible_crtcs & (1 << i))) {
-				drmModeFreePlane(drm_plane);
+			if (!(drm_plane->possible_crtcs & (1 << i)))
 				continue;
-			}
 
-			type = get_drm_plane_type(display->drm_fd,
-						  plane_resources->planes[j]);
+			type = global_plane->type;
 
 			if (type == DRM_PLANE_TYPE_PRIMARY && pipe->plane_primary == -1) {
 				plane = &pipe->planes[0];
@@ -1975,6 +1983,14 @@ void igt_display_require(igt_display_t *display, int drm_fd)
 			plane->pipe = pipe;
 			plane->drm_plane = drm_plane;
 			plane->values[IGT_PLANE_IN_FENCE_FD] = ~0ULL;
+			plane->ref = global_plane;
+
+			/*
+			 * HACK: point the global plane to the first pipe that
+			 * it can go on.
+			 */
+			if (!global_plane->ref)
+				igt_plane_set_pipe(plane, pipe);
 
 			igt_fill_plane_props(display, plane, IGT_NUM_PLANE_PROPS, igt_plane_prop_names);
 
@@ -2127,17 +2143,6 @@ igt_output_t *igt_output_from_connector(igt_display_t *display,
 
 static void igt_pipe_fini(igt_pipe_t *pipe)
 {
-	int i;
-
-	for (i = 0; i < pipe->n_planes; i++) {
-		igt_plane_t *plane = &pipe->planes[i];
-
-		if (plane->drm_plane) {
-			drmModeFreePlane(plane->drm_plane);
-			plane->drm_plane = NULL;
-		}
-	}
-
 	free(pipe->planes);
 	pipe->planes = NULL;
 
@@ -2163,6 +2168,15 @@ void igt_display_fini(igt_display_t *display)
 {
 	int i;
 
+	for (i = 0; i < display->n_planes; ++i) {
+		igt_plane_t *plane = &display->planes[i];
+
+		if (plane->drm_plane) {
+			drmModeFreePlane(plane->drm_plane);
+			plane->drm_plane = NULL;
+		}
+	}
+
 	for (i = 0; i < display->n_pipes; i++)
 		igt_pipe_fini(&display->pipes[i]);
 
@@ -2172,6 +2186,8 @@ void igt_display_fini(igt_display_t *display)
 	display->outputs = NULL;
 	free(display->pipes);
 	display->pipes = NULL;
+	free(display->planes);
+	display->planes = NULL;
 }
 
 static void igt_display_refresh(igt_display_t *display)
@@ -2789,6 +2805,10 @@ static int igt_pipe_commit(igt_pipe_t *pipe,
 	for (i = 0; i < pipe->n_planes; i++) {
 		igt_plane_t *plane = &pipe->planes[i];
 
+		/* skip planes that are handled by another pipe */
+		if (plane->ref->pipe != pipe)
+			continue;
+
 		ret = igt_plane_commit(plane, pipe, s, fail_on_error);
 		CHECK_RETURN(ret, fail_on_error);
 	}
@@ -3177,6 +3197,10 @@ static int igt_atomic_commit(igt_display_t *display, uint32_t flags, void *user_
 			igt_atomic_prepare_crtc_commit(pipe_obj, req);
 
 		for_each_plane_on_pipe(display, pipe, plane) {
+			/* skip planes that are handled by another pipe */
+			if (plane->ref->pipe != pipe_obj)
+				continue;
+
 			if (plane->changed)
 				igt_atomic_prepare_plane_commit(plane, pipe_obj, req);
 		}
@@ -3709,6 +3733,9 @@ void igt_plane_set_fb(igt_plane_t *plane, struct igt_fb *fb)
 		if (igt_plane_has_prop(plane, IGT_PLANE_COLOR_RANGE))
 			igt_plane_set_prop_enum(plane, IGT_PLANE_COLOR_RANGE,
 				igt_color_range_to_str(fb->color_range));
+
+		/* Hack to prioritize the plane on the pipe that last set fb */
+		igt_plane_set_pipe(plane, pipe);
 	} else {
 		igt_plane_set_size(plane, 0, 0);
 
@@ -3743,6 +3770,23 @@ void igt_plane_set_fence_fd(igt_plane_t *plane, int fence_fd)
 	igt_plane_set_prop_value(plane, IGT_PLANE_IN_FENCE_FD, fd);
 }
 
+/**
+ * igt_plane_set_pipe:
+ * @plane: Target plane pointer
+ * @pipe: The pipe to assign the plane to
+ *
+ */
+void igt_plane_set_pipe(igt_plane_t *plane, igt_pipe_t *pipe)
+{
+	/*
+	 * HACK: Point the global plane back to the local plane.
+	 * This is used to help apply the correct atomic state while
+	 * we're moving away from the single pipe per plane model.
+	 */
+	plane->ref->ref = plane;
+	plane->ref->pipe = pipe;
+}
+
 /**
  * igt_plane_set_position:
  * @plane: Plane pointer for which position is to be set
diff --git a/lib/igt_kms.h b/lib/igt_kms.h
index 5755b160..e1c5c967 100644
--- a/lib/igt_kms.h
+++ b/lib/igt_kms.h
@@ -300,9 +300,10 @@ typedef enum {
 #define IGT_ROTATION_MASK \
 	(IGT_ROTATION_0 | IGT_ROTATION_90 | IGT_ROTATION_180 | IGT_ROTATION_270)
 
-typedef struct {
+typedef struct igt_plane {
 	/*< private >*/
 	igt_pipe_t *pipe;
+	struct igt_plane *ref;
 	int index;
 	/* capabilities */
 	int type;
@@ -372,8 +373,10 @@ struct igt_display {
 	int drm_fd;
 	int log_shift;
 	int n_pipes;
+	int n_planes;
 	int n_outputs;
 	igt_output_t *outputs;
+	igt_plane_t *planes;
 	igt_pipe_t *pipes;
 	bool has_cursor_plane;
 	bool is_atomic;
@@ -413,6 +416,7 @@ void igt_pipe_request_out_fence(igt_pipe_t *pipe);
 
 void igt_plane_set_fb(igt_plane_t *plane, struct igt_fb *fb);
 void igt_plane_set_fence_fd(igt_plane_t *plane, int fence_fd);
+void igt_plane_set_pipe(igt_plane_t *plane, igt_pipe_t *pipe);
 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.17.1

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

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