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

[Boris Brezillon <boris.brezillon@free-electrons.com>]

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