diff mbox

Poll for HDMI disconnects.

Message ID 1349998074-10414-1-git-send-email-sabercrombie@chromium.org (mailing list archive)
State New, archived
Headers show

Commit Message

Stuart Abercrombie Oct. 11, 2012, 11:27 p.m. UTC 
Following a hotplug interrupt the driver uses a successful EDID read to indicate HDMI sink presence.

This leads to missing HDMI cable unplug events because the DDC lines can remain up, allowing an EDID read to complete, well after the HPD line goes down during unplugging

Since it is only the disconnect case that suffers from unplug ordering problems with the DDC lines, restrict polling to that.  Otherwise we waste power.
---
 drivers/gpu/drm/drm_crtc_helper.c |    8 ++++++--
 drivers/gpu/drm/i915/intel_hdmi.c |    2 +-
 include/drm/drm_crtc.h            |    2 ++
 3 files changed, 9 insertions(+), 3 deletions(-)

Comments

Daniel Vetter Oct. 12, 2012, 8:24 a.m. UTC | #1 
On Thu, Oct 11, 2012 at 04:27:54PM -0700, Stuart Abercrombie wrote:
> Following a hotplug interrupt the driver uses a successful EDID read to indicate HDMI sink presence.
> 
> This leads to missing HDMI cable unplug events because the DDC lines can remain up, allowing an EDID read to complete, well after the HPD line goes down during unplugging
> 
> Since it is only the disconnect case that suffers from unplug ordering problems with the DDC lines, restrict polling to that.  Otherwise we waste power.

Nope, the real fix is to simply check the status of the hpd line before
trying the edid read. We already have that for g4x class chips, check
g4x_hdmi_connected. We'd need to add similar checks for all other
platforms.
-Daniel

> ---
>  drivers/gpu/drm/drm_crtc_helper.c |    8 ++++++--
>  drivers/gpu/drm/i915/intel_hdmi.c |    2 +-
>  include/drm/drm_crtc.h            |    2 ++
>  3 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
> index 3252e70..b38ea4f 100644
> --- a/drivers/gpu/drm/drm_crtc_helper.c
> +++ b/drivers/gpu/drm/drm_crtc_helper.c
> @@ -938,8 +938,12 @@ static void output_poll_execute(struct work_struct *work)
>  		if (!connector->polled)
>  			continue;
>  
> -		else if (connector->polled & (DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT))
> -			repoll = true;
> +		if (connector->polled & (DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT)) {
> +			if (connector->polled & DRM_CONNECTOR_POLL_DISCONNECT_ONLY)
> +				repoll = (connector->status == connector_status_connected);
> +			else
> +				repoll = true;
> +		}
>  
>  		old_status = connector->status;
>  		/* if we are connected and don't want to poll for disconnect
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 229897f..246e8f4 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -964,7 +964,7 @@ void intel_hdmi_init(struct drm_device *dev, int sdvox_reg, enum port port)
>  
>  	intel_encoder->type = INTEL_OUTPUT_HDMI;
>  
> -	connector->polled = DRM_CONNECTOR_POLL_HPD;
> +	connector->polled = DRM_CONNECTOR_POLL_DISCONNECT | DRM_CONNECTOR_POLL_DISCONNECT_ONLY;
>  	connector->interlace_allowed = 1;
>  	connector->doublescan_allowed = 0;
>  	intel_encoder->crtc_mask = (1 << 0) | (1 << 1) | (1 << 2);
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 316ce64..a60abb5 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -503,6 +503,8 @@ enum drm_connector_force {
>  /* can cleanly poll for disconnections without flickering the screen */
>  /* DACs should rarely do this without a lot of testing */
>  #define DRM_CONNECTOR_POLL_DISCONNECT (1 << 2)
> +/* Only poll for disconnections. */
> +#define DRM_CONNECTOR_POLL_DISCONNECT_ONLY (1 << 3)
>  
>  #define MAX_ELD_BYTES	128
>  
> -- 
> 1.7.7.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Stuart Abercrombie Oct. 12, 2012, 5:29 p.m. UTC | #2 
> Nope, the real fix is to simply check the status of the hpd line before
> trying the edid read. We already have that for g4x class chips, check
> g4x_hdmi_connected. We'd need to add similar checks for all other
> platforms.
> -Daniel

That would be preferable.  Unfortunately we did not find a way to
check the HPD line status.  See
https://bugs.freedesktop.org/show_bug.cgi?id=55372.

Where is this status available to be read?

Stuart
Daniel Vetter Oct. 12, 2012, 6:05 p.m. UTC | #3 
On Fri, Oct 12, 2012 at 10:29:38AM -0700, Stuart Abercrombie wrote:
> > Nope, the real fix is to simply check the status of the hpd line before
> > trying the edid read. We already have that for g4x class chips, check
> > g4x_hdmi_connected. We'd need to add similar checks for all other
> > platforms.
> > -Daniel
> 
> That would be preferable.  Unfortunately we did not find a way to
> check the HPD line status.  See
> https://bugs.freedesktop.org/show_bug.cgi?id=55372.
> 
> Where is this status available to be read?

In the hotplug register, like on g4x. But that moved to to PCH_IIR on pch
platforms. I plan to rework the entire hotplug handling for 3.8, hence why
I haven't bothered to wire this up yet.

Yours, Daniel
Stuart Abercrombie Oct. 12, 2012, 6:40 p.m. UTC | #4 
> In the hotplug register, like on g4x. But that moved to to PCH_IIR on pch
> platforms. I plan to rework the entire hotplug handling for 3.8, hence why
> I haven't bothered to wire this up yet.

You are saying that the HPD line state is available in the register
called SDEIIR in the code?  The documentation only describes pulse
detection bits in this register, not anything directly reflecting the
line state.  The same is true of PCH_PORT_HOTPLUG/SHOTPLUG_CTL.

Testing did not show these registers, or others in the same range,
reflecting the HPD line state.

Hence my question.

Stuart
Daniel Vetter Oct. 12, 2012, 6:45 p.m. UTC | #5 
On Fri, Oct 12, 2012 at 8:40 PM, Stuart Abercrombie
<sabercrombie@chromium.org> wrote:
>> In the hotplug register, like on g4x. But that moved to to PCH_IIR on pch
>> platforms. I plan to rework the entire hotplug handling for 3.8, hence why
>> I haven't bothered to wire this up yet.
>
> You are saying that the HPD line state is available in the register
> called SDEIIR in the code?  The documentation only describes pulse
> detection bits in this register, not anything directly reflecting the
> line state.  The same is true of PCH_PORT_HOTPLUG/SHOTPLUG_CTL.
>
> Testing did not show these registers, or others in the same range,
> reflecting the HPD line state.
>
> Hence my question.

My docs here say that the SDE_ISR reg contains what we want - high
level irq bit when the hpd line is enabled. I admit, I haven't tested
this ...
-Daniel
Stuart Abercrombie Oct. 12, 2012, 7:03 p.m. UTC | #6 
> My docs here say that the SDE_ISR reg contains what we want - high
> level irq bit when the hpd line is enabled. I admit, I haven't tested
> this ...

I'm looking at 2.1.1 in this
http://intellinuxgraphics.org/documentation/SNB/IHD_OS_Vol3_Part3.pdf.
 All it has relating to hotplug in SDEIIR are D, C and B versions of
this:

"The ISR is an active high level representing the DIgital Port B
hotplug line when the Digital Port B hotplug detect input is enabled.
The unmasked IIR is set on either a short or long pulse detection
status in the Digital Port Hot Plug Control Register."

The SHOTPLUG_CTL description in 2.1.6 says DP_B_HPD_Status "reflects
the hot plug detect status on the digital port B... When either a long
or short pulse is detected, one of these bits will set.  These bits
are ORed together to go in the main ISR hotplug register bit."

Based on this description, and observed behavior, it looks as if these
registers only tell you about pulses/interrupt sources, not the line
state.

Am I missing something?

Stuart
Daniel Vetter Oct. 12, 2012, 7:27 p.m. UTC | #7 
On Fri, Oct 12, 2012 at 9:03 PM, Stuart Abercrombie
<sabercrombie@chromium.org> wrote:
>> My docs here say that the SDE_ISR reg contains what we want - high
>> level irq bit when the hpd line is enabled. I admit, I haven't tested
>> this ...
>
> I'm looking at 2.1.1 in this
> http://intellinuxgraphics.org/documentation/SNB/IHD_OS_Vol3_Part3.pdf.
>  All it has relating to hotplug in SDEIIR are D, C and B versions of
> this:
>
> "The ISR is an active high level representing the DIgital Port B
> hotplug line when the Digital Port B hotplug detect input is enabled.
> The unmasked IIR is set on either a short or long pulse detection
> status in the Digital Port Hot Plug Control Register."
>
> The SHOTPLUG_CTL description in 2.1.6 says DP_B_HPD_Status "reflects
> the hot plug detect status on the digital port B... When either a long
> or short pulse is detected, one of these bits will set.  These bits
> are ORed together to go in the main ISR hotplug register bit."
>
> Based on this description, and observed behavior, it looks as if these
> registers only tell you about pulses/interrupt sources, not the line
> state.
>
> Am I missing something?

I've tried to be slightly lazy than in my previous mail and quickly
tested this on my snb here: Bit 23 in SDEISR (0xc4000) is set when the
cable is plugged in, and cleared when nothing is plugged in. Afaict it
works as advertised. Note tha the SDE irq definitions nicely
differentiates between "active high pulse" type interrupts and "active
high level" interrupts. The hpd irq sources are all of the level type.

Cheers, Daniel
Stuart Abercrombie Oct. 12, 2012, 9:32 p.m. UTC | #8 
> I've tried to be slightly lazy than in my previous mail and quickly
> tested this on my snb here: Bit 23 in SDEISR (0xc4000) is set when the
> cable is plugged in, and cleared when nothing is plugged in. Afaict it
> works as advertised. Note tha the SDE irq definitions nicely
> differentiates between "active high pulse" type interrupts and "active
> high level" interrupts. The hpd irq sources are all of the level type.

OK, so this is more complicated than I realized.  For whatever reason
this bit does not work with all monitor configurations.  Thanks for
clarifying the expected behavior.

Stuart
diff mbox

Patch

diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
index 3252e70..b38ea4f 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -938,8 +938,12 @@  static void output_poll_execute(struct work_struct *work)
 		if (!connector->polled)
 			continue;
 
-		else if (connector->polled & (DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT))
-			repoll = true;
+		if (connector->polled & (DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT)) {
+			if (connector->polled & DRM_CONNECTOR_POLL_DISCONNECT_ONLY)
+				repoll = (connector->status == connector_status_connected);
+			else
+				repoll = true;
+		}
 
 		old_status = connector->status;
 		/* if we are connected and don't want to poll for disconnect
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 229897f..246e8f4 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -964,7 +964,7 @@  void intel_hdmi_init(struct drm_device *dev, int sdvox_reg, enum port port)
 
 	intel_encoder->type = INTEL_OUTPUT_HDMI;
 
-	connector->polled = DRM_CONNECTOR_POLL_HPD;
+	connector->polled = DRM_CONNECTOR_POLL_DISCONNECT | DRM_CONNECTOR_POLL_DISCONNECT_ONLY;
 	connector->interlace_allowed = 1;
 	connector->doublescan_allowed = 0;
 	intel_encoder->crtc_mask = (1 << 0) | (1 << 1) | (1 << 2);
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 316ce64..a60abb5 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -503,6 +503,8 @@  enum drm_connector_force {
 /* can cleanly poll for disconnections without flickering the screen */
 /* DACs should rarely do this without a lot of testing */
 #define DRM_CONNECTOR_POLL_DISCONNECT (1 << 2)
+/* Only poll for disconnections. */
+#define DRM_CONNECTOR_POLL_DISCONNECT_ONLY (1 << 3)
 
 #define MAX_ELD_BYTES	128