Hi all,
This patch series contains fixes and improvements for specifying video
modes on the kernel command line.
Changes compared to v1[1]:
- Add Reviewed-by, Acked-by,
- Keep length check.
This has been tested on ARAnyM using a work-in-progress Atari DRM driver
(more info and related patches can be found in [2]).
Thanks for your comments!
[1] "[PATCH 0/5] drm/modes: Command line mode selection fixes and
improvements"
https://lore.kernel.org/r/[email protected]
[2] "[PATCH v3 00/10] drm: Add support for low-color frame buffer formats"
https://lore.kernel.org/r/[email protected]
Geert Uytterhoeven (5):
drm/modes: parse_cmdline: Handle empty mode name part
drm/modes: Extract drm_mode_parse_cmdline_named_mode()
drm/modes: parse_cmdline: Make mode->*specified handling more uniform
drm/modes: Add support for driver-specific named modes
drm/modes: parse_cmdline: Add support for named modes containing
dashes
drivers/gpu/drm/drm_modes.c | 56 ++++++++++++++++++++++++++-----------
include/drm/drm_connector.h | 10 +++++++
2 files changed, 49 insertions(+), 17 deletions(-)
--
2.25.1
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
If no mode name part was specified, mode_end is zero, and the "ret ==
mode_end" check does the wrong thing.
Fix this by skipping all named mode handling when mode_end is zero.
Fixes: 7b1cce760afe38b4 ("drm/modes: parse_cmdline: Allow specifying stand-alone options")
Signed-off-by: Geert Uytterhoeven <[email protected]>
Reviewed-by: Hans de Goede <[email protected]>
Acked-by: Thomas Zimmermann <[email protected]>
---
v2:
- Add Reviewed-by, Acked-by,
- Keep "ret == mode_end" check.
---
drivers/gpu/drm/drm_modes.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 14b746f7ba975954..67773740c74c9ba0 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1823,7 +1823,7 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
}
/* First check for a named mode */
- for (i = 0; i < ARRAY_SIZE(drm_named_modes_whitelist); i++) {
+ for (i = 0; mode_end && 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)
--
2.25.1
The mode parsing code recognizes named modes only if they are explicitly
listed in the internal whitelist, which is currently limited to "NTSC"
and "PAL".
Provide a mechanism for drivers to override this list to support custom
mode names.
Ideally, this list should just come from the driver's actual list of
modes, but connector->probed_modes is not yet populated at the time of
parsing.
Signed-off-by: Geert Uytterhoeven <[email protected]>
Reviewed-by: Hans de Goede <[email protected]>
---
v2:
- Add Reviewed-by.
---
drivers/gpu/drm/drm_modes.c | 15 +++++++++++----
include/drm/drm_connector.h | 10 ++++++++++
2 files changed, 21 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 0cbf0467f263b30a..bfc3a08614522689 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1748,24 +1748,30 @@ static int drm_mode_parse_cmdline_options(const char *str,
static const char * const drm_named_modes_whitelist[] = {
"NTSC",
"PAL",
+ NULL
};
static int drm_mode_parse_cmdline_named_mode(const char *name,
unsigned int length, bool refresh,
+ const struct drm_connector *connector,
struct drm_cmdline_mode *mode)
{
+ const char * const *named_modes_whitelist;
unsigned int i;
int ret;
- for (i = 0; i < ARRAY_SIZE(drm_named_modes_whitelist); i++) {
- ret = str_has_prefix(name, drm_named_modes_whitelist[i]);
+ named_modes_whitelist = connector->named_modes_whitelist ? :
+ drm_named_modes_whitelist;
+
+ for (i = 0; named_modes_whitelist[i]; i++) {
+ ret = str_has_prefix(name, named_modes_whitelist[i]);
if (ret != length)
continue;
if (refresh)
return -EINVAL; /* named + refresh is invalid */
- strcpy(mode->name, drm_named_modes_whitelist[i]);
+ strcpy(mode->name, named_modes_whitelist[i]);
mode->specified = true;
return 0;
}
@@ -1849,7 +1855,8 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
/* First check for a named mode */
if (mode_end) {
ret = drm_mode_parse_cmdline_named_mode(name, mode_end,
- refresh_ptr, mode);
+ refresh_ptr, connector,
+ mode);
if (ret)
return false;
}
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 3ac4bf87f2571c4c..6361f8a596c01107 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -1659,6 +1659,16 @@ struct drm_connector {
/** @hdr_sink_metadata: HDR Metadata Information read from sink */
struct hdr_sink_metadata hdr_sink_metadata;
+
+ /**
+ * @named_modes_whitelist:
+ *
+ * Optional NULL-terminated array of names to be considered valid mode
+ * names. This lets the command line option parser distinguish between
+ * mode names and freestanding extras and/or options.
+ * If not set, a set of defaults will be used.
+ */
+ const char * const *named_modes_whitelist;
};
#define obj_to_connector(x) container_of(x, struct drm_connector, base)
--
2.25.1
Extract the code to check for a named mode parameter into its own
function, to streamline the main parsing flow.
Signed-off-by: Geert Uytterhoeven <[email protected]>
Reviewed-by: Hans de Goede <[email protected]>
Acked-by: Thomas Zimmermann <[email protected]>
---
v2:
- Add Reviewed-by, Acked-by,
- Fix length check.
---
drivers/gpu/drm/drm_modes.c | 40 +++++++++++++++++++++++++++----------
1 file changed, 29 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 67773740c74c9ba0..a3df18fccb31fa77 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1749,6 +1749,29 @@ static const char * const drm_named_modes_whitelist[] = {
"PAL",
};
+static int drm_mode_parse_cmdline_named_mode(const char *name,
+ unsigned int length, bool refresh,
+ struct drm_cmdline_mode *mode)
+{
+ unsigned int i;
+ int ret;
+
+ for (i = 0; i < ARRAY_SIZE(drm_named_modes_whitelist); i++) {
+ ret = str_has_prefix(name, drm_named_modes_whitelist[i]);
+ if (ret != length)
+ continue;
+
+ if (refresh)
+ return -EINVAL; /* named + refresh is invalid */
+
+ strcpy(mode->name, drm_named_modes_whitelist[i]);
+ mode->specified = true;
+ return 0;
+ }
+
+ return 0;
+}
+
/**
* drm_mode_parse_command_line_for_connector - parse command line modeline for connector
* @mode_option: optional per connector mode option
@@ -1785,7 +1808,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;
@@ -1823,16 +1846,11 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
}
/* First check for a named mode */
- for (i = 0; mode_end && 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,
+ refresh_ptr, mode);
+ if (ret)
+ return false;
}
/* No named mode? Check for a normal mode argument, e.g. 1024x768 */
--
2.25.1
The various mode->*specified flags are not handled in an uniform way:
some flags are set by the corresponding drm_mode_parse_cmdline_*()
function, some flags by the caller of the function, and some flags by
both.
Make this uniform by making this the responsibility of the various
parsing helpers, i.e.
- Move the setting of mode->specified from caller to callee,
- Drop the duplicate setting of mode->bpp_specified and
mode->refresh_specified from callers.
Signed-off-by: Geert Uytterhoeven <[email protected]>
Reviewed-by: Hans de Goede <[email protected]>
Acked-by: Thomas Zimmermann <[email protected]>
---
v2:
- Add Reviewed-by, Acked-by.
---
drivers/gpu/drm/drm_modes.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index a3df18fccb31fa77..0cbf0467f263b30a 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1599,6 +1599,7 @@ static int drm_mode_parse_cmdline_res_mode(const char *str, unsigned int length,
mode->yres = yres;
mode->cvt = cvt;
mode->rb = rb;
+ mode->specified = true;
return 0;
}
@@ -1861,8 +1862,6 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
mode);
if (ret)
return false;
-
- mode->specified = true;
}
/* No mode? Check for freestanding extras and/or options */
@@ -1884,8 +1883,6 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
ret = drm_mode_parse_cmdline_bpp(bpp_ptr, &bpp_end_ptr, mode);
if (ret)
return false;
-
- mode->bpp_specified = true;
}
if (refresh_ptr) {
@@ -1893,8 +1890,6 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
&refresh_end_ptr, mode);
if (ret)
return false;
-
- mode->refresh_specified = true;
}
/*
--
2.25.1
Hi,
On Thu, Jul 14, 2022 at 11:04:06AM +0200, Geert Uytterhoeven wrote:
> If no mode name part was specified, mode_end is zero, and the "ret ==
> mode_end" check does the wrong thing.
>
> Fix this by skipping all named mode handling when mode_end is zero.
>
> Fixes: 7b1cce760afe38b4 ("drm/modes: parse_cmdline: Allow specifying stand-alone options")
> Signed-off-by: Geert Uytterhoeven <[email protected]>
> Reviewed-by: Hans de Goede <[email protected]>
> Acked-by: Thomas Zimmermann <[email protected]>
We should add a test for that in drivers/gpu/drm/tests/drm_cmdline_parser_test.c
Maxime
On Thu, Jul 14, 2022 at 11:04:07AM +0200, Geert Uytterhoeven wrote:
> Extract the code to check for a named mode parameter into its own
> function, to streamline the main parsing flow.
>
> Signed-off-by: Geert Uytterhoeven <[email protected]>
> Reviewed-by: Hans de Goede <[email protected]>
> Acked-by: Thomas Zimmermann <[email protected]>
Acked-by: Maxime Ripard <[email protected]>
Maxime
On Thu, Jul 14, 2022 at 11:04:08AM +0200, Geert Uytterhoeven wrote:
> The various mode->*specified flags are not handled in an uniform way:
> some flags are set by the corresponding drm_mode_parse_cmdline_*()
> function, some flags by the caller of the function, and some flags by
> both.
>
> Make this uniform by making this the responsibility of the various
> parsing helpers, i.e.
> - Move the setting of mode->specified from caller to callee,
> - Drop the duplicate setting of mode->bpp_specified and
> mode->refresh_specified from callers.
>
> Signed-off-by: Geert Uytterhoeven <[email protected]>
> Reviewed-by: Hans de Goede <[email protected]>
> Acked-by: Thomas Zimmermann <[email protected]>
Acked-by: Maxime Ripard <[email protected]>
Maxime
On Thu, Jul 14, 2022 at 11:04:09AM +0200, Geert Uytterhoeven wrote:
> The mode parsing code recognizes named modes only if they are explicitly
> listed in the internal whitelist, which is currently limited to "NTSC"
> and "PAL".
>
> Provide a mechanism for drivers to override this list to support custom
> mode names.
>
> Ideally, this list should just come from the driver's actual list of
> modes, but connector->probed_modes is not yet populated at the time of
> parsing.
>
> Signed-off-by: Geert Uytterhoeven <[email protected]>
> Reviewed-by: Hans de Goede <[email protected]>
Like we discussed on IRC, I'm not sure allowing drivers to handle named
modes is the right thing to do.
Named modes in general were a workaround the fact that we were missing
infos in drm_display_mode to describe all the modes.
I think we really should focus on addressing that first, and then
creating some kind of backward compat layer to create an initial DRM
state from a named mode provided on the command line.
Maxime