[PATCH] drm/i915/dp: Cable type identification for DP2.1

[PATCH] drm/i915/dp: Cable type identification for DP2.1

[Animesh Manna]

For DP alt mode display driver get the information
about cable speed and cable type through TCSS_DDI_STATUS
register which will be updated by type-c platform driver.
Accodingly Update dpcd 0x110 with cable information before
link training start. This change came part of DP2.1 SCR.

Note: This patch is not tested due to unavailability of
cable. Sending as RFC for design review.

Signed-off-by: Animesh Manna <animesh.manna@intel.com>
---
 drivers/gpu/drm/i915/display/intel_ddi.c | 57 ++++++++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_tc.c  | 10 +++++
 drivers/gpu/drm/i915/display/intel_tc.h  |  1 +
 drivers/gpu/drm/i915/i915_reg.h          |  5 +++
 include/drm/display/drm_dp.h             |  9 ++++
 5 files changed, 82 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
index 70d44edd8c6e..3a0f6a3c9f98 100644
--- a/drivers/gpu/drm/i915/display/intel_ddi.c
+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
@@ -2208,6 +2208,55 @@ static void intel_dp_sink_set_msa_timing_par_ignore_state(struct intel_dp *intel
 			    str_enable_disable(enable));
 }
 
+#define CABLE_SPEED_SHIFT 4
+
+enum dp_cable_speed {
+	DP_CABLE_HBR3 = 1,
+	DP_CABLE_UHBR10,
+	DP_CABLE_GEN3_UHBR20,
+	DP_CABLE_GEN4_UHBR20
+};
+
+static void intel_dp_set_cable_attributes(struct intel_dp *intel_dp,
+					  u8 cable_attributes)
+{
+	u8 cable_speed;
+	bool active_cable, retimer;
+	u8 cable_attr_dpcd;
+
+	cable_speed = cable_attributes >> CABLE_SPEED_SHIFT;
+
+	switch (cable_speed) {
+	case DP_CABLE_HBR3:
+		cable_attr_dpcd = 0;
+		break;
+	case DP_CABLE_UHBR10:
+		cable_attr_dpcd = 1;
+		break;
+	case DP_CABLE_GEN3_UHBR20:
+	case DP_CABLE_GEN4_UHBR20:
+		cable_attr_dpcd = 2;
+		break;
+	default:
+		cable_attr_dpcd = 0;
+		break;
+	}
+
+	active_cable = (cable_attributes << TCSS_DDI_STATUS_CABLE_ATTR_SHIFT) &
+		       TCSS_DDI_STATUS_ACTIVE_CABLE;
+	retimer = (cable_attributes << TCSS_DDI_STATUS_CABLE_ATTR_SHIFT) &
+		  TCSS_DDI_STATUS_RETIMER_REDRIVER;
+	if (retimer && active_cable)
+		cable_attr_dpcd |= DP_CABLE_TYPE_RETIMER_ACTIVE;
+	else if (active_cable)
+		cable_attr_dpcd |= DP_CABLE_TYPE_LRD_ACTIVE;
+	else
+		cable_attr_dpcd |= DP_CABLE_TYPE_PASSIVE;
+
+	drm_dp_dpcd_writeb(&intel_dp->aux, DP_CABLE_ATTRIBUTES_UPDATED_BY_TX,
+			   cable_attr_dpcd);
+}
+
 static void intel_dp_sink_set_fec_ready(struct intel_dp *intel_dp,
 					const struct intel_crtc_state *crtc_state)
 {
@@ -2414,6 +2463,7 @@ static void mtl_ddi_pre_enable_dp(struct intel_atomic_state *state,
 {
 	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
 	bool is_mst = intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST);
+	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
 
 	intel_dp_set_link_params(intel_dp,
 				 crtc_state->port_clock,
@@ -2480,6 +2530,13 @@ static void mtl_ddi_pre_enable_dp(struct intel_atomic_state *state,
 	intel_dp_check_frl_training(intel_dp);
 	intel_dp_pcon_dsc_configure(intel_dp, crtc_state);
 
+	if (intel_tc_port_in_dp_alt_mode(dig_port)) {
+		u8 cable_attributes;
+
+		cable_attributes = intel_tc_get_cable_attributes(dig_port);
+		intel_dp_set_cable_attributes(intel_dp, cable_attributes);
+	}
+
 	/*
 	 * 6. The rest of the below are substeps under the bspec's "Enable and
 	 * Train Display Port" step.  Note that steps that are specific to
diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c
index 3ebf41859043..6b10a8839563 100644
--- a/drivers/gpu/drm/i915/display/intel_tc.c
+++ b/drivers/gpu/drm/i915/display/intel_tc.c
@@ -260,6 +260,16 @@ assert_tc_port_power_enabled(struct intel_tc_port *tc)
 		    !intel_display_power_is_enabled(i915, tc_port_power_domain(tc)));
 }
 
+u8 intel_tc_get_cable_attributes(struct intel_digital_port *dig_port)
+{
+	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
+	enum tc_port tc_port = intel_port_to_tc(i915, dig_port->base.port);
+
+	return (intel_de_read(i915, TCSS_DDI_STATUS(tc_port)) &
+		TCSS_DDI_STATUS_CABLE_ATTR_MASK) >>
+		TCSS_DDI_STATUS_CABLE_ATTR_SHIFT;
+}
+
 u32 intel_tc_port_get_lane_mask(struct intel_digital_port *dig_port)
 {
 	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
diff --git a/drivers/gpu/drm/i915/display/intel_tc.h b/drivers/gpu/drm/i915/display/intel_tc.h
index 3b16491925fa..edafe92844b4 100644
--- a/drivers/gpu/drm/i915/display/intel_tc.h
+++ b/drivers/gpu/drm/i915/display/intel_tc.h
@@ -43,5 +43,6 @@ int intel_tc_port_init(struct intel_digital_port *dig_port, bool is_legacy);
 void intel_tc_port_cleanup(struct intel_digital_port *dig_port);
 
 bool intel_tc_cold_requires_aux_pw(struct intel_digital_port *dig_port);
+u8 intel_tc_get_cable_attributes(struct intel_digital_port *dig_port);
 
 #endif /* __INTEL_TC_H__ */
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 0523418129c5..991ecf082b5c 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -6576,6 +6576,11 @@ enum skl_power_gate {
 #define TCSS_DDI_STATUS(tc)			_MMIO(_PICK_EVEN(tc, \
 								 _TCSS_DDI_STATUS_1, \
 								 _TCSS_DDI_STATUS_2))
+#define  TCSS_DDI_STATUS_CABLE_ATTR_SHIFT	9
+#define  TCSS_DDI_STATUS_CABLE_ATTR_MASK	REG_GENMASK(14, 9)
+#define  TCSS_DDI_STATUS_ACTIVE_CABLE		REG_BIT(11)
+#define  TCSS_DDI_STATUS_CABLE_TYPE		REG_BIT(10)
+#define  TCSS_DDI_STATUS_RETIMER_REDRIVER	REG_BIT(9)
 #define  TCSS_DDI_STATUS_READY			REG_BIT(2)
 #define  TCSS_DDI_STATUS_HPD_LIVE_STATUS_TBT	REG_BIT(1)
 #define  TCSS_DDI_STATUS_HPD_LIVE_STATUS_ALT	REG_BIT(0)
diff --git a/include/drm/display/drm_dp.h b/include/drm/display/drm_dp.h
index b046f79f4744..dde715d567c2 100644
--- a/include/drm/display/drm_dp.h
+++ b/include/drm/display/drm_dp.h
@@ -654,6 +654,13 @@
 # define DP_LANE13_POST_CURSOR2_SET_MASK    (3 << 4)
 # define DP_LANE13_MAX_POST_CURSOR2_REACHED (1 << 6)
 
+#define DP_CABLE_ATTRIBUTES_UPDATED_BY_TX   0x110
+# define DP_CABLE_TYPE_MASK		    (0x7 << 3)
+# define DP_CABLE_TYPE_UNKNOWN		    (0x0 << 3)
+# define DP_CABLE_TYPE_PASSIVE		    (0x1 << 3)
+# define DP_CABLE_TYPE_LRD_ACTIVE	    (0x2 << 3)
+# define DP_CABLE_TYPE_RETIMER_ACTIVE	    (0x3 << 3)
+
 #define DP_MSTM_CTRL			    0x111   /* 1.2 */
 # define DP_MST_EN			    (1 << 0)
 # define DP_UP_REQ_EN			    (1 << 1)
@@ -1139,6 +1146,8 @@
 # define DP_128B132B_TRAINING_AUX_RD_INTERVAL_32_MS             0x05
 # define DP_128B132B_TRAINING_AUX_RD_INTERVAL_64_MS             0x06
 
+#define DP_CABLE_ATTRIBUTES_UPDATED_BY_RX               0x2217 /* 2.1 */
+
 #define DP_TEST_264BIT_CUSTOM_PATTERN_7_0		0x2230
 #define DP_TEST_264BIT_CUSTOM_PATTERN_263_256	0x2250
 
-- 
2.29.0

Re: [PATCH] drm/i915/dp: Cable type identification for DP2.1

[Jani Nikula]

On Fri, 09 Jun 2023, Animesh Manna <animesh.manna@intel.com> wrote:
> For DP alt mode display driver get the information
> about cable speed and cable type through TCSS_DDI_STATUS
> register which will be updated by type-c platform driver.
> Accodingly Update dpcd 0x110 with cable information before
> link training start. This change came part of DP2.1 SCR.

No need to refer to the SCR anymore, as DP 2.1 is out.

There are a bunch of detailed comments inline.

High level, this should probably be done much earlier. See Table 5-21 in
DP 2.1. We need to read DPCD 0x2217 before writing 0x110. The DPRX
updates 0x2217 before asserting hotplug, so we should probably read it
at detect where we read all other DPCD too.

How early is TCSS_DDI_STATUS available, should we read that at hotplug
too? For USB-C we should write to DPCD 0x110 the least common
denominator between DPCD 0x2217 and 0x110.

Another question which I didn't find an answer to yet, does writing
0x110 impact what the RPRX reports for capabilities i.e. can we proceed
with link training normally from there, *or* should we limit the
sink_rates/common_rates based on TCSS_DDI_STATUS and DPCD 0x2217
i.e. filter out UHBR as needed.

Please read bspec and DP 2.1 further to find answers.

>
> Note: This patch is not tested due to unavailability of
> cable. Sending as RFC for design review.
>
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_ddi.c | 57 ++++++++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_tc.c  | 10 +++++
>  drivers/gpu/drm/i915/display/intel_tc.h  |  1 +
>  drivers/gpu/drm/i915/i915_reg.h          |  5 +++
>  include/drm/display/drm_dp.h             |  9 ++++
>  5 files changed, 82 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> index 70d44edd8c6e..3a0f6a3c9f98 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -2208,6 +2208,55 @@ static void intel_dp_sink_set_msa_timing_par_ignore_state(struct intel_dp *intel
>  			    str_enable_disable(enable));
>  }
>  
> +#define CABLE_SPEED_SHIFT 4
> +
> +enum dp_cable_speed {
> +	DP_CABLE_HBR3 = 1,
> +	DP_CABLE_UHBR10,
> +	DP_CABLE_GEN3_UHBR20,
> +	DP_CABLE_GEN4_UHBR20
> +};
> +
> +static void intel_dp_set_cable_attributes(struct intel_dp *intel_dp,
> +					  u8 cable_attributes)

There are two "domains" for the cable information, the hardware register
and the DPCD register. However, cable_attributes is neither, but also
not helpful, which makes this function cumbersome.

Usually in cases like this, you'd pick one or the other, *or* if you
want to have a generic middle ground, you'd make it helpful and easy to
use and understand (e.g. a struct).

In this case, I'd just pick the DPCD as the format, because it's
platform independent and the whole thing is simple enough.

So this function would really reduce down to a single DPCD write.

> +{
> +	u8 cable_speed;
> +	bool active_cable, retimer;
> +	u8 cable_attr_dpcd;
> +
> +	cable_speed = cable_attributes >> CABLE_SPEED_SHIFT;
> +
> +	switch (cable_speed) {
> +	case DP_CABLE_HBR3:
> +		cable_attr_dpcd = 0;
> +		break;
> +	case DP_CABLE_UHBR10:
> +		cable_attr_dpcd = 1;
> +		break;
> +	case DP_CABLE_GEN3_UHBR20:
> +	case DP_CABLE_GEN4_UHBR20:
> +		cable_attr_dpcd = 2;
> +		break;
> +	default:
> +		cable_attr_dpcd = 0;
> +		break;
> +	}
> +
> +	active_cable = (cable_attributes << TCSS_DDI_STATUS_CABLE_ATTR_SHIFT) &
> +		       TCSS_DDI_STATUS_ACTIVE_CABLE;
> +	retimer = (cable_attributes << TCSS_DDI_STATUS_CABLE_ATTR_SHIFT) &
> +		  TCSS_DDI_STATUS_RETIMER_REDRIVER;
> +	if (retimer && active_cable)
> +		cable_attr_dpcd |= DP_CABLE_TYPE_RETIMER_ACTIVE;
> +	else if (active_cable)
> +		cable_attr_dpcd |= DP_CABLE_TYPE_LRD_ACTIVE;
> +	else
> +		cable_attr_dpcd |= DP_CABLE_TYPE_PASSIVE;
> +
> +	drm_dp_dpcd_writeb(&intel_dp->aux, DP_CABLE_ATTRIBUTES_UPDATED_BY_TX,
> +			   cable_attr_dpcd);
> +}
> +
>  static void intel_dp_sink_set_fec_ready(struct intel_dp *intel_dp,
>  					const struct intel_crtc_state *crtc_state)
>  {
> @@ -2414,6 +2463,7 @@ static void mtl_ddi_pre_enable_dp(struct intel_atomic_state *state,
>  {
>  	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
>  	bool is_mst = intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST);
> +	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>  
>  	intel_dp_set_link_params(intel_dp,
>  				 crtc_state->port_clock,
> @@ -2480,6 +2530,13 @@ static void mtl_ddi_pre_enable_dp(struct intel_atomic_state *state,
>  	intel_dp_check_frl_training(intel_dp);
>  	intel_dp_pcon_dsc_configure(intel_dp, crtc_state);
>  
> +	if (intel_tc_port_in_dp_alt_mode(dig_port)) {
> +		u8 cable_attributes;
> +
> +		cable_attributes = intel_tc_get_cable_attributes(dig_port);
> +		intel_dp_set_cable_attributes(intel_dp, cable_attributes);
> +	}
> +
>  	/*
>  	 * 6. The rest of the below are substeps under the bspec's "Enable and
>  	 * Train Display Port" step.  Note that steps that are specific to
> diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c
> index 3ebf41859043..6b10a8839563 100644
> --- a/drivers/gpu/drm/i915/display/intel_tc.c
> +++ b/drivers/gpu/drm/i915/display/intel_tc.c
> @@ -260,6 +260,16 @@ assert_tc_port_power_enabled(struct intel_tc_port *tc)
>  		    !intel_display_power_is_enabled(i915, tc_port_power_domain(tc)));
>  }
>  
> +u8 intel_tc_get_cable_attributes(struct intel_digital_port *dig_port)
> +{
> +	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> +	enum tc_port tc_port = intel_port_to_tc(i915, dig_port->base.port);

So I think this function should return the information in DPCD 0x110
format.

Read the register, convert to DPCD format, return. Make this the single
point of conversion between the two, and don't pass intermediate info
around.

Whoever calls this should then have DPCD 0x2217 and the info returned by
this function, and find the least common denominator, and update 0x110
accordingly. And *maybe* also update sink_rates/common_rates
accordingly.

> +
> +	return (intel_de_read(i915, TCSS_DDI_STATUS(tc_port)) &
> +		TCSS_DDI_STATUS_CABLE_ATTR_MASK) >>
> +		TCSS_DDI_STATUS_CABLE_ATTR_SHIFT;
> +}
> +
>  u32 intel_tc_port_get_lane_mask(struct intel_digital_port *dig_port)
>  {
>  	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> diff --git a/drivers/gpu/drm/i915/display/intel_tc.h b/drivers/gpu/drm/i915/display/intel_tc.h
> index 3b16491925fa..edafe92844b4 100644
> --- a/drivers/gpu/drm/i915/display/intel_tc.h
> +++ b/drivers/gpu/drm/i915/display/intel_tc.h
> @@ -43,5 +43,6 @@ int intel_tc_port_init(struct intel_digital_port *dig_port, bool is_legacy);
>  void intel_tc_port_cleanup(struct intel_digital_port *dig_port);
>  
>  bool intel_tc_cold_requires_aux_pw(struct intel_digital_port *dig_port);
> +u8 intel_tc_get_cable_attributes(struct intel_digital_port *dig_port);
>  
>  #endif /* __INTEL_TC_H__ */
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 0523418129c5..991ecf082b5c 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6576,6 +6576,11 @@ enum skl_power_gate {
>  #define TCSS_DDI_STATUS(tc)			_MMIO(_PICK_EVEN(tc, \
>  								 _TCSS_DDI_STATUS_1, \
>  								 _TCSS_DDI_STATUS_2))
> +#define  TCSS_DDI_STATUS_CABLE_ATTR_SHIFT	9
> +#define  TCSS_DDI_STATUS_CABLE_ATTR_MASK	REG_GENMASK(14, 9)

This "cable attr" thing defines something that I think should not be
used, a field in a register where you can't even use the other defines
to parse. Please remove it, and replace with mask and values for
CABLE_SPEED.

This reflects the comment on cable_attributes parameter in
intel_dp_set_cable_attributes().

> +#define  TCSS_DDI_STATUS_ACTIVE_CABLE		REG_BIT(11)
> +#define  TCSS_DDI_STATUS_CABLE_TYPE		REG_BIT(10)
> +#define  TCSS_DDI_STATUS_RETIMER_REDRIVER	REG_BIT(9)

Usually I promote following the spec for macro naming, but the above two
are silly.

I think the options are:

1) just define them for what they are:

#define  TCSS_DDI_STATUS_CABLE_TYPE_OPTICAL	REG_BIT(10)
#define  TCSS_DDI_STATUS_RETIMER		REG_BIT(9)

2) consider them reg fields:

#define  TCSS_DDI_STATUS_CABLE_TYPE		REG_GENMASK(10, 10)
#define  TCSS_DDI_STATUS_CABLE_TYPE_ELECTRICAL	REG_FIELD_PREP(TCSS_DDI_STATUS_CABLE_TYPE, 0)
#define  TCSS_DDI_STATUS_CABLE_TYPE_OPTICAL	REG_FIELD_PREP(TCSS_DDI_STATUS_CABLE_TYPE, 1)

#define  TCSS_DDI_STATUS_RETIMER_REDRIVER	REG_GENMASK(9, 9)
#define  TCSS_DDI_STATUS_REDRIVER		REG_FIELD_PREP(TCSS_DDI_STATUS_RETIMER_REDRIVER, 0)
#define  TCSS_DDI_STATUS_RETIMER		REG_FIELD_PREP(TCSS_DDI_STATUS_RETIMER_REDRIVER, 1)

I think the latter is just too verbose, so I'd go for 1).

>  #define  TCSS_DDI_STATUS_READY			REG_BIT(2)
>  #define  TCSS_DDI_STATUS_HPD_LIVE_STATUS_TBT	REG_BIT(1)
>  #define  TCSS_DDI_STATUS_HPD_LIVE_STATUS_ALT	REG_BIT(0)
> diff --git a/include/drm/display/drm_dp.h b/include/drm/display/drm_dp.h
> index b046f79f4744..dde715d567c2 100644
> --- a/include/drm/display/drm_dp.h
> +++ b/include/drm/display/drm_dp.h
> @@ -654,6 +654,13 @@
>  # define DP_LANE13_POST_CURSOR2_SET_MASK    (3 << 4)
>  # define DP_LANE13_MAX_POST_CURSOR2_REACHED (1 << 6)
>  
> +#define DP_CABLE_ATTRIBUTES_UPDATED_BY_TX   0x110

Please use _DPTX suffix like in the spec.

/* 2.1 */ missing at the end.

The UHBR capabilities bits should be defined here.

> +# define DP_CABLE_TYPE_MASK		    (0x7 << 3)
> +# define DP_CABLE_TYPE_UNKNOWN		    (0x0 << 3)
> +# define DP_CABLE_TYPE_PASSIVE		    (0x1 << 3)
> +# define DP_CABLE_TYPE_LRD_ACTIVE	    (0x2 << 3)
> +# define DP_CABLE_TYPE_RETIMER_ACTIVE	    (0x3 << 3)

The values could just be decimal instead of hex.

> +
>  #define DP_MSTM_CTRL			    0x111   /* 1.2 */
>  # define DP_MST_EN			    (1 << 0)
>  # define DP_UP_REQ_EN			    (1 << 1)
> @@ -1139,6 +1146,8 @@
>  # define DP_128B132B_TRAINING_AUX_RD_INTERVAL_32_MS             0x05
>  # define DP_128B132B_TRAINING_AUX_RD_INTERVAL_64_MS             0x06
>  
> +#define DP_CABLE_ATTRIBUTES_UPDATED_BY_RX               0x2217 /* 2.1 */

Please use _DPRX suffix like in the spec.

> +
>  #define DP_TEST_264BIT_CUSTOM_PATTERN_7_0		0x2230
>  #define DP_TEST_264BIT_CUSTOM_PATTERN_263_256	0x2250

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-gfx] [PATCH] drm/i915/dp: Cable type identification for DP2.1

[Almahallawy, Khaled]

On Fri, 2023-06-09 at 11:35 +0300, Jani Nikula wrote:
> On Fri, 09 Jun 2023, Animesh Manna <animesh.manna@intel.com> wrote:
> > For DP alt mode display driver get the information
> > about cable speed and cable type through TCSS_DDI_STATUS
> > register which will be updated by type-c platform driver.
> > Accodingly Update dpcd 0x110 with cable information before
> > link training start. This change came part of DP2.1 SCR.
> 
> No need to refer to the SCR anymore, as DP 2.1 is out.
> 
> There are a bunch of detailed comments inline.
> 
> High level, this should probably be done much earlier. See Table 5-21 
> in
> DP 2.1. We need to read DPCD 0x2217 before writing 0x110. The DPRX
> updates 0x2217 before asserting hotplug, so we should probably read
> it
> at detect where we read all other DPCD too.
> 
> How early is TCSS_DDI_STATUS available, should we read that at
> hotplug
> too? 

This is available once the cable is inserted and is configured
by TCSS/EC in Chrome and PD in Windows. 
Please check: VLK-42522

> For USB-C we should write to DPCD 0x110 the least common
> denominator between DPCD 0x2217 and 0x110.
> 
> Another question which I didn't find an answer to yet, does writing
> 0x110 impact what the RPRX reports for capabilities i.e. can we
> proceed

No, DPRX caps will not change. DP2.1 Sink will still repot UHBRx even
if the cable doesn't support UHBRX

> with link training normally from there, *or* should we limit the
> sink_rates/common_rates based on TCSS_DDI_STATUS and DPCD 0x2217
> i.e. filter out UHBR as needed.

Yes, we should limit "common rates" to the intersection of (source,
sink, cable)

The question, do we really need to care about reading/writing DPCD
0x110 & 0x2217 given TCSS_DDI_STATUS already reflects that? 

Thank You
Khaled
> 
> Please read bspec and DP 2.1 further to find answers.
> 
> > Note: This patch is not tested due to unavailability of
> > cable. Sending as RFC for design review.
> > 
> > Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_ddi.c | 57
> > ++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/display/intel_tc.c  | 10 +++++
> >  drivers/gpu/drm/i915/display/intel_tc.h  |  1 +
> >  drivers/gpu/drm/i915/i915_reg.h          |  5 +++
> >  include/drm/display/drm_dp.h             |  9 ++++
> >  5 files changed, 82 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> > b/drivers/gpu/drm/i915/display/intel_ddi.c
> > index 70d44edd8c6e..3a0f6a3c9f98 100644
> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > @@ -2208,6 +2208,55 @@ static void
> > intel_dp_sink_set_msa_timing_par_ignore_state(struct intel_dp
> > *intel
> >  			    str_enable_disable(enable));
> >  }
> >  
> > +#define CABLE_SPEED_SHIFT 4
> > +
> > +enum dp_cable_speed {
> > +	DP_CABLE_HBR3 = 1,
> > +	DP_CABLE_UHBR10,
> > +	DP_CABLE_GEN3_UHBR20,
> > +	DP_CABLE_GEN4_UHBR20
> > +};
> > +
> > +static void intel_dp_set_cable_attributes(struct intel_dp
> > *intel_dp,
> > +					  u8 cable_attributes)
> 
> There are two "domains" for the cable information, the hardware
> register
> and the DPCD register. However, cable_attributes is neither, but also
> not helpful, which makes this function cumbersome.
> 
> Usually in cases like this, you'd pick one or the other, *or* if you
> want to have a generic middle ground, you'd make it helpful and easy
> to
> use and understand (e.g. a struct).
> 
> In this case, I'd just pick the DPCD as the format, because it's
> platform independent and the whole thing is simple enough.
> 
> So this function would really reduce down to a single DPCD write.
> 
> > +{
> > +	u8 cable_speed;
> > +	bool active_cable, retimer;
> > +	u8 cable_attr_dpcd;
> > +
> > +	cable_speed = cable_attributes >> CABLE_SPEED_SHIFT;
> > +
> > +	switch (cable_speed) {
> > +	case DP_CABLE_HBR3:
> > +		cable_attr_dpcd = 0;
> > +		break;
> > +	case DP_CABLE_UHBR10:
> > +		cable_attr_dpcd = 1;
> > +		break;
> > +	case DP_CABLE_GEN3_UHBR20:
> > +	case DP_CABLE_GEN4_UHBR20:
> > +		cable_attr_dpcd = 2;
> > +		break;
> > +	default:
> > +		cable_attr_dpcd = 0;
> > +		break;
> > +	}
> > +
> > +	active_cable = (cable_attributes <<
> > TCSS_DDI_STATUS_CABLE_ATTR_SHIFT) &
> > +		       TCSS_DDI_STATUS_ACTIVE_CABLE;
> > +	retimer = (cable_attributes <<
> > TCSS_DDI_STATUS_CABLE_ATTR_SHIFT) &
> > +		  TCSS_DDI_STATUS_RETIMER_REDRIVER;
> > +	if (retimer && active_cable)
> > +		cable_attr_dpcd |= DP_CABLE_TYPE_RETIMER_ACTIVE;
> > +	else if (active_cable)
> > +		cable_attr_dpcd |= DP_CABLE_TYPE_LRD_ACTIVE;
> > +	else
> > +		cable_attr_dpcd |= DP_CABLE_TYPE_PASSIVE;
> > +
> > +	drm_dp_dpcd_writeb(&intel_dp->aux,
> > DP_CABLE_ATTRIBUTES_UPDATED_BY_TX,
> > +			   cable_attr_dpcd);
> > +}
> > +
> >  static void intel_dp_sink_set_fec_ready(struct intel_dp *intel_dp,
> >  					const struct intel_crtc_state
> > *crtc_state)
> >  {
> > @@ -2414,6 +2463,7 @@ static void mtl_ddi_pre_enable_dp(struct
> > intel_atomic_state *state,
> >  {
> >  	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> >  	bool is_mst = intel_crtc_has_type(crtc_state,
> > INTEL_OUTPUT_DP_MST);
> > +	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> >  
> >  	intel_dp_set_link_params(intel_dp,
> >  				 crtc_state->port_clock,
> > @@ -2480,6 +2530,13 @@ static void mtl_ddi_pre_enable_dp(struct
> > intel_atomic_state *state,
> >  	intel_dp_check_frl_training(intel_dp);
> >  	intel_dp_pcon_dsc_configure(intel_dp, crtc_state);
> >  
> > +	if (intel_tc_port_in_dp_alt_mode(dig_port)) {
> > +		u8 cable_attributes;
> > +
> > +		cable_attributes =
> > intel_tc_get_cable_attributes(dig_port);
> > +		intel_dp_set_cable_attributes(intel_dp,
> > cable_attributes);
> > +	}
> > +
> >  	/*
> >  	 * 6. The rest of the below are substeps under the bspec's
> > "Enable and
> >  	 * Train Display Port" step.  Note that steps that are specific
> > to
> > diff --git a/drivers/gpu/drm/i915/display/intel_tc.c
> > b/drivers/gpu/drm/i915/display/intel_tc.c
> > index 3ebf41859043..6b10a8839563 100644
> > --- a/drivers/gpu/drm/i915/display/intel_tc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_tc.c
> > @@ -260,6 +260,16 @@ assert_tc_port_power_enabled(struct
> > intel_tc_port *tc)
> >  		    !intel_display_power_is_enabled(i915,
> > tc_port_power_domain(tc)));
> >  }
> >  
> > +u8 intel_tc_get_cable_attributes(struct intel_digital_port
> > *dig_port)
> > +{
> > +	struct drm_i915_private *i915 = to_i915(dig_port-
> > >base.base.dev);
> > +	enum tc_port tc_port = intel_port_to_tc(i915, dig_port-
> > >base.port);
> 
> So I think this function should return the information in DPCD 0x110
> format.
> 
> Read the register, convert to DPCD format, return. Make this the
> single
> point of conversion between the two, and don't pass intermediate info
> around.
> 
> Whoever calls this should then have DPCD 0x2217 and the info returned
> by
> this function, and find the least common denominator, and update
> 0x110
> accordingly. And *maybe* also update sink_rates/common_rates
> accordingly.
> 
> > +
> > +	return (intel_de_read(i915, TCSS_DDI_STATUS(tc_port)) &
> > +		TCSS_DDI_STATUS_CABLE_ATTR_MASK) >>
> > +		TCSS_DDI_STATUS_CABLE_ATTR_SHIFT;
> > +}
> > +
> >  u32 intel_tc_port_get_lane_mask(struct intel_digital_port
> > *dig_port)
> >  {
> >  	struct drm_i915_private *i915 = to_i915(dig_port-
> > >base.base.dev);
> > diff --git a/drivers/gpu/drm/i915/display/intel_tc.h
> > b/drivers/gpu/drm/i915/display/intel_tc.h
> > index 3b16491925fa..edafe92844b4 100644
> > --- a/drivers/gpu/drm/i915/display/intel_tc.h
> > +++ b/drivers/gpu/drm/i915/display/intel_tc.h
> > @@ -43,5 +43,6 @@ int intel_tc_port_init(struct intel_digital_port
> > *dig_port, bool is_legacy);
> >  void intel_tc_port_cleanup(struct intel_digital_port *dig_port);
> >  
> >  bool intel_tc_cold_requires_aux_pw(struct intel_digital_port
> > *dig_port);
> > +u8 intel_tc_get_cable_attributes(struct intel_digital_port
> > *dig_port);
> >  
> >  #endif /* __INTEL_TC_H__ */
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index 0523418129c5..991ecf082b5c 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -6576,6 +6576,11 @@ enum skl_power_gate {
> >  #define TCSS_DDI_STATUS(tc)			_MMIO(_PICK_EVE
> > N(tc, \
> >  								 _TCSS_
> > DDI_STATUS_1, \
> >  								 _TCSS_
> > DDI_STATUS_2))
> > +#define  TCSS_DDI_STATUS_CABLE_ATTR_SHIFT	9
> > +#define  TCSS_DDI_STATUS_CABLE_ATTR_MASK	REG_GENMASK(14, 9)
> 
> This "cable attr" thing defines something that I think should not be
> used, a field in a register where you can't even use the other
> defines
> to parse. Please remove it, and replace with mask and values for
> CABLE_SPEED.
> 
> This reflects the comment on cable_attributes parameter in
> intel_dp_set_cable_attributes().
> 
> > +#define  TCSS_DDI_STATUS_ACTIVE_CABLE		REG_BIT(11)
> > +#define  TCSS_DDI_STATUS_CABLE_TYPE		REG_BIT(10)
> > +#define  TCSS_DDI_STATUS_RETIMER_REDRIVER	REG_BIT(9)
> 
> Usually I promote following the spec for macro naming, but the above
> two
> are silly.
> 
> I think the options are:
> 
> 1) just define them for what they are:
> 
> #define  TCSS_DDI_STATUS_CABLE_TYPE_OPTICAL	REG_BIT(10)
> #define  TCSS_DDI_STATUS_RETIMER		REG_BIT(9)
> 
> 2) consider them reg fields:
> 
> #define  TCSS_DDI_STATUS_CABLE_TYPE		REG_GENMASK(10, 10)
> #define  TCSS_DDI_STATUS_CABLE_TYPE_ELECTRICAL	REG_FIELD_PREP(
> TCSS_DDI_STATUS_CABLE_TYPE, 0)
> #define  TCSS_DDI_STATUS_CABLE_TYPE_OPTICAL	REG_FIELD_PREP(TCSS_DDI
> _STATUS_CABLE_TYPE, 1)
> 
> #define  TCSS_DDI_STATUS_RETIMER_REDRIVER	REG_GENMASK(9, 9)
> #define  TCSS_DDI_STATUS_REDRIVER		REG_FIELD_PREP(TCSS_DDI
> _STATUS_RETIMER_REDRIVER, 0)
> #define  TCSS_DDI_STATUS_RETIMER		REG_FIELD_PREP(TCSS_DDI
> _STATUS_RETIMER_REDRIVER, 1)
> 
> I think the latter is just too verbose, so I'd go for 1).
> 
> >  #define  TCSS_DDI_STATUS_READY			REG_BIT(2)
> >  #define  TCSS_DDI_STATUS_HPD_LIVE_STATUS_TBT	REG_BIT(1)
> >  #define  TCSS_DDI_STATUS_HPD_LIVE_STATUS_ALT	REG_BIT(0)
> > diff --git a/include/drm/display/drm_dp.h
> > b/include/drm/display/drm_dp.h
> > index b046f79f4744..dde715d567c2 100644
> > --- a/include/drm/display/drm_dp.h
> > +++ b/include/drm/display/drm_dp.h
> > @@ -654,6 +654,13 @@
> >  # define DP_LANE13_POST_CURSOR2_SET_MASK    (3 << 4)
> >  # define DP_LANE13_MAX_POST_CURSOR2_REACHED (1 << 6)
> >  
> > +#define DP_CABLE_ATTRIBUTES_UPDATED_BY_TX   0x110
> 
> Please use _DPTX suffix like in the spec.
> 
> /* 2.1 */ missing at the end.
> 
> The UHBR capabilities bits should be defined here.
> 
> > +# define DP_CABLE_TYPE_MASK		    (0x7 << 3)
> > +# define DP_CABLE_TYPE_UNKNOWN		    (0x0 << 3)
> > +# define DP_CABLE_TYPE_PASSIVE		    (0x1 << 3)
> > +# define DP_CABLE_TYPE_LRD_ACTIVE	    (0x2 << 3)
> > +# define DP_CABLE_TYPE_RETIMER_ACTIVE	    (0x3 << 3)
> 
> The values could just be decimal instead of hex.
> 
> > +
> >  #define DP_MSTM_CTRL			    0x111   /* 1.2 */
> >  # define DP_MST_EN			    (1 << 0)
> >  # define DP_UP_REQ_EN			    (1 << 1)
> > @@ -1139,6 +1146,8 @@
> >  # define
> > DP_128B132B_TRAINING_AUX_RD_INTERVAL_32_MS             0x05
> >  # define
> > DP_128B132B_TRAINING_AUX_RD_INTERVAL_64_MS             0x06
> >  
> > +#define DP_CABLE_ATTRIBUTES_UPDATED_BY_RX               0x2217 /*
> > 2.1 */
> 
> Please use _DPRX suffix like in the spec.
> 
> > +
> >  #define DP_TEST_264BIT_CUSTOM_PATTERN_7_0		0x2230
> >  #define DP_TEST_264BIT_CUSTOM_PATTERN_263_256	0x2250