[igt-dev] [PATCH i-g-t v3 1/3] lib/kms: Commit primary plane props with COMMIT_LEGACY

* [igt-dev] [PATCH i-g-t v3 1/3] lib/kms: Commit primary plane props with COMMIT_LEGACY
@ 2021-04-20 22:17 Ville Syrjala
  2021-04-20 22:17 ` [igt-dev] [PATCH i-g-t v3 2/3] lib/kms: Reset color encoding to BT.709 Ville Syrjala
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Ville Syrjala @ 2021-04-20 22:17 UTC (permalink / raw)
  To: igt-dev

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Currently COMMIT_LEGACY for the primary plane only issues the
setcrtc ioctl, leaving all other plane properties unchanged
even if they were flagged as needing an update. Let's issue
the appropriate setprop ioctls in addition to the setcrtc to
make sure the plane state really reflects what was requested.

Without this the prop values can leak between tests. Eg. on
skl/derivatives running one of the failing kms_plane_alpha_blend
subtests first, and following it up with kms_cursor_crc (which
only uses legacy commits) causes crc mismatches since the
primary plane alpha is still left at some stale value despite
igt_plane_reset() trying to reset it back to 1.0.

v2: do the SetProps even if SetCrtc isn't needed
    clear the changed flags for the props

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 lib/igt_kms.c | 62 ++++++++++++++++++++++++++++++---------------------
 1 file changed, 36 insertions(+), 26 deletions(-)

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index 08d429a8190a..279b8cee42d0 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -2660,6 +2660,36 @@ igt_atomic_prepare_plane_commit(igt_plane_t *plane, igt_pipe_t *pipe,
 	   (1ULL << IGT_PLANE_CRTC_ID) | \
 	   (1ULL << IGT_PLANE_IN_FENCE_FD)))
 
+static int plane_commit_props(igt_plane_t *plane,
+			      igt_pipe_t *pipe,
+			      bool fail_on_error)
+{
+	igt_display_t *display = pipe->display;
+	uint64_t changed_mask;
+	int ret, i;
+
+	changed_mask = plane->changed & LEGACY_PLANE_COMMIT_MASK;
+
+	for (i = 0; i < IGT_NUM_PLANE_PROPS; i++) {
+		if (!(changed_mask & (1 << i)))
+			continue;
+
+		LOG(display, "SetProp plane %s.%d \"%s\" to 0x%"PRIx64"/%"PRIi64"\n",
+			kmstest_pipe_name(pipe->pipe), plane->index, igt_plane_prop_names[i],
+			plane->values[i], plane->values[i]);
+
+		igt_assert(plane->props[i]);
+
+		ret = igt_plane_set_property(plane,
+					     plane->props[i],
+					     plane->values[i]);
+
+		CHECK_RETURN(ret, fail_on_error);
+	}
+
+	return 0;
+}
+
 /*
  * Commit position and fb changes to a DRM plane via the SetPlane ioctl; if the
  * DRM call to program the plane fails, we'll either fail immediately (for
@@ -2672,7 +2702,7 @@ static int igt_drm_plane_commit(igt_plane_t *plane,
 {
 	igt_display_t *display = pipe->display;
 	uint32_t fb_id, crtc_id;
-	int ret, i;
+	int ret;
 	uint32_t src_x;
 	uint32_t src_y;
 	uint32_t src_w;
@@ -2681,7 +2711,6 @@ static int igt_drm_plane_commit(igt_plane_t *plane,
 	int32_t crtc_y;
 	uint32_t crtc_w;
 	uint32_t crtc_h;
-	uint64_t changed_mask;
 	bool setplane =
 		igt_plane_is_prop_changed(plane, IGT_PLANE_FB_ID) ||
 		plane->changed & IGT_PLANE_COORD_CHANGED_MASK;
@@ -2742,26 +2771,7 @@ static int igt_drm_plane_commit(igt_plane_t *plane,
 		CHECK_RETURN(ret, fail_on_error);
 	}
 
-	changed_mask = plane->changed & LEGACY_PLANE_COMMIT_MASK;
-
-	for (i = 0; i < IGT_NUM_PLANE_PROPS; i++) {
-		if (!(changed_mask & (1 << i)))
-			continue;
-
-		LOG(display, "SetProp plane %s.%d \"%s\" to 0x%"PRIx64"/%"PRIi64"\n",
-			kmstest_pipe_name(pipe->pipe), plane->index, igt_plane_prop_names[i],
-			plane->values[i], plane->values[i]);
-
-		igt_assert(plane->props[i]);
-
-		ret = igt_plane_set_property(plane,
-					     plane->props[i],
-					     plane->values[i]);
-
-		CHECK_RETURN(ret, fail_on_error);
-	}
-
-	return 0;
+	return plane_commit_props(plane, pipe, fail_on_error);
 }
 
 /*
@@ -2841,7 +2851,7 @@ static int igt_primary_plane_commit_legacy(igt_plane_t *primary,
 	if (!igt_plane_is_prop_changed(primary, IGT_PLANE_FB_ID) &&
 	    !(primary->changed & IGT_PLANE_COORD_CHANGED_MASK) &&
 	    !igt_pipe_obj_is_prop_changed(primary->pipe, IGT_CRTC_MODE_ID))
-		return 0;
+		goto skip_setcrtc;
 
 	crtc_id = pipe->crtc_id;
 	fb_id = output ? igt_plane_get_fb_id(primary) : 0;
@@ -2886,7 +2896,8 @@ static int igt_primary_plane_commit_legacy(igt_plane_t *primary,
 
 	CHECK_RETURN(ret, fail_on_error);
 
-	return 0;
+skip_setcrtc:
+	return plane_commit_props(primary, pipe, fail_on_error);
 }
 
 static int igt_plane_fixup_rotation(igt_plane_t *plane,
@@ -3467,8 +3478,7 @@ display_commit_changed(igt_display_t *display, enum igt_commit_style s)
 				igt_plane_clear_prop_changed(plane, IGT_PLANE_FB_ID);
 
 				if (s != COMMIT_LEGACY ||
-				    !(plane->type == DRM_PLANE_TYPE_PRIMARY ||
-				      plane->type == DRM_PLANE_TYPE_CURSOR))
+				    plane->type != DRM_PLANE_TYPE_CURSOR)
 					plane->changed &= ~LEGACY_PLANE_COMMIT_MASK;
 
 				if (display->first_commit)
-- 
2.26.3

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

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