Re: [PATCH v5 06/12] drm/modes: Support modes names on the command line

[Maxime Ripard <maxime.ripard@bootlin.com>]

[-- Attachment #1.1: Type: text/plain, Size: 6252 bytes --]

Hi Thierry,

On Wed, Jun 26, 2019 at 05:26:59PM +0200, Thierry Reding wrote:
> On Mon, Jun 17, 2019 at 04:51:33PM +0200, Maxime Ripard wrote:
> > From: Maxime Ripard <maxime.ripard@free-electrons.com>
> >
> > The drm subsystem also uses the video= kernel parameter, and in the
> > documentation refers to the fbdev documentation for that parameter.
> >
> > However, that documentation also says that instead of giving the mode using
> > its resolution we can also give a name. However, DRM doesn't handle that
> > case at the moment. Even though in most case it shouldn't make any
> > difference, it might be useful for analog modes, where different standards
> > might have the same resolution, but still have a few different parameters
> > that are not encoded in the modes (NTSC vs NTSC-J vs PAL-M for example).
> >
> > Reviewed-by: Noralf Trønnes <noralf@tronnes.org>
> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > ---
> >  drivers/gpu/drm/drm_client_modeset.c |  4 ++-
> >  drivers/gpu/drm/drm_connector.c      |  3 +-
> >  drivers/gpu/drm/drm_modes.c          | 62 +++++++++++++++++++++--------
> >  include/drm/drm_connector.h          |  7 +++-
> >  4 files changed, 59 insertions(+), 17 deletions(-)
>
> This patch causes an issue on various Tegra boards that have so far been
> running flawlessly. Here's an extract from the boot log:
>
> 	[    0.000000] Kernel command line: root=/dev/nfs rw netdevwait ip=:::::eth0:on nfsroot=192.168.23.1:/srv/nfs/tegra194 console=ttyTCU0,115200n8 console=tty0 fbcon=map:0 net.ifnames=0 rootfstype=ext4 video=tegrafb no_console_suspend=1 earlycon=tegra_comb_uart,mmio32,0x0c168000 gpt usbcore.old_scheme_first=1 tegraid=19.1.2.0.0 maxcpus=8 boot.slot_suffix= boot.ratchetvalues=0.2.2 vpr=0x8000000@0xf0000000 sdhci_tegra.en_boot_part_access=1
> 	...
> 	[   18.597001] [drm:drm_connector_init [drm]] cmdline mode for connector DP-1 tegrafb 0x0@60Hz
> 	...
> 	[   18.627145] [drm:drm_connector_init [drm]] cmdline mode for connector DP-2 tegrafb 0x0@60Hz
> 	...
> 	[   18.673770] [drm:drm_connector_init [drm]] cmdline mode for connector HDMI-A-1 tegrafb 0x0@60Hz
> 	...
> 	[   19.057500] [drm:drm_mode_debug_printmodeline [drm]] Modeline "0x0": 0 0 0 0 0 0 0 1 4 1 0x20 0x6
> 	[   19.066341] [drm:drm_mode_prune_invalid [drm]] Not using 0x0 mode: CLOCK_LOW
> 	...
> 	[   19.677803] [drm:drm_client_modeset_probe [drm]] looking for cmdline mode on connector 60
> 	[   19.686019] [drm:drm_client_modeset_probe [drm]] found mode 0x0
> 	...
> 	[   19.851843] drm drm: failed to set initial configuration: -28
>
> So basically what's happening here is that the bootloader is passing a
> video= parameter on the command-line and after this patch, the DRM core
> will consider the tegrafb in that parameter to be a named video mode.
> The mode is then filtered out because it doesn't make any sense, but
> then drm_client_modeset_probe() still tries to use it, eventually
> leading to failure because we can't allocate memory for a 0x0
> framebuffer.

What was the behaviour before? That it wouldn't set a mode at all?

> Now, there are obviously a couple of places where things go wrong. On
> one hand I think if the mode specified on the command-line is already
> filtered out, then drm_client_modeset_probe() should not be trying to
> use it.

Yeah, that would make sense

> One could also argue that the bootloader shouldn't be passing that
> video=tegrafb parameter in the first place. Then again, this is nothing
> out of the ordinary (as documented in Documentation/fb/modedb.rst).

I've read that documentation, and I'm not sure which section in there
allows to do that?

> The problem with named modes, and you already highlighted this in your
> comment in the code, is that it's not possible to distinguish between a
> mode name and a video= option that defines the framebuffer device to
> use.
>
> That said, I wouldn't be surprised if this change ended up breaking on
> other devices. I'm also not sure that under these circumstances it's a
> good idea to support named modes. At least not until we have a better
> way of determining what's a real mode name and what isn't. Looking at
> the old modedb from fb, not even the standard modes listed there have
> names associated with them, so I'm not sure how this was ever supposed
> to work. From the looks of it, some of the fbdev drivers seem to take a
> mode list from board-code (see for example the mx21ads_modes array from
> arch/arm/mach-imx/mach-mx21ads.c). The imxfb driver can then take a mode
> name from the command line and try to match it against a list of known
> modes. That seems to match what the documentation says.
>
> However, that also really only works because this is all directly dealt
> with in the fbdev drivers. For DRM/KMS we don't do that and instead we
> rely on the core to provide this backwards-compatibility. However, at
> the time when we parse the mode from the command line we don't have the
> list of modes that are considered to be valid, so your patch currently
> needs to assume that it is a valid mode. I don't think that's a good
> idea, because clearly not all strings that currently make it through the
> filter are actually modes.

We have a list of named modes in the connector, and we match against
that. It already works for sunxi (minus the bugs..).

> So if we really need this, I think we want some way for the connector to
> provide the list of named modes that it supports so that by the time we
> want to parse the command-line we can check whether it's actually a name
> to avoid false positives like the ones I'm seeing on Tegra.

I guess that could work yep

> For now it might just be easiest to avoid any of this and disable the
> named mode support until it's a bit more mature. The patch no longer
> reverts cleanly, but it should be fairly easy to disable the feature in
> a follow-up patch again.

If we were to do that, I'd really like to have least get a unit test
for that case, and some documentation on how we're supposed to deal
with this case.

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel