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(-)
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
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);@@ -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
-----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);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
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);@@ -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
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);@@ -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
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);@@ -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
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.
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.
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
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.
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.
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
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
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.
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.
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
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.
dri-devel@lists.freedesktop.org
-
Jani Nikula -
Matt Roper -
Souza, Jose -
Surendrakumar Upadhyay, TejaskumarX -
Tejas Upadhyay
-
Ville Syrjälä