drm: Split connection_mutex out of mode_config.mutex (v3)

After the split-out of crtc locks from the big mode_config.mutex
there's still two major areas it protects:
- Various connector probe states, like connector->status, EDID
properties, probed mode lists and similar information.
- The links from connector->encoder and encoder->crtc and other
modeset-relevant connector state (e.g. properties which control the
panel fitter).

The later is used by modeset operations. But they don't really care
about the former since it's allowed to e.g. enable a disconnected VGA
output or with a mode not in the probed list.

Thus far this hasn't been a problem, but for the atomic modeset
conversion Rob Clark needs to convert all modeset relevant locks into
w/w locks. This is required because the order of acquisition is
determined by how userspace supplies the atomic modeset data. This has
run into troubles in the detect path since the i915 load detect code
needs _both_ protections offered by the mode_config.mutex: It updates
probe state and it needs to change the modeset configuration to enable
the temporary load detect pipe.

The big deal here is that for the probe/detect users of this lock a
plain mutex fits best, but for atomic modesets we really want a w/w
mutex. To fix this lets split out a new connection_mutex lock for the
modeset relevant parts.

For simplicity I've decided to only add one additional lock for all
connector/encoder links and modeset configuration states. We have
piles of different modeset objects in addition to those (like bridges
or panels), so adding per-object locks would be much more effort.

Also, we're guaranteed (at least for now) to do a full modeset if we
need to acquire this lock. Which means that fine-grained locking is
fairly irrelevant compared to the amount of time the full modeset will
take.

I've done a full audit, and there's just a few things that justify
special focus:
- Locking in drm_sysfs.c is almost completely absent. We should
sprinkle mode_config.connection_mutex over this file a bit, but
since it already lacks mode_config.mutex this patch wont make the
situation any worse. This is material for a follow-up patch.

- omap has a omap_framebuffer_flush function which walks the
connector->encoder->crtc links and is called from many contexts.
Some look like they don't acquire mode_config.mutex, so this is
already racy. Again fixing this is material for a separate patch.

- The radeon hot_plug function to retrain DP links looks at
connector->dpms. Currently this happens without any locking, so is
already racy. I think radeon_hotplug_work_func should gain
mutex_lock/unlock calls for the mode_config.connection_mutex.

- Same applies to i915's intel_dp_hot_plug. But again, this is already
racy.

- i915 load_detect code needs to acquire this lock. Which means the
w/w dance due to Rob's work will be nicely contained to _just_ this
function.

I've added fixme comments everywhere where it looks suspicious but in
the sysfs code. After a quick irc discussion with Dave Airlie it
sounds like the lack of locking in there is due to sysfs cleanup fun
at module unload.

v1: original (only compile tested)

v2: missing mutex_init(), etc (from Rob Clark)

v3: i915 needs more care in the conversion:
- Protect the edp pp logic with the connection_mutex.
- Use connection_mutex in the backlight code due to
get_pipe_from_connector.
- Use drm_modeset_lock_all in suspend/resume paths.
- Update lock checks in the overlay code.

Cc: Alex Deucher <[e-mail]>
Cc: Rob Clark <[e-mail]>
Signed-off-by: Daniel Vetter <[e-mail]>
Reviewed-by: Rob Clark <[e-mail]>

drivers/gpu/drm/drm_crtc.c

···
 
 	mutex_lock(&dev->mode_config.mutex);
 
+	mutex_lock(&dev->mode_config.connection_mutex);
+
 	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
 		mutex_lock_nest_lock(&crtc->mutex, &dev->mode_config.mutex);
 }
···
 
 	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
 		mutex_unlock(&crtc->mutex);
+
+	mutex_unlock(&dev->mode_config.connection_mutex);
 
 	mutex_unlock(&dev->mode_config.mutex);
 }
···
 	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
 		WARN_ON(!mutex_is_locked(&crtc->mutex));
 
+	WARN_ON(!mutex_is_locked(&dev->mode_config.connection_mutex));
 	WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
 }
 EXPORT_SYMBOL(drm_warn_on_modeset_not_all_locked);
···
 	DRM_DEBUG_KMS("[CONNECTOR:%d:?]\n", out_resp->connector_id);
 
 	mutex_lock(&dev->mode_config.mutex);
+	mutex_lock(&dev->mode_config.connection_mutex);
 
 	connector = drm_connector_find(dev, out_resp->connector_id);
 	if (!connector) {
···
 	out_resp->count_encoders = encoders_count;
 
 out:
+	mutex_unlock(&dev->mode_config.connection_mutex);
 	mutex_unlock(&dev->mode_config.mutex);
 
 	return ret;
···
 void drm_mode_config_init(struct drm_device *dev)
 {
 	mutex_init(&dev->mode_config.mutex);
+	mutex_init(&dev->mode_config.connection_mutex);
 	mutex_init(&dev->mode_config.idr_mutex);
 	mutex_init(&dev->mode_config.fb_lock);
 	INIT_LIST_HEAD(&dev->mode_config.fb_list);

drivers/gpu/drm/drm_crtc_helper.c

···
 	struct drm_device *dev = encoder->dev;
 
 	WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
+	WARN_ON(!mutex_is_locked(&dev->mode_config.connection_mutex));
+
 	list_for_each_entry(connector, &dev->mode_config.connector_list, head)
 		if (connector->encoder == encoder)
 			return true;

drivers/gpu/drm/drm_edid.c

···
 	struct drm_connector *connector;
 	struct drm_device *dev = encoder->dev;
 
+	WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
+
 	list_for_each_entry(connector, &dev->mode_config.connector_list, head)
 		if (connector->encoder == encoder && connector->eld[0])
 			return connector;

drivers/gpu/drm/drm_plane_helper.c

···
 	struct drm_connector *connector;
 	int count = 0;
 
+	/*
+	 * Note: Once we change the plane hooks to more fine-grained locking we
+	 * need to grab the connection_mutex here to be able to make these
+	 * checks.
+	 */
+	WARN_ON(!mutex_is_locked(&dev->mode_config.connection_mutex));
+
 	list_for_each_entry(connector, &dev->mode_config.connector_list, head)
 		if (connector->encoder && connector->encoder->crtc == crtc) {
 			if (connector_list != NULL && count < num_connectors)

drivers/gpu/drm/i915/i915_debugfs.c

···
 
 	*source = INTEL_PIPE_CRC_SOURCE_PIPE;
 
-	mutex_lock(&dev->mode_config.mutex);
+	drm_modeset_lock_all(dev);
 	list_for_each_entry(encoder, &dev->mode_config.encoder_list,
 			    base.head) {
 		if (!encoder->base.crtc)
···
 			break;
 		}
 	}
-	mutex_unlock(&dev->mode_config.mutex);
+	drm_modeset_unlock_all(dev);
 
 	return ret;
 }

drivers/gpu/drm/i915/i915_drv.c

···
 		 * Disable CRTCs directly since we want to preserve sw state
 		 * for _thaw.
 		 */
-		mutex_lock(&dev->mode_config.mutex);
+		drm_modeset_lock_all(dev);
 		for_each_crtc(dev, crtc) {
-			mutex_lock(&crtc->mutex);
 			dev_priv->display.crtc_disable(crtc);
-			mutex_unlock(&crtc->mutex);
 		}
-		mutex_unlock(&dev->mode_config.mutex);
+		drm_modeset_unlock_all(dev);
 
 		intel_modeset_suspend_hw(dev);
 	}

drivers/gpu/drm/i915/intel_display.c

···
 		      connector->base.id, connector->name,
 		      encoder->base.id, encoder->name);
 
+	mutex_lock(&dev->mode_config.connection_mutex);
+
 	/*
 	 * Algorithm gets a little messy:
 	 *
···
 	 */
 	if (!crtc) {
 		DRM_DEBUG_KMS("no pipe available for load-detect\n");
-		return false;
+		goto fail_unlock_connector;
 	}
 
 	mutex_lock(&crtc->mutex);
···
 	else
 		intel_crtc->new_config = NULL;
 	mutex_unlock(&crtc->mutex);
+fail_unlock_connector:
+	mutex_unlock(&dev->mode_config.connection_mutex);
+
 	return false;
 }
 
···
 		}
 
 		mutex_unlock(&crtc->mutex);
+		mutex_unlock(&connector->dev->mode_config.connection_mutex);
 		return;
 	}
 
···
 		connector->funcs->dpms(connector, old->dpms_mode);
 
 	mutex_unlock(&crtc->mutex);
+	mutex_unlock(&connector->dev->mode_config.connection_mutex);
 }
 
 static int i9xx_pll_refclk(struct drm_device *dev,
···
 enum pipe intel_get_pipe_from_connector(struct intel_connector *connector)
 {
 	struct drm_encoder *encoder = connector->base.encoder;
+	struct drm_device *dev = connector->base.dev;
 
-	WARN_ON(!mutex_is_locked(&connector->base.dev->mode_config.mutex));
+	WARN_ON(!mutex_is_locked(&dev->mode_config.connection_mutex));
 
 	if (!encoder)
 		return INVALID_PIPE;
···
 	/* Just in case the BIOS is doing something questionable. */
 	intel_disable_fbc(dev);
 
-	mutex_lock(&dev->mode_config.mutex);
+	drm_modeset_lock_all(dev);
 	intel_modeset_setup_hw_state(dev, false);
-	mutex_unlock(&dev->mode_config.mutex);
+	drm_modeset_unlock_all(dev);
 
 	for_each_intel_crtc(dev, crtc) {
 		if (!crtc->active)

drivers/gpu/drm/i915/intel_dp.c

···
 	u32 pp;
 	u32 pp_stat_reg, pp_ctrl_reg;
 
-	WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
+	WARN_ON(!mutex_is_locked(&dev->mode_config.connection_mutex));
 
 	if (!intel_dp->want_panel_vdd && edp_have_panel_vdd(intel_dp)) {
 		struct intel_digital_port *intel_dig_port =
···
 						 struct intel_dp, panel_vdd_work);
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
 
-	mutex_lock(&dev->mode_config.mutex);
+	mutex_lock(&dev->mode_config.connection_mutex);
 	edp_panel_vdd_off_sync(intel_dp);
-	mutex_unlock(&dev->mode_config.mutex);
+	mutex_unlock(&dev->mode_config.connection_mutex);
 }
 
 static void edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync)
···
 	u8 sink_irq_vector;
 	u8 link_status[DP_LINK_STATUS_SIZE];
 
+	/* FIXME: This access isn't protected by any locks. */
 	if (!intel_encoder->connectors_active)
 		return;
 
···
 	drm_encoder_cleanup(encoder);
 	if (is_edp(intel_dp)) {
 		cancel_delayed_work_sync(&intel_dp->panel_vdd_work);
-		mutex_lock(&dev->mode_config.mutex);
+		mutex_lock(&dev->mode_config.connection_mutex);
 		edp_panel_vdd_off_sync(intel_dp);
-		mutex_unlock(&dev->mode_config.mutex);
+		mutex_unlock(&dev->mode_config.connection_mutex);
 	}
 	kfree(intel_dig_port);
 }
···
 		drm_dp_aux_unregister_i2c_bus(&intel_dp->aux);
 		if (is_edp(intel_dp)) {
 			cancel_delayed_work_sync(&intel_dp->panel_vdd_work);
-			mutex_lock(&dev->mode_config.mutex);
+			mutex_lock(&dev->mode_config.connection_mutex);
 			edp_panel_vdd_off_sync(intel_dp);
-			mutex_unlock(&dev->mode_config.mutex);
+			mutex_unlock(&dev->mode_config.connection_mutex);
 		}
 		drm_sysfs_connector_remove(connector);
 		drm_connector_cleanup(connector);

drivers/gpu/drm/i915/intel_opregion.c

···
 	if (bclp > 255)
 		return ASLC_BACKLIGHT_FAILED;
 
-	mutex_lock(&dev->mode_config.mutex);
+	mutex_lock(&dev->mode_config.connection_mutex);
 
 	/*
 	 * Update backlight on all connectors that support backlight (usually
···
 		intel_panel_set_backlight(intel_connector, bclp, 255);
 	iowrite32(DIV_ROUND_UP(bclp * 100, 255) | ASLE_CBLV_VALID, &asle->cblv);
 
-	mutex_unlock(&dev->mode_config.mutex);
+	mutex_unlock(&dev->mode_config.connection_mutex);
 
 
 	return 0;

drivers/gpu/drm/i915/intel_overlay.c

···
 	u32 swidth, swidthsw, sheight, ostride;
 
 	BUG_ON(!mutex_is_locked(&dev->struct_mutex));
-	BUG_ON(!mutex_is_locked(&dev->mode_config.mutex));
+	BUG_ON(!mutex_is_locked(&dev->mode_config.connection_mutex));
 	BUG_ON(!overlay);
 
 	ret = intel_overlay_release_old_vid(overlay);
···
 	int ret;
 
 	BUG_ON(!mutex_is_locked(&dev->struct_mutex));
-	BUG_ON(!mutex_is_locked(&dev->mode_config.mutex));
+	BUG_ON(!mutex_is_locked(&dev->mode_config.connection_mutex));
 
 	ret = intel_overlay_recover_from_interrupt(overlay);
 	if (ret != 0)

drivers/gpu/drm/i915/intel_panel.c

···
 	struct intel_connector *connector = bl_get_data(bd);
 	struct drm_device *dev = connector->base.dev;
 
-	mutex_lock(&dev->mode_config.mutex);
+	mutex_lock(&dev->mode_config.connection_mutex);
 	DRM_DEBUG_KMS("updating intel_backlight, brightness=%d/%d\n",
 		      bd->props.brightness, bd->props.max_brightness);
 	intel_panel_set_backlight(connector, bd->props.brightness,
 				  bd->props.max_brightness);
-	mutex_unlock(&dev->mode_config.mutex);
+	mutex_unlock(&dev->mode_config.connection_mutex);
 	return 0;
 }
 
···
 	int ret;
 
 	intel_runtime_pm_get(dev_priv);
-	mutex_lock(&dev->mode_config.mutex);
+	mutex_lock(&dev->mode_config.connection_mutex);
 	ret = intel_panel_get_backlight(connector);
-	mutex_unlock(&dev->mode_config.mutex);
+	mutex_unlock(&dev->mode_config.connection_mutex);
 	intel_runtime_pm_put(dev_priv);
 
 	return ret;

drivers/gpu/drm/omapdrm/omap_fb.c

···
 
 	VERB("flush: %d,%d %dx%d, fb=%p", x, y, w, h, fb);
 
+	/* FIXME: This is racy - no protection against modeset config changes. */
 	while ((connector = omap_framebuffer_get_next_connector(fb, connector))) {
 		/* only consider connectors that are part of a chain */
 		if (connector->encoder && connector->encoder->crtc) {

drivers/gpu/drm/radeon/radeon_connectors.c

···
 	radeon_hpd_set_polarity(rdev, radeon_connector->hpd.hpd);
 
 	/* if the connector is already off, don't turn it back on */
+	/* FIXME: This access isn't protected by any locks. */
 	if (connector->dpms != DRM_MODE_DPMS_ON)
 		return;
 

include/drm/drm_crtc.h

···
  */
 struct drm_mode_config {
 	struct mutex mutex; /* protects configuration (mode lists etc.) */
+	struct mutex connection_mutex; /* protects connector->encoder and encoder->crtc links */
 	struct mutex idr_mutex; /* for IDR management */
 	struct idr crtc_idr; /* use this idr for all IDs, fb, crtc, connector, modes - just makes life easier */
 	/* this is limited to one for now */