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

From: "Jindal, Sonika" <sonika.jindal@intel.com>
To: "Zhang, Xiong Y" <xiong.y.zhang@intel.com>,
	"Vivi, Rodrigo" <rodrigo.vivi@intel.com>
Cc: "intel-gfx@lists.freedesktop.org" <intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 7/6] drm/i915/skl: DDI-E and DDI-A shares 4 lanes.
Date: Thu, 13 Aug 2015 11:18:09 +0530	[thread overview]
Message-ID: <55CC2F99.2010802@intel.com> (raw)
In-Reply-To: <8082FF9BCB2B054996454E47167FF4EC029C0735@SHSMSX104.ccr.corp.intel.com>


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 :)
>>
>>>
>>> thanks
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx