Hi all,
Dave&I have been discussing connector hotplug and unplugging around DP MST and if there's one thing that's clear it's that we don't even really know where all the problems are. Hence first step is to figure that out. One of the bigger items is walking the encoder/connector lists without appropriate locking to protect against concurrent hotadd/removal of connectors.
This patch series tries to untangle things a bit here. RFC since only lightly tested and missing conversion of the radoen mst code to the new locking scheme. I think rolling out the new macros for i915 should be done as a second step, to avoid hitting too many WARN_ON ;-)
Cheers, Daniel
Daniel Vetter (11): drm: Simplify drm_for_each_legacy_plane arguments drm: Add modeset object iterators drm/probe-helper: Grab mode_config.mutex in poll_init/enable drm/fbdev-helper: Grab mode_config.mutex in drm_fb_helper_single_add_all_connectors drm: Check locking in drm_for_each_encoder/connector drm/i915: Use drm_for_each_fb in i915_debugfs.c drm: Check locking in drm_for_each_fb drm/i915: Take all modeset locks for DP MST hotplug drm: Amend connector/encoder list locking rules drm: Roll out drm_for_each_connector more drm: Roll out drm_for_each_{plane,crtc,encoder}
drivers/gpu/drm/drm_atomic.c | 2 +- drivers/gpu/drm/drm_atomic_helper.c | 4 +-- drivers/gpu/drm/drm_crtc.c | 56 +++++++++++++++---------------- drivers/gpu/drm/drm_crtc_helper.c | 42 +++++++++++------------ drivers/gpu/drm/drm_edid.c | 2 +- drivers/gpu/drm/drm_fb_cma_helper.c | 2 +- drivers/gpu/drm/drm_fb_helper.c | 19 +++++++---- drivers/gpu/drm/drm_modeset_lock.c | 7 ++-- drivers/gpu/drm/drm_of.c | 2 +- drivers/gpu/drm/drm_plane_helper.c | 3 +- drivers/gpu/drm/drm_probe_helper.c | 45 +++++++++++++++---------- drivers/gpu/drm/i915/i915_debugfs.c | 4 ++- drivers/gpu/drm/i915/intel_dp_mst.c | 12 +++---- drivers/gpu/drm/i915/intel_pm.c | 2 +- drivers/gpu/drm/shmobile/shmob_drm_crtc.c | 2 +- include/drm/drm_crtc.h | 33 ++++++++++++++++-- 16 files changed, 140 insertions(+), 97 deletions(-)
No need to pass the planelist when everyone just uses dev->mode_config.plane_list anyway.
I want to add a pile more of iterators with unified (obj, dev) arguments. This is just prep.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/i915/intel_pm.c | 2 +- drivers/gpu/drm/shmobile/shmob_drm_crtc.c | 2 +- include/drm/drm_crtc.h | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 32ff034a0875..d4657868a78c 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -2084,7 +2084,7 @@ static void ilk_compute_wm_parameters(struct drm_crtc *crtc, p->pri.horiz_pixels = intel_crtc->config->pipe_src_w; p->cur.horiz_pixels = intel_crtc->base.cursor->state->crtc_w;
- drm_for_each_legacy_plane(plane, &dev->mode_config.plane_list) { + drm_for_each_legacy_plane(plane, dev) { struct intel_plane *intel_plane = to_intel_plane(plane);
if (intel_plane->pipe == pipe) { diff --git a/drivers/gpu/drm/shmobile/shmob_drm_crtc.c b/drivers/gpu/drm/shmobile/shmob_drm_crtc.c index 859ccb658601..e9272b0a8592 100644 --- a/drivers/gpu/drm/shmobile/shmob_drm_crtc.c +++ b/drivers/gpu/drm/shmobile/shmob_drm_crtc.c @@ -248,7 +248,7 @@ static void shmob_drm_crtc_start(struct shmob_drm_crtc *scrtc) lcdc_write(sdev, LDDDSR, value);
/* Setup planes. */ - drm_for_each_legacy_plane(plane, &dev->mode_config.plane_list) { + drm_for_each_legacy_plane(plane, dev) { if (plane->crtc == crtc) shmob_drm_plane_setup(plane); } diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 3b4d8a4a23fb..bc58c030ac89 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -1577,8 +1577,8 @@ static inline struct drm_property *drm_property_find(struct drm_device *dev, }
/* Plane list iterator for legacy (overlay only) planes. */ -#define drm_for_each_legacy_plane(plane, planelist) \ - list_for_each_entry(plane, planelist, head) \ +#define drm_for_each_legacy_plane(plane, dev) \ + list_for_each_entry(plane, &(dev)->mode_config.plane_list, head) \ if (plane->type == DRM_PLANE_TYPE_OVERLAY)
#endif /* __DRM_CRTC_H__ */
And roll them out across drm_* files. The point here isn't code prettification (it helps with that too) but that some of these lists aren't static any more. And having macros will gives us a convenient place to put locking checks into.
I didn't add an iterator for props since that's only used by a list_for_each_entry_safe in the driver teardown code.
Search&replace was done with the below cocci spatch. Note that there's a bunch more places that didn't match and which would need some manual changes, but I've intentially left these out for this mostly automated patch.
iterator name drm_for_each_crtc; struct drm_crtc *crtc; struct drm_device *dev; expression head; @@ - list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { + drm_for_each_crtc (crtc, dev) { ... }
@@ iterator name drm_for_each_encoder; struct drm_encoder *encoder; struct drm_device *dev; expression head; @@ - list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) { + drm_for_each_encoder (encoder, dev) { ... }
@@ iterator name drm_for_each_fb; struct drm_framebuffer *fb; struct drm_device *dev; expression head; @@ - list_for_each_entry(fb, &dev->mode_config.fb_list, head) { + drm_for_each_fb (fb, dev) { ... }
@@ iterator name drm_for_each_connector; struct drm_connector *connector; struct drm_device *dev; expression head; @@ - list_for_each_entry(connector, &dev->mode_config.connector_list, head) { + drm_for_each_connector (connector, dev) { ... }
Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_crtc.c | 21 ++++++++------------- drivers/gpu/drm/drm_crtc_helper.c | 34 +++++++++++++++++----------------- drivers/gpu/drm/drm_fb_helper.c | 10 +++++----- drivers/gpu/drm/drm_of.c | 2 +- drivers/gpu/drm/drm_probe_helper.c | 6 +++--- include/drm/drm_crtc.h | 15 +++++++++++++++ 6 files changed, 49 insertions(+), 39 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 2d57fc56dcbf..6f572733cbdd 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -615,7 +615,7 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb) if (atomic_read(&fb->refcount.refcount) > 1) { drm_modeset_lock_all(dev); /* remove from any CRTC */ - list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { + drm_for_each_crtc(crtc, dev) { if (crtc->primary->fb == fb) { /* should turn off the crtc */ memset(&set, 0, sizeof(struct drm_mode_set)); @@ -627,7 +627,7 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb) } }
- list_for_each_entry(plane, &dev->mode_config.plane_list, head) { + drm_for_each_plane(plane, dev) { if (plane->fb == fb) drm_plane_force_disable(plane); } @@ -1305,7 +1305,7 @@ drm_plane_from_index(struct drm_device *dev, int idx) struct drm_plane *plane; unsigned int i = 0;
- list_for_each_entry(plane, &dev->mode_config.plane_list, head) { + drm_for_each_plane(plane, dev) { if (i == idx) return plane; i++; @@ -1838,8 +1838,7 @@ int drm_mode_getresources(struct drm_device *dev, void *data, copied = 0; crtc_id = (uint32_t __user *)(unsigned long)card_res->crtc_id_ptr; if (!mode_group) { - list_for_each_entry(crtc, &dev->mode_config.crtc_list, - head) { + drm_for_each_crtc(crtc, dev) { DRM_DEBUG_KMS("[CRTC:%d]\n", crtc->base.id); if (put_user(crtc->base.id, crtc_id + copied)) { ret = -EFAULT; @@ -1865,9 +1864,7 @@ int drm_mode_getresources(struct drm_device *dev, void *data, copied = 0; encoder_id = (uint32_t __user *)(unsigned long)card_res->encoder_id_ptr; if (!mode_group) { - list_for_each_entry(encoder, - &dev->mode_config.encoder_list, - head) { + drm_for_each_encoder(encoder, dev) { DRM_DEBUG_KMS("[ENCODER:%d:%s]\n", encoder->base.id, encoder->name); if (put_user(encoder->base.id, encoder_id + @@ -1896,9 +1893,7 @@ int drm_mode_getresources(struct drm_device *dev, void *data, copied = 0; connector_id = (uint32_t __user *)(unsigned long)card_res->connector_id_ptr; if (!mode_group) { - list_for_each_entry(connector, - &dev->mode_config.connector_list, - head) { + drm_for_each_connector(connector, dev) { DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", connector->base.id, connector->name); @@ -2187,7 +2182,7 @@ static struct drm_crtc *drm_encoder_get_crtc(struct drm_encoder *encoder)
/* For atomic drivers only state objects are synchronously updated and * protected by modeset locks, so check those first. */ - list_for_each_entry(connector, &dev->mode_config.connector_list, head) { + drm_for_each_connector(connector, dev) { if (!connector->state) continue;
@@ -5389,7 +5384,7 @@ void drm_mode_config_reset(struct drm_device *dev) if (encoder->funcs->reset) encoder->funcs->reset(encoder);
- list_for_each_entry(connector, &dev->mode_config.connector_list, head) { + drm_for_each_connector(connector, dev) { connector->status = connector_status_unknown;
if (connector->funcs->reset) diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c index 393114df88a3..30254fb249fe 100644 --- a/drivers/gpu/drm/drm_crtc_helper.c +++ b/drivers/gpu/drm/drm_crtc_helper.c @@ -180,7 +180,7 @@ static void __drm_helper_disable_unused_functions(struct drm_device *dev)
drm_warn_on_modeset_not_all_locked(dev);
- list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) { + drm_for_each_encoder(encoder, dev) { if (!drm_helper_encoder_in_use(encoder)) { drm_encoder_disable(encoder); /* disconnect encoder from any connector */ @@ -188,7 +188,7 @@ static void __drm_helper_disable_unused_functions(struct drm_device *dev) } }
- list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { + drm_for_each_crtc(crtc, dev) { const struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private; crtc->enabled = drm_helper_crtc_in_use(crtc); if (!crtc->enabled) { @@ -230,7 +230,7 @@ drm_crtc_prepare_encoders(struct drm_device *dev) const struct drm_encoder_helper_funcs *encoder_funcs; struct drm_encoder *encoder;
- list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) { + drm_for_each_encoder(encoder, dev) { encoder_funcs = encoder->helper_private; /* Disable unused encoders */ if (encoder->crtc == NULL) @@ -305,7 +305,7 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, * adjust it according to limitations or connector properties, and also * a chance to reject the mode entirely. */ - list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) { + drm_for_each_encoder(encoder, dev) {
if (encoder->crtc != crtc) continue; @@ -334,7 +334,7 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, crtc->hwmode = *adjusted_mode;
/* Prepare the encoders and CRTCs before setting the mode. */ - list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) { + drm_for_each_encoder(encoder, dev) {
if (encoder->crtc != crtc) continue; @@ -359,7 +359,7 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, if (!ret) goto done;
- list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) { + drm_for_each_encoder(encoder, dev) {
if (encoder->crtc != crtc) continue; @@ -376,7 +376,7 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, /* Now enable the clocks, plane, pipe, and connectors that we set up. */ crtc_funcs->commit(crtc);
- list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) { + drm_for_each_encoder(encoder, dev) {
if (encoder->crtc != crtc) continue; @@ -418,11 +418,11 @@ drm_crtc_helper_disable(struct drm_crtc *crtc) struct drm_encoder *encoder;
/* Decouple all encoders and their attached connectors from this crtc */ - list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) { + drm_for_each_encoder(encoder, dev) { if (encoder->crtc != crtc) continue;
- list_for_each_entry(connector, &dev->mode_config.connector_list, head) { + drm_for_each_connector(connector, dev) { if (connector->encoder != encoder) continue;
@@ -519,12 +519,12 @@ int drm_crtc_helper_set_config(struct drm_mode_set *set) * restored, not the drivers personal bookkeeping. */ count = 0; - list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) { + drm_for_each_encoder(encoder, dev) { save_encoders[count++] = *encoder; }
count = 0; - list_for_each_entry(connector, &dev->mode_config.connector_list, head) { + drm_for_each_connector(connector, dev) { save_connectors[count++] = *connector; }
@@ -562,7 +562,7 @@ int drm_crtc_helper_set_config(struct drm_mode_set *set)
/* a) traverse passed in connector list and get encoders for them */ count = 0; - list_for_each_entry(connector, &dev->mode_config.connector_list, head) { + drm_for_each_connector(connector, dev) { const struct drm_connector_helper_funcs *connector_funcs = connector->helper_private; new_encoder = connector->encoder; @@ -602,7 +602,7 @@ int drm_crtc_helper_set_config(struct drm_mode_set *set) }
count = 0; - list_for_each_entry(connector, &dev->mode_config.connector_list, head) { + drm_for_each_connector(connector, dev) { if (!connector->encoder) continue;
@@ -685,12 +685,12 @@ int drm_crtc_helper_set_config(struct drm_mode_set *set) fail: /* Restore all previous data. */ count = 0; - list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) { + drm_for_each_encoder(encoder, dev) { *encoder = save_encoders[count++]; }
count = 0; - list_for_each_entry(connector, &dev->mode_config.connector_list, head) { + drm_for_each_connector(connector, dev) { *connector = save_connectors[count++]; }
@@ -862,7 +862,7 @@ void drm_helper_resume_force_mode(struct drm_device *dev) bool ret;
drm_modeset_lock_all(dev); - list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { + drm_for_each_crtc(crtc, dev) {
if (!crtc->enabled) continue; @@ -876,7 +876,7 @@ void drm_helper_resume_force_mode(struct drm_device *dev)
/* Turn off outputs that were already powered off */ if (drm_helper_choose_crtc_dpms(crtc)) { - list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) { + drm_for_each_encoder(encoder, dev) {
if(encoder->crtc != crtc) continue; diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index cac422916c7a..3630d92c9738 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -98,7 +98,7 @@ int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper *fb_helper) struct drm_connector *connector; int i;
- list_for_each_entry(connector, &dev->mode_config.connector_list, head) { + drm_for_each_connector(connector, dev) { struct drm_fb_helper_connector *fb_helper_connector;
fb_helper_connector = kzalloc(sizeof(struct drm_fb_helper_connector), GFP_KERNEL); @@ -269,7 +269,7 @@ static struct drm_framebuffer *drm_mode_config_fb(struct drm_crtc *crtc) struct drm_device *dev = crtc->dev; struct drm_crtc *c;
- list_for_each_entry(c, &dev->mode_config.crtc_list, head) { + drm_for_each_crtc(c, dev) { if (crtc->base.id == c->base.id) return c->primary->fb; } @@ -321,7 +321,7 @@ static bool restore_fbdev_mode(struct drm_fb_helper *fb_helper)
drm_warn_on_modeset_not_all_locked(dev);
- list_for_each_entry(plane, &dev->mode_config.plane_list, head) { + drm_for_each_plane(plane, dev) { if (plane->type != DRM_PLANE_TYPE_PRIMARY) drm_plane_force_disable(plane);
@@ -458,7 +458,7 @@ static bool drm_fb_helper_is_bound(struct drm_fb_helper *fb_helper) if (dev->primary->master) return false;
- list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { + drm_for_each_crtc(crtc, dev) { if (crtc->primary->fb) crtcs_bound++; if (crtc->primary->fb == fb_helper->fb) @@ -655,7 +655,7 @@ int drm_fb_helper_init(struct drm_device *dev, }
i = 0; - list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { + drm_for_each_crtc(crtc, dev) { fb_helper->crtc_info[i].mode_set.crtc = crtc; i++; } diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c index aaa130736bf8..be3884073ea4 100644 --- a/drivers/gpu/drm/drm_of.c +++ b/drivers/gpu/drm/drm_of.c @@ -19,7 +19,7 @@ static uint32_t drm_crtc_port_mask(struct drm_device *dev, unsigned int index = 0; struct drm_crtc *tmp;
- list_for_each_entry(tmp, &dev->mode_config.crtc_list, head) { + drm_for_each_crtc(tmp, dev) { if (tmp->port == port) return 1 << index;
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c index 04203c0d2ecb..64d85c1bd18b 100644 --- a/drivers/gpu/drm/drm_probe_helper.c +++ b/drivers/gpu/drm/drm_probe_helper.c @@ -312,7 +312,7 @@ static void output_poll_execute(struct work_struct *work) goto out;
mutex_lock(&dev->mode_config.mutex); - list_for_each_entry(connector, &dev->mode_config.connector_list, head) { + drm_for_each_connector(connector, dev) {
/* Ignore forced connectors. */ if (connector->force) @@ -413,7 +413,7 @@ void drm_kms_helper_poll_enable(struct drm_device *dev) if (!dev->mode_config.poll_enabled || !drm_kms_helper_poll) return;
- list_for_each_entry(connector, &dev->mode_config.connector_list, head) { + drm_for_each_connector(connector, dev) { if (connector->polled & (DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT)) poll = true; @@ -495,7 +495,7 @@ bool drm_helper_hpd_irq_event(struct drm_device *dev) return false;
mutex_lock(&dev->mode_config.mutex); - list_for_each_entry(connector, &dev->mode_config.connector_list, head) { + drm_for_each_connector(connector, dev) {
/* Only handle HPD capable connectors. */ if (!(connector->polled & DRM_CONNECTOR_POLL_HPD)) diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index bc58c030ac89..5994750f523a 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -1581,4 +1581,19 @@ static inline struct drm_property *drm_property_find(struct drm_device *dev, list_for_each_entry(plane, &(dev)->mode_config.plane_list, head) \ if (plane->type == DRM_PLANE_TYPE_OVERLAY)
+#define drm_for_each_plane(plane, dev) \ + list_for_each_entry(plane, &(dev)->mode_config.plane_list, head) + +#define drm_for_each_crtc(crtc, dev) \ + list_for_each_entry(crtc, &(dev)->mode_config.crtc_list, head) + +#define drm_for_each_connector(connector, dev) \ + list_for_each_entry(connector, &(dev)->mode_config.connector_list, head) + +#define drm_for_each_encoder(encoder, dev) \ + list_for_each_entry(encoder, &(dev)->mode_config.encoder_list, head) + +#define drm_for_each_fb(fb, dev) \ + list_for_each_entry(fb, &(dev)->mode_config.fb_list, head) + #endif /* __DRM_CRTC_H__ */
So on first looks this seems superflous since drivers should ensure correct ordering to not make this a problem. Otoh ordering constraints between hdp, fbdev load and enabling polling are already tricky on some hardware and it helps to be more robust.
But the real goal is to just shut up a locking WARN_ON I'd like to add, which means init code gets some additional locks just for uniformity.
v2: Also grab the lock for the public poll_enable, not just poll_init which is used for resume, with the same justification.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_probe_helper.c | 41 +++++++++++++++++++++++--------------- 1 file changed, 25 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c index 64d85c1bd18b..d734780b31c0 100644 --- a/drivers/gpu/drm/drm_probe_helper.c +++ b/drivers/gpu/drm/drm_probe_helper.c @@ -93,6 +93,27 @@ static int drm_helper_probe_add_cmdline_mode(struct drm_connector *connector) return 1; }
+#define DRM_OUTPUT_POLL_PERIOD (10*HZ) +static void __drm_kms_helper_poll_enable(struct drm_device *dev) +{ + bool poll = false; + struct drm_connector *connector; + + WARN_ON(!mutex_is_locked(&dev->mode_config.mutex)); + + if (!dev->mode_config.poll_enabled || !drm_kms_helper_poll) + return; + + drm_for_each_connector(connector, dev) { + if (connector->polled & (DRM_CONNECTOR_POLL_CONNECT | + DRM_CONNECTOR_POLL_DISCONNECT)) + poll = true; + } + + if (poll) + schedule_delayed_work(&dev->mode_config.output_poll_work, DRM_OUTPUT_POLL_PERIOD); +} + static int drm_helper_probe_single_connector_modes_merge_bits(struct drm_connector *connector, uint32_t maxX, uint32_t maxY, bool merge_type_bits) { @@ -153,7 +174,7 @@ static int drm_helper_probe_single_connector_modes_merge_bits(struct drm_connect
/* Re-enable polling in case the global poll config changed. */ if (drm_kms_helper_poll != dev->mode_config.poll_running) - drm_kms_helper_poll_enable(dev); + __drm_kms_helper_poll_enable(dev);
dev->mode_config.poll_running = drm_kms_helper_poll;
@@ -295,7 +316,6 @@ void drm_kms_helper_hotplug_event(struct drm_device *dev) } EXPORT_SYMBOL(drm_kms_helper_hotplug_event);
-#define DRM_OUTPUT_POLL_PERIOD (10*HZ) static void output_poll_execute(struct work_struct *work) { struct delayed_work *delayed_work = to_delayed_work(work); @@ -407,20 +427,9 @@ EXPORT_SYMBOL(drm_kms_helper_poll_disable); */ void drm_kms_helper_poll_enable(struct drm_device *dev) { - bool poll = false; - struct drm_connector *connector; - - if (!dev->mode_config.poll_enabled || !drm_kms_helper_poll) - return; - - drm_for_each_connector(connector, dev) { - if (connector->polled & (DRM_CONNECTOR_POLL_CONNECT | - DRM_CONNECTOR_POLL_DISCONNECT)) - poll = true; - } - - if (poll) - schedule_delayed_work(&dev->mode_config.output_poll_work, DRM_OUTPUT_POLL_PERIOD); + mutex_lock(&dev->mode_config.mutex); + __drm_kms_helper_poll_enable(dev); + mutex_unlock(&dev->mode_config.mutex); } EXPORT_SYMBOL(drm_kms_helper_poll_enable);
This is now truly only duct-tape to keep locking checks happy since calling this function when hpd or polling are already enabled is a bug. The fbdev helper can't cope with hotplug changes yet at this point, only after that.
Otoh a bit more robustness in this function can't hurt, and with this fbdev can actually cope with hotplug changes. And it's also more consistent with the connector hotadd/remove dp mst needs to do. Therefore document this as new official behavior.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_fb_helper.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 3630d92c9738..329d08167b77 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -89,8 +89,9 @@ static LIST_HEAD(kernel_fb_helper_list); * connectors to the fbdev, e.g. if some are reserved for special purposes or * not adequate to be used for the fbcon. * - * Since this is part of the initial setup before the fbdev is published, no - * locking is required. + * This function is protected against concurrent connector hotadds/removals + * using drm_fb_helper_add_one_connector() and + * drm_fb_helper_remove_one_connector(). */ int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper *fb_helper) { @@ -98,6 +99,7 @@ int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper *fb_helper) struct drm_connector *connector; int i;
+ mutex_lock(&dev->mode_config.mutex); drm_for_each_connector(connector, dev) { struct drm_fb_helper_connector *fb_helper_connector;
@@ -108,6 +110,7 @@ int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper *fb_helper) fb_helper_connector->connector = connector; fb_helper->connector_info[fb_helper->connector_count++] = fb_helper_connector; } + mutex_unlock(&dev->mode_config.mutex); return 0; fail: for (i = 0; i < fb_helper->connector_count; i++) { @@ -115,6 +118,8 @@ fail: fb_helper->connector_info[i] = NULL; } fb_helper->connector_count = 0; + mutex_unlock(&dev->mode_config.mutex); + return -ENOMEM; } EXPORT_SYMBOL(drm_fb_helper_single_add_all_connectors);
Because of DP MST encoders/connectors can now be hotplugged and we must hold the right lock when walking the encoder/connector lists. Enforce this by checking the locking in our shiny new list walking macros.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- include/drm/drm_crtc.h | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 5994750f523a..a912ba06cf45 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -1588,10 +1588,18 @@ static inline struct drm_property *drm_property_find(struct drm_device *dev, list_for_each_entry(crtc, &(dev)->mode_config.crtc_list, head)
#define drm_for_each_connector(connector, dev) \ - list_for_each_entry(connector, &(dev)->mode_config.connector_list, head) + for (WARN_ON(!mutex_is_locked(&(dev)->mode_config.mutex)), \ + connector = list_first_entry(&(dev)->mode_config.connector_list, \ + struct drm_connector, head); \ + &connector->head != (&(dev)->mode_config.connector_list); \ + connector = list_next_entry(connector, head))
#define drm_for_each_encoder(encoder, dev) \ - list_for_each_entry(encoder, &(dev)->mode_config.encoder_list, head) + for (WARN_ON(!mutex_is_locked(&(dev)->mode_config.mutex)), \ + encoder = list_first_entry(&(dev)->mode_config.encoder_list, \ + struct drm_encoder, head); \ + &encoder->head != (&(dev)->mode_config.encoder_list); \ + encoder = list_next_entry(encoder, head))
#define drm_for_each_fb(fb, dev) \ list_for_each_entry(fb, &(dev)->mode_config.fb_list, head)
Just so I have a user for this macro.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/i915/i915_debugfs.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index c49fe2afa223..6db920c913ee 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -1815,6 +1815,7 @@ static int i915_gem_framebuffer_info(struct seq_file *m, void *data) struct drm_device *dev = node->minor->dev; struct intel_fbdev *ifbdev = NULL; struct intel_framebuffer *fb; + struct drm_framebuffer *drm_fb;
#ifdef CONFIG_DRM_I915_FBDEV struct drm_i915_private *dev_priv = dev->dev_private; @@ -1834,7 +1835,8 @@ static int i915_gem_framebuffer_info(struct seq_file *m, void *data) #endif
mutex_lock(&dev->mode_config.fb_lock); - list_for_each_entry(fb, &dev->mode_config.fb_list, base.head) { + drm_for_each_plane(drm_fb, dev) { + fb = to_intel_framebuffer(drm_fb); if (ifbdev && &fb->base == ifbdev->helper.fb) continue;
Ever since framebuffers are reference counted we have a special lock for the global fb list. Make sure users of that list do hold that lock when using the new iterators.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- include/drm/drm_crtc.h | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index a912ba06cf45..3646c47b43de 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -1602,6 +1602,10 @@ static inline struct drm_property *drm_property_find(struct drm_device *dev, encoder = list_next_entry(encoder, head))
#define drm_for_each_fb(fb, dev) \ - list_for_each_entry(fb, &(dev)->mode_config.fb_list, head) + for (WARN_ON(!mutex_is_locked(&(dev)->mode_config.fb_lock)), \ + fb = list_first_entry(&(dev)->mode_config.fb_list, \ + struct drm_framebuffer, head); \ + &fb->head != (&(dev)->mode_config.fb_list); \ + fb = list_next_entry(fb, head))
#endif /* __DRM_CRTC_H__ */
While auditing various users of the connector/encoder lists I realized that the atomic code is a very prolific user of them. And it only ever grabs the mode_config->connection_mutex, but not the mode_config->mutex like all the other code walking encoder/connector lists.
The problem is that we can't grab the mode_config.mutex late in atomic code since that would lead to locking inversions. And we don't want to grab it unconditionally like the legacy set_config modeset path since that would render all the fine-grained locking moot.
Instead just grab more locks in the dp mst hotplug code. Note that drm_connector_init (which is the one adding the connector to these lists) already uses drm_modeset_lock_all.
The other reason for grabbing all locks is that the dpms off in the unplug function amounts to a modeset, so better to take all required locks for that.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/i915/intel_dp_mst.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c index 6e4cc5334f47..d0b2569c6241 100644 --- a/drivers/gpu/drm/i915/intel_dp_mst.c +++ b/drivers/gpu/drm/i915/intel_dp_mst.c @@ -442,9 +442,9 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
drm_mode_connector_set_path_property(connector, pathprop); drm_reinit_primary_mode_group(dev); - mutex_lock(&dev->mode_config.mutex); + drm_modeset_lock_all(dev); intel_connector_add_to_fbdev(intel_connector); - mutex_unlock(&dev->mode_config.mutex); + drm_modeset_unlock_all(dev); drm_connector_register(&intel_connector->base); return connector; } @@ -455,16 +455,16 @@ static void intel_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr, struct intel_connector *intel_connector = to_intel_connector(connector); struct drm_device *dev = connector->dev; /* need to nuke the connector */ - mutex_lock(&dev->mode_config.mutex); + drm_modeset_lock_all(dev); intel_connector_dpms(connector, DRM_MODE_DPMS_OFF); - mutex_unlock(&dev->mode_config.mutex); + drm_modeset_unlock_all(dev);
intel_connector->unregister(intel_connector);
- mutex_lock(&dev->mode_config.mutex); + drm_modeset_lock_all(dev); intel_connector_remove_from_fbdev(intel_connector); drm_connector_cleanup(connector); - mutex_unlock(&dev->mode_config.mutex); + drm_modeset_unlock_all(dev);
drm_reinit_primary_mode_group(dev);
Now that dp mst hotplug takes all locks we can amend the locking rules for the iterators. This is needed before we can roll these out in the atomic code to avoid getting burried in WARNINGs.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- include/drm/drm_crtc.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 3646c47b43de..c1c4f16f01ca 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -1588,14 +1588,16 @@ static inline struct drm_property *drm_property_find(struct drm_device *dev, list_for_each_entry(crtc, &(dev)->mode_config.crtc_list, head)
#define drm_for_each_connector(connector, dev) \ - for (WARN_ON(!mutex_is_locked(&(dev)->mode_config.mutex)), \ + for (WARN_ON(!mutex_is_locked(&(dev)->mode_config.mutex) && \ + !drm_modeset_is_locked(&(dev)->mode_config.connection_mutex)), \ connector = list_first_entry(&(dev)->mode_config.connector_list, \ struct drm_connector, head); \ &connector->head != (&(dev)->mode_config.connector_list); \ connector = list_next_entry(connector, head))
#define drm_for_each_encoder(encoder, dev) \ - for (WARN_ON(!mutex_is_locked(&(dev)->mode_config.mutex)), \ + for (WARN_ON(!mutex_is_locked(&(dev)->mode_config.mutex) && \ + !drm_modeset_is_locked(&(dev)->mode_config.connection_mutex)), \ encoder = list_first_entry(&(dev)->mode_config.encoder_list, \ struct drm_encoder, head); \ &encoder->head != (&(dev)->mode_config.encoder_list); \
On Tue, Jun 23, 2015 at 10:46:00PM +0200, Daniel Vetter wrote:
Now that dp mst hotplug takes all locks we can amend the locking rules for the iterators. This is needed before we can roll these out in the atomic code to avoid getting burried in WARNINGs.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com
include/drm/drm_crtc.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 3646c47b43de..c1c4f16f01ca 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -1588,14 +1588,16 @@ static inline struct drm_property *drm_property_find(struct drm_device *dev, list_for_each_entry(crtc, &(dev)->mode_config.crtc_list, head)
#define drm_for_each_connector(connector, dev) \
- for (WARN_ON(!mutex_is_locked(&(dev)->mode_config.mutex)), \
- for (WARN_ON(!mutex_is_locked(&(dev)->mode_config.mutex) && \
connector = list_first_entry(&(dev)->mode_config.connector_list, \ struct drm_connector, head); \ &connector->head != (&(dev)->mode_config.connector_list); \ connector = list_next_entry(connector, head))!drm_modeset_is_locked(&(dev)->mode_config.connection_mutex)), \
#define drm_for_each_encoder(encoder, dev) \
- for (WARN_ON(!mutex_is_locked(&(dev)->mode_config.mutex)), \
- for (WARN_ON(!mutex_is_locked(&(dev)->mode_config.mutex) && \
encoder = list_first_entry(&(dev)->mode_config.encoder_list, \ struct drm_encoder, head); \ &encoder->head != (&(dev)->mode_config.encoder_list); \!drm_modeset_is_locked(&(dev)->mode_config.connection_mutex)), \
Maybe something like:
// not sure how specifc each lock will end up being static inline void assert_drm_mode_list_locked(dev) { /* lockdep for accuracy and verbose debug reportts, * WARN_ON for warnings everywhere. */ lockdep_assert_held(&dev->mode_config.mutex); WARN_ON(!(mutex_is_locked(&dev->mode_config.mutex) && drm_modeset_is_locked(&dev->mode_config.connection_mutex)));
// Daniel, it is not clear from context whether you mean // !a && !b, or !(a && b) }
static inline struct drm_connector *drm_first_connector(dev) { assert_drm_mode_list_locked(dev); return list_first_entry(&dev->mode_config.connector_list, struct drm_connector, head); }
#define drm_for_each_connector(connector, dev) \ for (connector = drm_first_connector(dev); \ &connector->head != (&(dev)->mode_config.connector_list); \ connector = list_next_entry(connector, head))
static inline struct drm_encoder *drm_first_encoder(dev) { assert_drm_mode_list_locked(dev); return list_first_entry(&dev->mode_config.encoder_list, struct drm_encoder, head); }
#define drm_for_each_encoder(encoder, dev) \ for (encoder = drm_first_encoder(dev); \ &encoder->head != (&(dev)->mode_config.encoder_list); \ encoder = list_next_entry(encoder, head))
Usual arguments of reusing code for clarity. -Chris
On Wed, Jun 24, 2015 at 08:57:23AM +0100, Chris Wilson wrote:
On Tue, Jun 23, 2015 at 10:46:00PM +0200, Daniel Vetter wrote:
Now that dp mst hotplug takes all locks we can amend the locking rules for the iterators. This is needed before we can roll these out in the atomic code to avoid getting burried in WARNINGs.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com
include/drm/drm_crtc.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 3646c47b43de..c1c4f16f01ca 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -1588,14 +1588,16 @@ static inline struct drm_property *drm_property_find(struct drm_device *dev, list_for_each_entry(crtc, &(dev)->mode_config.crtc_list, head)
#define drm_for_each_connector(connector, dev) \
- for (WARN_ON(!mutex_is_locked(&(dev)->mode_config.mutex)), \
- for (WARN_ON(!mutex_is_locked(&(dev)->mode_config.mutex) && \
connector = list_first_entry(&(dev)->mode_config.connector_list, \ struct drm_connector, head); \ &connector->head != (&(dev)->mode_config.connector_list); \ connector = list_next_entry(connector, head))!drm_modeset_is_locked(&(dev)->mode_config.connection_mutex)), \
#define drm_for_each_encoder(encoder, dev) \
- for (WARN_ON(!mutex_is_locked(&(dev)->mode_config.mutex)), \
- for (WARN_ON(!mutex_is_locked(&(dev)->mode_config.mutex) && \
encoder = list_first_entry(&(dev)->mode_config.encoder_list, \ struct drm_encoder, head); \ &encoder->head != (&(dev)->mode_config.encoder_list); \!drm_modeset_is_locked(&(dev)->mode_config.connection_mutex)), \
Maybe something like:
// not sure how specifc each lock will end up being static inline void assert_drm_mode_list_locked(dev) { /* lockdep for accuracy and verbose debug reportts, * WARN_ON for warnings everywhere. */ lockdep_assert_held(&dev->mode_config.mutex); WARN_ON(!(mutex_is_locked(&dev->mode_config.mutex) && drm_modeset_is_locked(&dev->mode_config.connection_mutex)));
// Daniel, it is not clear from context whether you mean // !a && !b, or !(a && b)
Write access holds both locks, which means for read access it's ok to just hold one. But since that's caused confusions I fully agree that extracting this into a static inline and commenting properly is a good idea. -Daniel
}
static inline struct drm_connector *drm_first_connector(dev) { assert_drm_mode_list_locked(dev); return list_first_entry(&dev->mode_config.connector_list, struct drm_connector, head); }
#define drm_for_each_connector(connector, dev) \ for (connector = drm_first_connector(dev); \ &connector->head != (&(dev)->mode_config.connector_list); \ connector = list_next_entry(connector, head))
static inline struct drm_encoder *drm_first_encoder(dev) { assert_drm_mode_list_locked(dev); return list_first_entry(&dev->mode_config.encoder_list, struct drm_encoder, head); }
#define drm_for_each_encoder(encoder, dev) \ for (encoder = drm_first_encoder(dev); \ &encoder->head != (&(dev)->mode_config.encoder_list); \ encoder = list_next_entry(encoder, head))
Usual arguments of reusing code for clarity. -Chris
-- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Now that we also grab the connection_mutex and so fixed the race with atomic modeset we can use the iterator there too.
The other special case is drm_connector_unplug_all which would have a locking inversion with the sysfs store/show functions if we'd grab the mode_config.mutex around the unplug. We could just grab connection_mutex instead, but that's a bit too much a dirty trick for my taste. Also it's only used by udl, which doesn't do any other kind of connector hotplugging, so should be race-free. Hence just stick with a comment for now.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_atomic.c | 2 +- drivers/gpu/drm/drm_atomic_helper.c | 4 ++-- drivers/gpu/drm/drm_crtc.c | 9 +++++---- drivers/gpu/drm/drm_crtc_helper.c | 6 +++--- drivers/gpu/drm/drm_edid.c | 2 +- drivers/gpu/drm/drm_plane_helper.c | 3 ++- 6 files changed, 14 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index f6f2fb58eb37..dfc8aaa1457e 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1063,7 +1063,7 @@ drm_atomic_add_affected_connectors(struct drm_atomic_state *state, * Changed connectors are already in @state, so only need to look at the * current configuration. */ - list_for_each_entry(connector, &config->connector_list, head) { + drm_for_each_connector(connector, state->dev) { if (connector->state->crtc != crtc) continue;
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 0898afbc9e23..91ad6bd13734 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -89,7 +89,7 @@ get_current_crtc_for_encoder(struct drm_device *dev,
WARN_ON(!drm_modeset_is_locked(&config->connection_mutex));
- list_for_each_entry(connector, &config->connector_list, head) { + drm_for_each_connector(connector, dev) { if (connector->state->best_encoder != encoder) continue;
@@ -1982,7 +1982,7 @@ retry:
WARN_ON(!drm_modeset_is_locked(&config->connection_mutex));
- list_for_each_entry(tmp_connector, &config->connector_list, head) { + drm_for_each_connector(tmp_connector, connector->dev) { if (tmp_connector->state->crtc != crtc) continue;
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 6f572733cbdd..9c1440b2d84c 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -988,7 +988,7 @@ unsigned int drm_connector_index(struct drm_connector *connector)
WARN_ON(!drm_modeset_is_locked(&config->connection_mutex));
- list_for_each_entry(tmp, &connector->dev->mode_config.connector_list, head) { + drm_for_each_connector(tmp, connector->dev) { if (tmp == connector) return index;
@@ -1054,7 +1054,7 @@ void drm_connector_unplug_all(struct drm_device *dev) { struct drm_connector *connector;
- /* taking the mode config mutex ends up in a clash with sysfs */ + /* FIXME: taking the mode config mutex ends up in a clash with sysfs */ list_for_each_entry(connector, &dev->mode_config.connector_list, head) drm_connector_unregister(connector);
@@ -1726,7 +1726,7 @@ int drm_mode_group_init_legacy_group(struct drm_device *dev, group->id_list[group->num_crtcs + group->num_encoders++] = encoder->base.id;
- list_for_each_entry(connector, &dev->mode_config.connector_list, head) + drm_for_each_connector(connector, dev) group->id_list[group->num_crtcs + group->num_encoders + group->num_connectors++] = connector->base.id;
@@ -1810,12 +1810,13 @@ int drm_mode_getresources(struct drm_device *dev, void *data, * connector hot-adding. CRTC/Plane lists are invariant. */ mutex_lock(&dev->mode_config.mutex); if (!drm_is_primary_client(file_priv)) { + struct drm_connector *connector;
mode_group = NULL; list_for_each(lh, &dev->mode_config.crtc_list) crtc_count++;
- list_for_each(lh, &dev->mode_config.connector_list) + drm_for_each_connector(connector, dev) connector_count++;
list_for_each(lh, &dev->mode_config.encoder_list) diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c index 30254fb249fe..4a83c22f5e61 100644 --- a/drivers/gpu/drm/drm_crtc_helper.c +++ b/drivers/gpu/drm/drm_crtc_helper.c @@ -121,7 +121,7 @@ bool drm_helper_encoder_in_use(struct drm_encoder *encoder) WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex)); }
- list_for_each_entry(connector, &dev->mode_config.connector_list, head) + drm_for_each_connector(connector, dev) if (connector->encoder == encoder) return true; return false; @@ -712,7 +712,7 @@ static int drm_helper_choose_encoder_dpms(struct drm_encoder *encoder) struct drm_connector *connector; struct drm_device *dev = encoder->dev;
- list_for_each_entry(connector, &dev->mode_config.connector_list, head) + drm_for_each_connector(connector, dev) if (connector->encoder == encoder) if (connector->dpms < dpms) dpms = connector->dpms; @@ -746,7 +746,7 @@ static int drm_helper_choose_crtc_dpms(struct drm_crtc *crtc) struct drm_connector *connector; struct drm_device *dev = crtc->dev;
- list_for_each_entry(connector, &dev->mode_config.connector_list, head) + drm_for_each_connector(connector, dev) if (connector->encoder && connector->encoder->crtc == crtc) if (connector->dpms < dpms) dpms = connector->dpms; diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 7087da37dae0..e6e05bb75a77 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -3413,7 +3413,7 @@ struct drm_connector *drm_select_eld(struct drm_encoder *encoder, WARN_ON(!mutex_is_locked(&dev->mode_config.mutex)); WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
- list_for_each_entry(connector, &dev->mode_config.connector_list, head) + drm_for_each_connector(connector, dev) if (connector->encoder == encoder && connector->eld[0]) return connector;
diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c index 2f0ed11024eb..7e1cde803bf3 100644 --- a/drivers/gpu/drm/drm_plane_helper.c +++ b/drivers/gpu/drm/drm_plane_helper.c @@ -91,13 +91,14 @@ static int get_connectors_for_crtc(struct drm_crtc *crtc, */ WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
- list_for_each_entry(connector, &dev->mode_config.connector_list, head) + drm_for_each_connector(connector, dev) { if (connector->encoder && connector->encoder->crtc == crtc) { if (connector_list != NULL && count < num_connectors) *(connector_list++) = connector;
count++; } + }
return count; }
Remaining manual work in the drm core&helpers. Nothing special here, no surprises.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_crtc.c | 26 ++++++++++++++------------ drivers/gpu/drm/drm_crtc_helper.c | 2 +- drivers/gpu/drm/drm_fb_cma_helper.c | 2 +- drivers/gpu/drm/drm_modeset_lock.c | 7 +++---- 4 files changed, 19 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 9c1440b2d84c..b4ecf92cd668 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -736,7 +736,7 @@ unsigned int drm_crtc_index(struct drm_crtc *crtc) unsigned int index = 0; struct drm_crtc *tmp;
- list_for_each_entry(tmp, &crtc->dev->mode_config.crtc_list, head) { + drm_for_each_crtc(tmp, crtc->dev) { if (tmp == crtc) return index;
@@ -1280,7 +1280,7 @@ unsigned int drm_plane_index(struct drm_plane *plane) unsigned int index = 0; struct drm_plane *tmp;
- list_for_each_entry(tmp, &plane->dev->mode_config.plane_list, head) { + drm_for_each_plane(tmp, plane->dev) { if (tmp == plane) return index;
@@ -1719,10 +1719,10 @@ int drm_mode_group_init_legacy_group(struct drm_device *dev, if (ret) return ret;
- list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) + drm_for_each_crtc(crtc, dev) group->id_list[group->num_crtcs++] = crtc->base.id;
- list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) + drm_for_each_encoder(encoder, dev) group->id_list[group->num_crtcs + group->num_encoders++] = encoder->base.id;
@@ -1811,15 +1811,17 @@ int drm_mode_getresources(struct drm_device *dev, void *data, mutex_lock(&dev->mode_config.mutex); if (!drm_is_primary_client(file_priv)) { struct drm_connector *connector; + struct drm_encoder *encoder; + struct drm_crtc *crtc;
mode_group = NULL; - list_for_each(lh, &dev->mode_config.crtc_list) + drm_for_each_crtc(crtc, dev) crtc_count++;
drm_for_each_connector(connector, dev) connector_count++;
- list_for_each(lh, &dev->mode_config.encoder_list) + drm_for_each_encoder(encoder, dev) encoder_count++; } else {
@@ -2287,7 +2289,7 @@ int drm_mode_getplane_res(struct drm_device *dev, void *data, plane_ptr = (uint32_t __user *)(unsigned long)plane_resp->plane_id_ptr;
/* Plane lists are invariant, no locking needed. */ - list_for_each_entry(plane, &config->plane_list, head) { + drm_for_each_plane(plane, dev) { /* * Unless userspace set the 'universal planes' * capability bit, only advertise overlays. @@ -2592,7 +2594,7 @@ int drm_mode_set_config_internal(struct drm_mode_set *set) * connectors from it), hence we need to refcount the fbs across all * crtcs. Atomic modeset will have saner semantics ... */ - list_for_each_entry(tmp, &crtc->dev->mode_config.crtc_list, head) + drm_for_each_crtc(tmp, crtc->dev) tmp->primary->old_fb = tmp->primary->fb;
fb = set->fb; @@ -2603,7 +2605,7 @@ int drm_mode_set_config_internal(struct drm_mode_set *set) crtc->primary->fb = fb; }
- list_for_each_entry(tmp, &crtc->dev->mode_config.crtc_list, head) { + drm_for_each_crtc(tmp, crtc->dev) { if (tmp->primary->fb) drm_framebuffer_reference(tmp->primary->fb); if (tmp->primary->old_fb) @@ -5373,15 +5375,15 @@ void drm_mode_config_reset(struct drm_device *dev) struct drm_encoder *encoder; struct drm_connector *connector;
- list_for_each_entry(plane, &dev->mode_config.plane_list, head) + drm_for_each_plane(plane, dev) if (plane->funcs->reset) plane->funcs->reset(plane);
- list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) + drm_for_each_crtc(crtc, dev) if (crtc->funcs->reset) crtc->funcs->reset(crtc);
- list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) + drm_for_each_encoder(encoder, dev) if (encoder->funcs->reset) encoder->funcs->reset(encoder);
diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c index 4a83c22f5e61..2e89d8b7de18 100644 --- a/drivers/gpu/drm/drm_crtc_helper.c +++ b/drivers/gpu/drm/drm_crtc_helper.c @@ -151,7 +151,7 @@ bool drm_helper_crtc_in_use(struct drm_crtc *crtc) if (!oops_in_progress) WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
- list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) + drm_for_each_encoder(encoder, dev) if (encoder->crtc == crtc && drm_helper_encoder_in_use(encoder)) return true; return false; diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c index 5c1aca443e54..87ce7c36b0db 100644 --- a/drivers/gpu/drm/drm_fb_cma_helper.c +++ b/drivers/gpu/drm/drm_fb_cma_helper.c @@ -221,7 +221,7 @@ int drm_fb_cma_debugfs_show(struct seq_file *m, void *arg) return ret; }
- list_for_each_entry(fb, &dev->mode_config.fb_list, head) + drm_for_each_fb(fb, dev) drm_fb_cma_describe(fb, m);
mutex_unlock(&dev->struct_mutex); diff --git a/drivers/gpu/drm/drm_modeset_lock.c b/drivers/gpu/drm/drm_modeset_lock.c index c0a5cd8c5262..744dfbc6a329 100644 --- a/drivers/gpu/drm/drm_modeset_lock.c +++ b/drivers/gpu/drm/drm_modeset_lock.c @@ -276,7 +276,7 @@ void drm_warn_on_modeset_not_all_locked(struct drm_device *dev) if (oops_in_progress) return;
- list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) + drm_for_each_crtc(crtc, dev) WARN_ON(!drm_modeset_is_locked(&crtc->mutex));
WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex)); @@ -464,18 +464,17 @@ EXPORT_SYMBOL(drm_modeset_unlock); int drm_modeset_lock_all_crtcs(struct drm_device *dev, struct drm_modeset_acquire_ctx *ctx) { - struct drm_mode_config *config = &dev->mode_config; struct drm_crtc *crtc; struct drm_plane *plane; int ret = 0;
- list_for_each_entry(crtc, &config->crtc_list, head) { + drm_for_each_crtc(crtc, dev) { ret = drm_modeset_lock(&crtc->mutex, ctx); if (ret) return ret; }
- list_for_each_entry(plane, &config->plane_list, head) { + drm_for_each_plane(plane, dev) { ret = drm_modeset_lock(&plane->mutex, ctx); if (ret) return ret;
On Tue, Jun 23, 2015 at 10:45:51PM +0200, Daniel Vetter wrote:
Hi all,
Dave&I have been discussing connector hotplug and unplugging around DP MST and if there's one thing that's clear it's that we don't even really know where all the problems are. Hence first step is to figure that out. One of the bigger items is walking the encoder/connector lists without appropriate locking to protect against concurrent hotadd/removal of connectors.
This patch series tries to untangle things a bit here. RFC since only lightly tested and missing conversion of the radoen mst code to the new locking scheme. I think rolling out the new macros for i915 should be done as a second step, to avoid hitting too many WARN_ON ;-)
Bah, I was hoping to see scru! :) -Chris
On Wed, Jun 24, 2015 at 08:44:47AM +0100, Chris Wilson wrote:
On Tue, Jun 23, 2015 at 10:45:51PM +0200, Daniel Vetter wrote:
Hi all,
Dave&I have been discussing connector hotplug and unplugging around DP MST and if there's one thing that's clear it's that we don't even really know where all the problems are. Hence first step is to figure that out. One of the bigger items is walking the encoder/connector lists without appropriate locking to protect against concurrent hotadd/removal of connectors.
This patch series tries to untangle things a bit here. RFC since only lightly tested and missing conversion of the radoen mst code to the new locking scheme. I think rolling out the new macros for i915 should be done as a second step, to avoid hitting too many WARN_ON ;-)
Bah, I was hoping to see scru! :)
This is just to start figuring out how bad the situation is and who's all affected. Later on we need to figure out how to properly protect the connector/encoder list without just grabbing all kinds of locks in the hotadd/remove code - stalling screen updates on the main screen just because you plug something in tends to upset people. -Daniel
dri-devel@lists.freedesktop.org