Nouveau - Aug 2022 - [PATCH v2 14/41] drm/modes: Move named modes parsing to a separate function

Hi Maxime,

On Mon, Aug 29, 2022 at 3:13 PM 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.
>
> Signed-off-by: Maxime Ripard <maxime at cerno.tech>
Thanks for your patch!
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -1909,6 +1909,9 @@ void drm_connector_list_update(struct drm_connector
*connector)
>  }
>  EXPORT_SYMBOL(drm_connector_list_update);
>
> +#define STR_STRICT_EQ(str, len, cmp) \
> +       ((strlen(cmp) == len) && !strncmp(str, cmp, len))
This is not part of the move, but newly added.
> +
>  static int drm_mode_parse_cmdline_bpp(const char *str, char **end_ptr,
>                                       struct drm_cmdline_mode *mode)
>  {
> @@ -2208,6 +2211,52 @@ 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;
This is already checked by the caller.
> +
> +       /* 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 (STR_STRICT_EQ(name, name_end, "d") ||
> +           STR_STRICT_EQ(name, name_end, "D") ||
> +           STR_STRICT_EQ(name, name_end, "e"))
> +               return 0;
These checks are not part of the move, and should probably be added
in a separate patch.
> +
> +       /*
> +        * 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
> @@ -2244,7 +2293,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;
> @@ -2285,17 +2334,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
*/
>
Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at
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

On Tue, 30 Aug 2022, Geert Uytterhoeven <geert at linux-m68k.org>
wrote:
> On Mon, Aug 29, 2022 at 3:13 PM Maxime Ripard <maxime at cerno.tech>
wrote:
>> +#define STR_STRICT_EQ(str, len, cmp) \
>> +       ((strlen(cmp) == len) && !strncmp(str, cmp, len))
>
> This is not part of the move, but newly added.
The same construct is also duplicated elsewhere in the series, and I
kept being confused by it. The above is precisely the same as:

	str_has_prefix(str, cmp) == len

Which is more intuitive and available in string.h instead of being a
local duplicate.


BR,
Jani.

-- 
Jani Nikula, Intel Open Source Graphics Center