* [PATCH] drm/atomic: Acquire connection_mutex lock in drm_helper_probe_single_connector_modes. @ 2017-03-29 10:16 Maarten Lankhorst 2017-03-29 10:54 ` ✓ Fi.CI.BAT: success for " Patchwork 2017-03-29 13:26 ` [PATCH] " Daniel Vetter 0 siblings, 2 replies; 7+ messages in thread From: Maarten Lankhorst @ 2017-03-29 10:16 UTC (permalink / raw) To: dri-devel; +Cc: Manasi Navare, intel-gfx 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> --- 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. */ + int (*detect_ctx)(struct drm_connector *connector, + struct drm_modeset_acquire_ctx *ctx); + /** * @force: * -- 2.7.4 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 7+ messages in thread
* ✓ Fi.CI.BAT: success for drm/atomic: Acquire connection_mutex lock in drm_helper_probe_single_connector_modes. 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 ` Patchwork 2017-03-29 13:26 ` [PATCH] " Daniel Vetter 1 sibling, 0 replies; 7+ messages in thread From: Patchwork @ 2017-03-29 10:54 UTC (permalink / raw) To: Maarten Lankhorst; +Cc: intel-gfx == Series Details == Series: drm/atomic: Acquire connection_mutex lock in drm_helper_probe_single_connector_modes. URL : https://patchwork.freedesktop.org/series/22073/ State : success == Summary == Series 22073v1 drm/atomic: Acquire connection_mutex lock in drm_helper_probe_single_connector_modes. https://patchwork.freedesktop.org/api/1.0/series/22073/revisions/1/mbox/ Test gem_exec_suspend: Subgroup basic-s4-devices: dmesg-warn -> PASS (fi-kbl-7560u) fdo#100428 dmesg-warn -> PASS (fi-snb-2600) fdo#100428 https://bugs.freedesktop.org/show_bug.cgi?id=100428 fi-bdw-5557u total:278 pass:267 dwarn:0 dfail:0 fail:0 skip:11 time: 466s fi-bdw-gvtdvm total:278 pass:256 dwarn:8 dfail:0 fail:0 skip:14 time: 453s fi-bsw-n3050 total:278 pass:239 dwarn:0 dfail:0 fail:0 skip:39 time: 583s fi-bxt-j4205 total:278 pass:259 dwarn:0 dfail:0 fail:0 skip:19 time: 538s fi-bxt-t5700 total:278 pass:258 dwarn:0 dfail:0 fail:0 skip:20 time: 582s fi-byt-j1900 total:278 pass:251 dwarn:0 dfail:0 fail:0 skip:27 time: 502s fi-byt-n2820 total:278 pass:247 dwarn:0 dfail:0 fail:0 skip:31 time: 510s fi-hsw-4770 total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time: 436s fi-hsw-4770r total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time: 432s fi-ilk-650 total:278 pass:228 dwarn:0 dfail:0 fail:0 skip:50 time: 435s fi-ivb-3520m total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time: 513s fi-ivb-3770 total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time: 495s fi-kbl-7500u total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time: 481s fi-kbl-7560u total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time: 594s fi-skl-6260u total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time: 479s fi-skl-6700hq total:278 pass:261 dwarn:0 dfail:0 fail:0 skip:17 time: 597s fi-skl-6700k total:278 pass:256 dwarn:4 dfail:0 fail:0 skip:18 time: 494s fi-skl-6770hq total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time: 523s fi-skl-gvtdvm total:278 pass:265 dwarn:0 dfail:0 fail:0 skip:13 time: 470s fi-snb-2520m total:278 pass:250 dwarn:0 dfail:0 fail:0 skip:28 time: 546s fi-snb-2600 total:278 pass:249 dwarn:0 dfail:0 fail:0 skip:29 time: 420s 1610e07cbf696204259f584fcc74fde1c16a4d29 drm-tip: 2017y-03m-29d-10h-12m-03s UTC integration manifest 7205f15 drm/atomic: Acquire connection_mutex lock in drm_helper_probe_single_connector_modes. == Logs == For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4337/ _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/atomic: Acquire connection_mutex lock in drm_helper_probe_single_connector_modes. 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 2017-03-29 13:31 ` [Intel-gfx] " Boris Brezillon 1 sibling, 1 reply; 7+ messages in thread From: Daniel Vetter @ 2017-03-29 13:26 UTC (permalink / raw) To: Maarten Lankhorst; +Cc: Boris Brezillon, intel-gfx, dri-devel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/atomic: Acquire connection_mutex lock in drm_helper_probe_single_connector_modes. 2017-03-29 13:26 ` [PATCH] " Daniel Vetter @ 2017-03-29 13:31 ` Boris Brezillon 2017-03-29 13:51 ` Maarten Lankhorst 0 siblings, 1 reply; 7+ messages in thread From: Boris Brezillon @ 2017-03-29 13:31 UTC (permalink / raw) To: Daniel Vetter; +Cc: intel-gfx, dri-devel On Wed, 29 Mar 2017 15:26:45 +0200 Daniel Vetter <daniel@ffwll.ch> wrote: > 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). Yes, I was about to say the same thing: the interface is not very clear, and I don't understand why ctx = NULL implies force = false. _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/atomic: Acquire connection_mutex lock in drm_helper_probe_single_connector_modes. 2017-03-29 13:31 ` [Intel-gfx] " Boris Brezillon @ 2017-03-29 13:51 ` Maarten Lankhorst 2017-03-29 14:06 ` Daniel Vetter 0 siblings, 1 reply; 7+ messages in thread From: Maarten Lankhorst @ 2017-03-29 13:51 UTC (permalink / raw) To: Boris Brezillon, Daniel Vetter; +Cc: intel-gfx, dri-devel Op 29-03-17 om 15:31 schreef Boris Brezillon: > On Wed, 29 Mar 2017 15:26:45 +0200 > Daniel Vetter <daniel@ffwll.ch> wrote: > >> 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). > Yes, I was about to say the same thing: the interface is not very > clear, and I don't understand why ctx = NULL implies force = false. They're the same thing I fear. I could perhaps call it force_ctx instead, but non-zero ctx implies force, and other way around. Though I guess we could relax it, and have force = true imply ctx, but not the other way around. Would that be ok? _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/atomic: Acquire connection_mutex lock in drm_helper_probe_single_connector_modes. 2017-03-29 13:51 ` Maarten Lankhorst @ 2017-03-29 14:06 ` Daniel Vetter 2017-03-29 14:16 ` Maarten Lankhorst 0 siblings, 1 reply; 7+ messages in thread From: Daniel Vetter @ 2017-03-29 14:06 UTC (permalink / raw) To: Maarten Lankhorst; +Cc: intel-gfx, dri-devel On Wed, Mar 29, 2017 at 03:51:08PM +0200, Maarten Lankhorst wrote: > Op 29-03-17 om 15:31 schreef Boris Brezillon: > > On Wed, 29 Mar 2017 15:26:45 +0200 > > Daniel Vetter <daniel@ffwll.ch> wrote: > > > >> 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). > > Yes, I was about to say the same thing: the interface is not very > > clear, and I don't understand why ctx = NULL implies force = false. > > They're the same thing I fear. I could perhaps call it force_ctx instead, > but non-zero ctx implies force, and other way around. Though I guess we could > relax it, and have force = true imply ctx, but not the other way around. > > Would that be ok? Why can't we supply a ctx always? I didn't see any reason not to in the code ... -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/atomic: Acquire connection_mutex lock in drm_helper_probe_single_connector_modes. 2017-03-29 14:06 ` Daniel Vetter @ 2017-03-29 14:16 ` Maarten Lankhorst 0 siblings, 0 replies; 7+ messages in thread From: Maarten Lankhorst @ 2017-03-29 14:16 UTC (permalink / raw) To: Daniel Vetter; +Cc: intel-gfx, dri-devel Op 29-03-17 om 16:06 schreef Daniel Vetter: > On Wed, Mar 29, 2017 at 03:51:08PM +0200, Maarten Lankhorst wrote: >> Op 29-03-17 om 15:31 schreef Boris Brezillon: >>> On Wed, 29 Mar 2017 15:26:45 +0200 >>> Daniel Vetter <daniel@ffwll.ch> wrote: >>> >>>> 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). >>> Yes, I was about to say the same thing: the interface is not very >>> clear, and I don't understand why ctx = NULL implies force = false. >> They're the same thing I fear. I could perhaps call it force_ctx instead, >> but non-zero ctx implies force, and other way around. Though I guess we could >> relax it, and have force = true imply ctx, but not the other way around. >> >> Would that be ok? > Why can't we supply a ctx always? I didn't see any reason not to in the > code ... > -Daniel Then drm_helper_hpd_irq_event and intel_hpd_irq_event have to do -EDEADLK handling too, it could be done but nothing would return -EDEADLK so it's unused code. _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-03-29 14:16 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 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 ` [PATCH] " Daniel Vetter 2017-03-29 13:31 ` [Intel-gfx] " Boris Brezillon 2017-03-29 13:51 ` Maarten Lankhorst 2017-03-29 14:06 ` Daniel Vetter 2017-03-29 14:16 ` Maarten Lankhorst
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.