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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> Cc: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxxxxxxx>
> Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> ---
> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
