Re: [PATCH 4/5] drm/modes: Add support for driver-specific named modes

[Geert Uytterhoeven]

Hi Maxime,

On Wed, Jul 13, 2022 at 11:37 AM Maxime Ripard <maxime@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxxxxxx

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