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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <7r7vdbeols4suew7rlvogft4b5lmg22osipydxzkubxsychewi@lpyj6vmoapzb>
Date: Mon, 16 Jun 2025 16:48:06 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
To: Yongxing Mou <quic_yongmou@...cinc.com>
Cc: Rob Clark <robin.clark@....qualcomm.com>,
        Abhinav Kumar <abhinav.kumar@...ux.dev>,
        Jessica Zhang <jessica.zhang@....qualcomm.com>,
        Sean Paul <sean@...rly.run>,
        Marijn Suijten <marijn.suijten@...ainline.org>,
        David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
        linux-arm-msm@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        freedreno@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
        Abhinav Kumar <quic_abhinavk@...cinc.com>
Subject: Re: [PATCH v2 29/38] drm/msm/dp: add connector abstraction for DP MST

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@...cinc.com>
> > > 
> > > 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@...cinc.com>
> > > Signed-off-by: Yongxing Mou <quic_yongmou@...cinc.com>
> > > ---
> > >   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?

> 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@...10, 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.

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

-- 
With best wishes
Dmitry