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@xxxxxxxxxxxxxxx>
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@xxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
