Re: [PATCH 4/8] drm/client: Make copies of modes

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi

Am 03.10.24 um 13:33 schrieb Ville Syrjala:
From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>

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 instead
of just storing pointers. We do have to replace the NULL checks
with something else though. Checking that mode->clock!=0
should be sufficient.

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.

I think it would be a lot better to first build that mode list while holding the mutex, and afterwards copy the resulting modes before releasing the lock. The code below is convoluted with drm_mode_copy().


Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
---
  drivers/gpu/drm/drm_client_modeset.c | 80 +++++++++++++++-------------
  1 file changed, 43 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
index 888323137a6a..d413e119db3f 100644
--- a/drivers/gpu/drm/drm_client_modeset.c
+++ b/drivers/gpu/drm/drm_client_modeset.c
@@ -265,10 +265,15 @@ static void drm_client_connectors_enabled(struct drm_connector *connectors[],
  		enabled[i] = drm_connector_enabled(connectors[i], false);
  }
+static bool mode_valid(const struct drm_display_mode *mode)
+{
+	return mode->clock != 0;

A mode's clock isn't always known and not all drivers might set it correctly. At least in simpledrm/ofdrm, we have to make up a clock value for the firmware framebuffer. Otherwise some of our userspace would oops.

The test for clock != 0 makes sense, but it's maybe the wrong place to do this. Would a test for hdisplay/vdisplay != 0 work instead?

+}
+
  static bool drm_client_target_cloned(struct drm_device *dev,
  				     struct drm_connector *connectors[],
  				     unsigned int connector_count,
-				     const struct drm_display_mode *modes[],
+				     struct drm_display_mode modes[],
  				     struct drm_client_offset offsets[],
  				     bool enabled[], int width, int height)
  {
@@ -296,15 +301,16 @@ 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]);
-		if (!modes[i]) {
+
+		drm_mode_copy(&modes[i], drm_connector_pick_cmdline_mode(connectors[i]));
+		if (!mode_valid(&modes[i])) {

You're copying and only then test for validity?

  			can_clone = false;
  			break;
  		}
  		for (j = 0; j < i; j++) {
  			if (!enabled[j])
  				continue;
-			if (!drm_mode_match(modes[j], modes[i],
+			if (!drm_mode_match(&modes[j], &modes[i],
  					    DRM_MODE_MATCH_TIMINGS |
  					    DRM_MODE_MATCH_CLOCK |
  					    DRM_MODE_MATCH_FLAGS |
@@ -335,9 +341,9 @@ 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;
+				drm_mode_copy(&modes[i], mode);
  		}
-		if (!modes[i])
+		if (!mode_valid(&modes[i]))
  			can_clone = false;
  	}
  	drm_mode_destroy(dev, dmt_mode);
@@ -354,7 +360,7 @@ static bool drm_client_target_cloned(struct drm_device *dev,
  static int drm_client_get_tile_offsets(struct drm_device *dev,
  				       struct drm_connector *connectors[],
  				       unsigned int connector_count,
-				       const struct drm_display_mode *modes[],
+				       const struct drm_display_mode modes[],
  				       struct drm_client_offset offsets[],
  				       int idx,
  				       int h_idx, int v_idx)
@@ -368,17 +374,17 @@ static int drm_client_get_tile_offsets(struct drm_device *dev,
  		if (!connector->has_tile)
  			continue;
- if (!modes[i] && (h_idx || v_idx)) {
+		if (!mode_valid(&modes[i]) && (h_idx || v_idx)) {
  			drm_dbg_kms(dev,
  				    "[CONNECTOR:%d:%s] no modes for connector tiled %d\n",
  				    connector->base.id, connector->name, i);
  			continue;
  		}
  		if (connector->tile_h_loc < h_idx)
-			hoffset += modes[i]->hdisplay;
+			hoffset += modes[i].hdisplay;
if (connector->tile_v_loc < v_idx)
-			voffset += modes[i]->vdisplay;
+			voffset += modes[i].vdisplay;
  	}
  	offsets[idx].x = hoffset;
  	offsets[idx].y = voffset;
@@ -389,7 +395,7 @@ static int drm_client_get_tile_offsets(struct drm_device *dev,
  static bool drm_client_target_preferred(struct drm_device *dev,
  					struct drm_connector *connectors[],
  					unsigned int connector_count,
-					const struct drm_display_mode *modes[],
+					struct drm_display_mode modes[],
  					struct drm_client_offset offsets[],
  					bool enabled[], int width, int height)
  {
@@ -445,16 +451,16 @@ static bool drm_client_target_preferred(struct drm_device *dev,
  		}
mode_type = "cmdline";
-		modes[i] = drm_connector_pick_cmdline_mode(connector);
+		drm_mode_copy(&modes[i], drm_connector_pick_cmdline_mode(connector));
- if (!modes[i]) {
+		if (!mode_valid(&modes[i])) {
  			mode_type = "preferred";
-			modes[i] = drm_connector_preferred_mode(connector, width, height);
+			drm_mode_copy(&modes[i], drm_connector_preferred_mode(connector, width, height));
  		}
- if (!modes[i]) {
+		if (!mode_valid(&modes[i])) {
  			mode_type = "first";
-			modes[i] = drm_connector_first_mode(connector);
+			drm_mode_copy(&modes[i], drm_connector_first_mode(connector));
  		}
/*
@@ -472,17 +478,17 @@ 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);
+				drm_mode_copy(&modes[i], drm_connector_fallback_non_tiled_mode(connector));
  			} else {
  				mode_type = "tiled";
-				modes[i] = drm_connector_get_tiled_mode(connector);
+				drm_mode_copy(&modes[i], drm_connector_get_tiled_mode(connector));
  			}
  		}
- if (modes[i])
+		if (mode_valid(&modes[i]))
  			drm_dbg_kms(dev, "[CONNECTOR:%d:%s] found %s mode: %s\n",
  				    connector->base.id, connector->name,
-				    mode_type, modes[i]->name);
+				    mode_type, modes[i].name);
  		else
  			drm_dbg_kms(dev, "[CONNECTOR:%d:%s] no mode found\n",
  				    connector->base.id, connector->name);
@@ -514,7 +520,7 @@ static int drm_client_pick_crtcs(struct drm_client_dev *client,
  				 struct drm_connector *connectors[],
  				 unsigned int connector_count,
  				 struct drm_crtc *best_crtcs[],
-				 const struct drm_display_mode *modes[],
+				 const struct drm_display_mode modes[],
  				 int n, int width, int height)
  {
  	struct drm_device *dev = client->dev;
@@ -532,7 +538,7 @@ static int drm_client_pick_crtcs(struct drm_client_dev *client,
  	best_crtcs[n] = NULL;
  	best_score = drm_client_pick_crtcs(client, connectors, connector_count,
  					   best_crtcs, modes, n + 1, width, height);
-	if (modes[n] == NULL)
+	if (!mode_valid(&modes[n]))
  		return best_score;
crtcs = kcalloc(connector_count, sizeof(*crtcs), GFP_KERNEL);
@@ -566,7 +572,7 @@ static int drm_client_pick_crtcs(struct drm_client_dev *client,
  			if (dev->mode_config.num_crtc > 1)
  				continue;
- if (!drm_mode_equal(modes[o], modes[n]))
+			if (!drm_mode_equal(&modes[o], &modes[n]))
  				continue;
  		}
@@ -589,7 +595,7 @@ static bool drm_client_firmware_config(struct drm_client_dev *client,
  				       struct drm_connector *connectors[],
  				       unsigned int connector_count,
  				       struct drm_crtc *crtcs[],
-				       const struct drm_display_mode *modes[],
+				       struct drm_display_mode modes[],
  				       struct drm_client_offset offsets[],
  				       bool enabled[], int width, int height)
  {
@@ -690,20 +696,20 @@ static bool drm_client_firmware_config(struct drm_client_dev *client,
  		}
mode_type = "cmdline";
-		modes[i] = drm_connector_pick_cmdline_mode(connector);
+		drm_mode_copy(&modes[i], drm_connector_pick_cmdline_mode(connector));
- if (!modes[i]) {
+		if (!mode_valid(&modes[i])) {
  			mode_type = "preferred";
-			modes[i] = drm_connector_preferred_mode(connector, width, height);
+			drm_mode_copy(&modes[i], drm_connector_preferred_mode(connector, width, height));
  		}
- if (!modes[i]) {
+		if (!mode_valid(&modes[i])) {
  			mode_type = "first";
-			modes[i] = drm_connector_first_mode(connector);
+			drm_mode_copy(&modes[i], drm_connector_first_mode(connector));
  		}
/* last resort: use current mode */
-		if (!modes[i]) {
+		if (!mode_valid(&modes[i])) {
  			/*
  			 * IMPORTANT: We want to use the adjusted mode (i.e.
  			 * after the panel fitter upscaling) as the initial
@@ -716,7 +722,7 @@ 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;
+			drm_mode_copy(&modes[i], &connector->state->crtc->mode);
  		}
/*
@@ -726,14 +732,14 @@ 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);
+			drm_mode_copy(&modes[i], drm_connector_fallback_non_tiled_mode(connector));
  		}
  		crtcs[i] = new_crtc;
drm_dbg_kms(dev, "[CONNECTOR::%d:%s] on [CRTC:%d:%s] using %s mode: %s\n",
  			    connector->base.id, connector->name,
  			    new_crtc->base.id, new_crtc->name,
-			    mode_type, modes[i]->name);
+			    mode_type, modes[i].name);
fallback = false;
  		conn_configured |= BIT(i);
@@ -789,8 +795,7 @@ 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_display_mode *modes;
  	struct drm_crtc **crtcs;
  	int i, ret = 0;
  	bool *enabled;
@@ -858,10 +863,12 @@ 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++) {
-		const struct drm_display_mode *mode = modes[i];
+		const struct drm_display_mode *mode = &modes[i];
  		struct drm_crtc *crtc = crtcs[i];
  		struct drm_client_offset *offset = &offsets[i];
@@ -892,7 +899,6 @@ 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:

--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux