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

Jindal, Sonika
2015-07-17T04:29:23Z

On 7/15/2015 2:37 PM, Daniel Vetter wrote:
> 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

So, for now, I will just correct it in intel_ddi_init and leave the
handling of HPD for port A untouched. I will only change the irq_port
for external DP (port B). And the other part with hpd_pin to handle hdmi
hotplug will remain as is.
Please let me know if you see any issue in this.

Regards,
Sonika