| 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: <5emeno6zpefewmysmmfb6s64mme32pzatgpzeu6hnuzgfi3q4t@i6zpgj5am3ie>
Date: Mon, 9 Jun 2025 16:12:04 +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 05/38] drm/msm/dp: allow dp_ctrl stream APIs to use
any panel passed to it
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?
>
> 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.
>
> 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
>
--
With best wishes
Dmitry
Powered by blists - more mailing lists