Re: [PATCH] drm/atomic: Acquire connection_mutex lock in drm_helper_probe_single_connector_modes.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Mar 29, 2017 at 12:16:50PM +0200, Maarten Lankhorst wrote:
> mode_valid() and get_modes() 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.
> It might be NULL, in which case expensive operations should be avoided.
> 
> intel_dp.c however ignores the force flag, so still lock
> connection_mutex there if needed.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> Cc: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxxxxxxx>
> Cc: Manasi Navare <manasi.d.navare@xxxxxxxxx>

Hm only noticed this now, but mixing up force with the acquire_ctx sounds
like very bad interface design. Yes, the only user of the new hook works
like that, but that's really accidental I think. I think just having the
ctx everywhere (for atomic drivers at least) would be a lot safer. This is
extremely surprising (and undocumented suprise at that).

> ---
>  drivers/gpu/drm/drm_probe_helper.c   | 41 ++++++++++++++++++++++++++++++------
>  drivers/gpu/drm/i915/intel_crt.c     | 29 +++++++++++++------------
>  drivers/gpu/drm/i915/intel_display.c | 25 +++++++++++-----------
>  drivers/gpu/drm/i915/intel_dp.c      | 22 ++++++++++++-------
>  drivers/gpu/drm/i915/intel_drv.h     |  8 +++----
>  drivers/gpu/drm/i915/intel_hotplug.c |  6 +++++-
>  drivers/gpu/drm/i915/intel_tv.c      | 24 ++++++++++-----------
>  include/drm/drm_connector.h          |  5 +++++
>  8 files changed, 101 insertions(+), 59 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index 85005d57bde6..a48cff963871 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -169,11 +169,15 @@ 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_connector_detect(struct drm_connector *connector,
> +		     struct drm_modeset_acquire_ctx *ctx)
>  {
> -	return connector->funcs->detect ?
> -		connector->funcs->detect(connector, force) :
> -		connector_status_connected;
> +	if (connector->funcs->detect_ctx)
> +		return connector->funcs->detect_ctx(connector, ctx);
> +	else if (connector->funcs->detect)
> +		return connector->funcs->detect(connector, !!ctx);
> +	else
> +		return connector_status_connected;
>  }
>  
>  /**
> @@ -239,15 +243,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 +279,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_connector_detect(connector, &ctx);
> +
> +		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 +379,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 +467,7 @@ static void output_poll_execute(struct work_struct *work)
>  
>  		repoll = true;
>  
> -		connector->status = drm_connector_detect(connector, false);
> +		connector->status = drm_connector_detect(connector, NULL);
>  		if (old_status != connector->status) {
>  			const char *old, *new;
>  
> diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
> index 8c82607294c6..1186c3f5fea0 100644
> --- a/drivers/gpu/drm/i915/intel_crt.c
> +++ b/drivers/gpu/drm/i915/intel_crt.c
> @@ -669,19 +669,19 @@ 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)
>  {
>  	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,
> -		      force);
> +		      !!ctx);
>  
>  	/* Skip machines without VGA that falsely report hotplug events */
>  	if (dmi_check_system(intel_spurious_crt_detect))
> @@ -716,15 +716,19 @@ intel_crt_detect(struct drm_connector *connector, bool force)
>  		goto out;
>  	}
>  
> -	if (!force) {
> +	if (!ctx) {
>  		status = connector->status;
>  		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) {
> +		status = ret;
> +		goto out;
> +	}
> +
> +	if (ret > 0) {
>  		if (intel_crt_detect_ddc(connector))
>  			status = connector_status_connected;
>  		else if (INTEL_GEN(dev_priv) < 4)
> @@ -734,13 +738,10 @@ 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);
> +		intel_release_load_detect_pipe(connector, &tmp, ctx);
>  	} else
>  		status = connector_status_unknown;
>  
> -	drm_modeset_drop_locks(&ctx);
> -	drm_modeset_acquire_fini(&ctx);
> -
>  out:
>  	intel_display_power_put(dev_priv, intel_encoder->power_domain);
>  	return status;
> @@ -811,7 +812,7 @@ 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,
> +	.detect_ctx = intel_crt_detect,
>  	.fill_modes = drm_helper_probe_single_connector_modes,
>  	.late_register = intel_connector_register,
>  	.early_unregister = intel_connector_unregister,
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a79063fac951..baa8d836c8e7 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9503,10 +9503,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 =
> @@ -9529,10 +9529,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:
> @@ -9682,10 +9679,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;
>  }
> @@ -15112,6 +15107,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
> @@ -15128,7 +15124,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..10b3727b7381 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4567,7 +4567,8 @@ intel_dp_unset_edid(struct intel_dp *intel_dp)
>  }
>  
>  static enum drm_connector_status
> -intel_dp_long_pulse(struct intel_connector *intel_connector)
> +intel_dp_long_pulse(struct intel_connector *intel_connector,
> +		    struct drm_modeset_acquire_ctx *ctx)
>  {
>  	struct drm_connector *connector = &intel_connector->base;
>  	struct intel_dp *intel_dp = intel_attached_dp(connector);
> @@ -4640,9 +4641,13 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
>  		 * check links status, there has been known issues of
>  		 * link loss triggerring long pulse!!!!
>  		 */
> -		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> +		if (!ctx)
> +			drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> +
>  		intel_dp_check_link_status(intel_dp);
> -		drm_modeset_unlock(&dev->mode_config.connection_mutex);
> +
> +		if (!ctx)
> +			drm_modeset_unlock(&dev->mode_config.connection_mutex);
>  		goto out;
>  	}
>  
> @@ -4682,18 +4687,19 @@ 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)
>  {
>  	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);
>  
>  	/* If full detect is not performed yet, do a full detect */
>  	if (!intel_dp->detect_done)
> -		status = intel_dp_long_pulse(intel_dp->attached_connector);
> +		status = intel_dp_long_pulse(intel_dp->attached_connector, ctx);
>  
>  	intel_dp->detect_done = false;
>  
> @@ -5014,7 +5020,7 @@ 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,
> +	.detect_ctx = intel_dp_detect,
>  	.force = intel_dp_force,
>  	.fill_modes = drm_helper_probe_single_connector_modes,
>  	.set_property = intel_dp_set_property,
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index e24641b559e2..6ea8b9cd68c5 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1362,10 +1362,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..d2d2af7ef305 100644
> --- a/drivers/gpu/drm/i915/intel_hotplug.c
> +++ b/drivers/gpu/drm/i915/intel_hotplug.c
> @@ -243,7 +243,11 @@ 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);
> +	if (connector->funcs->detect_ctx)
> +		connector->status = connector->funcs->detect_ctx(connector, NULL);
> +	else
> +		connector->status = connector->funcs->detect(connector, 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..d2c0120048b2 100644
> --- a/drivers/gpu/drm/i915/intel_tv.c
> +++ b/drivers/gpu/drm/i915/intel_tv.c
> @@ -1315,8 +1315,9 @@ 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)
>  {
>  	struct drm_display_mode mode;
>  	struct intel_tv *intel_tv = intel_attached_tv(connector);
> @@ -1325,27 +1326,26 @@ intel_tv_detect(struct drm_connector *connector, bool force)
>  
>  	DRM_DEBUG_KMS("[CONNECTOR:%d:%s] force=%d\n",
>  		      connector->base.id, connector->name,
> -		      force);
> +		      !!ctx);
>  
>  	mode = reported_modes[0];
>  
> -	if (force) {
> +	if (ctx) {
>  		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 +1516,7 @@ 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,
> +	.detect_ctx = intel_tv_detect,
>  	.late_register = intel_connector_register,
>  	.early_unregister = intel_connector_unregister,
>  	.destroy = intel_tv_destroy,
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 941f57f311aa..834d1ba709ea 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;
> @@ -385,6 +386,10 @@ struct drm_connector_funcs {
>  	enum drm_connector_status (*detect)(struct drm_connector *connector,
>  					    bool force);
>  
> +	/* ctx = NULL <> detect(force = false), but may also return -EDEADLK. */

At least copy-paste the kernel-doc for the existing hook. And run make
hmtldocs to check it ...

When doing that you'll also notice that there's a FIXME telling you the
hook should probably be in drm_connector_helper_funcs.

Thanks, Daniel

> +	int (*detect_ctx)(struct drm_connector *connector,
> +			  struct drm_modeset_acquire_ctx *ctx);
> +
>  	/**
>  	 * @force:
>  	 *
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux