On Wed, Feb 12, 2025 at 05:13:08PM -0800, Abhinav Kumar wrote:
> Hi Marijn
>
> On 2/10/2025 2:17 PM, Abhinav Kumar wrote:
> >
> >
> > On 2/10/2025 6:24 AM, Dmitry Baryshkov wrote:
> > > On Mon, Feb 10, 2025 at 01:54:29PM +0100, Marijn Suijten wrote:
> > > > On 2025-02-10 01:11:59, Dmitry Baryshkov wrote:
> > > > > On Sun, Feb 09, 2025 at 10:42:53PM +0100, Marijn Suijten wrote:
> > > > > > Ordering issues here cause an uninitialized (default STANDALONE)
> > > > > > usecase to be programmed (which appears to be a MUX) in some cases
> > > > > > when msm_dsi_host_register() is called, leading to the slave PLL in
> > > > > > bonded-DSI mode to source from a clock parent (dsi1vco) that is off.
> > > > > >
> > > > > > This should seemingly not be a problem as the actual
> > > > > > dispcc clocks from
> > > > > > DSI1 that are muxed in the clock tree of DSI0 are way
> > > > > > further down, this
> > > > > > bit still seems to have an effect on them somehow and causes the right
> > > > > > side of the panel controlled by DSI1 to not function.
> > > > > >
> > > > > > In an ideal world this code is refactored to no longer have such
> > > > > > error-prone calls "across subsystems", and instead model the "PLL src"
> > > > > > register field as a regular mux so that changing the clock parents
> > > > > > programmatically or in DTS via `assigned-clock-parents` has the
> > > > > > desired effect.
> > > > > > But for the avid reader, the clocks that we *are* muxing into DSI0's
> > > > > > tree are way further down, so if this bit turns out to be a simple mux
> > > > > > between dsiXvco and out_div, that shouldn't have any effect as this
> > > > > > whole tree is off anyway.
> > > > > >
> > > > > > Signed-off-by: Marijn Suijten <marijn.suijten@xxxxxxxxxxxxxx>
> > > > > > ---
> > > > > > drivers/gpu/drm/msm/dsi/dsi_manager.c | 30
> > > > > > ++++++++++++++++++ +-----------
> > > > > > 1 file changed, 19 insertions(+), 11 deletions(-)
> > > > >
> > > > >
> > > > > Fixes: 57bf43389337 ("drm/msm/dsi: Pass down use case to PHY")
> > > >
> > > > I'm not exactly confident about that. Abhinav pointed out in
> > > > https://gitlab.freedesktop.org/drm/msm/-/issues/41#note_2375646 that
> > > > msm_dsi_host_register() was not supposed to be enabling the PHY, which I
> > > > provided a counter-stacktrace for to show that is indeed the case.
> > > >
> > > > Either this was always a problem that's only become visible now
> > > > (and it's an
> > > > issue with that patch), or a different change causes
> > > > msm_dsi_host_register()
> > > > to enable the PHY and program the usecase too early?
> > >
> > > As currently usecase is being programmed after the DSI host being
> > > registered, there might be a race condition between panel driver probe
> > > _and_ usecase programming.
> > >
> > > >
> > > > What do you think?
> > > >
> > > > - Marijn
> > > >
> >
> > Yes I agree with Dmitry's explanation. The race condition between the
> > two can cause this. Hence I am also fine with this change.
> >
> > > > > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > > > >
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > > > > > b/drivers/gpu/ drm/msm/dsi/dsi_manager.c
> > > > > > index a210b7c9e5ca281a46fbdb226e25832719a684ea..b93205c034e4acc73d536deeddce6ebd694b4a80
> > > > > > 100644
> > > > > > --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > > > > > +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > > > > > @@ -74,17 +74,33 @@ static int dsi_mgr_setup_components(int id)
> > > > > > int ret;
> > > > > > if (!IS_BONDED_DSI()) {
> > > > > > + /* Set the usecase before calling
> > > > > > msm_dsi_host_register(), which would
> > > > > > + * already program the PLL source mux based on
> > > > > > a default usecase.
> > > > > > + */
> > > > > > + msm_dsi_phy_set_usecase(msm_dsi->phy,
> > > > > > MSM_DSI_PHY_STANDALONE);
> > > > > > + msm_dsi_host_set_phy_mode(msm_dsi->host, msm_dsi->phy);
> > > > > > +
> > > > > > ret = msm_dsi_host_register(msm_dsi->host);
> > > > > > if (ret)
> > > > > > return ret;
> > > > > > -
> > > > > > - msm_dsi_phy_set_usecase(msm_dsi->phy,
> > > > > > MSM_DSI_PHY_STANDALONE);
> > > > > > - msm_dsi_host_set_phy_mode(msm_dsi->host, msm_dsi->phy);
> > > > > > } else if (other_dsi) {
> > > > > > struct msm_dsi *master_link_dsi = IS_MASTER_DSI_LINK(id) ?
> > > > > > msm_dsi : other_dsi;
> > > > > > struct msm_dsi *slave_link_dsi = IS_MASTER_DSI_LINK(id) ?
> > > > > > other_dsi : msm_dsi;
> > > > > > +
> > > > > > + /* PLL0 is to drive both DSI link clocks in bonded DSI mode.
> > > > > > + *
> > > > > > + /* Set the usecase before calling
> > > > > > msm_dsi_host_register(), which would
> > > > > > + * already program the PLL source mux based on
> > > > > > a default usecase.
> > > > > > + */
>
> My compiler is throwing a warning here
>
> drivers/gpu/drm/msm/dsi/dsi_manager.c: In function
> 'dsi_mgr_setup_components':
> drivers/gpu/drm/msm/dsi/dsi_manager.c:94:3: warning: "/*" within comment
> [-Wcomment]
> /* Set the usecase before calling msm_dsi_host_register(), which would
> ^
> drivers/gpu/drm/msm/dsi/dsi_manager.c: At top level:
>
> Can you pls address this one so that I can pick this one up?
While you are at it, Marijn, could you please also fix the first comment
to be properly formatted?
--
With best wishes
Dmitry
