Thread (151 messages) 151 messages, 9 authors, 2022-08-30

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

From: Geert Uytterhoeven <geert@linux-m68k.org>
Date: 2022-08-12 13:27:37
Also in: dri-devel, linux-amlogic, linux-sunxi, lkml

Hi Maxime,

On Fri, Jul 29, 2022 at 6:36 PM Maxime Ripard [off-list ref] 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 <redacted>
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 (local)
;-)
quoted hunk ↗ jump to hunk
--- 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?
quoted hunk ↗ jump to hunk
+}
+
 /**
  * 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 (local)).
        /* 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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help