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_abhin...@quicinc.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_abhin...@quicinc.com>
> > > Signed-off-by: Yongxing Mou <quic_yong...@quicinc.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.
[...]
> > > +
> > > + 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.
> >
> > > +{
> > > + 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
> >
> > > + 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
> >
> > > +
> > > +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.
...
> >
> > > + 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?
...
> >
> > > +
> > > + /*
> > > + * check if vcpi slots are trying to get allocated in same phase
> > > + * as deallocation. If so, go to end to avoid allocation.
> > > + */
> > > + if (vcpi_released) {
> > > + drm_dbg_dp(dp_display->drm_dev,
> > > + "skipping allocation since vcpi was released
> > > in the same state\n");
> > > + goto end;
> > > + }
> > > +
> > > + if (WARN_ON(bridge_state->num_slots)) {
> > > + rc = -EINVAL;
> > > + goto end;
> > > + }
> > > +
> > > + slots = msm_dp_mst_compute_config(state, mst, connector,
> > > &crtc_state->mode);
> > > + if (slots < 0) {
> > > + rc = slots;
> > > + goto end;
> > > + }
> > > +
> > > + bridge_state->num_slots = slots;
> > > + }
> > > +
> > > +end:
> > > + drm_dbg_dp(dp_display->drm_dev, "mst connector:%d atomic check ret
> > > %d\n",
> > > + connector->base.id, rc);
> > > + return rc;
> > > +}
> > > +
> > > +static void dp_mst_connector_destroy(struct drm_connector *connector)
> > > +{
> > > + struct msm_dp_mst_connector *mst_conn =
> > > to_msm_dp_mst_connector(connector);
> > > +
> > > + drm_connector_cleanup(connector);
> > > + drm_dp_mst_put_port_malloc(mst_conn->mst_port);
> > > +}
> > > +
> > > +/* DRM MST callbacks */
> > > +static const struct drm_connector_helper_funcs
> > > msm_dp_drm_mst_connector_helper_funcs = {
> > > + .get_modes = msm_dp_mst_connector_get_modes,
> > > + .detect_ctx = msm_dp_mst_connector_detect,
> > > + .mode_valid = msm_dp_mst_connector_mode_valid,
> > > + .atomic_best_encoder = msm_dp_mst_atomic_best_encoder,
> > > + .atomic_check = msm_dp_mst_connector_atomic_check,
> > > +};
> > > +
> > > +static const struct drm_connector_funcs msm_dp_drm_mst_connector_funcs =
> > > {
> > > + .reset = drm_atomic_helper_connector_reset,
> > > + .destroy = dp_mst_connector_destroy,
> > > + .fill_modes = drm_helper_probe_single_connector_modes,
> > > + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> > > + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> > > +};
> > > +
> > > +static struct drm_connector *
> > > +msm_dp_mst_add_connector(struct drm_dp_mst_topology_mgr *mgr,
> > > + struct drm_dp_mst_port *port, const char *pathprop)
> > > +{
> > > + struct msm_dp_mst *dp_mst;
> > > + struct drm_device *dev;
> > > + struct msm_dp *dp_display;
> > > + struct msm_dp_mst_connector *mst_connector;
> > > + struct drm_connector *connector;
> > > + int rc, i;
> > > +
> > > + dp_mst = container_of(mgr, struct msm_dp_mst, mst_mgr);
> > > +
> > > + dp_display = dp_mst->msm_dp;
> > > + dev = dp_display->drm_dev;
> > > +
> > > + mst_connector = devm_kzalloc(dev->dev, sizeof(*mst_connector),
> > > GFP_KERNEL);
> >
> > This shows that somebody doesn't understand the reason for drmm and the
> > difference between devm and drmm and the lifetime of the objects. Do you
> > see two issues in this line?
> >
> > Let me help you. Please use normal (non-managed) memory here. It is the
> > only correct way to allocate memory for MST connectors.
> >
> Thanks for point it.. it will lead to mem leak.. so we need to use
> kzalloc()...
- Did you understand why devm is unsuitable here?
- Why drmm is also unsutable?
- What is the implication of using kzalloc() here?
> > > +
> > > + drm_modeset_lock_all(dev);
> > > +
> > > + rc = drm_connector_dynamic_init(dev, &mst_connector->connector,
> > > + &msm_dp_drm_mst_connector_funcs,
> > > + DRM_MODE_CONNECTOR_DisplayPort, NULL);
> > > + if (rc) {
> > > + drm_modeset_unlock_all(dev);
> > > + return NULL;
> > > + }
> > > +
> > > + mst_connector->dp_panel = msm_dp_display_get_panel(dp_display);
> > > + if (!mst_connector->dp_panel) {
> > > + DRM_ERROR("failed to get dp_panel for connector\n");
> > > + drm_modeset_unlock_all(dev);
> > > + return NULL;
> > > + }
> > > +
> > > + mst_connector->dp_panel->connector = &mst_connector->connector;
> > > + mst_connector->msm_dp = dp_display;
> > > + connector = &mst_connector->connector;
> > > + drm_connector_helper_add(&mst_connector->connector,
> > > &msm_dp_drm_mst_connector_helper_funcs);
> > > +
> > > + if (connector->funcs->reset)
> > > + connector->funcs->reset(connector);
> > > +
> > > + /* add all encoders as possible encoders */
> > > + for (i = 0; i < dp_mst->max_streams; i++) {
> > > + rc = drm_connector_attach_encoder(&mst_connector->connector,
> > > +
> > > dp_mst->mst_bridge[i].encoder);
> > > + if (rc) {
> > > + DRM_ERROR("failed to attach encoder to connector,
> > > %d\n", rc);
> > > + drm_modeset_unlock_all(dev);
> > > + return NULL;
> > > + }
> > > + }
> > > +
> > > + mst_connector->mst_port = port;
> > > + drm_dp_mst_get_port_malloc(mst_connector->mst_port);
> > > +
> > > + drm_object_attach_property(&mst_connector->connector.base,
> > > + dev->mode_config.path_property, 0);
> > > + drm_object_attach_property(&mst_connector->connector.base,
> > > + dev->mode_config.tile_property, 0);
> >
> > subconnector? Or do we report the subconnector only for the main DP
> > port?
...
> >
> > > +
> > > + drm_modeset_unlock_all(dev);
> > > +
--
With best wishes
Dmitry
