Re: [PATCH 7/6] drm/i915/skl: DDI-E and DDI-A shares 4 lanes.

[Timo Aaltonen <tjaalton@ubuntu.com>]

On 27.08.2015 05:52, Zhang, Xiong Y wrote:
>> On Wed, 2015-08-26 at 11:15 +0300, Jani Nikula wrote:
>>> On Thu, 13 Aug 2015, "Jindal, Sonika" <sonika.jindal@intel.com>
>>> wrote:
>>>> On 8/13/2015 8:57 AM, Zhang, Xiong Y wrote:
>>>>>> On Wed, 2015-08-12 at 02:20 +0000, Zhang, Xiong Y wrote:
>>>>>>>> On Tue, 2015-08-11 at 07:05 +0000, Zhang, Xiong Y wrote:
>>>>>>>>>> -----Original Message-----
>>>>>>>>>> From: Vivi, Rodrigo
>>>>>>>>>> Sent: Saturday, August 8, 2015 8:34 AM
>>>>>>>>>> To: intel-gfx@lists.freedesktop.org
>>>>>>>>>> Cc: Vivi, Rodrigo; Zhang, Xiong Y
>>>>>>>>>> Subject: [PATCH 7/6] drm/i915/skl: DDI-E and DDI-A
>>>>>>>>>> shares 4
>>>>>>>>>> lanes.
>>>>>>>>>>
>>>>>>>>>> DDI-A and DDI-E shares the 4 lanes. So when DDI-E is
>>>>>>>>>> present we
>>>>>>>>>> need to configure lane count propperly for both.
>>>>>>>>>>
>>>>>>>>>> This was based on Sonika's
>>>>>>>>>> [PATCH] drm/i915/skl: Select DDIA lane capability based
>>>>>>>>>> upon vbt
>>>>>>>>>>
>>>>>>>>>> Credits-to: Sonika Jindal <sonika.jindal@intel.com>
>>>>>>>>>> Cc: Xiong Zhang <xiong.y.zhang@intel.com>
>>>>>>>>>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>>>>>>>>> ---
>>>>>>>>>>   drivers/gpu/drm/i915/intel_ddi.c | 12 ++++++++++--
>>>>>>>>>> drivers/gpu/drm/i915/intel_dp.c  |  8 +++++---
>>>>>>>>>>   2 files changed, 15 insertions(+), 5 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c
>>>>>>>>>> b/drivers/gpu/drm/i915/intel_ddi.c
>>>>>>>>>> index 110d546..557cecf 100644
>>>>>>>>>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>>>>>>>>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>>>>>>>>>> @@ -3178,7 +3178,15 @@ void intel_ddi_init(struct
>>>>>>>>>> drm_device
>>>>>>>>>> *dev, enum port port)
>>>>>>>>>>   	struct intel_digital_port *intel_dig_port;
>>>>>>>>>>   	struct intel_encoder *intel_encoder;
>>>>>>>>>>   	struct drm_encoder *encoder;
>>>>>>>>>> -	bool init_hdmi, init_dp;
>>>>>>>>>> +	bool init_hdmi, init_dp, ddi_e_present;
>>>>>>>>>> +
>>>>>>>>>> +	/*
>>>>>>>>>> +	 * On SKL we don't have a way to detect DDI-E
>>>>>>>>>> so we
>>>>>>>>>> rely
>>>>>>>>>> on VBT.
>>>>>>>>>> +	 */
>>>>>>>>>> +	ddie_present = IS_SKYLAKE(dev) &&
>>>>>>>>>> +		(dev_priv
>>>>>>>>>> ->vbt.ddi_port_info[PORT_E].supports_dp
>>>>>>>>>>>>
>>>>>>>>>> +		 dev_priv
>>>>>>>>>> ->vbt.ddi_port_info[PORT_E].supports_dvi
>>>>>>>>>>>>
>>>>>>>>>> +		 dev_priv
>>>>>>>>>> ->vbt.ddi_port_info[PORT_E].supports_hdmi);
>>>>>>>>> [Zhang, Xiong Y]  ddie_present should be ddi_e_present
>>>>>>>>>>
>>>>>>>>>>   	init_hdmi = (dev_priv
>>>>>>>>>> ->vbt.ddi_port_info[port].supports_dvi ||
>>>>>>>>>>   		     dev_priv
>>>>>>>>>> ->vbt.ddi_port_info[port].supports_hdmi);
>>>>>>>>>> @@ -3210,7 +3218,7 @@ void intel_ddi_init(struct
>>>>>>>>>> drm_device
>>>>>>>>>> *dev, enum port port)
>>>>>>>>>>   	intel_dig_port->port = port;
>>>>>>>>>>   	intel_dig_port->saved_port_bits =
>>>>>>>>>> I915_READ(DDI_BUF_CTL(port)) &
>>>>>>>>>>
>>>>>>>>>>   (DDI_BUF_PORT_REVERSAL |
>>>>>>>>>> -
>>>>>>>>>>  DDI_A_4_LANES);
>>>>>>>>>> +
>>>>>>>>>>  ddi_e_present ? 0 :
>>>>>>>>>> DDI_A_4_LANES);
>>>>>>>>> [Zhang, Xiong Y] Sonika's patch will set DDI-A to 4 lanes
>>>>>>>>> when
>>>>>>>>> DDI-E doesn't exist, I think your patch will do nothing.
>>>>>>>>
>>>>>>>> Actually DDI_A_4_LANES is being already set
>>>>>>>> unconditionally, so
>>>>>>>> Sonika's patch has no effect.
>>>>>>> [Zhang, Xiong Y] No. Sonika's patch set DDI_A_4_LANES under
>>>>>>> many
>>>>>>> conditions.
>>>>>>> +	if (IS_SKYLAKE(dev) && port == PORT_A
>>>>>>> +		&& !(val & DDI_BUF_CTL_ENABLE)
>>>>>>> +		&& !dev_priv->vbt.ddi_e_used)
>>>>>>> +		I915_WRITE(DDI_BUF_CTL(port), val |
>>>>>>> DDI_A_4_LANES)
>>>>>>>>
>>>>>>>> saved_port_bits goes to intel_dp->DP that goes to
>>>>>>>> DDI_BUF_CTL and
>>>>>>>> also it is used to calculate the number of lanes.
>>>>>>>>
>>>>>>>> With this patch we stop setting DDI_A_4_LANES when ddi_e is
>>>>>>>> present
>>>>>>>> so DDI-A keeps with 2 lanes and let other 2 lanes for DDI-E
>>>>>>> [Zhang, Xiong Y] Yes, this patch will clear DDI_A_4_LANES
>>>>>>> when ddi_e
>>>>>>> is present.
>>>>>>> But this patch won't set DDI_A_4_LANES under following
>>>>>>> conditions
>>>>>>> which is purpose for Sonika patch 1. Bios fail to driver eDP
>>>>>>> and
>>>>>>> doesn't enable DDI_A buffer
>>>>>>
>>>>>> If DDI_A isn't enabled we don't need to set DDI_A_4_LANES
>>>>> [Zhang, Xiong Y] From commit message on Sonika patch, she want to
>>>>> set DDI_A_4_LANES on such case. Maybe she met such fail case on
>>>>> one high
>>>>> resolution eDP screen. Let's Sonikia explain it.
>>>>>>
>>>>>>> 2. Bios clear DDI_A_4_LANES
>>>>>>> 3. DDI_E isn't present
>>>>>>
>>>>>> I don't agree... This is already covered on current code.
>>>>>> DDI_A_4_LANES is
>>>>>> already being set when enabling DDI_A.
>>>>>>
>>>> As Zhang mentioned and as my commit message explains, my patch is
>>>> needed
>>>> when bios failed to drive edp (In my case it was an intermediate
>>>> frequency supported panel which was set to 3.24 GHz and bios didn't
>>>> have
>>>> support for intermediate frequencies), it will not enable DDIA in
>>>> which
>>>> case, it will not set DDI_BUF_CTL and DDI Lane capability will
>>>> remain 0
>>>> (which is DDIA with 2 lanes and DDIE with 2 lanes).
>>>> So, since the native resolution of that panel was high and couldn't
>>>> work
>>>> with 2 lanes.
>>>> So, ideally we should not rely on bios to set the initial value and
>>>> set
>>>> it based upon whether DDI_E is used or not.
>>>> So, my patch has some effect :)
>>>
>>> Rodrigo? Please figure out what the needed patch is, and send it.
>>
>> I've just read Sonika's patch again to see if I could convince myself
>> to stop being stubborn...
>>
>> I realized that our patches are independent. I still believe we need
>> this one here... We just need a reviewer.
>>
>> But I'm really a stubborn and I'm not convinced we need the other
>> patch. I still can't see how we would end up enabling DDI-A without
>> setting the lanes. For me if we don't call intel_ddi_init(port_A) we
>> don't need to set lanes or there is something else really wrong, and if
>> we call it this bit will be *always* set already.
> [Zhang, Xiong Y] From Sonika experience, "always" isn't true because bios fail in initialize eDP.
> In a short, if DDI-E is present, we should clear DDI_A_4_LANES, your patch will do this.
> If DDI-E isn't present, we should set DDI_A_4_LANES, Sonika's patch will do it, but your patch miss it.
> If you think your and Sonika's patch are independent, you could resend your patch with modified commit message.

If it helps with the debate, this patch caused a regression on an I+N
hybrid machine where the display remained black after booting up..


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