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(-)
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",
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
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;
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
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)
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
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
dri-devel@lists.freedesktop.org
-
Javier Martinez Canillas -
Maxime Ripard -
Thomas Zimmermann