On Mon, Jun 29, 2026 at 10:14:45PM +0800, Yongxing Mou wrote: > From: Abhinav Kumar <[email protected]> > > Introduce an MST connector abstraction for DP MST, with each MST > connector associated with a DP panel and connected through a DRM bridge > to an MST encoder. > > The connector is only used for MST helper callbacks, such as detect, > get_modes, and get_encoder. Display enable/disable, hotplug handling, > and modeset sequencing continue to be handled by the bridge path.
If it is so generic, can it be completely generic? Can we move it to the drm/display/* helper code? Or, at least, leverage significant parts of it there, providing necessary callbacks from the MSM code? It feels that only the connector -> encoder part is not generic enough. > > Signed-off-by: Abhinav Kumar <[email protected]> > Signed-off-by: Yongxing Mou <[email protected]> > --- > drivers/gpu/drm/msm/dp/dp_mst_drm.c | 232 > ++++++++++++++++++++++++++++++++++++ > 1 file changed, 232 insertions(+) > > diff --git a/drivers/gpu/drm/msm/dp/dp_mst_drm.c > b/drivers/gpu/drm/msm/dp/dp_mst_drm.c > index 6a77fdef85e9..12b47a413793 100644 > --- a/drivers/gpu/drm/msm/dp/dp_mst_drm.c > +++ b/drivers/gpu/drm/msm/dp/dp_mst_drm.c > @@ -7,6 +7,7 @@ > #include <drm/drm_fixed.h> > #include <drm/drm_atomic_helper.h> > #include <drm/display/drm_dp_mst_helper.h> > +#include <linux/pm_runtime.h> > > #include "dp_mst_drm.h" > #include "dp_panel.h" > @@ -50,6 +51,18 @@ static struct msm_dp_panel > *msm_dp_mst_panel_from_encoder(struct msm_dp_mst *mst > return NULL; > } > > +static int msm_dp_mst_encoder_stream_id(struct msm_dp_mst *mst, > + struct drm_encoder *enc) > +{ > + int i; > + Ca> + for (i = 0; i < mst->max_streams; i++) { > + if (mst->mst_encoders[i].enc == enc) > + return mst->mst_encoders[i].stream_id; > + } > + return -1; > +} > + > static void msm_dp_mst_update_timeslots(struct msm_dp_mst *mst, > struct msm_dp_panel *panel, > struct drm_dp_mst_atomic_payload > *payload) > @@ -275,6 +288,224 @@ int msm_dp_mst_display_set_mgr_state(struct msm_dp > *dp_display, bool state) > return rc; > } > > +/* DP MST Connector OPs */ > +static int > +msm_dp_mst_connector_detect(struct drm_connector *connector, > + struct drm_modeset_acquire_ctx *ctx, > + bool force) > +{ > + struct msm_dp_mst_connector *mst_conn = to_dp_mst_connector(connector); > + struct msm_dp_mst *mst = mst_conn->dp_mst; > + struct msm_dp *dp_display = mst->msm_dp; > + struct device *dev = dp_display->drm_dev->dev; > + enum drm_connector_status status = connector_status_disconnected; > + int ret; > + > + ret = pm_runtime_resume_and_get(dev); > + if (ret < 0) > + return status; > + > + if (dp_display->mst_active) Can it be inactive here? > + status = drm_dp_mst_detect_port(connector, > + ctx, &mst->mst_mgr, > mst_conn->mst_port); > + > + pm_runtime_put_autosuspend(dev); > + > + return status; > +} > + > +static int msm_dp_mst_connector_get_modes(struct drm_connector *connector) > +{ > + struct msm_dp_mst_connector *mst_conn = to_dp_mst_connector(connector); > + struct msm_dp_mst *mst = mst_conn->dp_mst; > + const struct drm_edid *drm_edid; > + int rc; > + > + drm_edid = drm_dp_mst_edid_read(connector, &mst->mst_mgr, > mst_conn->mst_port); > + drm_edid_connector_update(connector, drm_edid); > + > + rc = drm_edid_connector_add_modes(connector); > + > + drm_edid_free(drm_edid); > + > + return rc; > +} > + > +static enum drm_mode_status msm_dp_mst_connector_mode_valid(struct > drm_connector *connector, > + const struct > drm_display_mode *mode) > +{ > + struct msm_dp_mst_connector *mst_conn; > + struct drm_dp_mst_port *mst_port; > + struct msm_dp *dp_display; > + int required_pbn; > + > + if (drm_connector_is_unregistered(connector)) > + return 0; > + > + mst_conn = to_dp_mst_connector(connector); > + mst_port = mst_conn->mst_port; > + dp_display = mst_conn->dp_mst->msm_dp; > + > + /* FIXME: use negotiated bpp (DSC, YUV 4:2:0, etc.); for now use You can't use negotiated bpp, because it well might be that the negoation didn't happen. Anyway, mode_valid should be returning the modes which are even theoretically not possible to be set. > + * 18bpp (6bpc) as a conservative lower bound like i915/nouveau. > + */ > + required_pbn = drm_dp_calc_pbn_mode(mode->clock, (6 * 3) << 4); > + > + if (required_pbn > mst_port->full_pbn) { > + drm_dbg_dp(dp_display->drm_dev, "mode:%s not supported.\n", > mode->name); > + return MODE_CLOCK_HIGH; > + } > + > + return msm_dp_display_mode_valid(dp_display, &connector->display_info, > mode); > +} > + > +static const struct drm_connector_funcs msm_dp_drm_mst_connector_funcs; > + > +static struct drm_encoder * > +msm_dp_mst_atomic_best_encoder(struct drm_connector *connector, struct > drm_atomic_commit *state) > +{ > + struct msm_dp_mst_connector *mst_conn = to_dp_mst_connector(connector); > + struct msm_dp_mst *mst = mst_conn->dp_mst; > + struct drm_connector_state *conn_state; > + struct drm_connector *iter; > + struct drm_connector_list_iter conn_iter; > + u32 stream_mask = 0; > + u32 i; > + > + conn_state = drm_atomic_get_new_connector_state(state, connector); > + if (!conn_state) > + return NULL; > + > + if (conn_state->best_encoder) > + return conn_state->best_encoder; > + > + drm_connector_list_iter_begin(connector->dev, &conn_iter); > + drm_for_each_connector_iter(iter, &conn_iter) { > + struct drm_connector_state *peer_state; > + int stream_id; > + > + if (iter == connector || > + iter->funcs != &msm_dp_drm_mst_connector_funcs || > + to_dp_mst_connector(iter)->dp_mst != mst) > + continue; > + > + peer_state = drm_atomic_get_new_connector_state(state, iter) ?: > iter->state; > + if (!peer_state || !peer_state->crtc || > !peer_state->best_encoder) > + continue; > + > + stream_id = msm_dp_mst_encoder_stream_id(mst, > peer_state->best_encoder); > + if (stream_id >= 0 && stream_id < mst->max_streams) > + stream_mask |= BIT(stream_id); > + } > + drm_connector_list_iter_end(&conn_iter); > + > + for (i = 0; i < mst->max_streams; i++) { > + if (!(stream_mask & BIT(i))) { > + conn_state->best_encoder = mst->mst_encoders[i].enc; > + return mst->mst_encoders[i].enc; > + } > + } > + > + return NULL; > +} > + > +static int msm_dp_mst_connector_atomic_check(struct drm_connector *connector, > + struct drm_atomic_commit *state) > +{ > + struct msm_dp_mst_connector *mst_conn = to_dp_mst_connector(connector); > + struct msm_dp_mst *mst = mst_conn->dp_mst; > + > + return drm_dp_atomic_release_time_slots(state, &mst->mst_mgr, > mst_conn->mst_port); Why is it being called here? Don't we need to perform slot allocation too? > +} > + > +static void dp_mst_connector_destroy(struct drm_connector *connector) > +{ > + struct msm_dp_mst_connector *mst_conn = to_dp_mst_connector(connector); > + > + drm_connector_cleanup(connector); > + drm_dp_mst_put_port_malloc(mst_conn->mst_port); > + kfree(mst_conn); > +} > + > +/* 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 *mst = container_of(mgr, struct msm_dp_mst, mst_mgr); > + struct drm_device *dev = mst->msm_dp->drm_dev; > + struct msm_dp_mst_connector *mst_conn; > + struct drm_connector *connector; > + int rc, i; > + > + mst_conn = kzalloc_obj(*mst_conn); > + if (!mst_conn) > + return NULL; > + > + connector = &mst_conn->connector; > + rc = drm_connector_dynamic_init(dev, connector, > + &msm_dp_drm_mst_connector_funcs, > + DRM_MODE_CONNECTOR_DisplayPort, NULL); > + if (rc) > + goto err_free; > + > + mst_conn->dp_mst = mst; > + > + drm_connector_helper_add(connector, > &msm_dp_drm_mst_connector_helper_funcs); > + connector->funcs->reset(connector); > + > + /* add all encoders as possible encoders */ > + for (i = 0; i < mst->max_streams; i++) { > + rc = drm_connector_attach_encoder(connector, > mst->mst_encoders[i].enc); > + if (rc) { > + drm_err(dev, "[MST] failed to attach encoder:%u to > conn:%d rc:%d\n", > + mst->mst_encoders[i].enc->base.id, > + connector->base.id, rc); > + goto err_connector; > + } > + } > + > + mst_conn->mst_port = port; > + drm_dp_mst_get_port_malloc(port); > + > + drm_object_attach_property(&connector->base, > + dev->mode_config.path_property, 0); > + drm_object_attach_property(&connector->base, > + dev->mode_config.tile_property, 0); > + drm_connector_set_path_property(connector, pathprop); > + > + drm_dbg_kms(dev, "[MST] add_connector done conn:%d max_streams:%u\n", > + connector->base.id, mst->max_streams); > + > + return connector; > + > +err_connector: > + drm_connector_cleanup(connector); > +err_free: > + kfree(mst_conn); > + return NULL; > +} > + > +static const struct drm_dp_mst_topology_cbs msm_dp_mst_drm_cbs = { > + .add_connector = msm_dp_mst_add_connector, > +}; > + > int msm_dp_mst_init(struct msm_dp *dp_display, u32 max_streams, struct > drm_dp_aux *drm_aux) > { > struct drm_device *dev = dp_display->drm_dev; > @@ -285,6 +516,7 @@ int msm_dp_mst_init(struct msm_dp *dp_display, u32 > max_streams, struct drm_dp_au > if (!mst) > return -ENOMEM; > > + mst->mst_mgr.cbs = &msm_dp_mst_drm_cbs; > mst->msm_dp = dp_display; > mst->max_streams = max_streams; > mst->dp_aux = drm_aux; > > -- > 2.43.0 > -- With best wishes Dmitry
