Message-ID: <CAO9ioeXHqJ80y=07RyaRuPeWXu4OhjHKOznrWwunSu81EYGVEw@mail.gmail.com>
Date: Wed, 25 Jun 2025 01:25:05 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
To: Yongxing Mou <quic_yongmou@...cinc.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 Tue, 24 Jun 2025 at 12:57, Yongxing Mou <quic_yongmou@...cinc.com> wrote:
>
>
>
> 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(),

I'm not sure about this. Our encoders are implemented by the DPU
driver, so I'd rather not call MST functions from it.
You might call it from drm_bridge_funcs::atomic_check(), it is being
called at the same stage.

> 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.

These two are fine. Please move them to proper places.

> >> 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.

This is fine. We should drop state-related data from
msm_dp_mst_bridge, the msm_dp_mst_bridge_state can stay.
I wanted to get rid of unnecessary slots management in the MSM DP
driver, the state is fine to exist.

> >>>
> >>>>>>>
> >>>>>>>> +        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.

Can't we keep msm_dp_panel in the connector's state and access it by
getting the state by the connector?

> >
> >
> >>>>>>>
> >>>>>>>> +
> >>>>>>>> +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.

Then please eliminate a way for the system to enter an 'inconsistent
error state'.

-- 
With best wishes
Dmitry

Powered by blists - more mailing lists