Re: [PATCH v1 09/35] drm/modes: Move named modes parsing to a separate function

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi Maxime,

On Fri, Jul 29, 2022 at 6:36 PM Maxime Ripard <maxime@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.
>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Thanks for your patch, which looks similar to my "[PATCH v2 2/5]
drm/modes: Extract drm_mode_parse_cmdline_named_mode()"
(https://lore.kernel.org/dri-devel/1371554419ae63cb54c2a377db0c1016fcf200bb.1657788997.git.geert@linux-m68k.org
;-)

> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -1773,6 +1773,28 @@ static const char * const drm_named_modes_whitelist[] = {
>         "PAL",
>  };
>
> +static bool drm_mode_parse_cmdline_named_mode(const char *name,
> +                                             unsigned int name_end,
> +                                             struct drm_cmdline_mode *cmdline_mode)
> +{
> +       unsigned int i;
> +
> +       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 true;
> +       }
> +
> +       return false;

What's the point in returning a value, if it is never checked?
Just make this function return void?

> +}
> +
>  /**
>   * drm_mode_parse_command_line_for_connector - parse command line modeline for connector
>   * @mode_option: optional per connector mode option
> @@ -1848,18 +1870,14 @@ 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 */
> +       /*
> +        * 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 (!isdigit(name[0]) && refresh_ptr)

This condition may have to be relaxed, if we want to support e.g.
"hd720p@50", cfr. my comments on "[PATCH v1 05/35] drm/connector:
Add TV standard property".

> +               return false;
>
> -                       strcpy(mode->name, drm_named_modes_whitelist[i]);
> -                       mode->specified = true;
> -                       break;
> -               }
> -       }
> +       drm_mode_parse_cmdline_named_mode(name, mode_end, mode);

This call needs to be conditional on mode_end being non-zero, cfr. my
patch "[PATCH v2 1/5] drm/modes: parse_cmdline: Handle empty mode name
part" (https://lore.kernel.org/dri-devel/302d0737539daa2053134e8f24fdf37e3d939e1e.1657788997.git.geert@linux-m68k.org).

>
>         /* No named mode? Check for a normal mode argument, e.g. 1024x768 */
>         if (!mode->specified && isdigit(name[0])) {


Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds