Nouveau - Oct 2022 - [PATCH v5 08/22] 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.

Signed-off-by: Maxime Ripard <maxime at cerno.tech>

---
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 | 73 ++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 62 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index c0dceff51cac..2f020ef2ddf2 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -2229,6 +2229,55 @@ 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;
+
+#define STR_STRICT_EQ(str, len, cmp) \
+	(str_has_prefix(str, cmp) == len)
+
+	/* The connection status extras can be set without a mode. */
+	if (STR_STRICT_EQ(name, name_end, "d") ||
+	    STR_STRICT_EQ(name, name_end, "D") ||
+	    STR_STRICT_EQ(name, name_end, "e"))
+		return 0;
+
+	/*
+	 * We're sure we're a named mode at that 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 +2314,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,17 +2355,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 */
 
-			strcpy(mode->name, drm_named_modes_whitelist[i]);
-			mode->specified = true;
-			break;
-		}
+	if (mode_end) {
+		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 */

-- 
b4 0.11.0-dev-7da52

Den 13.10.2022 15.18, skrev Maxime Ripard:
> 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.
> 
> Signed-off-by: Maxime Ripard <maxime at cerno.tech>
> 
I was hoping that someone else would step up and review these parser
patches since the parser code is rather difficult to read, for me at
least. I have studied it now, so I'll give it a try.
> ---
> 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 | 73
++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 62 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index c0dceff51cac..2f020ef2ddf2 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -2229,6 +2229,55 @@ 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;
name_end can't be zero since the argument is checked before calling this
function.
> +
> +	/* 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;
I think this check actually belongs in
drm_mode_parse_command_line_for_connector() after options_off is set. If
theres an equal sign it should skip all mode parsing and skip down to
drm_mode_parse_cmdline_options(). Which probably means that the mode
parsing should have been moved out to separate function to avoid using a
goto.
But that's probably beyond the scope of this patchset :)
> +
> +#define STR_STRICT_EQ(str, len, cmp) \
> +	(str_has_prefix(str, cmp) == len)
> +
> +	/* The connection status extras can be set without a mode. */
> +	if (STR_STRICT_EQ(name, name_end, "d") ||
> +	    STR_STRICT_EQ(name, name_end, "D") ||
> +	    STR_STRICT_EQ(name, name_end, "e"))
> +		return 0;
It took me a while to understand what is going on here.
If str_has_prefix() finds a match it returns strlen(prefix). Since
prefix is always of length 1, name_end has to always be 1 for the
statement to be true.

I would have written it like this:

	/* The connection status extras can be set without a mode. */
	if (name_end == 1) {
		if (name[0] == "d" || name[0] == "D" || name[0] ==
"e")
			return 0;
	}
> +
> +	/*
> +	 * We're sure we're a named mode at that point, iterate over the
that -> this ?
> +	 * 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 +2314,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,17 +2355,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 */
>  
> -			strcpy(mode->name, drm_named_modes_whitelist[i]);
> -			mode->specified = true;
> -			break;
> -		}
> +	if (mode_end) {
Shouldn't this be:

	if (!mode_end)
		return false;

I can't see how mode_end == 0 can produce anything valid.
Scenarios for mode_option that gives mode_end == 0:
- zero length
- starts with a comma
- starts with -bpp
- starts with @refresh

Maybe it's a separate patch with matching tests ;)

Noralf.
> +		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 */
>