[v2,4/8] drm/client: Make copies of modes

Commit Message

Ville Syrjala Feb. 28, 2025, 9:14 p.m. UTC

From: Ville Syrjälä <[e-mail]>

drm_client_firmware_config() is currently picking up the current
mode of the crtc via the legacy crtc->mode, which is not supposed
to be used by atomic drivers at all. We can't simply switch over
to the proper crtc->state->mode because we drop the crtc->mutex
(which protects crtc->state) before the mode gets used.

The most straightforward solution to extend the lifetime of
modes[] seem to be to make full copies of the modes.

And with this we can undo also commit 3eadd887dbac
("drm/client:Fully protect modes[] with dev->mode_config.mutex")
as the lifetime of modes[] no longer has anything to do with
that lock.

v2: Don't try to copy NULL modes
v3: Keep storing pointers and use drm_mode_{duplicate,destroy}()

Signed-off-by: Ville Syrjälä <[e-mail]>
---
 drivers/gpu/drm/drm_client_modeset.c | 62 +++++++++++++++++++++-------
 1 file changed, 47 insertions(+), 15 deletions(-)

Patch

diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
index 148257287ae4..ff034359f063 100644
--- a/drivers/gpu/drm/drm_client_modeset.c
+++ b/drivers/gpu/drm/drm_client_modeset.c
@@ -265,6 +265,25 @@  static void drm_client_connectors_enabled(struct drm_connector *connectors[],
 		enabled[i] = drm_connector_enabled(connectors[i], false);
 }
 
+static void mode_replace(struct drm_device *dev,
+			 const struct drm_display_mode **dst,
+			 const struct drm_display_mode *src)
+{
+	drm_mode_destroy(dev, (struct drm_display_mode *)*dst);
+
+	*dst = src ? drm_mode_duplicate(dev, src) : NULL;
+}
+
+static void modes_destroy(struct drm_device *dev,
+			  const struct drm_display_mode *modes[],
+			  int count)
+{
+	int i;
+
+	for (i = 0; i < count; i++)
+		mode_replace(dev, &modes[i], NULL);
+}
+
 static bool drm_client_target_cloned(struct drm_device *dev,
 				     struct drm_connector *connectors[],
 				     unsigned int connector_count,
@@ -296,7 +315,9 @@  static bool drm_client_target_cloned(struct drm_device *dev,
 	for (i = 0; i < connector_count; i++) {
 		if (!enabled[i])
 			continue;
-		modes[i] = drm_connector_pick_cmdline_mode(connectors[i]);
+
+		mode_replace(dev, &modes[i],
+			     drm_connector_pick_cmdline_mode(connectors[i]));
 		if (!modes[i]) {
 			can_clone = false;
 			break;
@@ -335,7 +356,7 @@  static bool drm_client_target_cloned(struct drm_device *dev,
 					   DRM_MODE_MATCH_CLOCK |
 					   DRM_MODE_MATCH_FLAGS |
 					   DRM_MODE_MATCH_3D_FLAGS))
-				modes[i] = mode;
+				mode_replace(dev, &modes[i], mode);
 		}
 		if (!modes[i])
 			can_clone = false;
@@ -445,16 +466,19 @@  static bool drm_client_target_preferred(struct drm_device *dev,
 		}
 
 		mode_type = "cmdline";
-		modes[i] = drm_connector_pick_cmdline_mode(connector);
+		mode_replace(dev, &modes[i],
+			     drm_connector_pick_cmdline_mode(connector));
 
 		if (!modes[i]) {
 			mode_type = "preferred";
-			modes[i] = drm_connector_preferred_mode(connector, width, height);
+			mode_replace(dev, &modes[i],
+				     drm_connector_preferred_mode(connector, width, height));
 		}
 
 		if (!modes[i]) {
 			mode_type = "first";
-			modes[i] = drm_connector_first_mode(connector);
+			mode_replace(dev, &modes[i],
+				     drm_connector_first_mode(connector));
 		}
 
 		/*
@@ -472,10 +496,12 @@  static bool drm_client_target_preferred(struct drm_device *dev,
 			     connector->tile_v_loc == 0 &&
 			     !drm_connector_get_tiled_mode(connector))) {
 				mode_type = "non tiled";
-				modes[i] = drm_connector_fallback_non_tiled_mode(connector);
+				mode_replace(dev, &modes[i],
+					     drm_connector_fallback_non_tiled_mode(connector));
 			} else {
 				mode_type = "tiled";
-				modes[i] = drm_connector_get_tiled_mode(connector);
+				mode_replace(dev, &modes[i],
+					     drm_connector_get_tiled_mode(connector));
 			}
 		}
 
@@ -690,16 +716,19 @@  static bool drm_client_firmware_config(struct drm_client_dev *client,
 		}
 
 		mode_type = "cmdline";
-		modes[i] = drm_connector_pick_cmdline_mode(connector);
+		mode_replace(dev, &modes[i],
+			     drm_connector_pick_cmdline_mode(connector));
 
 		if (!modes[i]) {
 			mode_type = "preferred";
-			modes[i] = drm_connector_preferred_mode(connector, width, height);
+			mode_replace(dev, &modes[i],
+				     drm_connector_preferred_mode(connector, width, height));
 		}
 
 		if (!modes[i]) {
 			mode_type = "first";
-			modes[i] = drm_connector_first_mode(connector);
+			mode_replace(dev, &modes[i],
+				     drm_connector_first_mode(connector));
 		}
 
 		/* last resort: use current mode */
@@ -716,7 +745,8 @@  static bool drm_client_firmware_config(struct drm_client_dev *client,
 			 * fastboot check to work correctly.
 			 */
 			mode_type = "current";
-			modes[i] = &connector->state->crtc->mode;
+			mode_replace(dev, &modes[i],
+				     &connector->state->crtc->mode);
 		}
 
 		/*
@@ -726,7 +756,8 @@  static bool drm_client_firmware_config(struct drm_client_dev *client,
 		if (connector->has_tile &&
 		    num_tiled_conns < connector->num_h_tile * connector->num_v_tile) {
 			mode_type = "non tiled";
-			modes[i] = drm_connector_fallback_non_tiled_mode(connector);
+			mode_replace(dev, &modes[i],
+				     drm_connector_fallback_non_tiled_mode(connector));
 		}
 		crtcs[i] = new_crtc;
 
@@ -798,7 +829,6 @@  int drm_client_modeset_probe(struct drm_client_dev *client, unsigned int width,
 	unsigned int total_modes_count = 0;
 	struct drm_client_offset *offsets;
 	unsigned int connector_count = 0;
-	/* points to modes protected by mode_config.mutex */
 	const struct drm_display_mode **modes;
 	struct drm_crtc **crtcs;
 	int i, ret = 0;
@@ -850,7 +880,7 @@  int drm_client_modeset_probe(struct drm_client_dev *client, unsigned int width,
 
 	if (!drm_client_firmware_config(client, connectors, connector_count, crtcs,
 					modes, offsets, enabled, width, height)) {
-		memset(modes, 0, connector_count * sizeof(*modes));
+		modes_destroy(dev, modes, connector_count);
 		memset(crtcs, 0, connector_count * sizeof(*crtcs));
 		memset(offsets, 0, connector_count * sizeof(*offsets));
 
@@ -867,6 +897,8 @@  int drm_client_modeset_probe(struct drm_client_dev *client, unsigned int width,
 				      crtcs, modes, 0, width, height);
 	}
 
+	mutex_unlock(&dev->mode_config.mutex);
+
 	drm_client_modeset_release(client);
 
 	for (i = 0; i < connector_count; i++) {
@@ -901,11 +933,11 @@  int drm_client_modeset_probe(struct drm_client_dev *client, unsigned int width,
 			modeset->y = offset->y;
 		}
 	}
-	mutex_unlock(&dev->mode_config.mutex);
 
 	mutex_unlock(&client->modeset_mutex);
 out:
 	kfree(crtcs);
+	modes_destroy(dev, modes, connector_count);
 	kfree(modes);
 	kfree(offsets);
 	kfree(enabled);