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

[Jani Nikula <jani.nikula@linux.intel.com>]

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.

BR,
Jani.



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

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx