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 <[email protected]>
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 */
--
b4 0.10.0-dev-65ba7
Hi Maxime,
On Mon, Aug 29, 2022 at 3:13 PM Maxime Ripard <[email protected]> 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 <[email protected]>
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 -- [email protected]
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 <[email protected]> wrote:
> On Mon, Aug 29, 2022 at 3:13 PM Maxime Ripard <[email protected]> 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
Hi,
On Tue, Aug 30, 2022 at 01:43:07PM +0300, Jani Nikula wrote:
> On Tue, 30 Aug 2022, Geert Uytterhoeven <[email protected]> wrote:
> > On Mon, Aug 29, 2022 at 3:13 PM Maxime Ripard <[email protected]> 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 <[email protected]> wrote:
> Hi,
>
> On Tue, Aug 30, 2022 at 01:43:07PM +0300, Jani Nikula wrote:
>> On Tue, 30 Aug 2022, Geert Uytterhoeven <[email protected]> wrote:
>> > On Mon, Aug 29, 2022 at 3:13 PM Maxime Ripard <[email protected]> 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.
--
Jani Nikula, Intel Open Source Graphics Center
Hi,
On Tue, Aug 30, 2022 at 04:36:23PM +0300, Jani Nikula wrote:
> On Tue, 30 Aug 2022, Maxime Ripard <[email protected]> wrote:
> > Hi,
> >
> > On Tue, Aug 30, 2022 at 01:43:07PM +0300, Jani Nikula wrote:
> >> On Tue, 30 Aug 2022, Geert Uytterhoeven <[email protected]> wrote:
> >> > On Mon, Aug 29, 2022 at 3:13 PM Maxime Ripard <[email protected]> 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