[PATCH 0/2] drm/client: Keep larger modes in connector->modes on fbdev hotplug

[PATCH 0/2] drm/client: Keep larger modes in connector->modes on fbdev hotplug

[Hugo Sales]

On an ASUS ROG Flow X13 with a 3840x2400 eDP panel, booted with
video=eDP-1:1920x1200@60, an HDMI hotplug makes fbdev reprobe every
connector with its 1920x1200 framebuffer as the limit. connector->modes
loses the panel's 3840x2400 mode, and after suspend/resume amdgpu drives
the panel with the 1920x1200 timing. The image ends up squeezed into the
top-left corner. Details are in
https://gitlab.freedesktop.org/drm/amd/-/issues/5983

Patch 1 makes drm_client_modeset_probe() probe with the device's limits
and moves the framebuffer size check into the fbdev mode pickers.
Patch 2 adds KUnit tests for it

fbdev's hotplug probe has passed the framebuffer size since 2010
(4abe35204af8). The pickers didn't check mode sizes then, so leaving
larger modes out of the list was what kept fbdev from picking one. They
check the size themselves now, so the probe can use the device's limits
like initial setup, drm_log and GETCONNECTOR already do

The HiBMC thread "[RFC] drm/client: Relax clone mode selection in
drm_client_target_cloned" runs into the same pruning: its 640x480
framebuffer removes 1024x768 from both connectors' lists. With this
series userspace sees that mode again, but fbdev still can't use it, so
this doesn't replace that RFC:
https://lore.kernel.org/r/a48e0b33-7eab-478e-a396-b6e76b7a066f@huawei.com

I sent an amdgpu fix for the same symptom separately. It makes amdgpu
fall back to the encoder's native mode on eDP and LVDS:
https://lore.kernel.org/r/20261014223041.171317-1-hugo@hsal.es
This series fixes the shared mode list for every driver instead. Either
one alone fixes the reported bug, from my testing

On amd-staging-drm-next with only this series applied, with an HDMI
hotplug and suspend/resume (pm_test=platform) in a loop, 255 of 255
resumes kept the native timing. Without it, 28 of 29 came back with the
wrong timing, and the same loop on unpatched v7.2.8 got it wrong in 103
of 103. The drm KUnit tests pass, also under KASAN, which reports
nothing in the code this series touches

Hugo Sales (2):
  drm/client: Probe connectors with the device's mode size limits
  drm/tests: Add KUnit tests for the client mode size limits

 drivers/gpu/drm/drm_client_modeset.c          |  57 +++--
 .../gpu/drm/tests/drm_client_modeset_test.c   | 222 +++++++++++++++++-
 2 files changed, 260 insertions(+), 19 deletions(-)

-- 
2.55.0

[PATCH 1/2] drm/client: Probe connectors with the device's mode size limits

[Hugo Sales]

Currently, drm_client_modeset_probe() passes the client's width and
height to ->fill_modes(), and the probe helper leaves every larger mode
out of connector->modes. drm_log, fbdev's initial setup and the
GETCONNECTOR ioctl already probe with the device's limits, but on a
hotplug fbdev passes its framebuffer size. After that, the list that
userspace and the driver share lacks every mode larger than the
framebuffer. That includes the panel's preferred mode when video= sets
a smaller one

Probe with the device's limits instead. Make the mode pickers skip
modes that don't fit width x height, using the drm_mode_validate_size()
check the probe helper applied before. The probe helper still runs
mode_valid on every mode, so only the size check moves

fbdev picks the same modes as before, with one exception. A DisplayPort
sink with no mode that fits used to end up with an empty list, so the
probe helper added its 640x480 fallback modes. Its list now keeps the
larger modes, and fbdev leaves that output off

Link: https://gitlab.freedesktop.org/drm/amd/-/issues/5983
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Hugo Sales <hugo@hsal.es>
---
 drivers/gpu/drm/drm_client_modeset.c          | 57 +++++++++++++------
 .../gpu/drm/tests/drm_client_modeset_test.c   |  4 +-
 2 files changed, 43 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
index 0080a8e9556a..e79c7257aaa2 100644
--- a/drivers/gpu/drm/drm_client_modeset.c
+++ b/drivers/gpu/drm/drm_client_modeset.c
@@ -121,11 +121,13 @@ drm_client_find_modeset(struct drm_client_dev *client, struct drm_crtc *crtc)
 }
 
 static const struct drm_display_mode *
-drm_connector_get_tiled_mode(struct drm_connector *connector)
+drm_connector_get_tiled_mode(struct drm_connector *connector, int width, int height)
 {
 	const struct drm_display_mode *mode;
 
 	list_for_each_entry(mode, &connector->modes, head) {
+		if (drm_mode_validate_size(mode, width, height) != MODE_OK)
+			continue;
 		if (mode->hdisplay == connector->tile_h_size &&
 		    mode->vdisplay == connector->tile_v_size)
 			return mode;
@@ -134,11 +136,13 @@ drm_connector_get_tiled_mode(struct drm_connector *connector)
 }
 
 static const struct drm_display_mode *
-drm_connector_fallback_non_tiled_mode(struct drm_connector *connector)
+drm_connector_fallback_non_tiled_mode(struct drm_connector *connector, int width, int height)
 {
 	const struct drm_display_mode *mode;
 
 	list_for_each_entry(mode, &connector->modes, head) {
+		if (drm_mode_validate_size(mode, width, height) != MODE_OK)
+			continue;
 		if (mode->hdisplay == connector->tile_h_size &&
 		    mode->vdisplay == connector->tile_v_size)
 			continue;
@@ -163,14 +167,19 @@ drm_connector_preferred_mode(struct drm_connector *connector, int width, int hei
 }
 
 static const struct drm_display_mode *
-drm_connector_first_mode(struct drm_connector *connector)
+drm_connector_first_mode(struct drm_connector *connector, int width, int height)
 {
-	return list_first_entry_or_null(&connector->modes,
-					struct drm_display_mode, head);
+	const struct drm_display_mode *mode;
+
+	list_for_each_entry(mode, &connector->modes, head) {
+		if (drm_mode_validate_size(mode, width, height) == MODE_OK)
+			return mode;
+	}
+	return NULL;
 }
 
 static const struct drm_display_mode *
-drm_connector_pick_cmdline_mode(struct drm_connector *connector)
+drm_connector_pick_cmdline_mode(struct drm_connector *connector, int width, int height)
 {
 	const struct drm_cmdline_mode *cmdline_mode;
 	const struct drm_display_mode *mode;
@@ -183,6 +192,8 @@ drm_connector_pick_cmdline_mode(struct drm_connector *connector)
 	 */
 
 	list_for_each_entry(mode, &connector->modes, head) {
+		if (drm_mode_validate_size(mode, width, height) != MODE_OK)
+			continue;
 		if (mode->type & DRM_MODE_TYPE_USERDEF)
 			return mode;
 	}
@@ -199,6 +210,8 @@ drm_connector_pick_cmdline_mode(struct drm_connector *connector)
 	prefer_non_interlace = !cmdline_mode->interlace;
 again:
 	list_for_each_entry(mode, &connector->modes, head) {
+		if (drm_mode_validate_size(mode, width, height) != MODE_OK)
+			continue;
 		/* check width/height */
 		if (mode->hdisplay != cmdline_mode->xres ||
 		    mode->vdisplay != cmdline_mode->yres)
@@ -321,7 +334,7 @@ static bool drm_client_target_cloned(struct drm_device *dev,
 			continue;
 
 		mode_replace(dev, &modes[i],
-			     drm_connector_pick_cmdline_mode(connectors[i]));
+			     drm_connector_pick_cmdline_mode(connectors[i], width, height));
 		if (!modes[i]) {
 			can_clone = false;
 			break;
@@ -357,6 +370,8 @@ static bool drm_client_target_cloned(struct drm_device *dev,
 			continue;
 
 		list_for_each_entry(mode, &connectors[i]->modes, head) {
+			if (drm_mode_validate_size(mode, width, height) != MODE_OK)
+				continue;
 			if (drm_mode_match(mode, dmt_mode,
 					   DRM_MODE_MATCH_TIMINGS |
 					   DRM_MODE_MATCH_CLOCK |
@@ -472,7 +487,7 @@ static bool drm_client_target_preferred(struct drm_device *dev,
 
 		mode_type = "cmdline";
 		mode_replace(dev, &modes[i],
-			     drm_connector_pick_cmdline_mode(connector));
+			     drm_connector_pick_cmdline_mode(connector, width, height));
 
 		if (!modes[i]) {
 			mode_type = "preferred";
@@ -483,7 +498,7 @@ static bool drm_client_target_preferred(struct drm_device *dev,
 		if (!modes[i]) {
 			mode_type = "first";
 			mode_replace(dev, &modes[i],
-				     drm_connector_first_mode(connector));
+				     drm_connector_first_mode(connector, width, height));
 		}
 
 		/*
@@ -499,14 +514,16 @@ static bool drm_client_target_preferred(struct drm_device *dev,
 			    connector->num_h_tile * connector->num_v_tile ||
 			    (connector->tile_h_loc == 0 &&
 			     connector->tile_v_loc == 0 &&
-			     !drm_connector_get_tiled_mode(connector))) {
+			     !drm_connector_get_tiled_mode(connector, width, height))) {
 				mode_type = "non tiled";
 				mode_replace(dev, &modes[i],
-					     drm_connector_fallback_non_tiled_mode(connector));
+					     drm_connector_fallback_non_tiled_mode(connector,
+										   width, height));
 			} else {
 				mode_type = "tiled";
 				mode_replace(dev, &modes[i],
-					     drm_connector_get_tiled_mode(connector));
+					     drm_connector_get_tiled_mode(connector,
+									  width, height));
 			}
 		}
 
@@ -721,7 +738,7 @@ static bool drm_client_firmware_config(struct drm_client_dev *client,
 
 		mode_type = "cmdline";
 		mode_replace(dev, &modes[i],
-			     drm_connector_pick_cmdline_mode(connector));
+			     drm_connector_pick_cmdline_mode(connector, width, height));
 
 		if (!modes[i]) {
 			mode_type = "preferred";
@@ -732,7 +749,7 @@ static bool drm_client_firmware_config(struct drm_client_dev *client,
 		if (!modes[i]) {
 			mode_type = "first";
 			mode_replace(dev, &modes[i],
-				     drm_connector_first_mode(connector));
+				     drm_connector_first_mode(connector, width, height));
 		}
 
 		/* last resort: use current mode */
@@ -750,7 +767,8 @@ static bool drm_client_firmware_config(struct drm_client_dev *client,
 		    num_tiled_conns < connector->num_h_tile * connector->num_v_tile) {
 			mode_type = "non tiled";
 			mode_replace(dev, &modes[i],
-				     drm_connector_fallback_non_tiled_mode(connector));
+				     drm_connector_fallback_non_tiled_mode(connector,
+									   width, height));
 		}
 		crtcs[i] = crtc;
 
@@ -865,8 +883,15 @@ int drm_client_modeset_probe(struct drm_client_dev *client, unsigned int width,
 	mutex_lock(&client->modeset_mutex);
 
 	mutex_lock(&dev->mode_config.mutex);
+	/*
+	 * Probe with the device limits, not width x height. connector->modes is
+	 * shared with userspace and the driver, so it must keep modes larger
+	 * than this client's size. The mode pickers skip those instead
+	 */
 	for (i = 0; i < connector_count; i++)
-		total_modes_count += connectors[i]->funcs->fill_modes(connectors[i], width, height);
+		total_modes_count += connectors[i]->funcs->fill_modes(connectors[i],
+								      dev->mode_config.max_width,
+								      dev->mode_config.max_height);
 	if (!total_modes_count)
 		drm_dbg_kms(dev, "No connectors reported connected with modes\n");
 	drm_client_connectors_enabled(connectors, connector_count, enabled);
diff --git a/drivers/gpu/drm/tests/drm_client_modeset_test.c b/drivers/gpu/drm/tests/drm_client_modeset_test.c
index ec58fe064d86..00f43fca9795 100644
--- a/drivers/gpu/drm/tests/drm_client_modeset_test.c
+++ b/drivers/gpu/drm/tests/drm_client_modeset_test.c
@@ -112,7 +112,7 @@ static void drm_test_pick_cmdline_res_1920_1080_60(struct kunit *test)
 	mutex_unlock(&drm->mode_config.mutex);
 	KUNIT_ASSERT_GT(test, ret, 0);
 
-	mode = drm_connector_pick_cmdline_mode(connector);
+	mode = drm_connector_pick_cmdline_mode(connector, 1920, 1080);
 	KUNIT_ASSERT_NOT_NULL(test, mode);
 
 	KUNIT_EXPECT_TRUE(test, drm_mode_equal(expected_mode, mode));
@@ -151,7 +151,7 @@ static void drm_test_pick_cmdline_named(struct kunit *test)
 	mutex_unlock(&drm->mode_config.mutex);
 	KUNIT_ASSERT_GT(test, ret, 0);
 
-	mode = drm_connector_pick_cmdline_mode(connector);
+	mode = drm_connector_pick_cmdline_mode(connector, 1920, 1080);
 	KUNIT_ASSERT_NOT_NULL(test, mode);
 
 	expected_mode = params->func(drm);
-- 
2.55.0

Re: [PATCH 1/2] drm/client: Probe connectors with the device's mode size limits

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm: NULL pointer dereference of modes[i] in drm_client_firmware_config
- [Medium] drm: Logical bug in drm_client_target_cloned due to stale modes[i] values
--

commit 0e7e38749489efdbc43f11da085113315e670013
Author: Hugo Sales <hugo@hsal.es>

drm/client: Probe connectors with the device's mode size limits

This patch updates drm_client_modeset_probe() to use the device's mode
size limits rather than the client's dimensions when probing connectors.
It shifts the size validation to the mode pickers by calling
drm_mode_validate_size() to ensure the shared modes list retains larger
modes. This allows keeping panels' preferred modes even if smaller modes
are initially selected.

Link: https://gitlab.freedesktop.org/drm/amd/-/issues/5983

> diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
> index 0080a8e9556a5..e79c7257aaa2e 100644
> --- a/drivers/gpu/drm/drm_client_modeset.c
> +++ b/drivers/gpu/drm/drm_client_modeset.c
> @@ -357,6 +370,8 @@ static bool drm_client_target_cloned(struct drm_device *dev,
>  			continue;
>  
>  		list_for_each_entry(mode, &connectors[i]->modes, head) {
> +			if (drm_mode_validate_size(mode, width, height) != MODE_OK)
> +				continue;
>  			if (drm_mode_match(mode, dmt_mode,
>  					   DRM_MODE_MATCH_TIMINGS |
>  					   DRM_MODE_MATCH_CLOCK |

[Severity: Medium]
Can this result in a stale modes[i] value being incorrectly accepted?

In drm_client_target_cloned(), the driver tries to find a matching 1024x768 
mode across all connectors if command-line cloning fails. 

If a connector doesn't have a 1024x768 mode, or if the mode is skipped 
because it exceeds the width and height constraints added by this patch, 
mode_replace() is bypassed:

		list_for_each_entry(mode, &connectors[i]->modes, head) {
			if (drm_mode_validate_size(mode, width, height) != MODE_OK)
				continue;
			if (drm_mode_match(mode, dmt_mode, ...))
				mode_replace(dev, &modes[i], mode);
		}
		if (!modes[i])
			can_clone = false;

Because modes[i] is not cleared before this loop, it retains whatever value 
it was assigned in the earlier command-line pass. The termination check 
if (!modes[i]) evaluates to false, and can_clone incorrectly remains true 
even if heterogeneous modes are present. 

While this is a preexisting structural flaw in the function, does adding 
the drm_mode_validate_size() check inside the 1024x768 loop trigger it by 
making it far more likely for a 1024x768 mode to be skipped?

[ ... ]

> @@ -750,7 +767,8 @@ static bool drm_client_firmware_config(struct drm_client_dev *client,
>  		    num_tiled_conns < connector->num_h_tile * connector->num_v_tile) {
>  			mode_type = "non tiled";
>  			mode_replace(dev, &modes[i],
> -				     drm_connector_fallback_non_tiled_mode(connector));
> +				     drm_connector_fallback_non_tiled_mode(connector,
> +									   width, height));
>  		}
>  		crtcs[i] = crtc;

[Severity: High]
Does this code risk a NULL pointer dereference in drm_client_firmware_config()?

If a connector is part of a tiled display but some tiles are missing, the 
driver attempts to fall back to a non-tiled mode by calling 
drm_connector_fallback_non_tiled_mode(). 

This patch modifies the fallback function to filter out any modes that exceed 
the client's width and height. If no modes fit these constraints, the function 
returns NULL. 

When this happens, mode_replace() safely destroys the existing mode and 
assigns NULL to modes[i]. However, immediately following this block, 
drm_dbg_kms() unconditionally dereferences modes[i]->name when logging 
the selected mode:

		crtcs[i] = crtc;

		drm_dbg_kms(dev, "[CONNECTOR::%d:%s] on [CRTC:%d:%s] using %s mode: %s\n",
			    connector->base.id, connector->name,
			    crtc->base.id, crtc->name,
			    mode_type, modes[i]->name);

Since the new size filter makes returning NULL far more likely, can this 
cause a crash if modes[i] becomes NULL?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015212545.1989962-1-hugo@hsal.es?part=1

[PATCH 2/2] drm/tests: Add KUnit tests for the client mode size limits

[Hugo Sales]

Test that the first-mode, cmdline, tiled and clone pickers skip modes
larger than the client's size, and that drm_client_modeset_probe() keeps
the preferred mode in connector->modes when the client is smaller than
it

The probe test calls drm_client_modeset_create() directly, because
drm_client_init() needs a dumb_create callback the test driver doesn't
have

Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Hugo Sales <hugo@hsal.es>
---
 .../gpu/drm/tests/drm_client_modeset_test.c   | 218 +++++++++++++++++-
 1 file changed, 217 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/tests/drm_client_modeset_test.c b/drivers/gpu/drm/tests/drm_client_modeset_test.c
index 00f43fca9795..93aa919d4f31 100644
--- a/drivers/gpu/drm/tests/drm_client_modeset_test.c
+++ b/drivers/gpu/drm/tests/drm_client_modeset_test.c
@@ -18,6 +18,8 @@ struct drm_client_modeset_test_priv {
 	struct drm_device *drm;
 	struct device *dev;
 	struct drm_connector connector;
+	struct drm_connector second_connector;
+	struct drm_encoder encoder;
 };
 
 static int drm_client_modeset_connector_get_modes(struct drm_connector *connector)
@@ -48,9 +50,23 @@ static const struct drm_connector_helper_funcs drm_client_modeset_connector_help
 	.get_modes = drm_client_modeset_connector_get_modes,
 };
 
+static int drm_client_modeset_connector_get_modes_preferred(struct drm_connector *connector)
+{
+	int count = drm_add_modes_noedid(connector, 1920, 1080);
+
+	drm_set_preferred_mode(connector, 1920, 1080);
+
+	return count;
+}
+
+static const struct drm_connector_helper_funcs drm_client_modeset_preferred_helper_funcs = {
+	.get_modes = drm_client_modeset_connector_get_modes_preferred,
+};
+
 static const struct drm_connector_funcs drm_client_modeset_connector_funcs = {
 	.atomic_destroy_state   = drm_atomic_helper_connector_destroy_state,
-	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+	.fill_modes             = drm_helper_probe_single_connector_modes,
 };
 
 static int drm_client_modeset_test_init(struct kunit *test)
@@ -197,6 +213,206 @@ static struct kunit_suite drm_test_pick_cmdline_test_suite = {
 
 kunit_test_suite(drm_test_pick_cmdline_test_suite);
 
+static void drm_test_pick_first_mode_within_limits(struct kunit *test)
+{
+	struct drm_client_modeset_test_priv *priv = test->priv;
+	struct drm_device *drm = priv->drm;
+	struct drm_connector *connector = &priv->connector;
+	const struct drm_display_mode *mode;
+	int ret;
+
+	drm_connector_helper_add(connector, &drm_client_modeset_preferred_helper_funcs);
+
+	mutex_lock(&drm->mode_config.mutex);
+	ret = drm_helper_probe_single_connector_modes(connector, 1920, 1080);
+	mutex_unlock(&drm->mode_config.mutex);
+	KUNIT_ASSERT_GT(test, ret, 0);
+
+	mode = drm_connector_first_mode(connector, 1920, 1080);
+	KUNIT_ASSERT_NOT_NULL(test, mode);
+	KUNIT_EXPECT_EQ(test, mode->hdisplay, 1920);
+	KUNIT_EXPECT_EQ(test, mode->vdisplay, 1080);
+
+	KUNIT_ASSERT_NULL(test, drm_connector_preferred_mode(connector, 1024, 768));
+
+	mode = drm_connector_first_mode(connector, 1024, 768);
+	KUNIT_ASSERT_NOT_NULL(test, mode);
+	KUNIT_EXPECT_EQ(test, mode->hdisplay, 1024);
+	KUNIT_EXPECT_EQ(test, mode->vdisplay, 768);
+
+	KUNIT_EXPECT_NULL(test, drm_connector_first_mode(connector, 320, 240));
+}
+
+static void drm_test_pick_cmdline_mode_within_limits(struct kunit *test)
+{
+	struct drm_client_modeset_test_priv *priv = test->priv;
+	struct drm_device *drm = priv->drm;
+	struct drm_connector *connector = &priv->connector;
+	struct drm_cmdline_mode *cmdline_mode = &connector->cmdline_mode;
+	const struct drm_display_mode *mode;
+	int ret;
+
+	KUNIT_ASSERT_TRUE(test, drm_mode_parse_command_line_for_connector("1920x1080", connector,
+									  cmdline_mode));
+
+	mutex_lock(&drm->mode_config.mutex);
+	ret = drm_helper_probe_single_connector_modes(connector, 1920, 1080);
+	mutex_unlock(&drm->mode_config.mutex);
+	KUNIT_ASSERT_GT(test, ret, 0);
+
+	mode = drm_connector_pick_cmdline_mode(connector, 1920, 1080);
+	KUNIT_ASSERT_NOT_NULL(test, mode);
+	KUNIT_EXPECT_EQ(test, mode->hdisplay, 1920);
+	KUNIT_EXPECT_EQ(test, mode->vdisplay, 1080);
+
+	KUNIT_EXPECT_NULL(test, drm_connector_pick_cmdline_mode(connector, 1024, 768));
+}
+
+static void drm_test_pick_tiled_mode_within_limits(struct kunit *test)
+{
+	struct drm_client_modeset_test_priv *priv = test->priv;
+	struct drm_device *drm = priv->drm;
+	struct drm_connector *connector = &priv->connector;
+	const struct drm_display_mode *mode;
+	int ret;
+
+	drm_connector_helper_add(connector, &drm_client_modeset_preferred_helper_funcs);
+	connector->tile_h_size = 1920;
+	connector->tile_v_size = 1080;
+
+	mutex_lock(&drm->mode_config.mutex);
+	ret = drm_helper_probe_single_connector_modes(connector, 1920, 1080);
+	mutex_unlock(&drm->mode_config.mutex);
+	KUNIT_ASSERT_GT(test, ret, 0);
+
+	mode = drm_connector_get_tiled_mode(connector, 1920, 1080);
+	KUNIT_ASSERT_NOT_NULL(test, mode);
+	KUNIT_EXPECT_EQ(test, mode->hdisplay, 1920);
+	KUNIT_EXPECT_EQ(test, mode->vdisplay, 1080);
+
+	KUNIT_EXPECT_NULL(test, drm_connector_get_tiled_mode(connector, 1024, 768));
+
+	mode = drm_connector_fallback_non_tiled_mode(connector, 1024, 768);
+	KUNIT_ASSERT_NOT_NULL(test, mode);
+	KUNIT_EXPECT_EQ(test, mode->hdisplay, 1024);
+	KUNIT_EXPECT_EQ(test, mode->vdisplay, 768);
+}
+
+static void drm_test_pick_clone_mode_within_limits(struct kunit *test)
+{
+	struct drm_client_modeset_test_priv *priv = test->priv;
+	struct drm_device *drm = priv->drm;
+	struct drm_connector *connectors[] = { &priv->connector, &priv->second_connector };
+	const struct drm_display_mode *modes[ARRAY_SIZE(connectors)] = {};
+	struct drm_client_offset offsets[ARRAY_SIZE(connectors)] = {};
+	bool enabled[ARRAY_SIZE(connectors)] = { true, true };
+	unsigned int i;
+	int ret;
+
+	ret = drmm_connector_init(drm, &priv->second_connector,
+				  &drm_client_modeset_connector_funcs,
+				  DRM_MODE_CONNECTOR_Unknown,
+				  NULL);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	drm_connector_helper_add(&priv->second_connector,
+				 &drm_client_modeset_connector_helper_funcs);
+
+	mutex_lock(&drm->mode_config.mutex);
+	for (i = 0; i < ARRAY_SIZE(connectors); i++) {
+		ret = drm_helper_probe_single_connector_modes(connectors[i], 1920, 1080);
+		KUNIT_EXPECT_GT(test, ret, 0);
+	}
+	mutex_unlock(&drm->mode_config.mutex);
+
+	KUNIT_EXPECT_TRUE(test, drm_client_target_cloned(drm, connectors, ARRAY_SIZE(connectors),
+							 modes, offsets, enabled, 1024, 768));
+	for (i = 0; i < ARRAY_SIZE(connectors); i++) {
+		KUNIT_ASSERT_NOT_NULL(test, modes[i]);
+		KUNIT_EXPECT_EQ(test, modes[i]->hdisplay, 1024);
+		KUNIT_EXPECT_EQ(test, modes[i]->vdisplay, 768);
+	}
+	modes_destroy(drm, modes, ARRAY_SIZE(connectors));
+
+	KUNIT_EXPECT_FALSE(test, drm_client_target_cloned(drm, connectors, ARRAY_SIZE(connectors),
+							  modes, offsets, enabled, 800, 600));
+	modes_destroy(drm, modes, ARRAY_SIZE(connectors));
+}
+
+KUNIT_DEFINE_ACTION_WRAPPER(drm_client_modeset_free_wrapper, drm_client_modeset_free,
+			    struct drm_client_dev *);
+
+static void drm_test_probe_keeps_modes_above_limits(struct kunit *test)
+{
+	struct drm_client_modeset_test_priv *priv = test->priv;
+	struct drm_device *drm = priv->drm;
+	struct drm_connector *connector = &priv->connector;
+	const struct drm_display_mode *mode;
+	struct drm_client_dev *client;
+	struct drm_encoder *encoder = &priv->encoder;
+	struct drm_plane *plane;
+	struct drm_crtc *crtc;
+	int ret;
+
+	drm->mode_config.max_width = 4096;
+	drm->mode_config.max_height = 4096;
+
+	plane = drm_kunit_helper_create_primary_plane(test, drm, NULL, NULL, NULL, 0, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, plane);
+
+	crtc = drm_kunit_helper_create_crtc(test, drm, plane, NULL, NULL, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, crtc);
+
+	ret = drmm_encoder_init(drm, encoder, NULL, DRM_MODE_ENCODER_NONE, NULL);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+	encoder->possible_crtcs = drm_crtc_mask(crtc);
+
+	ret = drm_connector_attach_encoder(connector, encoder);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	drm_connector_helper_add(connector, &drm_client_modeset_preferred_helper_funcs);
+
+	client = kunit_kzalloc(test, sizeof(*client), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_NULL(test, client);
+	client->dev = drm;
+
+	ret = drm_client_modeset_create(client);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	ret = kunit_add_action_or_reset(test, drm_client_modeset_free_wrapper, client);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	ret = drm_client_modeset_probe(client, 1024, 768);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	mode = client->modesets[0].mode;
+	KUNIT_ASSERT_NOT_NULL(test, mode);
+	KUNIT_EXPECT_EQ(test, mode->hdisplay, 1024);
+	KUNIT_EXPECT_EQ(test, mode->vdisplay, 768);
+
+	mode = drm_connector_preferred_mode(connector, 1920, 1080);
+	KUNIT_ASSERT_NOT_NULL(test, mode);
+	KUNIT_EXPECT_EQ(test, mode->hdisplay, 1920);
+	KUNIT_EXPECT_EQ(test, mode->vdisplay, 1080);
+}
+
+static struct kunit_case drm_test_pick_mode_limits_tests[] = {
+	KUNIT_CASE(drm_test_pick_first_mode_within_limits),
+	KUNIT_CASE(drm_test_pick_cmdline_mode_within_limits),
+	KUNIT_CASE(drm_test_pick_tiled_mode_within_limits),
+	KUNIT_CASE(drm_test_pick_clone_mode_within_limits),
+	KUNIT_CASE(drm_test_probe_keeps_modes_above_limits),
+	{}
+};
+
+static struct kunit_suite drm_test_pick_mode_limits_test_suite = {
+	.name = "drm_test_pick_mode_limits",
+	.init = drm_client_modeset_test_init,
+	.test_cases = drm_test_pick_mode_limits_tests
+};
+
+kunit_test_suite(drm_test_pick_mode_limits_test_suite);
+
 /*
  * This file is included directly by drm_client_modeset.c so we can't
  * use any MODULE_* macro here.
-- 
2.55.0

Re: [PATCH 2/2] drm/tests: Add KUnit tests for the client mode size limits

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] drm/tests: lockless access to connector->modes violates locking rules
- [Low] drm/tests: memory leak of cloned modes on KUnit assertion failure
--

commit 806dc20896099436a3d060ce16a4a8cb84cf8467
Author: Hugo Sales <hugo@hsal.es>
Subject: drm/tests: Add KUnit tests for the client mode size limits

This commit adds KUnit tests for various client mode pickers to ensure they
skip modes larger than the client's size. It also tests that the modeset
probe properly retains the preferred mode.

> diff --git a/drivers/gpu/drm/tests/drm_client_modeset_test.c b/drivers/gpu/drm/tests/drm_client_modeset_test.c
> index 00f43fca97954..93aa919d4f317 100644
> --- a/drivers/gpu/drm/tests/drm_client_modeset_test.c
> +++ b/drivers/gpu/drm/tests/drm_client_modeset_test.c

[ ... ]

> @@ -197,6 +213,206 @@ static struct kunit_suite drm_test_pick_cmdline_test_suite = {
>  
>  kunit_test_suite(drm_test_pick_cmdline_test_suite);
>  
> +static void drm_test_pick_first_mode_within_limits(struct kunit *test)
> +{

[ ... ]

> +	mutex_lock(&drm->mode_config.mutex);
> +	ret = drm_helper_probe_single_connector_modes(connector, 1920, 1080);
> +	mutex_unlock(&drm->mode_config.mutex);
> +	KUNIT_ASSERT_GT(test, ret, 0);
> +
> +	mode = drm_connector_first_mode(connector, 1920, 1080);

[Severity: Low]
The connector->modes list is explicitly documented as being protected by
dev->mode_config.mutex. Here, the mutex is unlocked just before calling
drm_connector_first_mode(), which iterates over connector->modes.

While this is isolated test code and no concurrent modification occurs,
could this lockless iteration trigger static analysis tools or lockdep
warnings?

> +	KUNIT_ASSERT_NOT_NULL(test, mode);
> +	KUNIT_EXPECT_EQ(test, mode->hdisplay, 1920);
> +	KUNIT_EXPECT_EQ(test, mode->vdisplay, 1080);

[ ... ]

> +static void drm_test_pick_cmdline_mode_within_limits(struct kunit *test)
> +{

[ ... ]

> +	mutex_lock(&drm->mode_config.mutex);
> +	ret = drm_helper_probe_single_connector_modes(connector, 1920, 1080);
> +	mutex_unlock(&drm->mode_config.mutex);
> +	KUNIT_ASSERT_GT(test, ret, 0);
> +
> +	mode = drm_connector_pick_cmdline_mode(connector, 1920, 1080);

[Severity: Low]
Similarly here, drm_connector_pick_cmdline_mode() iterates over
connector->modes outside of drm->mode_config.mutex. Does this violate
the locking rules for the connector modes list?

> +	KUNIT_ASSERT_NOT_NULL(test, mode);

[ ... ]

> +static void drm_test_pick_tiled_mode_within_limits(struct kunit *test)
> +{

[ ... ]

> +	mutex_lock(&drm->mode_config.mutex);
> +	ret = drm_helper_probe_single_connector_modes(connector, 1920, 1080);
> +	mutex_unlock(&drm->mode_config.mutex);
> +	KUNIT_ASSERT_GT(test, ret, 0);
> +
> +	mode = drm_connector_get_tiled_mode(connector, 1920, 1080);

[Severity: Low]
As with the previous pickers, drm_connector_get_tiled_mode() accesses
connector->modes without the required drm->mode_config.mutex held.

> +	KUNIT_ASSERT_NOT_NULL(test, mode);

[ ... ]

> +static void drm_test_pick_clone_mode_within_limits(struct kunit *test)
> +{

[ ... ]

> +	mutex_lock(&drm->mode_config.mutex);
> +	for (i = 0; i < ARRAY_SIZE(connectors); i++) {
> +		ret = drm_helper_probe_single_connector_modes(connectors[i], 1920, 1080);
> +		KUNIT_EXPECT_GT(test, ret, 0);
> +	}
> +	mutex_unlock(&drm->mode_config.mutex);
> +
> +	KUNIT_EXPECT_TRUE(test, drm_client_target_cloned(drm, connectors, ARRAY_SIZE(connectors),
> +							 modes, offsets, enabled, 1024, 768));

[Severity: Low]
This call to drm_client_target_cloned() also iterates over
connector->modes after the config mutex has been released.

> +	for (i = 0; i < ARRAY_SIZE(connectors); i++) {
> +		KUNIT_ASSERT_NOT_NULL(test, modes[i]);

[Severity: Low]
If this KUNIT_ASSERT_NOT_NULL() assertion fails, it immediately forces an
early return from the test.

Because drm_client_target_cloned() allocates new modes via mode_replace(),
would an early return here bypass the mandatory modes_destroy() cleanup
below and leak the dynamically allocated mode objects?

Could KUnit's managed allocations (like kunit_add_action) be used to ensure
modes_destroy() always runs on cleanup?

> +		KUNIT_EXPECT_EQ(test, modes[i]->hdisplay, 1024);
> +		KUNIT_EXPECT_EQ(test, modes[i]->vdisplay, 768);
> +	}
> +	modes_destroy(drm, modes, ARRAY_SIZE(connectors));

[ ... ]

> +static void drm_test_probe_keeps_modes_above_limits(struct kunit *test)
> +{

[ ... ]

> +	ret = drm_client_modeset_probe(client, 1024, 768);
> +	KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +	mode = client->modesets[0].mode;
> +	KUNIT_ASSERT_NOT_NULL(test, mode);
> +	KUNIT_EXPECT_EQ(test, mode->hdisplay, 1024);
> +	KUNIT_EXPECT_EQ(test, mode->vdisplay, 768);
> +
> +	mode = drm_connector_preferred_mode(connector, 1920, 1080);

[Severity: Low]
Once again, drm_connector_preferred_mode() is called to iterate the
connector->modes list without the config mutex held.

> +	KUNIT_ASSERT_NOT_NULL(test, mode);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015212545.1989962-1-hugo@hsal.es?part=2

Re: [PATCH 0/2] drm/client: Keep larger modes in connector->modes on fbdev hotplug

[Ville Syrjälä]

On Thu, Oct 15, 2026 at 10:25:43PM +0100, Hugo Sales wrote:
> On an ASUS ROG Flow X13 with a 3840x2400 eDP panel, booted with
> video=eDP-1:1920x1200@60, an HDMI hotplug makes fbdev reprobe every
> connector with its 1920x1200 framebuffer as the limit. connector->modes
> loses the panel's 3840x2400 mode, and after suspend/resume amdgpu drives
> the panel with the 1920x1200 timing. The image ends up squeezed into the
> top-left corner. Details are in
> https://gitlab.freedesktop.org/drm/amd/-/issues/5983

Sounds like an amdgpu bug.

> 
> Patch 1 makes drm_client_modeset_probe() probe with the device's limits
> and moves the framebuffer size check into the fbdev mode pickers.
> Patch 2 adds KUnit tests for it
> 
> fbdev's hotplug probe has passed the framebuffer size since 2010
> (4abe35204af8). The pickers didn't check mode sizes then, so leaving
> larger modes out of the list was what kept fbdev from picking one. They
> check the size themselves now, so the probe can use the device's limits
> like initial setup, drm_log and GETCONNECTOR already do
> 
> The HiBMC thread "[RFC] drm/client: Relax clone mode selection in
> drm_client_target_cloned" runs into the same pruning: its 640x480
> framebuffer removes 1024x768 from both connectors' lists. With this
> series userspace sees that mode again, but fbdev still can't use it, so
> this doesn't replace that RFC:
> https://lore.kernel.org/r/a48e0b33-7eab-478e-a396-b6e76b7a066f@huawei.com
> 
> I sent an amdgpu fix for the same symptom separately. It makes amdgpu
> fall back to the encoder's native mode on eDP and LVDS:
> https://lore.kernel.org/r/20261014223041.171317-1-hugo@hsal.es
> This series fixes the shared mode list for every driver instead. Either
> one alone fixes the reported bug, from my testing
> 
> On amd-staging-drm-next with only this series applied, with an HDMI
> hotplug and suspend/resume (pm_test=platform) in a loop, 255 of 255
> resumes kept the native timing. Without it, 28 of 29 came back with the
> wrong timing, and the same loop on unpatched v7.2.8 got it wrong in 103
> of 103. The drm KUnit tests pass, also under KASAN, which reports
> nothing in the code this series touches
> 
> Hugo Sales (2):
>   drm/client: Probe connectors with the device's mode size limits
>   drm/tests: Add KUnit tests for the client mode size limits
> 
>  drivers/gpu/drm/drm_client_modeset.c          |  57 +++--
>  .../gpu/drm/tests/drm_client_modeset_test.c   | 222 +++++++++++++++++-
>  2 files changed, 260 insertions(+), 19 deletions(-)
> 
> -- 
> 2.55.0

-- 
Ville Syrjälä
Intel

Re: [PATCH 0/2] drm/client: Keep larger modes in connector->modes on fbdev hotplug

[Ville Syrjälä]

On Fri, Oct 16, 2026 at 02:16:52PM +0100, Hugo Sales wrote:
> On Fri, Oct 16, 2026 at 03:28:39PM +0300, Ville Syrjälä wrote:
> > On Thu, Oct 15, 2026 at 10:25:43PM +0100, Hugo Sales wrote:
> > > On an ASUS ROG Flow X13 with a 3840x2400 eDP panel, booted with
> > > video=eDP-1:1920x1200@60, an HDMI hotplug makes fbdev reprobe every
> > > connector with its 1920x1200 framebuffer as the limit. connector->modes
> > > loses the panel's 3840x2400 mode, and after suspend/resume amdgpu drives
> > > the panel with the 1920x1200 timing. The image ends up squeezed into the
> > > top-left corner. Details are in
> > > https://gitlab.freedesktop.org/drm/amd/-/issues/5983
> >
> > Sounds like an amdgpu bug.
> 
> Agreed, the resume bug is in amdgpu. It looks up the eDP native mode in
> connector->modes, which fbdev's hotplug probe can trim. This series
> fixes it there:
> https://lore.kernel.org/r/20261014223041.171317-1-hugo@hsal.es
> 
> I sent this one for the shared list itself because after a hotplug,
> connector->modes depends on fbdev's framebuffer size, while initial
> setup, drm_log and GETCONNECTOR probe with the device's limits.

Connectors aren't probed before something actually probes them.
Typically that would be fbdev.

> Until
> something reprobes, the sysfs modes file and any driver that reads the
> list see the trimmed version
> 
> amdgpu isn't the only driver that reads it. stm/lvds and
> shmob_drm_encoder_mode_fixup() take the first entry of connector->modes
> as the panel mode, so they may hit the same thing with a video= mode
> smaller than the panel. I haven't tested those, because I don't have
> the hardware. Fixing it in the core covers them without each driver
> keeping its own copy of the native mode

Those things will need to be fixed regardless because they are
broken wrt locking.

But I suppose not filtering the mode list based on the fbdev size
might be a bit less confusing for anyone who looks at the mode
list via sysfs. Can't immediately think what else besises the 
fbdev/client code would depend on this, so maybe it's fine.

If we do this then I think we should stop passing the size to
.fill_modes() entirely, because we always pass the same device
limits. The downside is all those annoying extra
drm_mode_validate_size() calls everywhere, but I guess we
could live with that.

-- 
Ville Syrjälä
Intel

Re: [PATCH 0/2] drm/client: Keep larger modes in connector->modes on fbdev hotplug

[Ville Syrjälä]

On Fri, Oct 16, 2026 at 03:40:13PM +0100, Hugo Sales wrote:
> On Fri, Oct 16, 2026 at 04:57:30PM +0300, Ville Syrjälä wrote:
> > Connectors aren't probed before something actually probes them.
> > Typically that would be fbdev.
> 
> I think I may have misspoken before. fbdev's first probe already uses
> the device's limits: __drm_fb_helper_initial_config_and_unlock() passes
> max_width and max_height. Only drm_fb_helper_hotplug_event() passes the
> framebuffer size, so the list starts with every mode and loses the
> larger ones on the first hotplug. This series makes the hotplug probe
> match the first one

Right, forgot about the initial probe. And I think we definitely
want the initial probe to use the max size because we don't want
to keep the BIOS framebuffer if it's tiny. A long time ago we
kept the BIOS fb no matter what, but that made the console pretty
much unusable (due to being too small) on some machines.

> 
> Are you suggesting a different approach?

Nah, I guess what you're doing should be fine.

-- 
Ville Syrjälä
Intel