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

[Boris Brezillon]

Hi Maarteen,

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

On Thu,  6 Apr 2017 20:55:20 +0200
Maarten Lankhorst  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] '.

> 
> Signed-off-by: Maarten Lankhorst 
> Cc: Boris Brezillon 
> Reviewed-by: Daniel Vetter 
> ---
>  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(, 0);
> +
> +retry:
> + ret = drm_modeset_lock(>dev->mode_config.connection_mutex, 
> );
> + if (!ret) {
> + if (funcs->detect_ctx)
> + ret = funcs->detect_ctx(connector, , force);
> + else if (connector->funcs->detect)
> + ret = connector->funcs->detect(connector, force);
> + else
> + ret = connector_status_connected;
> + }
> +
> + if (ret == -EDEADLK) {
> + drm_modeset_backoff();
> + goto retry;
> + }
> +
> + if (WARN_ON(ret < 0))
> + ret = connector_status_unknown;
> +
> + drm_modeset_drop_locks();
> + drm_modeset_acquire_fini();
> +
> + 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 _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 _mode_config.connection_mutex will always be locked with
> +  * the ctx parameter set to this ctx. This allows taking additional
> +  * locks as required.
> +  *
> +  * RETURNS:
> +  *
> +  * 

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

[Maarten Lankhorst]

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 
Cc: Boris Brezillon 
Reviewed-by: Daniel Vetter 
---
 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(, 0);
+
+retry:
+   ret = drm_modeset_lock(>dev->mode_config.connection_mutex, 
);
+   if (!ret) {
+   if (funcs->detect_ctx)
+   ret = funcs->detect_ctx(connector, , force);
+   else if (connector->funcs->detect)
+   ret = connector->funcs->detect(connector, force);
+   else
+   ret = connector_status_connected;
+   }
+
+   if (ret == -EDEADLK) {
+   drm_modeset_backoff();
+   goto retry;
+   }
+
+   if (WARN_ON(ret < 0))
+   ret = connector_status_unknown;
+
+   drm_modeset_drop_locks();
+   drm_modeset_acquire_fini();
+
+   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 _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(>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