Hi Ville, First of all, thank you very much for the review. I added some comments below. On 09/05, Wentland, Harry wrote: > On 2019-09-05 1:29 p.m., Ville Syrjälä wrote: > > On Wed, Sep 04, 2019 at 07:02:18PM +0000, Siqueira, Rodrigo wrote: > >> DP 1.4a specification defines Link Training Tunable PHY Repeater (LTTPR) > > > > A bunch of this stuff was already in DP 1.3 so the statement here > > (and in the comment) is a bit misleading. > > > > "LTTPR" is not a name that appears anywhere in the spec AFAICS, so > > calling it that is a bit confusing. > > We are using "VESA DisplayPort (DP) Standard 1.4a" as a reference. We double-checked the specification, and we found many occurrences of LTTPR. > >> which is required to add support for systems with Thunderbolt or other > >> repeater devices. > > > > "required" seems a bit strong. IIRC by default these things should be in > > transparent mode so the DPTX can remain blissfully unaware of their > > presence. > > Just for adding some extra information: LTTPR can work in two modes: non-transparent and transparent. In the non-transparent mode we need to train each repeater in the link which brings the benefit of better signal quality in contrast to the transparent mode. > That's the idea but in reality things usually don't work out like this. > I remember a couple years back debugging Thunderbolt and having it > modify DPCD register reads on me and messing up link training with > certain receivers. > > Either way, we've found that in order to receive a reliable > implementation we need to make use of the LTTPR functionality. > > >> > >> Changes since V3: > >> - Replace spaces by tabs > >> Changes since V2: > >> - Drop the kernel-doc comment > >> - Reorder LTTPR according to register offset > >> Changes since V1: > >> - Adjusts registers names to be aligned with spec and the rest of the > >> file > >> - Update spec comment from 1.4 to 1.4a > >> > >> Cc: Abdoulaye Berthe <Abdoulaye.Berthe@xxxxxxx> > >> Cc: Harry Wentland <harry.wentland@xxxxxxx> > >> Cc: Leo Li <sunpeng.li@xxxxxxx> > >> Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> > >> Cc: Manasi Navare <manasi.d.navare@xxxxxxxxx> > >> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > >> Signed-off-by: Rodrigo Siqueira <Rodrigo.Siqueira@xxxxxxx> > >> Signed-off-by: Abdoulaye Berthe <Abdoulaye.Berthe@xxxxxxx> > >> Reviewed-by: Harry Wentland <Harry.Wentland@xxxxxxx> > >> --- > >> include/drm/drm_dp_helper.h | 25 +++++++++++++++++++++++++ > >> 1 file changed, 25 insertions(+) > >> > >> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h > >> index 8364502f92cf..5abed96a1cb1 100644 > >> --- a/include/drm/drm_dp_helper.h > >> +++ b/include/drm/drm_dp_helper.h > >> @@ -966,6 +966,31 @@ > >> #define DP_HDCP_2_2_REG_STREAM_TYPE_OFFSET 0x69494 > >> #define DP_HDCP_2_2_REG_DBG_OFFSET 0x69518 > >> > >> +/* Link Training (LT)-tunable Physical Repeaters - DP 1.4a */ > > > > s/Physical/PHY/ to match the spec. > > > >> +#define DP_LT_TUNABLE_PHY_REPEATER_FIELD_DATA_STRUCTURE_REV 0xf0000 > >> +#define DP_MAX_LINK_RATE_PHY_REPEATER 0xf0001 > >> +#define DP_PHY_REPEATER_CNT 0xf0002 > >> +#define DP_PHY_REPEATER_MODE 0xf0003 > >> +#define DP_MAX_LANE_COUNT_PHY_REPEATER 0xf0004 > >> +#define DP_PHY_REPEATER_EXTENDED_WAIT_TIMEOUT 0xf0005 > > > > The last two are DP 1.4a it seems. > > > > 0xf0004 was called Repeater_FEC_CAPABILITY in 1.4. But the spec doesn't > > say anything about the DPCD revision so I have no idea how you're > > supposed to decide which definition to use. > > > > DP 1.4a seems to have added FEC_CAPABILITY_PHY_REPEATER1 at 0xf0294. > > To replace the 1.4 Repeater_FEC_CAPABILITY I suppose. > > Touché! :) Really good point, we have to check it. For the next version, I'll add comments for showing if an address came from 1.4 or 1.4a. I'll also apply the other suggestions. Thank you again! > This part confused me when I saw it in 1.4 and 1.4a. It's probably > safest to go with the 1.4a definition. > > >> +#define DP_TRAINING_PATTERN_SET_PHY_REPEATER1 0xf0010 > >> +#define DP_TRAINING_LANE0_SET_PHY_REPEATER1 0xf0011 > >> +#define DP_TRAINING_LANE1_SET_PHY_REPEATER1 0xf0012 > >> +#define DP_TRAINING_LANE2_SET_PHY_REPEATER1 0xf0013 > >> +#define DP_TRAINING_LANE3_SET_PHY_REPEATER1 0xf0014 > >> +#define DP_TRAINING_AUX_RD_INTERVAL_PHY_REPEATER1 0xf0020 > >> +#define DP_TRANSMITTER_CAPABILITY_PHY_REPEATER1 0xf0021 > > > > Above two are DP 1.4a. > > > >> +#define DP_LANE0_1_STATUS_PHY_REPEATER1 0xf0030 > >> +#define DP_LANE2_3_STATUS_PHY_REPEATER1 0xf0031 > >> +#define DP_LANE_ALIGN_STATUS_UPDATED_PHY_REPEATER1 0xf0032 > >> +#define DP_ADJUST_REQUEST_LANE0_1_PHY_REPEATER1 0xf0033 > >> +#define DP_ADJUST_REQUEST_LANE2_3_PHY_REPEATER1 0xf0034 > >> +#define DP_SYMBOL_ERROR_COUNT_LANE0_PHY_REPEATER1 0xf0035 > >> +#define DP_SYMBOL_ERROR_COUNT_LANE1_PHY_REPEATER1 0xf0037 > >> +#define DP_SYMBOL_ERROR_COUNT_LANE2_PHY_REPEATER1 0xf0039 > >> +#define DP_SYMBOL_ERROR_COUNT_LANE3_PHY_REPEATER1 0xf003b > >> +#define DP_FEC_STATUS_PHY_REPEATER1 0xf0290 > > > > This seems to have appared in DP 1.4. > > > > You skipped quite a few registers here. I guess those were deemed not > > important? > > > > They won't be used by us at the moment. > > Harry > > >> + > >> /* DP HDCP message start offsets in DPCD address space */ > >> #define DP_HDCP_2_2_AKE_INIT_OFFSET DP_HDCP_2_2_REG_RTX_OFFSET > >> #define DP_HDCP_2_2_AKE_SEND_CERT_OFFSET DP_HDCP_2_2_REG_CERT_RX_OFFSET > >> -- > >> 2.23.0 > > > > > > -- Rodrigo Siqueira Software Engineer, Advanced Micro Devices (AMD) https://siqueira.tech
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
