| Message ID | 20190520232541.16085-1-khaled.almahallawy@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Fix the interpretation of MAX_PRE-EMPHASIS_REACHED bit inorder to pass Link Layer compliance test number 400.3.1.15 | expand |
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. Also we need to update the code to pick the correct swing/pre-emphasis when we can't do what is being requested. 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." > } 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
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; > > - } As per the Bspec Voltage swing programming section, the above code makes sense where the max pre emphasis if vswing level is 0 is 3, max pre emphasis if vswing level 1 is 2 and so on.. So then are you suggesting modifying the entire logic for intel_get_adjust_train() in case we want to always set the max_pre_emphasis to say 3 and max vswing to 3 and then adjust to the requested values as long as the combination is as per the above table (as per the bspec) Regards Manasi > > -} > > - > > 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. > > Also we need to update the code to pick the correct swing/pre-emphasis > when we can't do what is being requested. > > 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." > > > > > } 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
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. > > 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 > > 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 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_possible_vswing_max() 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
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
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 > > > 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 > >
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 >>
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; } 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:
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). 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(-)