Re: [PATCH 2/2] drm/i915/bxt: WA for swapped HPD pins in A stepping

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Jul 15, 2015 at 01:34:29PM +0530, Jindal, Sonika wrote:
> 
> 
> On 7/15/2015 12:05 PM, Jindal, Sonika wrote:
> >
> >
> >On 7/14/2015 7:52 PM, Imre Deak wrote:
> >>On ti, 2015-07-14 at 11:18 +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.
> >>>
> >>>Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
> >>>---
> >>>   drivers/gpu/drm/i915/intel_dp.c   |   18 ++++++++++++++++--
> >>>   drivers/gpu/drm/i915/intel_hdmi.c |    9 ++++++++-
> >>>   2 files changed, 24 insertions(+), 3 deletions(-)
> >>>
> >>>diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> >>>index 7b54b9d..c32f3d3 100644
> >>>--- a/drivers/gpu/drm/i915/intel_dp.c
> >>>+++ b/drivers/gpu/drm/i915/intel_dp.c
> >>>@@ -5869,10 +5869,24 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
> >>>   	/* Set up the hotplug pin. */
> >>>   	switch (port) {
> >>>   	case PORT_A:
> >>>-		intel_encoder->hpd_pin = HPD_PORT_A;
> >>>+		/*
> >>>+		 * On BXT A0/A1, sw needs to activate DDIC HPD logic and
> >>>+		 * interrupts to check the eDP panel connection.
> >>>+		 */
> >>>+		if (IS_BROXTON(dev_priv) && (INTEL_REVID(dev) < BXT_REVID_B0))
> >>>+			intel_encoder->hpd_pin = HPD_PORT_C;
> >>>+		else
> >>>+			intel_encoder->hpd_pin = HPD_PORT_A;
> >>>   		break;
> >>>   	case PORT_B:
> >>>-		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:
> >>>   		intel_encoder->hpd_pin = HPD_PORT_C;
> >>
> >>This won't work for a couple of reasons: atm i915_digport_work_func()
> >>assumes a fixed pin->port mapping, for example it'll call the HPD
> >>handler for the port A encoder for a short/long pulse event triggered
> >>via the HPD_PORT_A pin, whereas after the above patch you want to select
> >>port B's encoder on BXT A0/1. This could be fixed by setting up
> >>hotplug.irq_port in intel_ddi_init() according to the above change, or
> >>not using irq_port at all, but instead looking up the encoder the same
> >>way i915_hotplug_work_func() does using intel_encoder->hpd_pin.

Yeah that's kinda a bug in digport_work_func, but that part is also a
mess. Simplest would be to rename hotplug.irq_port to irq_pin and change
the assignment for that too.

> >Hmm, i didnt realize that.
> >Moving this check to intel_ddi_init, will again make the checks spread
> >all over which we wanted to avoid since hpd_pin was in place.
> >But looks like hpd_pin is only for i915_hotplug_work_func.
> >I feel we can move back to the older approach itself
> >What do you suggest?
> >
> But then we can remove these checks from here, and have it only in
> intel_ddi_init, right? So it should look fine.
> 
> For HPD_PORT_A, I think we can skip that part as of now.
> 
> Please suggest..

I still think that fake-handling hpd A in the low-level irq handler is
massively confusing. And we need to change all the same parts anyway (i.e.
you'd have to change the digport_work_func too with the old approach).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx