All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/atomic: Acquire connection_mutex lock in drm_helper_probe_single_connector_modes, v4.
@ 2017-04-06 18:55 Maarten Lankhorst
  2017-04-06 19:15 ` ✓ Fi.CI.BAT: success for " Patchwork
  2017-04-10 10:16 ` [PATCH] " Boris Brezillon
  0 siblings, 2 replies; 3+ messages in thread
From: Maarten Lankhorst @ 2017-04-06 18:55 UTC (permalink / raw)
  To: dri-devel; +Cc: intel-gfx

mode_valid() called from drm_helper_probe_single_connector_modes()
may need to look at connector->state because what a valid mode is may
depend on connector properties being set. For example some HDMI modes
might be rejected when a connector property forces the connector
into DVI mode.

Some implementations of detect() already lock all state,
so we have to pass an acquire_ctx to them to prevent a deadlock.

This means changing the function signature of detect() slightly,
and passing the acquire_ctx for locking multiple crtc's.
For the callbacks, it will always be non-zero. To allow callers
not to worry about this, drm_helper_probe_detect_ctx is added
which might handle -EDEADLK for you.

Changes since v1:
- Always set ctx parameter.
Changes since v2:
- Always take connection_mutex when probing.
Changes since v3:
- Remove the ctx from intel_dp_long_pulse, and add
  WARN_ON(!connection_mutex) (danvet)
- Update docs to clarify the locking situation. (danvet)

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_probe_helper.c       | 100 ++++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/intel_crt.c         |  25 ++++----
 drivers/gpu/drm/i915/intel_display.c     |  25 ++++----
 drivers/gpu/drm/i915/intel_dp.c          |  21 +++----
 drivers/gpu/drm/i915/intel_drv.h         |   8 +--
 drivers/gpu/drm/i915/intel_hotplug.c     |   3 +-
 drivers/gpu/drm/i915/intel_tv.c          |  21 +++----
 include/drm/drm_connector.h              |   6 ++
 include/drm/drm_crtc_helper.h            |   3 +
 include/drm/drm_modeset_helper_vtables.h |  36 +++++++++++
 10 files changed, 187 insertions(+), 61 deletions(-)

diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index efb5e5e8ce62..1b0c14ab3fff 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -169,12 +169,73 @@ void drm_kms_helper_poll_enable(struct drm_device *dev)
 EXPORT_SYMBOL(drm_kms_helper_poll_enable);
 
 static enum drm_connector_status
-drm_connector_detect(struct drm_connector *connector, bool force)
+drm_helper_probe_detect_ctx(struct drm_connector *connector, bool force)
 {
-	return connector->funcs->detect ?
-		connector->funcs->detect(connector, force) :
-		connector_status_connected;
+	const struct drm_connector_helper_funcs *funcs = connector->helper_private;
+	struct drm_modeset_acquire_ctx ctx;
+	int ret;
+
+	drm_modeset_acquire_init(&ctx, 0);
+
+retry:
+	ret = drm_modeset_lock(&connector->dev->mode_config.connection_mutex, &ctx);
+	if (!ret) {
+		if (funcs->detect_ctx)
+			ret = funcs->detect_ctx(connector, &ctx, force);
+		else if (connector->funcs->detect)
+			ret = connector->funcs->detect(connector, force);
+		else
+			ret = connector_status_connected;
+	}
+
+	if (ret == -EDEADLK) {
+		drm_modeset_backoff(&ctx);
+		goto retry;
+	}
+
+	if (WARN_ON(ret < 0))
+		ret = connector_status_unknown;
+
+	drm_modeset_drop_locks(&ctx);
+	drm_modeset_acquire_fini(&ctx);
+
+	return ret;
+}
+
+/**
+ * drm_helper_probe_detect - probe connector status
+ * @connector: connector to probe
+ * @ctx: acquire_ctx, or NULL to let this function handle locking.
+ * @force: Whether destructive probe operations should be performed.
+ *
+ * This function calls the detect callbacks of the connector.
+ * This function returns &drm_connector_status, or
+ * if @ctx is set, it might also return -EDEADLK.
+ */
+int
+drm_helper_probe_detect(struct drm_connector *connector,
+			struct drm_modeset_acquire_ctx *ctx,
+			bool force)
+{
+	const struct drm_connector_helper_funcs *funcs = connector->helper_private;
+	struct drm_device *dev = connector->dev;
+	int ret;
+
+	if (!ctx)
+		return drm_helper_probe_detect_ctx(connector, force);
+
+	ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
+	if (ret)
+		return ret;
+
+	if (funcs->detect_ctx)
+		return funcs->detect_ctx(connector, ctx, force);
+	else if (connector->funcs->detect)
+		return connector->funcs->detect(connector, force);
+	else
+		return connector_status_connected;
 }
+EXPORT_SYMBOL(drm_helper_probe_detect);
 
 /**
  * drm_helper_probe_single_connector_modes - get complete set of display modes
@@ -239,15 +300,27 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
 	struct drm_display_mode *mode;
 	const struct drm_connector_helper_funcs *connector_funcs =
 		connector->helper_private;
-	int count = 0;
+	int count = 0, ret;
 	int mode_flags = 0;
 	bool verbose_prune = true;
 	enum drm_connector_status old_status;
+	struct drm_modeset_acquire_ctx ctx;
 
 	WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
 
+	drm_modeset_acquire_init(&ctx, 0);
+
 	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", connector->base.id,
 			connector->name);
+
+retry:
+	ret = drm_modeset_lock(&dev->mode_config.connection_mutex, &ctx);
+	if (ret == -EDEADLK) {
+		drm_modeset_backoff(&ctx);
+		goto retry;
+	} else
+		WARN_ON(ret < 0);
+
 	/* set all old modes to the stale state */
 	list_for_each_entry(mode, &connector->modes, head)
 		mode->status = MODE_STALE;
@@ -263,7 +336,15 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
 		if (connector->funcs->force)
 			connector->funcs->force(connector);
 	} else {
-		connector->status = drm_connector_detect(connector, true);
+		ret = drm_helper_probe_detect(connector, &ctx, true);
+
+		if (ret == -EDEADLK) {
+			drm_modeset_backoff(&ctx);
+			goto retry;
+		} else if (WARN(ret < 0, "Invalid return value %i for connector detection\n", ret))
+			ret = connector_status_unknown;
+
+		connector->status = ret;
 	}
 
 	/*
@@ -355,6 +436,9 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
 prune:
 	drm_mode_prune_invalid(dev, &connector->modes, verbose_prune);
 
+	drm_modeset_drop_locks(&ctx);
+	drm_modeset_acquire_fini(&ctx);
+
 	if (list_empty(&connector->modes))
 		return 0;
 
@@ -440,7 +524,7 @@ static void output_poll_execute(struct work_struct *work)
 
 		repoll = true;
 
-		connector->status = drm_connector_detect(connector, false);
+		connector->status = drm_helper_probe_detect(connector, NULL, false);
 		if (old_status != connector->status) {
 			const char *old, *new;
 
@@ -588,7 +672,7 @@ bool drm_helper_hpd_irq_event(struct drm_device *dev)
 
 		old_status = connector->status;
 
-		connector->status = drm_connector_detect(connector, false);
+		connector->status = drm_helper_probe_detect(connector, NULL, false);
 		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %s to %s\n",
 			      connector->base.id,
 			      connector->name,
diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index 8c82607294c6..2797bf37c3ac 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -669,15 +669,16 @@ static const struct dmi_system_id intel_spurious_crt_detect[] = {
 	{ }
 };
 
-static enum drm_connector_status
-intel_crt_detect(struct drm_connector *connector, bool force)
+static int
+intel_crt_detect(struct drm_connector *connector,
+		 struct drm_modeset_acquire_ctx *ctx,
+		 bool force)
 {
 	struct drm_i915_private *dev_priv = to_i915(connector->dev);
 	struct intel_crt *crt = intel_attached_crt(connector);
 	struct intel_encoder *intel_encoder = &crt->base;
-	enum drm_connector_status status;
+	int status, ret;
 	struct intel_load_detect_pipe tmp;
-	struct drm_modeset_acquire_ctx ctx;
 
 	DRM_DEBUG_KMS("[CONNECTOR:%d:%s] force=%d\n",
 		      connector->base.id, connector->name,
@@ -721,10 +722,9 @@ intel_crt_detect(struct drm_connector *connector, bool force)
 		goto out;
 	}
 
-	drm_modeset_acquire_init(&ctx, 0);
-
 	/* for pre-945g platforms use load detect */
-	if (intel_get_load_detect_pipe(connector, NULL, &tmp, &ctx)) {
+	ret = intel_get_load_detect_pipe(connector, NULL, &tmp, ctx);
+	if (ret > 0) {
 		if (intel_crt_detect_ddc(connector))
 			status = connector_status_connected;
 		else if (INTEL_GEN(dev_priv) < 4)
@@ -734,12 +734,11 @@ intel_crt_detect(struct drm_connector *connector, bool force)
 			status = connector_status_disconnected;
 		else
 			status = connector_status_unknown;
-		intel_release_load_detect_pipe(connector, &tmp, &ctx);
-	} else
+		intel_release_load_detect_pipe(connector, &tmp, ctx);
+	} else if (ret == 0)
 		status = connector_status_unknown;
-
-	drm_modeset_drop_locks(&ctx);
-	drm_modeset_acquire_fini(&ctx);
+	else if (ret < 0)
+		status = ret;
 
 out:
 	intel_display_power_put(dev_priv, intel_encoder->power_domain);
@@ -811,7 +810,6 @@ void intel_crt_reset(struct drm_encoder *encoder)
 
 static const struct drm_connector_funcs intel_crt_connector_funcs = {
 	.dpms = drm_atomic_helper_connector_dpms,
-	.detect = intel_crt_detect,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.late_register = intel_connector_register,
 	.early_unregister = intel_connector_unregister,
@@ -823,6 +821,7 @@ static const struct drm_connector_funcs intel_crt_connector_funcs = {
 };
 
 static const struct drm_connector_helper_funcs intel_crt_connector_helper_funcs = {
+	.detect_ctx = intel_crt_detect,
 	.mode_valid = intel_crt_mode_valid,
 	.get_modes = intel_crt_get_modes,
 };
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 602fd479dd30..e217d04133e6 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9480,10 +9480,10 @@ static int intel_modeset_setup_plane_state(struct drm_atomic_state *state,
 	return 0;
 }
 
-bool intel_get_load_detect_pipe(struct drm_connector *connector,
-				struct drm_display_mode *mode,
-				struct intel_load_detect_pipe *old,
-				struct drm_modeset_acquire_ctx *ctx)
+int intel_get_load_detect_pipe(struct drm_connector *connector,
+			       struct drm_display_mode *mode,
+			       struct intel_load_detect_pipe *old,
+			       struct drm_modeset_acquire_ctx *ctx)
 {
 	struct intel_crtc *intel_crtc;
 	struct intel_encoder *intel_encoder =
@@ -9506,10 +9506,7 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector,
 
 	old->restore_state = NULL;
 
-retry:
-	ret = drm_modeset_lock(&config->connection_mutex, ctx);
-	if (ret)
-		goto fail;
+	WARN_ON(!drm_modeset_is_locked(&config->connection_mutex));
 
 	/*
 	 * Algorithm gets a little messy:
@@ -9659,10 +9656,8 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector,
 		restore_state = NULL;
 	}
 
-	if (ret == -EDEADLK) {
-		drm_modeset_backoff(ctx);
-		goto retry;
-	}
+	if (ret == -EDEADLK)
+		return ret;
 
 	return false;
 }
@@ -15030,6 +15025,7 @@ static void intel_enable_pipe_a(struct drm_device *dev)
 	struct drm_connector *crt = NULL;
 	struct intel_load_detect_pipe load_detect_temp;
 	struct drm_modeset_acquire_ctx *ctx = dev->mode_config.acquire_ctx;
+	int ret;
 
 	/* We can't just switch on the pipe A, we need to set things up with a
 	 * proper mode and output configuration. As a gross hack, enable pipe A
@@ -15046,7 +15042,10 @@ static void intel_enable_pipe_a(struct drm_device *dev)
 	if (!crt)
 		return;
 
-	if (intel_get_load_detect_pipe(crt, NULL, &load_detect_temp, ctx))
+	ret = intel_get_load_detect_pipe(crt, NULL, &load_detect_temp, ctx);
+	WARN(ret < 0, "All modeset mutexes are locked, but intel_get_load_detect_pipe failed\n");
+
+	if (ret > 0)
 		intel_release_load_detect_pipe(crt, &load_detect_temp, ctx);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index fd96a6cf7326..6e04cb54e3ff 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4566,7 +4566,7 @@ intel_dp_unset_edid(struct intel_dp *intel_dp)
 	intel_dp->has_audio = false;
 }
 
-static enum drm_connector_status
+static int
 intel_dp_long_pulse(struct intel_connector *intel_connector)
 {
 	struct drm_connector *connector = &intel_connector->base;
@@ -4577,6 +4577,8 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
 	enum drm_connector_status status;
 	u8 sink_irq_vector = 0;
 
+	WARN_ON(!drm_modeset_is_locked(&connector->dev->mode_config.connection_mutex));
+
 	intel_display_power_get(to_i915(dev), intel_dp->aux_power_domain);
 
 	/* Can't disconnect eDP, but you can close the lid... */
@@ -4635,14 +4637,7 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
 		status = connector_status_disconnected;
 		goto out;
 	} else if (connector->status == connector_status_connected) {
-		/*
-		 * If display was connected already and is still connected
-		 * check links status, there has been known issues of
-		 * link loss triggerring long pulse!!!!
-		 */
-		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
 		intel_dp_check_link_status(intel_dp);
-		drm_modeset_unlock(&dev->mode_config.connection_mutex);
 		goto out;
 	}
 
@@ -4682,11 +4677,13 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
 	return status;
 }
 
-static enum drm_connector_status
-intel_dp_detect(struct drm_connector *connector, bool force)
+static int
+intel_dp_detect(struct drm_connector *connector,
+		struct drm_modeset_acquire_ctx *ctx,
+		bool force)
 {
 	struct intel_dp *intel_dp = intel_attached_dp(connector);
-	enum drm_connector_status status = connector->status;
+	int status = connector->status;
 
 	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
 		      connector->base.id, connector->name);
@@ -5014,7 +5011,6 @@ void intel_dp_encoder_reset(struct drm_encoder *encoder)
 
 static const struct drm_connector_funcs intel_dp_connector_funcs = {
 	.dpms = drm_atomic_helper_connector_dpms,
-	.detect = intel_dp_detect,
 	.force = intel_dp_force,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.set_property = intel_dp_set_property,
@@ -5027,6 +5023,7 @@ static const struct drm_connector_funcs intel_dp_connector_funcs = {
 };
 
 static const struct drm_connector_helper_funcs intel_dp_connector_helper_funcs = {
+	.detect_ctx = intel_dp_detect,
 	.get_modes = intel_dp_get_modes,
 	.mode_valid = intel_dp_mode_valid,
 };
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 51228fe4283b..fb7afcf9e8be 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1353,10 +1353,10 @@ int ironlake_get_lanes_required(int target_clock, int link_bw, int bpp);
 void vlv_wait_port_ready(struct drm_i915_private *dev_priv,
 			 struct intel_digital_port *dport,
 			 unsigned int expected_mask);
-bool intel_get_load_detect_pipe(struct drm_connector *connector,
-				struct drm_display_mode *mode,
-				struct intel_load_detect_pipe *old,
-				struct drm_modeset_acquire_ctx *ctx);
+int intel_get_load_detect_pipe(struct drm_connector *connector,
+			       struct drm_display_mode *mode,
+			       struct intel_load_detect_pipe *old,
+			       struct drm_modeset_acquire_ctx *ctx);
 void intel_release_load_detect_pipe(struct drm_connector *connector,
 				    struct intel_load_detect_pipe *old,
 				    struct drm_modeset_acquire_ctx *ctx);
diff --git a/drivers/gpu/drm/i915/intel_hotplug.c b/drivers/gpu/drm/i915/intel_hotplug.c
index 7d210097eefa..f1200272a699 100644
--- a/drivers/gpu/drm/i915/intel_hotplug.c
+++ b/drivers/gpu/drm/i915/intel_hotplug.c
@@ -243,7 +243,8 @@ static bool intel_hpd_irq_event(struct drm_device *dev,
 	WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
 	old_status = connector->status;
 
-	connector->status = connector->funcs->detect(connector, false);
+	connector->status = drm_helper_probe_detect(connector, NULL, false);
+
 	if (old_status == connector->status)
 		return false;
 
diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
index 6ed1a3ce47b7..e077c2a9e694 100644
--- a/drivers/gpu/drm/i915/intel_tv.c
+++ b/drivers/gpu/drm/i915/intel_tv.c
@@ -1315,8 +1315,10 @@ static void intel_tv_find_better_format(struct drm_connector *connector)
  * Currently this always returns CONNECTOR_STATUS_UNKNOWN, as we need to be sure
  * we have a pipe programmed in order to probe the TV.
  */
-static enum drm_connector_status
-intel_tv_detect(struct drm_connector *connector, bool force)
+static int
+intel_tv_detect(struct drm_connector *connector,
+		struct drm_modeset_acquire_ctx *ctx,
+		bool force)
 {
 	struct drm_display_mode mode;
 	struct intel_tv *intel_tv = intel_attached_tv(connector);
@@ -1331,21 +1333,20 @@ intel_tv_detect(struct drm_connector *connector, bool force)
 
 	if (force) {
 		struct intel_load_detect_pipe tmp;
-		struct drm_modeset_acquire_ctx ctx;
+		int ret;
 
-		drm_modeset_acquire_init(&ctx, 0);
+		ret = intel_get_load_detect_pipe(connector, &mode, &tmp, ctx);
+		if (ret < 0)
+			return ret;
 
-		if (intel_get_load_detect_pipe(connector, &mode, &tmp, &ctx)) {
+		if (ret > 0) {
 			type = intel_tv_detect_type(intel_tv, connector);
-			intel_release_load_detect_pipe(connector, &tmp, &ctx);
+			intel_release_load_detect_pipe(connector, &tmp, ctx);
 			status = type < 0 ?
 				connector_status_disconnected :
 				connector_status_connected;
 		} else
 			status = connector_status_unknown;
-
-		drm_modeset_drop_locks(&ctx);
-		drm_modeset_acquire_fini(&ctx);
 	} else
 		return connector->status;
 
@@ -1516,7 +1517,6 @@ intel_tv_set_property(struct drm_connector *connector, struct drm_property *prop
 
 static const struct drm_connector_funcs intel_tv_connector_funcs = {
 	.dpms = drm_atomic_helper_connector_dpms,
-	.detect = intel_tv_detect,
 	.late_register = intel_connector_register,
 	.early_unregister = intel_connector_unregister,
 	.destroy = intel_tv_destroy,
@@ -1528,6 +1528,7 @@ static const struct drm_connector_funcs intel_tv_connector_funcs = {
 };
 
 static const struct drm_connector_helper_funcs intel_tv_connector_helper_funcs = {
+	.detect_ctx = intel_tv_detect,
 	.mode_valid = intel_tv_mode_valid,
 	.get_modes = intel_tv_get_modes,
 };
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 941f57f311aa..e2ee74c20b8f 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -32,6 +32,7 @@
 struct drm_device;
 
 struct drm_connector_helper_funcs;
+struct drm_modeset_acquire_ctx;
 struct drm_device;
 struct drm_crtc;
 struct drm_encoder;
@@ -378,6 +379,11 @@ struct drm_connector_funcs {
 	 * the helper library vtable purely for historical reasons. The only DRM
 	 * core	entry point to probe connector state is @fill_modes.
 	 *
+	 * Note that the helper library will already hold
+	 * &drm_mode_config.connection_mutex. Drivers which need to grab additional
+	 * locks to avoid races with concurrent modeset changes need to use
+	 * &drm_connector_helper_funcs.detect_ctx instead.
+	 *
 	 * RETURNS:
 	 *
 	 * drm_connector_status indicating the connector's status.
diff --git a/include/drm/drm_crtc_helper.h b/include/drm/drm_crtc_helper.h
index 43505c7b2b3f..76e237bd989b 100644
--- a/include/drm/drm_crtc_helper.h
+++ b/include/drm/drm_crtc_helper.h
@@ -67,6 +67,9 @@ int drm_helper_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y,
 int drm_helper_probe_single_connector_modes(struct drm_connector
 					    *connector, uint32_t maxX,
 					    uint32_t maxY);
+int drm_helper_probe_detect(struct drm_connector *connector,
+			    struct drm_modeset_acquire_ctx *ctx,
+			    bool force);
 void drm_kms_helper_poll_init(struct drm_device *dev);
 void drm_kms_helper_poll_fini(struct drm_device *dev);
 bool drm_helper_hpd_irq_event(struct drm_device *dev);
diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
index 091c42205667..7847babd893c 100644
--- a/include/drm/drm_modeset_helper_vtables.h
+++ b/include/drm/drm_modeset_helper_vtables.h
@@ -747,6 +747,10 @@ struct drm_connector_helper_funcs {
 	 * This callback is used by the probe helpers in e.g.
 	 * drm_helper_probe_single_connector_modes().
 	 *
+	 * To avoid races with concurrent connector state updates, the helper
+	 * libraries always call this with the &drm_mode_config.connection_mutex
+	 * held. Because of this it's safe to inspect &drm_connector->state.
+	 *
 	 * RETURNS:
 	 *
 	 * The number of modes added by calling drm_mode_probed_add().
@@ -754,6 +758,34 @@ struct drm_connector_helper_funcs {
 	int (*get_modes)(struct drm_connector *connector);
 
 	/**
+	 * @detect_ctx:
+	 *
+	 * Check to see if anything is attached to the connector. The parameter
+	 * force is set to false whilst polling, true when checking the
+	 * connector due to a user request. force can be used by the driver to
+	 * avoid expensive, destructive operations during automated probing.
+	 *
+	 * This callback is optional, if not implemented the connector will be
+	 * considered as always being attached.
+	 *
+	 * This is the atomic version of &drm_connector_funcs.detect.
+	 *
+	 * To avoid races against concurrent connector state updates, the
+	 * helper libraries always call this with ctx set to a valid context,
+	 * and &drm_mode_config.connection_mutex will always be locked with
+	 * the ctx parameter set to this ctx. This allows taking additional
+	 * locks as required.
+	 *
+	 * RETURNS:
+	 *
+	 * &drm_connector_status indicating the connector's status,
+	 * or the error code returned by drm_modeset_lock(), -EDEADLK.
+	 */
+	int (*detect_ctx)(struct drm_connector *connector,
+			  struct drm_modeset_acquire_ctx *ctx,
+			  bool force);
+
+	/**
 	 * @mode_valid:
 	 *
 	 * Callback to validate a mode for a connector, irrespective of the
@@ -771,6 +803,10 @@ struct drm_connector_helper_funcs {
 	 * CRTC helpers will not call this function. Drivers therefore must
 	 * still fully validate any mode passed in in a modeset request.
 	 *
+	 * To avoid races with concurrent connector state updates, the helper
+	 * libraries always call this with the &drm_mode_config.connection_mutex
+	 * held. Because of this it's safe to inspect &drm_connector->state.
+         *
 	 * RETURNS:
 	 *
 	 * Either &drm_mode_status.MODE_OK or one of the failure reasons in &enum
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 3+ messages in thread

* ✓ Fi.CI.BAT: success for drm/atomic: Acquire connection_mutex lock in drm_helper_probe_single_connector_modes, v4.
  2017-04-06 18:55 [PATCH] drm/atomic: Acquire connection_mutex lock in drm_helper_probe_single_connector_modes, v4 Maarten Lankhorst
@ 2017-04-06 19:15 ` Patchwork
  2017-04-10 10:16 ` [PATCH] " Boris Brezillon
  1 sibling, 0 replies; 3+ messages in thread
From: Patchwork @ 2017-04-06 19:15 UTC (permalink / raw)
  To: Maarten Lankhorst; +Cc: intel-gfx

== Series Details ==

Series: drm/atomic: Acquire connection_mutex lock in drm_helper_probe_single_connector_modes, v4.
URL   : https://patchwork.freedesktop.org/series/22602/
State : success

== Summary ==

Series 22602v1 drm/atomic: Acquire connection_mutex lock in drm_helper_probe_single_connector_modes, v4.
https://patchwork.freedesktop.org/api/1.0/series/22602/revisions/1/mbox/

Test gem_exec_suspend:
        Subgroup basic-s4-devices:
                pass       -> DMESG-WARN (fi-bxt-t5700) fdo#100125

fdo#100125 https://bugs.freedesktop.org/show_bug.cgi?id=100125

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time: 434s
fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time: 425s
fi-bsw-n3050     total:278  pass:242  dwarn:0   dfail:0   fail:0   skip:36  time: 569s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time: 509s
fi-bxt-t5700     total:278  pass:257  dwarn:1   dfail:0   fail:0   skip:20  time: 543s
fi-byt-j1900     total:278  pass:254  dwarn:0   dfail:0   fail:0   skip:24  time: 488s
fi-byt-n2820     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time: 481s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 408s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 403s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time: 420s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 489s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 472s
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 458s
fi-kbl-7560u     total:278  pass:267  dwarn:1   dfail:0   fail:0   skip:10  time: 567s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 451s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time: 569s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time: 460s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 494s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time: 430s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time: 537s
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time: 407s

e087f8395ca39c6988de8680bd6f80a20b08c0f4 drm-tip: 2017y-04m-06d-13h-28m-42s UTC integration manifest
6ea878b drm/atomic: Acquire connection_mutex lock in drm_helper_probe_single_connector_modes, v4.

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4425/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] drm/atomic: Acquire connection_mutex lock in drm_helper_probe_single_connector_modes, v4.
  2017-04-06 18:55 [PATCH] drm/atomic: Acquire connection_mutex lock in drm_helper_probe_single_connector_modes, v4 Maarten Lankhorst
  2017-04-06 19:15 ` ✓ Fi.CI.BAT: success for " Patchwork
@ 2017-04-10 10:16 ` Boris Brezillon
  1 sibling, 0 replies; 3+ messages in thread
From: Boris Brezillon @ 2017-04-10 10:16 UTC (permalink / raw)
  To: Maarten Lankhorst; +Cc: intel-gfx, dri-devel

Hi Maarteen,

Sorry for the late review, I was on vacation last week.

On Thu,  6 Apr 2017 20:55:20 +0200
Maarten Lankhorst <maarten.lankhorst@linux.intel.com> wrote:

> mode_valid() called from drm_helper_probe_single_connector_modes()
> may need to look at connector->state because what a valid mode is may
> depend on connector properties being set. For example some HDMI modes
> might be rejected when a connector property forces the connector
> into DVI mode.
> 
> Some implementations of detect() already lock all state,
> so we have to pass an acquire_ctx to them to prevent a deadlock.
> 
> This means changing the function signature of detect() slightly,
> and passing the acquire_ctx for locking multiple crtc's.
> For the callbacks, it will always be non-zero. To allow callers
> not to worry about this, drm_helper_probe_detect_ctx is added
> which might handle -EDEADLK for you.
> 
> Changes since v1:
> - Always set ctx parameter.
> Changes since v2:
> - Always take connection_mutex when probing.
> Changes since v3:
> - Remove the ctx from intel_dp_long_pulse, and add
>   WARN_ON(!connection_mutex) (danvet)
> - Update docs to clarify the locking situation. (danvet)

Maybe this is something DRM-specific, but usually we put the changelog
after the '---' to avoid having it in the final commit.

Same goes for the ", v4" suffix in the commit title, it should be
'[PATCH vX] <commit description>'.

> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_probe_helper.c       | 100 ++++++++++++++++++++++++++++---
>  drivers/gpu/drm/i915/intel_crt.c         |  25 ++++----
>  drivers/gpu/drm/i915/intel_display.c     |  25 ++++----
>  drivers/gpu/drm/i915/intel_dp.c          |  21 +++----
>  drivers/gpu/drm/i915/intel_drv.h         |   8 +--
>  drivers/gpu/drm/i915/intel_hotplug.c     |   3 +-
>  drivers/gpu/drm/i915/intel_tv.c          |  21 +++----
>  include/drm/drm_connector.h              |   6 ++
>  include/drm/drm_crtc_helper.h            |   3 +
>  include/drm/drm_modeset_helper_vtables.h |  36 +++++++++++
>  10 files changed, 187 insertions(+), 61 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index efb5e5e8ce62..1b0c14ab3fff 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -169,12 +169,73 @@ void drm_kms_helper_poll_enable(struct drm_device *dev)
>  EXPORT_SYMBOL(drm_kms_helper_poll_enable);
>  
>  static enum drm_connector_status
> -drm_connector_detect(struct drm_connector *connector, bool force)
> +drm_helper_probe_detect_ctx(struct drm_connector *connector, bool force)

The function name is misleading IMHO. AFAIU, this helper is
instantiating a new context because the caller did not provide one, so
how about renaming it drm_helper_probe_detect_no_ctx()?

>  {
> -	return connector->funcs->detect ?
> -		connector->funcs->detect(connector, force) :
> -		connector_status_connected;
> +	const struct drm_connector_helper_funcs *funcs = connector->helper_private;
> +	struct drm_modeset_acquire_ctx ctx;
> +	int ret;
> +
> +	drm_modeset_acquire_init(&ctx, 0);
> +
> +retry:
> +	ret = drm_modeset_lock(&connector->dev->mode_config.connection_mutex, &ctx);
> +	if (!ret) {
> +		if (funcs->detect_ctx)
> +			ret = funcs->detect_ctx(connector, &ctx, force);
> +		else if (connector->funcs->detect)
> +			ret = connector->funcs->detect(connector, force);
> +		else
> +			ret = connector_status_connected;
> +	}
> +
> +	if (ret == -EDEADLK) {
> +		drm_modeset_backoff(&ctx);
> +		goto retry;
> +	}
> +
> +	if (WARN_ON(ret < 0))
> +		ret = connector_status_unknown;
> +
> +	drm_modeset_drop_locks(&ctx);
> +	drm_modeset_acquire_fini(&ctx);
> +
> +	return ret;
> +}

[...]

>  	/**
> +	 * @detect_ctx:
> +	 *
> +	 * Check to see if anything is attached to the connector. The parameter
> +	 * force is set to false whilst polling, true when checking the
> +	 * connector due to a user request. force can be used by the driver to
> +	 * avoid expensive, destructive operations during automated probing.
> +	 *
> +	 * This callback is optional, if not implemented the connector will be
> +	 * considered as always being attached.
> +	 *
> +	 * This is the atomic version of &drm_connector_funcs.detect.
> +	 *
> +	 * To avoid races against concurrent connector state updates, the
> +	 * helper libraries always call this with ctx set to a valid context,
> +	 * and &drm_mode_config.connection_mutex will always be locked with
> +	 * the ctx parameter set to this ctx. This allows taking additional
> +	 * locks as required.
> +	 *
> +	 * RETURNS:
> +	 *
> +	 * &drm_connector_status indicating the connector's status,
> +	 * or the error code returned by drm_modeset_lock(), -EDEADLK.
> +	 */
> +	int (*detect_ctx)(struct drm_connector *connector,
> +			  struct drm_modeset_acquire_ctx *ctx,
> +			  bool force);

AFAIU (correct me if I'm wrong), those who don't care about the ctx
because they don't need to take extra locks can just ignore it. If this
is the case, I wonder if we shouldn't patch all drivers to use the new
prototype instead of adding a new method (which adds to the DRM API
complexity, even if it's well documented ;-)).

Anyway, this is just my opinion, and Daniel was okay with that, so
don't bother changing that right now.

Regards,

Boris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2017-04-10 10:16 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-06 18:55 [PATCH] drm/atomic: Acquire connection_mutex lock in drm_helper_probe_single_connector_modes, v4 Maarten Lankhorst
2017-04-06 19:15 ` ✓ Fi.CI.BAT: success for " Patchwork
2017-04-10 10:16 ` [PATCH] " Boris Brezillon

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.