Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / Atom feed
From: Maxime Ripard <maxime.ripard@bootlin.com>
To: Thierry Reding <thierry.reding@gmail.com>
Cc: eben@raspberrypi.org, David Airlie <airlied@linux.ie>,
	Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
	dri-devel@lists.freedesktop.org,
	Paul Kocialkowski <paul.kocialkowski@bootlin.com>,
	Sean Paul <seanpaul@chromium.org>,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
	Daniel Vetter <daniel.vetter@intel.com>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v5 06/12] drm/modes: Support modes names on the command line
Date: Fri, 28 Jun 2019 15:43:54 +0200
Message-ID: <20190628134354.trogpmqedr5h3fvl@flea> (raw)
In-Reply-To: <20190626152659.GA11481@ulmo>

[-- 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

  reply index

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-17 14:51 [PATCH v5 00/12] drm/vc4: Allow for more boot-time configuration Maxime Ripard
2019-06-17 14:51 ` [PATCH v5 01/12] drm/connector: Add documentation for drm_cmdline_mode Maxime Ripard
2019-06-17 14:51 ` [PATCH v5 02/12] drm/client: Restrict the plane_state scope Maxime Ripard
2019-06-17 14:51 ` [PATCH v5 03/12] drm/client: Restrict the rotation check to the rotation itself Maxime Ripard
2019-06-17 14:51 ` [PATCH v5 04/12] drm/client: Change drm_client_panel_rotation name Maxime Ripard
2019-06-17 14:51 ` [PATCH v5 05/12] drm/modes: Rewrite the command line parser Maxime Ripard
2019-07-05 16:54   ` Dmitry Osipenko
2019-07-09 12:45     ` Maxime Ripard
2019-07-09 12:52       ` Dmitry Osipenko
2019-07-09 13:26         ` Jon Hunter
2019-07-09 13:28           ` Jon Hunter
2019-07-09 13:40             ` Dmitry Osipenko
2019-08-19 18:54   ` [v5,05/12] " Jernej Škrabec
2019-08-19 19:20     ` Thomas Graichen
2019-08-20 15:00       ` Maxime Ripard
2019-06-17 14:51 ` [PATCH v5 06/12] drm/modes: Support modes names on the command line Maxime Ripard
2019-06-26 15:26   ` Thierry Reding
2019-06-28 13:43     ` Maxime Ripard [this message]
2019-06-17 14:51 ` [PATCH v5 07/12] drm/modes: Allow to specify rotation and reflection on the commandline Maxime Ripard
2019-06-18 17:47   ` Noralf Trønnes
2019-06-19 10:16     ` Maxime Ripard
2019-06-17 14:51 ` [PATCH v5 08/12] drm/connector: Introduce a TV margins structure Maxime Ripard
2019-06-17 14:51 ` [PATCH v5 09/12] drm/modes: Parse overscan properties Maxime Ripard
2019-06-17 14:51 ` [PATCH v5 10/12] drm/atomic: Add a function to reset connector TV properties Maxime Ripard
2019-06-17 14:51 ` [PATCH v5 11/12] drm/selftests: Add command line parser selftests Maxime Ripard
2019-06-17 14:51 ` [PATCH v5 12/12] drm/vc4: hdmi: Set default state margin at reset Maxime Ripard
2019-06-17 18:06   ` Eric Anholt

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190628134354.trogpmqedr5h3fvl@flea \
    --to=maxime.ripard@bootlin.com \
    --cc=airlied@linux.ie \
    --cc=daniel.vetter@intel.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=eben@raspberrypi.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=maarten.lankhorst@linux.intel.com \
    --cc=paul.kocialkowski@bootlin.com \
    --cc=seanpaul@chromium.org \
    --cc=thierry.reding@gmail.com \
    --cc=thomas.petazzoni@bootlin.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-ARM-Kernel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/0 linux-arm-kernel/git/0.git
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/1 linux-arm-kernel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-kernel linux-arm-kernel/ https://lore.kernel.org/linux-arm-kernel \
		linux-arm-kernel@lists.infradead.org infradead-linux-arm-kernel@archiver.kernel.org
	public-inbox-index linux-arm-kernel


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-arm-kernel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox