Message ID | 20230609055435.299584-1-animesh.manna@intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | drm/i915/dp: Cable type identification for DP2.1 | expand |
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
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
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
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(+)