On 10/17/2014 1:43 AM, Ville Syrjälä wrote:
On Thu, Oct 16, 2014 at 12:38:55PM -0700, Todd Previte wrote:
On 10/16/2014 10:46 AM, ville.syrjala@xxxxxxxxxxxxxxx wrote:
From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
Turning vdd on/off can generate a long hpd pulse on eDP ports. In order
to handle hpd we would need to turn on vdd to perform aux transfers.
This would lead to an endless cycle of
"vdd off -> long hpd -> vdd on -> detect -> vdd off -> ..."
So ignore long hpd pulses on eDP ports. eDP panels should be physically
tied to the machine anyway so they should not actually disappear and
thus don't need long hpd handling. Short hpds are still needed for link
re-train and whatnot so we can't just turn off the hpd interrupt
entirely for eDP ports. Perhaps we could turn it off whenever the panel
is disabled, but just ignoring the long hpd seems sufficient.
Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
---
drivers/gpu/drm/i915/intel_dp.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index f07f02c..4455009 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4611,6 +4611,18 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
if (intel_dig_port->base.type != INTEL_OUTPUT_EDP)
intel_dig_port->base.type = INTEL_OUTPUT_DISPLAYPORT;
+ if (long_hpd && intel_dig_port->base.type == INTEL_OUTPUT_EDP) {
+ /*
+ * vdd off can generate a long pulse on eDP which
+ * would require vdd on to handle it, and thus we
+ * would end up in an endless cycle of
+ * "vdd off -> long hpd -> vdd on -> detect -> vdd off -> ..."
+ */
+ DRM_DEBUG_KMS("ignoring long hpd on eDP port %c\n",
+ port_name(intel_dig_port->port));
+ return false;
+ }
+
DRM_DEBUG_KMS("got hpd irq on port %c - %s\n",
port_name(intel_dig_port->port),
long_hpd ? "long" : "short");
I'm not sure that ignoring a long pulse is the best way to handle it.
eDP does not appear to differentiate between short and long pulses per
the specification (not to mention that HPD support for eDP is optional
in the first place). It seems to me that it would probably be better to
handle them as a normal (short) HPD pulse and just do the regular link
maintenance stuff. As I said, the spec doesn't differentiate between the
long and short pulses for eDP so it's a safer bet to handle them as a
short pulse than to ignore them entirely.
The spec does talk a lot about IRQ_HPD (which is the short pulse) and
the power sequencing diagrams explicitly show that HPD should be asserted
after the T3 delay and deasserted immediately on VDD off, so those would
be the long pulses. So based on that my patch seems to make sense.
It seems HPD is optional for the source only, in the sink it's madatory.
But that doesn't really matter anyway because if either end doesn't have
it it won't work. The spec does go on to say that we should periodically
poll the sink status if HPD_IRQ isn't available. We don't do that
currently, but it does sound like a sane idea in case the link drops out.
Yeah I saw some of the info on IRQ_HPD but didn't see the long pulse
stuff. In any case, my concern was with missing a valid HPD event. In
light of the above, that doesn't look like it's an issue, so I'm good
with this patch.
It looks like HPD support in an eDP sink device is optional as well, at
least according to the table on pg.34 of the eDP 1.4 spec. As you
pointed out though, unless both source and sink devices support HPD, it
doesn't really matter. I saw the bit about polling in there, too. It
might be worth implementing a polling mechanism as a backup, but I don't
know how useful it would be. I don't recall running across a sink device
that doesn't support HPD, but that's not to say they don't exist.
Reviewed-by: Todd Previte <tprevite@xxxxxxxxx>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
