[PATCH] drm/i915/dp: Cable type identification for DP2.1
Jani Nikula
jani.nikula at intel.com
Fri Jun 9 08:35:59 UTC 2023
On Fri, 09 Jun 2023, Animesh Manna <animesh.manna at 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 at 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
More information about the dri-devel
mailing list