| Message ID | 391b8727-c981-c728-30ed-90b5f8c116f5@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Apr 05, 2017 at 10:11:18AM +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. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Boris Brezillon <boris.brezillon@free-electrons.com> A few minor nits, mostly to improve checks and docs. With those all addressed: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > Fixed drm_helper_probe_detect, > is this version better? > 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 | 24 ++++---- > 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 | 5 ++ > include/drm/drm_crtc_helper.h | 3 + > include/drm/drm_modeset_helper_vtables.h | 24 ++++++++ > 10 files changed, 175 insertions(+), 63 deletions(-) > > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c > index efb5e5e8ce62..ba0160446aac 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(&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; > +} > + > +/** > + * 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 drm_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(&dev->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 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 +336,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_helper_probe_detect(connector, &ctx, true); > + > + 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 +436,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 +524,7 @@ static void output_poll_execute(struct work_struct *work) > > repoll = true; > > - connector->status = drm_connector_detect(connector, false); > + connector->status = drm_helper_probe_detect(connector, NULL, false); > if (old_status != connector->status) { > const char *old, *new; > > @@ -588,7 +672,7 @@ bool drm_helper_hpd_irq_event(struct drm_device *dev) > > old_status = connector->status; > > - connector->status = drm_connector_detect(connector, false); > + connector->status = drm_helper_probe_detect(connector, NULL, false); > DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %s to %s\n", > connector->base.id, > connector->name, > diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c > index 8c82607294c6..2797bf37c3ac 100644 > --- a/drivers/gpu/drm/i915/intel_crt.c > +++ b/drivers/gpu/drm/i915/intel_crt.c > @@ -669,15 +669,16 @@ 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, > + bool force) > { > 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, > @@ -721,10 +722,9 @@ intel_crt_detect(struct drm_connector *connector, bool force) > 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) { > if (intel_crt_detect_ddc(connector)) > status = connector_status_connected; > else if (INTEL_GEN(dev_priv) < 4) > @@ -734,12 +734,11 @@ 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); > - } else > + intel_release_load_detect_pipe(connector, &tmp, ctx); > + } else if (ret == 0) > status = connector_status_unknown; > - > - drm_modeset_drop_locks(&ctx); > - drm_modeset_acquire_fini(&ctx); > + else if (ret < 0) > + status = ret; > > out: > intel_display_power_put(dev_priv, intel_encoder->power_domain); > @@ -811,7 +810,6 @@ 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, > .fill_modes = drm_helper_probe_single_connector_modes, > .late_register = intel_connector_register, > .early_unregister = intel_connector_unregister, > @@ -823,6 +821,7 @@ static const struct drm_connector_funcs intel_crt_connector_funcs = { > }; > > static const struct drm_connector_helper_funcs intel_crt_connector_helper_funcs = { > + .detect_ctx = intel_crt_detect, > .mode_valid = intel_crt_mode_valid, > .get_modes = intel_crt_get_modes, > }; > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index bf81afe2748c..a76a8b040ccf 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; > } > @@ -15116,6 +15111,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 > @@ -15132,7 +15128,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..68896d4742cb 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -4566,8 +4566,9 @@ intel_dp_unset_edid(struct intel_dp *intel_dp) > intel_dp->has_audio = false; > } > > -static enum drm_connector_status > -intel_dp_long_pulse(struct intel_connector *intel_connector) > +static int > +intel_dp_long_pulse(struct intel_connector *intel_connector, > + struct drm_modeset_acquire_ctx *ctx) We don't really seem to need the ctx, but no harm in wiring this up either. > { > struct drm_connector *connector = &intel_connector->base; > struct intel_dp *intel_dp = intel_attached_dp(connector); > @@ -4635,14 +4636,7 @@ intel_dp_long_pulse(struct intel_connector *intel_connector) > status = connector_status_disconnected; > goto out; > } else if (connector->status == connector_status_connected) { > - /* > - * If display was connected already and is still connected > - * check links status, there has been known issues of > - * link loss triggerring long pulse!!!! > - */ > - drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); WARN_ON(!modeset_locked) would be good here instead. Just in case we refactor and move this into e.g. the hpd handler. > intel_dp_check_link_status(intel_dp); > - drm_modeset_unlock(&dev->mode_config.connection_mutex); > goto out; > } > > @@ -4682,18 +4676,20 @@ 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, > + bool force) > { > 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 +5010,6 @@ 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, > .force = intel_dp_force, > .fill_modes = drm_helper_probe_single_connector_modes, > .set_property = intel_dp_set_property, > @@ -5027,6 +5022,7 @@ static const struct drm_connector_funcs intel_dp_connector_funcs = { > }; > > static const struct drm_connector_helper_funcs intel_dp_connector_helper_funcs = { > + .detect_ctx = intel_dp_detect, > .get_modes = intel_dp_get_modes, > .mode_valid = intel_dp_mode_valid, > }; > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 313fad059bfa..aaee3949a422 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1358,10 +1358,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..f1200272a699 100644 > --- a/drivers/gpu/drm/i915/intel_hotplug.c > +++ b/drivers/gpu/drm/i915/intel_hotplug.c > @@ -243,7 +243,8 @@ 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); > + connector->status = drm_helper_probe_detect(connector, NULL, 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..e077c2a9e694 100644 > --- a/drivers/gpu/drm/i915/intel_tv.c > +++ b/drivers/gpu/drm/i915/intel_tv.c > @@ -1315,8 +1315,10 @@ 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, > + bool force) > { > struct drm_display_mode mode; > struct intel_tv *intel_tv = intel_attached_tv(connector); > @@ -1331,21 +1333,20 @@ intel_tv_detect(struct drm_connector *connector, bool force) > > if (force) { > 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 +1517,6 @@ 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, > .late_register = intel_connector_register, > .early_unregister = intel_connector_unregister, > .destroy = intel_tv_destroy, > @@ -1528,6 +1528,7 @@ static const struct drm_connector_funcs intel_tv_connector_funcs = { > }; > > static const struct drm_connector_helper_funcs intel_tv_connector_helper_funcs = { > + .detect_ctx = intel_tv_detect, > .mode_valid = intel_tv_mode_valid, > .get_modes = intel_tv_get_modes, > }; > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h > index 941f57f311aa..8d36ab2b1666 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; > @@ -378,6 +379,10 @@ struct drm_connector_funcs { > * the helper library vtable purely for historical reasons. The only DRM > * core entry point to probe connector state is @fill_modes. > * > + * Atomic drivers that might have state that shouldn't race against > + * atomic_check must use &drm_connector_helper_funcs.detect_ctx > + * instead. Nah, we already hold the lock. Instead: "Note that the helper library will already hold @drm_mode_config.connection_mutex. Drivers which need to grab additional locks to avoid races with concurrent modeset changes need to instead use &drm_connector_helper_funcs.detect.ctx." Also, this patch changes the samantics for @force and @get_modes. Please add there a new para to explain this, e.g. "To avoid races with concurrent modeset changes the helper libraries always call this with the &drm_mode_config.connection_mutex held." Also, it would be good if we could smash some more WARN_ON(!locked) over our get_modes functions, to make sure this won't break. We can do that later on in the follow-up patch series to fix up all the i915 connector state though. > + * > * RETURNS: > * > * drm_connector_status indicating the connector's status. > diff --git a/include/drm/drm_crtc_helper.h b/include/drm/drm_crtc_helper.h > index 43505c7b2b3f..76e237bd989b 100644 > --- a/include/drm/drm_crtc_helper.h > +++ b/include/drm/drm_crtc_helper.h > @@ -67,6 +67,9 @@ int drm_helper_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y, > int drm_helper_probe_single_connector_modes(struct drm_connector > *connector, uint32_t maxX, > uint32_t maxY); > +int drm_helper_probe_detect(struct drm_connector *connector, > + struct drm_modeset_acquire_ctx *ctx, > + bool force); > void drm_kms_helper_poll_init(struct drm_device *dev); > void drm_kms_helper_poll_fini(struct drm_device *dev); > bool drm_helper_hpd_irq_event(struct drm_device *dev); > diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h > index 091c42205667..b304950b402d 100644 > --- a/include/drm/drm_modeset_helper_vtables.h > +++ b/include/drm/drm_modeset_helper_vtables.h > @@ -754,6 +754,30 @@ struct drm_connector_helper_funcs { > int (*get_modes)(struct drm_connector *connector); > > /** > + * @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. > + * > + * ctx is always set, and connection_mutex is always locked. &drm_mode_config.connection_mutex > + * > + * RETURNS: > + * > + * drm_connector_status indicating the connector's status, &enum drm_connector_status > + * or the error code returned by drm_modeset_lock (-EDEADLK). drm_modeset_lock() Please build docs to double-check all the links work and point in the right direction :-) > + */ > + int (*detect_ctx)(struct drm_connector *connector, > + struct drm_modeset_acquire_ctx *ctx, > + bool force); > + > + /** > * @mode_valid: > * > * Callback to validate a mode for a connector, irrespective of the > -- > 2.7.4 > >
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c index efb5e5e8ce62..ba0160446aac 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(&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; +} + +/** + * 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 drm_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(&dev->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 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 +336,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_helper_probe_detect(connector, &ctx, true); + + 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 +436,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 +524,7 @@ static void output_poll_execute(struct work_struct *work) repoll = true; - connector->status = drm_connector_detect(connector, false); + connector->status = drm_helper_probe_detect(connector, NULL, false); if (old_status != connector->status) { const char *old, *new; @@ -588,7 +672,7 @@ bool drm_helper_hpd_irq_event(struct drm_device *dev) old_status = connector->status; - connector->status = drm_connector_detect(connector, false); + connector->status = drm_helper_probe_detect(connector, NULL, false); DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %s to %s\n", connector->base.id, connector->name, diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c index 8c82607294c6..2797bf37c3ac 100644 --- a/drivers/gpu/drm/i915/intel_crt.c +++ b/drivers/gpu/drm/i915/intel_crt.c @@ -669,15 +669,16 @@ 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, + bool force) { 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, @@ -721,10 +722,9 @@ intel_crt_detect(struct drm_connector *connector, bool force) 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) { if (intel_crt_detect_ddc(connector)) status = connector_status_connected; else if (INTEL_GEN(dev_priv) < 4) @@ -734,12 +734,11 @@ 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); - } else + intel_release_load_detect_pipe(connector, &tmp, ctx); + } else if (ret == 0) status = connector_status_unknown; - - drm_modeset_drop_locks(&ctx); - drm_modeset_acquire_fini(&ctx); + else if (ret < 0) + status = ret; out: intel_display_power_put(dev_priv, intel_encoder->power_domain); @@ -811,7 +810,6 @@ 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, .fill_modes = drm_helper_probe_single_connector_modes, .late_register = intel_connector_register, .early_unregister = intel_connector_unregister, @@ -823,6 +821,7 @@ static const struct drm_connector_funcs intel_crt_connector_funcs = { }; static const struct drm_connector_helper_funcs intel_crt_connector_helper_funcs = { + .detect_ctx = intel_crt_detect, .mode_valid = intel_crt_mode_valid, .get_modes = intel_crt_get_modes, }; diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index bf81afe2748c..a76a8b040ccf 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; } @@ -15116,6 +15111,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 @@ -15132,7 +15128,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..68896d4742cb 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -4566,8 +4566,9 @@ intel_dp_unset_edid(struct intel_dp *intel_dp) intel_dp->has_audio = false; } -static enum drm_connector_status -intel_dp_long_pulse(struct intel_connector *intel_connector) +static int +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); @@ -4635,14 +4636,7 @@ intel_dp_long_pulse(struct intel_connector *intel_connector) status = connector_status_disconnected; goto out; } else if (connector->status == connector_status_connected) { - /* - * If display was connected already and is still connected - * check links status, there has been known issues of - * link loss triggerring long pulse!!!! - */ - drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); intel_dp_check_link_status(intel_dp); - drm_modeset_unlock(&dev->mode_config.connection_mutex); goto out; } @@ -4682,18 +4676,20 @@ 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, + bool force) { 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 +5010,6 @@ 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, .force = intel_dp_force, .fill_modes = drm_helper_probe_single_connector_modes, .set_property = intel_dp_set_property, @@ -5027,6 +5022,7 @@ static const struct drm_connector_funcs intel_dp_connector_funcs = { }; static const struct drm_connector_helper_funcs intel_dp_connector_helper_funcs = { + .detect_ctx = intel_dp_detect, .get_modes = intel_dp_get_modes, .mode_valid = intel_dp_mode_valid, }; diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 313fad059bfa..aaee3949a422 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1358,10 +1358,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..f1200272a699 100644 --- a/drivers/gpu/drm/i915/intel_hotplug.c +++ b/drivers/gpu/drm/i915/intel_hotplug.c @@ -243,7 +243,8 @@ 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); + connector->status = drm_helper_probe_detect(connector, NULL, 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..e077c2a9e694 100644 --- a/drivers/gpu/drm/i915/intel_tv.c +++ b/drivers/gpu/drm/i915/intel_tv.c @@ -1315,8 +1315,10 @@ 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, + bool force) { struct drm_display_mode mode; struct intel_tv *intel_tv = intel_attached_tv(connector); @@ -1331,21 +1333,20 @@ intel_tv_detect(struct drm_connector *connector, bool force) if (force) { 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 +1517,6 @@ 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, .late_register = intel_connector_register, .early_unregister = intel_connector_unregister, .destroy = intel_tv_destroy, @@ -1528,6 +1528,7 @@ static const struct drm_connector_funcs intel_tv_connector_funcs = { }; static const struct drm_connector_helper_funcs intel_tv_connector_helper_funcs = { + .detect_ctx = intel_tv_detect, .mode_valid = intel_tv_mode_valid, .get_modes = intel_tv_get_modes, }; diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index 941f57f311aa..8d36ab2b1666 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; @@ -378,6 +379,10 @@ struct drm_connector_funcs { * the helper library vtable purely for historical reasons. The only DRM * core entry point to probe connector state is @fill_modes. * + * Atomic drivers that might have state that shouldn't race against + * atomic_check must use &drm_connector_helper_funcs.detect_ctx + * instead. + * * RETURNS: * * drm_connector_status indicating the connector's status. diff --git a/include/drm/drm_crtc_helper.h b/include/drm/drm_crtc_helper.h index 43505c7b2b3f..76e237bd989b 100644 --- a/include/drm/drm_crtc_helper.h +++ b/include/drm/drm_crtc_helper.h @@ -67,6 +67,9 @@ int drm_helper_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y, int drm_helper_probe_single_connector_modes(struct drm_connector *connector, uint32_t maxX, uint32_t maxY); +int drm_helper_probe_detect(struct drm_connector *connector, + struct drm_modeset_acquire_ctx *ctx, + bool force); void drm_kms_helper_poll_init(struct drm_device *dev); void drm_kms_helper_poll_fini(struct drm_device *dev); bool drm_helper_hpd_irq_event(struct drm_device *dev); diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h index 091c42205667..b304950b402d 100644 --- a/include/drm/drm_modeset_helper_vtables.h +++ b/include/drm/drm_modeset_helper_vtables.h @@ -754,6 +754,30 @@ struct drm_connector_helper_funcs { int (*get_modes)(struct drm_connector *connector); /** + * @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. + * + * ctx is always set, and connection_mutex is always locked. + * + * 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); + + /** * @mode_valid: * * Callback to validate a mode for a connector, irrespective of the
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. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Boris Brezillon <boris.brezillon@free-electrons.com> --- Fixed drm_helper_probe_detect, is this version better? 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 | 24 ++++---- 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 | 5 ++ include/drm/drm_crtc_helper.h | 3 + include/drm/drm_modeset_helper_vtables.h | 24 ++++++++ 10 files changed, 175 insertions(+), 63 deletions(-)