| Message ID | 20240417-drm-msm-initial-dualpipe-dsc-fixes-v1-6-78ae3ee9a697@somainline.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | drm/msm: Initial fixes for DUALPIPE (+DSC) topology | expand |
On Wed, 17 Apr 2024 at 02:57, Marijn Suijten <marijn.suijten@somainline.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@somainline.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. Also please update the non-bonded case. > } > > -- > 2.44.0 >
On 2024-04-17 11:18:58, Dmitry Baryshkov wrote: > On Wed, 17 Apr 2024 at 02:57, Marijn Suijten > <marijn.suijten@somainline.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@somainline.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
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); }
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@somainline.org> --- drivers/gpu/drm/msm/dsi/dsi_manager.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)