Re: [PATCH] drm/msm/dp: remove limitation of link rate at 5.4G to support HBR3

[Dmitry Baryshkov <dmitry.baryshkov@linaro.org>]

On 02/11/2022 18:47, Doug Anderson wrote:
> Hi,
> 
> On Tue, Nov 1, 2022 at 7:37 AM Doug Anderson <dianders@chromium.org> wrote:
>>
>> Hi,
>>
>> On Mon, Oct 31, 2022 at 5:15 PM Dmitry Baryshkov
>> <dmitry.baryshkov@linaro.org> wrote:
>>>
>>> On 01/11/2022 03:08, Doug Anderson wrote:
>>>> Hi,
>>>>
>>>> On Mon, Oct 31, 2022 at 2:11 PM Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>>>>>
>>>>> Hi Dmitry,
>>>>>
>>>>>
>>>>> Link rate is advertised by sink, but adjusted (reduced the link rate)
>>>>> by host during link training.
>>>>>
>>>>> Therefore should be fine if host did not support HBR3 rate.
>>>>>
>>>>> It will reduce to lower link rate during link training procedures.
>>>>>
>>>>> kuogee
>>>>>
>>>>> On 10/31/2022 11:46 AM, Dmitry Baryshkov wrote:
>>>>>> On 31/10/2022 20:27, Kuogee Hsieh wrote:
>>>>>>> An HBR3-capable device shall also support TPS4. Since TPS4 feature
>>>>>>> had been implemented already, it is not necessary to limit link
>>>>>>> rate at HBR2 (5.4G). This patch remove this limitation to support
>>>>>>> HBR3 (8.1G) link rate.
>>>>>>
>>>>>> The DP driver supports several platforms including sdm845 and can
>>>>>> support, if I'm not mistaken, platforms up to msm8998/sdm630/660.
>>>>>> Could you please confirm that all these SoCs have support for HBR3?
>>>>>>
>>>>>> With that fact being confirmed:
>>>>>>
>>>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>>>>>>> ---
>>>>>>>     drivers/gpu/drm/msm/dp/dp_panel.c | 4 ----
>>>>>>>     1 file changed, 4 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c
>>>>>>> b/drivers/gpu/drm/msm/dp/dp_panel.c
>>>>>>> index 5149ceb..3344f5a 100644
>>>>>>> --- a/drivers/gpu/drm/msm/dp/dp_panel.c
>>>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_panel.c
>>>>>>> @@ -78,10 +78,6 @@ static int dp_panel_read_dpcd(struct dp_panel
>>>>>>> *dp_panel)
>>>>>>>         if (link_info->num_lanes > dp_panel->max_dp_lanes)
>>>>>>>             link_info->num_lanes = dp_panel->max_dp_lanes;
>>>>>>>     -    /* Limit support upto HBR2 until HBR3 support is added */
>>>>>>> -    if (link_info->rate >=
>>>>>>> (drm_dp_bw_code_to_link_rate(DP_LINK_BW_5_4)))
>>>>>>> -        link_info->rate = drm_dp_bw_code_to_link_rate(DP_LINK_BW_5_4);
>>>>>>> -
>>>>>>>         drm_dbg_dp(panel->drm_dev, "version: %d.%d\n", major, minor);
>>>>>>>         drm_dbg_dp(panel->drm_dev, "link_rate=%d\n", link_info->rate);
>>>>>>>         drm_dbg_dp(panel->drm_dev, "lane_count=%d\n",
>>>>>>> link_info->num_lanes);
>>>>
>>>> Stephen might remember better, but I could have sworn that the problem
>>>> was that there might be something in the middle that couldn't support
>>>> the higher link rate. In other words, I think we have:
>>>>
>>>> SoC <--> TypeC Port Controller <--> Display
>>>>
>>>> The SoC might support HBR3 and the display might support HBR3, but the
>>>> TCPC (Type C Port Controller) might not. I think that the TCPC is a
>>>> silent/passive component so it can't really let anyone know about its
>>>> limitations.
>>>>
>>>> In theory I guess you could rely on link training to just happen to
>>>> fail if you drive the link too fast for the TCPC to handle. Does this
>>>> actually work reliably?
>>>>
>>>> I think the other option that was discussed in the past was to add
>>>> something in the device tree for this. Either you could somehow model
>>>> the TCPC in DRM and thus know that a given model of TCPC limits the
>>>> link rate or you could hack in a property in the DP controller to
>>>> limit it.
>>>
>>> Latest pmic_glink proposal from Bjorn include adding the drm_bridge for
>>> the TCPC. Such bridge can in theory limit supported modes and rates.
>>
>> Excellent! Even so, I think this isn't totally a solved problem,
>> right? Even though a bridge seems like a good place for this, last I
>> remember checking the bridge API wasn't expressive enough to solve
>> this problem. A bridge could limit pixel clocks just fine, but here we
>> need to take into account other considerations to know if a given
>> pixel clock can work at 5.4 GHz or not. For instance, if we're at 4
>> lanes we could maybe make a given pixel clock at 5.4 GHz but not if we
>> only have 2 lanes. I don't think that the DP controller passes the
>> number of lanes to other parts of the bridge chain, though maybe
>> there's some trick for it?
>>
>> ...I guess the other problem is that all existing users aren't
>> currently modeling their TCPC in this way. What happens to them?
> 
> FWIW, I did more research on the "let's rely on link training to
> detect TCPC's that only support HBR2". I haven't tested it myself, but
> from looking at a 1.5 year old internal bug where we discussed this
> before, both others at Qualcomm and others at Google were skeptical
> about this. Both parties had past experience where link training would
> succeed but the display wouldn't be reliable at the higher link rate.
> 
> I guess that leaves us with 3 possible approaches:
> 
> 1. Someone figures out how to model this with the bridge chain and
> then we only allow HBR3 if we detect we've got a TCPC that supports
> it. This seems like the cleanest / best but feels like a long pole.
> Not only have we been trying to get the TCPC-modeled-as-a-bridge stuff
> landed for a long time but even when we do it we still don't have a
> solution for how to communicate the number of lanes and other stuff
> between the TCPC and the DP controller so we have to enrich the bridge
> interface.

I think we'd need some OOB interface. For example for DSI interfaces we 
have mipi_dsi_device struct to communicate such OOB data.

Also take a note regarding data-lanes from my previous email.

> 
> 2. We add in a DT property to the display controller node that says
> the max link rate for use on this board. This feels like a hack, but
> maybe it's not too bad. Certainly it would be incredibly simple to
> implement. Actually... ...one could argue that even if we later model
> the TCPC as a bridge that this property would still be valid / useful!
> You could certainly imagine that the SoC supports HBR3 and the TCPC
> supports HBR3 but that the board routing between the SoC and the TCPC
> is bad and only supports HBR2. In this case the only way out is
> essentially a "board constraint" AKA a DT property in the DP
> controller.

We have been discussing similar topics with Abhinav. Krzysztof suggested 
using link-frequencies property to provide max and min values.

> 
> 3. We could do some hack based on the SoC. We could assume that newer
> SoCs will have a TCPC that was tested with HBR3. This doesn't require
> any DT changes and would work, but feels like it won't stand the test
> of time.
> 
> I'd vote for #2 but I'm interested in what others say.
> 
> -Doug

-- 
With best wishes
Dmitry