| Message ID | 20220728-rpi-analog-tv-properties-v1-9-3d53ae722097@cerno.tech (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Neil Armstrong |
| Headers | show |
| Series | drm: Analog TV Improvements | expand |
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
On Fri, Aug 12, 2022 at 03:27:17PM +0200, Geert Uytterhoeven wrote: > 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 > ;-) Indeed, I forgot about that one, sorry :/ I think I'd still prefer to have the check for refresh + named mode outside of the function, since I see them as an "integration" issue, not a parsing one. It's not the named mode parsing that fails, but the fact that we both have a valid refresh and a valid named mode. > > > --- 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? Yeah, it's something I went back and forth to between the dev, and it's an artifact. I'll drop that patch, take your version and move the refresh check to drm_mode_parse_command_line_for_connector if that's alright for you? Maxime
Hi Maxime, On Tue, Aug 16, 2022 at 3:46 PM Maxime Ripard <maxime@cerno.tech> wrote: > On Fri, Aug 12, 2022 at 03:27:17PM +0200, Geert Uytterhoeven wrote: > > 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 > > ;-) > > Indeed, I forgot about that one, sorry :/ > > I think I'd still prefer to have the check for refresh + named mode > outside of the function, since I see them as an "integration" issue, not > a parsing one. > > It's not the named mode parsing that fails, but the fact that we both > have a valid refresh and a valid named mode. > > > > > > --- 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? > > Yeah, it's something I went back and forth to between the dev, and it's > an artifact. > > I'll drop that patch, take your version and move the refresh check to > drm_mode_parse_command_line_for_connector if that's alright for you? Fine for me. 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
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index 06a006e0b2e3..e85099df0326 100644 --- 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; +} + /** * drm_mode_parse_command_line_for_connector - parse command line modeline for connector * @mode_option: optional per connector mode option @@ -1809,7 +1831,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; @@ -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) + return false; - strcpy(mode->name, drm_named_modes_whitelist[i]); - mode->specified = true; - break; - } - } + drm_mode_parse_cmdline_named_mode(name, mode_end, mode); /* No named mode? Check for a normal mode argument, e.g. 1024x768 */ if (!mode->specified && isdigit(name[0])) {
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>