All of lore.kernel.org
 help / color / mirror / Atom feed
From: Daniel Vetter <daniel@ffwll.ch>
To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Boris Brezillon <boris.brezillon@free-electrons.com>,
	intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH] drm/atomic: Acquire connection_mutex lock in drm_helper_probe_single_connector_modes.
Date: Wed, 29 Mar 2017 15:26:45 +0200	[thread overview]
Message-ID: <20170329132645.w5j7gvizgrtkvxon@phenom.ffwll.local> (raw)
In-Reply-To: <1490782610-5481-1-git-send-email-maarten.lankhorst@linux.intel.com>

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@linux.intel.com>
> Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
> Cc: Manasi Navare <manasi.d.navare@intel.com>

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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

  parent reply	other threads:[~2017-03-29 13:26 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-03-29 10:16 [PATCH] drm/atomic: Acquire connection_mutex lock in drm_helper_probe_single_connector_modes Maarten Lankhorst
2017-03-29 10:54 ` ✓ Fi.CI.BAT: success for " Patchwork
2017-03-29 13:26 ` Daniel Vetter [this message]
2017-03-29 13:31   ` [Intel-gfx] [PATCH] " Boris Brezillon
2017-03-29 13:51     ` Maarten Lankhorst
2017-03-29 14:06       ` Daniel Vetter
2017-03-29 14:16         ` Maarten Lankhorst

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170329132645.w5j7gvizgrtkvxon@phenom.ffwll.local \
    --to=daniel@ffwll.ch \
    --cc=boris.brezillon@free-electrons.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=maarten.lankhorst@linux.intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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.