diff mbox series

[v2] drm/i915/edp/jsl: Update vswing table for HBR and HBR2

Message ID 20200929121127.254086-1-tejaskumarx.surendrakumar.upadhyay@intel.com (mailing list archive)
State New, archived
Headers show
Series [v2] drm/i915/edp/jsl: Update vswing table for HBR and HBR2 | expand

Commit Message

Tejas Upadhyay Sept. 29, 2020, 12:11 p.m. UTC
JSL has update in vswing table for eDP

BSpec: 21257

Changes since V1 : 
	- IS_ELKHARTLAKE and IS_JASPERLAKE is replaced with
          HAS_PCH_MCC(EHL) and HAS_PCH_JSP(JSL) respectively
	- Reverted EHL/JSL PCI ids split change

Signed-off-by: Tejas Upadhyay <tejaskumarx.surendrakumar.upadhyay@intel.com>
---
 drivers/gpu/drm/i915/display/intel_ddi.c | 67 ++++++++++++++++++++++--
 1 file changed, 64 insertions(+), 3 deletions(-)

Comments

Ville Syrjälä Sept. 29, 2020, 12:52 p.m. UTC | #1
On Tue, Sep 29, 2020 at 05:41:27PM +0530, Tejas Upadhyay wrote:
> JSL has update in vswing table for eDP
> 
> BSpec: 21257
> 
> Changes since V1 : 
> 	- IS_ELKHARTLAKE and IS_JASPERLAKE is replaced with
>           HAS_PCH_MCC(EHL) and HAS_PCH_JSP(JSL) respectively

What do vswing values have to do with the PCH type?

> 	- Reverted EHL/JSL PCI ids split change
> 
> Signed-off-by: Tejas Upadhyay <tejaskumarx.surendrakumar.upadhyay@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_ddi.c | 67 ++++++++++++++++++++++--
>  1 file changed, 64 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> index 4d06178cd76c..e6e93d01d0ce 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -582,6 +582,34 @@ static const struct cnl_ddi_buf_trans ehl_combo_phy_ddi_translations_dp[] = {
>  	{ 0x6, 0x7F, 0x3F, 0x00, 0x00 },	/* 900   900      0.0   */
>  };
>  
> +static const struct cnl_ddi_buf_trans jsl_combo_phy_ddi_translations_edp_hbr[] = {
> +						/* NT mV Trans mV db    */
> +	{ 0x8, 0x7F, 0x3F, 0x00, 0x00 },	/* 200   200      0.0   */
> +	{ 0x8, 0x7F, 0x38, 0x00, 0x07 },	/* 200   250      1.9   */
> +	{ 0x1, 0x7F, 0x33, 0x00, 0x0C },	/* 200   300      3.5   */
> +	{ 0xA, 0x35, 0x36, 0x00, 0x09 },	/* 200   350      4.9   */
> +	{ 0x8, 0x7F, 0x3F, 0x00, 0x00 },	/* 250   250      0.0   */
> +	{ 0x1, 0x7F, 0x38, 0x00, 0x07 },	/* 250   300      1.6   */
> +	{ 0xA, 0x35, 0x35, 0x00, 0x0A },	/* 250   350      2.9   */
> +	{ 0x1, 0x7F, 0x3F, 0x00, 0x00 },	/* 300   300      0.0   */
> +	{ 0xA, 0x35, 0x38, 0x00, 0x07 },	/* 300   350      1.3   */
> +	{ 0xA, 0x35, 0x3F, 0x00, 0x00 },	/* 350   350      0.0   */
> +};
> +
> +static const struct cnl_ddi_buf_trans jsl_combo_phy_ddi_translations_edp_hbr2[] = {
> +						/* NT mV Trans mV db    */
> +	{ 0x8, 0x7F, 0x3F, 0x00, 0x00 },	/* 200   200      0.0   */
> +	{ 0x8, 0x7F, 0x3F, 0x00, 0x00 },	/* 200   250      1.9   */
> +	{ 0x1, 0x7F, 0x3D, 0x00, 0x02 },	/* 200   300      3.5   */
> +	{ 0xA, 0x35, 0x38, 0x00, 0x07 },	/* 200   350      4.9   */
> +	{ 0x8, 0x7F, 0x3F, 0x00, 0x00 },	/* 250   250      0.0   */
> +	{ 0x1, 0x7F, 0x3F, 0x00, 0x00 },	/* 250   300      1.6   */
> +	{ 0xA, 0x35, 0x3A, 0x00, 0x05 },	/* 250   350      2.9   */
> +	{ 0x1, 0x7F, 0x3F, 0x00, 0x00 },	/* 300   300      0.0   */
> +	{ 0xA, 0x35, 0x38, 0x00, 0x07 },	/* 300   350      1.3   */
> +	{ 0xA, 0x35, 0x3F, 0x00, 0x00 },	/* 350   350      0.0   */
> +};
> +
>  struct icl_mg_phy_ddi_buf_trans {
>  	u32 cri_txdeemph_override_11_6;
>  	u32 cri_txdeemph_override_5_0;
> @@ -1069,7 +1097,6 @@ icl_get_mg_buf_trans(struct intel_encoder *encoder, int type, int rate,
>  	*n_entries = ARRAY_SIZE(icl_mg_phy_ddi_translations_rbr_hbr);
>  	return icl_mg_phy_ddi_translations_rbr_hbr;
>  }
> -
>  static const struct cnl_ddi_buf_trans *
>  ehl_get_combo_buf_trans(struct intel_encoder *encoder, int type, int rate,
>  			int *n_entries)
> @@ -1098,6 +1125,34 @@ ehl_get_combo_buf_trans(struct intel_encoder *encoder, int type, int rate,
>  	}
>  }
>  
> +static const struct cnl_ddi_buf_trans *
> +jsl_get_combo_buf_trans(struct intel_encoder *encoder, int type, int rate,
> +			int *n_entries)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> +
> +	switch (type) {
> +	case INTEL_OUTPUT_HDMI:
> +		*n_entries = ARRAY_SIZE(icl_combo_phy_ddi_translations_hdmi);
> +		return icl_combo_phy_ddi_translations_hdmi;
> +	case INTEL_OUTPUT_EDP:
> +		if (dev_priv->vbt.edp.low_vswing) {
> +			if (rate > 270000) {
> +				*n_entries = ARRAY_SIZE(jsl_combo_phy_ddi_translations_edp_hbr2);
> +				return jsl_combo_phy_ddi_translations_edp_hbr2;
> +			} else {
> +				*n_entries = ARRAY_SIZE(jsl_combo_phy_ddi_translations_edp_hbr);
> +				return jsl_combo_phy_ddi_translations_edp_hbr;
> +			}
> +		}
> +		/* fall through */
> +	default:
> +		/* All combo DP and eDP ports that do not support low_vswing */
> +		*n_entries = ARRAY_SIZE(icl_combo_phy_ddi_translations_dp_hbr2);
> +		return icl_combo_phy_ddi_translations_dp_hbr2;
> +	}
> +}
> +
>  static const struct cnl_ddi_buf_trans *
>  tgl_get_combo_buf_trans(struct intel_encoder *encoder, int type, int rate,
>  			int *n_entries)
> @@ -2265,7 +2320,10 @@ static u8 intel_ddi_dp_voltage_max(struct intel_dp *intel_dp)
>  			tgl_get_dkl_buf_trans(encoder, encoder->type,
>  					      intel_dp->link_rate, &n_entries);
>  	} else if (INTEL_GEN(dev_priv) == 11) {
> -		if (IS_ELKHARTLAKE(dev_priv))
> +		if (HAS_PCH_JSP(dev_priv))
> +			jsl_get_combo_buf_trans(encoder, encoder->type,
> +						intel_dp->link_rate, &n_entries);
> +		else if (HAS_PCH_MCC(dev_priv))
>  			ehl_get_combo_buf_trans(encoder, encoder->type,
>  						intel_dp->link_rate, &n_entries);
>  		else if (intel_phy_is_combo(dev_priv, phy))
> @@ -2454,7 +2512,10 @@ static void icl_ddi_combo_vswing_program(struct intel_encoder *encoder,
>  	if (INTEL_GEN(dev_priv) >= 12)
>  		ddi_translations = tgl_get_combo_buf_trans(encoder, type, rate,
>  							   &n_entries);
> -	else if (IS_ELKHARTLAKE(dev_priv))
> +	else if (HAS_PCH_JSP(dev_priv))
> +		ddi_translations = jsl_get_combo_buf_trans(encoder, type, rate,
> +							   &n_entries);
> +	else if (HAS_PCH_MCC(dev_priv))
>  		ddi_translations = ehl_get_combo_buf_trans(encoder, type, rate,
>  							   &n_entries);
>  	else
> -- 
> 2.28.0
Tejas Upadhyay Sept. 29, 2020, 1:17 p.m. UTC | #2
-----Original Message-----
From: Ville Syrjälä <ville.syrjala@linux.intel.com> 
Sent: 29 September 2020 18:23
To: Surendrakumar Upadhyay, TejaskumarX <tejaskumarx.surendrakumar.upadhyay@intel.com>
Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Ausmus, James <james.ausmus@intel.com>; Roper, Matthew D <matthew.d.roper@intel.com>; Souza, Jose <jose.souza@intel.com>; De Marchi, Lucas <lucas.demarchi@intel.com>; Pandey, Hariom <hariom.pandey@intel.com>
Subject: Re: [PATCH v2] drm/i915/edp/jsl: Update vswing table for HBR and HBR2

On Tue, Sep 29, 2020 at 05:41:27PM +0530, Tejas Upadhyay wrote:
> JSL has update in vswing table for eDP
> 
> BSpec: 21257
> 
> Changes since V1 : 
> 	- IS_ELKHARTLAKE and IS_JASPERLAKE is replaced with
>           HAS_PCH_MCC(EHL) and HAS_PCH_JSP(JSL) respectively

What do vswing values have to do with the PCH type?

Tejas : There is difference in voltage swing values for EHL and JSL. Till now we were not differentiating between EHL and JSL as both were
based on ICL. Now as per compliance test we have found change in JSL eDP vswing values which does not apply to EHL which leads to differentiate
between EHL and JSL. Thus differentiator between JSL and EHL is PCH i.e HAS_PCH_MCC(EHL) and HAS_PCH_JSP(JSL). There is no direct relation of PCH with vswing.

> 	- Reverted EHL/JSL PCI ids split change
> 
> Signed-off-by: Tejas Upadhyay 
> <tejaskumarx.surendrakumar.upadhyay@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_ddi.c | 67 
> ++++++++++++++++++++++--
>  1 file changed, 64 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c 
> b/drivers/gpu/drm/i915/display/intel_ddi.c
> index 4d06178cd76c..e6e93d01d0ce 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -582,6 +582,34 @@ static const struct cnl_ddi_buf_trans ehl_combo_phy_ddi_translations_dp[] = {
>  	{ 0x6, 0x7F, 0x3F, 0x00, 0x00 },	/* 900   900      0.0   */
>  };
>  
> +static const struct cnl_ddi_buf_trans jsl_combo_phy_ddi_translations_edp_hbr[] = {
> +						/* NT mV Trans mV db    */
> +	{ 0x8, 0x7F, 0x3F, 0x00, 0x00 },	/* 200   200      0.0   */
> +	{ 0x8, 0x7F, 0x38, 0x00, 0x07 },	/* 200   250      1.9   */
> +	{ 0x1, 0x7F, 0x33, 0x00, 0x0C },	/* 200   300      3.5   */
> +	{ 0xA, 0x35, 0x36, 0x00, 0x09 },	/* 200   350      4.9   */
> +	{ 0x8, 0x7F, 0x3F, 0x00, 0x00 },	/* 250   250      0.0   */
> +	{ 0x1, 0x7F, 0x38, 0x00, 0x07 },	/* 250   300      1.6   */
> +	{ 0xA, 0x35, 0x35, 0x00, 0x0A },	/* 250   350      2.9   */
> +	{ 0x1, 0x7F, 0x3F, 0x00, 0x00 },	/* 300   300      0.0   */
> +	{ 0xA, 0x35, 0x38, 0x00, 0x07 },	/* 300   350      1.3   */
> +	{ 0xA, 0x35, 0x3F, 0x00, 0x00 },	/* 350   350      0.0   */
> +};
> +
> +static const struct cnl_ddi_buf_trans jsl_combo_phy_ddi_translations_edp_hbr2[] = {
> +						/* NT mV Trans mV db    */
> +	{ 0x8, 0x7F, 0x3F, 0x00, 0x00 },	/* 200   200      0.0   */
> +	{ 0x8, 0x7F, 0x3F, 0x00, 0x00 },	/* 200   250      1.9   */
> +	{ 0x1, 0x7F, 0x3D, 0x00, 0x02 },	/* 200   300      3.5   */
> +	{ 0xA, 0x35, 0x38, 0x00, 0x07 },	/* 200   350      4.9   */
> +	{ 0x8, 0x7F, 0x3F, 0x00, 0x00 },	/* 250   250      0.0   */
> +	{ 0x1, 0x7F, 0x3F, 0x00, 0x00 },	/* 250   300      1.6   */
> +	{ 0xA, 0x35, 0x3A, 0x00, 0x05 },	/* 250   350      2.9   */
> +	{ 0x1, 0x7F, 0x3F, 0x00, 0x00 },	/* 300   300      0.0   */
> +	{ 0xA, 0x35, 0x38, 0x00, 0x07 },	/* 300   350      1.3   */
> +	{ 0xA, 0x35, 0x3F, 0x00, 0x00 },	/* 350   350      0.0   */
> +};
> +
>  struct icl_mg_phy_ddi_buf_trans {
>  	u32 cri_txdeemph_override_11_6;
>  	u32 cri_txdeemph_override_5_0;
> @@ -1069,7 +1097,6 @@ icl_get_mg_buf_trans(struct intel_encoder *encoder, int type, int rate,
>  	*n_entries = ARRAY_SIZE(icl_mg_phy_ddi_translations_rbr_hbr);
>  	return icl_mg_phy_ddi_translations_rbr_hbr;
>  }
> -
>  static const struct cnl_ddi_buf_trans *  
> ehl_get_combo_buf_trans(struct intel_encoder *encoder, int type, int rate,
>  			int *n_entries)
> @@ -1098,6 +1125,34 @@ ehl_get_combo_buf_trans(struct intel_encoder *encoder, int type, int rate,
>  	}
>  }
>  
> +static const struct cnl_ddi_buf_trans * 
> +jsl_get_combo_buf_trans(struct intel_encoder *encoder, int type, int rate,
> +			int *n_entries)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> +
> +	switch (type) {
> +	case INTEL_OUTPUT_HDMI:
> +		*n_entries = ARRAY_SIZE(icl_combo_phy_ddi_translations_hdmi);
> +		return icl_combo_phy_ddi_translations_hdmi;
> +	case INTEL_OUTPUT_EDP:
> +		if (dev_priv->vbt.edp.low_vswing) {
> +			if (rate > 270000) {
> +				*n_entries = ARRAY_SIZE(jsl_combo_phy_ddi_translations_edp_hbr2);
> +				return jsl_combo_phy_ddi_translations_edp_hbr2;
> +			} else {
> +				*n_entries = ARRAY_SIZE(jsl_combo_phy_ddi_translations_edp_hbr);
> +				return jsl_combo_phy_ddi_translations_edp_hbr;
> +			}
> +		}
> +		/* fall through */
> +	default:
> +		/* All combo DP and eDP ports that do not support low_vswing */
> +		*n_entries = ARRAY_SIZE(icl_combo_phy_ddi_translations_dp_hbr2);
> +		return icl_combo_phy_ddi_translations_dp_hbr2;
> +	}
> +}
> +
>  static const struct cnl_ddi_buf_trans *  
> tgl_get_combo_buf_trans(struct intel_encoder *encoder, int type, int rate,
>  			int *n_entries)
> @@ -2265,7 +2320,10 @@ static u8 intel_ddi_dp_voltage_max(struct intel_dp *intel_dp)
>  			tgl_get_dkl_buf_trans(encoder, encoder->type,
>  					      intel_dp->link_rate, &n_entries);
>  	} else if (INTEL_GEN(dev_priv) == 11) {
> -		if (IS_ELKHARTLAKE(dev_priv))
> +		if (HAS_PCH_JSP(dev_priv))
> +			jsl_get_combo_buf_trans(encoder, encoder->type,
> +						intel_dp->link_rate, &n_entries);
> +		else if (HAS_PCH_MCC(dev_priv))
>  			ehl_get_combo_buf_trans(encoder, encoder->type,
>  						intel_dp->link_rate, &n_entries);
>  		else if (intel_phy_is_combo(dev_priv, phy)) @@ -2454,7 +2512,10 @@ 
> static void icl_ddi_combo_vswing_program(struct intel_encoder *encoder,
>  	if (INTEL_GEN(dev_priv) >= 12)
>  		ddi_translations = tgl_get_combo_buf_trans(encoder, type, rate,
>  							   &n_entries);
> -	else if (IS_ELKHARTLAKE(dev_priv))
> +	else if (HAS_PCH_JSP(dev_priv))
> +		ddi_translations = jsl_get_combo_buf_trans(encoder, type, rate,
> +							   &n_entries);
> +	else if (HAS_PCH_MCC(dev_priv))
>  		ddi_translations = ehl_get_combo_buf_trans(encoder, type, rate,
>  							   &n_entries);
>  	else
> --
> 2.28.0

--
Ville Syrjälä
Intel
Souza, Jose Sept. 29, 2020, 7:33 p.m. UTC | #3
On Tue, 2020-09-29 at 17:41 +0530, Tejas Upadhyay wrote:
> JSL has update in vswing table for eDP

Would be nice to mention in the commit description why PCH is being used, that would avoid Ville's question.

> 
> BSpec: 21257
> 
> Changes since V1 : 
> 	- IS_ELKHARTLAKE and IS_JASPERLAKE is replaced with
>           HAS_PCH_MCC(EHL) and HAS_PCH_JSP(JSL) respectively
> 	- Reverted EHL/JSL PCI ids split change
> 
> Signed-off-by: Tejas Upadhyay <
> tejaskumarx.surendrakumar.upadhyay@intel.com
> >
> ---
>  drivers/gpu/drm/i915/display/intel_ddi.c | 67 ++++++++++++++++++++++--
>  1 file changed, 64 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> index 4d06178cd76c..e6e93d01d0ce 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -582,6 +582,34 @@ static const struct cnl_ddi_buf_trans ehl_combo_phy_ddi_translations_dp[] = {
>  	{ 0x6, 0x7F, 0x3F, 0x00, 0x00 },	/* 900   900      0.0   */
>  };
>  
> +static const struct cnl_ddi_buf_trans jsl_combo_phy_ddi_translations_edp_hbr[] = {
> +						/* NT mV Trans mV db    */
> +	{ 0x8, 0x7F, 0x3F, 0x00, 0x00 },	/* 200   200      0.0   */
> +	{ 0x8, 0x7F, 0x38, 0x00, 0x07 },	/* 200   250      1.9   */
> +	{ 0x1, 0x7F, 0x33, 0x00, 0x0C },	/* 200   300      3.5   */
> +	{ 0xA, 0x35, 0x36, 0x00, 0x09 },	/* 200   350      4.9   */
> +	{ 0x8, 0x7F, 0x3F, 0x00, 0x00 },	/* 250   250      0.0   */
> +	{ 0x1, 0x7F, 0x38, 0x00, 0x07 },	/* 250   300      1.6   */
> +	{ 0xA, 0x35, 0x35, 0x00, 0x0A },	/* 250   350      2.9   */
> +	{ 0x1, 0x7F, 0x3F, 0x00, 0x00 },	/* 300   300      0.0   */
> +	{ 0xA, 0x35, 0x38, 0x00, 0x07 },	/* 300   350      1.3   */
> +	{ 0xA, 0x35, 0x3F, 0x00, 0x00 },	/* 350   350      0.0   */
> +};
> +
> +static const struct cnl_ddi_buf_trans jsl_combo_phy_ddi_translations_edp_hbr2[] = {
> +						/* NT mV Trans mV db    */
> +	{ 0x8, 0x7F, 0x3F, 0x00, 0x00 },	/* 200   200      0.0   */
> +	{ 0x8, 0x7F, 0x3F, 0x00, 0x00 },	/* 200   250      1.9   */
> +	{ 0x1, 0x7F, 0x3D, 0x00, 0x02 },	/* 200   300      3.5   */
> +	{ 0xA, 0x35, 0x38, 0x00, 0x07 },	/* 200   350      4.9   */
> +	{ 0x8, 0x7F, 0x3F, 0x00, 0x00 },	/* 250   250      0.0   */
> +	{ 0x1, 0x7F, 0x3F, 0x00, 0x00 },	/* 250   300      1.6   */
> +	{ 0xA, 0x35, 0x3A, 0x00, 0x05 },	/* 250   350      2.9   */
> +	{ 0x1, 0x7F, 0x3F, 0x00, 0x00 },	/* 300   300      0.0   */
> +	{ 0xA, 0x35, 0x38, 0x00, 0x07 },	/* 300   350      1.3   */
> +	{ 0xA, 0x35, 0x3F, 0x00, 0x00 },	/* 350   350      0.0   */
> +};

Tables matches specification.

> +
>  struct icl_mg_phy_ddi_buf_trans {
>  	u32 cri_txdeemph_override_11_6;
>  	u32 cri_txdeemph_override_5_0;
> @@ -1069,7 +1097,6 @@ icl_get_mg_buf_trans(struct intel_encoder *encoder, int type, int rate,
>  	*n_entries = ARRAY_SIZE(icl_mg_phy_ddi_translations_rbr_hbr);
>  	return icl_mg_phy_ddi_translations_rbr_hbr;
>  }
> -

Probably not intentional.

Reviewed-by: José Roberto de Souza <jose.souza@intel.com>

Will push with this line fixed as soon as CI finish testing.


>  static const struct cnl_ddi_buf_trans *
>  ehl_get_combo_buf_trans(struct intel_encoder *encoder, int type, int rate,
>  			int *n_entries)
> @@ -1098,6 +1125,34 @@ ehl_get_combo_buf_trans(struct intel_encoder *encoder, int type, int rate,
>  	}
>  }
>  
> +static const struct cnl_ddi_buf_trans *
> +jsl_get_combo_buf_trans(struct intel_encoder *encoder, int type, int rate,
> +			int *n_entries)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> +
> +	switch (type) {
> +	case INTEL_OUTPUT_HDMI:
> +		*n_entries = ARRAY_SIZE(icl_combo_phy_ddi_translations_hdmi);
> +		return icl_combo_phy_ddi_translations_hdmi;
> +	case INTEL_OUTPUT_EDP:
> +		if (dev_priv->vbt.edp.low_vswing) {
> +			if (rate > 270000) {
> +				*n_entries = ARRAY_SIZE(jsl_combo_phy_ddi_translations_edp_hbr2);
> +				return jsl_combo_phy_ddi_translations_edp_hbr2;
> +			} else {
> +				*n_entries = ARRAY_SIZE(jsl_combo_phy_ddi_translations_edp_hbr);
> +				return jsl_combo_phy_ddi_translations_edp_hbr;
> +			}
> +		}
> +		/* fall through */
> +	default:
> +		/* All combo DP and eDP ports that do not support low_vswing */
> +		*n_entries = ARRAY_SIZE(icl_combo_phy_ddi_translations_dp_hbr2);
> +		return icl_combo_phy_ddi_translations_dp_hbr2;
> +	}
> +}
> +
>  static const struct cnl_ddi_buf_trans *
>  tgl_get_combo_buf_trans(struct intel_encoder *encoder, int type, int rate,
>  			int *n_entries)
> @@ -2265,7 +2320,10 @@ static u8 intel_ddi_dp_voltage_max(struct intel_dp *intel_dp)
>  			tgl_get_dkl_buf_trans(encoder, encoder->type,
>  					      intel_dp->link_rate, &n_entries);
>  	} else if (INTEL_GEN(dev_priv) == 11) {
> -		if (IS_ELKHARTLAKE(dev_priv))
> +		if (HAS_PCH_JSP(dev_priv))
> +			jsl_get_combo_buf_trans(encoder, encoder->type,
> +						intel_dp->link_rate, &n_entries);
> +		else if (HAS_PCH_MCC(dev_priv))
>  			ehl_get_combo_buf_trans(encoder, encoder->type,
>  						intel_dp->link_rate, &n_entries);
>  		else if (intel_phy_is_combo(dev_priv, phy))
> @@ -2454,7 +2512,10 @@ static void icl_ddi_combo_vswing_program(struct intel_encoder *encoder,
>  	if (INTEL_GEN(dev_priv) >= 12)
>  		ddi_translations = tgl_get_combo_buf_trans(encoder, type, rate,
>  							   &n_entries);
> -	else if (IS_ELKHARTLAKE(dev_priv))
> +	else if (HAS_PCH_JSP(dev_priv))
> +		ddi_translations = jsl_get_combo_buf_trans(encoder, type, rate,
> +							   &n_entries);
> +	else if (HAS_PCH_MCC(dev_priv))
>  		ddi_translations = ehl_get_combo_buf_trans(encoder, type, rate,
>  							   &n_entries);
>  	else
>
Ville Syrjälä Sept. 29, 2020, 8:02 p.m. UTC | #4
On Tue, Sep 29, 2020 at 07:33:45PM +0000, Souza, Jose wrote:
> On Tue, 2020-09-29 at 17:41 +0530, Tejas Upadhyay wrote:
> > JSL has update in vswing table for eDP
> 
> Would be nice to mention in the commit description why PCH is being used, that would avoid Ville's question.

If the thing has nothing to do PCH then it should not use the PCH type
for the the check. Instead we should just do the EHL/JSL split.

> 
> > 
> > BSpec: 21257
> > 
> > Changes since V1 : 
> > 	- IS_ELKHARTLAKE and IS_JASPERLAKE is replaced with
> >           HAS_PCH_MCC(EHL) and HAS_PCH_JSP(JSL) respectively
> > 	- Reverted EHL/JSL PCI ids split change
> > 
> > Signed-off-by: Tejas Upadhyay <
> > tejaskumarx.surendrakumar.upadhyay@intel.com
> > >
> > ---
> >  drivers/gpu/drm/i915/display/intel_ddi.c | 67 ++++++++++++++++++++++--
> >  1 file changed, 64 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> > index 4d06178cd76c..e6e93d01d0ce 100644
> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > @@ -582,6 +582,34 @@ static const struct cnl_ddi_buf_trans ehl_combo_phy_ddi_translations_dp[] = {
> >  	{ 0x6, 0x7F, 0x3F, 0x00, 0x00 },	/* 900   900      0.0   */
> >  };
> >  
> > +static const struct cnl_ddi_buf_trans jsl_combo_phy_ddi_translations_edp_hbr[] = {
> > +						/* NT mV Trans mV db    */
> > +	{ 0x8, 0x7F, 0x3F, 0x00, 0x00 },	/* 200   200      0.0   */
> > +	{ 0x8, 0x7F, 0x38, 0x00, 0x07 },	/* 200   250      1.9   */
> > +	{ 0x1, 0x7F, 0x33, 0x00, 0x0C },	/* 200   300      3.5   */
> > +	{ 0xA, 0x35, 0x36, 0x00, 0x09 },	/* 200   350      4.9   */
> > +	{ 0x8, 0x7F, 0x3F, 0x00, 0x00 },	/* 250   250      0.0   */
> > +	{ 0x1, 0x7F, 0x38, 0x00, 0x07 },	/* 250   300      1.6   */
> > +	{ 0xA, 0x35, 0x35, 0x00, 0x0A },	/* 250   350      2.9   */
> > +	{ 0x1, 0x7F, 0x3F, 0x00, 0x00 },	/* 300   300      0.0   */
> > +	{ 0xA, 0x35, 0x38, 0x00, 0x07 },	/* 300   350      1.3   */
> > +	{ 0xA, 0x35, 0x3F, 0x00, 0x00 },	/* 350   350      0.0   */
> > +};
> > +
> > +static const struct cnl_ddi_buf_trans jsl_combo_phy_ddi_translations_edp_hbr2[] = {
> > +						/* NT mV Trans mV db    */
> > +	{ 0x8, 0x7F, 0x3F, 0x00, 0x00 },	/* 200   200      0.0   */
> > +	{ 0x8, 0x7F, 0x3F, 0x00, 0x00 },	/* 200   250      1.9   */
> > +	{ 0x1, 0x7F, 0x3D, 0x00, 0x02 },	/* 200   300      3.5   */
> > +	{ 0xA, 0x35, 0x38, 0x00, 0x07 },	/* 200   350      4.9   */
> > +	{ 0x8, 0x7F, 0x3F, 0x00, 0x00 },	/* 250   250      0.0   */
> > +	{ 0x1, 0x7F, 0x3F, 0x00, 0x00 },	/* 250   300      1.6   */
> > +	{ 0xA, 0x35, 0x3A, 0x00, 0x05 },	/* 250   350      2.9   */
> > +	{ 0x1, 0x7F, 0x3F, 0x00, 0x00 },	/* 300   300      0.0   */
> > +	{ 0xA, 0x35, 0x38, 0x00, 0x07 },	/* 300   350      1.3   */
> > +	{ 0xA, 0x35, 0x3F, 0x00, 0x00 },	/* 350   350      0.0   */
> > +};
> 
> Tables matches specification.
> 
> > +
> >  struct icl_mg_phy_ddi_buf_trans {
> >  	u32 cri_txdeemph_override_11_6;
> >  	u32 cri_txdeemph_override_5_0;
> > @@ -1069,7 +1097,6 @@ icl_get_mg_buf_trans(struct intel_encoder *encoder, int type, int rate,
> >  	*n_entries = ARRAY_SIZE(icl_mg_phy_ddi_translations_rbr_hbr);
> >  	return icl_mg_phy_ddi_translations_rbr_hbr;
> >  }
> > -
> 
> Probably not intentional.
> 
> Reviewed-by: José Roberto de Souza <jose.souza@intel.com>
> 
> Will push with this line fixed as soon as CI finish testing.
> 
> 
> >  static const struct cnl_ddi_buf_trans *
> >  ehl_get_combo_buf_trans(struct intel_encoder *encoder, int type, int rate,
> >  			int *n_entries)
> > @@ -1098,6 +1125,34 @@ ehl_get_combo_buf_trans(struct intel_encoder *encoder, int type, int rate,
> >  	}
> >  }
> >  
> > +static const struct cnl_ddi_buf_trans *
> > +jsl_get_combo_buf_trans(struct intel_encoder *encoder, int type, int rate,
> > +			int *n_entries)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > +
> > +	switch (type) {
> > +	case INTEL_OUTPUT_HDMI:
> > +		*n_entries = ARRAY_SIZE(icl_combo_phy_ddi_translations_hdmi);
> > +		return icl_combo_phy_ddi_translations_hdmi;
> > +	case INTEL_OUTPUT_EDP:
> > +		if (dev_priv->vbt.edp.low_vswing) {
> > +			if (rate > 270000) {
> > +				*n_entries = ARRAY_SIZE(jsl_combo_phy_ddi_translations_edp_hbr2);
> > +				return jsl_combo_phy_ddi_translations_edp_hbr2;
> > +			} else {
> > +				*n_entries = ARRAY_SIZE(jsl_combo_phy_ddi_translations_edp_hbr);
> > +				return jsl_combo_phy_ddi_translations_edp_hbr;
> > +			}
> > +		}
> > +		/* fall through */
> > +	default:
> > +		/* All combo DP and eDP ports that do not support low_vswing */
> > +		*n_entries = ARRAY_SIZE(icl_combo_phy_ddi_translations_dp_hbr2);
> > +		return icl_combo_phy_ddi_translations_dp_hbr2;
> > +	}
> > +}
> > +
> >  static const struct cnl_ddi_buf_trans *
> >  tgl_get_combo_buf_trans(struct intel_encoder *encoder, int type, int rate,
> >  			int *n_entries)
> > @@ -2265,7 +2320,10 @@ static u8 intel_ddi_dp_voltage_max(struct intel_dp *intel_dp)
> >  			tgl_get_dkl_buf_trans(encoder, encoder->type,
> >  					      intel_dp->link_rate, &n_entries);
> >  	} else if (INTEL_GEN(dev_priv) == 11) {
> > -		if (IS_ELKHARTLAKE(dev_priv))
> > +		if (HAS_PCH_JSP(dev_priv))
> > +			jsl_get_combo_buf_trans(encoder, encoder->type,
> > +						intel_dp->link_rate, &n_entries);
> > +		else if (HAS_PCH_MCC(dev_priv))
> >  			ehl_get_combo_buf_trans(encoder, encoder->type,
> >  						intel_dp->link_rate, &n_entries);
> >  		else if (intel_phy_is_combo(dev_priv, phy))
> > @@ -2454,7 +2512,10 @@ static void icl_ddi_combo_vswing_program(struct intel_encoder *encoder,
> >  	if (INTEL_GEN(dev_priv) >= 12)
> >  		ddi_translations = tgl_get_combo_buf_trans(encoder, type, rate,
> >  							   &n_entries);
> > -	else if (IS_ELKHARTLAKE(dev_priv))
> > +	else if (HAS_PCH_JSP(dev_priv))
> > +		ddi_translations = jsl_get_combo_buf_trans(encoder, type, rate,
> > +							   &n_entries);
> > +	else if (HAS_PCH_MCC(dev_priv))
> >  		ddi_translations = ehl_get_combo_buf_trans(encoder, type, rate,
> >  							   &n_entries);
> >  	else
> >
Souza, Jose Sept. 29, 2020, 8:20 p.m. UTC | #5
On Tue, 2020-09-29 at 23:02 +0300, Ville Syrjälä wrote:
> On Tue, Sep 29, 2020 at 07:33:45PM +0000, Souza, Jose wrote:
> > On Tue, 2020-09-29 at 17:41 +0530, Tejas Upadhyay wrote:
> > > JSL has update in vswing table for eDP
> > 
> > Would be nice to mention in the commit description why PCH is being used, that would avoid Ville's question.
> 
> If the thing has nothing to do PCH then it should not use the PCH type
> for the the check. Instead we should just do the EHL/JSL split.

In the first version Matt Roper suggested to use PCH to differentiate between EHL and JSL, Jani also agreed with this solution.This 2 PCHs can only be
associate with EHL and JSL respectively, so no downsides here.

> 
> > > BSpec: 21257
> > > 
> > > Changes since V1 : 
> > > 	- IS_ELKHARTLAKE and IS_JASPERLAKE is replaced with
> > >           HAS_PCH_MCC(EHL) and HAS_PCH_JSP(JSL) respectively
> > > 	- Reverted EHL/JSL PCI ids split change
> > > 
> > > Signed-off-by: Tejas Upadhyay <
> > > tejaskumarx.surendrakumar.upadhyay@intel.com
> > > 
> > > 
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_ddi.c | 67 ++++++++++++++++++++++--
> > >  1 file changed, 64 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > index 4d06178cd76c..e6e93d01d0ce 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > @@ -582,6 +582,34 @@ static const struct cnl_ddi_buf_trans ehl_combo_phy_ddi_translations_dp[] = {
> > >  	{ 0x6, 0x7F, 0x3F, 0x00, 0x00 },	/* 900   900      0.0   */
> > >  };
> > >  
> > > +static const struct cnl_ddi_buf_trans jsl_combo_phy_ddi_translations_edp_hbr[] = {
> > > +						/* NT mV Trans mV db    */
> > > +	{ 0x8, 0x7F, 0x3F, 0x00, 0x00 },	/* 200   200      0.0   */
> > > +	{ 0x8, 0x7F, 0x38, 0x00, 0x07 },	/* 200   250      1.9   */
> > > +	{ 0x1, 0x7F, 0x33, 0x00, 0x0C },	/* 200   300      3.5   */
> > > +	{ 0xA, 0x35, 0x36, 0x00, 0x09 },	/* 200   350      4.9   */
> > > +	{ 0x8, 0x7F, 0x3F, 0x00, 0x00 },	/* 250   250      0.0   */
> > > +	{ 0x1, 0x7F, 0x38, 0x00, 0x07 },	/* 250   300      1.6   */
> > > +	{ 0xA, 0x35, 0x35, 0x00, 0x0A },	/* 250   350      2.9   */
> > > +	{ 0x1, 0x7F, 0x3F, 0x00, 0x00 },	/* 300   300      0.0   */
> > > +	{ 0xA, 0x35, 0x38, 0x00, 0x07 },	/* 300   350      1.3   */
> > > +	{ 0xA, 0x35, 0x3F, 0x00, 0x00 },	/* 350   350      0.0   */
> > > +};
> > > +
> > > +static const struct cnl_ddi_buf_trans jsl_combo_phy_ddi_translations_edp_hbr2[] = {
> > > +						/* NT mV Trans mV db    */
> > > +	{ 0x8, 0x7F, 0x3F, 0x00, 0x00 },	/* 200   200      0.0   */
> > > +	{ 0x8, 0x7F, 0x3F, 0x00, 0x00 },	/* 200   250      1.9   */
> > > +	{ 0x1, 0x7F, 0x3D, 0x00, 0x02 },	/* 200   300      3.5   */
> > > +	{ 0xA, 0x35, 0x38, 0x00, 0x07 },	/* 200   350      4.9   */
> > > +	{ 0x8, 0x7F, 0x3F, 0x00, 0x00 },	/* 250   250      0.0   */
> > > +	{ 0x1, 0x7F, 0x3F, 0x00, 0x00 },	/* 250   300      1.6   */
> > > +	{ 0xA, 0x35, 0x3A, 0x00, 0x05 },	/* 250   350      2.9   */
> > > +	{ 0x1, 0x7F, 0x3F, 0x00, 0x00 },	/* 300   300      0.0   */
> > > +	{ 0xA, 0x35, 0x38, 0x00, 0x07 },	/* 300   350      1.3   */
> > > +	{ 0xA, 0x35, 0x3F, 0x00, 0x00 },	/* 350   350      0.0   */
> > > +};
> > 
> > Tables matches specification.
> > 
> > > +
> > >  struct icl_mg_phy_ddi_buf_trans {
> > >  	u32 cri_txdeemph_override_11_6;
> > >  	u32 cri_txdeemph_override_5_0;
> > > @@ -1069,7 +1097,6 @@ icl_get_mg_buf_trans(struct intel_encoder *encoder, int type, int rate,
> > >  	*n_entries = ARRAY_SIZE(icl_mg_phy_ddi_translations_rbr_hbr);
> > >  	return icl_mg_phy_ddi_translations_rbr_hbr;
> > >  }
> > > -
> > 
> > Probably not intentional.
> > 
> > Reviewed-by: José Roberto de Souza <
> > jose.souza@intel.com
> > >
> > 
> > Will push with this line fixed as soon as CI finish testing.
> > 
> > 
> > >  static const struct cnl_ddi_buf_trans *
> > >  ehl_get_combo_buf_trans(struct intel_encoder *encoder, int type, int rate,
> > >  			int *n_entries)
> > > @@ -1098,6 +1125,34 @@ ehl_get_combo_buf_trans(struct intel_encoder *encoder, int type, int rate,
> > >  	}
> > >  }
> > >  
> > > +static const struct cnl_ddi_buf_trans *
> > > +jsl_get_combo_buf_trans(struct intel_encoder *encoder, int type, int rate,
> > > +			int *n_entries)
> > > +{
> > > +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > > +
> > > +	switch (type) {
> > > +	case INTEL_OUTPUT_HDMI:
> > > +		*n_entries = ARRAY_SIZE(icl_combo_phy_ddi_translations_hdmi);
> > > +		return icl_combo_phy_ddi_translations_hdmi;
> > > +	case INTEL_OUTPUT_EDP:
> > > +		if (dev_priv->vbt.edp.low_vswing) {
> > > +			if (rate > 270000) {
> > > +				*n_entries = ARRAY_SIZE(jsl_combo_phy_ddi_translations_edp_hbr2);
> > > +				return jsl_combo_phy_ddi_translations_edp_hbr2;
> > > +			} else {
> > > +				*n_entries = ARRAY_SIZE(jsl_combo_phy_ddi_translations_edp_hbr);
> > > +				return jsl_combo_phy_ddi_translations_edp_hbr;
> > > +			}
> > > +		}
> > > +		/* fall through */
> > > +	default:
> > > +		/* All combo DP and eDP ports that do not support low_vswing */
> > > +		*n_entries = ARRAY_SIZE(icl_combo_phy_ddi_translations_dp_hbr2);
> > > +		return icl_combo_phy_ddi_translations_dp_hbr2;
> > > +	}
> > > +}
> > > +
> > >  static const struct cnl_ddi_buf_trans *
> > >  tgl_get_combo_buf_trans(struct intel_encoder *encoder, int type, int rate,
> > >  			int *n_entries)
> > > @@ -2265,7 +2320,10 @@ static u8 intel_ddi_dp_voltage_max(struct intel_dp *intel_dp)
> > >  			tgl_get_dkl_buf_trans(encoder, encoder->type,
> > >  					      intel_dp->link_rate, &n_entries);
> > >  	} else if (INTEL_GEN(dev_priv) == 11) {
> > > -		if (IS_ELKHARTLAKE(dev_priv))
> > > +		if (HAS_PCH_JSP(dev_priv))
> > > +			jsl_get_combo_buf_trans(encoder, encoder->type,
> > > +						intel_dp->link_rate, &n_entries);
> > > +		else if (HAS_PCH_MCC(dev_priv))
> > >  			ehl_get_combo_buf_trans(encoder, encoder->type,
> > >  						intel_dp->link_rate, &n_entries);
> > >  		else if (intel_phy_is_combo(dev_priv, phy))
> > > @@ -2454,7 +2512,10 @@ static void icl_ddi_combo_vswing_program(struct intel_encoder *encoder,
> > >  	if (INTEL_GEN(dev_priv) >= 12)
> > >  		ddi_translations = tgl_get_combo_buf_trans(encoder, type, rate,
> > >  							   &n_entries);
> > > -	else if (IS_ELKHARTLAKE(dev_priv))
> > > +	else if (HAS_PCH_JSP(dev_priv))
> > > +		ddi_translations = jsl_get_combo_buf_trans(encoder, type, rate,
> > > +							   &n_entries);
> > > +	else if (HAS_PCH_MCC(dev_priv))
> > >  		ddi_translations = ehl_get_combo_buf_trans(encoder, type, rate,
> > >  							   &n_entries);
> > >  	else
> > > 
> 
>
Ville Syrjälä Sept. 29, 2020, 8:30 p.m. UTC | #6
On Tue, Sep 29, 2020 at 08:20:22PM +0000, Souza, Jose wrote:
> On Tue, 2020-09-29 at 23:02 +0300, Ville Syrjälä wrote:
> > On Tue, Sep 29, 2020 at 07:33:45PM +0000, Souza, Jose wrote:
> > > On Tue, 2020-09-29 at 17:41 +0530, Tejas Upadhyay wrote:
> > > > JSL has update in vswing table for eDP
> > > 
> > > Would be nice to mention in the commit description why PCH is being used, that would avoid Ville's question.
> > 
> > If the thing has nothing to do PCH then it should not use the PCH type
> > for the the check. Instead we should just do the EHL/JSL split.
> 
> In the first version Matt Roper suggested to use PCH to differentiate between EHL and JSL, Jani also agreed with this solution.This 2 PCHs can only be
> associate with EHL and JSL respectively, so no downsides here.

The downside is that the code makes no sense on the first glance.
It's going to generate a "wtf?" exception in the brain and require
me to take a second look to figure what is going on. Exception
handling is expensive and shouldn't be needed in cases where it's
trivial to make the code 100% obvious.
Souza, Jose Sept. 29, 2020, 8:34 p.m. UTC | #7
On Tue, 2020-09-29 at 23:30 +0300, Ville Syrjälä wrote:
> On Tue, Sep 29, 2020 at 08:20:22PM +0000, Souza, Jose wrote:
> > On Tue, 2020-09-29 at 23:02 +0300, Ville Syrjälä wrote:
> > > On Tue, Sep 29, 2020 at 07:33:45PM +0000, Souza, Jose wrote:
> > > > On Tue, 2020-09-29 at 17:41 +0530, Tejas Upadhyay wrote:
> > > > > JSL has update in vswing table for eDP
> > > > 
> > > > Would be nice to mention in the commit description why PCH is being used, that would avoid Ville's question.
> > > 
> > > If the thing has nothing to do PCH then it should not use the PCH type
> > > for the the check. Instead we should just do the EHL/JSL split.
> > 
> > In the first version Matt Roper suggested to use PCH to differentiate between EHL and JSL, Jani also agreed with this solution.This 2 PCHs can only be
> > associate with EHL and JSL respectively, so no downsides here.
> 
> The downside is that the code makes no sense on the first glance.
> It's going to generate a "wtf?" exception in the brain and require
> me to take a second look to figure what is going on. Exception
> handling is expensive and shouldn't be needed in cases where it's
> trivial to make the code 100% obvious.
> 

Adding a comment on the top of jsl_get_combo_buf_trans() would help?
Otherwise Tejas you will need to rework this then.
Matt Roper Sept. 29, 2020, 9:01 p.m. UTC | #8
On Tue, Sep 29, 2020 at 11:30:22PM +0300, Ville Syrjälä wrote:
> On Tue, Sep 29, 2020 at 08:20:22PM +0000, Souza, Jose wrote:
> > On Tue, 2020-09-29 at 23:02 +0300, Ville Syrjälä wrote:
> > > On Tue, Sep 29, 2020 at 07:33:45PM +0000, Souza, Jose wrote:
> > > > On Tue, 2020-09-29 at 17:41 +0530, Tejas Upadhyay wrote:
> > > > > JSL has update in vswing table for eDP
> > > > 
> > > > Would be nice to mention in the commit description why PCH is being used, that would avoid Ville's question.
> > > 
> > > If the thing has nothing to do PCH then it should not use the PCH type
> > > for the the check. Instead we should just do the EHL/JSL split.
> > 
> > In the first version Matt Roper suggested to use PCH to differentiate between EHL and JSL, Jani also agreed with this solution.This 2 PCHs can only be
> > associate with EHL and JSL respectively, so no downsides here.
> 
> The downside is that the code makes no sense on the first glance.
> It's going to generate a "wtf?" exception in the brain and require
> me to take a second look to figure what is going on. Exception
> handling is expensive and shouldn't be needed in cases where it's
> trivial to make the code 100% obvious.

The bspec documents EHL and JSL as being the same platform and identical
in all programming since they are literally the same display IP; this
vswing table is the one and only place where the two are treated in a
distinct manner for reasons that lie outside the display controller.  If
you had to stop and take a closer look at the code here, that's a
probably a good thing since in general there should generally never be a
difference in the behavior between the two.  Adding an additional
clarifying comment is probably in order too since this is a very
exceptional special case.

If we deviate from the bspec's guidance and try to split IS_ELKHARTLAKE
and IS_JASPERLAKE across the whole driver, that's going to be a lot more
pain to maintain down the road since we'll almost certainly have cases
where someone silently leaves one or the other off a condition and gets
unexepcted behavior.  I could see arguments for using a SUBPLATFORM here
like we do for TGL_U vs TGL_Y, but even that seems like overkill if we
already have a clear way to distinguish the two cases (PCH pairing) and
can just leave a clarifying comment.


Matt


> 
> -- 
> Ville Syrjälä
> Intel
Ville Syrjälä Sept. 29, 2020, 9:11 p.m. UTC | #9
On Tue, Sep 29, 2020 at 02:01:44PM -0700, Matt Roper wrote:
> On Tue, Sep 29, 2020 at 11:30:22PM +0300, Ville Syrjälä wrote:
> > On Tue, Sep 29, 2020 at 08:20:22PM +0000, Souza, Jose wrote:
> > > On Tue, 2020-09-29 at 23:02 +0300, Ville Syrjälä wrote:
> > > > On Tue, Sep 29, 2020 at 07:33:45PM +0000, Souza, Jose wrote:
> > > > > On Tue, 2020-09-29 at 17:41 +0530, Tejas Upadhyay wrote:
> > > > > > JSL has update in vswing table for eDP
> > > > > 
> > > > > Would be nice to mention in the commit description why PCH is being used, that would avoid Ville's question.
> > > > 
> > > > If the thing has nothing to do PCH then it should not use the PCH type
> > > > for the the check. Instead we should just do the EHL/JSL split.
> > > 
> > > In the first version Matt Roper suggested to use PCH to differentiate between EHL and JSL, Jani also agreed with this solution.This 2 PCHs can only be
> > > associate with EHL and JSL respectively, so no downsides here.
> > 
> > The downside is that the code makes no sense on the first glance.
> > It's going to generate a "wtf?" exception in the brain and require
> > me to take a second look to figure what is going on. Exception
> > handling is expensive and shouldn't be needed in cases where it's
> > trivial to make the code 100% obvious.
> 
> The bspec documents EHL and JSL as being the same platform and identical
> in all programming since they are literally the same display IP; this
> vswing table is the one and only place where the two are treated in a
> distinct manner for reasons that lie outside the display controller.  If
> you had to stop and take a closer look at the code here, that's a
> probably a good thing since in general there should generally never be a
> difference in the behavior between the two.  Adding an additional
> clarifying comment is probably in order too since this is a very
> exceptional special case.
> 
> If we deviate from the bspec's guidance and try to split IS_ELKHARTLAKE
> and IS_JASPERLAKE across the whole driver, that's going to be a lot more
> pain to maintain down the road since we'll almost certainly have cases
> where someone silently leaves one or the other off a condition and gets
> unexepcted behavior.  I could see arguments for using a SUBPLATFORM here
> like we do for TGL_U vs TGL_Y, but even that seems like overkill if we
> already have a clear way to distinguish the two cases (PCH pairing) and
> can just leave a clarifying comment.

That fixed PCH pairing is totally undocumented AFAICS. And vswing has
nothing to do with the south display, so the wtf will still happen.
Comment or no comment.
Ville Syrjälä Sept. 29, 2020, 9:59 p.m. UTC | #10
On Wed, Sep 30, 2020 at 12:11:48AM +0300, Ville Syrjälä wrote:
> On Tue, Sep 29, 2020 at 02:01:44PM -0700, Matt Roper wrote:
> > On Tue, Sep 29, 2020 at 11:30:22PM +0300, Ville Syrjälä wrote:
> > > On Tue, Sep 29, 2020 at 08:20:22PM +0000, Souza, Jose wrote:
> > > > On Tue, 2020-09-29 at 23:02 +0300, Ville Syrjälä wrote:
> > > > > On Tue, Sep 29, 2020 at 07:33:45PM +0000, Souza, Jose wrote:
> > > > > > On Tue, 2020-09-29 at 17:41 +0530, Tejas Upadhyay wrote:
> > > > > > > JSL has update in vswing table for eDP
> > > > > > 
> > > > > > Would be nice to mention in the commit description why PCH is being used, that would avoid Ville's question.
> > > > > 
> > > > > If the thing has nothing to do PCH then it should not use the PCH type
> > > > > for the the check. Instead we should just do the EHL/JSL split.
> > > > 
> > > > In the first version Matt Roper suggested to use PCH to differentiate between EHL and JSL, Jani also agreed with this solution.This 2 PCHs can only be
> > > > associate with EHL and JSL respectively, so no downsides here.
> > > 
> > > The downside is that the code makes no sense on the first glance.
> > > It's going to generate a "wtf?" exception in the brain and require
> > > me to take a second look to figure what is going on. Exception
> > > handling is expensive and shouldn't be needed in cases where it's
> > > trivial to make the code 100% obvious.
> > 
> > The bspec documents EHL and JSL as being the same platform and identical
> > in all programming since they are literally the same display IP; this
> > vswing table is the one and only place where the two are treated in a
> > distinct manner for reasons that lie outside the display controller.  If
> > you had to stop and take a closer look at the code here, that's a
> > probably a good thing since in general there should generally never be a
> > difference in the behavior between the two.  Adding an additional
> > clarifying comment is probably in order too since this is a very
> > exceptional special case.
> > 
> > If we deviate from the bspec's guidance and try to split IS_ELKHARTLAKE
> > and IS_JASPERLAKE across the whole driver, that's going to be a lot more
> > pain to maintain down the road since we'll almost certainly have cases
> > where someone silently leaves one or the other off a condition and gets
> > unexepcted behavior.  I could see arguments for using a SUBPLATFORM here
> > like we do for TGL_U vs TGL_Y, but even that seems like overkill if we
> > already have a clear way to distinguish the two cases (PCH pairing) and
> > can just leave a clarifying comment.
> 
> That fixed PCH pairing is totally undocumented AFAICS. And vswing has
> nothing to do with the south display, so the wtf will still happen.
> Comment or no comment.

Oh and JSP does not show up in bspec even once. MCC appears exactly once
where it talks about the differences between MCC and ICL-N PCH (which I
guess is the same as JSP?).

Furthermore the spec never really talks about EHL except in very select
places. So the IS_ELKHARTLAKE is already totally confusing and IMO more
likely to cause maintenance problems than the split. Making it
IS_JSL||IS_EHL at least gives the reader some hint as to where they
should look in the spec.

Another argument why the split is fine is CFL/CML. Those are more
or less exactly in the same boat as EHL. Ie. only really mentioned
in the "configurations" section. Beyond that only KBL is ever really
mentioned. And yet we have separate IS_FOOs for all of them, and
apparently no one had any objections to that situation.

tldr;we have an established way to handle these things, so IMO lets
just follow it and stop adding special cases.
Matt Roper Sept. 29, 2020, 11:38 p.m. UTC | #11
On Wed, Sep 30, 2020 at 12:59:58AM +0300, Ville Syrjälä wrote:
> On Wed, Sep 30, 2020 at 12:11:48AM +0300, Ville Syrjälä wrote:
> > On Tue, Sep 29, 2020 at 02:01:44PM -0700, Matt Roper wrote:
> > > On Tue, Sep 29, 2020 at 11:30:22PM +0300, Ville Syrjälä wrote:
> > > > On Tue, Sep 29, 2020 at 08:20:22PM +0000, Souza, Jose wrote:
> > > > > On Tue, 2020-09-29 at 23:02 +0300, Ville Syrjälä wrote:
> > > > > > On Tue, Sep 29, 2020 at 07:33:45PM +0000, Souza, Jose wrote:
> > > > > > > On Tue, 2020-09-29 at 17:41 +0530, Tejas Upadhyay wrote:
> > > > > > > > JSL has update in vswing table for eDP
> > > > > > > 
> > > > > > > Would be nice to mention in the commit description why PCH is being used, that would avoid Ville's question.
> > > > > > 
> > > > > > If the thing has nothing to do PCH then it should not use the PCH type
> > > > > > for the the check. Instead we should just do the EHL/JSL split.
> > > > > 
> > > > > In the first version Matt Roper suggested to use PCH to differentiate between EHL and JSL, Jani also agreed with this solution.This 2 PCHs can only be
> > > > > associate with EHL and JSL respectively, so no downsides here.
> > > > 
> > > > The downside is that the code makes no sense on the first glance.
> > > > It's going to generate a "wtf?" exception in the brain and require
> > > > me to take a second look to figure what is going on. Exception
> > > > handling is expensive and shouldn't be needed in cases where it's
> > > > trivial to make the code 100% obvious.
> > > 
> > > The bspec documents EHL and JSL as being the same platform and identical
> > > in all programming since they are literally the same display IP; this
> > > vswing table is the one and only place where the two are treated in a
> > > distinct manner for reasons that lie outside the display controller.  If
> > > you had to stop and take a closer look at the code here, that's a
> > > probably a good thing since in general there should generally never be a
> > > difference in the behavior between the two.  Adding an additional
> > > clarifying comment is probably in order too since this is a very
> > > exceptional special case.
> > > 
> > > If we deviate from the bspec's guidance and try to split IS_ELKHARTLAKE
> > > and IS_JASPERLAKE across the whole driver, that's going to be a lot more
> > > pain to maintain down the road since we'll almost certainly have cases
> > > where someone silently leaves one or the other off a condition and gets
> > > unexepcted behavior.  I could see arguments for using a SUBPLATFORM here
> > > like we do for TGL_U vs TGL_Y, but even that seems like overkill if we
> > > already have a clear way to distinguish the two cases (PCH pairing) and
> > > can just leave a clarifying comment.
> > 
> > That fixed PCH pairing is totally undocumented AFAICS. And vswing has
> > nothing to do with the south display, so the wtf will still happen.
> > Comment or no comment.
> 
> Oh and JSP does not show up in bspec even once. MCC appears exactly once
> where it talks about the differences between MCC and ICL-N PCH (which I
> guess is the same as JSP?).

No, ICL-N PCH is something different.  :-(  There were some early test
chips created that paired the EHL/JSL graphics/media/display IP with an
ICL PCH just for early debug/test purposes, but that pairing isn't
something that actually exists as a real platform.

I think the confusion here arises because most driver developers only
look at (or have access to) the bspec, which does not aim to document
end-user platforms, but rather IP families that the
graphics/media/display hardware IP teams design.  The bspec is not an
authoritative document for anything that lies outside the GMD IP itself.
The GMD architects do sometimes try to pull in additional information
from external teams/sources (such as PCH pairing or the electrical
details like the vswing programming here) to make life easier for the
software teams like us that only really deal with the bspec, but that
information comes from external sources, so it's a bit inconsistent in
terms of how much detail there is (or even whether it's described at
all).  We could probably file bspec defects to get them to include the
PCH pairing details for EHL/JSL in the bspec, but ultimately EHL="EHL
G/M/D + MCC PCH" and JSL="EHL G/M/D + JSP PCH;" this has already been
confirmed in an offline email thread with the hardware teams.

Elkhart Lake and Jasper Lake are two separate end-user platforms, that
both incorporated the same G/M/D IP design.  The name "Jasper Lake"
existed as a codename first, so that's the name that shows up in the
bspec; this wound up being a bit confusing when Elkhart Lake was
actually the first of the two to be released and thus wound up being the
name we have in our code.  But the situation seems similar to CHT vs BSW
which are both referred to as "CHV" in the bspec and in our code
(although obviously there was no PCH pairing for those SoCs).
Steppings, workarounds, etc. are unified for EHL/JSL because they're
literally the same IP, rather than one being a derivative of the other. 

If you want full details about the PCHs of a platform (most of which is
unimportant to graphics drivers) or the electrical characteristics that
feed into the vswing programming then there are other authoritative
documents that cover that (like the Electrical Design Specification and
such).  I'm not sure if those documents are posted anywhere publicly;
fortunately we only need a small amount of information in those areas
and the GMD architects are often nice enough to try to copy the relevant
info into the bspec as a courtesy.

> 
> Furthermore the spec never really talks about EHL except in very select
> places. So the IS_ELKHARTLAKE is already totally confusing and IMO more
 * > likely to cause maintenance problems than the split. Making it
> IS_JSL||IS_EHL at least gives the reader some hint as to where they
> should look in the spec.
> 
> Another argument why the split is fine is CFL/CML. Those are more
> or less exactly in the same boat as EHL. Ie. only really mentioned
> in the "configurations" section. Beyond that only KBL is ever really
> mentioned. And yet we have separate IS_FOOs for all of them, and
> apparently no one had any objections to that situation.
> 
> tldr;we have an established way to handle these things, so IMO lets
> just follow it and stop adding special cases.

Isn't CML graphics a derivative of CFL (rather than being exactly the
same IP)?  The fact that we have differences in the GMD IP itself that
need different workarounds implies that it's not quite the same
situation we're talking about here (otherwise we'd have been able to
just check the stepping revision ID).  IS_CML only got split out from
IS_CFL a couple months ago, so it's probably too soon to see how many
bugs eventually creep in when developers accidentally leave it off a CFL
condition or vice versa.

And we do still unify WHL, AML, etc., in i915 as far as I can see even
though the IP teams track those platforms separately, so the precedent
appears to be keeping them combined as far as I can see?


Matt

> 
> -- 
> Ville Syrjälä
> Intel
Ville Syrjälä Sept. 30, 2020, 10:38 a.m. UTC | #12
On Tue, Sep 29, 2020 at 04:38:22PM -0700, Matt Roper wrote:
> On Wed, Sep 30, 2020 at 12:59:58AM +0300, Ville Syrjälä wrote:
> > On Wed, Sep 30, 2020 at 12:11:48AM +0300, Ville Syrjälä wrote:
> > > On Tue, Sep 29, 2020 at 02:01:44PM -0700, Matt Roper wrote:
> > > > On Tue, Sep 29, 2020 at 11:30:22PM +0300, Ville Syrjälä wrote:
> > > > > On Tue, Sep 29, 2020 at 08:20:22PM +0000, Souza, Jose wrote:
> > > > > > On Tue, 2020-09-29 at 23:02 +0300, Ville Syrjälä wrote:
> > > > > > > On Tue, Sep 29, 2020 at 07:33:45PM +0000, Souza, Jose wrote:
> > > > > > > > On Tue, 2020-09-29 at 17:41 +0530, Tejas Upadhyay wrote:
> > > > > > > > > JSL has update in vswing table for eDP
> > > > > > > > 
> > > > > > > > Would be nice to mention in the commit description why PCH is being used, that would avoid Ville's question.
> > > > > > > 
> > > > > > > If the thing has nothing to do PCH then it should not use the PCH type
> > > > > > > for the the check. Instead we should just do the EHL/JSL split.
> > > > > > 
> > > > > > In the first version Matt Roper suggested to use PCH to differentiate between EHL and JSL, Jani also agreed with this solution.This 2 PCHs can only be
> > > > > > associate with EHL and JSL respectively, so no downsides here.
> > > > > 
> > > > > The downside is that the code makes no sense on the first glance.
> > > > > It's going to generate a "wtf?" exception in the brain and require
> > > > > me to take a second look to figure what is going on. Exception
> > > > > handling is expensive and shouldn't be needed in cases where it's
> > > > > trivial to make the code 100% obvious.
> > > > 
> > > > The bspec documents EHL and JSL as being the same platform and identical
> > > > in all programming since they are literally the same display IP; this
> > > > vswing table is the one and only place where the two are treated in a
> > > > distinct manner for reasons that lie outside the display controller.  If
> > > > you had to stop and take a closer look at the code here, that's a
> > > > probably a good thing since in general there should generally never be a
> > > > difference in the behavior between the two.  Adding an additional
> > > > clarifying comment is probably in order too since this is a very
> > > > exceptional special case.
> > > > 
> > > > If we deviate from the bspec's guidance and try to split IS_ELKHARTLAKE
> > > > and IS_JASPERLAKE across the whole driver, that's going to be a lot more
> > > > pain to maintain down the road since we'll almost certainly have cases
> > > > where someone silently leaves one or the other off a condition and gets
> > > > unexepcted behavior.  I could see arguments for using a SUBPLATFORM here
> > > > like we do for TGL_U vs TGL_Y, but even that seems like overkill if we
> > > > already have a clear way to distinguish the two cases (PCH pairing) and
> > > > can just leave a clarifying comment.
> > > 
> > > That fixed PCH pairing is totally undocumented AFAICS. And vswing has
> > > nothing to do with the south display, so the wtf will still happen.
> > > Comment or no comment.
> > 
> > Oh and JSP does not show up in bspec even once. MCC appears exactly once
> > where it talks about the differences between MCC and ICL-N PCH (which I
> > guess is the same as JSP?).
> 
> No, ICL-N PCH is something different.  :-(  There were some early test
> chips created that paired the EHL/JSL graphics/media/display IP with an
> ICL PCH just for early debug/test purposes, but that pairing isn't
> something that actually exists as a real platform.
> 
> I think the confusion here arises because most driver developers only
> look at (or have access to) the bspec, which does not aim to document
> end-user platforms, but rather IP families that the
> graphics/media/display hardware IP teams design.  The bspec is not an
> authoritative document for anything that lies outside the GMD IP itself.
> The GMD architects do sometimes try to pull in additional information
> from external teams/sources (such as PCH pairing or the electrical
> details like the vswing programming here) to make life easier for the
> software teams like us that only really deal with the bspec, but that
> information comes from external sources, so it's a bit inconsistent in
> terms of how much detail there is (or even whether it's described at
> all).  We could probably file bspec defects to get them to include the
> PCH pairing details for EHL/JSL in the bspec, but ultimately EHL="EHL
> G/M/D + MCC PCH" and JSL="EHL G/M/D + JSP PCH;" this has already been
> confirmed in an offline email thread with the hardware teams.
> 
> Elkhart Lake and Jasper Lake are two separate end-user platforms, that
> both incorporated the same G/M/D IP design.  The name "Jasper Lake"
> existed as a codename first, so that's the name that shows up in the
> bspec; this wound up being a bit confusing when Elkhart Lake was
> actually the first of the two to be released and thus wound up being the
> name we have in our code.  But the situation seems similar to CHT vs BSW
> which are both referred to as "CHV" in the bspec and in our code
> (although obviously there was no PCH pairing for those SoCs).
> Steppings, workarounds, etc. are unified for EHL/JSL because they're
> literally the same IP, rather than one being a derivative of the other. 
> 
> If you want full details about the PCHs of a platform (most of which is
> unimportant to graphics drivers) or the electrical characteristics that
> feed into the vswing programming then there are other authoritative
> documents that cover that (like the Electrical Design Specification and
> such).  I'm not sure if those documents are posted anywhere publicly;
> fortunately we only need a small amount of information in those areas
> and the GMD architects are often nice enough to try to copy the relevant
> info into the bspec as a courtesy.
> 
> > 
> > Furthermore the spec never really talks about EHL except in very select
> > places. So the IS_ELKHARTLAKE is already totally confusing and IMO more
>  * > likely to cause maintenance problems than the split. Making it
> > IS_JSL||IS_EHL at least gives the reader some hint as to where they
> > should look in the spec.
> > 
> > Another argument why the split is fine is CFL/CML. Those are more
> > or less exactly in the same boat as EHL. Ie. only really mentioned
> > in the "configurations" section. Beyond that only KBL is ever really
> > mentioned. And yet we have separate IS_FOOs for all of them, and
> > apparently no one had any objections to that situation.
> > 
> > tldr;we have an established way to handle these things, so IMO lets
> > just follow it and stop adding special cases.
> 
> Isn't CML graphics a derivative of CFL (rather than being exactly the
> same IP)?

No idea, and I don't really see why it would matter anyway
from a driver developer's POV.

> The fact that we have differences in the GMD IP itself that
> need different workarounds implies that it's not quite the same
> situation we're talking about here (otherwise we'd have been able to
> just check the stepping revision ID).  IS_CML only got split out from
> IS_CFL a couple months ago, so it's probably too soon to see how many
> bugs eventually creep in when developers accidentally leave it off a CFL
> condition or vice versa.
> 
> And we do still unify WHL, AML, etc., in i915 as far as I can see even
> though the IP teams track those platforms separately, so the precedent
> appears to be keeping them combined as far as I can see?

We split when there is any functional difference. Eg. CML was
considered the same as CFL until an actual difference came up,
at which point it was split up.

Now we have an actual difference between EHL and JSL so we
should split. Granted it's a bit annoying to have to do it
just for some vswing tables. Ideally that stuff would be
specified in a sane way by the VBT. But since VBT is generally
useless we need to deal with this on a platform level.
Jani Nikula Sept. 30, 2020, 12:31 p.m. UTC | #13
On Tue, 29 Sep 2020, "Souza, Jose" <jose.souza@intel.com> wrote:
> On Tue, 2020-09-29 at 23:02 +0300, Ville Syrjälä wrote:
>> If the thing has nothing to do PCH then it should not use the PCH type
>> for the the check. Instead we should just do the EHL/JSL split.
>
> In the first version Matt Roper suggested to use PCH to differentiate
> between EHL and JSL, Jani also agreed with this solution.This 2 PCHs
> can only be associate with EHL and JSL respectively, so no downsides
> here.

FWIW I said, "If the difference is in the PCH", without pondering
further.

BR,
Jani.
Jani Nikula Sept. 30, 2020, 12:57 p.m. UTC | #14
On Wed, 30 Sep 2020, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> Now we have an actual difference between EHL and JSL so we
> should split. Granted it's a bit annoying to have to do it
> just for some vswing tables. Ideally that stuff would be
> specified in a sane way by the VBT. But since VBT is generally
> useless we need to deal with this on a platform level.

Just to recap, we have three basic approaches for differentiating
platforms based on PCI ID:

- Separate platforms, each with their own device info and enum
  intel_platform, using IS_<PLATFORM>() for checks.

- Same platform, with subplatforms, using IS_SUBPLATFORM() for
  checks. Generally only used for the ULT/ULX checks, but there's also
  the CNL/ICL port F case which is perhaps comparable.

- Same platform, each with their own device info, and a feature flag.

(In this case, checking the PCH is a proxy; there is no actual
difference in the PCHs to account for the different values to be
used. Mixing PCHs with the platforms would lead to problems.)

We've been told JSL and EHL are the same, which would argue for keeping
them INTEL_ELKHARTLAKE. We've done this with other platforms that are
identical. However, now it looks like they're not the same... why not if
they're supposed to be identical? What else is there?

BR,
Jani.
Matt Roper Sept. 30, 2020, 6:20 p.m. UTC | #15
On Wed, Sep 30, 2020 at 03:57:58PM +0300, Jani Nikula wrote:
> On Wed, 30 Sep 2020, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > Now we have an actual difference between EHL and JSL so we
> > should split. Granted it's a bit annoying to have to do it
> > just for some vswing tables. Ideally that stuff would be
> > specified in a sane way by the VBT. But since VBT is generally
> > useless we need to deal with this on a platform level.
> 
> Just to recap, we have three basic approaches for differentiating
> platforms based on PCI ID:
> 
> - Separate platforms, each with their own device info and enum
>   intel_platform, using IS_<PLATFORM>() for checks.
> 
> - Same platform, with subplatforms, using IS_SUBPLATFORM() for
>   checks. Generally only used for the ULT/ULX checks, but there's also
>   the CNL/ICL port F case which is perhaps comparable.
> 
> - Same platform, each with their own device info, and a feature flag.
> 
> (In this case, checking the PCH is a proxy; there is no actual
> difference in the PCHs to account for the different values to be
> used. Mixing PCHs with the platforms would lead to problems.)
> 
> We've been told JSL and EHL are the same, which would argue for keeping
> them INTEL_ELKHARTLAKE. We've done this with other platforms that are
> identical. However, now it looks like they're not the same... why not if
> they're supposed to be identical? What else is there?

My understanding is that they are identical, but the design guidelines
for the *motherboards* that they will plug into are different, which
necessitates different electrical tuning values to guarantee clean
display signals.  Ville's right that it would be nice if this kind of
stuff was just available from something like the VBT instead of being
hardcoded into the driver, but sadly that's just not the case today.

So yes, none of this is related to the South Display which is the only
place we usually care about the PCH in the graphics driver.  But PCH is
correlated with board type, which is why I suggested matching on the PCH
in the first place.

If we really want to split these two platforms then I'd suggest we add a
new macro like

        #define IS_EHL_JSL(i915) ( \
                IS_PLATFORM(dev_priv, INTEL_ELKHARTLAKE) || \
                IS_PLATFORM(dev_priv, INTEL_JASPERLAKE))

and use that everywhere else in the driver.  For the vswing code itself
we'd just do a direct IS_PLATFORM() check with just one platform or the
other provided; no need to add IS_ELKHARTLAKE/IS_JASPERLAKE macros in
that case since it would be a bug to differentiate between the two
anywhere else in the driver.


Matt

> 
> BR,
> Jani.
> 
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
index 4d06178cd76c..e6e93d01d0ce 100644
--- a/drivers/gpu/drm/i915/display/intel_ddi.c
+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
@@ -582,6 +582,34 @@  static const struct cnl_ddi_buf_trans ehl_combo_phy_ddi_translations_dp[] = {
 	{ 0x6, 0x7F, 0x3F, 0x00, 0x00 },	/* 900   900      0.0   */
 };
 
+static const struct cnl_ddi_buf_trans jsl_combo_phy_ddi_translations_edp_hbr[] = {
+						/* NT mV Trans mV db    */
+	{ 0x8, 0x7F, 0x3F, 0x00, 0x00 },	/* 200   200      0.0   */
+	{ 0x8, 0x7F, 0x38, 0x00, 0x07 },	/* 200   250      1.9   */
+	{ 0x1, 0x7F, 0x33, 0x00, 0x0C },	/* 200   300      3.5   */
+	{ 0xA, 0x35, 0x36, 0x00, 0x09 },	/* 200   350      4.9   */
+	{ 0x8, 0x7F, 0x3F, 0x00, 0x00 },	/* 250   250      0.0   */
+	{ 0x1, 0x7F, 0x38, 0x00, 0x07 },	/* 250   300      1.6   */
+	{ 0xA, 0x35, 0x35, 0x00, 0x0A },	/* 250   350      2.9   */
+	{ 0x1, 0x7F, 0x3F, 0x00, 0x00 },	/* 300   300      0.0   */
+	{ 0xA, 0x35, 0x38, 0x00, 0x07 },	/* 300   350      1.3   */
+	{ 0xA, 0x35, 0x3F, 0x00, 0x00 },	/* 350   350      0.0   */
+};
+
+static const struct cnl_ddi_buf_trans jsl_combo_phy_ddi_translations_edp_hbr2[] = {
+						/* NT mV Trans mV db    */
+	{ 0x8, 0x7F, 0x3F, 0x00, 0x00 },	/* 200   200      0.0   */
+	{ 0x8, 0x7F, 0x3F, 0x00, 0x00 },	/* 200   250      1.9   */
+	{ 0x1, 0x7F, 0x3D, 0x00, 0x02 },	/* 200   300      3.5   */
+	{ 0xA, 0x35, 0x38, 0x00, 0x07 },	/* 200   350      4.9   */
+	{ 0x8, 0x7F, 0x3F, 0x00, 0x00 },	/* 250   250      0.0   */
+	{ 0x1, 0x7F, 0x3F, 0x00, 0x00 },	/* 250   300      1.6   */
+	{ 0xA, 0x35, 0x3A, 0x00, 0x05 },	/* 250   350      2.9   */
+	{ 0x1, 0x7F, 0x3F, 0x00, 0x00 },	/* 300   300      0.0   */
+	{ 0xA, 0x35, 0x38, 0x00, 0x07 },	/* 300   350      1.3   */
+	{ 0xA, 0x35, 0x3F, 0x00, 0x00 },	/* 350   350      0.0   */
+};
+
 struct icl_mg_phy_ddi_buf_trans {
 	u32 cri_txdeemph_override_11_6;
 	u32 cri_txdeemph_override_5_0;
@@ -1069,7 +1097,6 @@  icl_get_mg_buf_trans(struct intel_encoder *encoder, int type, int rate,
 	*n_entries = ARRAY_SIZE(icl_mg_phy_ddi_translations_rbr_hbr);
 	return icl_mg_phy_ddi_translations_rbr_hbr;
 }
-
 static const struct cnl_ddi_buf_trans *
 ehl_get_combo_buf_trans(struct intel_encoder *encoder, int type, int rate,
 			int *n_entries)
@@ -1098,6 +1125,34 @@  ehl_get_combo_buf_trans(struct intel_encoder *encoder, int type, int rate,
 	}
 }
 
+static const struct cnl_ddi_buf_trans *
+jsl_get_combo_buf_trans(struct intel_encoder *encoder, int type, int rate,
+			int *n_entries)
+{
+	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
+
+	switch (type) {
+	case INTEL_OUTPUT_HDMI:
+		*n_entries = ARRAY_SIZE(icl_combo_phy_ddi_translations_hdmi);
+		return icl_combo_phy_ddi_translations_hdmi;
+	case INTEL_OUTPUT_EDP:
+		if (dev_priv->vbt.edp.low_vswing) {
+			if (rate > 270000) {
+				*n_entries = ARRAY_SIZE(jsl_combo_phy_ddi_translations_edp_hbr2);
+				return jsl_combo_phy_ddi_translations_edp_hbr2;
+			} else {
+				*n_entries = ARRAY_SIZE(jsl_combo_phy_ddi_translations_edp_hbr);
+				return jsl_combo_phy_ddi_translations_edp_hbr;
+			}
+		}
+		/* fall through */
+	default:
+		/* All combo DP and eDP ports that do not support low_vswing */
+		*n_entries = ARRAY_SIZE(icl_combo_phy_ddi_translations_dp_hbr2);
+		return icl_combo_phy_ddi_translations_dp_hbr2;
+	}
+}
+
 static const struct cnl_ddi_buf_trans *
 tgl_get_combo_buf_trans(struct intel_encoder *encoder, int type, int rate,
 			int *n_entries)
@@ -2265,7 +2320,10 @@  static u8 intel_ddi_dp_voltage_max(struct intel_dp *intel_dp)
 			tgl_get_dkl_buf_trans(encoder, encoder->type,
 					      intel_dp->link_rate, &n_entries);
 	} else if (INTEL_GEN(dev_priv) == 11) {
-		if (IS_ELKHARTLAKE(dev_priv))
+		if (HAS_PCH_JSP(dev_priv))
+			jsl_get_combo_buf_trans(encoder, encoder->type,
+						intel_dp->link_rate, &n_entries);
+		else if (HAS_PCH_MCC(dev_priv))
 			ehl_get_combo_buf_trans(encoder, encoder->type,
 						intel_dp->link_rate, &n_entries);
 		else if (intel_phy_is_combo(dev_priv, phy))
@@ -2454,7 +2512,10 @@  static void icl_ddi_combo_vswing_program(struct intel_encoder *encoder,
 	if (INTEL_GEN(dev_priv) >= 12)
 		ddi_translations = tgl_get_combo_buf_trans(encoder, type, rate,
 							   &n_entries);
-	else if (IS_ELKHARTLAKE(dev_priv))
+	else if (HAS_PCH_JSP(dev_priv))
+		ddi_translations = jsl_get_combo_buf_trans(encoder, type, rate,
+							   &n_entries);
+	else if (HAS_PCH_MCC(dev_priv))
 		ddi_translations = ehl_get_combo_buf_trans(encoder, type, rate,
 							   &n_entries);
 	else