Re: [PATCH v3 28/38] drm/msm/dp: add dp_mst_drm to manage DP MST bridge operations

[Dmitry Baryshkov]

On 01/04/2026 10:07, Yongxing Mou wrote:
> On 8/27/2025 1:36 AM, Dmitry Baryshkov wrote:
> > On Mon, Aug 25, 2025 at 10:16:14PM +0800, Yongxing Mou wrote:
> > > From: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>
> > >
> > > Add a new file dp_mst_drm to manage the DP MST bridge operations
> > > similar to the dp_drm file which manages the SST bridge operations.
> > > Each MST encoder creates one bridge and each bridge is bound to its
> > > own dp_panel abstraction to manage the operations of its pipeline.
> > >
> > > Signed-off-by: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>
> > > Signed-off-by: Yongxing Mou <yongxing.mou@xxxxxxxxxxxxxxxx>
> > > ---
> > >   drivers/gpu/drm/msm/Makefile        |   3 +-
> > >   drivers/gpu/drm/msm/dp/dp_display.h |   3 +
> > >   drivers/gpu/drm/msm/dp/dp_mst_drm.c | 556 +++++++++++++++++++++++++ 
> > > +++++++++++
> > >   drivers/gpu/drm/msm/dp/dp_mst_drm.h |  86 ++++++
> > >   4 files changed, 647 insertions(+), 1 deletion(-)

> > > +
> > > +struct msm_dp_mst_bridge {
> > > +    struct drm_bridge base;
> > > +    struct drm_private_obj obj;
> > > +    u32 id;
> > > +
> > > +    bool in_use;
> >
> > What does it mean? In use currently for one of outputs?
> >
> > > +
> > > +    struct msm_dp *display;
> > > +    struct drm_encoder *encoder;
> > > +
> > > +    struct drm_connector *connector;
> >
> > Why do you have connector both as a part of the bridge and bridge state?
> >
> > Please describe design decisions in the commit mesage or as comments.
> Emm. i go through the msm_dp_mst_bridge_state and msm_dp_mst_bridge.. 
> Can we just drop msm_dp_mst_bridge_state? just track connector and 
> anything else on bridge.

If something is being handled by atomic helpers or being computed by 
atomic_check() for later use, it should be a state. If not, it can be a 
part of the single bridge struct. The most obvious mistake is to set 
globals or object data in atomic_check().

> > > +    struct msm_dp_panel *msm_dp_panel;
> > > +};
> > > +
> > > +struct msm_dp_mst_bridge_state {
> > > +    struct drm_private_state base;
> > > +    struct drm_connector *connector;
> > > +    struct msm_dp_panel *msm_dp_panel;
> > > +
> > > +    int vcpi;
> > > +    int pbn;
> > > +    int num_slots;
> > > +    int start_slot;
> >
> > I'd definitely prefer to have payload pointer here, if that's also a
> > part of the state.
> >
> > > +};
> > > +
> > > +struct msm_dp_mst {
> > > +    struct drm_dp_mst_topology_mgr mst_mgr;
> > > +    struct msm_dp_mst_bridge *mst_bridge[DP_STREAM_MAX];
> > > +    struct msm_dp *msm_dp;
> > > +    u32 max_streams;
> > > +    struct mutex mst_lock;
> > > +};
> > > +
> > > +struct msm_dp_mst_connector {
> > > +    struct drm_connector connector;
> > > +    struct drm_dp_mst_port *mst_port;
> > > +    struct msm_dp *msm_dp;
> > > +    struct msm_dp_panel *dp_panel;
> > > +};
> > > +
> > > +int msm_dp_mst_drm_bridge_init(struct msm_dp *dp, struct drm_encoder 
> > > *encoder);
> > > +
> > > +#endif /* _DP_MST_DRM_H_ */
> > >
> > > --
> > > 2.34.1


--
With best wishes
Dmitry