| Message ID | 20191209183254.211428-1-stephan@gerhold.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/modes: Support video parameters with only reflect option | expand |
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@gerhold.net> Thanks for that commit message. Can you also add a selftest to make sure we don't get a regression in the future? Thanks! Maxime
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@gerhold.net> > > 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
Hi Stephan, On Tue, Dec 10, 2019 at 11:42:37AM +0100, Stephan Gerhold wrote: > 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@gerhold.net> > > > > 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). The cmdline here is the kernel command line. You were mentionning in your commit log that video=540x960,reflect_x was broken? But yeah, I meant this one. Maxime
On Wed, Dec 11, 2019 at 07:10:46PM +0100, Maxime Ripard wrote: > Hi Stephan, > > On Tue, Dec 10, 2019 at 11:42:37AM +0100, Stephan Gerhold wrote: > > 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@gerhold.net> > > > > > > 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). > > The cmdline here is the kernel command line. You were mentionning in > your commit log that video=540x960,reflect_x was broken? > The parameter is parsed correctly and placed into connector->cmdline_mode. Therefore, not the *parsing* is broken, only the way we try to apply and merge them with the panel orientation in drm_client_modeset.c. There are existing test cases for the parsing of parameters similar to video=540x960,reflect_x, see drm_cmdline_test_hmirror() in the aforementioned test file. Maybe my commit message was not as clear as I hoped :) Thanks, Stephan
Hi Stephan, On Wed, Dec 11, 2019 at 08:08:39PM +0100, Stephan Gerhold wrote: > On Wed, Dec 11, 2019 at 07:10:46PM +0100, Maxime Ripard wrote: > > On Tue, Dec 10, 2019 at 11:42:37AM +0100, Stephan Gerhold wrote: > > > 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@gerhold.net> > > > > > > > > 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). > > > > The cmdline here is the kernel command line. You were mentionning in > > your commit log that video=540x960,reflect_x was broken? > > > > The parameter is parsed correctly and placed into connector->cmdline_mode. > Therefore, not the *parsing* is broken, only the way we try to apply > and merge them with the panel orientation in drm_client_modeset.c. > > There are existing test cases for the parsing of parameters similar to > video=540x960,reflect_x, see drm_cmdline_test_hmirror() > in the aforementioned test file. > > Maybe my commit message was not as clear as I hoped :) Ah, I see what you meant by "silently ignored" now :) Yeah, maybe we can change that by "not taken into account when applying the mode" or something like that? Maxime
On Fri, Dec 13, 2019 at 10:39:50AM +0100, Maxime Ripard wrote: > Hi Stephan, > > On Wed, Dec 11, 2019 at 08:08:39PM +0100, Stephan Gerhold wrote: > > On Wed, Dec 11, 2019 at 07:10:46PM +0100, Maxime Ripard wrote: > > > On Tue, Dec 10, 2019 at 11:42:37AM +0100, Stephan Gerhold wrote: > > > > 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@gerhold.net> > > > > > > > > > > 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). > > > > > > The cmdline here is the kernel command line. You were mentionning in > > > your commit log that video=540x960,reflect_x was broken? > > > > > > > The parameter is parsed correctly and placed into connector->cmdline_mode. > > Therefore, not the *parsing* is broken, only the way we try to apply > > and merge them with the panel orientation in drm_client_modeset.c. > > > > There are existing test cases for the parsing of parameters similar to > > video=540x960,reflect_x, see drm_cmdline_test_hmirror() > > in the aforementioned test file. > > > > Maybe my commit message was not as clear as I hoped :) > > Ah, I see what you meant by "silently ignored" now :) > > Yeah, maybe we can change that by "not taken into account when > applying the mode" or something like that? Sure, I will try to clarify it for v2. Anything else I should change? Thanks, Stephan
On Fri, Dec 13, 2019 at 01:16:19PM +0100, Stephan Gerhold wrote: > On Fri, Dec 13, 2019 at 10:39:50AM +0100, Maxime Ripard wrote: > > Hi Stephan, > > > > On Wed, Dec 11, 2019 at 08:08:39PM +0100, Stephan Gerhold wrote: > > > On Wed, Dec 11, 2019 at 07:10:46PM +0100, Maxime Ripard wrote: > > > > On Tue, Dec 10, 2019 at 11:42:37AM +0100, Stephan Gerhold wrote: > > > > > 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@gerhold.net> > > > > > > > > > > > > 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). > > > > > > > > The cmdline here is the kernel command line. You were mentionning in > > > > your commit log that video=540x960,reflect_x was broken? > > > > > > > > > > The parameter is parsed correctly and placed into connector->cmdline_mode. > > > Therefore, not the *parsing* is broken, only the way we try to apply > > > and merge them with the panel orientation in drm_client_modeset.c. > > > > > > There are existing test cases for the parsing of parameters similar to > > > video=540x960,reflect_x, see drm_cmdline_test_hmirror() > > > in the aforementioned test file. > > > > > > Maybe my commit message was not as clear as I hoped :) > > > > Ah, I see what you meant by "silently ignored" now :) > > > > Yeah, maybe we can change that by "not taken into account when > > applying the mode" or something like that? > > Sure, I will try to clarify it for v2. > > Anything else I should change? The rest looks fine to me. Thanks! Maxime
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;
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@gerhold.net> --- drivers/gpu/drm/drm_client_modeset.c | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-)