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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Oct 21, 2014 at 06:00:05PM +0200, Daniel Vetter wrote:
> 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.

Yeah, but we should. Then we could actually enable PSR main-link off
mode and the fallback to manual retrain would work if/when the automagic
training fails.

> 
> 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

-- 
Ville Syrjälä
Intel OTC