| Message ID | 1418020850-7664-1-git-send-email-markz@nvidia.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Sun, Dec 7, 2014 at 10:40 PM, Mark Zhang <markz@nvidia.com> wrote: > This patch adds the suspend/resume support for Tegra drm > driver by calling the corresponding DPMS functions. > > Signed-off-by: Mark Zhang <markz@nvidia.com> > --- > Hi, > > This patch hooks DSI driver's suspend/resume to implement the whole > display system's suspend/resume. I know this is a super ugly way, > but as we all know, Tegra DRM driver doesn't have a dedicate drm device > so honestly I didn't find a better way to do that. > > Thanks, > Mark > Hi Mark, Thanks for posting this. I've reproduced my Gerrit comments from the CrOS tree below for the benefit of others. > drivers/gpu/drm/tegra/dsi.c | 96 ++++++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 90 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c > index 33f67fd601c6..25cd0d93f392 100644 > --- a/drivers/gpu/drm/tegra/dsi.c > +++ b/drivers/gpu/drm/tegra/dsi.c > @@ -61,6 +61,9 @@ struct tegra_dsi { > struct tegra_dsi *slave; > }; > > +static int tegra_dsi_pad_calibrate(struct tegra_dsi *); > +static int tegra_dsi_ganged_setup(struct tegra_dsi *dsi); > + > static inline struct tegra_dsi * > host1x_client_to_dsi(struct host1x_client *client) > { > @@ -805,6 +808,20 @@ static int tegra_output_dsi_setup_clock(struct tegra_output *output, > > lanes = tegra_dsi_get_lanes(dsi); > > + err = tegra_dsi_pad_calibrate(dsi); > + if (err < 0) { > + dev_err(dsi->dev, "MIPI calibration failed: %d\n", err); > + return err; > + } > + if (dsi->slave) { > + err = tegra_dsi_pad_calibrate(dsi->slave); > + if (err < 0) { > + dev_err(dsi->slave->dev, > + "MIPI calibration failed: %d\n", err); > + return err; > + } > + } Move this duplicate call into tegra_dsi_pad_calibrate() to be consistent with the other functions in this file (eg: tegra_dsi_set_timeout). > + > err = tegra_dsi_get_muldiv(dsi->format, &mul, &div); > if (err < 0) > return err; > @@ -833,6 +850,13 @@ static int tegra_output_dsi_setup_clock(struct tegra_output *output, > dev_err(dsi->dev, "failed to set parent clock: %d\n", err); > return err; > } > + if (dsi->slave) { > + err = tegra_dsi_ganged_setup(dsi); > + if (err < 0) { > + dev_err(dsi->dev, "DSI ganged setup failed: %d\n", err); > + return err; > + } > + } I think this belongs in ->enable() instead of here. > > err = clk_set_rate(dsi->clk_parent, plld); > if (err < 0) { > @@ -1470,12 +1494,6 @@ static int tegra_dsi_probe(struct platform_device *pdev) > goto disable_vdd; > } > > - err = tegra_dsi_pad_calibrate(dsi); > - if (err < 0) { > - dev_err(dsi->dev, "MIPI calibration failed: %d\n", err); > - goto mipi_free; > - } > - > dsi->host.ops = &tegra_dsi_host_ops; > dsi->host.dev = &pdev->dev; > > @@ -1544,6 +1562,67 @@ static int tegra_dsi_remove(struct platform_device *pdev) > return 0; > } > > +#ifdef CONFIG_PM > +static int tegra_dsi_suspend(struct platform_device *pdev, pm_message_t state) > +{ > + struct tegra_dsi *dsi = platform_get_drvdata(pdev); > + struct tegra_drm *tegra = dev_get_drvdata(dsi->client.parent); > + struct drm_connector *connector; > + > + if (dsi->master) { > + regulator_disable(dsi->vdd); > + return 0; > + } > + > + drm_modeset_lock_all(tegra->drm); > + list_for_each_entry(connector, &tegra->drm->mode_config.connector_list, > + head) { > + int old_dpms = connector->dpms; > + > + if (connector->funcs->dpms) > + connector->funcs->dpms(connector, DRM_MODE_DPMS_OFF); > + > + /* Set the old mode back to the connector for resume */ > + connector->dpms = old_dpms; > + } > + drm_modeset_unlock_all(tegra->drm); > + > + regulator_disable(dsi->vdd); > + > + return 0; > +} > + > +static int tegra_dsi_resume(struct platform_device *pdev) > +{ > + struct tegra_dsi *dsi = platform_get_drvdata(pdev); > + struct tegra_drm *tegra = dev_get_drvdata(dsi->client.parent); > + struct drm_connector *connector; > + int err = 0; > + > + err = regulator_enable(dsi->vdd); > + if (err < 0) { > + dev_err(&pdev->dev, "Enable DSI vdd failed: %d\n", err); > + return err; > + } > + > + if (dsi->master) > + return 0; > + > + drm_modeset_lock_all(tegra->drm); > + list_for_each_entry(connector, &tegra->drm->mode_config.connector_list, > + head) { > + if (connector->funcs->dpms) > + connector->funcs->dpms(connector, connector->dpms); > + } > + drm_modeset_unlock_all(tegra->drm); > + > + drm_helper_resume_force_mode(tegra->drm); > + > + return 0; > +} > +#endif So this is the tricky chunk, it definitely does not belong here. I think it makes most sense for this to live in drm.c, however host1x_driver doesn't have suspend/resume hooks. I suspect the correct thing to do here is to add them to host1x_driver, but I'm not sure how much effort is involved in doing that. Sean > + > + > static const struct of_device_id tegra_dsi_of_match[] = { > { .compatible = "nvidia,tegra114-dsi", }, > { }, > @@ -1557,4 +1636,9 @@ struct platform_driver tegra_dsi_driver = { > }, > .probe = tegra_dsi_probe, > .remove = tegra_dsi_remove, > +#ifdef CONFIG_PM > + .suspend = tegra_dsi_suspend, > + .resume = tegra_dsi_resume, > +#endif > + > }; > -- > 1.8.1.5 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/10/2014 03:29 AM, Sean Paul wrote: > On Sun, Dec 7, 2014 at 10:40 PM, Mark Zhang <markz@nvidia.com> wrote: >> This patch adds the suspend/resume support for Tegra drm >> driver by calling the corresponding DPMS functions. [...] >> + if (dsi->slave) { >> + err = tegra_dsi_pad_calibrate(dsi->slave); >> + if (err < 0) { >> + dev_err(dsi->slave->dev, >> + "MIPI calibration failed: %d\n", err); >> + return err; >> + } >> + } > > Move this duplicate call into tegra_dsi_pad_calibrate() to be > consistent with the other functions in this file (eg: > tegra_dsi_set_timeout). > Yeah, will do. >> + >> err = tegra_dsi_get_muldiv(dsi->format, &mul, &div); >> if (err < 0) >> return err; >> @@ -833,6 +850,13 @@ static int tegra_output_dsi_setup_clock(struct tegra_output *output, >> dev_err(dsi->dev, "failed to set parent clock: %d\n", err); >> return err; >> } >> + if (dsi->slave) { >> + err = tegra_dsi_ganged_setup(dsi); >> + if (err < 0) { >> + dev_err(dsi->dev, "DSI ganged setup failed: %d\n", err); >> + return err; >> + } >> + } > > I think this belongs in ->enable() instead of here. > Actually "tegra_dsi_ganged_setup" is not needed any more. This function ensures that DSIA & DSIB use the same PLL in ganged mode. But if you read the Tegra124/Tegra132 TRM, you'll find there is no way to select the clock source for DSIB. I.E, DSIB always uses the same clock source with DSIA. Besides, PLLD is the only clock source for DSIA. I.E, "clk_get_parent"/"clk_set_parent" doesn't make sense for dsia & dsib clock now. I've posted a patch(in tegra clock driver) to fix this: http://article.gmane.org/gmane.linux.ports.tegra/20313 And the corresponding changes in dsi driver will arrive soon. >> >> err = clk_set_rate(dsi->clk_parent, plld); >> if (err < 0) { [...] >> + >> + drm_modeset_lock_all(tegra->drm); >> + list_for_each_entry(connector, &tegra->drm->mode_config.connector_list, >> + head) { >> + if (connector->funcs->dpms) >> + connector->funcs->dpms(connector, connector->dpms); >> + } >> + drm_modeset_unlock_all(tegra->drm); >> + >> + drm_helper_resume_force_mode(tegra->drm); >> + >> + return 0; >> +} >> +#endif > > So this is the tricky chunk, it definitely does not belong here. I > think it makes most sense for this to live in drm.c, however > host1x_driver doesn't have suspend/resume hooks. > Agree. drm.c is the best place for putting this. > I suspect the correct thing to do here is to add them to > host1x_driver, but I'm not sure how much effort is involved in doing > that. > I thought about this before. If we do it in host1x driver, IIUC, it means that when host1x starts suspending, it will suspend all host1x client devices, right? Honestly I feel this is not a good thing despite I can't tell what's the problem in this right now... Mark > Sean > >> + >> + >> static const struct of_device_id tegra_dsi_of_match[] = { >> { .compatible = "nvidia,tegra114-dsi", }, >> { }, >> @@ -1557,4 +1636,9 @@ struct platform_driver tegra_dsi_driver = { >> }, >> .probe = tegra_dsi_probe, >> .remove = tegra_dsi_remove, >> +#ifdef CONFIG_PM >> + .suspend = tegra_dsi_suspend, >> + .resume = tegra_dsi_resume, >> +#endif >> + >> }; >> -- >> 1.8.1.5 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Dec 10, 2014 at 10:08:57AM +0800, Mark Zhang wrote: > On 12/10/2014 03:29 AM, Sean Paul wrote: > > On Sun, Dec 7, 2014 at 10:40 PM, Mark Zhang <markz@nvidia.com> wrote: [...] > >> > >> err = clk_set_rate(dsi->clk_parent, plld); > >> if (err < 0) { > [...] > >> + > >> + drm_modeset_lock_all(tegra->drm); > >> + list_for_each_entry(connector, &tegra->drm->mode_config.connector_list, > >> + head) { > >> + if (connector->funcs->dpms) > >> + connector->funcs->dpms(connector, connector->dpms); > >> + } > >> + drm_modeset_unlock_all(tegra->drm); > >> + > >> + drm_helper_resume_force_mode(tegra->drm); > >> + > >> + return 0; > >> +} > >> +#endif > > > > So this is the tricky chunk, it definitely does not belong here. I > > think it makes most sense for this to live in drm.c, however > > host1x_driver doesn't have suspend/resume hooks. > > > > Agree. drm.c is the best place for putting this. > > > I suspect the correct thing to do here is to add them to > > host1x_driver, but I'm not sure how much effort is involved in doing > > that. > > > > I thought about this before. If we do it in host1x driver, IIUC, it > means that when host1x starts suspending, it will suspend all host1x > client devices, right? Honestly I feel this is not a good thing despite > I can't tell what's the problem in this right now... I've just sent out patches for review that add the missing infrastructure to properly do suspend/resume. The idea behind this is that the DRM host1x device's ->pm_ops are responsible for dealing with the suspend/resume at a subsystem level (call connectors' ->dpms() callbacks, etc.) whereas more fine-grained suspend/resume can further be done in the ->pm_ops of the individual subdevices. The infrastructure makes sure that these get called in the right order. Thierry
diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c index 33f67fd601c6..25cd0d93f392 100644 --- a/drivers/gpu/drm/tegra/dsi.c +++ b/drivers/gpu/drm/tegra/dsi.c @@ -61,6 +61,9 @@ struct tegra_dsi { struct tegra_dsi *slave; }; +static int tegra_dsi_pad_calibrate(struct tegra_dsi *); +static int tegra_dsi_ganged_setup(struct tegra_dsi *dsi); + static inline struct tegra_dsi * host1x_client_to_dsi(struct host1x_client *client) { @@ -805,6 +808,20 @@ static int tegra_output_dsi_setup_clock(struct tegra_output *output, lanes = tegra_dsi_get_lanes(dsi); + err = tegra_dsi_pad_calibrate(dsi); + if (err < 0) { + dev_err(dsi->dev, "MIPI calibration failed: %d\n", err); + return err; + } + if (dsi->slave) { + err = tegra_dsi_pad_calibrate(dsi->slave); + if (err < 0) { + dev_err(dsi->slave->dev, + "MIPI calibration failed: %d\n", err); + return err; + } + } + err = tegra_dsi_get_muldiv(dsi->format, &mul, &div); if (err < 0) return err; @@ -833,6 +850,13 @@ static int tegra_output_dsi_setup_clock(struct tegra_output *output, dev_err(dsi->dev, "failed to set parent clock: %d\n", err); return err; } + if (dsi->slave) { + err = tegra_dsi_ganged_setup(dsi); + if (err < 0) { + dev_err(dsi->dev, "DSI ganged setup failed: %d\n", err); + return err; + } + } err = clk_set_rate(dsi->clk_parent, plld); if (err < 0) { @@ -1470,12 +1494,6 @@ static int tegra_dsi_probe(struct platform_device *pdev) goto disable_vdd; } - err = tegra_dsi_pad_calibrate(dsi); - if (err < 0) { - dev_err(dsi->dev, "MIPI calibration failed: %d\n", err); - goto mipi_free; - } - dsi->host.ops = &tegra_dsi_host_ops; dsi->host.dev = &pdev->dev; @@ -1544,6 +1562,67 @@ static int tegra_dsi_remove(struct platform_device *pdev) return 0; } +#ifdef CONFIG_PM +static int tegra_dsi_suspend(struct platform_device *pdev, pm_message_t state) +{ + struct tegra_dsi *dsi = platform_get_drvdata(pdev); + struct tegra_drm *tegra = dev_get_drvdata(dsi->client.parent); + struct drm_connector *connector; + + if (dsi->master) { + regulator_disable(dsi->vdd); + return 0; + } + + drm_modeset_lock_all(tegra->drm); + list_for_each_entry(connector, &tegra->drm->mode_config.connector_list, + head) { + int old_dpms = connector->dpms; + + if (connector->funcs->dpms) + connector->funcs->dpms(connector, DRM_MODE_DPMS_OFF); + + /* Set the old mode back to the connector for resume */ + connector->dpms = old_dpms; + } + drm_modeset_unlock_all(tegra->drm); + + regulator_disable(dsi->vdd); + + return 0; +} + +static int tegra_dsi_resume(struct platform_device *pdev) +{ + struct tegra_dsi *dsi = platform_get_drvdata(pdev); + struct tegra_drm *tegra = dev_get_drvdata(dsi->client.parent); + struct drm_connector *connector; + int err = 0; + + err = regulator_enable(dsi->vdd); + if (err < 0) { + dev_err(&pdev->dev, "Enable DSI vdd failed: %d\n", err); + return err; + } + + if (dsi->master) + return 0; + + drm_modeset_lock_all(tegra->drm); + list_for_each_entry(connector, &tegra->drm->mode_config.connector_list, + head) { + if (connector->funcs->dpms) + connector->funcs->dpms(connector, connector->dpms); + } + drm_modeset_unlock_all(tegra->drm); + + drm_helper_resume_force_mode(tegra->drm); + + return 0; +} +#endif + + static const struct of_device_id tegra_dsi_of_match[] = { { .compatible = "nvidia,tegra114-dsi", }, { }, @@ -1557,4 +1636,9 @@ struct platform_driver tegra_dsi_driver = { }, .probe = tegra_dsi_probe, .remove = tegra_dsi_remove, +#ifdef CONFIG_PM + .suspend = tegra_dsi_suspend, + .resume = tegra_dsi_resume, +#endif + };
This patch adds the suspend/resume support for Tegra drm driver by calling the corresponding DPMS functions. Signed-off-by: Mark Zhang <markz@nvidia.com> --- Hi, This patch hooks DSI driver's suspend/resume to implement the whole display system's suspend/resume. I know this is a super ugly way, but as we all know, Tegra DRM driver doesn't have a dedicate drm device so honestly I didn't find a better way to do that. Thanks, Mark drivers/gpu/drm/tegra/dsi.c | 96 ++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 90 insertions(+), 6 deletions(-)