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

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

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.

> 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