[6/6] drm/i915/bxt: Fix irq_port for eDP
diff mbox

Message ID 4D68720C2E767A4AA6A8796D42C8EB590925249B@BGSMSX101.gar.corp.intel.com
State New
Headers show

Commit Message

durgadoss.r@intel.com Sept. 10, 2015, 1:07 p.m. UTC
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.

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

--
1.7.10.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Comments

Ville Syrjälä Sept. 10, 2015, 1:35 p.m. UTC | #1
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
Daniel Vetter Sept. 10, 2015, 2:51 p.m. UTC | #2
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

Patch
diff mbox

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