| Message ID | ba320b3a13c4444102b77c4d00f7c1dc810adc3c.1555591281.git-series.maxime.ripard@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/vc4: Allow for more boot-time configuration | expand |
Den 18.04.2019 14.41, skrev Maxime Ripard: > Rotations and reflections setup are needed in some scenarios to initialise > properly the initial framebuffer. Some drivers already had a bunch of > quirks to deal with this, such as either a private kernel command line > parameter (omapdss) or on the device tree (various panels). > > In order to accomodate this, let's create a video mode parameter to deal > with the rotation and reflexion. > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> > --- > drivers/gpu/drm/drm_client_modeset.c | 10 +++- > drivers/gpu/drm/drm_modes.c | 110 ++++++++++++++++++++++------ > include/drm/drm_connector.h | 9 ++- > 3 files changed, 109 insertions(+), 20 deletions(-) > > diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c > index f2869c82510c..15145d2c86d5 100644 > --- a/drivers/gpu/drm/drm_client_modeset.c > +++ b/drivers/gpu/drm/drm_client_modeset.c > @@ -823,6 +823,7 @@ EXPORT_SYMBOL(drm_client_modeset_probe); > bool drm_client_panel_rotation(struct drm_mode_set *modeset, unsigned int *rotation) > { > struct drm_connector *connector = modeset->connectors[0]; > + struct drm_cmdline_mode *cmdline; > struct drm_plane *plane = modeset->crtc->primary; > u64 valid_mask = 0; > unsigned int i; > @@ -844,6 +845,15 @@ bool drm_client_panel_rotation(struct drm_mode_set *modeset, unsigned int *rotat > *rotation = DRM_MODE_ROTATE_0; > } > > + /** > + * We want the rotation on the command line to overwrite > + * whatever comes from the panel. > + */ > + cmdline = &connector->cmdline_mode; > + if (cmdline->specified && > + cmdline->rotation != DRM_MODE_ROTATE_0) I believe you need to drop that second check, otherwise rotate=0 will not overwrite panel rotation. > + *rotation = cmdline->rotation; > + > /* > * TODO: support 90 / 270 degree hardware rotation, > * depending on the hardware this may require the framebuffer > diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c > index 9613c1a28487..ac8d70b92b62 100644 > --- a/drivers/gpu/drm/drm_modes.c > +++ b/drivers/gpu/drm/drm_modes.c > @@ -1531,6 +1531,71 @@ static int drm_mode_parse_cmdline_res_mode(const char *str, unsigned int length, > return 0; > } > > +static int drm_mode_parse_cmdline_options(char *str, size_t len, > + struct drm_connector *connector, > + struct drm_cmdline_mode *mode) > +{ > + unsigned int rotation = 0; > + char *sep = str; > + > + while ((sep = strchr(sep, ','))) { > + char *delim, *option; > + > + option = sep + 1; > + delim = strchr(option, '='); > + if (!delim) { > + delim = strchr(option, ','); > + > + if (!delim) > + delim = str + len; > + } > + > + if (!strncmp(option, "rotate", delim - option)) { > + const char *value = delim + 1; > + unsigned int deg; > + > + deg = simple_strtol(value, &sep, 10); > + > + /* Make sure we have parsed something */ > + if (sep == value) > + return -EINVAL; > + > + switch (deg) { > + case 0: > + rotation |= DRM_MODE_ROTATE_0; > + break; > + > + case 90: > + rotation |= DRM_MODE_ROTATE_90; > + break; > + > + case 180: > + rotation |= DRM_MODE_ROTATE_180; > + break; > + > + case 270: > + rotation |= DRM_MODE_ROTATE_270; > + break; > + > + default: > + return -EINVAL; > + } > + } else if (!strncmp(option, "reflect_x", delim - option)) { > + rotation |= DRM_MODE_REFLECT_X; > + sep = delim; > + } else if (!strncmp(option, "reflect_y", delim - option)) { I think you should drop reflect_x and _y for now since they're not supported. People like me that only reads code and not documentation (ahem..) will get the impression that this should work. Documentation/fb/modedb.txt needs to be updated with this new video= option. Noralf. > + rotation |= DRM_MODE_REFLECT_Y; > + sep = delim; > + } else { > + return -EINVAL; > + } > + } > + > + mode->rotation = rotation; > + > + return 0; > +} > + > /** > * drm_mode_parse_command_line_for_connector - parse command line modeline for connector > * @mode_option: optional per connector mode option > @@ -1558,9 +1623,10 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option, > { > const char *name; > bool named_mode = false, parse_extras = false; > - unsigned int bpp_off = 0, refresh_off = 0; > + unsigned int bpp_off = 0, refresh_off = 0, options_off = 0; > unsigned int mode_end = 0; > char *bpp_ptr = NULL, *refresh_ptr = NULL, *extra_ptr = NULL; > + char *options_ptr = NULL; > char *bpp_end_ptr = NULL, *refresh_end_ptr = NULL; > int ret; > > @@ -1601,13 +1667,18 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option, > mode->refresh_specified = true; > } > > + /* Locate the start of named options */ > + options_ptr = strchr(name, ','); > + if (options_ptr) > + options_off = options_ptr - name; > + > /* Locate the end of the name / resolution, and parse it */ > - if (bpp_ptr && refresh_ptr) { > - mode_end = min(bpp_off, refresh_off); > - } else if (bpp_ptr) { > + if (bpp_ptr) { > mode_end = bpp_off; > } else if (refresh_ptr) { > mode_end = refresh_off; > + } else if (options_ptr) { > + mode_end = options_off; > } else { > mode_end = strlen(name); > parse_extras = true; > @@ -1649,24 +1720,23 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option, > else if (refresh_ptr) > extra_ptr = refresh_end_ptr; > > - if (extra_ptr) { > - if (!named_mode) { > - int len = strlen(name) - (extra_ptr - name); > + if (extra_ptr && > + extra_ptr != options_ptr) { > + int len = strlen(name) - (extra_ptr - name); > > - ret = drm_mode_parse_cmdline_extra(extra_ptr, len, > - connector, mode); > - if (ret) > - return false; > - } else { > - int remaining = strlen(name) - (extra_ptr - name); > + ret = drm_mode_parse_cmdline_extra(extra_ptr, len, > + connector, mode); > + if (ret) > + return false; > + } > > - /* > - * We still have characters to process, while > - * we shouldn't have any > - */ > - if (remaining > 0) > - return false; > - } > + if (options_ptr) { > + int len = strlen(name) - (options_ptr - name); > + > + ret = drm_mode_parse_cmdline_options(options_ptr, len, > + connector, mode); > + if (ret) > + return false; > } > > return true; > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h > index 06aa3b9cb920..6f57c1a3afff 100644 > --- a/include/drm/drm_connector.h > +++ b/include/drm/drm_connector.h > @@ -908,6 +908,15 @@ struct drm_cmdline_mode { > bool cvt; > bool margins; > enum drm_connector_force force; > + > + /** > + * @rotation: > + * > + * Initial rotation of the mode setup from the command line. > + * See DRM_MODE_ROTATE_*. Only DRM_MODE_ROTATE_0 and > + * DRM_MODE_ROTATE_180 are supported at the moment. > + */ > + unsigned int rotation; > }; > > /** >
Den 18.04.2019 18.40, skrev Noralf Trønnes: > > > Den 18.04.2019 14.41, skrev Maxime Ripard: >> Rotations and reflections setup are needed in some scenarios to initialise >> properly the initial framebuffer. Some drivers already had a bunch of >> quirks to deal with this, such as either a private kernel command line >> parameter (omapdss) or on the device tree (various panels). >> >> In order to accomodate this, let's create a video mode parameter to deal >> with the rotation and reflexion. >> >> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> >> --- >> drivers/gpu/drm/drm_client_modeset.c | 10 +++- >> drivers/gpu/drm/drm_modes.c | 110 ++++++++++++++++++++++------ >> include/drm/drm_connector.h | 9 ++- >> 3 files changed, 109 insertions(+), 20 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c >> index f2869c82510c..15145d2c86d5 100644 >> --- a/drivers/gpu/drm/drm_client_modeset.c >> +++ b/drivers/gpu/drm/drm_client_modeset.c >> @@ -823,6 +823,7 @@ EXPORT_SYMBOL(drm_client_modeset_probe); >> bool drm_client_panel_rotation(struct drm_mode_set *modeset, unsigned int *rotation) >> { >> struct drm_connector *connector = modeset->connectors[0]; >> + struct drm_cmdline_mode *cmdline; >> struct drm_plane *plane = modeset->crtc->primary; >> u64 valid_mask = 0; >> unsigned int i; >> @@ -844,6 +845,15 @@ bool drm_client_panel_rotation(struct drm_mode_set *modeset, unsigned int *rotat >> *rotation = DRM_MODE_ROTATE_0; >> } >> >> + /** >> + * We want the rotation on the command line to overwrite >> + * whatever comes from the panel. >> + */ >> + cmdline = &connector->cmdline_mode; >> + if (cmdline->specified && >> + cmdline->rotation != DRM_MODE_ROTATE_0) > > I believe you need to drop that second check, otherwise rotate=0 will > not overwrite panel rotation. > >> + *rotation = cmdline->rotation; I remembered that you wanted this to propagate to DRM userspace. That's not happening here. The only way I see for that to happen, is to set ->panel_orientation. And to repeat myself, imo that makes 'orientation' a better name for this video= option. Noralf. >> + >> /* >> * TODO: support 90 / 270 degree hardware rotation, >> * depending on the hardware this may require the framebuffer >> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c >> index 9613c1a28487..ac8d70b92b62 100644 >> --- a/drivers/gpu/drm/drm_modes.c >> +++ b/drivers/gpu/drm/drm_modes.c >> @@ -1531,6 +1531,71 @@ static int drm_mode_parse_cmdline_res_mode(const char *str, unsigned int length, >> return 0; >> } >> >> +static int drm_mode_parse_cmdline_options(char *str, size_t len, >> + struct drm_connector *connector, >> + struct drm_cmdline_mode *mode) >> +{ >> + unsigned int rotation = 0; >> + char *sep = str; >> + >> + while ((sep = strchr(sep, ','))) { >> + char *delim, *option; >> + >> + option = sep + 1; >> + delim = strchr(option, '='); >> + if (!delim) { >> + delim = strchr(option, ','); >> + >> + if (!delim) >> + delim = str + len; >> + } >> + >> + if (!strncmp(option, "rotate", delim - option)) { >> + const char *value = delim + 1; >> + unsigned int deg; >> + >> + deg = simple_strtol(value, &sep, 10); >> + >> + /* Make sure we have parsed something */ >> + if (sep == value) >> + return -EINVAL; >> + >> + switch (deg) { >> + case 0: >> + rotation |= DRM_MODE_ROTATE_0; >> + break; >> + >> + case 90: >> + rotation |= DRM_MODE_ROTATE_90; >> + break; >> + >> + case 180: >> + rotation |= DRM_MODE_ROTATE_180; >> + break; >> + >> + case 270: >> + rotation |= DRM_MODE_ROTATE_270; >> + break; >> + >> + default: >> + return -EINVAL; >> + } >> + } else if (!strncmp(option, "reflect_x", delim - option)) { >> + rotation |= DRM_MODE_REFLECT_X; >> + sep = delim; >> + } else if (!strncmp(option, "reflect_y", delim - option)) { > > I think you should drop reflect_x and _y for now since they're not > supported. People like me that only reads code and not documentation > (ahem..) will get the impression that this should work. > > Documentation/fb/modedb.txt needs to be updated with this new video= option. > > Noralf. > >> + rotation |= DRM_MODE_REFLECT_Y; >> + sep = delim; >> + } else { >> + return -EINVAL; >> + } >> + } >> + >> + mode->rotation = rotation; >> + >> + return 0; >> +} >> + >> /** >> * drm_mode_parse_command_line_for_connector - parse command line modeline for connector >> * @mode_option: optional per connector mode option >> @@ -1558,9 +1623,10 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option, >> { >> const char *name; >> bool named_mode = false, parse_extras = false; >> - unsigned int bpp_off = 0, refresh_off = 0; >> + unsigned int bpp_off = 0, refresh_off = 0, options_off = 0; >> unsigned int mode_end = 0; >> char *bpp_ptr = NULL, *refresh_ptr = NULL, *extra_ptr = NULL; >> + char *options_ptr = NULL; >> char *bpp_end_ptr = NULL, *refresh_end_ptr = NULL; >> int ret; >> >> @@ -1601,13 +1667,18 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option, >> mode->refresh_specified = true; >> } >> >> + /* Locate the start of named options */ >> + options_ptr = strchr(name, ','); >> + if (options_ptr) >> + options_off = options_ptr - name; >> + >> /* Locate the end of the name / resolution, and parse it */ >> - if (bpp_ptr && refresh_ptr) { >> - mode_end = min(bpp_off, refresh_off); >> - } else if (bpp_ptr) { >> + if (bpp_ptr) { >> mode_end = bpp_off; >> } else if (refresh_ptr) { >> mode_end = refresh_off; >> + } else if (options_ptr) { >> + mode_end = options_off; >> } else { >> mode_end = strlen(name); >> parse_extras = true; >> @@ -1649,24 +1720,23 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option, >> else if (refresh_ptr) >> extra_ptr = refresh_end_ptr; >> >> - if (extra_ptr) { >> - if (!named_mode) { >> - int len = strlen(name) - (extra_ptr - name); >> + if (extra_ptr && >> + extra_ptr != options_ptr) { >> + int len = strlen(name) - (extra_ptr - name); >> >> - ret = drm_mode_parse_cmdline_extra(extra_ptr, len, >> - connector, mode); >> - if (ret) >> - return false; >> - } else { >> - int remaining = strlen(name) - (extra_ptr - name); >> + ret = drm_mode_parse_cmdline_extra(extra_ptr, len, >> + connector, mode); >> + if (ret) >> + return false; >> + } >> >> - /* >> - * We still have characters to process, while >> - * we shouldn't have any >> - */ >> - if (remaining > 0) >> - return false; >> - } >> + if (options_ptr) { >> + int len = strlen(name) - (options_ptr - name); >> + >> + ret = drm_mode_parse_cmdline_options(options_ptr, len, >> + connector, mode); >> + if (ret) >> + return false; >> } >> >> return true; >> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h >> index 06aa3b9cb920..6f57c1a3afff 100644 >> --- a/include/drm/drm_connector.h >> +++ b/include/drm/drm_connector.h >> @@ -908,6 +908,15 @@ struct drm_cmdline_mode { >> bool cvt; >> bool margins; >> enum drm_connector_force force; >> + >> + /** >> + * @rotation: >> + * >> + * Initial rotation of the mode setup from the command line. >> + * See DRM_MODE_ROTATE_*. Only DRM_MODE_ROTATE_0 and >> + * DRM_MODE_ROTATE_180 are supported at the moment. >> + */ >> + unsigned int rotation; >> }; >> >> /** >> > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel >
Hi Noralf, On Thu, Apr 18, 2019 at 06:40:42PM +0200, Noralf Trønnes wrote: > Den 18.04.2019 14.41, skrev Maxime Ripard: > > + /** > > + * We want the rotation on the command line to overwrite > > + * whatever comes from the panel. > > + */ > > + cmdline = &connector->cmdline_mode; > > + if (cmdline->specified && > > + cmdline->rotation != DRM_MODE_ROTATE_0) > > I believe you need to drop that second check, otherwise rotate=0 will > not overwrite panel rotation. Good catch :) > > + } else if (!strncmp(option, "reflect_x", delim - option)) { > > + rotation |= DRM_MODE_REFLECT_X; > > + sep = delim; > > + } else if (!strncmp(option, "reflect_y", delim - option)) { > > I think you should drop reflect_x and _y for now since they're not > supported. People like me that only reads code and not documentation > (ahem..) will get the impression that this should work. I'm not sure what you mean here, this is definitely supposed to work. Is there a limitation you're thinking of? > Documentation/fb/modedb.txt needs to be updated with this new video= option. Will do, thanks! maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Hi Noralf, On Fri, Apr 19, 2019 at 10:53:28AM +0200, Noralf Trønnes wrote: > Den 18.04.2019 18.40, skrev Noralf Trønnes: > > > > > > Den 18.04.2019 14.41, skrev Maxime Ripard: > >> Rotations and reflections setup are needed in some scenarios to initialise > >> properly the initial framebuffer. Some drivers already had a bunch of > >> quirks to deal with this, such as either a private kernel command line > >> parameter (omapdss) or on the device tree (various panels). > >> > >> In order to accomodate this, let's create a video mode parameter to deal > >> with the rotation and reflexion. > >> > >> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> > >> --- > >> drivers/gpu/drm/drm_client_modeset.c | 10 +++- > >> drivers/gpu/drm/drm_modes.c | 110 ++++++++++++++++++++++------ > >> include/drm/drm_connector.h | 9 ++- > >> 3 files changed, 109 insertions(+), 20 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c > >> index f2869c82510c..15145d2c86d5 100644 > >> --- a/drivers/gpu/drm/drm_client_modeset.c > >> +++ b/drivers/gpu/drm/drm_client_modeset.c > >> @@ -823,6 +823,7 @@ EXPORT_SYMBOL(drm_client_modeset_probe); > >> bool drm_client_panel_rotation(struct drm_mode_set *modeset, unsigned int *rotation) > >> { > >> struct drm_connector *connector = modeset->connectors[0]; > >> + struct drm_cmdline_mode *cmdline; > >> struct drm_plane *plane = modeset->crtc->primary; > >> u64 valid_mask = 0; > >> unsigned int i; > >> @@ -844,6 +845,15 @@ bool drm_client_panel_rotation(struct drm_mode_set *modeset, unsigned int *rotat > >> *rotation = DRM_MODE_ROTATE_0; > >> } > >> > >> + /** > >> + * We want the rotation on the command line to overwrite > >> + * whatever comes from the panel. > >> + */ > >> + cmdline = &connector->cmdline_mode; > >> + if (cmdline->specified && > >> + cmdline->rotation != DRM_MODE_ROTATE_0) > > > > I believe you need to drop that second check, otherwise rotate=0 will > > not overwrite panel rotation. > > > >> + *rotation = cmdline->rotation; > > I remembered that you wanted this to propagate to DRM userspace. That's > not happening here. It's propated to the userspace through the plane's rotation property, I just checked. > The only way I see for that to happen, is to set > ->panel_orientation. And to repeat myself, imo that makes > 'orientation' a better name for this video= option. orientation and rotation are two different things to me. The orientation of a panel for example is absolute, while the rotation is a transformation. In this particular case, I think that both the orientation and the rotation should be taken into account, with the orientation being the default state, and the hardware / panel will tell us that, while the rotation would be a transformation from that default to whatever the user wants. More importantly, the orientation is a property of the hardware (ie, how the display has been assembled), while the rotation is a software construct. And if the property being used to expose that is the rotation, I guess it would make sense to just use the same name and remain consistent. Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On Tue, 11 Jun 2019, Maxime Ripard <maxime.ripard@bootlin.com> wrote: > Hi Noralf, > > On Fri, Apr 19, 2019 at 10:53:28AM +0200, Noralf Trønnes wrote: >> Den 18.04.2019 18.40, skrev Noralf Trønnes: >> > >> > >> > Den 18.04.2019 14.41, skrev Maxime Ripard: >> >> Rotations and reflections setup are needed in some scenarios to initialise >> >> properly the initial framebuffer. Some drivers already had a bunch of >> >> quirks to deal with this, such as either a private kernel command line >> >> parameter (omapdss) or on the device tree (various panels). >> >> >> >> In order to accomodate this, let's create a video mode parameter to deal >> >> with the rotation and reflexion. >> >> >> >> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> >> >> --- >> >> drivers/gpu/drm/drm_client_modeset.c | 10 +++- >> >> drivers/gpu/drm/drm_modes.c | 110 ++++++++++++++++++++++------ >> >> include/drm/drm_connector.h | 9 ++- >> >> 3 files changed, 109 insertions(+), 20 deletions(-) >> >> >> >> diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c >> >> index f2869c82510c..15145d2c86d5 100644 >> >> --- a/drivers/gpu/drm/drm_client_modeset.c >> >> +++ b/drivers/gpu/drm/drm_client_modeset.c >> >> @@ -823,6 +823,7 @@ EXPORT_SYMBOL(drm_client_modeset_probe); >> >> bool drm_client_panel_rotation(struct drm_mode_set *modeset, unsigned int *rotation) >> >> { >> >> struct drm_connector *connector = modeset->connectors[0]; >> >> + struct drm_cmdline_mode *cmdline; >> >> struct drm_plane *plane = modeset->crtc->primary; >> >> u64 valid_mask = 0; >> >> unsigned int i; >> >> @@ -844,6 +845,15 @@ bool drm_client_panel_rotation(struct drm_mode_set *modeset, unsigned int *rotat >> >> *rotation = DRM_MODE_ROTATE_0; >> >> } >> >> >> >> + /** >> >> + * We want the rotation on the command line to overwrite >> >> + * whatever comes from the panel. >> >> + */ >> >> + cmdline = &connector->cmdline_mode; >> >> + if (cmdline->specified && >> >> + cmdline->rotation != DRM_MODE_ROTATE_0) >> > >> > I believe you need to drop that second check, otherwise rotate=0 will >> > not overwrite panel rotation. >> > >> >> + *rotation = cmdline->rotation; >> >> I remembered that you wanted this to propagate to DRM userspace. That's >> not happening here. > > It's propated to the userspace through the plane's rotation property, > I just checked. > >> The only way I see for that to happen, is to set >> ->panel_orientation. And to repeat myself, imo that makes >> 'orientation' a better name for this video= option. > > orientation and rotation are two different things to me. The > orientation of a panel for example is absolute, while the rotation is > a transformation. In this particular case, I think that both the > orientation and the rotation should be taken into account, with the > orientation being the default state, and the hardware / panel will > tell us that, while the rotation would be a transformation from that > default to whatever the user wants. > > More importantly, the orientation is a property of the hardware (ie, > how the display has been assembled), while the rotation is a software > construct. FWIW, agreed. The immutable orientation property is exposed using the drm_connector_init_panel_orientation_property() call. You then rotate to take the orientation into account, to not display stuff sideways or upside down wrt the natural orientation of the device. BR, Jani. > > And if the property being used to expose that is the rotation, I guess > it would make sense to just use the same name and remain consistent. > > Maxime > > -- > Maxime Ripard, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Den 11.06.2019 14.49, skrev Maxime Ripard: > Hi Noralf, > > On Thu, Apr 18, 2019 at 06:40:42PM +0200, Noralf Trønnes wrote: >> Den 18.04.2019 14.41, skrev Maxime Ripard: >>> + /** >>> + * We want the rotation on the command line to overwrite >>> + * whatever comes from the panel. >>> + */ >>> + cmdline = &connector->cmdline_mode; >>> + if (cmdline->specified && >>> + cmdline->rotation != DRM_MODE_ROTATE_0) >> >> I believe you need to drop that second check, otherwise rotate=0 will >> not overwrite panel rotation. > > Good catch :) > >>> + } else if (!strncmp(option, "reflect_x", delim - option)) { >>> + rotation |= DRM_MODE_REFLECT_X; >>> + sep = delim; >>> + } else if (!strncmp(option, "reflect_y", delim - option)) { >> >> I think you should drop reflect_x and _y for now since they're not >> supported. People like me that only reads code and not documentation >> (ahem..) will get the impression that this should work. > > I'm not sure what you mean here, this is definitely supposed to > work. Is there a limitation you're thinking of? > It's this one in drm_client_panel_rotation() which limits rotation to DRM_MODE_ROTATE_180: /* * TODO: support 90 / 270 degree hardware rotation, * depending on the hardware this may require the framebuffer * to be in a specific tiling format. */ if (*rotation != DRM_MODE_ROTATE_180 || !plane->rotation_property) return false; Noralf. >> Documentation/fb/modedb.txt needs to be updated with this new video= option. > > Will do, thanks! > > maxime > > -- > Maxime Ripard, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com >
Den 11.06.2019 15.20, skrev Maxime Ripard: > Hi Noralf, > > On Fri, Apr 19, 2019 at 10:53:28AM +0200, Noralf Trønnes wrote: >> Den 18.04.2019 18.40, skrev Noralf Trønnes: >>> >>> >>> Den 18.04.2019 14.41, skrev Maxime Ripard: >>>> Rotations and reflections setup are needed in some scenarios to initialise >>>> properly the initial framebuffer. Some drivers already had a bunch of >>>> quirks to deal with this, such as either a private kernel command line >>>> parameter (omapdss) or on the device tree (various panels). >>>> >>>> In order to accomodate this, let's create a video mode parameter to deal >>>> with the rotation and reflexion. >>>> >>>> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> >>>> --- >>>> drivers/gpu/drm/drm_client_modeset.c | 10 +++- >>>> drivers/gpu/drm/drm_modes.c | 110 ++++++++++++++++++++++------ >>>> include/drm/drm_connector.h | 9 ++- >>>> 3 files changed, 109 insertions(+), 20 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c >>>> index f2869c82510c..15145d2c86d5 100644 >>>> --- a/drivers/gpu/drm/drm_client_modeset.c >>>> +++ b/drivers/gpu/drm/drm_client_modeset.c >>>> @@ -823,6 +823,7 @@ EXPORT_SYMBOL(drm_client_modeset_probe); >>>> bool drm_client_panel_rotation(struct drm_mode_set *modeset, unsigned int *rotation) >>>> { >>>> struct drm_connector *connector = modeset->connectors[0]; >>>> + struct drm_cmdline_mode *cmdline; >>>> struct drm_plane *plane = modeset->crtc->primary; >>>> u64 valid_mask = 0; >>>> unsigned int i; >>>> @@ -844,6 +845,15 @@ bool drm_client_panel_rotation(struct drm_mode_set *modeset, unsigned int *rotat >>>> *rotation = DRM_MODE_ROTATE_0; >>>> } >>>> >>>> + /** >>>> + * We want the rotation on the command line to overwrite >>>> + * whatever comes from the panel. >>>> + */ >>>> + cmdline = &connector->cmdline_mode; >>>> + if (cmdline->specified && >>>> + cmdline->rotation != DRM_MODE_ROTATE_0) >>> >>> I believe you need to drop that second check, otherwise rotate=0 will >>> not overwrite panel rotation. >>> >>>> + *rotation = cmdline->rotation; >> >> I remembered that you wanted this to propagate to DRM userspace. That's >> not happening here. > > It's propated to the userspace through the plane's rotation property, > I just checked. > Oh, so the rotation property stores its value in plane_state->rotation. And this is set in: drm_client_modeset_commit_atomic() -> drm_client_panel_rotation(): plane_state->rotation = rotation; >> The only way I see for that to happen, is to set >> ->panel_orientation. And to repeat myself, imo that makes >> 'orientation' a better name for this video= option. > > orientation and rotation are two different things to me. The > orientation of a panel for example is absolute, while the rotation is > a transformation. In this particular case, I think that both the > orientation and the rotation should be taken into account, with the > orientation being the default state, and the hardware / panel will > tell us that, while the rotation would be a transformation from that > default to whatever the user wants. > > More importantly, the orientation is a property of the hardware (ie, > how the display has been assembled), while the rotation is a software > construct. > > And if the property being used to expose that is the rotation, I guess > it would make sense to just use the same name and remain consistent. > Ok, I see. Based on this, I would assume that rotation would be relative to the orientation, but I see that in this patch rotation doesn't take orintation into account, it just overwrites it. I don't how userspace deals with rotation on top of orientation. Noralf. > Maxime > > -- > Maxime Ripard, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com >
Hi, On Wed, Jun 12, 2019 at 02:43:44PM +0200, Noralf Trønnes wrote: > Den 11.06.2019 14.49, skrev Maxime Ripard: > >>> + } else if (!strncmp(option, "reflect_x", delim - option)) { > >>> + rotation |= DRM_MODE_REFLECT_X; > >>> + sep = delim; > >>> + } else if (!strncmp(option, "reflect_y", delim - option)) { > >> > >> I think you should drop reflect_x and _y for now since they're not > >> supported. People like me that only reads code and not documentation > >> (ahem..) will get the impression that this should work. > > > > I'm not sure what you mean here, this is definitely supposed to > > work. Is there a limitation you're thinking of? > > > > It's this one in drm_client_panel_rotation() which limits rotation to > DRM_MODE_ROTATE_180: > > /* > * TODO: support 90 / 270 degree hardware rotation, > * depending on the hardware this may require the framebuffer > * to be in a specific tiling format. > */ > if (*rotation != DRM_MODE_ROTATE_180 || !plane->rotation_property) > return false; Indeed, it doesn't work anymore since I rebased on your work. I'll fix this, thanks! Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Hi, On Wed, Jun 12, 2019 at 03:21:19PM +0200, Noralf Trønnes wrote: > >> The only way I see for that to happen, is to set > >> ->panel_orientation. And to repeat myself, imo that makes > >> 'orientation' a better name for this video= option. > > > > orientation and rotation are two different things to me. The > > orientation of a panel for example is absolute, while the rotation is > > a transformation. In this particular case, I think that both the > > orientation and the rotation should be taken into account, with the > > orientation being the default state, and the hardware / panel will > > tell us that, while the rotation would be a transformation from that > > default to whatever the user wants. > > > > More importantly, the orientation is a property of the hardware (ie, > > how the display has been assembled), while the rotation is a software > > construct. > > > > And if the property being used to expose that is the rotation, I guess > > it would make sense to just use the same name and remain consistent. > > Ok, I see. Based on this, I would assume that rotation would be relative > to the orientation, but I see that in this patch rotation doesn't take > orintation into account, it just overwrites it. Yeah, that's a good point. I've updated the next version to add the rotation on the command line to the orientation. > I don't how userspace deals with rotation on top of orientation. The orientation is exposed through the property, and the result is available through the plane's rotation, so I guess that it's enough? Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Den 13.06.2019 14.50, skrev Maxime Ripard: > Hi, > > On Wed, Jun 12, 2019 at 03:21:19PM +0200, Noralf Trønnes wrote: >>>> The only way I see for that to happen, is to set >>>> ->panel_orientation. And to repeat myself, imo that makes >>>> 'orientation' a better name for this video= option. >>> >>> orientation and rotation are two different things to me. The >>> orientation of a panel for example is absolute, while the rotation is >>> a transformation. In this particular case, I think that both the >>> orientation and the rotation should be taken into account, with the >>> orientation being the default state, and the hardware / panel will >>> tell us that, while the rotation would be a transformation from that >>> default to whatever the user wants. >>> >>> More importantly, the orientation is a property of the hardware (ie, >>> how the display has been assembled), while the rotation is a software >>> construct. >>> >>> And if the property being used to expose that is the rotation, I guess >>> it would make sense to just use the same name and remain consistent. >> >> Ok, I see. Based on this, I would assume that rotation would be relative >> to the orientation, but I see that in this patch rotation doesn't take >> orintation into account, it just overwrites it. > > Yeah, that's a good point. I've updated the next version to add the > rotation on the command line to the orientation. > >> I don't how userspace deals with rotation on top of orientation. > > The orientation is exposed through the property, and the result is > available through the plane's rotation, so I guess that it's enough? > I was just wondering if Xserver/wayland applied rotation on top of orientation or not. Maybe it would make sense to treat internal drm clients the same. I understand that the DRM rotation property doesn't apply on top orientation, just wondering how mutter/wayland (whoever is in charge) does this. Not important to me, so feel free to disregard, I'm just wondering. Noralf. > Maxime > > -- > Maxime Ripard, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com >
diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c index f2869c82510c..15145d2c86d5 100644 --- a/drivers/gpu/drm/drm_client_modeset.c +++ b/drivers/gpu/drm/drm_client_modeset.c @@ -823,6 +823,7 @@ EXPORT_SYMBOL(drm_client_modeset_probe); bool drm_client_panel_rotation(struct drm_mode_set *modeset, unsigned int *rotation) { struct drm_connector *connector = modeset->connectors[0]; + struct drm_cmdline_mode *cmdline; struct drm_plane *plane = modeset->crtc->primary; u64 valid_mask = 0; unsigned int i; @@ -844,6 +845,15 @@ bool drm_client_panel_rotation(struct drm_mode_set *modeset, unsigned int *rotat *rotation = DRM_MODE_ROTATE_0; } + /** + * We want the rotation on the command line to overwrite + * whatever comes from the panel. + */ + cmdline = &connector->cmdline_mode; + if (cmdline->specified && + cmdline->rotation != DRM_MODE_ROTATE_0) + *rotation = cmdline->rotation; + /* * TODO: support 90 / 270 degree hardware rotation, * depending on the hardware this may require the framebuffer diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index 9613c1a28487..ac8d70b92b62 100644 --- a/drivers/gpu/drm/drm_modes.c +++ b/drivers/gpu/drm/drm_modes.c @@ -1531,6 +1531,71 @@ static int drm_mode_parse_cmdline_res_mode(const char *str, unsigned int length, return 0; } +static int drm_mode_parse_cmdline_options(char *str, size_t len, + struct drm_connector *connector, + struct drm_cmdline_mode *mode) +{ + unsigned int rotation = 0; + char *sep = str; + + while ((sep = strchr(sep, ','))) { + char *delim, *option; + + option = sep + 1; + delim = strchr(option, '='); + if (!delim) { + delim = strchr(option, ','); + + if (!delim) + delim = str + len; + } + + if (!strncmp(option, "rotate", delim - option)) { + const char *value = delim + 1; + unsigned int deg; + + deg = simple_strtol(value, &sep, 10); + + /* Make sure we have parsed something */ + if (sep == value) + return -EINVAL; + + switch (deg) { + case 0: + rotation |= DRM_MODE_ROTATE_0; + break; + + case 90: + rotation |= DRM_MODE_ROTATE_90; + break; + + case 180: + rotation |= DRM_MODE_ROTATE_180; + break; + + case 270: + rotation |= DRM_MODE_ROTATE_270; + break; + + default: + return -EINVAL; + } + } else if (!strncmp(option, "reflect_x", delim - option)) { + rotation |= DRM_MODE_REFLECT_X; + sep = delim; + } else if (!strncmp(option, "reflect_y", delim - option)) { + rotation |= DRM_MODE_REFLECT_Y; + sep = delim; + } else { + return -EINVAL; + } + } + + mode->rotation = rotation; + + return 0; +} + /** * drm_mode_parse_command_line_for_connector - parse command line modeline for connector * @mode_option: optional per connector mode option @@ -1558,9 +1623,10 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option, { const char *name; bool named_mode = false, parse_extras = false; - unsigned int bpp_off = 0, refresh_off = 0; + unsigned int bpp_off = 0, refresh_off = 0, options_off = 0; unsigned int mode_end = 0; char *bpp_ptr = NULL, *refresh_ptr = NULL, *extra_ptr = NULL; + char *options_ptr = NULL; char *bpp_end_ptr = NULL, *refresh_end_ptr = NULL; int ret; @@ -1601,13 +1667,18 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option, mode->refresh_specified = true; } + /* Locate the start of named options */ + options_ptr = strchr(name, ','); + if (options_ptr) + options_off = options_ptr - name; + /* Locate the end of the name / resolution, and parse it */ - if (bpp_ptr && refresh_ptr) { - mode_end = min(bpp_off, refresh_off); - } else if (bpp_ptr) { + if (bpp_ptr) { mode_end = bpp_off; } else if (refresh_ptr) { mode_end = refresh_off; + } else if (options_ptr) { + mode_end = options_off; } else { mode_end = strlen(name); parse_extras = true; @@ -1649,24 +1720,23 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option, else if (refresh_ptr) extra_ptr = refresh_end_ptr; - if (extra_ptr) { - if (!named_mode) { - int len = strlen(name) - (extra_ptr - name); + if (extra_ptr && + extra_ptr != options_ptr) { + int len = strlen(name) - (extra_ptr - name); - ret = drm_mode_parse_cmdline_extra(extra_ptr, len, - connector, mode); - if (ret) - return false; - } else { - int remaining = strlen(name) - (extra_ptr - name); + ret = drm_mode_parse_cmdline_extra(extra_ptr, len, + connector, mode); + if (ret) + return false; + } - /* - * We still have characters to process, while - * we shouldn't have any - */ - if (remaining > 0) - return false; - } + if (options_ptr) { + int len = strlen(name) - (options_ptr - name); + + ret = drm_mode_parse_cmdline_options(options_ptr, len, + connector, mode); + if (ret) + return false; } return true; diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index 06aa3b9cb920..6f57c1a3afff 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -908,6 +908,15 @@ struct drm_cmdline_mode { bool cvt; bool margins; enum drm_connector_force force; + + /** + * @rotation: + * + * Initial rotation of the mode setup from the command line. + * See DRM_MODE_ROTATE_*. Only DRM_MODE_ROTATE_0 and + * DRM_MODE_ROTATE_180 are supported at the moment. + */ + unsigned int rotation; }; /**
Rotations and reflections setup are needed in some scenarios to initialise properly the initial framebuffer. Some drivers already had a bunch of quirks to deal with this, such as either a private kernel command line parameter (omapdss) or on the device tree (various panels). In order to accomodate this, let's create a video mode parameter to deal with the rotation and reflexion. Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> --- drivers/gpu/drm/drm_client_modeset.c | 10 +++- drivers/gpu/drm/drm_modes.c | 110 ++++++++++++++++++++++------ include/drm/drm_connector.h | 9 ++- 3 files changed, 109 insertions(+), 20 deletions(-)