Re: [PATCH RESEND v5 24/25] drm/msm/dp: add connector abstraction for DP MST

On 7/13/2026 8:52 AM, Dmitry Baryshkov wrote:
> 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++) {
I think your comment got truncated. Could you please resend it?

>> +		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?
> 
Yes, when we unplug, here mst_active is false.
>> +		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?
> 
See here: 
https://elixir.bootlin.com/linux/v7.2-rc7/source/drivers/gpu/drm/display/drm_dp_mst_topology.c#L4511

drm_dp_atomic_find_time_slots called in msm_dp_mst_stream_atomic_check().
>> +}
>> +
>> +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
>>
>