| Message ID | 1470134069-12178-2-git-send-email-jonathanh@nvidia.com |
|---|---|
| State | Superseded |
| Delegated to: | Thierry Reding |
| Headers | show |
On Tue, Aug 02, 2016 at 11:34:26AM +0100, Jon Hunter wrote: > Commit 33a8eb8d40ee ("drm/tegra: dc: Implement runtime PM") disables the > display power partition when probing and this causes the Tegra114 > Dalmore to hang during boot. > > The hang occurs when accessing the MIPI calibration registers (which are > accessed during the configuration of the DSI interface). Ideally the > MIPI driver should manage the power partition itself to ensure it is on > when needed. The problem is that the legacy PMC APIs used for managing > the power partitions do not support reference counting and so this > cannot be easily done currently. Long-term we will migrate devices to > use generic PM domains and such scenarios will be easy to support. For > now fix this by removing the code to turn off the display power > partition when probing the DC and always keep the DC on so that the > power partition is not turned off. This is consistent with how the power > partition was managed prior to this commit. > > Please note that for earlier devices such as Tegra114 the MIPI > calibration logic is part of the display power partition, where as for > newer devices, such as Tegra124/210 it is part of the SOR power > partition. Hence, in the long-term is makes more sense to handle such > power partitions via the generic PM domain framework. > > Fixes: 33a8eb8d40ee ("drm/tegra: dc: Implement runtime PM") > > Signed-off-by: Jon Hunter <jonathanh@nvidia.com> > --- > > Please note that the hang is only seen on Tegra114 with v4.8 if the > patch "ARM: tegra: Correct polarity for Tegra114 PMIC interrupt" (2nd > patch in series) is applied without this patch. Without the fix for the > PMIC interrupt polarity the Palmas PMIC probe fails and the display > probe also fails because the regulators are not found. > > drivers/gpu/drm/tegra/dc.c | 21 ++++++++++++++------- > 1 file changed, 14 insertions(+), 7 deletions(-) I don't think this fixes the problem at the root. After looking at the code I think what you're seeing is caused by the tegra_mipi_power_up() call that happens as part of the tegra_mipi_request() from the DSI driver's ->probe(). Generally there shouldn't be a problem because the display controller will always get enabled before the encoder (DSI) and hence the power partition should be enabled when the actual calibration happens. The fundamental problem in this case is that we're actually powering up the MIPI calibration logic at the wrong time. So I think what we'll want for a proper fix is to move all register accesses out of the tegra_mipi_request() function and add tegra_mipi_enable() and tegra_mipi_disable() functions that power up and power down the MIPI calibration logic, respectively. That way we can move all the code which relies on the power partition into the tegra_dsi_encoder_enable() and tegra_dsi_encoder_disable() functions. Perhaps an even better place to call these new functions from would be the DSI driver's ->suspend() and ->resume() functions. An added benefit of this will be that the MIPI calibration logic could be powered off when DSI is disabled (provided no other user requires it to be powered on), whereas currently it will remain powered even if the DSI output is off, since the power on happens in ->probe(). I'll go write a patch. Thierry
On 12/08/16 14:46, Thierry Reding wrote: > * PGP Signed by an unknown key > > On Tue, Aug 02, 2016 at 11:34:26AM +0100, Jon Hunter wrote: >> Commit 33a8eb8d40ee ("drm/tegra: dc: Implement runtime PM") disables the >> display power partition when probing and this causes the Tegra114 >> Dalmore to hang during boot. >> >> The hang occurs when accessing the MIPI calibration registers (which are >> accessed during the configuration of the DSI interface). Ideally the >> MIPI driver should manage the power partition itself to ensure it is on >> when needed. The problem is that the legacy PMC APIs used for managing >> the power partitions do not support reference counting and so this >> cannot be easily done currently. Long-term we will migrate devices to >> use generic PM domains and such scenarios will be easy to support. For >> now fix this by removing the code to turn off the display power >> partition when probing the DC and always keep the DC on so that the >> power partition is not turned off. This is consistent with how the power >> partition was managed prior to this commit. >> >> Please note that for earlier devices such as Tegra114 the MIPI >> calibration logic is part of the display power partition, where as for >> newer devices, such as Tegra124/210 it is part of the SOR power >> partition. Hence, in the long-term is makes more sense to handle such >> power partitions via the generic PM domain framework. >> >> Fixes: 33a8eb8d40ee ("drm/tegra: dc: Implement runtime PM") >> >> Signed-off-by: Jon Hunter <jonathanh@nvidia.com> >> --- >> >> Please note that the hang is only seen on Tegra114 with v4.8 if the >> patch "ARM: tegra: Correct polarity for Tegra114 PMIC interrupt" (2nd >> patch in series) is applied without this patch. Without the fix for the >> PMIC interrupt polarity the Palmas PMIC probe fails and the display >> probe also fails because the regulators are not found. >> >> drivers/gpu/drm/tegra/dc.c | 21 ++++++++++++++------- >> 1 file changed, 14 insertions(+), 7 deletions(-) > > I don't think this fixes the problem at the root. After looking at the > code I think what you're seeing is caused by the tegra_mipi_power_up() > call that happens as part of the tegra_mipi_request() from the DSI > driver's ->probe(). No it doesn't. It is more of a bandaid. I think that this issue has always been there but no exposed until the move to RPM for the DC. I can't say I was too happy with it! > Generally there shouldn't be a problem because the display controller > will always get enabled before the encoder (DSI) and hence the power > partition should be enabled when the actual calibration happens. The > fundamental problem in this case is that we're actually powering up > the MIPI calibration logic at the wrong time. So I think what we'll > want for a proper fix is to move all register accesses out of the > tegra_mipi_request() function and add tegra_mipi_enable() and > tegra_mipi_disable() functions that power up and power down the MIPI > calibration logic, respectively. That way we can move all the code > which relies on the power partition into the tegra_dsi_encoder_enable() > and tegra_dsi_encoder_disable() functions. > > Perhaps an even better place to call these new functions from would be > the DSI driver's ->suspend() and ->resume() functions. > > An added benefit of this will be that the MIPI calibration logic could > be powered off when DSI is disabled (provided no other user requires it > to be powered on), whereas currently it will remain powered even if the > DSI output is off, since the power on happens in ->probe(). > > I'll go write a patch. Great! Thanks. Jon
diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c index 8495bd01b544..17bd80a745d6 100644 --- a/drivers/gpu/drm/tegra/dc.c +++ b/drivers/gpu/drm/tegra/dc.c @@ -1217,8 +1217,6 @@ static void tegra_crtc_disable(struct drm_crtc *crtc) tegra_dc_stats_reset(&dc->stats); drm_crtc_vblank_off(crtc); - - pm_runtime_put_sync(dc->dev); } static void tegra_crtc_enable(struct drm_crtc *crtc) @@ -1228,8 +1226,6 @@ static void tegra_crtc_enable(struct drm_crtc *crtc) struct tegra_dc *dc = to_tegra_dc(crtc); u32 value; - pm_runtime_get_sync(dc->dev); - /* initialize display controller */ if (dc->syncpt) { u32 syncpt = host1x_syncpt_id(dc->syncpt); @@ -1997,8 +1993,6 @@ static int tegra_dc_probe(struct platform_device *pdev) dc->powergate = TEGRA_POWERGATE_DIS; else dc->powergate = TEGRA_POWERGATE_DISB; - - tegra_powergate_power_off(dc->powergate); } regs = platform_get_resource(pdev, IORESOURCE_MEM, 0); @@ -2020,6 +2014,11 @@ static int tegra_dc_probe(struct platform_device *pdev) platform_set_drvdata(pdev, dc); pm_runtime_enable(&pdev->dev); + err = pm_runtime_get_sync(&pdev->dev); + if (err < 0) { + dev_err(&pdev->dev, "failed to enable device: %d\n", err); + goto rpm_disable; + } INIT_LIST_HEAD(&dc->client.list); dc->client.ops = &dc_client_ops; @@ -2029,10 +2028,17 @@ static int tegra_dc_probe(struct platform_device *pdev) if (err < 0) { dev_err(&pdev->dev, "failed to register host1x client: %d\n", err); - return err; + goto rpm_put; } return 0; + +rpm_put: + pm_runtime_put_sync(&pdev->dev); +rpm_disable: + pm_runtime_disable(&pdev->dev); + + return err; } static int tegra_dc_remove(struct platform_device *pdev) @@ -2053,6 +2059,7 @@ static int tegra_dc_remove(struct platform_device *pdev) return err; } + pm_runtime_put_sync(&pdev->dev); pm_runtime_disable(&pdev->dev); return 0;
Commit 33a8eb8d40ee ("drm/tegra: dc: Implement runtime PM") disables the display power partition when probing and this causes the Tegra114 Dalmore to hang during boot. The hang occurs when accessing the MIPI calibration registers (which are accessed during the configuration of the DSI interface). Ideally the MIPI driver should manage the power partition itself to ensure it is on when needed. The problem is that the legacy PMC APIs used for managing the power partitions do not support reference counting and so this cannot be easily done currently. Long-term we will migrate devices to use generic PM domains and such scenarios will be easy to support. For now fix this by removing the code to turn off the display power partition when probing the DC and always keep the DC on so that the power partition is not turned off. This is consistent with how the power partition was managed prior to this commit. Please note that for earlier devices such as Tegra114 the MIPI calibration logic is part of the display power partition, where as for newer devices, such as Tegra124/210 it is part of the SOR power partition. Hence, in the long-term is makes more sense to handle such power partitions via the generic PM domain framework. Fixes: 33a8eb8d40ee ("drm/tegra: dc: Implement runtime PM") Signed-off-by: Jon Hunter <jonathanh@nvidia.com> --- Please note that the hang is only seen on Tegra114 with v4.8 if the patch "ARM: tegra: Correct polarity for Tegra114 PMIC interrupt" (2nd patch in series) is applied without this patch. Without the fix for the PMIC interrupt polarity the Palmas PMIC probe fails and the display probe also fails because the regulators are not found. drivers/gpu/drm/tegra/dc.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-)