On Mon, Dec 16, 2019 at 06:10:17PM +0100, Stephan Gerhold wrote: > At the moment, video mode parameters like video=540x960,reflect_x, > (without rotation set) are not taken into account when applying the > mode in drm_client_modeset.c. A rotation value without exactly one rotation angle is illegal. IMO the code should not generate a value like that in the first place. > > One of the reasons for this is that the calculation that > combines the panel_orientation with cmdline->rotation_reflection > does not handle the case when cmdline->rotation_reflection does > not have any rotation set. > (i.e. cmdline->rotation_reflection & DRM_MODE_ROTATE_MASK == 0) > > Example: > *rotation = DRM_MODE_ROTATE_0 (no panel_orientation) > cmdline->rotation_reflection = DRM_MODE_REFLECT_X (video=MODE,reflect_x) > > The current code does: > panel_rot = ilog2(*rotation & DRM_MODE_ROTATE_MASK); > cmdline_rot = ilog2(cmdline->rotation_reflection & DRM_MODE_ROTATE_MASK); > sum_rot = (panel_rot + cmdline_rot) % 4; > > and therefore: > panel_rot = ilog2(DRM_MODE_ROTATE_0) = ilog2(1) = 0 > cmdline_rot = ilog2(0) = -1 > sum_rot = (0 + -1) % 4 = -1 % 4 = 3 > ... > *rotation = DRM_MODE_ROTATE_270 | DRM_MODE_REFLECT_X > > So we incorrectly generate DRM_MODE_ROTATE_270 in this case. > To prevent cmdline_rot from becoming -1, we need to treat > the rotation as DRM_MODE_ROTATE_0. > > On the other hand, there is no need to go through that calculation > at all if no rotation is set in rotation_reflection. > A simple XOR is enough to combine the reflections. > > Finally, also allow DRM_MODE_ROTATE_0 in the if statement below. > DRM_MODE_ROTATE_0 means "no rotation" and should therefore not > require any special handling (e.g. specific tiling format). > > This makes video parameters with only reflect option work correctly. > > Signed-off-by: Stephan Gerhold <stephan@xxxxxxxxxxx> > --- > v1: https://lists.freedesktop.org/archives/dri-devel/2019-December/248145.html > > Changes in v2: > - Clarified commit message - parameters are parsed correctly, > but not taken into account when applying the mode. > --- > drivers/gpu/drm/drm_client_modeset.c | 27 ++++++++++++++++----------- > 1 file changed, 16 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c > index 895b73f23079..cfebce4f19a5 100644 > --- a/drivers/gpu/drm/drm_client_modeset.c > +++ b/drivers/gpu/drm/drm_client_modeset.c > @@ -859,19 +859,23 @@ bool drm_client_rotation(struct drm_mode_set *modeset, unsigned int *rotation) > */ > cmdline = &connector->cmdline_mode; > if (cmdline->specified && cmdline->rotation_reflection) { > - unsigned int cmdline_rest, panel_rest; > - unsigned int cmdline_rot, panel_rot; > - unsigned int sum_rot, sum_rest; > + if (cmdline->rotation_reflection & DRM_MODE_ROTATE_MASK) { > + unsigned int cmdline_rest, panel_rest; > + unsigned int cmdline_rot, panel_rot; > + unsigned int sum_rot, sum_rest; > > - panel_rot = ilog2(*rotation & DRM_MODE_ROTATE_MASK); > - cmdline_rot = ilog2(cmdline->rotation_reflection & DRM_MODE_ROTATE_MASK); > - sum_rot = (panel_rot + cmdline_rot) % 4; > + panel_rot = ilog2(*rotation & DRM_MODE_ROTATE_MASK); > + cmdline_rot = ilog2(cmdline->rotation_reflection & DRM_MODE_ROTATE_MASK); > + sum_rot = (panel_rot + cmdline_rot) % 4; > > - panel_rest = *rotation & ~DRM_MODE_ROTATE_MASK; > - cmdline_rest = cmdline->rotation_reflection & ~DRM_MODE_ROTATE_MASK; > - sum_rest = panel_rest ^ cmdline_rest; > + panel_rest = *rotation & ~DRM_MODE_ROTATE_MASK; > + cmdline_rest = cmdline->rotation_reflection & ~DRM_MODE_ROTATE_MASK; > + sum_rest = panel_rest ^ cmdline_rest; > > - *rotation = (1 << sum_rot) | sum_rest; > + *rotation = (1 << sum_rot) | sum_rest; > + } else { > + *rotation ^= cmdline->rotation_reflection; > + } > } > > /* > @@ -879,7 +883,8 @@ bool drm_client_rotation(struct drm_mode_set *modeset, unsigned int *rotation) > * depending on the hardware this may require the framebuffer > * to be in a specific tiling format. > */ > - if ((*rotation & DRM_MODE_ROTATE_MASK) != DRM_MODE_ROTATE_180 || > + if (((*rotation & DRM_MODE_ROTATE_MASK) != DRM_MODE_ROTATE_0 && > + (*rotation & DRM_MODE_ROTATE_MASK) != DRM_MODE_ROTATE_180) || > !plane->rotation_property) > return false; > > -- > 2.24.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Ville Syrjälä Intel _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
