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