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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux