On Tue, Dec 10, 2019 at 11:20:46AM +0100, Maxime Ripard wrote: > Hi, > > On Mon, Dec 09, 2019 at 07:32:54PM +0100, Stephan Gerhold wrote: > > At the moment, video mode parameters like video=540x960,reflect_x, > > (without rotation set) are silently ignored. > > > > 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> > > Thanks for that commit message. > > Can you also add a selftest to make sure we don't get a regression in > the future? Can you explain how/where I would add a test for drm_client_rotation() in drm_client_modeset.c? I'm not familiar with selftests to be honest. I found test-drm_cmdline_parser.c but that seems to cover only the cmdline parsing (which is working correctly already). Thanks, Stephan _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
