Re: [PATCH v2 05/38] drm/msm/dp: allow dp_ctrl stream APIs to use any panel passed to it

[Yongxing Mou <quic_yongmou@quicinc.com>]

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@quicinc.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@quicinc.com>
>> Signed-off-by: Yongxing Mou <quic_yongmou@quicinc.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
>>
>