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