From mboxrd@z Thu Jan 1 00:00:00 1970 From: Todd Previte Subject: Re: [PATCH 2/2] drm/i915: Ignore long hpds on eDP ports Date: Fri, 17 Oct 2014 09:08:28 -0700 Message-ID: <54413EFC.6000204@gmail.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> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252"; Format="flowed" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mail-pa0-f45.google.com (mail-pa0-f45.google.com [209.85.220.45]) by gabe.freedesktop.org (Postfix) with ESMTP id 2BD7E6E34F for ; Fri, 17 Oct 2014 09:08:25 -0700 (PDT) Received: by mail-pa0-f45.google.com with SMTP id rd3so1078233pab.32 for ; Fri, 17 Oct 2014 09:08:25 -0700 (PDT) In-Reply-To: <20141017084301.GN4284@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: =?windows-1252?Q?Ville_Syrj=E4l=E4?= Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org 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 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=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/int= el_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 *in= tel_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 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