[PATCH 0/3] drm/client: Fix display-mode selection

[PATCH 0/3] drm/client: Fix display-mode selection

[Thomas Zimmermann]

Pick user-defined display mode in DRM clients if the mode has been
validated by the driver. Otherwise pick a preferred display mode.

Booting the kernel with video=<mode> and giving an unsupported display
mode can easily turn the display unusable. This is best tested by
booting simpledrm with a display mode that does not use the firmware
framebuffer's resolution. While simpledrm filter's out the mode as
invalid, the DRM client still picks it and the console won't show up.

Several factors contribute to this problem.

 * The connector invalidates the user-defined display mode, but never
   tells the user about it.
 * The DRM client doesn't look for user-defined display modes, but for
   modes that are similar.
 * If no similar mode can be found, the client adds the invalid display
   mode back to the connector's mode list for use.

Each of the patches in this patchset addresses one of these problems.
Overall the DRM client has no business in display-mode detection and
should only pick one of the modes that has been detected and validated 
by the connector.

Thomas Zimmermann (3):
  drm: Always warn if user-defined modes are not supported
  drm/client: Look for command-line modes first
  drm/client: Don't add new command-line mode

 drivers/gpu/drm/drm_client_modeset.c | 28 ++++++++++++++++------------
 drivers/gpu/drm/drm_modes.c          |  4 ++++
 2 files changed, 20 insertions(+), 12 deletions(-)

-- 
2.36.0

[PATCH 1/3] drm: Always warn if user-defined modes are not supported

[Thomas Zimmermann]

Print a warning if a user-specifed display mode is not supported by
the display pipeline. Users specified the display mode on the kernel
command line with the use of the video= parameter. Setting an
unsupported mode will leave the console blank, so we should at least
let the user know why.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_modes.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 14b746f7ba97..40b7b245e98c 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1328,6 +1328,10 @@ void drm_mode_prune_invalid(struct drm_device *dev,
 	list_for_each_entry_safe(mode, t, mode_list, head) {
 		if (mode->status != MODE_OK) {
 			list_del(&mode->head);
+			if (mode->type & DRM_MODE_TYPE_USERDEF) {
+				drm_warn(dev, "User-defined mode not supported: "
+					 DRM_MODE_FMT "\n", DRM_MODE_ARG(mode));
+			}
 			if (verbose) {
 				drm_mode_debug_printmodeline(mode);
 				DRM_DEBUG_KMS("Not using %s mode: %s\n",
-- 
2.36.0

Re: [PATCH 1/3] drm: Always warn if user-defined modes are not supported

[Javier Martinez Canillas]

Hello Thomas,

On 5/11/22 20:31, Thomas Zimmermann wrote:
> Print a warning if a user-specifed display mode is not supported by
> the display pipeline. Users specified the display mode on the kernel
> command line with the use of the video= parameter. Setting an
> unsupported mode will leave the console blank, so we should at least
> let the user know why.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/drm_modes.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index 14b746f7ba97..40b7b245e98c 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -1328,6 +1328,10 @@ void drm_mode_prune_invalid(struct drm_device *dev,
>  	list_for_each_entry_safe(mode, t, mode_list, head) {
>  		if (mode->status != MODE_OK) {
>  			list_del(&mode->head);
> +			if (mode->type & DRM_MODE_TYPE_USERDEF) {
> +				drm_warn(dev, "User-defined mode not supported: "
> +					 DRM_MODE_FMT "\n", DRM_MODE_ARG(mode));

I wonder if should be more explicit here like "... and it will not be used".

> +			}
>  			if (verbose) {
>  				drm_mode_debug_printmodeline(mode);
>  				DRM_DEBUG_KMS("Not using %s mode: %s\n",

Does it make sense to keep this line when verbose is set if there's a warn
now. Maybe just keep the drm_mode_debug_printmode() but remove the rest ?

I think the patch is good as is too, so regardless you do:

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

[PATCH 2/3] drm/client: Look for command-line modes first

[Thomas Zimmermann]

When picking a mode, first look for modes that have been specified
by the user on the kernel's command line. Only if that fails, use
the existing heuristic of picking a nearby mode from it's various
parameters.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_client_modeset.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
index e6346a67cd98..b777faa87f04 100644
--- a/drivers/gpu/drm/drm_client_modeset.c
+++ b/drivers/gpu/drm/drm_client_modeset.c
@@ -165,6 +165,17 @@ drm_connector_pick_cmdline_mode(struct drm_connector *connector)
 	struct drm_display_mode *mode;
 	bool prefer_non_interlace;
 
+	/*
+	 * Find a user-defined mode. If the user gave us a valid
+	 * mode on the kernel command line, it will show up in this
+	 * list.
+	 */
+
+	list_for_each_entry(mode, &connector->modes, head) {
+		if (mode->type & DRM_MODE_TYPE_USERDEF)
+			return mode;
+	}
+
 	cmdline_mode = &connector->cmdline_mode;
 	if (cmdline_mode->specified == false)
 		return NULL;
-- 
2.36.0

Re: [PATCH 2/3] drm/client: Look for command-line modes first

[Javier Martinez Canillas]

On 5/11/22 20:31, Thomas Zimmermann wrote:
> When picking a mode, first look for modes that have been specified
> by the user on the kernel's command line. Only if that fails, use
> the existing heuristic of picking a nearby mode from it's various
> parameters.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

[PATCH 3/3] drm/client: Don't add new command-line mode

[Thomas Zimmermann]

Don't add a mode for the kernel's command-line parameters from
within the DRM client code. Doing so can result in an unusable
display. If there's no compatible command-line mode, the client
will use one of the connector's preferred modes.

All mode creation and validation has to be performed by the
connector. When clients run, the connector's fill_modes callback
has already processes the kernel parameters and validates each
mode before adding it. The connector's mode list does not contain
invalid modes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_client_modeset.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
index b777faa87f04..48e6ce16439f 100644
--- a/drivers/gpu/drm/drm_client_modeset.c
+++ b/drivers/gpu/drm/drm_client_modeset.c
@@ -158,8 +158,7 @@ drm_connector_has_preferred_mode(struct drm_connector *connector, int width, int
 	return NULL;
 }
 
-static struct drm_display_mode *
-drm_connector_pick_cmdline_mode(struct drm_connector *connector)
+static struct drm_display_mode *drm_connector_pick_cmdline_mode(struct drm_connector *connector)
 {
 	struct drm_cmdline_mode *cmdline_mode;
 	struct drm_display_mode *mode;
@@ -180,11 +179,10 @@ drm_connector_pick_cmdline_mode(struct drm_connector *connector)
 	if (cmdline_mode->specified == false)
 		return NULL;
 
-	/* attempt to find a matching mode in the list of modes
-	 *  we have gotten so far, if not add a CVT mode that conforms
+	/*
+	 * Attempt to find a matching mode in the list of modes we
+	 * have gotten so far.
 	 */
-	if (cmdline_mode->rb || cmdline_mode->margins)
-		goto create_mode;
 
 	prefer_non_interlace = !cmdline_mode->interlace;
 again:
@@ -218,12 +216,7 @@ drm_connector_pick_cmdline_mode(struct drm_connector *connector)
 		goto again;
 	}
 
-create_mode:
-	mode = drm_mode_create_from_cmdline_mode(connector->dev, cmdline_mode);
-	if (mode)
-		list_add(&mode->head, &connector->modes);
-
-	return mode;
+	return NULL;
 }
 
 static bool drm_connector_enabled(struct drm_connector *connector, bool strict)
-- 
2.36.0

Re: [PATCH 3/3] drm/client: Don't add new command-line mode

[Javier Martinez Canillas]

On 5/11/22 20:31, Thomas Zimmermann wrote:
> Don't add a mode for the kernel's command-line parameters from
> within the DRM client code. Doing so can result in an unusable
> display. If there's no compatible command-line mode, the client
> will use one of the connector's preferred modes.
> 
> All mode creation and validation has to be performed by the
> connector. When clients run, the connector's fill_modes callback
> has already processes the kernel parameters and validates each

s/process/processed and s/validates/validated ?

or 

remove the "has" in the sentence, either work I think.

> mode before adding it. The connector's mode list does not contain
> invalid modes.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/drm_client_modeset.c | 17 +++++------------
>  1 file changed, 5 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
> index b777faa87f04..48e6ce16439f 100644
> --- a/drivers/gpu/drm/drm_client_modeset.c
> +++ b/drivers/gpu/drm/drm_client_modeset.c
> @@ -158,8 +158,7 @@ drm_connector_has_preferred_mode(struct drm_connector *connector, int width, int
>  	return NULL;
>  }
>  
> -static struct drm_display_mode *
> -drm_connector_pick_cmdline_mode(struct drm_connector *connector)
> +static struct drm_display_mode *drm_connector_pick_cmdline_mode(struct drm_connector *connector)

This seems like an unrelated cleanup.

The rest looks good to me.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 0/3] drm/client: Fix display-mode selection

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1310 bytes --]

On Wed, May 11, 2022 at 08:31:22PM +0200, Thomas Zimmermann wrote:
> Pick user-defined display mode in DRM clients if the mode has been
> validated by the driver. Otherwise pick a preferred display mode.
> 
> Booting the kernel with video=<mode> and giving an unsupported display
> mode can easily turn the display unusable. This is best tested by
> booting simpledrm with a display mode that does not use the firmware
> framebuffer's resolution. While simpledrm filter's out the mode as
> invalid, the DRM client still picks it and the console won't show up.
> 
> Several factors contribute to this problem.
> 
>  * The connector invalidates the user-defined display mode, but never
>    tells the user about it.
>  * The DRM client doesn't look for user-defined display modes, but for
>    modes that are similar.
>  * If no similar mode can be found, the client adds the invalid display
>    mode back to the connector's mode list for use.
> 
> Each of the patches in this patchset addresses one of these problems.
> Overall the DRM client has no business in display-mode detection and
> should only pick one of the modes that has been detected and validated 
> by the connector.

That's awesome, thanks!

For the series,
Reviewed-by: Maxime Ripard <maxime@cerno.tech>

Maxime

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