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

[Dmitry Baryshkov]

On Wed, Aug 13, 2025 at 05:52:04PM +0800, Yongxing Mou wrote:
> 
> 
> 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 at 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 at quicinc.com>
> > > Signed-off-by: Yongxing Mou <quic_yongmou at 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.

What is stored in ctrl->panel in the MST case? Can we split it into the
link and sink parts? E.g. dpcd or downstream_ports are not a part of the
panel itself. They describe a link between DPTX and DPRX.

Downstream_ports might be even more fun. How do we support MST hub being
connected through another MST hub?

> 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.

I'd prefer a cleaner interface here. Could you please separate
stream-dependent and stream-independent parts?

> > >   	link_info.num_lanes = ctrl->link->link_params.num_lanes;
> > >   	link_info.rate = ctrl->link->link_params.rate;

-- 
With best wishes
Dmitry