Re: [PATCH 2/2] drm/i915: Ignore long hpds on eDP ports

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Oct 17, 2014 at 09:08:28AM -0700, Todd Previte wrote:
> 
> 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@linux.intel.com wrote:
> >>>From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>
> >>>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@linux.intel.com>
> >>>---
> >>>   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@gmail.com>

Aside: We don't handle hpd for port A anyway, so for most panels this
doesn't matter all that much. Or we'd have piles more bug reports I think.

Anyway, Cc: stable@vger.kernel.org and one for Jani.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch