| Message ID | 68923c8a129b6c2a70b570103679a1cf7876bbc2.1657301107.git.geert@linux-m68k.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drm/modes: Command line mode selection fixes and improvements | expand |
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 > > 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)
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: > > 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 <tzimmermann@suse.de> wrote: > 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? 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 <tzimmermann@suse.de> wrote: >> 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? > > 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? > >> 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
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: >> 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? > > 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 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
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: > > 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: > > > > 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 > > 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 <maxime@cerno.tech> wrote: > On Mon, Jul 11, 2022 at 01:11:14PM +0200, Thomas Zimmermann wrote: > > Am 11.07.22 um 11:35 schrieb Maxime Ripard: > > > On Mon, Jul 11, 2022 at 11:03:38AM +0200, Thomas Zimmermann wrote: > > > > 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? > > > > > > 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 > > > > 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 <maxime@cerno.tech> wrote: > > On Mon, Jul 11, 2022 at 01:11:14PM +0200, Thomas Zimmermann wrote: > > > Am 11.07.22 um 11:35 schrieb Maxime Ripard: > > > > On Mon, Jul 11, 2022 at 11:03:38AM +0200, Thomas Zimmermann wrote: > > > > > 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? > > > > > > > > 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 > > > > > > 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 <maxime@cerno.tech> wrote: > On Mon, Jul 11, 2022 at 01:59:28PM +0200, Geert Uytterhoeven wrote: > > On Mon, Jul 11, 2022 at 1:42 PM Maxime Ripard <maxime@cerno.tech> wrote: > > > On Mon, Jul 11, 2022 at 01:11:14PM +0200, Thomas Zimmermann wrote: > > > > Am 11.07.22 um 11:35 schrieb Maxime Ripard: > > > > > On Mon, Jul 11, 2022 at 11:03:38AM +0200, Thomas Zimmermann wrote: > > > > > > 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? > > > > > > > > > > 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 > > > > > > > > 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 <maxime@cerno.tech> wrote: > > On Mon, Jul 11, 2022 at 01:59:28PM +0200, Geert Uytterhoeven wrote: > > > On Mon, Jul 11, 2022 at 1:42 PM Maxime Ripard <maxime@cerno.tech> wrote: > > > > On Mon, Jul 11, 2022 at 01:11:14PM +0200, Thomas Zimmermann wrote: > > > > > Am 11.07.22 um 11:35 schrieb Maxime Ripard: > > > > > > On Mon, Jul 11, 2022 at 11:03:38AM +0200, Thomas Zimmermann wrote: > > > > > > > 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? > > > > > > > > > > > > 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 > > > > > > > > > > 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 <maxime@cerno.tech> wrote: > On Mon, Jul 11, 2022 at 02:08:06PM +0200, Geert Uytterhoeven wrote: > > On Mon, Jul 11, 2022 at 2:02 PM Maxime Ripard <maxime@cerno.tech> wrote: > > > On Mon, Jul 11, 2022 at 01:59:28PM +0200, Geert Uytterhoeven wrote: > > > > On Mon, Jul 11, 2022 at 1:42 PM Maxime Ripard <maxime@cerno.tech> wrote: > > > > > On Mon, Jul 11, 2022 at 01:11:14PM +0200, Thomas Zimmermann wrote: > > > > > > Am 11.07.22 um 11:35 schrieb Maxime Ripard: > > > > > > > On Mon, Jul 11, 2022 at 11:03:38AM +0200, Thomas Zimmermann wrote: > > > > > > > > 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? > > > > > > > > > > > > > > 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 > > > > > > > > > > > > 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. 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? > > > (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
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)
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(-)