Date: Wed, 17 Apr 2024 14:50:03 +0200
From: Marijn Suijten <marijn.suijten@...ainline.org>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
Cc: Rob Clark <robdclark@...il.com>, 
	Abhinav Kumar <quic_abhinavk@...cinc.com>, Sean Paul <sean@...rly.run>, David Airlie <airlied@...il.com>, 
	Daniel Vetter <daniel@...ll.ch>, Archit Taneja <architt@...eaurora.org>, 
	Chandan Uddaraju <chandanu@...eaurora.org>, Vinod Koul <vkoul@...nel.org>, 
	Sravanthi Kollukuduru <skolluku@...eaurora.org>, linux-arm-msm@...r.kernel.org, dri-devel@...ts.freedesktop.org, 
	freedreno@...ts.freedesktop.org, linux-kernel@...r.kernel.org, 
	Jordan Crouse <jordan@...micpenguin.net>, Rajesh Yadav <ryadav@...eaurora.org>, 
	Jeykumar Sankaran <jsanka@...eaurora.org>, ~postmarketos/upstreaming@...ts.sr.ht, 
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>, Konrad Dybcio <konrad.dybcio@...aro.org>, 
	Martin Botka <martin.botka@...ainline.org>, Jami Kettunen <jami.kettunen@...ainline.org>
Subject: Re: [PATCH 6/7] drm/msm/dsi: Set PHY usescase before registering DSI
 host

On 2024-04-17 11:18:58, Dmitry Baryshkov wrote:
> On Wed, 17 Apr 2024 at 02:57, Marijn Suijten
> <marijn.suijten@...ainline.org> wrote:
> >
> > Ordering issues here cause an uninitalized (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@...ainline.org>
> > ---
> >  drivers/gpu/drm/msm/dsi/dsi_manager.c | 15 +++++++++++----
> >  1 file changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > index af2a287cb3bd..17f43b3c0494 100644
> > --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > @@ -85,6 +85,17 @@ static int dsi_mgr_setup_components(int 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 2 DSI link clocks in bonded DSI mode.
> > +                *
> > +                * Set the usecase before calling msm_dsi_host_register() to prevent it from
> > +                * enabling and configuring the usecase (which is just a mux bit) first.
> > +                */
> > +               msm_dsi_phy_set_usecase(clk_master_dsi->phy,
> > +                                       MSM_DSI_PHY_MASTER);
> > +               msm_dsi_phy_set_usecase(clk_slave_dsi->phy,
> > +                                       MSM_DSI_PHY_SLAVE);
> > +
> >                 /* Register slave host first, so that slave DSI device
> >                  * has a chance to probe, and do not block the master
> >                  * DSI device's probe.
> > @@ -100,10 +111,6 @@ static int dsi_mgr_setup_components(int id)
> >                         return ret;
> >
> >                 /* PLL0 is to drive both 2 DSI link clocks in bonded DSI mode. */
> > -               msm_dsi_phy_set_usecase(clk_master_dsi->phy,
> > -                                       MSM_DSI_PHY_MASTER);
> > -               msm_dsi_phy_set_usecase(clk_slave_dsi->phy,
> > -                                       MSM_DSI_PHY_SLAVE);
> >                 msm_dsi_host_set_phy_mode(msm_dsi->host, msm_dsi->phy);
> >                 msm_dsi_host_set_phy_mode(other_dsi->host, other_dsi->phy);
> 
> Please move msm_dsi_host_set_phy_mode() calls too.

Ack.  Yeah, given that msm_dsi_host_register() causes a modeset and finally the
PLL turning on, these should be set up as well.

For anyone else following along, I have pasted the stacktrace that showcases
the execution flow in the drm/msm tracker:

https://gitlab.freedesktop.org/drm/msm/-/issues/41#note_2376115

Abhinav also pointed out that this PLL source was correctly set in earlier
devcoredump reports, so it might have been a recent development/regression?
This seems to be the only issue originating from it, but folks were adamant that
dsi_mgr_setup_components() (ultimately) would never turn the PLL on, which is
"debunked" by said stacktrace.  Maybe other assumptions are affected by this
change?

> Also please update the non-bonded case.

Definitely, as suggested in the cover letter.  A similar stacktrace to the above
is acquired on a non-bonded setup, which is also relying on the variable to be
initialized to 0 to select the "local PLL source", rather than being correctly
set via this msm_dsi_phy_set_usecase() configuration.

- Marijn

> >         }
> >
> > --
> > 2.44.0
> >
> 
> 
> -- 
> With best wishes
> Dmitry

Powered by blists - more mailing lists