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 > > > > 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 > > Signed-off-by: Maxime Ripard > > --- > > 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