* [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.