| 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
| ||
|
Message-ID: <19895a7d-4f30-44f1-bc5f-45d200666860@quicinc.com>
Date: Tue, 17 Jun 2025 15:52:47 +0800
From: Yongxing Mou <quic_yongmou@...cinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.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 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@...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?
>
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@...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.
>
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.
>>>> +
>>>> + return msm_dp_display_mode_valid(dp_display, &dp_display->connector->display_info, mode);
>>>> +}
>>>> +
>>>
>>
>
Powered by blists - more mailing lists