On Fri, 28 Feb 2025, Ville Syrjala <[email protected]> wrote: > From: Ville Syrjälä <[email protected]> > > 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ä <[email protected]> > --- > 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 <[email protected]> > + > + *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
