[PATCH 0/5] drm/modes: Command line mode selection fixes and improvements

	Hi all,

This patch series contains fixes and improvements for specifying video
modes on the kernel command line.

This has been tested on ARAnyM using a work-in-progress Atari DRM driver
(more info and related patches can be found in [1]).

Thanks for your comments!

[1] "[PATCH v3 00/10] drm: Add support for low-color frame buffer formats"
    https://lore.kernel.org/r/cover.1657294931.git.geert@linux-m68k.org

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 | 57 ++++++++++++++++++++++++++-----------
 include/drm/drm_connector.h | 10 +++++++
 2 files changed, 50 insertions(+), 17 deletions(-)

-- 
2.25.1

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

If no mode name part was specified, mode_end is zero, and the "ret ==
mode_end" check does the wrong thing.

Fix this by checking for a non-zero return value instead.
While at it, skip all named mode handling when mode_end is zero, as it
is futile.

Fixes: 7b1cce760afe38b4 ("drm/modes: parse_cmdline: Allow specifying stand-alone options")
Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
 drivers/gpu/drm/drm_modes.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 14b746f7ba975954..30a7be97707bfb16 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c

@@ -1823,9 +1823,9 @@ 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 (ret) {
 			if (refresh_ptr)
 				return false; /* named + refresh is invalid */
 

-- 
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 <geert@linux-m68k.org>
---
 drivers/gpu/drm/drm_modes.c | 41 +++++++++++++++++++++++++++----------
 1 file changed, 30 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 30a7be97707bfb16..434383469e9d984d 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c

@@ -1749,6 +1749,30 @@ 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)
+			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 +1809,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 +1847,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) {
-			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 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 <geert@linux-m68k.org>
---
 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 9ce275fbda566b7c..7a00eb6df502e991 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c

@@ -1748,25 +1748,31 @@ 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)
 			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;
 	}

@@ -1850,7 +1856,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

It is fairly common for named video modes to contain dashes (e.g.
"tt-mid" on Atari, "dblntsc-ff" on Amiga).  Currently such mode names
are not recognized, as the dash is considered to be a separator between
mode name and bpp.

Fix this by skipping any dashes that are not followed immediately by a
digit when looking for the separator.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
 drivers/gpu/drm/drm_modes.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 7a00eb6df502e991..52e852518c6ad8e9 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c

@@ -1828,6 +1828,8 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
 
 	/* Try to locate the bpp and refresh specifiers, if any */
 	bpp_ptr = strchr(name, '-');
+	while (bpp_ptr && !isdigit(bpp_ptr[1]))
+		bpp_ptr = strchr(bpp_ptr + 1, '-');
 	if (bpp_ptr)
 		bpp_off = bpp_ptr - name;
 

-- 
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 <geert@linux-m68k.org>
---
 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 434383469e9d984d..9ce275fbda566b7c 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;
 }

@@ -1862,8 +1863,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 */

@@ -1885,8 +1884,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) {

@@ -1894,8 +1891,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 Geert,

On 7/8/22 20:21, 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 checking for a non-zero return value instead.

Which is wrong to do, since now if you have e.g. a mode list
with:

"dblntsc",
"dblntsc-ff"

in there and the cmdline contains "dblntsc-ff" then you
will already stop with a (wrong!) match at "dblntsc".

While at it, skip all named mode handling when mode_end is zero, as it
is futile.

AFAICT, this is actually what needs to be done to fix this, while keeping
the ret == mode_end check.

Regards,

Hans

Hi Geert,

On 7/8/22 20:21, Geert Uytterhoeven wrote:

	Hi all,

This patch series contains fixes and improvements for specifying video
modes on the kernel command line.

This has been tested on ARAnyM using a work-in-progress Atari DRM driver
(more info and related patches can be found in [1]).

Thanks for your comments!

[1] "[PATCH v3 00/10] drm: Add support for low-color frame buffer formats"
    https://lore.kernel.org/r/cover.1657294931.git.geert@linux-m68k.org

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

Thanks, I have some remarks on patches 1/5 and 2/5 the rest looks
good to me. 

For 1/5 and 2/5 with my remarks addressed:

Reviewed-by: Hans de Goede <redacted>

For 3/5, 4/5 and 5/5:

Reviewed-by: Hans de Goede <redacted>

Regards,

Hans

 drivers/gpu/drm/drm_modes.c | 57 ++++++++++++++++++++++++++-----------
 include/drm/drm_connector.h | 10 +++++++
 2 files changed, 50 insertions(+), 17 deletions(-)

Hi,

On 7/8/22 20:21, Geert Uytterhoeven wrote:

As discussed in my review of 1/5 this needs to become:

		if (ret != length)

+			continue;

Which renders my other comment on this patch (length not being used) mute.

Regards,

Hans

Hi Hans.

On Fri, Jul 8, 2022 at 9:28 PM Hans de Goede [off-list ref] wrote:

On 7/8/22 20:21, Geert Uytterhoeven wrote:

quoted

If no mode name part was specified, mode_end is zero, and the "ret ==
mode_end" check does the wrong thing.

Fix this by checking for a non-zero return value instead.

Which is wrong to do, since now if you have e.g. a mode list
with:

"dblntsc",
"dblntsc-ff"

in there and the cmdline contains "dblntsc-ff" then you
will already stop with a (wrong!) match at "dblntsc".

It indeed behaves that way, and did so before, as str_has_prefix()
checks for a matching prefix, and thus may never get to the full
match.  However, can we change that to an exact match, without
introducing regressions?
This can be avoided by reverse-sorting the modelist (or iterating
backwards through a sorted modelist), though.

quoted

While at it, skip all named mode handling when mode_end is zero, as it
is futile.

AFAICT, this is actually what needs to be done to fix this, while keeping
the ret == mode_end check.

"ret == mode_end" or "ret" doesn't matter (except for the special
case of mode_end is zero), as str_has_prefix() returns either zero or
the length of the prefix.  Hence it never returns a non-zero value
smaller than the length of the prefix.

quoted

Fixes: 7b1cce760afe38b4 ("drm/modes: parse_cmdline: Allow specifying stand-alone options")
Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
 drivers/gpu/drm/drm_modes.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 14b746f7ba975954..30a7be97707bfb16 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c

@@ -1823,9 +1823,9 @@ 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 (ret) {
                      if (refresh_ptr)
                              return false; /* named + refresh is invalid */

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

Hi Hans,

On Fri, Jul 8, 2022 at 10:06 PM Geert Uytterhoeven [off-list ref] wrote:

On Fri, Jul 8, 2022 at 9:28 PM Hans de Goede [off-list ref] wrote:

quoted

On 7/8/22 20:21, Geert Uytterhoeven wrote:

quoted

If no mode name part was specified, mode_end is zero, and the "ret ==
mode_end" check does the wrong thing.

Fix this by checking for a non-zero return value instead.

Which is wrong to do, since now if you have e.g. a mode list
with:

"dblntsc",
"dblntsc-ff"

in there and the cmdline contains "dblntsc-ff" then you
will already stop with a (wrong!) match at "dblntsc".

It indeed behaves that way, and did so before, as str_has_prefix()
checks for a matching prefix, and thus may never get to the full
match.  However, can we change that to an exact match, without
introducing regressions?
This can be avoided by reverse-sorting the modelist (or iterating
backwards through a sorted modelist), though.

quoted

quoted

While at it, skip all named mode handling when mode_end is zero, as it
is futile.

AFAICT, this is actually what needs to be done to fix this, while keeping
the ret == mode_end check.

"ret == mode_end" or "ret" doesn't matter (except for the special
case of mode_end is zero), as str_has_prefix() returns either zero or
the length of the prefix.  Hence it never returns a non-zero value
smaller than the length of the prefix.

Ignore that.  I finally saw what's really happening.
And I do agree with your comment.

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

Hi Hans,

On Fri, Jul 8, 2022 at 9:46 PM Hans de Goede [off-list ref] wrote:

On 7/8/22 20:21, Geert Uytterhoeven wrote:

quoted

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 <geert@linux-m68k.org>

quoted

--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c

@@ -1749,6 +1749,30 @@ 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)

As discussed in my review of 1/5 this needs to become:

                if (ret != length)

quoted

+                     continue;

Agreed.

Which renders my other comment on this patch (length not being used) mute.

/me wonders if he would have seen the light earlier if gcc would have
warned about that...

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

Hi Geert

Am 08.07.22 um 20:21 schrieb Geert Uytterhoeven:

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.

I've looked for code that uses these names, couldn't find any. How is 
this being used in practice? For example, if I say "PAL" on the command 
line, is there DRM code that fills in the PAL mode parameters?

And another question I have is whether this whitelist belongs into the 
driver at all. Standard modes exist independent from drivers or 
hardware. Shouldn't there simply be a global list of all possible mode 
names? Drivers would filter out the unsupported modes anyway.

Best regards
Thomas

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

Hi

Am 08.07.22 um 20:21 schrieb Geert Uytterhoeven:

	Hi all,

This patch series contains fixes and improvements for specifying video
modes on the kernel command line.

This has been tested on ARAnyM using a work-in-progress Atari DRM driver
(more info and related patches can be found in [1]).

Thanks for your comments!

Patches 1 to 3 look reasonable to me. For those:

Acked-by: Thomas Zimmermann <tzimmermann@suse.de>

Please see my questions on patch 4.

Best regards
Thomas

[1] "[PATCH v3 00/10] drm: Add support for low-color frame buffer formats"
     https://lore.kernel.org/r/cover.1657294931.git.geert@linux-m68k.org

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 | 57 ++++++++++++++++++++++++++-----------
  include/drm/drm_connector.h | 10 +++++++
  2 files changed, 50 insertions(+), 17 deletions(-)

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

Hi Thomas,

On Mon, Jul 11, 2022 at 11:03:38AM +0200, Thomas Zimmermann wrote:

Am 08.07.22 um 20:21 schrieb Geert Uytterhoeven:

quoted

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.

I've looked for code that uses these names, couldn't find any. How is this
being used in practice? For example, if I say "PAL" on the command line, is
there DRM code that fills in the PAL mode parameters?

We have some code to deal with this in sun4i:
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/sun4i/sun4i_tv.c#L292

It's a bit off topic, but for TV standards, I'm still not sure what the
best course of action is. There's several interactions that make this a
bit troublesome:

  * Some TV standards differ by their mode (ie, PAL vs NSTC), but some
    other differ by parameters that are not part of drm_display_mode
    (NTSC vs NSTC-J where the only difference is the black and blanking
    signal levels for example).

  * The mode names allow to provide a fairly convenient way to add that
    extra information, but the userspace is free to create its own mode
    and might omit the mode name entirely.

So in the code above, if the name has been preserved we match by name,
but we fall back to matching by mode if it hasn't been, which in this
case means that we have no way to differentiate between NTSC, NTSC-J,
PAL-M in this case.

We have some patches downstream for the RaspberryPi that has the TV
standard as a property. There's a few extra logic required for the
userspace (like setting the PAL property, with the NTSC mode) so I'm not
sure it's preferable.

Or we could do something like a property to try that standard, and
another that reports the one we actually chose.

And another question I have is whether this whitelist belongs into the
driver at all. Standard modes exist independent from drivers or hardware.
Shouldn't there simply be a global list of all possible mode names? Drivers
would filter out the unsupported modes anyway.

We should totally do something like that, yeah

Maxime

Hi Thomas,

On Mon, Jul 11, 2022 at 11:03 AM Thomas Zimmermann [off-list ref] wrote:

Am 08.07.22 um 20:21 schrieb Geert Uytterhoeven:

quoted

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.

I've looked for code that uses these names, couldn't find any. How is
this being used in practice? For example, if I say "PAL" on the command
line, is there DRM code that fills in the PAL mode parameters?

I guess Maxime knows, as he added the whitelist?
Reading the description of commit 3764137906a5acec ("drm/modes:
Introduce a whitelist for the named modes"), it looks like this is
more about preventing the parser from taking any string as a random
mode, than about adding support for "PAL" or "NTSC"?

Note that drivers/gpu/drm/i915/display/intel_tv.c defines an array of
tv_modes[], including "PAL", so perhaps these end up as named modes?

And another question I have is whether this whitelist belongs into the
driver at all. Standard modes exist independent from drivers or
hardware. Shouldn't there simply be a global list of all possible mode
names? Drivers would filter out the unsupported modes anyway.

For standard modes, I agree.  And these are usually specified by
resolution and refresh rate (e.g. "640x480@60", instead of "480p").

But legacy hardware may have very limited support for programmable
pixel clocks (e.g. Amiga is limited to pixel clocks of 7, 14, or 28
MHz), so the standard modes are a bad match, or may not work at all.
Hence drivers may need to provide their own modes, but it seems wrong
to me to make these non-standard modes global, and possibly pollute
the experience for everyone.

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

Hi

Am 11.07.22 um 11:35 schrieb Geert Uytterhoeven:

Hi Thomas,

On Mon, Jul 11, 2022 at 11:03 AM Thomas Zimmermann [off-list ref] wrote:

quoted

Am 08.07.22 um 20:21 schrieb Geert Uytterhoeven:

quoted

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.

I've looked for code that uses these names, couldn't find any. How is
this being used in practice? For example, if I say "PAL" on the command
line, is there DRM code that fills in the PAL mode parameters?

I guess Maxime knows, as he added the whitelist?

Yeah, I saw his reply already.

Reading the description of commit 3764137906a5acec ("drm/modes:
Introduce a whitelist for the named modes"), it looks like this is
more about preventing the parser from taking any string as a random
mode, than about adding support for "PAL" or "NTSC"?

Note that drivers/gpu/drm/i915/display/intel_tv.c defines an array of
tv_modes[], including "PAL", so perhaps these end up as named modes?

quoted

And another question I have is whether this whitelist belongs into the
driver at all. Standard modes exist independent from drivers or
hardware. Shouldn't there simply be a global list of all possible mode
names? Drivers would filter out the unsupported modes anyway.

For standard modes, I agree.  And these are usually specified by
resolution and refresh rate (e.g. "640x480@60", instead of "480p").

But legacy hardware may have very limited support for programmable
pixel clocks (e.g. Amiga is limited to pixel clocks of 7, 14, or 28
MHz), so the standard modes are a bad match, or may not work at all.
Hence drivers may need to provide their own modes, but it seems wrong
to me to make these non-standard modes global, and possibly pollute
the experience for everyone.

I don't really have a strong opinion, but having all modes in one global 
list is quite user-friendly. It's all there for everyone. Otherwise 
users would somehow have to know which hardware supports which modes. 
That's actually the job of each driver's mode_valid and atomic_check 
functions.

Best regards
Thomas

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

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

Hi Maxime

Am 11.07.22 um 11:35 schrieb Maxime Ripard:

Hi Thomas,

On Mon, Jul 11, 2022 at 11:03:38AM +0200, Thomas Zimmermann wrote:

quoted

Am 08.07.22 um 20:21 schrieb Geert Uytterhoeven:

quoted

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.

I've looked for code that uses these names, couldn't find any. How is this
being used in practice? For example, if I say "PAL" on the command line, is
there DRM code that fills in the PAL mode parameters?

We have some code to deal with this in sun4i:
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/sun4i/sun4i_tv.c#L292

It's a bit off topic, but for TV standards, I'm still not sure what the
best course of action is. There's several interactions that make this a
bit troublesome:

   * Some TV standards differ by their mode (ie, PAL vs NSTC), but some
     other differ by parameters that are not part of drm_display_mode
     (NTSC vs NSTC-J where the only difference is the black and blanking
     signal levels for example).

   * The mode names allow to provide a fairly convenient way to add that
     extra information, but the userspace is free to create its own mode
     and might omit the mode name entirely.

So in the code above, if the name has been preserved we match by name,
but we fall back to matching by mode if it hasn't been, which in this
case means that we have no way to differentiate between NTSC, NTSC-J,
PAL-M in this case.

We have some patches downstream for the RaspberryPi that has the TV
standard as a property. There's a few extra logic required for the
userspace (like setting the PAL property, with the NTSC mode) so I'm not
sure it's preferable.

Or we could do something like a property to try that standard, and
another that reports the one we actually chose.

quoted

And another question I have is whether this whitelist belongs into the
driver at all. Standard modes exist independent from drivers or hardware.
Shouldn't there simply be a global list of all possible mode names? Drivers
would filter out the unsupported modes anyway.

We should totally do something like that, yeah

That sun code already looks like sometihng the DRM core/helpers should 
be doing. And if we want to support named modes well, there's a long 
list of modes in Wikipedia.

 
https://en.wikipedia.org/wiki/Video_Graphics_Array#/media/File:Vector_Video_Standards2.svg

Best regards
Thomas

Maxime

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

On Mon, Jul 11, 2022 at 01:11:14PM +0200, Thomas Zimmermann wrote:

Hi Maxime

Am 11.07.22 um 11:35 schrieb Maxime Ripard:

quoted

Hi Thomas,

On Mon, Jul 11, 2022 at 11:03:38AM +0200, Thomas Zimmermann wrote:

quoted

Am 08.07.22 um 20:21 schrieb Geert Uytterhoeven:

quoted

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.

I've looked for code that uses these names, couldn't find any. How is this
being used in practice? For example, if I say "PAL" on the command line, is
there DRM code that fills in the PAL mode parameters?

We have some code to deal with this in sun4i:
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/sun4i/sun4i_tv.c#L292

It's a bit off topic, but for TV standards, I'm still not sure what the
best course of action is. There's several interactions that make this a
bit troublesome:

   * Some TV standards differ by their mode (ie, PAL vs NSTC), but some
     other differ by parameters that are not part of drm_display_mode
     (NTSC vs NSTC-J where the only difference is the black and blanking
     signal levels for example).

   * The mode names allow to provide a fairly convenient way to add that
     extra information, but the userspace is free to create its own mode
     and might omit the mode name entirely.

So in the code above, if the name has been preserved we match by name,
but we fall back to matching by mode if it hasn't been, which in this
case means that we have no way to differentiate between NTSC, NTSC-J,
PAL-M in this case.

We have some patches downstream for the RaspberryPi that has the TV
standard as a property. There's a few extra logic required for the
userspace (like setting the PAL property, with the NTSC mode) so I'm not
sure it's preferable.

Or we could do something like a property to try that standard, and
another that reports the one we actually chose.

quoted

And another question I have is whether this whitelist belongs into the
driver at all. Standard modes exist independent from drivers or hardware.
Shouldn't there simply be a global list of all possible mode names? Drivers
would filter out the unsupported modes anyway.

We should totally do something like that, yeah

That sun code already looks like sometihng the DRM core/helpers should be
doing. And if we want to support named modes well, there's a long list of
modes in Wikipedia.

https://en.wikipedia.org/wiki/Video_Graphics_Array#/media/File:Vector_Video_Standards2.svg

Yeah, and NTSC is missing :)

Thinking about this some more, I'm not sure how we would do that. Like I
said, we would need some extra parameters to drm_display_mode (like
blanking levels) that the core would need to pass to the driver.

If we go through the property route, I think the core could just look at
the name, with the new mode and state, and the driver should deal with
it. I'm not sure we can do more than that.

Maxime

Hi Maxime,

On Mon, Jul 11, 2022 at 1:42 PM Maxime Ripard [off-list ref] wrote:

On Mon, Jul 11, 2022 at 01:11:14PM +0200, Thomas Zimmermann wrote:

quoted

Am 11.07.22 um 11:35 schrieb Maxime Ripard:

quoted

On Mon, Jul 11, 2022 at 11:03:38AM +0200, Thomas Zimmermann wrote:

quoted

Am 08.07.22 um 20:21 schrieb Geert Uytterhoeven:

quoted

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.

I've looked for code that uses these names, couldn't find any. How is this
being used in practice? For example, if I say "PAL" on the command line, is
there DRM code that fills in the PAL mode parameters?

We have some code to deal with this in sun4i:
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/sun4i/sun4i_tv.c#L292

It's a bit off topic, but for TV standards, I'm still not sure what the
best course of action is. There's several interactions that make this a
bit troublesome:

   * Some TV standards differ by their mode (ie, PAL vs NSTC), but some
     other differ by parameters that are not part of drm_display_mode
     (NTSC vs NSTC-J where the only difference is the black and blanking
     signal levels for example).

   * The mode names allow to provide a fairly convenient way to add that
     extra information, but the userspace is free to create its own mode
     and might omit the mode name entirely.

So in the code above, if the name has been preserved we match by name,
but we fall back to matching by mode if it hasn't been, which in this
case means that we have no way to differentiate between NTSC, NTSC-J,
PAL-M in this case.

We have some patches downstream for the RaspberryPi that has the TV
standard as a property. There's a few extra logic required for the
userspace (like setting the PAL property, with the NTSC mode) so I'm not
sure it's preferable.

Or we could do something like a property to try that standard, and
another that reports the one we actually chose.

quoted

And another question I have is whether this whitelist belongs into the
driver at all. Standard modes exist independent from drivers or hardware.
Shouldn't there simply be a global list of all possible mode names? Drivers
would filter out the unsupported modes anyway.

We should totally do something like that, yeah

That sun code already looks like sometihng the DRM core/helpers should be
doing. And if we want to support named modes well, there's a long list of
modes in Wikipedia.

https://en.wikipedia.org/wiki/Video_Graphics_Array#/media/File:Vector_Video_Standards2.svg

Yeah, and NTSC is missing :)

And that diagram is about the "digital" variant of PAL.
If you go the analog route, the only fixed parts are vfreq/hfreq,
number of lines, and synchronization. Other parameters like overscan
can vary.  The actual dot clock can vary wildly: while there is an
upper limit due to bandwidth limitations, you can come up with an
almost infinite number of video modes that can be called PAL, which
is one of the reasons why I don't want hardware-specific variants to
end up in a global video mode database.

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 Mon, Jul 11, 2022 at 01:59:28PM +0200, Geert Uytterhoeven wrote:

Hi Maxime,

On Mon, Jul 11, 2022 at 1:42 PM Maxime Ripard [off-list ref] wrote:

quoted

On Mon, Jul 11, 2022 at 01:11:14PM +0200, Thomas Zimmermann wrote:

quoted

Am 11.07.22 um 11:35 schrieb Maxime Ripard:

quoted

On Mon, Jul 11, 2022 at 11:03:38AM +0200, Thomas Zimmermann wrote:

quoted

Am 08.07.22 um 20:21 schrieb Geert Uytterhoeven:

quoted

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.

I've looked for code that uses these names, couldn't find any. How is this
being used in practice? For example, if I say "PAL" on the command line, is
there DRM code that fills in the PAL mode parameters?

We have some code to deal with this in sun4i:
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/sun4i/sun4i_tv.c#L292

It's a bit off topic, but for TV standards, I'm still not sure what the
best course of action is. There's several interactions that make this a
bit troublesome:

   * Some TV standards differ by their mode (ie, PAL vs NSTC), but some
     other differ by parameters that are not part of drm_display_mode
     (NTSC vs NSTC-J where the only difference is the black and blanking
     signal levels for example).

   * The mode names allow to provide a fairly convenient way to add that
     extra information, but the userspace is free to create its own mode
     and might omit the mode name entirely.

So in the code above, if the name has been preserved we match by name,
but we fall back to matching by mode if it hasn't been, which in this
case means that we have no way to differentiate between NTSC, NTSC-J,
PAL-M in this case.

We have some patches downstream for the RaspberryPi that has the TV
standard as a property. There's a few extra logic required for the
userspace (like setting the PAL property, with the NTSC mode) so I'm not
sure it's preferable.

Or we could do something like a property to try that standard, and
another that reports the one we actually chose.

quoted

And another question I have is whether this whitelist belongs into the
driver at all. Standard modes exist independent from drivers or hardware.
Shouldn't there simply be a global list of all possible mode names? Drivers
would filter out the unsupported modes anyway.

We should totally do something like that, yeah

That sun code already looks like sometihng the DRM core/helpers should be
doing. And if we want to support named modes well, there's a long list of
modes in Wikipedia.

https://en.wikipedia.org/wiki/Video_Graphics_Array#/media/File:Vector_Video_Standards2.svg

Yeah, and NTSC is missing :)

And that diagram is about the "digital" variant of PAL.
If you go the analog route, the only fixed parts are vfreq/hfreq,
number of lines, and synchronization. Other parameters like overscan
can vary.  The actual dot clock can vary wildly: while there is an
upper limit due to bandwidth limitations, you can come up with an
almost infinite number of video modes that can be called PAL, which
is one of the reasons why I don't want hardware-specific variants to
end up in a global video mode database.

Do you have an example of what that would look like?

Maxime

Hi Maxime,

On Mon, Jul 11, 2022 at 2:02 PM Maxime Ripard [off-list ref] wrote:

On Mon, Jul 11, 2022 at 01:59:28PM +0200, Geert Uytterhoeven wrote:

quoted

On Mon, Jul 11, 2022 at 1:42 PM Maxime Ripard [off-list ref] wrote:

quoted

On Mon, Jul 11, 2022 at 01:11:14PM +0200, Thomas Zimmermann wrote:

quoted

Am 11.07.22 um 11:35 schrieb Maxime Ripard:

quoted

On Mon, Jul 11, 2022 at 11:03:38AM +0200, Thomas Zimmermann wrote:

quoted

Am 08.07.22 um 20:21 schrieb Geert Uytterhoeven:

quoted

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.

I've looked for code that uses these names, couldn't find any. How is this
being used in practice? For example, if I say "PAL" on the command line, is
there DRM code that fills in the PAL mode parameters?

We have some code to deal with this in sun4i:
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/sun4i/sun4i_tv.c#L292

It's a bit off topic, but for TV standards, I'm still not sure what the
best course of action is. There's several interactions that make this a
bit troublesome:

   * Some TV standards differ by their mode (ie, PAL vs NSTC), but some
     other differ by parameters that are not part of drm_display_mode
     (NTSC vs NSTC-J where the only difference is the black and blanking
     signal levels for example).

   * The mode names allow to provide a fairly convenient way to add that
     extra information, but the userspace is free to create its own mode
     and might omit the mode name entirely.

So in the code above, if the name has been preserved we match by name,
but we fall back to matching by mode if it hasn't been, which in this
case means that we have no way to differentiate between NTSC, NTSC-J,
PAL-M in this case.

We have some patches downstream for the RaspberryPi that has the TV
standard as a property. There's a few extra logic required for the
userspace (like setting the PAL property, with the NTSC mode) so I'm not
sure it's preferable.

Or we could do something like a property to try that standard, and
another that reports the one we actually chose.

quoted

And another question I have is whether this whitelist belongs into the
driver at all. Standard modes exist independent from drivers or hardware.
Shouldn't there simply be a global list of all possible mode names? Drivers
would filter out the unsupported modes anyway.

We should totally do something like that, yeah

That sun code already looks like sometihng the DRM core/helpers should be
doing. And if we want to support named modes well, there's a long list of
modes in Wikipedia.

https://en.wikipedia.org/wiki/Video_Graphics_Array#/media/File:Vector_Video_Standards2.svg

Yeah, and NTSC is missing :)

And that diagram is about the "digital" variant of PAL.
If you go the analog route, the only fixed parts are vfreq/hfreq,
number of lines, and synchronization. Other parameters like overscan
can vary.  The actual dot clock can vary wildly: while there is an
upper limit due to bandwidth limitations, you can come up with an
almost infinite number of video modes that can be called PAL, which
is one of the reasons why I don't want hardware-specific variants to
end up in a global video mode database.

Do you have an example of what that would look like?

You mean a PAL mode that does not use 768x576?

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/video/fbdev/amifb.c#n834
(TAG_HIRES is replaced by the actual dot clock at runtime, as it
 depends on the crystal present on the mainboard).
Amifb also supports 320x256, by doubling the dot clock, but that mode
is not part of the database.

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

Hi Geert,

On Mon, Jul 11, 2022 at 02:08:06PM +0200, Geert Uytterhoeven wrote:

On Mon, Jul 11, 2022 at 2:02 PM Maxime Ripard [off-list ref] wrote:

quoted

On Mon, Jul 11, 2022 at 01:59:28PM +0200, Geert Uytterhoeven wrote:

quoted

On Mon, Jul 11, 2022 at 1:42 PM Maxime Ripard [off-list ref] wrote:

quoted

On Mon, Jul 11, 2022 at 01:11:14PM +0200, Thomas Zimmermann wrote:

quoted

Am 11.07.22 um 11:35 schrieb Maxime Ripard:

quoted

On Mon, Jul 11, 2022 at 11:03:38AM +0200, Thomas Zimmermann wrote:

quoted

Am 08.07.22 um 20:21 schrieb Geert Uytterhoeven:

quoted

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.

I've looked for code that uses these names, couldn't find any. How is this
being used in practice? For example, if I say "PAL" on the command line, is
there DRM code that fills in the PAL mode parameters?

We have some code to deal with this in sun4i:
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/sun4i/sun4i_tv.c#L292

It's a bit off topic, but for TV standards, I'm still not sure what the
best course of action is. There's several interactions that make this a
bit troublesome:

   * Some TV standards differ by their mode (ie, PAL vs NSTC), but some
     other differ by parameters that are not part of drm_display_mode
     (NTSC vs NSTC-J where the only difference is the black and blanking
     signal levels for example).

   * The mode names allow to provide a fairly convenient way to add that
     extra information, but the userspace is free to create its own mode
     and might omit the mode name entirely.

So in the code above, if the name has been preserved we match by name,
but we fall back to matching by mode if it hasn't been, which in this
case means that we have no way to differentiate between NTSC, NTSC-J,
PAL-M in this case.

We have some patches downstream for the RaspberryPi that has the TV
standard as a property. There's a few extra logic required for the
userspace (like setting the PAL property, with the NTSC mode) so I'm not
sure it's preferable.

Or we could do something like a property to try that standard, and
another that reports the one we actually chose.

quoted

And another question I have is whether this whitelist belongs into the
driver at all. Standard modes exist independent from drivers or hardware.
Shouldn't there simply be a global list of all possible mode names? Drivers
would filter out the unsupported modes anyway.

We should totally do something like that, yeah

That sun code already looks like sometihng the DRM core/helpers should be
doing. And if we want to support named modes well, there's a long list of
modes in Wikipedia.

https://en.wikipedia.org/wiki/Video_Graphics_Array#/media/File:Vector_Video_Standards2.svg

Yeah, and NTSC is missing :)

And that diagram is about the "digital" variant of PAL.
If you go the analog route, the only fixed parts are vfreq/hfreq,
number of lines, and synchronization. Other parameters like overscan
can vary.  The actual dot clock can vary wildly: while there is an
upper limit due to bandwidth limitations, you can come up with an
almost infinite number of video modes that can be called PAL, which
is one of the reasons why I don't want hardware-specific variants to
end up in a global video mode database.

Do you have an example of what that would look like?

You mean a PAL mode that does not use 768x576?

I meant what the almost infinite number of video modes that can be
called PAL and would have to be defined in drivers

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/video/fbdev/amifb.c#n834

But that works :)

I don't see what really is troublesome if we go with the mode + property
setup here.

We can deal easily with the interlaced vs non-interlaced variants
already with DRM_MODE_FLAG_INTERLACE, and the ff variants can be dealt
with DRM_MODE_FLAG_DBLCLK.

We still need something to differentiate between, say, PAL-M and NTSC-J
where the differences are between things not exposed by the mode itself
(black and blanking levels differ from NSTC for NTSC-J, and the color
carrier frequency is PAL's for PAL-M)

Am I missing something?

(TAG_HIRES is replaced by the actual dot clock at runtime, as it
 depends on the crystal present on the mainboard).

If we have the crystal frequency in the kernel somehow, we could filter
them out from the driver (or fill them in) depending on that frequency.

I still think the mode + property is the way to go, possibly with some
generic component that would take the mode name from the command line
and create that initial state depending on the value for backward
compatibility.

What do you think?

Maxime

Hi Maxime,

On Wed, Jul 13, 2022 at 11:37 AM Maxime Ripard [off-list ref] wrote:

On Mon, Jul 11, 2022 at 02:08:06PM +0200, Geert Uytterhoeven wrote:

quoted

On Mon, Jul 11, 2022 at 2:02 PM Maxime Ripard [off-list ref] wrote:

quoted

On Mon, Jul 11, 2022 at 01:59:28PM +0200, Geert Uytterhoeven wrote:

quoted

On Mon, Jul 11, 2022 at 1:42 PM Maxime Ripard [off-list ref] wrote:

quoted

On Mon, Jul 11, 2022 at 01:11:14PM +0200, Thomas Zimmermann wrote:

quoted

Am 11.07.22 um 11:35 schrieb Maxime Ripard:

quoted

On Mon, Jul 11, 2022 at 11:03:38AM +0200, Thomas Zimmermann wrote:

quoted

Am 08.07.22 um 20:21 schrieb Geert Uytterhoeven:

quoted

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.

I've looked for code that uses these names, couldn't find any. How is this
being used in practice? For example, if I say "PAL" on the command line, is
there DRM code that fills in the PAL mode parameters?

We have some code to deal with this in sun4i:
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/sun4i/sun4i_tv.c#L292

It's a bit off topic, but for TV standards, I'm still not sure what the
best course of action is. There's several interactions that make this a
bit troublesome:

   * Some TV standards differ by their mode (ie, PAL vs NSTC), but some
     other differ by parameters that are not part of drm_display_mode
     (NTSC vs NSTC-J where the only difference is the black and blanking
     signal levels for example).

   * The mode names allow to provide a fairly convenient way to add that
     extra information, but the userspace is free to create its own mode
     and might omit the mode name entirely.

So in the code above, if the name has been preserved we match by name,
but we fall back to matching by mode if it hasn't been, which in this
case means that we have no way to differentiate between NTSC, NTSC-J,
PAL-M in this case.

We have some patches downstream for the RaspberryPi that has the TV
standard as a property. There's a few extra logic required for the
userspace (like setting the PAL property, with the NTSC mode) so I'm not
sure it's preferable.

Or we could do something like a property to try that standard, and
another that reports the one we actually chose.

quoted

And another question I have is whether this whitelist belongs into the
driver at all. Standard modes exist independent from drivers or hardware.
Shouldn't there simply be a global list of all possible mode names? Drivers
would filter out the unsupported modes anyway.

We should totally do something like that, yeah

That sun code already looks like sometihng the DRM core/helpers should be
doing. And if we want to support named modes well, there's a long list of
modes in Wikipedia.

https://en.wikipedia.org/wiki/Video_Graphics_Array#/media/File:Vector_Video_Standards2.svg

Yeah, and NTSC is missing :)

And that diagram is about the "digital" variant of PAL.
If you go the analog route, the only fixed parts are vfreq/hfreq,
number of lines, and synchronization. Other parameters like overscan
can vary.  The actual dot clock can vary wildly: while there is an
upper limit due to bandwidth limitations, you can come up with an
almost infinite number of video modes that can be called PAL, which
is one of the reasons why I don't want hardware-specific variants to
end up in a global video mode database.

Do you have an example of what that would look like?

You mean a PAL mode that does not use 768x576?

I meant what the almost infinite number of video modes that can be
called PAL and would have to be defined in drivers

quoted

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/video/fbdev/amifb.c#n834

But that works :)

I don't see what really is troublesome if we go with the mode + property
setup here.

We can deal easily with the interlaced vs non-interlaced variants
already with DRM_MODE_FLAG_INTERLACE, and the ff variants can be dealt
with DRM_MODE_FLAG_DBLCLK.

Sure. Interlace and doublescan are the easy parts.
(actually "ff" is not PAL, but a 31 kHz mode with the same resolution of
 the corresponding PAL mode).

We still need something to differentiate between, say, PAL-M and NTSC-J
where the differences are between things not exposed by the mode itself
(black and blanking levels differ from NSTC for NTSC-J, and the color
carrier frequency is PAL's for PAL-M)

Am I missing something?

quoted

(TAG_HIRES is replaced by the actual dot clock at runtime, as it
 depends on the crystal present on the mainboard).

If we have the crystal frequency in the kernel somehow, we could filter
them out from the driver (or fill them in) depending on that frequency.

I still think the mode + property is the way to go, possibly with some
generic component that would take the mode name from the command line
and create that initial state depending on the value for backward
compatibility.

What do you think?

The difficulty is the wild variety of resolutions supported by devices
that can be connected to a standard (legacy) analog PAL TV or monitor,
and thus are all called "PAL".  These range from 160x228 (Atari 2600)
over 176x184 (VIC-20), 256x192 (e.g. ZX Spectrum), 320x200 (Atari ST),
640x256/512i (Amiga) (I'm not saying we should support old 8-bit
machines, though ;-)
A longer list can be found at [1].  Most of the resolutions lower
than 0.3 Mpixels can be shown on a TV.

IMHO, only the modes backed by digital standards of PAL (and NTSC [2])
should be in a common mode database.  The rest is to be detained
to the individual drivers, as they are highly driver-specific, and
unlikely to be used with more than one driver or hardware platform.

[1] https://en.wikipedia.org/wiki/List_of_common_resolutions
[2] https://en.wikipedia.org/wiki/List_of_common_resolutions#Digital_Standards

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