On Mon, Jun 09, 2025 at 08:21:24PM +0800, Yongxing Mou wrote:
> From: Abhinav Kumar <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Yongxing Mou <[email protected]>
> ---
> 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
