Message ID | 4D68720C2E767A4AA6A8796D42C8EB590925249B@BGSMSX101.gar.corp.intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Thu, Sep 10, 2015 at 01:07:30PM +0000, R, Durgadoss wrote: > Hi Rodrigo, > > I had to add this to get HDMI hotplug working on BXT. > As you might already know, we have the HPD pins A & B > swapped in BXT. And, we are using HPD_PORT_A as > 'intel_encoder->hpd_pin' for HDMI on port B. People keep saying that, but according to the spec it's not even true. A and B are not swapped, but instead all the pins are sort of shifted by one position. HPD A = DDI0 / port B HPD B = DDI1 / port C HPD C = DDI2 / port A If the code actually did the remapping in a way that matches the spec, things would be a lot less confusing. > > Without this, When an HPD on HDMI (port B) happens, the interrupt > is handled as an eDP (port A) interrupt in 'intel_hpd_irq_handler', > since hpd_pin for HDMI port B is set as PIN A. > > Snippet: > --- > is_dig_port = intel_hpd_pin_to_port(i, &port) && > dev_priv->hotplug.irq_port[port]; > > --- > > The issue occurs only when we have eDP + HDMI combination. > MIPI + HDMI works well without this fix also. > > And all these workarounds, are due to pin swap in > BXT A0/A1. We have this fix enclosed with that check as well. > > I tried to few other changes, but this one was less intrusive > and easy to change (and notice the change). > Kindly let us know if you have better ideas. > > Thanks, > Durga > > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Rodrigo Vivi > Sent: Thursday, September 10, 2015 12:54 AM > To: Jindal, Sonika; intel-gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH 6/6] drm/i915/bxt: Fix irq_port for eDP > > Nak: I don't believe we need this... Actually I believe we need the opposite... we need to enable HPD on port A for eDP errors handling... > > > > > On Fri, Sep 4, 2015 at 6:38 AM Sonika Jindal <sonika.jindal@intel.com> wrote: > From: Durgadoss R <durgadoss.r@intel.com> > > Currently, HDMI hotplug with eDP as local panel is failing > because the HDMI hpd is detected as a long hpd for eDP; and is > thus rightfully ignored. But, it should really be handled as > an interrupt on port B for HDMI (due to BXT A1 platform having > HPD pins A and B swapped). This patch sets the irq_port[PORT_A] > to NULL in case eDP is on port A so that irq handler does not > treat it as a 'dig_port' interrupt. > > Signed-off-by: Durgadoss R <durgadoss.r@intel.com> > --- > drivers/gpu/drm/i915/intel_ddi.c | 15 ++++++++++----- > 1 file changed, 10 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > index 4823184..fec51df 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -3218,15 +3218,20 @@ void intel_ddi_init(struct drm_device *dev, enum port port) > goto err; > > intel_dig_port->hpd_pulse = intel_dp_hpd_pulse; > + dev_priv->hotplug.irq_port[port] = intel_dig_port; > /* > * On BXT A0/A1, sw needs to activate DDIA HPD logic and > * interrupts to check the external panel connection. > + * If eDP is connected on port A, set irq_port to NULL > + * so that we do not assume an interrupt here as a > + * 'dig_port' interrupt. > */ > - if (IS_BROXTON(dev_priv) && (INTEL_REVID(dev) < BXT_REVID_B0) > - && port == PORT_B) > - dev_priv->hotplug.irq_port[PORT_A] = intel_dig_port; > - else > - dev_priv->hotplug.irq_port[port] = intel_dig_port; > + if (IS_BROXTON(dev) && (INTEL_REVID(dev) < BXT_REVID_B0)) { > + if (port == PORT_B) > + dev_priv->hotplug.irq_port[PORT_A] = intel_dig_port; > + else if (intel_encoder->type == INTEL_OUTPUT_EDP) > + dev_priv->hotplug.irq_port[port] = NULL; > + } > } > > /* In theory we don't need the encoder->type check, but leave it just in > -- > 1.7.10.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Sep 10, 2015 at 04:35:57PM +0300, Ville Syrjälä wrote: > On Thu, Sep 10, 2015 at 01:07:30PM +0000, R, Durgadoss wrote: > > Hi Rodrigo, > > > > I had to add this to get HDMI hotplug working on BXT. > > As you might already know, we have the HPD pins A & B > > swapped in BXT. And, we are using HPD_PORT_A as > > 'intel_encoder->hpd_pin' for HDMI on port B. > > People keep saying that, but according to the spec it's not > even true. A and B are not swapped, but instead all the pins > are sort of shifted by one position. > > HPD A = DDI0 / port B > HPD B = DDI1 / port C > HPD C = DDI2 / port A > > If the code actually did the remapping in a way that matches > the spec, things would be a lot less confusing. And we even have the logic to allow that remapping, so I guess the real job is to figure out where that's not done correctly. Not throwing more hacks on top. -Daniel
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index 4823184..fec51df 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -3218,15 +3218,20 @@ void intel_ddi_init(struct drm_device *dev, enum port port) goto err; intel_dig_port->hpd_pulse = intel_dp_hpd_pulse; + dev_priv->hotplug.irq_port[port] = intel_dig_port; /* * On BXT A0/A1, sw needs to activate DDIA HPD logic and * interrupts to check the external panel connection. + * If eDP is connected on port A, set irq_port to NULL + * so that we do not assume an interrupt here as a + * 'dig_port' interrupt. */ - if (IS_BROXTON(dev_priv) && (INTEL_REVID(dev) < BXT_REVID_B0) - && port == PORT_B) - dev_priv->hotplug.irq_port[PORT_A] = intel_dig_port; - else - dev_priv->hotplug.irq_port[port] = intel_dig_port; + if (IS_BROXTON(dev) && (INTEL_REVID(dev) < BXT_REVID_B0)) { + if (port == PORT_B) + dev_priv->hotplug.irq_port[PORT_A] = intel_dig_port; + else if (intel_encoder->type == INTEL_OUTPUT_EDP) + dev_priv->hotplug.irq_port[port] = NULL; + } } /* In theory we don't need the encoder->type check, but leave it just in