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@xxxxxxxxxx>
>
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 */
>
