Re: [PATCH RESEND v5 23/25] drm/msm/dp: add dp_mst_drm to manage DP MST encoder operations

[Yongxing Mou <yongxing.mou@oss.qualcomm.com>]

On 7/13/2026 8:40 AM, Dmitry Baryshkov wrote:
> On Mon, Jun 29, 2026 at 10:14:44PM +0800, Yongxing Mou wrote:
>> drm/msm/dp: introduce dp_mst_drm for MST stream management
> 
> duplicate subject?
> 
>>
>> Add a dp_mst_drm layer to manage DP MST streams with a clear ownership
>> model between encoder, panel and connector.
>>
>> Each MST stream is represented by a dedicated drm_encoder. At modeset
>> initialization time, one (encoder, dp_panel) pair is created per
>> stream_id and remains fixed for the lifetime of the driver. The
>> dp_panel thus carries a stable stream context, including stream_id
>> and pixel mapping.
>>
>> MST connectors are created and destroyed dynamically on hotplug and
>> are attached to a dp_panel through atomic routing. During an atomic
>> commit, connectors are associated with encoders via
>> atomic_best_encoder(), forming a temporary binding for the duration
>> of the commit.
> 
> This doesnt't seem to be true any longer.
> 
Got it. will modify commit message next version.
>>
>> Encoder helper callbacks drive the MST stream lifecycle, including
>> timeslot allocation, link enable/disable and payload programming.
>>
>> A per-MST-instance lock serializes operations on shared link state
>> across multiple streams.
>>
>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>> Signed-off-by: Yongxing Mou <yongxing.mou@oss.qualcomm.com>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c |   6 +
>>   drivers/gpu/drm/msm/dp/dp_display.c     |   9 ++
>>   drivers/gpu/drm/msm/dp/dp_display.h     |   2 +
>>   drivers/gpu/drm/msm/dp/dp_mst_drm.c     | 245 ++++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/msm/msm_drv.h           |   7 +
>>   5 files changed, 269 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> index b32ecd5b0777..ac5dc844fead 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> @@ -696,6 +696,12 @@ static int _dpu_kms_initialize_displayport(struct drm_device *dev,
>>   					DPU_ERROR("encoder init failed for dp mst display\n");
>>   					return PTR_ERR(encoder);
>>   				}
>> +
>> +				rc = msm_dp_mst_attach_encoder(priv->kms->dp[i], encoder);
>> +				if (rc) {
>> +					DPU_ERROR("dp_mst attach_encoder failed, rc = %d\n", rc);
>> +					return rc;
>> +				}
> 
> Reoder, this should be a part of the patch, adding the encoder.
> 
Got it.
>>   			}
>>   		}
>>   	}
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
>> index fc9c1e3e57ab..6eac390af2e0 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -601,6 +601,15 @@ struct msm_dp_panel *msm_dp_display_get_panel(struct msm_dp *msm_dp_display,
>>   	return dp_panel;
>>   }
>>   
>> +void msm_dp_display_set_link_info(struct msm_dp *msm_dp_display,
>> +				  struct msm_dp_link_info *dst)
>> +{
>> +	struct msm_dp_display_private *dp =
>> +		container_of(msm_dp_display, struct msm_dp_display_private, msm_dp_display);
>> +
>> +	memcpy(dst, &dp->panel->link_info, sizeof(*dst));
>> +}
>> +
>>   static void msm_dp_display_deinit_sub_modules(struct msm_dp_display_private *dp)
>>   {
>>   	msm_dp_audio_put(dp->audio);
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h
>> index a185819ec57e..fb6bdd372b52 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.h
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.h
>> @@ -63,4 +63,6 @@ void msm_dp_display_unprepare(struct msm_dp *dp);
>>   
>>   struct msm_dp_panel *msm_dp_display_get_panel(struct msm_dp *msm_dp_display,
>>   					      enum msm_dp_stream_id stream_id);
>> +void msm_dp_display_set_link_info(struct msm_dp *msm_dp_display,
>> +				  struct msm_dp_link_info *dst);
>>   #endif /* _DP_DISPLAY_H_ */
>> diff --git a/drivers/gpu/drm/msm/dp/dp_mst_drm.c b/drivers/gpu/drm/msm/dp/dp_mst_drm.c
>> index 78b8dffe111b..6a77fdef85e9 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_mst_drm.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_mst_drm.c
>> @@ -4,18 +4,259 @@
>>    */
>>   
>>   #include <drm/drm_edid.h>
>> +#include <drm/drm_fixed.h>
>> +#include <drm/drm_atomic_helper.h>
>>   #include <drm/display/drm_dp_mst_helper.h>
>>   
>>   #include "dp_mst_drm.h"
>>   #include "dp_panel.h"
>> +#include "dpu_encoder.h"
>> +
>> +#define to_dp_mst_connector(x) \
>> +		container_of((x), struct msm_dp_mst_connector, connector)
>> +
>> +struct msm_dp_mst_encoder {
>> +	struct drm_encoder *enc;
>> +	int stream_id;
>> +	struct msm_dp_panel *dp_panel;
>> +};
>> +
>> +struct msm_dp_mst_connector {
>> +	struct drm_connector connector;
>> +	struct drm_dp_mst_port *mst_port;
>> +	struct msm_dp_mst *dp_mst;
>> +};
>> +
> 
> Extra empty line.
> 
>>   
>>   struct msm_dp_mst {
>>   	struct drm_dp_mst_topology_mgr mst_mgr;
>> +	struct msm_dp_mst_encoder mst_encoders[DP_STREAM_MAX];
>>   	struct msm_dp *msm_dp;
>>   	struct drm_dp_aux *dp_aux;
>>   	u32 max_streams;
>> +	struct mutex mst_lock;
>> +	struct msm_dp_link_info link_info;
>>   };
>>   
>> +static struct msm_dp_panel *msm_dp_mst_panel_from_encoder(struct msm_dp_mst *mst,
>> +							  struct drm_encoder *enc)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < mst->max_streams; i++) {
>> +		if (mst->mst_encoders[i].enc == enc)
>> +			return mst->mst_encoders[i].dp_panel;
>> +	}
>> +	return NULL;
>> +}
>> +
>> +static void msm_dp_mst_update_timeslots(struct msm_dp_mst *mst,
>> +					struct msm_dp_panel *panel,
>> +					struct drm_dp_mst_atomic_payload *payload)
>> +{
>> +	if (payload->vc_start_slot < 0)
>> +		msm_dp_display_set_stream_info(mst->msm_dp, panel, 1, 0, 0);
>> +	else
>> +		msm_dp_display_set_stream_info(mst->msm_dp, panel,
>> +					       payload->vc_start_slot,
>> +					       payload->time_slots, payload->pbn);
> 
> Can you pass payload as is? Then you don't need a wrapping function.
> 
Yes, we can directly use this.
>> +
>> +	drm_dbg_kms(mst->msm_dp->drm_dev,
>> +		    "[MST] stream:%u timeslots vc_start:%d slots:%d pbn:%d\n",
>> +		    panel->stream_id, payload->vc_start_slot,
>> +		    payload->time_slots, payload->pbn);
>> +}
>> +
>> +static void msm_dp_mst_stream_enable(struct drm_encoder *encoder,
>> +				     struct drm_atomic_commit *state)
>> +{
>> +	struct drm_connector *connector =
>> +		drm_atomic_get_new_connector_for_encoder(state, encoder);
>> +	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 msm_dp_panel *panel = msm_dp_mst_panel_from_encoder(mst, encoder);
>> +	struct drm_dp_mst_port *port = mst_conn->mst_port;
>> +	struct drm_dp_mst_topology_state *mst_state =
>> +		drm_atomic_get_new_mst_topology_state(state, &mst->mst_mgr);
>> +	struct drm_dp_mst_atomic_payload *payload =
>> +		drm_atomic_get_mst_payload_state(mst_state, port);
>> +	int rc;
>> +
>> +	panel->connector = connector;
> 
> The connector should be _get before assigning.
> 
Got it.
>> +
>> +	guard(mutex)(&mst->mst_lock);
>> +
>> +	rc = msm_dp_display_set_mode_helper(dp_display, state, encoder, panel);
>> +	if (rc) {
>> +		drm_err(dp_display->drm_dev,
>> +			"[MST] stream:%u set_mode failed rc=%d\n", panel->stream_id, rc);
>> +		goto out;
>> +	}
>> +
>> +	rc = msm_dp_display_prepare_link(dp_display);
>> +	if (rc) {
>> +		drm_err(dp_display->drm_dev,
>> +			"[MST] stream:%u prepare_link failed rc=%d\n", panel->stream_id, rc);
>> +		msm_dp_display_unprepare(dp_display);
>> +		goto out;
>> +	}
>> +
>> +	drm_dp_mst_update_slots(mst_state, DP_CAP_ANSI_8B10B);
>> +
>> +	rc = drm_dp_add_payload_part1(&mst->mst_mgr, mst_state, payload);
>> +	if (rc) {
>> +		drm_err(dp_display->drm_dev,
>> +			"[MST] payload allocation failure for conn:%d\n", connector->base.id);
>> +		msm_dp_display_unprepare(dp_display);
>> +		goto out;
>> +	}
>> +
>> +	msm_dp_mst_update_timeslots(mst, panel, payload);
>> +
>> +	msm_dp_display_enable_helper(dp_display, panel);
>> +
>> +	drm_dp_check_act_status(&mst->mst_mgr);
>> +
>> +	drm_dp_add_payload_part2(&mst->mst_mgr, payload);
>> +
>> +out:
>> +	drm_connector_get(connector);
>> +}
>> +
>> +static void msm_dp_mst_stream_disable(struct drm_encoder *encoder,
>> +				      struct drm_atomic_commit *state)
>> +{
>> +	struct drm_connector *connector = drm_atomic_get_old_connector_for_encoder(state, encoder);
>> +	struct msm_dp_mst_connector *mst_conn = to_dp_mst_connector(connector);
>> +	struct msm_dp_mst *mst = mst_conn->dp_mst;
>> +	struct msm_dp_panel *panel = msm_dp_mst_panel_from_encoder(mst, encoder);
>> +	struct drm_dp_mst_topology_state *old_mst_state =
>> +		drm_atomic_get_old_mst_topology_state(state, &mst->mst_mgr);
>> +	struct drm_dp_mst_topology_state *new_mst_state =
>> +		drm_atomic_get_new_mst_topology_state(state, &mst->mst_mgr);
>> +	struct drm_dp_mst_atomic_payload *old_payload =
>> +		drm_atomic_get_mst_payload_state(old_mst_state, mst_conn->mst_port);
>> +	struct drm_dp_mst_atomic_payload *new_payload =
>> +		drm_atomic_get_mst_payload_state(new_mst_state, mst_conn->mst_port);
>> +
>> +	guard(mutex)(&mst->mst_lock);
>> +
>> +	drm_dp_remove_payload_part1(&mst->mst_mgr, new_mst_state, new_payload);
>> +
>> +	drm_dp_remove_payload_part2(&mst->mst_mgr, new_mst_state, old_payload, new_payload);
>> +
>> +	msm_dp_mst_update_timeslots(mst, panel, new_payload);
>> +
>> +	msm_dp_display_disable_helper(mst->msm_dp, panel);
>> +
>> +	drm_dp_check_act_status(&mst->mst_mgr);
>> +}
>> +
>> +static void msm_dp_mst_stream_post_disable(struct drm_encoder *encoder,
>> +					   struct drm_atomic_commit *state)
>> +{
>> +	struct drm_connector *connector = drm_atomic_get_old_connector_for_encoder(state, encoder);
>> +	struct msm_dp_mst_connector *mst_conn = to_dp_mst_connector(connector);
>> +	struct msm_dp_mst *mst = mst_conn->dp_mst;
>> +	struct msm_dp_panel *panel = msm_dp_mst_panel_from_encoder(mst, encoder);
>> +
>> +	guard(mutex)(&mst->mst_lock);
>> +
>> +	msm_dp_display_atomic_post_disable_helper(mst->msm_dp, panel);
>> +
>> +	if (!mst->msm_dp->mst_active)
>> +		msm_dp_display_unprepare(mst->msm_dp);
>> +
>> +	panel->connector = NULL;
>> +
>> +	drm_connector_put(connector);
>> +}
>> +
>> +static int msm_dp_mst_enc_atomic_check(struct drm_encoder *enc,
>> +				       struct drm_crtc_state *crtc_state,
>> +				       struct drm_connector_state *conn_state)
>> +{
>> +	struct msm_dp_mst_connector *mst_conn = to_dp_mst_connector(conn_state->connector);
>> +	struct msm_dp_mst *mst = mst_conn->dp_mst;
>> +	struct drm_dp_mst_topology_state *mst_state;
>> +	int bpp, pbn, slots;
>> +
>> +	if (!conn_state->crtc)
>> +		return 0;
>> +
>> +	if (!drm_atomic_crtc_needs_modeset(crtc_state) || !crtc_state->active)
>> +		return 0;
>> +
>> +	bpp = (conn_state->connector->display_info.bpc * 3) ?: 24; /* fallback: assume 8bpc */
>> +	pbn = drm_dp_calc_pbn_mode(crtc_state->mode.clock, bpp << 4);
>> +
>> +	mst_state = drm_atomic_get_mst_topology_state(crtc_state->state, &mst->mst_mgr);
>> +	if (IS_ERR(mst_state))
>> +		return PTR_ERR(mst_state);
>> +
>> +	if (!dfixed_trunc(mst_state->pbn_div)) {
>> +		mst_state->pbn_div =
>> +			drm_dp_get_vc_payload_bw(mst->link_info.rate,
>> +						 mst->link_info.num_lanes);
>> +	}
>> +
>> +	slots = drm_dp_atomic_find_time_slots(crtc_state->state, &mst->mst_mgr,
>> +					      mst_conn->mst_port, pbn);
>> +	if (slots < 0)
>> +		return slots;
>> +
>> +	return 0;
>> +}
>> +
>> +static void msm_dp_mst_enc_atomic_enable(struct drm_encoder *enc,
>> +					 struct drm_atomic_commit *state)
>> +{
>> +	msm_dp_mst_stream_enable(enc, state);
>> +	dpu_encoder_phys_enable(enc, state);
> 
> Looking at this, no, squashing the bridge and the encoder was not a good
> idea, excuse me. The encoder is an internal implement of the DPU layer
> and we should leave it there as is. DPU can use DP functions, but not
> other way around.
> 
We could also invoke it from dpu_encoder.c, similar to how the MST 
encoder helper functions are handled:
static const struct drm_encoder_helper_funcs 
dpu_mst_encoder_helper_funcs = {
	.atomic_check    = dpu_encoder_mst_atomic_check,
	.atomic_mode_set = dpu_encoder_virt_atomic_mode_set,
	.atomic_enable   = dpu_encoder_mst_atomic_enable,
	.atomic_disable  = dpu_encoder_mst_atomic_disable,
};
......
	if (drm_enc_mode == DRM_MODE_ENCODER_DPMST)
		drm_encoder_helper_add(&dpu_enc->base, &dpu_mst_encoder_helper_funcs);
..。。。。。
This looks much better.
>> +}
>> +
>> +static void msm_dp_mst_enc_atomic_disable(struct drm_encoder *enc,
>> +					  struct drm_atomic_commit *state)
>> +{
>> +	msm_dp_mst_stream_disable(enc, state);
>> +	dpu_encoder_phys_disable(enc, state);
>> +	msm_dp_mst_stream_post_disable(enc, state);
>> +}
>> +
>> +static const struct drm_encoder_helper_funcs msm_dp_mst_encoder_helper_funcs = {
>> +	.atomic_check    = msm_dp_mst_enc_atomic_check,
>> +	.atomic_mode_set = dpu_encoder_atomic_mode_set,
>> +	.atomic_enable   = msm_dp_mst_enc_atomic_enable,
>> +	.atomic_disable  = msm_dp_mst_enc_atomic_disable,
>> +};
>> +
>> +int msm_dp_mst_attach_encoder(struct msm_dp *dp_display, struct drm_encoder *encoder)
>> +{
>> +	struct msm_dp_mst *mst = dp_display->msm_dp_mst;
>> +	struct msm_dp_panel *dp_panel;
>> +	int i;
>> +
>> +	for (i = 0; i < mst->max_streams; i++) {
>> +		if (!mst->mst_encoders[i].enc)
>> +			break;
>> +	}
>> +
>> +	dp_panel = msm_dp_display_get_panel(dp_display, i);
>> +	if (!dp_panel) {
>> +		drm_err(dp_display->drm_dev,
>> +			"[MST] failed to allocate panel for stream %d\n", i);
>> +		return -ENOMEM;
>> +	}
>> +
>> +	mst->mst_encoders[i].enc = encoder;
>> +	mst->mst_encoders[i].stream_id = i;
>> +	mst->mst_encoders[i].dp_panel = dp_panel;
>> +	drm_encoder_helper_add(encoder, &msm_dp_mst_encoder_helper_funcs);
>> +
>> +	return 0;
>> +}
>> +
>>   int msm_dp_mst_display_set_mgr_state(struct msm_dp *dp_display, bool state)
>>   {
>>   	struct msm_dp_mst *mst = dp_display->msm_dp_mst;
>> @@ -27,6 +268,9 @@ int msm_dp_mst_display_set_mgr_state(struct msm_dp *dp_display, bool state)
>>   			"[MST] failed to set topology mgr state to %d rc:%d\n", state, rc);
>>   	}
>>   
>> +	if (state)
> 
> Why?
> 
At the moment, link_info is maintained in the SST panel. After link 
training is completed, we need to propagate a copy of it to the MST context.
>> +		msm_dp_display_set_link_info(dp_display, &mst->link_info);
>> +
>>   	drm_dbg_kms(dp_display->drm_dev, "[MST] set_mgr_state state:%d\n", state);
>>   	return rc;
>>   }
>> @@ -55,6 +299,7 @@ int msm_dp_mst_init(struct msm_dp *dp_display, u32 max_streams, struct drm_dp_au
>>   		return ret;
>>   	}
>>   
>> +	mutex_init(&mst->mst_lock);
>>   	dp_display->msm_dp_mst = mst;
>>   	return 0;
>>   }
>> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
>> index f71200a790f3..854dd08eede2 100644
>> --- a/drivers/gpu/drm/msm/msm_drv.h
>> +++ b/drivers/gpu/drm/msm/msm_drv.h
>> @@ -358,6 +358,8 @@ bool msm_dp_needs_periph_flush(const struct msm_dp *dp_display,
>>   bool msm_dp_wide_bus_available(const struct msm_dp *dp_display);
>>   int msm_dp_get_mst_max_stream(struct msm_dp *dp_display);
>>   int msm_dp_mst_register(struct msm_dp *dp_display);
>> +int msm_dp_mst_attach_encoder(struct msm_dp *dp_display, struct drm_encoder *encoder);
>> +
>>   #else
>>   static inline int __init msm_dp_register(void)
>>   {
>> @@ -384,6 +386,11 @@ static inline int msm_dp_mst_register(struct msm_dp *dp_display)
>>   	return -EINVAL;
>>   }
>>   
>> +static inline int msm_dp_mst_attach_encoder(struct msm_dp *dp_display, struct drm_encoder *encoder)
>> +{
>> +	return -EINVAL;
>> +}
>> +
>>   static inline void msm_dp_snapshot(struct msm_disp_state *disp_state, struct msm_dp *dp_display)
>>   {
>>   }
>>
>> -- 
>> 2.43.0
>>
>