Maxime Ripard
2022-Nov-14 13:00 UTC
[Nouveau] [PATCH v9 09/25] drm/modes: Move named modes parsing to a separate function
The current construction of the named mode parsing doesn't allow to extend
it easily. Let's move it to a separate function so we can add more
parameters and modes.
In order for the tests to still pass, some extra checks are needed, so
it's not a 1:1 move.
Reviewed-by: Noralf Tr?nnes <noralf at tronnes.org>
Tested-by: Mateusz Kwiatkowski <kfyatek+publicgit at gmail.com>
Signed-off-by: Maxime Ripard <maxime at cerno.tech>
---
Changes in v7:
- Add Noralf Reviewed-by
Changes in v6:
- Simplify the test for connection status extras
- Simplify the code path to call drm_mode_parse_cmdline_named_mode
Changes in v4:
- Fold down all the named mode patches that were split into a single
patch again to maintain bisectability
---
drivers/gpu/drm/drm_modes.c | 70 +++++++++++++++++++++++++++++++++++++--------
1 file changed, 58 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 71c050c3ee6b..37542612912b 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -2229,6 +2229,51 @@ static const char * const drm_named_modes_whitelist[] = {
"PAL",
};
+static int drm_mode_parse_cmdline_named_mode(const char *name,
+ unsigned int name_end,
+ struct drm_cmdline_mode *cmdline_mode)
+{
+ unsigned int i;
+
+ if (!name_end)
+ return 0;
+
+ /* If the name starts with a digit, it's not a named mode */
+ if (isdigit(name[0]))
+ return 0;
+
+ /*
+ * If there's an equal sign in the name, the command-line
+ * contains only an option and no mode.
+ */
+ if (strnchr(name, name_end, '='))
+ return 0;
+
+ /* The connection status extras can be set without a mode. */
+ if (name_end == 1 &&
+ (name[0] == 'd' || name[0] == 'D' || name[0] ==
'e'))
+ return 0;
+
+ /*
+ * We're sure we're a named mode at this point, iterate over the
+ * list of modes we're aware of.
+ */
+ for (i = 0; i < ARRAY_SIZE(drm_named_modes_whitelist); i++) {
+ int ret;
+
+ ret = str_has_prefix(name, drm_named_modes_whitelist[i]);
+ if (ret != name_end)
+ continue;
+
+ strcpy(cmdline_mode->name, drm_named_modes_whitelist[i]);
+ cmdline_mode->specified = true;
+
+ return 1;
+ }
+
+ return -EINVAL;
+}
+
/**
* drm_mode_parse_command_line_for_connector - parse command line modeline for
connector
* @mode_option: optional per connector mode option
@@ -2265,7 +2310,7 @@ bool drm_mode_parse_command_line_for_connector(const char
*mode_option,
const char *bpp_ptr = NULL, *refresh_ptr = NULL, *extra_ptr = NULL;
const char *options_ptr = NULL;
char *bpp_end_ptr = NULL, *refresh_end_ptr = NULL;
- int i, len, ret;
+ int len, ret;
memset(mode, 0, sizeof(*mode));
mode->panel_orientation = DRM_MODE_PANEL_ORIENTATION_UNKNOWN;
@@ -2306,18 +2351,19 @@ bool drm_mode_parse_command_line_for_connector(const
char *mode_option,
parse_extras = true;
}
- /* First check for a named mode */
- for (i = 0; i < ARRAY_SIZE(drm_named_modes_whitelist); i++) {
- ret = str_has_prefix(name, drm_named_modes_whitelist[i]);
- if (ret == mode_end) {
- if (refresh_ptr)
- return false; /* named + refresh is invalid */
+ if (!mode_end)
+ return false;
- strcpy(mode->name, drm_named_modes_whitelist[i]);
- mode->specified = true;
- break;
- }
- }
+ ret = drm_mode_parse_cmdline_named_mode(name, mode_end, mode);
+ if (ret < 0)
+ return false;
+
+ /*
+ * Having a mode that starts by a letter (and thus is named) and
+ * an at-sign (used to specify a refresh rate) is disallowed.
+ */
+ if (ret && refresh_ptr)
+ return false;
/* No named mode? Check for a normal mode argument, e.g. 1024x768 */
if (!mode->specified && isdigit(name[0])) {
--
b4 0.11.0-dev-99e3a
Maxime Ripard
2022-Nov-15 09:13 UTC
[Nouveau] (subset) [PATCH v9 09/25] drm/modes: Move named modes parsing to a separate function
On Mon, 14 Nov 2022 14:00:28 +0100, Maxime Ripard wrote:> The current construction of the named mode parsing doesn't allow to extend > it easily. Let's move it to a separate function so we can add more > parameters and modes. > > In order for the tests to still pass, some extra checks are needed, so > it's not a 1:1 move. > > [...]Applied to drm/drm-misc (drm-misc-next). Thanks! Maxime
Dave Stevenson
2024-Jan-02 15:12 UTC
[PATCH v9 09/25] drm/modes: Move named modes parsing to a separate function
Hi Maxime On Mon, 14 Nov 2022 at 13:00, Maxime Ripard <maxime at cerno.tech> wrote:> > The current construction of the named mode parsing doesn't allow to extend > it easily. Let's move it to a separate function so we can add more > parameters and modes. > > In order for the tests to still pass, some extra checks are needed, so > it's not a 1:1 move. > > Reviewed-by: Noralf Tr?nnes <noralf at tronnes.org> > Tested-by: Mateusz Kwiatkowski <kfyatek+publicgit at gmail.com> > Signed-off-by: Maxime Ripard <maxime at cerno.tech> > > --- > Changes in v7: > - Add Noralf Reviewed-by > > Changes in v6: > - Simplify the test for connection status extras > - Simplify the code path to call drm_mode_parse_cmdline_named_mode > > Changes in v4: > - Fold down all the named mode patches that were split into a single > patch again to maintain bisectability > --- > drivers/gpu/drm/drm_modes.c | 70 +++++++++++++++++++++++++++++++++++++-------- > 1 file changed, 58 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c > index 71c050c3ee6b..37542612912b 100644 > --- a/drivers/gpu/drm/drm_modes.c > +++ b/drivers/gpu/drm/drm_modes.c > @@ -2229,6 +2229,51 @@ static const char * const drm_named_modes_whitelist[] = { > "PAL", > }; > > +static int drm_mode_parse_cmdline_named_mode(const char *name, > + unsigned int name_end, > + struct drm_cmdline_mode *cmdline_mode) > +{ > + unsigned int i; > + > + if (!name_end) > + return 0; > + > + /* If the name starts with a digit, it's not a named mode */ > + if (isdigit(name[0])) > + return 0; > + > + /* > + * If there's an equal sign in the name, the command-line > + * contains only an option and no mode. > + */ > + if (strnchr(name, name_end, '=')) > + return 0; > + > + /* The connection status extras can be set without a mode. */ > + if (name_end == 1 && > + (name[0] == 'd' || name[0] == 'D' || name[0] == 'e')) > + return 0; > + > + /* > + * We're sure we're a named mode at this point, iterate over the > + * list of modes we're aware of. > + */ > + for (i = 0; i < ARRAY_SIZE(drm_named_modes_whitelist); i++) { > + int ret; > + > + ret = str_has_prefix(name, drm_named_modes_whitelist[i]); > + if (ret != name_end) > + continue; > + > + strcpy(cmdline_mode->name, drm_named_modes_whitelist[i]); > + cmdline_mode->specified = true; > + > + return 1; > + } > + > + return -EINVAL; > +} > + > /** > * drm_mode_parse_command_line_for_connector - parse command line modeline for connector > * @mode_option: optional per connector mode option > @@ -2265,7 +2310,7 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option, > const char *bpp_ptr = NULL, *refresh_ptr = NULL, *extra_ptr = NULL; > const char *options_ptr = NULL; > char *bpp_end_ptr = NULL, *refresh_end_ptr = NULL; > - int i, len, ret; > + int len, ret; > > memset(mode, 0, sizeof(*mode)); > mode->panel_orientation = DRM_MODE_PANEL_ORIENTATION_UNKNOWN; > @@ -2306,18 +2351,19 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option, > parse_extras = true; > } > > - /* First check for a named mode */ > - for (i = 0; i < ARRAY_SIZE(drm_named_modes_whitelist); i++) { > - ret = str_has_prefix(name, drm_named_modes_whitelist[i]); > - if (ret == mode_end) { > - if (refresh_ptr) > - return false; /* named + refresh is invalid */ > + if (!mode_end) > + return false;I'm chasing down a change in behaviour between 6.1 and 6.6, and this patch seems to be at least part of the cause. Since [1] we've had the emulated framebuffer on Pi being 16bpp to save memory. All good. It used to be possible to use "video=HDMI-A-1:-32" on the kernel command line to set it back to 32bpp. After this patch that is no longer possible. "mode_end = bpp_off", and "bpp_off = bpp_ptr - name", so with bpp_ptr = name we get mode_end being 0. That fails this conditional. drm_mode_parse_cmdline_named_mode already aborts early but with no error if name_end / mode_end is 0, so this "if" clause seems redundant, and is a change in behaviour. We do then get a second parsing failure due to the check if (bpp_ptr || refresh_ptr) at [2]. Prior to this patch my video= line would get mode->specified set via "if (ret == mode_end)" removed above, as ret = mode_end = 0. We therefore didn't evaluate the conditional that now fails. So I guess my question is whether my command line is valid or not, and therefore is this a regression? If considered invalid, then presumably there is no way to update the bpp without also having to specify the resolution. That is rather annoying as almost everything else can be updated without having to set the resolution, so this one property would be the odd one out. Thanks, and Happy New Year. Dave [1] https://github.com/torvalds/linux/commit/f741b28fb299263d2d03a0fb701bfe648927cd47 [2] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_modes.c#L2441 [3] https://github.com/torvalds/linux/commit/a631bf30eb914affc0a574f44576833477346ad6> > - strcpy(mode->name, drm_named_modes_whitelist[i]); > - mode->specified = true; > - break; > - } > - } > + ret = drm_mode_parse_cmdline_named_mode(name, mode_end, mode); > + if (ret < 0) > + return false; > + > + /* > + * Having a mode that starts by a letter (and thus is named) and > + * an at-sign (used to specify a refresh rate) is disallowed. > + */ > + if (ret && refresh_ptr) > + return false; > > /* No named mode? Check for a normal mode argument, e.g. 1024x768 */ > if (!mode->specified && isdigit(name[0])) { > > -- > b4 0.11.0-dev-99e3a