| Message ID | 20241003113304.11700-5-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/client: Stop using legacy crtc->mode and a bunch of cleanups | expand |
On Thu, Oct 03, 2024 at 02:33:00PM +0300, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@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. > > Signed-off-by: Ville Syrjälä <ville.syrjala@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; > +} > + > 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])); Apparently this guy doesn't accept NULL, so this is oops central now. I'll have to wrap these up a bit I guess. > + if (!mode_valid(&modes[i])) { > 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: > -- > 2.45.2
Hi Am 03.10.24 um 13:33 schrieb Ville Syrjala: > From: Ville Syrjälä <ville.syrjala@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@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:
On Mon, Oct 07, 2024 at 09:36:13AM +0200, Thomas Zimmermann wrote: > Hi > > Am 03.10.24 um 13:33 schrieb Ville Syrjala: > > From: Ville Syrjälä <ville.syrjala@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(). My first thought was to make copies but still keep track of pointers. That idea was a complete disaster because you now had to carefully free the modes on the list. I then considred some kind of double list approach, but that too seemed more complicated/confusing than the (IMO fairly straightforward) apporach I ended up with. I'd prefer to reduce the nummber of arrays this thing uses rather than increase them. > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@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? That would work as well. drm_mode_validate_basic() rejects everything with clock/hdisplay/vdisplay==0. > > > +} > > + > > 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? I thought drm_mode_copy() is a nop for NULL. Turns out I was wrong, hence the v2. For this specific case I suppose one could also write something like if (whatever_mode()) drm_mode_copy(&modes[i], whatever_mode()); here, but having to repeat oneself is not so great. We could of course avoid that with mode = whatever_mode(); if (mode) drm_mode_copy(&modes[i], mode); with the downside of needing yet another local variable.
hi, Ville Syrjala,
we noticed there is a v2 for this commit
https://lore.kernel.org/all/20241003181553.8891-1-ville.syrjala@linux.intel.com/
but bot failed to analyze the patch mail structure and re-assemble it with
other patches in this serial to form a new branch (which need manual efforts).
so we just made this report out FYI. in case you are sure the issue should be
addressed by v2, please just ignore. if you want us to test v2, please let us
know. thanks
Hello,
kernel test robot noticed "Oops:general_protection_fault,probably_for_non-canonical_address#:#[##]PREEMPT_SMP_KASAN_PTI" on:
commit: 2cc919cccbb5d887534545618d696db4ec5fb691 ("[PATCH 4/8] drm/client: Make copies of modes")
url: https://github.com/intel-lab-lkp/linux/commits/Ville-Syrjala/drm-client-Constify-modes/20241004-061843
base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link: https://lore.kernel.org/all/20241003113304.11700-5-ville.syrjala@linux.intel.com/
patch subject: [PATCH 4/8] drm/client: Make copies of modes
in testcase: boot
compiler: gcc-12
test machine: qemu-system-x86_64 -enable-kvm -cpu SandyBridge -smp 2 -m 16G
(please refer to attached dmesg/kmsg for entire log/backtrace)
+----------------------------------------------------------------------------------------------+------------+------------+
| | 40327b7031 | 2cc919cccb |
+----------------------------------------------------------------------------------------------+------------+------------+
| boot_successes | 15 | 0 |
| boot_failures | 0 | 16 |
| Oops:general_protection_fault,probably_for_non-canonical_address#:#[##]PREEMPT_SMP_KASAN_PTI | 0 | 16 |
| KASAN:null-ptr-deref_in_range[#-#] | 0 | 16 |
| RIP:drm_mode_copy[drm] | 0 | 16 |
| Kernel_panic-not_syncing:Fatal_exception | 0 | 16 |
+----------------------------------------------------------------------------------------------+------------+------------+
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <oliver.sang@intel.com>
| Closes: https://lore.kernel.org/oe-lkp/202410091649.1353a717-oliver.sang@intel.com
[ 12.729071][ T116] bochs-drm 0000:00:02.0: vgaarb: deactivate vga console
[ 12.733522][ T116] Console: switching to colour dummy device 80x25
[ 12.738211][ T116] [drm] Found bochs VGA, ID 0xb0c5.
[ 12.738603][ T116] [drm] Framebuffer size 16384 kB @ 0xfd000000, mmio @ 0xfebf0000.
[ 12.742172][ T116] [drm] Initialized bochs-drm 1.0.0 for 0000:00:02.0 on minor 0
[ 12.744751][ T116] Oops: general protection fault, probably for non-canonical address 0xdffffc0000000000: 0000 [#1] PREEMPT SMP KASAN PTI
[ 12.745622][ T116] KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007]
[ 12.746193][ T116] CPU: 1 UID: 0 PID: 116 Comm: udevd Not tainted 6.12.0-rc1-00311-g2cc919cccbb5 #2
[ 12.746817][ T116] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
[ 12.747510][ T116] RIP: 0010:drm_mode_copy (kbuild/src/consumer/drivers/gpu/drm/drm_modes.c:1422) drm
[ 12.748000][ T116] Code: 40 84 c6 0f 85 01 01 00 00 84 c9 0f 95 c2 0f 9e c0 84 c2 0f 85 f1 00 00 00 48 ba 00 00 00 00 00 fc ff df 48 89 e8 48 c1 e8 03 <0f> b6 0c 10 48 8d 45 77 48 89 c6 83 e0 07 48 c1 ee 03 0f b6 14 16
All code
========
0: 40 84 c6 test %al,%sil
3: 0f 85 01 01 00 00 jne 0x10a
9: 84 c9 test %cl,%cl
b: 0f 95 c2 setne %dl
e: 0f 9e c0 setle %al
11: 84 c2 test %al,%dl
13: 0f 85 f1 00 00 00 jne 0x10a
19: 48 ba 00 00 00 00 00 movabs $0xdffffc0000000000,%rdx
20: fc ff df
23: 48 89 e8 mov %rbp,%rax
26: 48 c1 e8 03 shr $0x3,%rax
2a:* 0f b6 0c 10 movzbl (%rax,%rdx,1),%ecx <-- trapping instruction
2e: 48 8d 45 77 lea 0x77(%rbp),%rax
32: 48 89 c6 mov %rax,%rsi
35: 83 e0 07 and $0x7,%eax
38: 48 c1 ee 03 shr $0x3,%rsi
3c: 0f b6 14 16 movzbl (%rsi,%rdx,1),%edx
Code starting with the faulting instruction
===========================================
0: 0f b6 0c 10 movzbl (%rax,%rdx,1),%ecx
4: 48 8d 45 77 lea 0x77(%rbp),%rax
8: 48 89 c6 mov %rax,%rsi
b: 83 e0 07 and $0x7,%eax
e: 48 c1 ee 03 shr $0x3,%rsi
12: 0f b6 14 16 movzbl (%rsi,%rdx,1),%edx
[ 12.749333][ T116] RSP: 0000:ffffc900007ff548 EFLAGS: 00010246
[ 12.749749][ T116] RAX: 0000000000000000 RBX: ffff8881819c6600 RCX: 0000000000000000
[ 12.750284][ T116] RDX: dffffc0000000000 RSI: 1ffff11030338c01 RDI: ffff8881819c6648
[ 12.750820][ T116] RBP: 0000000000000000 R08: 0000000000000001 R09: 0000000000000001
[ 12.751354][ T116] R10: 0000000000000000 R11: dffffc0000000000 R12: 0000000000000000
[ 12.751889][ T116] R13: 0000000000000000 R14: 0000000000000001 R15: ffff8881819c6600
[ 12.752435][ T116] FS: 0000000000000000(0000) GS:ffff8883a8f00000(0063) knlGS:00000000f7cec740
[ 12.753057][ T116] CS: 0010 DS: 002b ES: 002b CR0: 0000000080050033
[ 12.753537][ T116] CR2: 00000000ffc27bf8 CR3: 0000000139596000 CR4: 00000000000406f0
[ 12.754112][ T116] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 12.754674][ T116] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 12.755239][ T116] Call Trace:
[ 12.755479][ T116] <TASK>
[ 12.755686][ T116] ? die_addr (kbuild/src/consumer/arch/x86/kernel/dumpstack.c:421 kbuild/src/consumer/arch/x86/kernel/dumpstack.c:460)
[ 12.755977][ T116] ? exc_general_protection (kbuild/src/consumer/arch/x86/kernel/traps.c:751 kbuild/src/consumer/arch/x86/kernel/traps.c:693)
[ 12.756372][ T116] ? asm_exc_general_protection (kbuild/src/consumer/arch/x86/include/asm/idtentry.h:617)
[ 12.756757][ T116] ? drm_mode_copy (kbuild/src/consumer/drivers/gpu/drm/drm_modes.c:1422) drm
The kernel config and materials to reproduce are available at:
https://download.01.org/0day-ci/archive/20241009/202410091649.1353a717-oliver.sang@intel.com
On Tue, Oct 08, 2024 at 10:33:44PM +0300, Ville Syrjälä wrote: > On Mon, Oct 07, 2024 at 09:36:13AM +0200, Thomas Zimmermann wrote: > > Hi > > > > Am 03.10.24 um 13:33 schrieb Ville Syrjala: > > > From: Ville Syrjälä <ville.syrjala@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(). > > My first thought was to make copies but still keep track > of pointers. That idea was a complete disaster because you > now had to carefully free the modes on the list. > > I then considred some kind of double list approach, but that > too seemed more complicated/confusing than the (IMO fairly > straightforward) apporach I ended up with. I'd prefer to reduce > the nummber of arrays this thing uses rather than increase them. Had another look at the double array approach, and still tought the result would be quite disgusting. So I think the only other viable option is to keep the single array of pointers, and stick copies onto it. But that introduces more ways to leak memory and/or access already freed memory. I don't really like the extra complexity that this requires. It'd perhaps be more palatable if the whole thing would be redesigned to be more AoS instead of SoA...
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; +} + 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])) { 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: