[Intel-gfx] [PATCH] drm/i915/bxt: WA for swapped HPD pins in A stepping

[Imre Deak]

On Mon, 2015-07-20 at 11:36 +0530, Jindal, Sonika wrote:
> 
> On 7/18/2015 5:17 AM, Imre Deak wrote:
> > On Fri, 2015-07-17 at 13:47 +0530, Sonika Jindal wrote:
> >> As per bspec, on BXT A0/A1, sw needs to activate DDIA HPD logic
> >> and interrupts to check the external panel connection and DDIC HPD
> >> logic for edp panel.
> >>
> >> v2: For DP, irq_port is used to determine the encoder instead of
> >> hpd_pin and removing the edp HPD logic because port A HPD is not
> >> present(Imre)
> >>
> >> Signed-off-by: Sonika Jindal <sonika.jindal at intel.com>
> >> ---
> >>   drivers/gpu/drm/i915/intel_ddi.c  |   10 +++++++++-
> >>   drivers/gpu/drm/i915/intel_hdmi.c |    9 ++++++++-
> >>   2 files changed, 17 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> >> index e2c6f73..777e3a3 100644
> >> --- a/drivers/gpu/drm/i915/intel_ddi.c
> >> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> >> @@ -3225,7 +3225,15 @@ 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 (IS_BROXTON(dev_priv) && (INTEL_REVID(dev) < BXT_REVID_B0)
> >> +					 && port == PORT_B)
> >> +			dev_priv->hotplug.irq_port[PORT_A] = intel_dig_port;
> >
> > This happens to work but is confusing. irq_port[PORT_A] will be set here
> > already and the above will simply overwrite it without explanation. I
> > would also handle the port == PORT_A case and not set irq_port for it.
> >
> > The same swapping for hpd_pin is missing from intel_dp_init_connector().
> >
> >> +		else
> >> +			dev_priv->hotplug.irq_port[port] = intel_dig_port;
> >>   	}
> >>
> >>   	/* In theory we don't need the encoder->type check, but leave it just in
> >> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> >> index 70bad5b..94fa716 100644
> >> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> >> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> >> @@ -1973,7 +1973,14 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
> >>   			intel_hdmi->ddc_bus = GMBUS_PIN_1_BXT;
> >>   		else
> >>   			intel_hdmi->ddc_bus = GMBUS_PIN_DPB;
> >> -		intel_encoder->hpd_pin = HPD_PORT_B;
> >> +		/*
> >> +		 * On BXT A0/A1, sw needs to activate DDIA HPD logic and
> >> +		 * interrupts to check the external panel connection.
> >> +		 */
> >> +		if (IS_BROXTON(dev_priv) && (INTEL_REVID(dev) < BXT_REVID_B0))
> >> +			intel_encoder->hpd_pin = HPD_PORT_A;
> >> +		else
> >> +			intel_encoder->hpd_pin = HPD_PORT_B;
> >>   		break;
> >>   	case PORT_C:
> >>   		if (IS_BROXTON(dev_priv))
> >
> > As I earlier pointed out with the above approach, you need to add
> > support for HPD events on the HPD_PORT_A pin. If you look at the
> > for_each_hpd_pin() macro and intel_hpd_irq_handler()/is_dig_port you'll
> > notice that any interrupt event on the HPD_PORT_A pin will be ignored
> > now.
> Hmm :(. For now, we can fix for_each_hpd_pin and add a check in 
> intel_hpd_pin_to_port to return PORT_B for pin 0 if A0/A1. Then we can 
> skip the check in intel_ddi_init. But this again will look very confusing.
> 
> So two options:
> 1. We move back to the older approach where we just use another hpd 
> ports array in i915_irq.c
> 2. Go with current approach.
> 
> I feel option 1 is more clean and less prone to further issues.
> What do you suggest?

I also think that adding support for HPD on the port A pin is the better
way and this would be needed in any case for eDP short pulse detection
support in the future regardless of this workaround. I'll send a
patchset that does this, I think on top of that we could apply the
current version of your 2 workaround patches (with the comments
addressed).

--Imre