| 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: <61834162-7e73-4467-9dd7-bfb1dcbd0afb@quicinc.com>
Date: Wed, 13 Aug 2025 17:52:04 +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 05/38] drm/msm/dp: allow dp_ctrl stream APIs to use any
panel passed to it
On 2025/6/9 21:12, Dmitry Baryshkov wrote:
> On Mon, Jun 09, 2025 at 08:21:24PM +0800, Yongxing Mou wrote:
>> From: Abhinav Kumar <quic_abhinavk@...cinc.com>
>>
>> Currently, the dp_ctrl stream APIs operate on their own dp_panel
>> which is cached inside the dp_ctrl's private struct. However with MST,
>> the cached panel represents the fixed link and not the sinks which
>> are hotplugged. Allow the stream related APIs to work on the panel
>> which is passed to them rather than the cached one. For SST cases,
>> this shall continue to use the cached dp_panel.
>>
>> Signed-off-by: Abhinav Kumar <quic_abhinavk@...cinc.com>
>> Signed-off-by: Yongxing Mou <quic_yongmou@...cinc.com>
>> ---
>> drivers/gpu/drm/msm/dp/dp_ctrl.c | 37 ++++++++++++++++++++-----------------
>> drivers/gpu/drm/msm/dp/dp_ctrl.h | 5 +++--
>> drivers/gpu/drm/msm/dp/dp_display.c | 4 ++--
>> 3 files changed, 25 insertions(+), 21 deletions(-)
>
> I think previous review comments got ignored. Please step back and
> review them. Maybe I should ask you to go back to v1 and actually check
> all the review comments there?
>
Sorry for that.. i will check all the comments again.. thanks
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> index 1ce3cca121d0c56b493e282c76eb9202371564cf..aee8e37655812439dfb65ae90ccb61b14e6e261f 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> @@ -135,7 +135,8 @@ void msm_dp_ctrl_push_idle(struct msm_dp_ctrl *msm_dp_ctrl)
>> drm_dbg_dp(ctrl->drm_dev, "mainlink off\n");
>> }
>>
>> -static void msm_dp_ctrl_config_ctrl(struct msm_dp_ctrl_private *ctrl)
>> +static void msm_dp_ctrl_config_ctrl(struct msm_dp_ctrl_private *ctrl,
>> + struct msm_dp_panel *msm_dp_panel)
>> {
>> u32 config = 0, tbd;
>> const u8 *dpcd = ctrl->panel->dpcd;
>> @@ -143,7 +144,7 @@ static void msm_dp_ctrl_config_ctrl(struct msm_dp_ctrl_private *ctrl)
>> /* Default-> LSCLK DIV: 1/4 LCLK */
>> config |= (2 << DP_CONFIGURATION_CTRL_LSCLK_DIV_SHIFT);
>>
>> - if (ctrl->panel->msm_dp_mode.out_fmt_is_yuv_420)
>> + if (msm_dp_panel->msm_dp_mode.out_fmt_is_yuv_420)
>> config |= DP_CONFIGURATION_CTRL_RGB_YUV; /* YUV420 */
>>
>> /* Scrambler reset enable */
>> @@ -151,7 +152,7 @@ static void msm_dp_ctrl_config_ctrl(struct msm_dp_ctrl_private *ctrl)
>> config |= DP_CONFIGURATION_CTRL_ASSR;
>>
>> tbd = msm_dp_link_get_test_bits_depth(ctrl->link,
>> - ctrl->panel->msm_dp_mode.bpp);
>> + msm_dp_panel->msm_dp_mode.bpp);
>>
>> config |= tbd << DP_CONFIGURATION_CTRL_BPC_SHIFT;
>>
>> @@ -174,20 +175,21 @@ static void msm_dp_ctrl_config_ctrl(struct msm_dp_ctrl_private *ctrl)
>> msm_dp_catalog_ctrl_config_ctrl(ctrl->catalog, config);
>> }
>>
>> -static void msm_dp_ctrl_configure_source_params(struct msm_dp_ctrl_private *ctrl)
>> +static void msm_dp_ctrl_configure_source_params(struct msm_dp_ctrl_private *ctrl,
>> + struct msm_dp_panel *msm_dp_panel)
>> {
>> u32 cc, tb;
>>
>> msm_dp_catalog_ctrl_lane_mapping(ctrl->catalog);
>> msm_dp_catalog_setup_peripheral_flush(ctrl->catalog);
>>
>> - msm_dp_ctrl_config_ctrl(ctrl);
>> + msm_dp_ctrl_config_ctrl(ctrl, msm_dp_panel);
>>
>> tb = msm_dp_link_get_test_bits_depth(ctrl->link,
>> - ctrl->panel->msm_dp_mode.bpp);
>> + msm_dp_panel->msm_dp_mode.bpp);
>> cc = msm_dp_link_get_colorimetry_config(ctrl->link);
>> msm_dp_catalog_ctrl_config_misc(ctrl->catalog, cc, tb);
>> - msm_dp_panel_timing_cfg(ctrl->panel);
>> + msm_dp_panel_timing_cfg(msm_dp_panel);
>> }
>>
>> /*
>> @@ -1317,7 +1319,7 @@ static int msm_dp_ctrl_link_train(struct msm_dp_ctrl_private *ctrl,
>> u8 assr;
>> struct msm_dp_link_info link_info = {0};
>>
>> - msm_dp_ctrl_config_ctrl(ctrl);
>> + msm_dp_ctrl_config_ctrl(ctrl, ctrl->panel);
>
> Could you please explain, when is it fine to use ctrl->panel and when it
> is not? Here you are passing msm_dp_panel to configure DP link for link
> training. I don't think we need the panel for that, so just using
> ctrl->panel here is incorrect too.
>
Emm, If we need to program registers related to the pixel clock or DP
link with MST(all of them need pass the stream_id to determine the
register address), we should pass in msm_dp_panel. If we're only
programming the other parts not related to the stream_id, passing in
ctrl->panel is sufficient.
here in link tranning, it's right, we actually don't need to pass in the
panel. But since in msm_dp_ctrl_config_ctrl, we will write config to
DP0/DP1 CONFIGURATION_CTRL, even mst2/mst3 link CONFIGURATION_CTRL. and
this func will also been called in msm_dp_ctrl_configure_source_params.
so we need add ctrl->panel here.
>>
>> link_info.num_lanes = ctrl->link->link_params.num_lanes;
>> link_info.rate = ctrl->link->link_params.rate;
>> @@ -1735,7 +1737,8 @@ static bool msm_dp_ctrl_send_phy_test_pattern(struct msm_dp_ctrl_private *ctrl)
>> return success;
>> }
>>
>> -static int msm_dp_ctrl_process_phy_test_request(struct msm_dp_ctrl_private *ctrl)
>> +static int msm_dp_ctrl_process_phy_test_request(struct msm_dp_ctrl_private *ctrl,
>> + struct msm_dp_panel *msm_dp_panel)
>> {
>> int ret;
>> unsigned long pixel_rate;
>> @@ -1759,7 +1762,7 @@ static int msm_dp_ctrl_process_phy_test_request(struct msm_dp_ctrl_private *ctrl
>> return ret;
>> }
>>
>> - pixel_rate = ctrl->panel->msm_dp_mode.drm_mode.clock;
>> + pixel_rate = msm_dp_panel->msm_dp_mode.drm_mode.clock;
>> ret = clk_set_rate(ctrl->pixel_clk, pixel_rate * 1000);
>> if (ret) {
>> DRM_ERROR("Failed to set pixel clock rate. ret=%d\n", ret);
>> @@ -1797,7 +1800,7 @@ void msm_dp_ctrl_handle_sink_request(struct msm_dp_ctrl *msm_dp_ctrl)
>>
>> if (sink_request & DP_TEST_LINK_PHY_TEST_PATTERN) {
>> drm_dbg_dp(ctrl->drm_dev, "PHY_TEST_PATTERN request\n");
>> - if (msm_dp_ctrl_process_phy_test_request(ctrl)) {
>> + if (msm_dp_ctrl_process_phy_test_request(ctrl, ctrl->panel)) {
>> DRM_ERROR("process phy_test_req failed\n");
>> return;
>> }
>> @@ -2015,7 +2018,7 @@ int msm_dp_ctrl_prepare_stream_on(struct msm_dp_ctrl *msm_dp_ctrl, bool force_li
>> return ret;
>> }
>>
>> -int msm_dp_ctrl_on_stream(struct msm_dp_ctrl *msm_dp_ctrl)
>> +int msm_dp_ctrl_on_stream(struct msm_dp_ctrl *msm_dp_ctrl, struct msm_dp_panel *msm_dp_panel)
>> {
>> int ret = 0;
>> bool mainlink_ready = false;
>> @@ -2028,9 +2031,9 @@ int msm_dp_ctrl_on_stream(struct msm_dp_ctrl *msm_dp_ctrl)
>>
>> ctrl = container_of(msm_dp_ctrl, struct msm_dp_ctrl_private, msm_dp_ctrl);
>>
>> - pixel_rate = pixel_rate_orig = ctrl->panel->msm_dp_mode.drm_mode.clock;
>> + pixel_rate = pixel_rate_orig = msm_dp_panel->msm_dp_mode.drm_mode.clock;
>>
>> - if (msm_dp_ctrl->wide_bus_en || ctrl->panel->msm_dp_mode.out_fmt_is_yuv_420)
>> + if (msm_dp_ctrl->wide_bus_en || msm_dp_panel->msm_dp_mode.out_fmt_is_yuv_420)
>> pixel_rate >>= 1;
>>
>> drm_dbg_dp(ctrl->drm_dev, "pixel_rate=%lu\n", pixel_rate);
>> @@ -2058,12 +2061,12 @@ int msm_dp_ctrl_on_stream(struct msm_dp_ctrl *msm_dp_ctrl)
>> */
>> reinit_completion(&ctrl->video_comp);
>>
>> - msm_dp_ctrl_configure_source_params(ctrl);
>> + msm_dp_ctrl_configure_source_params(ctrl, msm_dp_panel);
>>
>> msm_dp_catalog_ctrl_config_msa(ctrl->catalog,
>> ctrl->link->link_params.rate,
>> pixel_rate_orig,
>> - ctrl->panel->msm_dp_mode.out_fmt_is_yuv_420);
>> + msm_dp_panel->msm_dp_mode.out_fmt_is_yuv_420);
>>
>> msm_dp_ctrl_setup_tr_unit(ctrl);
>>
>> @@ -2081,7 +2084,7 @@ int msm_dp_ctrl_on_stream(struct msm_dp_ctrl *msm_dp_ctrl)
>> return ret;
>> }
>>
>> -void msm_dp_ctrl_clear_vsc_sdp_pkt(struct msm_dp_ctrl *msm_dp_ctrl)
>> +void msm_dp_ctrl_clear_vsc_sdp_pkt(struct msm_dp_ctrl *msm_dp_ctrl, struct msm_dp_panel *dp_panel)
>> {
>> struct msm_dp_ctrl_private *ctrl;
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.h b/drivers/gpu/drm/msm/dp/dp_ctrl.h
>> index edbe5766db74c4e4179141d895f9cb85e514f29b..fbe458c5a17bda0586097a61d925f608d99f9224 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.h
>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h
>> @@ -18,7 +18,7 @@ struct msm_dp_ctrl {
>> struct phy;
>>
>> int msm_dp_ctrl_on_link(struct msm_dp_ctrl *msm_dp_ctrl);
>> -int msm_dp_ctrl_on_stream(struct msm_dp_ctrl *msm_dp_ctrl);
>> +int msm_dp_ctrl_on_stream(struct msm_dp_ctrl *msm_dp_ctrl, struct msm_dp_panel *msm_dp_panel);
>> int msm_dp_ctrl_prepare_stream_on(struct msm_dp_ctrl *dp_ctrl, bool force_link_train);
>> void msm_dp_ctrl_off_link(struct msm_dp_ctrl *msm_dp_ctrl);
>> void msm_dp_ctrl_off(struct msm_dp_ctrl *msm_dp_ctrl);
>> @@ -41,7 +41,8 @@ void msm_dp_ctrl_config_psr(struct msm_dp_ctrl *msm_dp_ctrl);
>> int msm_dp_ctrl_core_clk_enable(struct msm_dp_ctrl *msm_dp_ctrl);
>> void msm_dp_ctrl_core_clk_disable(struct msm_dp_ctrl *msm_dp_ctrl);
>>
>> -void msm_dp_ctrl_clear_vsc_sdp_pkt(struct msm_dp_ctrl *msm_dp_ctrl);
>> +void msm_dp_ctrl_clear_vsc_sdp_pkt(struct msm_dp_ctrl *msm_dp_ctrl,
>> + struct msm_dp_panel *msm_dp_panel);
>> void msm_dp_ctrl_psm_config(struct msm_dp_ctrl *msm_dp_ctrl);
>> void msm_dp_ctrl_reinit_phy(struct msm_dp_ctrl *msm_dp_ctrl);
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
>> index a5ca498cb970d0c6a4095b0b7fc6269c2dc3ad31..17ccea4047500848c4fb3eda87a10e29b18e0cfb 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -872,7 +872,7 @@ static int msm_dp_display_enable(struct msm_dp_display_private *dp)
>> return 0;
>> }
>>
>> - rc = msm_dp_ctrl_on_stream(dp->ctrl);
>> + rc = msm_dp_ctrl_on_stream(dp->ctrl, dp->panel);
>> if (!rc)
>> msm_dp_display->power_on = true;
>>
>> @@ -925,7 +925,7 @@ static int msm_dp_display_disable(struct msm_dp_display_private *dp)
>> if (!msm_dp_display->power_on)
>> return 0;
>>
>> - msm_dp_ctrl_clear_vsc_sdp_pkt(dp->ctrl);
>> + msm_dp_ctrl_clear_vsc_sdp_pkt(dp->ctrl, dp->panel);
>>
>> /* dongle is still connected but sinks are disconnected */
>> if (dp->link->sink_count == 0) {
>>
>> --
>> 2.34.1
>>
>
Powered by blists - more mailing lists