Re: [PATCH v2 29/38] drm/msm/dp: add connector abstraction for DP MST

[Yongxing Mou]

On 2025/6/9 23:51, Dmitry Baryshkov wrote:
> On Mon, Jun 09, 2025 at 08:21:48PM +0800, Yongxing Mou wrote:
> > From: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>
> >
> > 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_abhinavk@xxxxxxxxxxx>
> > Signed-off-by: Yongxing Mou <quic_yongmou@xxxxxxxxxxx>
> > ---
> >   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(+)
>
> > +
> > +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 msm_dp *dp_display;
> > +	struct drm_dp_mst_port *mst_port;
> > +	struct msm_dp_panel *dp_panel;
> > +	struct msm_dp_mst *mst;
> > +	u16 full_pbn, required_pbn;
> > +	int available_slots, required_slots;
> > +	struct msm_dp_mst_bridge_state *dp_bridge_state;
> > +	int i, slots_in_use = 0, active_enc_cnt = 0;
> > +	const u32 tot_slots = 63;
> > +
> > +	if (drm_connector_is_unregistered(connector))
> > +		return 0;
> > +
> > +	mst_conn = to_msm_dp_mst_connector(connector);
> > +	dp_display = mst_conn->msm_dp;
> > +	mst = dp_display->msm_dp_mst;
> > +	mst_port = mst_conn->mst_port;
> > +	dp_panel = mst_conn->dp_panel;
> > +
> > +	if (!dp_panel || !mst_port)
> > +		return MODE_ERROR;
> > +
> > +	for (i = 0; i < mst->max_streams; i++) {
> > +		dp_bridge_state = to_msm_dp_mst_bridge_state(&mst->mst_bridge[i]);
> > +		if (dp_bridge_state->connector &&
> > +		    dp_bridge_state->connector != connector) {
> > +			active_enc_cnt++;
> > +			slots_in_use += dp_bridge_state->num_slots;
> > +		}
> > +	}
> > +
> > +	if (active_enc_cnt < DP_STREAM_MAX) {
> > +		full_pbn = mst_port->full_pbn;
> > +		available_slots = tot_slots - slots_in_use;
> > +	} else {
> > +		DRM_ERROR("all mst streams are active\n");
> > +		return MODE_BAD;
> > +	}
> > +
> > +	required_pbn = drm_dp_calc_pbn_mode(mode->clock, (connector->display_info.bpc * 3) << 4);
> > +
> > +	required_slots = msm_dp_mst_find_vcpi_slots(&mst->mst_mgr, required_pbn);
> > +
> > +	if (required_pbn > full_pbn || required_slots > available_slots) {
> > +		drm_dbg_dp(dp_display->drm_dev,
> > +			   "mode:%s not supported. pbn %d vs %d slots %d vs %d\n",
> > +			   mode->name, required_pbn, full_pbn,
> > +			   required_slots, available_slots);
> > +		return MODE_BAD;
> > +	}
>
> I almost missed this. Could you please point me, do other drivers
> perform mode_valid() check based on the current slots available or not?
> Could you please point me to the relevant code in other drivers? Because
> it doesn't look correct to me. The mode on the screen remains valid no
> matter if I plug or unplug other devices. The atomic_check() should fail
> if we don't have enough resources (which includes slots).
Currently, I haven't found other drivers checking available slots during 
mode_valid(). Intel will check the PBN in here. This condition can help 
us in the following case:

Assume two downstream devices both support 4K 60Hz 10-bit. In MST mode, 
when the first device occupies the 4Kx60Hzx10bit mode, the remaining 
bandwidth is insufficient to support the same mode for the second device.

If we check the slots in mode_valid(), the second device will reject the 
4Kx60Hzx10bit mode but accept 4Kx30Hzx10bit. However, if the check is 
done in atomic_check(), the second device will display a black screen 
(because 4Kx60Hzx10bit is considered valid in mode_valid() but failed in 
atomic_check()).
> > +
> > +	return msm_dp_display_mode_valid(dp_display, &dp_display->connector->display_info, mode);
> > +}
> > +