On Fri, 28 Feb 2025, Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
> 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.
>
> 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ä <ville.syrjala@xxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/drm_client_modeset.c | 62 +++++++++++++++++++++-------
> 1 file changed, 47 insertions(+), 15 deletions(-)
>
> 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);
Arguably drm_mode_destroy() should be changed to const too.
Anyway, I think I was able to wrap my head around this.
Reviewed-by: Jani Nikula <jani.nikula@xxxxxxxxx>
> +
> + *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);
--
Jani Nikula, Intel
