| Message ID | 20220728-rpi-analog-tv-properties-v2-14-459522d653a7@cerno.tech (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Analog TV Improvements | expand |
Hi Maxime, On Mon, Aug 29, 2022 at 3:13 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! > --- 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@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@linux-m68k.org> wrote: > On Mon, Aug 29, 2022 at 3:13 PM Maxime Ripard <maxime@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.
Hi, On Tue, Aug 30, 2022 at 01:43:07PM +0300, Jani Nikula wrote: > On Tue, 30 Aug 2022, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Mon, Aug 29, 2022 at 3:13 PM Maxime Ripard <maxime@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. I'm not sure what is confusing, but I can add a comment if needed. > The above is precisely the same as: > > str_has_prefix(str, cmp) == len Here, it's used to make sure we don't have a named mode starting with either e, d, or D. If I understood str_has_prefix() right, str_has_prefix("DUMB-MODE", "D") == strlen("DUMB-MODE") would return true, while it's actually what we want to avoid. It's also used indeed in drm_get_tv_mode_from_name(), where we try to match a list of names with one passed as argument. With drm_get_tv_mode_from_name("NSTC", strlen("NTSC")), we would end up calling str_has_prefix("NTSC-J", "NTSC") == strlen("NTSC-J") which would work. However, we end up calling prefix not a prefix, but an entire string we want to match against, which is very confusing to me too. Maxime
On Tue, 30 Aug 2022, Maxime Ripard <maxime@cerno.tech> wrote: > Hi, > > On Tue, Aug 30, 2022 at 01:43:07PM +0300, Jani Nikula wrote: >> On Tue, 30 Aug 2022, Geert Uytterhoeven <geert@linux-m68k.org> wrote: >> > On Mon, Aug 29, 2022 at 3:13 PM Maxime Ripard <maxime@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. > > I'm not sure what is confusing, but I can add a comment if needed. STR_STRICT_EQ() is what's confusing. I have to look at the implementation to understand what it means. What does "strict" string equality mean? > >> The above is precisely the same as: >> >> str_has_prefix(str, cmp) == len > > Here, it's used to make sure we don't have a named mode starting with > either e, d, or D. > > If I understood str_has_prefix() right, str_has_prefix("DUMB-MODE", "D") > == strlen("DUMB-MODE") would return true, while it's actually what we > want to avoid. That's not true, str_has_prefix("DUMB-MODE", "D") == strlen("D") is. > It's also used indeed in drm_get_tv_mode_from_name(), where we try to > match a list of names with one passed as argument. > > With drm_get_tv_mode_from_name("NSTC", strlen("NTSC")), we would end up > calling str_has_prefix("NTSC-J", "NTSC") == strlen("NTSC-J") which would > work. However, we end up calling prefix not a prefix, but an entire > string we want to match against, which is very confusing to me too. If I get this right, you have a string and you want to check if that has a certain prefix. Additionally, you want to check the prefix is a certain length. Sure, that the prefix is a certain length is more of a property of the string, which is NUL terminated later than at length, but that's doesn't really matter. That condition is simply str_has_prefix(string, prefix) == length. BR, Jani.
Hi, On Tue, Aug 30, 2022 at 04:36:23PM +0300, Jani Nikula wrote: > On Tue, 30 Aug 2022, Maxime Ripard <maxime@cerno.tech> wrote: > > Hi, > > > > On Tue, Aug 30, 2022 at 01:43:07PM +0300, Jani Nikula wrote: > >> On Tue, 30 Aug 2022, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > >> > On Mon, Aug 29, 2022 at 3:13 PM Maxime Ripard <maxime@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. > > > > I'm not sure what is confusing, but I can add a comment if needed. > > STR_STRICT_EQ() is what's confusing. I have to look at the > implementation to understand what it means. What does "strict" string > equality mean? Same length, same sequence of characters > > > >> The above is precisely the same as: > >> > >> str_has_prefix(str, cmp) == len > > > > Here, it's used to make sure we don't have a named mode starting with > > either e, d, or D. > > > > If I understood str_has_prefix() right, str_has_prefix("DUMB-MODE", "D") > > == strlen("DUMB-MODE") would return true, while it's actually what we > > want to avoid. > > That's not true, str_has_prefix("DUMB-MODE", "D") == strlen("D") is. > > > It's also used indeed in drm_get_tv_mode_from_name(), where we try to > > match a list of names with one passed as argument. > > > > With drm_get_tv_mode_from_name("NSTC", strlen("NTSC")), we would end up > > calling str_has_prefix("NTSC-J", "NTSC") == strlen("NTSC-J") which would > > work. However, we end up calling prefix not a prefix, but an entire > > string we want to match against, which is very confusing to me too. > > If I get this right, you have a string and you want to check if that has > a certain prefix. Additionally, you want to check the prefix is a > certain length. > > Sure, that the prefix is a certain length is more of a property of the > string, which is NUL terminated later than at length, but that's doesn't > really matter. > > That condition is simply str_has_prefix(string, prefix) == length. Ack. I'm ok with the implementation being done that way, but I'd really prefer to still have some macro to make the name less confusing. Would that work for you? What name would be better in your opinion? Thanks! Maxime
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index 99a21e5cd00d..0636cb707544 100644 --- 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)) + 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; + + /* 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; + + /* + * 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 */
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>