| 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: <4d8c89be-2589-40f9-841e-27cb7b1040de@oss.qualcomm.com>
Date: Tue, 17 Jun 2025 13:04:05 +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 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@...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.
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
Powered by blists - more mailing lists