| Message ID | 20191110154101.26486-10-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/modes: parse_cmdline: Add support for specifying panel_orientation on the kernel cmdline | expand |
Hi Hans, Thanks for this series (and thanks for bouncing the mails too). All the previous patches are Acked-by: Maxime Ripard <mripard@kernel.org> On Sun, Nov 10, 2019 at 04:40:58PM +0100, Hans de Goede wrote: > Sometimes we want to override a connector's panel_orientation from the > kernel commandline. Either for testing and for special cases, e.g. a kiosk > like setup which uses a TV mounted in portrait mode. > > Users can already specify a "rotate" option through a video= kernel cmdline > option. But that only supports 0/180 degrees (see drm_client_modeset TODO) > and only works for in kernel modeset clients, not for userspace kms users. > > The "panel-orientation" connector property OTOH does support 90/270 degrees > as it leaves dealing with the rotation up to userspace and this does work > for userspace kms clients (at least those which support this property). > > BugLink: https://gitlab.freedesktop.org/plymouth/plymouth/merge_requests/83 > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > Documentation/fb/modedb.rst | 3 ++ > drivers/gpu/drm/drm_modes.c | 32 +++++++++++++++++++ > .../gpu/drm/selftests/drm_cmdline_selftests.h | 1 + > .../drm/selftests/test-drm_cmdline_parser.c | 22 +++++++++++++ > include/drm/drm_connector.h | 8 +++++ > 5 files changed, 66 insertions(+) > > diff --git a/Documentation/fb/modedb.rst b/Documentation/fb/modedb.rst > index 9c4e3fd39e6d..624d08fd2856 100644 > --- a/Documentation/fb/modedb.rst > +++ b/Documentation/fb/modedb.rst > @@ -65,6 +65,9 @@ Valid options are:: > - reflect_y (boolean): Perform an axial symmetry on the Y axis > - rotate (integer): Rotate the initial framebuffer by x > degrees. Valid values are 0, 90, 180 and 270. > + - panel_orientation, one of "normal", "upside_down", "left_side_up", or > + "right_side_up". For KMS drivers only, this sets the "panel orientation" > + property on the kms connector as hint for kms users. Even though the semantic is a bit different, I think we should remain consistent and have the same argument than for rotate (ie, steps in clockwise rotation in steps of 90 degrees). The rest looks good Maxime
Hi, On 11-11-2019 13:53, Maxime Ripard wrote: > Hi Hans, > > Thanks for this series (and thanks for bouncing the mails too). > > All the previous patches are > Acked-by: Maxime Ripard <mripard@kernel.org> Thank you for the review. > On Sun, Nov 10, 2019 at 04:40:58PM +0100, Hans de Goede wrote: >> Sometimes we want to override a connector's panel_orientation from the >> kernel commandline. Either for testing and for special cases, e.g. a kiosk >> like setup which uses a TV mounted in portrait mode. >> >> Users can already specify a "rotate" option through a video= kernel cmdline >> option. But that only supports 0/180 degrees (see drm_client_modeset TODO) >> and only works for in kernel modeset clients, not for userspace kms users. >> >> The "panel-orientation" connector property OTOH does support 90/270 degrees >> as it leaves dealing with the rotation up to userspace and this does work >> for userspace kms clients (at least those which support this property). >> >> BugLink: https://gitlab.freedesktop.org/plymouth/plymouth/merge_requests/83 >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> Documentation/fb/modedb.rst | 3 ++ >> drivers/gpu/drm/drm_modes.c | 32 +++++++++++++++++++ >> .../gpu/drm/selftests/drm_cmdline_selftests.h | 1 + >> .../drm/selftests/test-drm_cmdline_parser.c | 22 +++++++++++++ >> include/drm/drm_connector.h | 8 +++++ >> 5 files changed, 66 insertions(+) >> >> diff --git a/Documentation/fb/modedb.rst b/Documentation/fb/modedb.rst >> index 9c4e3fd39e6d..624d08fd2856 100644 >> --- a/Documentation/fb/modedb.rst >> +++ b/Documentation/fb/modedb.rst >> @@ -65,6 +65,9 @@ Valid options are:: >> - reflect_y (boolean): Perform an axial symmetry on the Y axis >> - rotate (integer): Rotate the initial framebuffer by x >> degrees. Valid values are 0, 90, 180 and 270. >> + - panel_orientation, one of "normal", "upside_down", "left_side_up", or >> + "right_side_up". For KMS drivers only, this sets the "panel orientation" >> + property on the kms connector as hint for kms users. > > Even though the semantic is a bit different, I think we should remain > consistent and have the same argument than for rotate (ie, steps in > clockwise rotation in steps of 90 degrees). Well the kernel kms defines for rotation also talk about 90 / 180 / 270": #define DRM_MODE_ROTATE_0 (1<<0) #define DRM_MODE_ROTATE_90 (1<<1) #define DRM_MODE_ROTATE_180 (1<<2) #define DRM_MODE_ROTATE_270 (1<<3) Where as the panel orientation uses strings like right_side_up, which means that the side of the panel which normally is the right side of the panel is now pointing up as the panel is mounted 90 degrees rotated with its original right side now pointing up. This IMHO is much clearer then 90 / 270 degrees which are ambiguous and perhaps more importantly this matches the kernel defines for panel-orientation and matches the String values enumerated by the enum type panel-orientation connector property: static const struct drm_prop_enum_list drm_panel_orientation_enum_list[] = { { DRM_MODE_PANEL_ORIENTATION_NORMAL, "Normal" }, { DRM_MODE_PANEL_ORIENTATION_BOTTOM_UP, "Upside Down" }, { DRM_MODE_PANEL_ORIENTATION_LEFT_UP, "Left Side Up" }, { DRM_MODE_PANEL_ORIENTATION_RIGHT_UP, "Right Side Up" }, }; So I would prefer to stick to the strings. Regards, Hans
On Mon, Nov 11, 2019 at 06:25:33PM +0100, Hans de Goede wrote: > Hi, > > On 11-11-2019 13:53, Maxime Ripard wrote: > > Hi Hans, > > > > Thanks for this series (and thanks for bouncing the mails too). > > > > All the previous patches are > > Acked-by: Maxime Ripard <mripard@kernel.org> > > Thank you for the review. > > > On Sun, Nov 10, 2019 at 04:40:58PM +0100, Hans de Goede wrote: > > > Sometimes we want to override a connector's panel_orientation from the > > > kernel commandline. Either for testing and for special cases, e.g. a kiosk > > > like setup which uses a TV mounted in portrait mode. > > > > > > Users can already specify a "rotate" option through a video= kernel cmdline > > > option. But that only supports 0/180 degrees (see drm_client_modeset TODO) > > > and only works for in kernel modeset clients, not for userspace kms users. > > > > > > The "panel-orientation" connector property OTOH does support 90/270 degrees > > > as it leaves dealing with the rotation up to userspace and this does work > > > for userspace kms clients (at least those which support this property). > > > > > > BugLink: https://gitlab.freedesktop.org/plymouth/plymouth/merge_requests/83 > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > > --- > > > Documentation/fb/modedb.rst | 3 ++ > > > drivers/gpu/drm/drm_modes.c | 32 +++++++++++++++++++ > > > .../gpu/drm/selftests/drm_cmdline_selftests.h | 1 + > > > .../drm/selftests/test-drm_cmdline_parser.c | 22 +++++++++++++ > > > include/drm/drm_connector.h | 8 +++++ > > > 5 files changed, 66 insertions(+) > > > > > > diff --git a/Documentation/fb/modedb.rst b/Documentation/fb/modedb.rst > > > index 9c4e3fd39e6d..624d08fd2856 100644 > > > --- a/Documentation/fb/modedb.rst > > > +++ b/Documentation/fb/modedb.rst > > > @@ -65,6 +65,9 @@ Valid options are:: > > > - reflect_y (boolean): Perform an axial symmetry on the Y axis > > > - rotate (integer): Rotate the initial framebuffer by x > > > degrees. Valid values are 0, 90, 180 and 270. > > > + - panel_orientation, one of "normal", "upside_down", "left_side_up", or > > > + "right_side_up". For KMS drivers only, this sets the "panel orientation" > > > + property on the kms connector as hint for kms users. > > > > Even though the semantic is a bit different, I think we should remain > > consistent and have the same argument than for rotate (ie, steps in > > clockwise rotation in steps of 90 degrees). > > Well the kernel kms defines for rotation also talk about 90 / 180 / 270": > > #define DRM_MODE_ROTATE_0 (1<<0) > #define DRM_MODE_ROTATE_90 (1<<1) > #define DRM_MODE_ROTATE_180 (1<<2) > #define DRM_MODE_ROTATE_270 (1<<3) > > Where as the panel orientation uses strings like right_side_up, which means > that the side of the panel which normally is the right side of the panel > is now pointing up as the panel is mounted 90 degrees rotated with its > original right side now pointing up. This IMHO is much clearer then > 90 / 270 degrees which are ambiguous and perhaps more importantly this > matches the kernel defines for panel-orientation and matches the > String values enumerated by the enum type panel-orientation connector > property: > > static const struct drm_prop_enum_list drm_panel_orientation_enum_list[] = { > { DRM_MODE_PANEL_ORIENTATION_NORMAL, "Normal" }, > { DRM_MODE_PANEL_ORIENTATION_BOTTOM_UP, "Upside Down" }, > { DRM_MODE_PANEL_ORIENTATION_LEFT_UP, "Left Side Up" }, > { DRM_MODE_PANEL_ORIENTATION_RIGHT_UP, "Right Side Up" }, > }; > > So I would prefer to stick to the strings. Ok, that works for me then Maxime
diff --git a/Documentation/fb/modedb.rst b/Documentation/fb/modedb.rst index 9c4e3fd39e6d..624d08fd2856 100644 --- a/Documentation/fb/modedb.rst +++ b/Documentation/fb/modedb.rst @@ -65,6 +65,9 @@ Valid options are:: - reflect_y (boolean): Perform an axial symmetry on the Y axis - rotate (integer): Rotate the initial framebuffer by x degrees. Valid values are 0, 90, 180 and 270. + - panel_orientation, one of "normal", "upside_down", "left_side_up", or + "right_side_up". For KMS drivers only, this sets the "panel orientation" + property on the kms connector as hint for kms users. ----------------------------------------------------------------------------- diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index 2e82603f5d0a..119fed7ab815 100644 --- a/drivers/gpu/drm/drm_modes.c +++ b/drivers/gpu/drm/drm_modes.c @@ -1591,6 +1591,33 @@ static int drm_mode_parse_cmdline_int(const char *delim, unsigned int *int_ret) return 0; } +static int drm_mode_parse_panel_orientation(const char *delim, + struct drm_cmdline_mode *mode) +{ + const char *value; + + if (*delim != '=') + return -EINVAL; + + value = delim + 1; + delim = strchr(value, ','); + if (!delim) + delim = value + strlen(value); + + if (!strncmp(value, "normal", delim - value)) + mode->panel_orientation = DRM_MODE_PANEL_ORIENTATION_NORMAL; + else if (!strncmp(value, "upside_down", delim - value)) + mode->panel_orientation = DRM_MODE_PANEL_ORIENTATION_BOTTOM_UP; + else if (!strncmp(value, "left_side_up", delim - value)) + mode->panel_orientation = DRM_MODE_PANEL_ORIENTATION_LEFT_UP; + else if (!strncmp(value, "right_side_up", delim - value)) + mode->panel_orientation = DRM_MODE_PANEL_ORIENTATION_RIGHT_UP; + else + return -EINVAL; + + return 0; +} + static int drm_mode_parse_cmdline_options(const char *str, bool freestanding, const struct drm_connector *connector, @@ -1657,6 +1684,9 @@ static int drm_mode_parse_cmdline_options(const char *str, return -EINVAL; mode->tv_margins.bottom = margin; + } else if (!strncmp(option, "panel_orientation", delim - option)) { + if (drm_mode_parse_panel_orientation(delim, mode)) + return -EINVAL; } else { return -EINVAL; } @@ -1715,6 +1745,8 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option, char *bpp_end_ptr = NULL, *refresh_end_ptr = NULL; int i, len, ret; + mode->panel_orientation = DRM_MODE_PANEL_ORIENTATION_UNKNOWN; + #ifdef CONFIG_FB if (!mode_option) mode_option = fb_mode_option; diff --git a/drivers/gpu/drm/selftests/drm_cmdline_selftests.h b/drivers/gpu/drm/selftests/drm_cmdline_selftests.h index aee92ac2cc21..ceac7af9a172 100644 --- a/drivers/gpu/drm/selftests/drm_cmdline_selftests.h +++ b/drivers/gpu/drm/selftests/drm_cmdline_selftests.h @@ -64,3 +64,4 @@ cmdline_test(drm_cmdline_test_bpp_extra_and_option) cmdline_test(drm_cmdline_test_extra_and_option) cmdline_test(drm_cmdline_test_freestanding_options) cmdline_test(drm_cmdline_test_freestanding_force_e_and_options) +cmdline_test(drm_cmdline_test_panel_orientation) diff --git a/drivers/gpu/drm/selftests/test-drm_cmdline_parser.c b/drivers/gpu/drm/selftests/test-drm_cmdline_parser.c index 8248d4aa5aaa..520f3e66a384 100644 --- a/drivers/gpu/drm/selftests/test-drm_cmdline_parser.c +++ b/drivers/gpu/drm/selftests/test-drm_cmdline_parser.c @@ -1092,6 +1092,28 @@ static int drm_cmdline_test_freestanding_force_e_and_options(void *ignored) return 0; } +static int drm_cmdline_test_panel_orientation(void *ignored) +{ + struct drm_cmdline_mode mode = { }; + + FAIL_ON(!drm_mode_parse_command_line_for_connector("panel_orientation=upside_down", + &no_connector, + &mode)); + FAIL_ON(mode.specified); + FAIL_ON(mode.refresh_specified); + FAIL_ON(mode.bpp_specified); + + FAIL_ON(mode.panel_orientation != DRM_MODE_PANEL_ORIENTATION_BOTTOM_UP); + + FAIL_ON(mode.rb); + FAIL_ON(mode.cvt); + FAIL_ON(mode.interlace); + FAIL_ON(mode.margins); + FAIL_ON(mode.force != DRM_FORCE_UNSPECIFIED); + + return 0; +} + #include "drm_selftest.c" static int __init test_drm_cmdline_init(void) diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index 681cb590f952..e60bb3602497 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -1065,6 +1065,14 @@ struct drm_cmdline_mode { */ unsigned int rotation_reflection; + /** + * @panel_orientation + * + * drm-connector "panel orientation" property override value, + * DRM_MODE_PANEL_ORIENTATION_UNKNOWN if not set. + */ + enum drm_panel_orientation panel_orientation; + /** * @tv_margins: TV margins to apply to the mode. */
Sometimes we want to override a connector's panel_orientation from the kernel commandline. Either for testing and for special cases, e.g. a kiosk like setup which uses a TV mounted in portrait mode. Users can already specify a "rotate" option through a video= kernel cmdline option. But that only supports 0/180 degrees (see drm_client_modeset TODO) and only works for in kernel modeset clients, not for userspace kms users. The "panel-orientation" connector property OTOH does support 90/270 degrees as it leaves dealing with the rotation up to userspace and this does work for userspace kms clients (at least those which support this property). BugLink: https://gitlab.freedesktop.org/plymouth/plymouth/merge_requests/83 Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- Documentation/fb/modedb.rst | 3 ++ drivers/gpu/drm/drm_modes.c | 32 +++++++++++++++++++ .../gpu/drm/selftests/drm_cmdline_selftests.h | 1 + .../drm/selftests/test-drm_cmdline_parser.c | 22 +++++++++++++ include/drm/drm_connector.h | 8 +++++ 5 files changed, 66 insertions(+)