Message-ID: <69f0c53a-0ac3-4858-a644-373af0473ddd@quicinc.com>
Date: Tue, 24 Jun 2025 17:56:56 +0800
From: Yongxing Mou <quic_yongmou@...cinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
CC: Rob Clark <robin.clark@....qualcomm.com>,
        Abhinav Kumar
	<abhinav.kumar@...ux.dev>,
        Jessica Zhang <jessica.zhang@....qualcomm.com>,
        Sean Paul <sean@...rly.run>,
        Marijn Suijten <marijn.suijten@...ainline.org>,
        David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
        <linux-arm-msm@...r.kernel.org>, <dri-devel@...ts.freedesktop.org>,
        <freedreno@...ts.freedesktop.org>, <linux-kernel@...r.kernel.org>,
        "Abhinav
 Kumar" <quic_abhinavk@...cinc.com>
Subject: Re: [PATCH v2 29/38] drm/msm/dp: add connector abstraction for DP MST



On 2025/6/19 19:33, Dmitry Baryshkov wrote:
> [initially I responded off-list by mistake, sorry for the confusion and 
> possible duplicates]
> 
> On 19/06/2025 12:26, Yongxing Mou wrote:
>>
>>
>> On 2025/6/16 22:41, Dmitry Baryshkov wrote:
>>> On 16/06/2025 17:09, Yongxing Mou wrote:
>>>>
>>>>
>>>> On 2025/6/11 22:31, Dmitry Baryshkov wrote:
>>>>> On Wed, Jun 11, 2025 at 08:06:28PM +0800, Yongxing Mou wrote:
>>>>>>
>>>>>>
>>>>>> On 2025/6/9 23:44, Dmitry Baryshkov wrote:
>>>>>>> On Mon, Jun 09, 2025 at 08:21:48PM +0800, Yongxing Mou wrote:
>>>>>>>> From: Abhinav Kumar <quic_abhinavk@...cinc.com>
>>>>>>>>
>>>>>>>> Add connector abstraction for the DP MST. Each MST encoder
>>>>>>>> is connected through a DRM bridge to a MST connector and each
>>>>>>>> MST connector has a DP panel abstraction attached to it.
>>>>>>>>
>>>>>>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@...cinc.com>
>>>>>>>> Signed-off-by: Yongxing Mou <quic_yongmou@...cinc.com>
>>>>>>>> ---
>>>>>>>>    drivers/gpu/drm/msm/dp/dp_mst_drm.c | 515 +++++++++++++++++++ 
>>>>>>>> + + + ++++++++++++++
>>>>>>>>    drivers/gpu/drm/msm/dp/dp_mst_drm.h |   3 +
>>>>>>>>    2 files changed, 518 insertions(+)
>>>>>>>
>>>>>>> It generally feels liks 80% of this patch is a generic code. Please
>>>>>>> extract generic DP MST connector and push it under drm/display. 
>>>>>>> Other DP
>>>>>>> MST drivers should be able to use it.
>>>>>>>
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_mst_drm.c b/drivers/gpu/ 
>>>>>>>> drm/ msm/dp/dp_mst_drm.c
>>>>>>>> index 
>>>>>>>> a3ea34ae63511db0ac920cbeebe30c4e2320b8c4..489fa46aa518ff1cc5f4769b2153fc5153c4cb41 100644
>>>>>>>> --- a/drivers/gpu/drm/msm/dp/dp_mst_drm.c
>>>>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_mst_drm.c
>>>>>>>> @@ -25,8 +25,12 @@
>>>>>>>>     * OF THIS SOFTWARE.
>>>>>>>>     */
>>>>>>>> +#include <drm/drm_edid.h>
>>>>>>>> +#include <drm/drm_managed.h>
>>>>>>>>    #include "dp_mst_drm.h"
>>>>>>>> +#define MAX_DPCD_TRANSACTION_BYTES 16
>>>>>>>> +
>>>>>>>>    static struct drm_private_state 
>>>>>>>> *msm_dp_mst_duplicate_bridge_state(struct drm_private_obj *obj)
>>>>>>>>    {
>>>>>>>>        struct msm_dp_mst_bridge_state *state;
>>>>>>>> @@ -79,6 +83,61 @@ static int msm_dp_mst_find_vcpi_slots(struct 
>>>>>>>> drm_dp_mst_topology_mgr *mgr, int p
>>>>>>>>        return num_slots;
>>>>>>>>    }
>>>>>>>> +static int msm_dp_mst_get_mst_pbn_div(struct msm_dp_panel 
>>>>>>>> *msm_dp_panel)
>>>>>>>> +{
>>>>>>>> +    struct msm_dp_link_info *link_info;
>>>>>>>> +
>>>>>>>> +    link_info = &msm_dp_panel->link_info;
>>>>>>>> +
>>>>>>>> +    return link_info->rate * link_info->num_lanes / 54000;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static int msm_dp_mst_compute_config(struct drm_atomic_state 
>>>>>>>> *state,
>>>>>>>> +                      struct msm_dp_mst *mst, struct 
>>>>>>>> drm_connector *connector,
>>>>>>>> +                      struct drm_display_mode *mode)
>>>>>>>> +{
>>>>>>>> +    int slots = 0, pbn;
>>>>>>>> +    struct msm_dp_mst_connector *mst_conn = 
>>>>>>>> to_msm_dp_mst_connector(connector);
>>>>>>>> +    int rc = 0;
>>>>>>>> +    struct drm_dp_mst_topology_state *mst_state;
>>>>>>>> +    int pbn_div;
>>>>>>>> +    struct msm_dp *dp_display = mst->msm_dp;
>>>>>>>> +    u32 bpp;
>>>>>>>> +
>>>>>>>> +    bpp = connector->display_info.bpc * 3;
>>>>>>>> +
>>>>>>>> +    pbn = drm_dp_calc_pbn_mode(mode->clock, bpp << 4);
>>>>>>>
>>>>>>> Is this going to change if DSC is in place? Will it bring 
>>>>>>> fractional BPP
>>>>>>> here?
>>>>>>>
>>>>>> Actually, in this patch series, MST not support DSC. So we just don't
>>>>>> consider this scenario.
>>>>>
>>>>> But you still can answer the question.
>>>>>
>>>>>
>>>>> [...]
>>>>>
>>>> 1.Emm, for my current understanding, if DSC is enabled, the BPP 
>>>> should change and recaculated.
>>>> Will it bring fractional BPP here?
>>>
>>> That's what I am asking
>>>
>>>>  >>>I'm not entirely sure about this answer. I checked how other 
>>>> drivers call this function, and they all use bpp << 4, so can we 
>>>> assume that this way of calling it is valid?
>>>
>>> It is valid. I'm trying to understand the implications and future 
>>> changes.
>>>
>>>>>>>> +
>>>>>>>> +    return msm_dp_display_mode_valid(dp_display, &dp_display- 
>>>>>>>> >connector->display_info, mode);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static struct drm_encoder *
>>>>>>>> +msm_dp_mst_atomic_best_encoder(struct drm_connector *connector, 
>>>>>>>> struct drm_atomic_state *state)
>>>>>>>
>>>>>>> Do we need this callback? Don't we have a fixed relationship between
>>>>>>> connectors and encoders?
>>>>>
>>>>> This was left unanswered.
>>>>>
>>>> Sorry, I didn't mean to skip any questions — I just planned to reply 
>>>> a bit later. Apologies for the confusion.
>>>> For this question, yes , we don't have the fixed relationship 
>>>> between them. Under the current codes, the Connector selects the 
>>>> available encoder and bridge in order from index 0 to 4 (up to 
>>>> max_streams) when the connector's status changes to 'connected'.
>>>
>>> Why? Can we have 1:1 relationship as we do with other bridges?
>>>
>> Emm, It may be because the number of MST connectors is not fixed, but 
>> rather determined by the number of output ports on the dongle. For 
>> example, in a 2-MST case, there are 2 encoders, but there could be 
>> four MST connectors if the dongle has four DP output ports. 
>> add_connector() creates MST connectors based on the number of outputs 
>> on the dongle, rather than the actual number of connected devices.
> 
> Ack, this should be a part of the commit message.
> 
>>>>>>>
>>>>>>>> +{
>>>>>>>> +    struct msm_dp_mst_connector *mst_conn = 
>>>>>>>> to_msm_dp_mst_connector(connector);
>>>>>>>> +    struct msm_dp *dp_display = mst_conn->msm_dp;
>>>>>>>> +    struct msm_dp_mst *mst = dp_display->msm_dp_mst;
>>>>>>>> +    struct drm_encoder *enc = NULL;
>>>>>>>> +    struct msm_dp_mst_bridge_state *bridge_state;
>>>>>>>> +    u32 i;
>>>>>>>> +    struct drm_connector_state *conn_state = 
>>>>>>>> drm_atomic_get_new_connector_state(state,
>>>>>>>> +                                            connector);
>>>>>>>> +
>>>>>>>
>>>>>
>>>>> [...]
>>>>>
>>>>>>>> +    if (drm_atomic_crtc_needs_modeset(crtc_state)) {
>>>>>>>> +        if (WARN_ON(!old_conn_state->best_encoder)) {
>>>>>>>> +            rc = -EINVAL;
>>>>>>>> +            goto end;
>>>>>>>> +        }
>>>>>>>> +
>>>>>>>> +        drm_bridge = 
>>>>>>>> drm_bridge_chain_get_first_bridge(old_conn_state->best_encoder);
>>>>>>>
>>>>>>> This really looks like this should be a bridge's callback.
>>>>>
>>>>> And this one
>>>>>
>>>> Emm, the bridge does not implement atomic_check(). All MST-related 
>>>> checks (such as drm_dp_atomic_release_time_slots, 
>>>> drm_dp_mst_atomic_check, or others) are performed in the connector's 
>>>> atomic_check function. I believe this is because both num_slots and 
>>>> pbn are stored in the bridge, and we call this to get the drm_bridge..
>>>
>>> So, please split them into connector and bridge checks, calling them 
>>> from corresponding hooks. It might be easier to migrate completely to 
>>> the bridge's atomic_check(). At least it will save us from this 
>>> clumsy code getting the bridge for the connector.
>>>
>> Maybe we don't need to move to bridge's atomic_check(). Connector's 
>> atomic_check() do 2 things: 1.Call drm_dp_atomic_release_time_slots 
>> when unplug, 2. Call drm_dp_atomic_find_time_slots and 
>> drm_dp_mst_atomic_check when plug in.
> 
> Actually... I don't think you are calling it in a correct way. It should 
> be called from the drm_mode_config.atomic_check, not from connector's 
> atomic_check(). See how nouveau driver does it. Even documentation 
> insists that it should be called _after_ checking the rest of the state.
> 
In the documentation, drm_dp_atomic_find_time_slots should be placed in 
drm_encoder_helper_funcs.atomic_check(), 
drm_dp_atomic_release_time_slots in 
&drm_connector_helper_funcs.atomic_check(), and drm_dp_mst_atomic_check 
in &drm_mode_config_funcs.atomic_check(). So maybe we can move these 
atomic_check() calls back to their original positions as do, as 
recommended in the documenttation.
>> 3 functions need drm_atomic_state, but it seems that 
>> drm_bridge_funcs.atomic_check() does not pass in drm_atomic_state. 
> 
> You can get drm_atomic_state from bridge_state->base.state, crtc_state- 
>  >state, connector_state->state, that's not really an issue.
> 
>> Actually both 2 actions only occur when need modeset. Maybe we can 
>> optimize this function in the following ways: (1) reduce unnecessary 
>> references to drm_bridge, especially since bridge_state-  >num_slots 
>> can replace with payload->time_slots; (2)As your comments, split the 
>> function into smaller parts to better reflect the logic.
> 
> Yes, please. Getting rid of bridge_state->num_slots is a good path forward.
> 
Emm, even if we can drop bridge_state->num_slots, we still need to 
access bridge_state when clearing bridge_state->connector and 
bridge_state->msm_dp_panel, so it might not be possible to completely 
eliminate the use of bridge_state here.
>>>
>>>>>>>
>>>>>>>> +        if (WARN_ON(!drm_bridge)) {
>>>>>>>> +            rc = -EINVAL;
>>>>>>>> +            goto end;
>>>>>>>> +        }
>>>>>>>> +        bridge = to_msm_dp_mst_bridge(drm_bridge);
>>>>>>>> +
>>>>>>>> +        bridge_state = msm_dp_mst_br_priv_state(state, bridge);
>>>>>>>> +        if (IS_ERR(bridge_state)) {
>>>>>>>> +            rc = PTR_ERR(bridge_state);
>>>>>>>> +            goto end;
>>>>>>>> +        }
>>>>>>>> +
>>>>>>>> +        if (WARN_ON(bridge_state->connector != connector)) {
>>>>>>>> +            rc = -EINVAL;
>>>>>>>> +            goto end;
>>>>>>>> +        }
>>>>>>>> +
>>>>>>>> +        slots = bridge_state->num_slots;
>>>>>>>> +        if (slots > 0) {
>>>>>>>> +            rc = drm_dp_atomic_release_time_slots(state,
>>>>>>>> +                                  &mst->mst_mgr,
>>>>>>>> +                                  mst_conn->mst_port);
>>>>>>>> +            if (rc) {
>>>>>>>> +                DRM_ERROR("failed releasing %d vcpi slots 
>>>>>>>> %d\n", slots, rc);
>>>>>>>> +                goto end;
>>>>>>>> +            }
>>>>>>>> +            vcpi_released = true;
>>>>>>>> +        }
>>>>>>>> +
>>>>>>>> +        if (!new_conn_state->crtc) {
>>>>>>>> +            /* for cases where crtc is not disabled the slots 
>>>>>>>> are not
>>>>>>>> +             * freed by drm_dp_atomic_release_time_slots. this 
>>>>>>>> results
>>>>>>>> +             * in subsequent atomic_check failing since 
>>>>>>>> internal slots
>>>>>>>> +             * were freed but not the dp mst mgr's
>>>>>>>> +             */
>>>>>>>> +            bridge_state->num_slots = 0;
>>>>>>>> +            bridge_state->connector = NULL;
>>>>>>>> +            bridge_state->msm_dp_panel = NULL;
>>>>>>>> +
>>>>>>>> +            drm_dbg_dp(dp_display->drm_dev, "clear best 
>>>>>>>> encoder: %d\n", bridge->id);
>>>>>>>> +        }
>>>>>>>> +    }
>>>>>>>
>>>>>>> This looks like there are several functions fused together. Please
>>>>>>> unfuse those into small and neat code blocks.
>>>>>
>>>>> And this :-D
>>>>>
>>>> Got it.. this code only do one thing, check and try to release 
>>>> time_slots.. we can try to package it into small functions..
>>>
>>> I still don't understand, why do we need to release time_slots here 
>>> instead of using MST helpers.
>>>
>> Emm , just as above reply.. when we need modeset, call 
>> drm_dp_atomic_release_time_slots to release payload and then clear 
>> bridge_state cached ..
> 
> I don't see other drivers limiting drm_dp_atomic_release_time_slots() to 
> the modeset case. Any reason for MSM driver to deviate from that?
> 
Actually, you are right. I think the drm_dp_atomic_release_time_slots 
function can handle the slots releases quite well by itself. This 
function can handle various changes in crtc_state quite well. These 
constraints are more about supporting the cleanup of 
bridge_state->connector and bridge_state->msm_dp_panel.
> 
> 
>>>>>>>
>>>>>>>> +
>>>>>>>> +mode_set:
>>>>>>>> +    if (!new_conn_state->crtc)
>>>>>>>> +        goto end;
>>>>>>>> +
>>>>>>>> +    crtc_state = drm_atomic_get_new_crtc_state(state, 
>>>>>>>> new_conn_state->crtc);
>>>>>>>> +
>>>>>>>> +    if (drm_atomic_crtc_needs_modeset(crtc_state) && 
>>>>>>>> crtc_state- >active) {
>>>>>>>
>>>>>>> Use of crtc_state->active doesn't look correct.
>>>>>
>>>>>
>>>>> ...
>>>>>
>>>> Sorry, I'm still not quite sure where the issue is. Could you please 
>>>> help point it out? Thanks~~
>>>
>>>
>>> Please refer to the documentation for drm_crtc_state::active. The 
>>> drivers are not supposed to use this field in checks.
>>>
>> Got it , so maybe drm_crtc_state::enable might more appropriate here..
> 
> Well, why do you need it in the first place? This will determine a 
> correct set of conditions.
> 
Got it. Let me look into whether this can be optimized.
> 
>>>>>>>
>>>>>>>> +        if (WARN_ON(!new_conn_state->best_encoder)) {
>>>>>>>> +            rc = -EINVAL;
>>>>>>>> +            goto end;
>>>>>>>> +        }
>>>>>>>> +
>>>>>>>> +        drm_bridge = 
>>>>>>>> drm_bridge_chain_get_first_bridge(new_conn_state->best_encoder);
>>>>>>>> +        if (WARN_ON(!drm_bridge)) {
>>>>>>>> +            rc = -EINVAL;
>>>>>>>> +            goto end;
>>>>>>>> +        }
>>>>>>>> +        bridge = to_msm_dp_mst_bridge(drm_bridge);
>>>>>>>> +
>>>>>>>> +        bridge_state = msm_dp_mst_br_priv_state(state, bridge);
>>>>>>>> +        if (IS_ERR(bridge_state)) {
>>>>>>>> +            rc = PTR_ERR(bridge_state);
>>>>>>>> +            goto end;
>>>>>>>> +        }
>>>>>>>> +
>>>>>>>> +        if (WARN_ON(bridge_state->connector != connector)) {
>>>>>>>> +            rc = -EINVAL;
>>>>>>>> +            goto end;
>>>>>>>> +        }
>>>>>>>
>>>>>>> Can all of this actually happen?
>>>>>
>>>>> ...
>>>>>
>>>> Actually not, I haven't encountered it yet. I'm not sure how to 
>>>> trigger it, but it might occur under race conditions? Or we just 
>>>> remove it untill some case it really happen..
>>>
>>> No. You actually think whether this condition can happen, then keep 
>>> it if it can (and drop it if it can not happen).
>>>
>> Got it. Let me test a few different cases to see if these scenarios 
>> occur.
> 
> No. It's not about testing. It's about asserting if the scenario can 
> occur or not per your call stacks and per the design / contract.
> 
Got it. Let me check this part again. I don't think these errors would 
occur under the current design. But if the system enters a  inconsistent 
error state, this code could help improve our stability.
> 

Powered by blists - more mailing lists