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

From: Dmitry Baryshkov
Date: Tue Jun 17 2025 - 06:04:51 EST

On 17/06/2025 10:52, Yongxing Mou wrote:


On 2025/6/16 21:48, Dmitry Baryshkov wrote:
On Mon, Jun 16, 2025 at 08:43:40PM +0800, Yongxing Mou wrote:


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.

pointer? Also, what do AMD and nouveau do?

Hi,here is the link:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/ drivers/gpu/drm/i915/display/intel_dp_mst.c?h=v6.16-rc2#n1504

nouveau just check the mode_rate and ds_max_dotclock in MST connector mode_valid().
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/ drivers/gpu/drm/nouveau/nouveau_dp.c?h=v6.16-rc2#n527

The AMD driver seems much more complex, and I can't understand all the logic. It looks like AMD always tries to enable DSC and use the smallest possible bandwidth.
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()).

If we filter modes in mode_valid(), then consider the following
scenario: we plug monitor A, plug monitor B, then unplug monitor A. At
this point we only have monitor B, but it has all modes filtered when A
has been plugged. So, it is impossible to select 4k@60x10, even though
it is a perfectly valid mode now.

Also, with the check happening in the atomic_check() the user will not
get the black screen: the commit will get rejected, letting userspace to
lower the mode for the second monitor.

Oh, this scenario is indeed just as you described. So let's remove this part of the logic and let userspace decide the final mode.

Ack. I think all three major drivers don't perform a check against currently allocated slots.

+
+    return msm_dp_display_mode_valid(dp_display, &dp_display- >connector->display_info, mode);
+}
+






--
With best wishes
Dmitry