On Fri, 2023-06-09 at 11:35 +0300, Jani Nikula wrote: > On Fri, 09 Jun 2023, Animesh Manna <animesh.manna@xxxxxxxxx> 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@xxxxxxxxx> > > --- > > 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