Re: [PATCH] drm/i915: Fix the interpretation of MAX_PRE-EMPHASIS_REACHED bit inorder to pass Link Layer compliance test number 400.3.1.15

[Khaled Almahallawy <khaled.almahallawy@intel.com>]

On 5/30/19 2:20 PM, Manasi Navare wrote:
> On Thu, May 30, 2019 at 12:33:40PM -0700, Almahallawy, Khaled wrote:
>> On Wed, 2019-05-22 at 12:25 -0700, Manasi Navare wrote:
>>> On Tue, May 21, 2019 at 04:24:58PM +0300, Ville Syrjälä wrote:
>>>> On Mon, May 20, 2019 at 04:25:41PM -0700, Khaled Almahallawy wrote:
>>>>> According to DP 1.4 standard, if the source supports four pre-
>>>>> emphasis levels, then the source shall set the bit MAX_PRE-
>>>>> EMPHASIS_REACHED = 1 only when trasmitter programmed PRE-
>>>>> EMPHASIS_SET field (bits 4:3) to 11b (Level 3). Pre-emphasis
>>>>> level 3 is the maximum pre-emphasis level that the source
>>>>> supports.
>>>>> Currently the MAX_PRE-EMPHASIS_REACHED bit is interpreted as the
>>>>> Max Pre-Emphasis level for certain Swing Level. This
>>>>> interpretation fails Link Layer compliance test 400.3.1.15 step
>>>>> 17 according to the following Fail condition:
>>>>> TRAINING_LANEx_SET.MAX_PRE-EMPHASIS_REACHED = 1 (check all active
>>>>> lanes) and the Source DUT supports pre-emphasis level 3 (9.5db).
>>>> Hmm. I guess that's correct. The spec doesn't say anything about
>>>> per-vswing pre-emphasis when talking about the 'max reached' bit.
>>>>
>>>>> Cc: Clint Taylor <Clinton.A.Taylor@intel.com>
>>>>> Cc: Manasi Navare <manasi.d.navare@intel.com>
>>>>> Signed-off-by: Khaled Almahallawy <khaled.almahallawy@intel.com>
>>>>> ---
>>>>>   drivers/gpu/drm/i915/intel_ddi.c | 20 --------------------
>>>>>   drivers/gpu/drm/i915/intel_dp.c  |  2 +-
>>>>>   2 files changed, 1 insertion(+), 21 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c
>>>>> b/drivers/gpu/drm/i915/intel_ddi.c
>>>>> index 0af47f343faa..6540c979c098 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>>>>> @@ -2239,26 +2239,6 @@ u8 intel_ddi_dp_voltage_max(struct
>>>>> intel_encoder *encoder)
>>>>>   		DP_TRAIN_VOLTAGE_SWING_MASK;
>>>>>   }
>>>>>   
>>>>> -/*
>>>>> - * We assume that the full set of pre-emphasis values can be
>>>>> - * used on all DDI platforms. Should that change we need to
>>>>> - * rethink this code.
>>>>> - */
>>>>> -u8 intel_ddi_dp_pre_emphasis_max(struct intel_encoder *encoder,
>>>>> u8 voltage_swing)
>>>>> -{
>>>>> -	switch (voltage_swing & DP_TRAIN_VOLTAGE_SWING_MASK) {
>>>>> -	case DP_TRAIN_VOLTAGE_SWING_LEVEL_0:
>>>>> -		return DP_TRAIN_PRE_EMPH_LEVEL_3;
>>>>> -	case DP_TRAIN_VOLTAGE_SWING_LEVEL_1:
>>>>> -		return DP_TRAIN_PRE_EMPH_LEVEL_2;
>>>>> -	case DP_TRAIN_VOLTAGE_SWING_LEVEL_2:
>>>>> -		return DP_TRAIN_PRE_EMPH_LEVEL_1;
>>>>> -	case DP_TRAIN_VOLTAGE_SWING_LEVEL_3:
>>>>> -	default:
>>>>> -		return DP_TRAIN_PRE_EMPH_LEVEL_0;
>>>>> -	}
>>>>> -}
>>>>> -
>>>>>   static void cnl_ddi_vswing_program(struct intel_encoder
>>>>> *encoder,
>>>>>   				   int level, enum intel_output_type
>>>>> type)
>>>>>   {
>>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c
>>>>> b/drivers/gpu/drm/i915/intel_dp.c
>>>>> index 77ba4da6b981..f94759e45862 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>>>> @@ -3541,7 +3541,7 @@ intel_dp_pre_emphasis_max(struct intel_dp
>>>>> *intel_dp, u8 voltage_swing)
>>>>>   	enum port port = encoder->port;
>>>>>   
>>>>>   	if (HAS_DDI(dev_priv)) {
>>>>> -		return intel_ddi_dp_pre_emphasis_max(encoder,
>>>>> voltage_swing);
>>>>> +		return DP_TRAIN_PRE_EMPH_LEVEL_3;
>>>> We're going to have to change this for all platforms.
>> Yes, I'm going to change for all platforms in intel_dp_pre_emphasis_max
>> function. I will also add the missing condition:
>> 	else if (HAS_PCH_CPT(dev_priv) && port != PORT_A)
>> similar to intel_dp_voltage_max function
>>
>>>> Also we need to update the code to pick the correct swing/pre-
>>>> emphasis
>>>> when we can't do what is being requested.
>>> This check will need to be added in adjust_train() function
>> sure, I will implement this logic in intel_get_adjust_train
>>>> The spec says:
>>>> "When the combination of the requested pre-emphasis level and
>>>> voltage
>>>>   swing exceeds the capability of a DPTX, the DPTX shall set the
>>>> pre-emphasis
>>>>   level according to the request and use the highest voltage swing
>>>> it can
>>>>   output with the given pre-emphasis level."
>>>> and
>>>> "When a DPTX reads a request beyond the limits of this Standard,
>>>> the
>>>>   DPTX shall set the pre-emphasis level according to the request and
>>>> set
>>>>   the highest voltage swing level it can output with the given pre-
>>>> emphasis
>>>>   level. If a DPTX is requested for 9.5dB of pre-emphasis level (may
>>>> be
>>>>   supported for a DPTX) and cannot support that level, it shall set
>>>> the
>>>>   pre-emphasis level to the next highest level, 6dB."
>>> So my interpretation of this is :
>>>
>>> In adjust_train() function:
>>>
>>> vswing_max = intel_dp_voltage_max() which is set per platform
>>> pre_emphasis_max = set to level 3
>>   
>> 	pre_emphasis_max = intel_dp_pre_emphasis_max
>> because it is set per platfrom as well, similar to vswing_max
>>
>>>   
>>> v = get_requested_voltage_swing() - Limit this to vmax
>>> p = get_requested_pre_emphasis() - Limit this to pmax
>>>
>>> Now rewrite the intel_ddi_dp_pre_emphasis_max() function to call it
>> intel_ddi_dp_pre_emphasis_max is needed to determine the max pre-
>> emphasis level per platform.
> What I meant was that define another function that will return a pre_emphasis_max
> per platform but independent of the vswing values.
> Makes sense?
>
> Manasi


Yes that make sense.

I thought of modifying intel_dp.c/intel_dp_pre_emphasis_max(struct 
intel_dp *intel_dp, u8 voltage_swing)

to intel_dp_pre_emphasis_max(struct intel_dp *intel_dp) to return the 
max pre-emphasis per platform

independent of vswing values. This function should be similar to 
intel_dp.c/intel_dp_voltage_max(struct intel_dp *intel_dp)

And create intel_dp.c/intel_dp_possible_vswing_max(intel_dp *intel_dp, 
u8 emphasis) to return the correct vswing/pre-emphasis combination, 
given the requested

pre-emphasis value

What do you think?

Thanks

Khaled


>>> intel_ddi_dp_possible_vswing_max()
>> I think the logic for choosing the correct vswing/emphasis combination
>> should be in a different function, as you suggested it can be in
>> intel_ddi_dp_possible_vswing_max
>>
>> Thanks
>> Khaled
>>
>>> where we set the possible vswing max based on requested pre emphasis
>>> and table in the bspec
>>> Eg: if requested pre emphasis is 3, the vswing is 0 which is the max
>>> vswing value
>>> it can ouput with that pre emphasis level based on the bspec vswing
>>> programming values
>>>
>>> Set v = that value
>>> p = requested
>>>
>>> Link train
>>>
>>> Ville, is this logic correct?
>>>
>>> Regards
>>> Manasi
>>>
>>>
>>>>
>>>>
>>>>>   	} else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
>>>>> {
>>>>>   		switch (voltage_swing & DP_TRAIN_VOLTAGE_SWING_MASK) {
>>>>>   		case DP_TRAIN_VOLTAGE_SWING_LEVEL_0:
>>>>> -- 
>>>>> 2.17.1
>>>>>
>>>>> _______________________________________________
>>>>> Intel-gfx mailing list
>>>>> Intel-gfx@lists.freedesktop.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>>> -- 
>>>> Ville Syrjälä
>>>> Intel
>>>> _______________________________________________
>>>> Intel-gfx mailing list
>>>> Intel-gfx@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx