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.
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)
However, we really should not generate such a value in the first place. Rotation values should have exactly one rotation angle set (DRM_MODE_ROTATE_0 for "no rotation").
This patch set changes the command line parser to make sure that we generate only valid rotation values (defaulting to DRM_MODE_ROTATE_0).
Finally it allows DRM_MODE_ROTATE_0 when applying the rotation from the video mode parameters to make parameters without rotation work correctly.
Changes in v3: - Fix the cmdline parser to generate only valid rotation values instead. (There should be exactly one rotation angle in a rotation value.) - Removed most of the original patch - combining the rotations works correctly if we only have valid rotation values
v2: https://lore.kernel.org/dri-devel/20191216171017.173326-1-stephan@gerhold.ne... - Clarified commit message - parameters are parsed correctly, but not taken into account when applying the mode. v1: https://lore.kernel.org/dri-devel/20191209183254.211428-1-stephan@gerhold.ne...
Stephan Gerhold (2): drm/modes: Make sure to parse valid rotation value from cmdline drm/modes: Allow DRM_MODE_ROTATE_0 when applying video mode parameters
drivers/gpu/drm/drm_client_modeset.c | 3 ++- drivers/gpu/drm/drm_modes.c | 7 +++++++ drivers/gpu/drm/selftests/drm_cmdline_selftests.h | 1 + .../gpu/drm/selftests/test-drm_cmdline_parser.c | 15 +++++++++++++-- 4 files changed, 23 insertions(+), 3 deletions(-)
A rotation value should have exactly one rotation angle. At the moment there is no validation for this when parsing video= parameters from the command line. This causes problems later on when we try to combine the command line rotation with the panel orientation.
To make sure that we generate a valid rotation value: - Set DRM_MODE_ROTATE_0 by default (if no rotate= option is set) - Validate that there is exactly one rotation angle set (i.e. specifying the rotate= option multiple times is invalid)
Signed-off-by: Stephan Gerhold stephan@gerhold.net --- drivers/gpu/drm/drm_modes.c | 7 +++++++ drivers/gpu/drm/selftests/drm_cmdline_selftests.h | 1 + .../gpu/drm/selftests/test-drm_cmdline_parser.c | 15 +++++++++++++-- 3 files changed, 21 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index 10336b144c72..d4d64518e11b 100644 --- a/drivers/gpu/drm/drm_modes.c +++ b/drivers/gpu/drm/drm_modes.c @@ -1698,6 +1698,13 @@ static int drm_mode_parse_cmdline_options(const char *str, if (rotation && freestanding) return -EINVAL;
+ if (!(rotation & DRM_MODE_ROTATE_MASK)) + rotation |= DRM_MODE_ROTATE_0; + + /* Make sure there is exactly one rotation defined */ + if (!is_power_of_2(rotation & DRM_MODE_ROTATE_MASK)) + return -EINVAL; + mode->rotation_reflection = rotation;
return 0; diff --git a/drivers/gpu/drm/selftests/drm_cmdline_selftests.h b/drivers/gpu/drm/selftests/drm_cmdline_selftests.h index ceac7af9a172..29e367db6118 100644 --- a/drivers/gpu/drm/selftests/drm_cmdline_selftests.h +++ b/drivers/gpu/drm/selftests/drm_cmdline_selftests.h @@ -53,6 +53,7 @@ cmdline_test(drm_cmdline_test_rotate_0) cmdline_test(drm_cmdline_test_rotate_90) cmdline_test(drm_cmdline_test_rotate_180) cmdline_test(drm_cmdline_test_rotate_270) +cmdline_test(drm_cmdline_test_rotate_multiple) cmdline_test(drm_cmdline_test_rotate_invalid_val) cmdline_test(drm_cmdline_test_rotate_truncated) cmdline_test(drm_cmdline_test_hmirror) diff --git a/drivers/gpu/drm/selftests/test-drm_cmdline_parser.c b/drivers/gpu/drm/selftests/test-drm_cmdline_parser.c index 520f3e66a384..d96cd890def6 100644 --- a/drivers/gpu/drm/selftests/test-drm_cmdline_parser.c +++ b/drivers/gpu/drm/selftests/test-drm_cmdline_parser.c @@ -856,6 +856,17 @@ static int drm_cmdline_test_rotate_270(void *ignored) return 0; }
+static int drm_cmdline_test_rotate_multiple(void *ignored) +{ + struct drm_cmdline_mode mode = { }; + + FAIL_ON(drm_mode_parse_command_line_for_connector("720x480,rotate=0,rotate=90", + &no_connector, + &mode)); + + return 0; +} + static int drm_cmdline_test_rotate_invalid_val(void *ignored) { struct drm_cmdline_mode mode = { }; @@ -888,7 +899,7 @@ static int drm_cmdline_test_hmirror(void *ignored) FAIL_ON(!mode.specified); FAIL_ON(mode.xres != 720); FAIL_ON(mode.yres != 480); - FAIL_ON(mode.rotation_reflection != DRM_MODE_REFLECT_X); + FAIL_ON(mode.rotation_reflection != (DRM_MODE_ROTATE_0 | DRM_MODE_REFLECT_X));
FAIL_ON(mode.refresh_specified);
@@ -913,7 +924,7 @@ static int drm_cmdline_test_vmirror(void *ignored) FAIL_ON(!mode.specified); FAIL_ON(mode.xres != 720); FAIL_ON(mode.yres != 480); - FAIL_ON(mode.rotation_reflection != DRM_MODE_REFLECT_Y); + FAIL_ON(mode.rotation_reflection != (DRM_MODE_ROTATE_0 | DRM_MODE_REFLECT_Y));
FAIL_ON(mode.refresh_specified);
At the moment, only DRM_MODE_ROTATE_180 is allowed when we try to apply the rotation from the video mode parameters. It is also useful to allow DRM_MODE_ROTATE_0 in case there is only a reflect option in the video mode parameter (e.g. video=540x960,reflect_x).
DRM_MODE_ROTATE_0 means "no rotation" and should therefore not require any special handling, so we can just add it to the if condition.
Signed-off-by: Stephan Gerhold stephan@gerhold.net --- drivers/gpu/drm/drm_client_modeset.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c index 895b73f23079..29367b6506a8 100644 --- a/drivers/gpu/drm/drm_client_modeset.c +++ b/drivers/gpu/drm/drm_client_modeset.c @@ -879,7 +879,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;
On Fri, Jan 17, 2020 at 04:34:27PM +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.
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)
However, we really should not generate such a value in the first place. Rotation values should have exactly one rotation angle set (DRM_MODE_ROTATE_0 for "no rotation").
This patch set changes the command line parser to make sure that we generate only valid rotation values (defaulting to DRM_MODE_ROTATE_0).
Finally it allows DRM_MODE_ROTATE_0 when applying the rotation from the video mode parameters to make parameters without rotation work correctly.
For both, Acked-by: Maxime Ripard mripard@kernel.org
Thanks! Maxime
Hi Maxime,
On Fri, Jan 17, 2020 at 07:51:00PM +0100, Maxime Ripard wrote:
On Fri, Jan 17, 2020 at 04:34:27PM +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.
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)
However, we really should not generate such a value in the first place. Rotation values should have exactly one rotation angle set (DRM_MODE_ROTATE_0 for "no rotation").
This patch set changes the command line parser to make sure that we generate only valid rotation values (defaulting to DRM_MODE_ROTATE_0).
Finally it allows DRM_MODE_ROTATE_0 when applying the rotation from the video mode parameters to make parameters without rotation work correctly.
For both, Acked-by: Maxime Ripard mripard@kernel.org
Can you apply these two patches for me? This is my second contribution to the DRM subsystem, so I don't have commit access to drm-misc yet.
Thanks, Stephan
On Wed, Feb 12, 2020 at 04:29:48PM +0100, Stephan Gerhold wrote:
Hi Maxime,
On Fri, Jan 17, 2020 at 07:51:00PM +0100, Maxime Ripard wrote:
On Fri, Jan 17, 2020 at 04:34:27PM +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.
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)
However, we really should not generate such a value in the first place. Rotation values should have exactly one rotation angle set (DRM_MODE_ROTATE_0 for "no rotation").
This patch set changes the command line parser to make sure that we generate only valid rotation values (defaulting to DRM_MODE_ROTATE_0).
Finally it allows DRM_MODE_ROTATE_0 when applying the rotation from the video mode parameters to make parameters without rotation work correctly.
For both, Acked-by: Maxime Ripard mripard@kernel.org
Can you apply these two patches for me? This is my second contribution to the DRM subsystem, so I don't have commit access to drm-misc yet.
Applied both, thanks! Maxime
dri-devel@lists.freedesktop.org
-
Maxime Ripard -
Maxime Ripard -
Stephan Gerhold