[PATCH 4/8] drm/client: Make copies of modes
Thomas Zimmermann
tzimmermann at suse.de
Mon Oct 7 07:36:13 UTC 2024
Hi
Am 03.10.24 um 13:33 schrieb Ville Syrjala:
> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
>
> 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 at linux.intel.com>
> ---
> 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)
More information about the Intel-gfx
mailing list