[PATCH i-g-t] lib/igt_kms: Handle changing rotation property correctly

[PATCH i-g-t] lib/igt_kms: Handle changing rotation property correctly

[Maarten Lankhorst]

The rotation property sucks because it may affect whether
drmModeSetPlane succeeds or not. Add some code to handle
this.

First try to set rotation directly, if that succeeds we
return immediately. If it fails we disable the plane, set
the rotation property and run the rest of the code.

This will hopefully make legacy rotation work in more cases when
scaling is not supported.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 lib/igt_kms.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 lib/igt_kms.h |  1 +
 2 files changed, 66 insertions(+), 6 deletions(-)

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index f144f0d395fc..92dcd3cad4aa 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -1604,11 +1604,8 @@ static void igt_plane_reset(igt_plane_t *plane)
 	igt_plane_set_prop_value(plane, IGT_PLANE_CRTC_ID, 0);
 
 	/* Use default rotation */
-	if (igt_plane_has_prop(plane, IGT_PLANE_ROTATION)) {
-		plane->values[IGT_PLANE_ROTATION] =
-			igt_plane_get_prop(plane, IGT_PLANE_ROTATION);
-		igt_plane_clear_prop_changed(plane, IGT_PLANE_ROTATION);
-	}
+	if (igt_plane_has_prop(plane, IGT_PLANE_ROTATION))
+		igt_plane_set_prop_value(plane, IGT_PLANE_ROTATION, IGT_ROTATION_0);
 
 	igt_plane_clear_prop_changed(plane, IGT_PLANE_IN_FENCE_FD);
 	plane->values[IGT_PLANE_IN_FENCE_FD] = ~0ULL;
@@ -1666,6 +1663,12 @@ void igt_display_reset(igt_display_t *display)
 	enum pipe pipe;
 	int i;
 
+	/*
+	 * Allow resetting rotation on all planes, which is normally
+	 * prohibited on the primary and cursor plane for legacy commits.
+	 */
+	display->first_commit = true;
+
 	for_each_pipe(display, pipe) {
 		igt_pipe_t *pipe_obj = &display->pipes[pipe];
 		igt_plane_t *plane;
@@ -2298,7 +2301,8 @@ static int igt_primary_plane_commit_legacy(igt_plane_t *primary,
 	igt_assert((primary->values[IGT_PLANE_CRTC_X] == 0 && primary->values[IGT_PLANE_CRTC_Y] == 0));
 
 	/* nor rotated */
-	igt_assert(!igt_plane_is_prop_changed(primary, IGT_PLANE_ROTATION));
+	if (!pipe->display->first_commit)
+		igt_assert(!igt_plane_is_prop_changed(primary, IGT_PLANE_ROTATION));
 
 	if (!igt_plane_is_prop_changed(primary, IGT_PLANE_FB_ID) &&
 	    !(primary->changed & IGT_PLANE_COORD_CHANGED_MASK) &&
@@ -2351,6 +2355,48 @@ static int igt_primary_plane_commit_legacy(igt_plane_t *primary,
 	return 0;
 }
 
+static int igt_plane_fixup_rotation(igt_plane_t *plane,
+				    igt_pipe_t *pipe)
+{
+	int ret;
+
+	if (!igt_plane_has_prop(plane, IGT_PLANE_ROTATION))
+		return 0;
+
+	LOG(pipe->display, "Fixing up initial rotation pipe %s, plane %d\n",
+	    kmstest_pipe_name(pipe->pipe), plane->index);
+
+	/* First try the easy case, can we change rotation without problems? */
+	ret = igt_plane_set_property(plane, plane->props[IGT_PLANE_ROTATION],
+				     plane->values[IGT_PLANE_ROTATION]);
+	if (!ret)
+		return 0;
+
+	/* Disable the plane, while we tinker with rotation */
+	ret = drmModeSetPlane(pipe->display->drm_fd,
+			      plane->drm_plane->plane_id,
+			      pipe->crtc_id, 0, /* fb_id */
+			      0, /* flags */
+			      0, 0, 0, 0, /* crtc_x, crtc_y, crtc_w, crtc_h */
+			      IGT_FIXED(0,0), IGT_FIXED(0,0), /* src_x, src_y */
+			      IGT_FIXED(0,0), IGT_FIXED(0,0)); /* src_w, src_h */
+
+	if (ret && plane->type != DRM_PLANE_TYPE_PRIMARY)
+		return ret;
+
+	/* For primary plane, fall back to disabling the crtc. */
+	if (ret) {
+		ret = drmModeSetCrtc(pipe->display->drm_fd,
+				     pipe->crtc_id, 0, 0, 0, NULL, 0, NULL);
+
+		if (ret)
+			return ret;
+	}
+
+	/* and finally, set rotation property. */
+	return igt_plane_set_property(plane, plane->props[IGT_PLANE_ROTATION],
+				      plane->values[IGT_PLANE_ROTATION]);
+}
 
 /*
  * Commit position and fb changes to a plane.  The value of @s will determine
@@ -2361,6 +2407,14 @@ static int igt_plane_commit(igt_plane_t *plane,
 			    enum igt_commit_style s,
 			    bool fail_on_error)
 {
+	if (pipe->display->first_commit || (s == COMMIT_UNIVERSAL &&
+	     igt_plane_is_prop_changed(plane, IGT_PLANE_ROTATION))) {
+		int ret;
+
+		ret = igt_plane_fixup_rotation(plane, pipe);
+		CHECK_RETURN(ret, fail_on_error);
+	}
+
 	if (plane->type == DRM_PLANE_TYPE_CURSOR && s == COMMIT_LEGACY) {
 		return igt_cursor_commit_legacy(plane, pipe, fail_on_error);
 	} else if (plane->type == DRM_PLANE_TYPE_PRIMARY && s == COMMIT_LEGACY) {
@@ -2783,6 +2837,9 @@ display_commit_changed(igt_display_t *display, enum igt_commit_style s)
 				    !(plane->type == DRM_PLANE_TYPE_PRIMARY ||
 				      plane->type == DRM_PLANE_TYPE_CURSOR))
 					plane->changed &= ~LEGACY_PLANE_COMMIT_MASK;
+
+				if (display->first_commit)
+					igt_plane_clear_prop_changed(plane, IGT_PLANE_ROTATION);
 			}
 		}
 	}
@@ -2796,6 +2853,8 @@ display_commit_changed(igt_display_t *display, enum igt_commit_style s)
 			/* no modeset in universal commit, no change to crtc. */
 			output->changed &= 1 << IGT_CONNECTOR_CRTC_ID;
 	}
+
+	display->first_commit = false;
 }
 
 /*
diff --git a/lib/igt_kms.h b/lib/igt_kms.h
index e1883bf1b8a3..2a480bf39956 100644
--- a/lib/igt_kms.h
+++ b/lib/igt_kms.h
@@ -349,6 +349,7 @@ struct igt_display {
 	igt_pipe_t *pipes;
 	bool has_cursor_plane;
 	bool is_atomic;
+	bool first_commit;
 };
 
 void igt_display_init(igt_display_t *display, int drm_fd);
-- 
2.15.0

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

Re: [PATCH i-g-t] lib/igt_kms: Handle changing rotation property correctly

[Daniel Vetter]

On Wed, Nov 22, 2017 at 10:50:57AM +0100, Maarten Lankhorst wrote:
> The rotation property sucks because it may affect whether
> drmModeSetPlane succeeds or not. Add some code to handle
> this.
> 
> First try to set rotation directly, if that succeeds we
> return immediately. If it fails we disable the plane, set
> the rotation property and run the rest of the code.
> 
> This will hopefully make legacy rotation work in more cases when
> scaling is not supported.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

It's kinda why we have atomic. And strictly speaking, almost anything
could be affected by this with the legacy plane api, requiring a disable
plane, upate props, enable plane sequence.

I guess we should just aim for more atomic in igts :-)

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  lib/igt_kms.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
>  lib/igt_kms.h |  1 +
>  2 files changed, 66 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index f144f0d395fc..92dcd3cad4aa 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -1604,11 +1604,8 @@ static void igt_plane_reset(igt_plane_t *plane)
>  	igt_plane_set_prop_value(plane, IGT_PLANE_CRTC_ID, 0);
>  
>  	/* Use default rotation */
> -	if (igt_plane_has_prop(plane, IGT_PLANE_ROTATION)) {
> -		plane->values[IGT_PLANE_ROTATION] =
> -			igt_plane_get_prop(plane, IGT_PLANE_ROTATION);
> -		igt_plane_clear_prop_changed(plane, IGT_PLANE_ROTATION);
> -	}
> +	if (igt_plane_has_prop(plane, IGT_PLANE_ROTATION))
> +		igt_plane_set_prop_value(plane, IGT_PLANE_ROTATION, IGT_ROTATION_0);
>  
>  	igt_plane_clear_prop_changed(plane, IGT_PLANE_IN_FENCE_FD);
>  	plane->values[IGT_PLANE_IN_FENCE_FD] = ~0ULL;
> @@ -1666,6 +1663,12 @@ void igt_display_reset(igt_display_t *display)
>  	enum pipe pipe;
>  	int i;
>  
> +	/*
> +	 * Allow resetting rotation on all planes, which is normally
> +	 * prohibited on the primary and cursor plane for legacy commits.
> +	 */
> +	display->first_commit = true;
> +
>  	for_each_pipe(display, pipe) {
>  		igt_pipe_t *pipe_obj = &display->pipes[pipe];
>  		igt_plane_t *plane;
> @@ -2298,7 +2301,8 @@ static int igt_primary_plane_commit_legacy(igt_plane_t *primary,
>  	igt_assert((primary->values[IGT_PLANE_CRTC_X] == 0 && primary->values[IGT_PLANE_CRTC_Y] == 0));
>  
>  	/* nor rotated */
> -	igt_assert(!igt_plane_is_prop_changed(primary, IGT_PLANE_ROTATION));
> +	if (!pipe->display->first_commit)
> +		igt_assert(!igt_plane_is_prop_changed(primary, IGT_PLANE_ROTATION));
>  
>  	if (!igt_plane_is_prop_changed(primary, IGT_PLANE_FB_ID) &&
>  	    !(primary->changed & IGT_PLANE_COORD_CHANGED_MASK) &&
> @@ -2351,6 +2355,48 @@ static int igt_primary_plane_commit_legacy(igt_plane_t *primary,
>  	return 0;
>  }
>  
> +static int igt_plane_fixup_rotation(igt_plane_t *plane,
> +				    igt_pipe_t *pipe)
> +{
> +	int ret;
> +
> +	if (!igt_plane_has_prop(plane, IGT_PLANE_ROTATION))
> +		return 0;
> +
> +	LOG(pipe->display, "Fixing up initial rotation pipe %s, plane %d\n",
> +	    kmstest_pipe_name(pipe->pipe), plane->index);
> +
> +	/* First try the easy case, can we change rotation without problems? */
> +	ret = igt_plane_set_property(plane, plane->props[IGT_PLANE_ROTATION],
> +				     plane->values[IGT_PLANE_ROTATION]);
> +	if (!ret)
> +		return 0;
> +
> +	/* Disable the plane, while we tinker with rotation */
> +	ret = drmModeSetPlane(pipe->display->drm_fd,
> +			      plane->drm_plane->plane_id,
> +			      pipe->crtc_id, 0, /* fb_id */
> +			      0, /* flags */
> +			      0, 0, 0, 0, /* crtc_x, crtc_y, crtc_w, crtc_h */
> +			      IGT_FIXED(0,0), IGT_FIXED(0,0), /* src_x, src_y */
> +			      IGT_FIXED(0,0), IGT_FIXED(0,0)); /* src_w, src_h */
> +
> +	if (ret && plane->type != DRM_PLANE_TYPE_PRIMARY)
> +		return ret;
> +
> +	/* For primary plane, fall back to disabling the crtc. */
> +	if (ret) {
> +		ret = drmModeSetCrtc(pipe->display->drm_fd,
> +				     pipe->crtc_id, 0, 0, 0, NULL, 0, NULL);
> +
> +		if (ret)
> +			return ret;
> +	}
> +
> +	/* and finally, set rotation property. */
> +	return igt_plane_set_property(plane, plane->props[IGT_PLANE_ROTATION],
> +				      plane->values[IGT_PLANE_ROTATION]);
> +}
>  
>  /*
>   * Commit position and fb changes to a plane.  The value of @s will determine
> @@ -2361,6 +2407,14 @@ static int igt_plane_commit(igt_plane_t *plane,
>  			    enum igt_commit_style s,
>  			    bool fail_on_error)
>  {
> +	if (pipe->display->first_commit || (s == COMMIT_UNIVERSAL &&
> +	     igt_plane_is_prop_changed(plane, IGT_PLANE_ROTATION))) {
> +		int ret;
> +
> +		ret = igt_plane_fixup_rotation(plane, pipe);
> +		CHECK_RETURN(ret, fail_on_error);
> +	}
> +
>  	if (plane->type == DRM_PLANE_TYPE_CURSOR && s == COMMIT_LEGACY) {
>  		return igt_cursor_commit_legacy(plane, pipe, fail_on_error);
>  	} else if (plane->type == DRM_PLANE_TYPE_PRIMARY && s == COMMIT_LEGACY) {
> @@ -2783,6 +2837,9 @@ display_commit_changed(igt_display_t *display, enum igt_commit_style s)
>  				    !(plane->type == DRM_PLANE_TYPE_PRIMARY ||
>  				      plane->type == DRM_PLANE_TYPE_CURSOR))
>  					plane->changed &= ~LEGACY_PLANE_COMMIT_MASK;
> +
> +				if (display->first_commit)
> +					igt_plane_clear_prop_changed(plane, IGT_PLANE_ROTATION);
>  			}
>  		}
>  	}
> @@ -2796,6 +2853,8 @@ display_commit_changed(igt_display_t *display, enum igt_commit_style s)
>  			/* no modeset in universal commit, no change to crtc. */
>  			output->changed &= 1 << IGT_CONNECTOR_CRTC_ID;
>  	}
> +
> +	display->first_commit = false;
>  }
>  
>  /*
> diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> index e1883bf1b8a3..2a480bf39956 100644
> --- a/lib/igt_kms.h
> +++ b/lib/igt_kms.h
> @@ -349,6 +349,7 @@ struct igt_display {
>  	igt_pipe_t *pipes;
>  	bool has_cursor_plane;
>  	bool is_atomic;
> +	bool first_commit;
>  };
>  
>  void igt_display_init(igt_display_t *display, int drm_fd);
> -- 
> 2.15.0
> 
> _______________________________________________
> 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 i-g-t] lib/igt_kms: Handle changing rotation property correctly

[Maarten Lankhorst]

Op 22-11-17 om 11:06 schreef Daniel Vetter:
> On Wed, Nov 22, 2017 at 10:50:57AM +0100, Maarten Lankhorst wrote:
>> The rotation property sucks because it may affect whether
>> drmModeSetPlane succeeds or not. Add some code to handle
>> this.
>>
>> First try to set rotation directly, if that succeeds we
>> return immediately. If it fails we disable the plane, set
>> the rotation property and run the rest of the code.
>>
>> This will hopefully make legacy rotation work in more cases when
>> scaling is not supported.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> It's kinda why we have atomic. And strictly speaking, almost anything
> could be affected by this with the legacy plane api, requiring a disable
> plane, upate props, enable plane sequence.
>
> I guess we should just aim for more atomic in igts :-)
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

In general only when you have to update 2 incompatible properties at the
same time, rotation is just a very special case we care about because
there are patches that may allow fbcon to be rotated.

A lot of code in IGT has the following setup pattern:

igt_output_set_pipe(output, pipe);
igt_plane_set_fb(primary, fb);
igt_display_commit();

I want this to continue to work in case of rotated FBCON, without having to
special case each test. I would also not mind if we make the first commit
always atomic, if possible. But that wouldn't work with legacy rotated fbcon,
so this workaround would still be needed. :)

Pushed, thanks for review.

~Maarten

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

✗ Fi.CI.BAT: failure for lib/igt_kms: Handle changing rotation property correctly

[Patchwork]

== Series Details ==

Series: lib/igt_kms: Handle changing rotation property correctly
URL   : https://patchwork.freedesktop.org/series/34210/
State : failure

== Summary ==

Series 34210 revision 1 was fully merged or fully failed: no git log

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