From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH 2/2] drm/i915: Ignore long hpds on eDP ports Date: Wed, 22 Oct 2014 10:39:43 +0300 Message-ID: <20141022073943.GS4284@intel.com> References: <1413481570-18288-1-git-send-email-ville.syrjala@linux.intel.com> <1413481570-18288-2-git-send-email-ville.syrjala@linux.intel.com> <54401ECF.6010107@gmail.com> <20141017084301.GN4284@intel.com> <54413EFC.6000204@gmail.com> <20141021160005.GY26941@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by gabe.freedesktop.org (Postfix) with ESMTP id 3887F6E277 for ; Wed, 22 Oct 2014 00:40:29 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20141021160005.GY26941@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Daniel Vetter Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org 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=E4l=E4 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=E4l=E4 > > >>> > > >>>Turning vdd on/off can generate a long hpd pulse on eDP ports. In or= der > > >>>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 physica= lly > > >>>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 l= ink > > >>>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 pa= nel > > >>>is disabled, but just ignoring the long hpd seems sufficient. > > >>> > > >>>Signed-off-by: Ville Syrj=E4l=E4 > > >>>--- > > >>> 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 !=3D INTEL_OUTPUT_EDP) > > >>> intel_dig_port->base.type =3D INTEL_OUTPUT_DISPLAYPORT; > > >>>+ if (long_hpd && intel_dig_port->base.type =3D=3D 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 asser= ted > > >after the T3 delay and deasserted immediately on VDD off, so those wou= ld > > >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 madator= y. > > >But that doesn't really matter anyway because if either end doesn't ha= ve > > >it it won't work. The spec does go on to say that we should periodical= ly > > >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 o= ut. > > > > > = > > Yeah I saw some of the info on IRQ_HPD but didn't see the long pulse st= uff. > > 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 patc= h. > > = > > 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 point= ed > > 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 w= orth > > implementing a polling mechanism as a backup, but I don't know how usef= ul it > > would be. I don't recall running across a sink device that doesn't supp= ort > > HPD, but that's not to say they don't exist. > > = > > Reviewed-by: Todd Previte > = > 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=E4l=E4 Intel OTC