Message-ID: <CAMcHhXos9oQ+aQgMdZLMeSXYQh6obNTW8WfEcuBJ4s6Nc-T9+Q@mail.gmail.com>
Date: Thu, 8 May 2025 00:22:45 +0200
From: Aleksandrs Vinarskis <alex.vinarskis@...il.com>
To: Abhinav Kumar <quic_abhinavk@...cinc.com>
Cc: Dmitry Baryshkov <lumag@...nel.org>, linux-arm-msm@...r.kernel.org, 
	dri-devel@...ts.freedesktop.org, freedreno@...ts.freedesktop.org, 
	linux-kernel@...r.kernel.org, dmitry.baryshkov@....qualcomm.com, 
	Rob Clark <robdclark@...il.com>, Sean Paul <sean@...rly.run>, 
	Marijn Suijten <marijn.suijten@...ainline.org>, David Airlie <airlied@...il.com>, 
	Simona Vetter <simona@...ll.ch>, laurentiu.tudor1@...l.com, abel.vesa@...aro.org, 
	johan@...nel.org, Johan Hovold <johan+linaro@...nel.org>, 
	Stefan Schmidt <stefan.schmidt@...aro.org>
Subject: Re: [PATCH v4 4/4] drm/msm/dp: Introduce link training per-segment
 for LTTPRs

On Thu, 8 May 2025 at 00:08, Abhinav Kumar <quic_abhinavk@...cinc.com> wrote:
>
> Hi Alex
>
>
> On 5/7/2025 3:01 PM, Aleksandrs Vinarskis wrote:
> > On Tue, 6 May 2025 at 01:41, Abhinav Kumar <quic_abhinavk@...cinc.com> wrote:
> >>
> >> Hi Alex
> >>
> >> On 5/4/2025 3:06 PM, Aleksandrs Vinarskis wrote:
> >>> On Sun, 4 May 2025 at 05:02, Abhinav Kumar <quic_abhinavk@...cinc.com> wrote:
> >>>>
> >>>> Hi Alex
> >>>>
> >>>> Thanks for the response.
> >>>>
> >>>> My updates below. I also had one question for Abel below.
> >>>>
> >>>> Thanks
> >>>>
> >>>> Abhinav
> >>>>
> >>>> On 5/1/2025 8:56 AM, Aleksandrs Vinarskis wrote:
> >>>>> On Thu, 1 May 2025 at 04:11, Abhinav Kumar <quic_abhinavk@...cinc.com> wrote:
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> On 4/29/2025 5:09 PM, Aleksandrs Vinarskis wrote:
> >>>>>>> DisplayPort requires per-segment link training when LTTPR are switched
> >>>>>>> to non-transparent mode, starting with LTTPR closest to the source.
> >>>>>>> Only when each segment is trained individually, source can link train
> >>>>>>> to sink.
> >>>>>>>
> >>>>>>> Implement per-segment link traning when LTTPR(s) are detected, to
> >>>>>>> support external docking stations. On higher level, changes are:
> >>>>>>>
> >>>>>>> * Pass phy being trained down to all required helpers
> >>>>>>> * Run CR, EQ link training per phy
> >>>>>>> * Set voltage swing, pre-emphasis levels per phy
> >>>>>>>
> >>>>>>> This ensures successful link training both when connected directly to
> >>>>>>> the monitor (single LTTPR onboard most X1E laptops) and via the docking
> >>>>>>> station (at least two LTTPRs).
> >>>>>>>
> >>>>>>> Fixes: 72d0af4accd9 ("drm/msm/dp: Add support for LTTPR handling")
> >>>>>>>
> >>>>>>
> >>>>>> Thanks for the patch to improve and add support for link training in
> >>>>>> non-transparent mode.
> >>>>>>
> >>>>>> Some questions below as the DP 2.1a spec documentation is not very clear
> >>>>>> about segmented link training as you noted in the cover letter, so I am
> >>>>>> also only reviewing i915 as reference here.
> >>>>>>
> >>>>>>
> >>>>>>> Tested-by: Johan Hovold <johan+linaro@...nel.org>
> >>>>>>> Tested-by: Rob Clark <robdclark@...il.com>
> >>>>>>> Tested-by: Stefan Schmidt <stefan.schmidt@...aro.org>
> >>>>>>> Signed-off-by: Aleksandrs Vinarskis <alex.vinarskis@...il.com>
> >>>>>>> Reviewed-by: Abel Vesa <abel.vesa@...aro.org>
> >>>>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
> >>>>>>> ---
> >>>>>>>      drivers/gpu/drm/msm/dp/dp_ctrl.c | 126 ++++++++++++++++++++++---------
> >>>>>>>      1 file changed, 89 insertions(+), 37 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> >>>>>>> index d8633a596f8d..35b28c2fcd64 100644
> >>>>>>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> >>>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> >>>>>>> @@ -1034,10 +1034,12 @@ static int msm_dp_ctrl_set_vx_px(struct msm_dp_ctrl_private *ctrl,
> >>>>>>>          return 0;
> >>>>>>>      }
> >>>>>>>
> >>>>>>> -static int msm_dp_ctrl_update_vx_px(struct msm_dp_ctrl_private *ctrl)
> >>>>>>> +static int msm_dp_ctrl_update_phy_vx_px(struct msm_dp_ctrl_private *ctrl,
> >>>>>>> +                                     enum drm_dp_phy dp_phy)
> >>>>>>>      {
> >>>>>>>          struct msm_dp_link *link = ctrl->link;
> >>>>>>> -     int ret = 0, lane, lane_cnt;
> >>>>>>> +     int lane, lane_cnt, reg;
> >>>>>>> +     int ret = 0;
> >>>>>>>          u8 buf[4];
> >>>>>>>          u32 max_level_reached = 0;
> >>>>>>>          u32 voltage_swing_level = link->phy_params.v_level;
> >>>>>>> @@ -1075,8 +1077,13 @@ static int msm_dp_ctrl_update_vx_px(struct msm_dp_ctrl_private *ctrl)
> >>>>>>>
> >>>>>>>          drm_dbg_dp(ctrl->drm_dev, "sink: p|v=0x%x\n",
> >>>>>>>                          voltage_swing_level | pre_emphasis_level);
> >>>>>>> -     ret = drm_dp_dpcd_write(ctrl->aux, DP_TRAINING_LANE0_SET,
> >>>>>>> -                                     buf, lane_cnt);
> >>>>>>> +
> >>>>>>> +     if (dp_phy == DP_PHY_DPRX)
> >>>>>>> +             reg = DP_TRAINING_LANE0_SET;
> >>>>>>> +     else
> >>>>>>> +             reg = DP_TRAINING_LANE0_SET_PHY_REPEATER(dp_phy);
> >>>>>>> +
> >>>>>>> +     ret = drm_dp_dpcd_write(ctrl->aux, reg, buf, lane_cnt);
> >>>>>>
> >>>>>> For the max voltage and swing levels, it seems like we need to use the
> >>>>>> source (DPTX) or the DPRX immediately upstream of the RX we are trying
> >>>>>> to train. i915 achieves it with below:
> >>>>>>
> >>>>>>             /*
> >>>>>>              * Get voltage_max from the DPTX_PHY (source or LTTPR) upstream
> >>>>>> from
> >>>>>>              * the DPRX_PHY we train.
> >>>>>>              */
> >>>>>>             if (intel_dp_phy_is_downstream_of_source(intel_dp, dp_phy))
> >>>>>>                     voltage_max = intel_dp->voltage_max(intel_dp, crtc_state);
> >>>>>>             else
> >>>>>>                     voltage_max = intel_dp_lttpr_voltage_max(intel_dp,
> >>>>>> dp_phy + 1);
> >>>>>>
> >>>>
> >>>> Before I update on the below set of questions from Alex, let me clarify
> >>>> one point from Abel.
> >>>>
> >>>> Hi Abel
> >>>>
> >>>> Apologies to ask this late, but as per the earlier discussions we had
> >>>> internally, I thought we wanted to set the LTTPR to transparent mode to
> >>>> avoid the issues. The per-segment link training becomes a requirement if
> >>>> we use non-transparent mode iiuc.
> >>>>
> >>>> In the description of the PHY_REPEATER_MODE DPCD register, it states
> >>>> like below:
> >>>>
> >>>> "A DPTX operating with 8b/10b Link Layer (MAIN_LINK_CHANNEL_CODING_SET
> >>>> register (DPCD Address 00108h) is programmed to 01h) may configure LTTPRs
> >>>> to either Transparent (default) or Non-transparent mode.
> >>>> A DPTX that establishes the DP link with 128b/132b channel coding shall
> >>>> write
> >>>> 02h to the MAIN_LINK_CHANNEL_CODING_SET register and configure LTTPRs
> >>>> to Non-transparent mode."
> >>>>
> >>>> As per the msm dp code, we are using 8b/10b encoding, like below
> >>>>
> >>>> static int msm_dp_ctrl_link_train(struct msm_dp_ctrl_private *ctrl,
> >>>>                            int *training_step)
> >>>> {
> >>>>            int ret = 0;
> >>>>            const u8 *dpcd = ctrl->panel->dpcd;
> >>>>            u8 encoding[] = { 0, DP_SET_ANSI_8B10B };
> >>>>
> >>>> So can you pls elaborate why we set the PHY_REPEATER_MODE to
> >>>> non-transparent mode because drm_dp_lttpr_init() will set the LTTPR to
> >>>> non-transparent mode.
> >>>>
> >>>> The second part of the section is what was described in the commit text
> >>>> of the 72d0af4accd9 ("drm/msm/dp: Add support for LTTPR handling") but
> >>>>
> >>>> "Before performing link training with LTTPR(s), the DPTX may place the
> >>>> LTTPR(s) in
> >>>> Non-transparent mode by first writing 55h to the PHY_REPEATER_MODE
> >>>> register, and then
> >>>> writing AAh. This operation does not need to be performed on subsequent
> >>>> link training actions
> >>>> unless a downstream device unplug event is detected."
> >>>>
> >>>> So just wanted to understand this better that was there any requirement
> >>>> to put it to non-transparent mode other than the section of the spec
> >>>> highlighted above? Because above lines are only suggesting that if we
> >>>> want to put the LTTPR to non-transparent mode, how to do it but not to
> >>>> always do it. Please let me know your comments.
> >>>>
> >>>> I shall also check internally on this to close this.
> >>>>
> >>>>
> >>>> Hi Alex
> >>>>
> >>>>>>
> >>>>>> But I do not see (unless I missed) how this patch takes care of this
> >>>>>> requirement.
> >>>>>>
> >>>>>> Same holds true for preemph too
> >>>>>
> >>>>> Thanks for you review,
> >>>>>
> >>>>> This is a very good point. You are right, in the present state it does
> >>>>> not. Intel's driver is verifying whether LTTPRs supports
> >>>>> DP_TRAIN_LEVEL_3 or only DP_TRAIN_LEVEL_2, while my current change
> >>>>> follows msm-dp's default which was recently set to DP_TRAIN_LEVEL_3
> >>>>> [1]. I came to conclusion that in particular case it was not required
> >>>>> to verify that LTTPR indeed supports training level 3, but do not
> >>>>> remember the details as its been a few months... should've document it
> >>>>> :)
> >>>>>
> >>>>
> >>>>> As I recall, from one of the DP specs onward (has to be 1.4a then,
> >>>>> since LTTPR was initially introduced in DP 1.3, but register for phy
> >>>>> capabilities only added in 1.4a [2]) it mandates training level 3
> >>>>> support for LTTPRs, so the assumption would've be correct in that
> >>>>> case. Is this something you could verify from the official
> >>>>> documentation? Unfortunately I do not have sources to back this
> >>>>> statement, so it may be incorrect...
> >>>>>
> >>>>
> >>>> I went through DP spec 1.4(a), DP 2.0 and DP 2.1(a). This is what is
> >>>> mentioned below:
> >>>>
> >>>>
> >>>> "LTTPR shall support all required voltage swing and pre-emphasis
> >>>> combinations defined
> >>>> in Table 3-2. The LTTPR shall reflect its support of optional Voltage
> >>>> Swing Level 3
> >>>> and Pre-emphasis Level 3 in the VOLTAGE_SWING_LEVEL_3_SUPPORTED and
> >>>> VOLTAGE_SWING_LEVEL_3_SUPPORTED bits, respectively, in the
> >>>> TRANSMITTER_CAPABILITY_PHY_REPEATERx register(s) (e.g., DPCD
> >>>> Address F0021h for LTTPR1, bits 0 and 1, respectively)."
> >>>>
> >>>>    From this paragraph, it means that LTTPR support for levels 0/1/2 can
> >>>> be assumed and level 3 is optional. Whether or not level 3 is supported
> >>>> comes from the TRANSMITTER_CAPABILITY_PHY_REPEATERx register(s).
> >>>>
> >>>> This aligns with i915 implementation.
> >>>>
> >>>>
> >>>> Now, right after this, there is another paragraph in the spec:
> >>>>
> >>>> "If the DPTX sets the voltage swing or pre-emphasis to a level that the
> >>>> LTTPR does not support,
> >>>> the LTTPR shall set its transmitter levels as close as possible to those
> >>>> requested by the DPTX.
> >>>> Although the LTTPR’s level choosing is implementation-specific, the
> >>>> levels chosen shall
> >>>> comply with Section 3.5.4."
> >>>
> >>> Hi Abhinav,
> >>>
> >>> Could you please provide the exact section number and DP spec version
> >>> for this paragraph? For reference in the commit message, see below.
> >>>
> >>
> >> This is in the section "3.6.7.2 8b/10b DP Link Layer LTTPR Link Training
> >> Mandates" of DP spec 2.1(a)"
> >
> > Perfect, thanks.
> >
> >>
> >>>>
> >>>> This tells us that even if we try to do a level3 and the LTTPR does not
> >>>> support it, it will use the one closest to this.
> >>>>
> >>>> So overall, even though i915's implementation is the accurate one, the
> >>>> DP spec does mention that the LTTPR can adjust. I just hope all LTTPRs
> >>>> can adjust to this. Hopefully this clarifies the requirements spec-wise.
> >>>
> >>> Thanks for this clarification, this is extremely useful. A bit sad
> >>> that DP spec is only available to VESA members.
> >>> So my assumption was indeed incorrect. This also explains why eg.
> >>> AMD's driver works, nice.
> >>>
> >>
> >> Yes. This was good to know.
> >>
> >>>>
> >>>> Hence I am okay with this change as such as multiple folks including us
> >>>> have given a Tested-by but I would like this to be documented in the
> >>>> commit text so that full context is preserved. The only concern I have
> >>>> is I hope that the level to which the LTTPR adjusts will be correct as
> >>>> that again is "implementation specific".
> >>>
> >>> I started implementing i915's approach meanwhile, to see the
> >>> difference in behaviour. POC fixup for patch 3,4 of this series can be
> >>> found in [1]. Discovered something interesting:
> >>> * Dell WD19TB docking station's LTTPR reports support of training level 3
> >>> * PS8833 retimer in Asus Zenbook A14 reports support of training level 3
> >>> * PS8830 retimer in Dell XPS 9345 claims to _not_ report support
> >>> training level 3. This is the case on two different machines with BIOS
> >>> 1.9.0 (PS8830 payload version 5.3.0.14) and BIOS 2.5.0 (PS8830 payload
> >>> version 9.3.0.01).
> >>>
> >>> This leads to interesting test results:
> >>> * Asus Zenbook A14 (PS8833, supports train level 3) with direct
> >>> monitor connection via Type-C works, both in current version of msm-dp
> >>> (aka AMD's approach) and with additional patches I linked above (aka
> >>> i915's approach)
> >>> * Dell XPS 9345 (PS8830, claims to not support train level 3) with
> >>> Dell WD19TB (supports train level 3) works, both in current version of
> >>> msm-dp and with additional patches I linked above. In this
> >>> combination, PS8830->WD19TB segment training succeeds with vs=2, pe=0
> >>> already.
> >>> * Dell XPS 9345 (PS8830, claims to not support train level 3) with
> >>> direct monitor connection via Type-C works with current version of
> >>> msm-dp, but does _not_ work with additional patches I linked above.
> >>> For PS8830->Monitor segment training, after reaching vs=2,pe=0 and
> >>> being stopped from going higher (due to PS8830 claiming it cannot do
> >>> train level 3), link training fails. With current msm-dp state
> >>> however, the same PS8830->Monitor segment training succeeds with
> >>> vs=2,pe=1. This is contrary to retimer reporting it does not support
> >>> train level 3 - it in fact does, and in case with 1m long Type-C to DP
> >>> cable it only works with train level 3. Bug in P8830's LTTPR
> >>> implementation? :)
> >>>
> >>
> >> Wow, thats a very good finding!
> >>
> >>> Combining both patches linked above as well as debug patch to force
> >>> max train level to 3 like it was before [2], here are detailed logs:
> >>> Asus Zenbook A14, BIOS version "UX3407QA.305":
> >>> ```
> >>> phy #1: params reset                                                 #
> >>> training DPRX (phy1/PS8833)
> >>> phy #1: max_v_level=3, max_p_level=3                    # DPTX source
> >>> (X1E) supports train level 3
> >>> phy #1: forcing max_v_level=3, max_p_level=3
> >>> phy #1: v_level=0, p_level=0                                      #
> >>> passes with vs=0,ps=0
> >>> phy #1: max_v_level=3, max_p_level=3
> >>> phy #0: params reset
> >>> # training DPRX (phy0/Monitor)
> >>> phy #0: max_v_level=3, max_p_level=3                     # DPTX source
> >>> (phy1/PS8833) supports train level 3
> >>> phy #0: forcing max_v_level=3, max_p_level=3
> >>> phy #0: v_level=0, p_level=0
> >>> phy #0: max_v_level=3, max_p_level=3
> >>> phy #X: v_level=2, p_level=0
> >>> phy #0: v_level=2, p_level=0
> >>> phy #0: max_v_level=3, max_p_level=3
> >>> phy #X: v_level=2, p_level=1
> >>> phy #0: v_level=2, p_level=1                                       #
> >>> training passes with vs=2,ps=1
> >>> phy #0: max_v_level=3, max_p_level=3
> >>> ```
> >>>
> >>> Dell XPS 9345, BIOS version "2.5.0", PS8830 payload version "9.3.0.01":
> >>> ```
> >>> phy #1: params reset                                                 #
> >>> training DPRX (phy1/PS8830)
> >>> phy #1: max_v_level=3, max_p_level=3                    # DPTX source
> >>> (X1E) supports train level 3
> >>> phy #1: forcing max_v_level=3, max_p_level=3
> >>> phy #1: v_level=0, p_level=0                                     #
> >>> passes with vs=0,ps=0
> >>> phy #1: max_v_level=3, max_p_level=3
> >>> phy #0: params reset                                                 #
> >>> training DPRX (phy0/Monitor)
> >>> phy #0: max_v_level=2, max_p_level=2                    # DPTX source
> >>> (phy1/PS8830) claims to not support train level 3
> >>> phy #0: forcing max_v_level=3, max_p_level=3        # Ignore
> >>> advertised levels, force to max=3, otherwise training fails
> >>> phy #0: v_level=0, p_level=0
> >>> phy #0: max_v_level=3, max_p_level=3
> >>> phy #X: v_level=2, p_level=0
> >>> phy #0: v_level=2, p_level=0
> >>> phy #0: max_v_level=3, max_p_level=3
> >>> phy #X: v_level=2, p_level=1
> >>> phy #0: v_level=2, p_level=1                                     #
> >>> training passes with vs=2,ps=1 (aka train level 3)
> >>> phy #0: max_v_level=3, max_p_level=3
> >>> ```
> >>>
> >>> While, as you correctly mentioned, i915's implementation would be a
> >>> more accurate one, and I can respin to v5 with [1] applied to patches
> >>> 3,4 of this series respectively, it appears that at least on some X1E
> >>> based devices with PS8830 that would break DP output support at least
> >>> in some cases. The fact that the same device with the same monitor
> >>> works on Windows suggests that Windows driver also uses AMD's approach
> >>> of just assuming LTTPR can do train level 3, without verifying it, and
> >>> letting LTTPR figure the rest. I have asked other community members to
> >>> cross-check these findings on another X1E platform with PS8830
> >>> retimers. With this in mind, I am very glad to hear that you are okay
> >>> with this change as such, as it now appears that a more accurate
> >>> implementation would've caused additional issues.
> >>>
> >>
> >> Yes seems like it but certainly looks like a bug in PS8830.
> >>
> >>>>
> >>>> I would still like to hear from Abel though about whether setting to
> >>>> non-transparent mode was needed in the first place.
> >>>
> >>> Fwiw, without Abel's initial change DP output didn't work on X1E
> >>> platform at all, neither direct monitor connection nor docking
> >>> station. Not sure if that is because PS883x found in most X1E/X1P
> >>> laptops do not work in transparent mode at all (even though they
> >>> should've), or laptop's firmware would leave it in some weird state,
> >>> and perhaps re-enabling transparent mode would've also fixed it.
> >>>
> >>> Lets wait for Abel's answer and the rest of this conversation to be
> >>> resolved, and as I see it the next step would be for me to respin to
> >>> v5 current change as is, in order to update the commit message of 4th
> >>> patch to reflect the new findings and reference DP spec and section,
> >>> as per the first comment of this reply.
> >>>
> >>
> >> Yes correct, nothing else pending from your side.
> >>
> >> Thanks for your deep analysis and interest in this topic.
> >>
> >>> Thanks for your help,
> >>> Alex
> >>>
> >>
> >> By waiting for Abel, I am mostly trying to make sure :
> >>
> >> Was enabling non-transparent mode more of a requirement of the parade
> >> retimer chip in Xelite? Because from our earlier discussion, I thought
> >> we wanted to enable transparent mode. Then these issues would perhaps
> >> have not happened as per-segment link training is a requirement of
> >> non-transparent mode. So I am surprised how Xelite worked without this.
> >>
> >> It seems like to me we enabled non-transparent mode to match AMD/i915
> >> behavior and not as a requirement of the retimer chip of Xelite.
> >>
> >> The commit text of
> >> https://patchwork.freedesktop.org/patch/msgid/20250203-drm-dp-msm-add-lttpr-transparent-mode-set-v5-4-c865d0e56d6e@linaro.org
> >> mentions
> >>
> >> "The section 3.6.6.1 from the DisplayPort v2.0 specification mandates
> >> that before link training with the LTTPR is started, the DPTX may place
> >> the LTTPR in non-transparent mode by first switching to transparent mode
> >> and then to non-transparent mode. This operation seems to be needed only
> >> on first link training and doesn't need to be done again until device is
> >> unplugged."
> >>
> >> This talks about how to enable non-transparent mode and not why.
> >>
> >> But this part "It has been observed on a few X Elite-based platforms
> >> which have such LTTPRs in their board design that the DPTX needs to
> >> follow the procedure described above in order for the link training to
> >> be successful" is really my doubt. Because from my earlier
> >> understanding, I thought enabling transparent mode was enough.
> >
> > To speed up the process a little as the 6.15-rcX window shrinks (and
> > it appears Abel may be OOO?), I run a series of tests to attempt to
> > answer your questions. In short - PS8830 is a very quirky device, and
> > you were right that the current implementation could've simply set
> > transparent mode.
> >
> > To clarify the test matrix: PS8830 was tested with Dell XPS 9345.
> > PS8833 was tested with Asus Zenbook A14. Unfortunately, my dock (Dell
> > WD19TB) is a universal Thunderbolt/DP-Alt mode dock, and only works if
> > it's forced to DP alt mode, since PCIE tunneling is not yet supported
> > on qcom. Dell allows to disable thunderbolt/PCIE tunneling support in
> > BIOS, hence forcing the dock to be Type-C/DP-alt mode (and show up
> > with LTTPR onboard). No such feature exists on Asus, so I could not
> > test PS8833 with the docking station at all.
> >
> > Complete test matrix;
> >
> > 1. Do nothing/pre Abel's series (LTTPRs assumed to be in transparent
> > mode unless firmware pre-configured them):
> > PS8833:
> > - Type-C to HDMI: works almost always
> > - Type-C to DP: never works, EDID read fails
> >
> > PS8830:
> > - Type-C to HDMI: never works, CR LT fails (-22)
> > - Type-C to DP: never works, EDID read fails
> > - Type-C to dock: never works, EQ LT fails (-110)
> >
> > 2. Explicitly set LTTPRs to transparent mode (early exit LTTPR helper
> > introduced by Abel, after setting transparent mode, but before setting
> > non-transparent mode):
> > PS8833:
> > - Type-C to HDMI: works. Occasionally fails to get DP sink modes (may
> > be unrelated)
> > - Type-C to DP: works**
> >
> > PS8830:
> > - Type-C to HDMI: works
> > - Type-C to DP: works**
> > - Type-C to dock: Sometimes all works. Sometimes video works, but USB
> > (2.0 nor 3.0) is not working. Sometimes EQ LT fails (-110) and nothing
> > works. Overall extremely unstable.
> >
> > 3. Explicitly set LTTPRs to non-transparent mode (aka Abel's series):
> > PS8833:
> > - Type-C to HDMI: never works, CR LT fails, max v_level reached (-11)
> > - Type-C to DP: never works, CR LT fails, max v_level reached (-11)
> >
> > PS8830:
> > - Type-C to HDMI: works
> > - Type-C to DP: works**
> > - Type-C to dock: never works, CR LT fails (-110)
> >
> > 4. Explicitly set LTTPRs to non-transparent mode, support per-segment
> > training (aka Abel's initial LTTPR support series + this series):
> > PS8833:
> > - Type-C to HDMI: works
> > - Type-C to DP: works
> >
> > PS8830:
> > - Type-C to HDMI: works
> > - Type-C to DP: works
> > - Type-C to dock: works
> >
> >
>
> Thanks for testing these combinations.
>
> > ** At first, Type-C to DP was frequently/always depending on the use
> > case failing to read panel EDID, just like in the 1st test case. As I
> > am 100% certain it worked in the past, did a few more tests. It
> > appears that in an earlier version of Abel's patch (<=v4), DPRX caps
> > were read _after_ LTTPR init, just like DP standard mandates (don't
> > have exact quote, something along the lines 'source shall re-read sink
> > caps after LTTPR init'). In v4 there was a suggestion [1] (from you
> > actually :)) to first try to read DPRX caps, then init LTTPRs in order
> > to fail early if caps readout fails. Reverting this change fixes EDID
> > read error. Since I was running Abel's series long before it landed, I
> > never used the broken v5. With the order of functions reverted, Type-C
> > to DP started working/failing in the same way Type-C to HDMI dongle
> > did, just as expected. Wrt to the issue itself, the first patch of
> > this very series actually both fixes this issue by conforming to DP
> > spec, and also takes into account your suggestion in Abel's v4 series
> > to be able to fail early in case of DPRX caps readout failure.
> >
> > To summarize the findings:
> > - PS8830 is a very quirky device. It does not work in
> > default/transparent mode unless explicitly set. It does work in
> > non-transparent mode without per-segment training, even though it
> > should've not. As per last email, it is lying about not supporting
> > training level 3.
> > - PS8833 seems to be a fixed version of PS8830. It does work in
> > default/transparent mode oob. It does not work in non-transparent mode
> > without per-segment training, just as expected. As per last email, it
> > correctly reports training level 3 capability.
> >
> > To answer some of your questions (from a 3rd party view, cannot speak
> > for the authors):
> > - "So I am surprised how Xelite worked without this." - From tests
> > above: PS8830 worked when it should've not, seems because it's quirky.
> > PS8833 did not work, which makes sense.
> > - Doubts about non-transparent mode requirement for X1E/X1P systems -
> >  From tests above: seems you are right. I don't know why it was forced
> > to be non-transparent without per-segment training, instead of simply
> > transparent. Though, seeing how weird the PS8830 is, I wouldn't be
> > surprised if it behaves differently in other laptops... just
> > speculating though.
> >   - "Was enabling non-transparent mode more of a requirement of the
> > parade retimer chip in Xelite?" - cannot fully answer. Initializing
> > LTTPRs as such appears to be a requirement of the Parade PS8830 (not
> > PS8833, which wasn't around back then afaik), as it just wouldn't work
> > oob. Choice of non-transparent instead of transparent mode is not very
> > clear to me.
> >
> > Even though it appears initial LTTPR support could've been done
> > slightly differently, combination of those initial patches + this
> > series seem to provide both the best practical results, as well as
> > most (well, almost, excluding LTTPR's train level verification)
> > accurate LTTPR implementation, while also making msm-dp similar/up to
> > date with Intel/AMD's LTTPR implementation. Also learned something new
> > today myself, don't buy PS8830 :)
> >
> > Looking forward to hearing from you,
> > Alex
> >
>
> I did sync up with Abel internally and we could not conclude on why we
> did not enable transparent mode from beginning and went with
> non-transparent mode. But, I did sync up with Imre (author of i915's
> LTTPR changes) on IRC dri-devel and Imre mentioned that there is
> actually an SCR which changed the lines of the spec I was referring to that

Ah, got it. That aligns with my today's findings then, nice.

>
> "A DPTX operating with 8b/10b Link Layer (MAIN_LINK_CHANNEL_CODING_SET
> register (DPCD Address 00108h) is programmed to 01h) may configure
> LTTPRs to either Transparent (default) or Non-transparent mode."
>
>
> Although I cannot locate this SCR (working out that), I am convinced
> with the results, so feel free to re-spin this with
>
> Reviewed-by: Abhinav Kumar <quic_abhinavk@...cinc.com>

Great to hear, coming shortly!

>
> Excellent work on your side with such deep testing and analysis!

Thanks, really want to have it working :)

Alex

>

Powered by blists - more mailing lists