drm/i915: Hold struct_mutex during hotplug processing

Commit Message

Keith Packard July 26, 2011, 5:54 a.m. UTC

On Mon, 25 Jul 2011 22:52:15 -0400, Andrew Lutomirski <[e-mail]> wrote:

> Doesn't help :(

Oh, it helps, just not this issue :-)

> When I do 'xset dpms force off', one of two things happens.  Either
> the display comes back all by itself or it never comes back until I
> power cycle it.

Oh. Right. Actual DPMS, as in power savings. So, I think Jesse and I
were both wrong -- we *do* need to track DPMS in the DP driver so that
we can skip link retraining when DPMS is off. The rest of the connection
is disabled, so doing the link retraining may well confuse the DP system
badly. Turning the monitor off and back on will generate suitable hot
plug messages and presumably clean things up once the rest of the stack
is running again.

> If the display is generating a hotplug event after the dpms code drops
> the link, then with "drm/i915/dp: remove DPMS mode tracking from DP"
> applied the driver will try to bring the display back up.  Maybe my
> display can't handle coming back up that quickly after being told to
> go to sleep, or maybe there's another bug.

Indeed you are correct -- we don't gate the retraining on whether the
monitor is supposed to be running or not.

> Is the original patch supposed to bring the display up if the user
> unplugs it and re-plugs it?  If so, why?

Yes. In the absence of an external agent listening to the hotplug events
(which is hard to arrange these days, unless you're running an X
environment that doesn't watch for monitor hot-plug), the kernel will
see the hot-plug event, decide that because the monitor is supposed to
be running (has a CRTC assigned), it will go ahead and try to retrain
the DP link.

> And shouldn't a dpms off
> command at least stick until a hotplug event reports that the display
> isn't there?

The DPMS off should stick until DPMS is set back on -- hotplug shouldn't
have any effect on DPMS.

Here's a patch which reverts the DPMS tracking, and then fixes the bug
that it had -- you wouldn't get retraining on hotplug after the driver
had been initialized because nothing in the mode setting path would set
the dpms_mode to DRM_MODE_DPMS_ON, so the hotplug code would bail every
time. With that fixed, this patch should work for you *and* for others.

Care to give it a try?

From 59b920597999381fab70c485c161dd50590e561a Mon Sep 17 00:00:00 2001
From: Keith Packard <[e-mail]>
Date: Mon, 25 Jul 2011 22:37:51 -0700
Subject: [PATCH] Revert and fix "drm/i915/dp: remove DPMS mode tracking from
 DP"

This reverts commit 885a50147f00a8a80108904bf58a18af357717f3.

We actually *do* need to track DPMS state so that on hotplug, we don't
retrain the link until DPMS is disabled.

However, that code had a small bug -- it wouldn't set the dpms_mode at
mode set time, and so link retraining would not actually occur on
monitor hotplug until the monitor had gone through a DPMS off/DPMS on
cycle. Setting dpms_mode to DRM_MODE_DPMS_ON in intel_dp_commit fixes that.

Signed-off-by: Keith Packard <[e-mail]>
---
 drivers/gpu/drm/i915/intel_dp.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

Comments

Andrew Lutomirski July 26, 2011, 12:35 p.m. UTC | #1

On Tue, Jul 26, 2011 at 1:54 AM, Keith Packard <[e-mail]> wrote:
> From 59b920597999381fab70c485c161dd50590e561a Mon Sep 17 00:00:00 2001
> From: Keith Packard <[e-mail]>
> Date: Mon, 25 Jul 2011 22:37:51 -0700
> Subject: [PATCH] Revert and fix "drm/i915/dp: remove DPMS mode tracking from
>  DP"
>
> This reverts commit 885a50147f00a8a80108904bf58a18af357717f3.
>
> We actually *do* need to track DPMS state so that on hotplug, we don't
> retrain the link until DPMS is disabled.
>

>        intel_dp->output_reg = output_reg;
> +       intel_dp->dpms_mode = -1;

Should that be some actual mode constant instead of -1?

In any case, this patch, applied manually on top of the struct_mutex
fix, seems to work.  xset dpms force off turns the display off briefly
(presumably X or GNOME helpfully turns it back on), but if I let the
display turn off on its own, it comes back.

There's still an obnoxious bug, though: I use audio over DP, and when
the display turns off, the attached speaker crackles loudly for a
second or two.  I don't think it's a hardware problem with the
monitor, because if I just pull the plug it doesn't make any noise.
Should the driver do something to drop the audio link before turning
off the DP link as a whole?

--Andy

Patch

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 9f134d2..4493641 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -50,6 +50,7 @@  struct intel_dp {
 	bool has_audio;
 	int force_audio;
 	uint32_t color_range;
+	int dpms_mode;
 	uint8_t link_bw;
 	uint8_t lane_count;
 	uint8_t dpcd[8];
@@ -1011,6 +1012,8 @@  static void intel_dp_commit(struct drm_encoder *encoder)
 
 	if (is_edp(intel_dp))
 		ironlake_edp_backlight_on(dev);
+
+	intel_dp->dpms_mode = DRM_MODE_DPMS_ON;
 }
 
 static void
@@ -1045,6 +1048,7 @@  intel_dp_dpms(struct drm_encoder *encoder, int mode)
 		if (is_edp(intel_dp))
 			ironlake_edp_backlight_on(dev);
 	}
+	intel_dp->dpms_mode = mode;
 }
 
 /*
@@ -1591,6 +1595,9 @@  intel_dp_get_dpcd(struct intel_dp *intel_dp)
 static void
 intel_dp_check_link_status(struct intel_dp *intel_dp)
 {
+	if (intel_dp->dpms_mode != DRM_MODE_DPMS_ON)
+		return;
+
 	if (!intel_dp->base.base.crtc)
 		return;
 
@@ -1939,6 +1946,7 @@  intel_dp_init(struct drm_device *dev, int output_reg)
 		return;
 
 	intel_dp->output_reg = output_reg;
+	intel_dp->dpms_mode = -1;
 
 	intel_connector = kzalloc(sizeof(struct intel_connector), GFP_KERNEL);
 	if (!intel_connector) {