* RFC: Add drm_dev_suspend/resume() ?
@ 2017-10-22 16:52 Noralf Trønnes
2017-10-30 9:34 ` Daniel Vetter
0 siblings, 1 reply; 6+ messages in thread
From: Noralf Trønnes @ 2017-10-22 16:52 UTC (permalink / raw)
To: dri-devel
Hi,
I've spent some time in the fbdev emulation code and discovered a
recurring pattern around suspend/resume.
Should we add some more helpers :-)
struct drm_device {
/**
* @suspend_state:
*
* Atomic state when suspended.
* Set by drm_dev_suspend(), cleared by drm_dev_resume().
*/
struct drm_atomic_state *suspend_state;
};
int drm_dev_suspend(struct drm_device *dev)
{
struct drm_atomic_state *state;
if (!dev)
return 0;
drm_kms_helper_poll_disable(dev);
drm_fb_helper_set_suspend_unlocked(dev->fb_helper, 1);
state = drm_atomic_helper_suspend(dev);
if (IS_ERR(state)) {
drm_fb_helper_set_suspend_unlocked(dev->fb_helper, 0);
drm_kms_helper_poll_enable(dev);
return PTR_ERR(state);
}
dev->suspend_state = state;
return 0;
}
int drm_dev_resume(struct drm_device *dev)
{
int ret;
if (!dev || WARN_ON(!dev->suspend_state))
return 0;
ret = drm_atomic_helper_resume(dev, dev->suspend_state);
if (ret)
DRM_ERROR("Failed to resume (%d)\n", ret);
dev->suspend_state = NULL;
drm_fb_helper_set_suspend_unlocked(dev->fb_helper, 0);
drm_kms_helper_poll_enable(dev);
return 0;
}
The drm_kms_helper_poll_enable() docs states that it's not allowed to
call this function if polling support has been set up, but the code
allows it. drm_kms_helper_poll_disable() docs states that it's ok to
call it even if polling is not enabled.
Here are the suspend/resume code from the drivers that use
drm_atomic_helper_suspend/resume():
static int __maybe_unused hdlcd_pm_suspend(struct device *dev)
{
struct drm_device *drm = dev_get_drvdata(dev);
struct hdlcd_drm_private *hdlcd = drm ? drm->dev_private : NULL;
if (!hdlcd)
return 0;
drm_kms_helper_poll_disable(drm);
hdlcd->state = drm_atomic_helper_suspend(drm);
if (IS_ERR(hdlcd->state)) {
drm_kms_helper_poll_enable(drm);
return PTR_ERR(hdlcd->state);
}
return 0;
}
static int __maybe_unused hdlcd_pm_resume(struct device *dev)
{
struct drm_device *drm = dev_get_drvdata(dev);
struct hdlcd_drm_private *hdlcd = drm ? drm->dev_private : NULL;
if (!hdlcd)
return 0;
drm_atomic_helper_resume(drm, hdlcd->state);
drm_kms_helper_poll_enable(drm);
pm_runtime_set_active(dev);
return 0;
}
static int __maybe_unused malidp_pm_suspend(struct device *dev)
{
struct drm_device *drm = dev_get_drvdata(dev);
struct malidp_drm *malidp = drm->dev_private;
drm_kms_helper_poll_disable(drm);
console_lock();
drm_fbdev_cma_set_suspend(malidp->fbdev, 1);
console_unlock();
malidp->pm_state = drm_atomic_helper_suspend(drm);
if (IS_ERR(malidp->pm_state)) {
console_lock();
drm_fbdev_cma_set_suspend(malidp->fbdev, 0);
console_unlock();
drm_kms_helper_poll_enable(drm);
return PTR_ERR(malidp->pm_state);
}
return 0;
}
static int __maybe_unused malidp_pm_resume(struct device *dev)
{
struct drm_device *drm = dev_get_drvdata(dev);
struct malidp_drm *malidp = drm->dev_private;
drm_atomic_helper_resume(drm, malidp->pm_state);
console_lock();
drm_fbdev_cma_set_suspend(malidp->fbdev, 0);
console_unlock();
drm_kms_helper_poll_enable(drm);
return 0;
}
static int atmel_hlcdc_dc_drm_suspend(struct device *dev)
{
struct drm_device *drm_dev = dev_get_drvdata(dev);
struct atmel_hlcdc_dc *dc = drm_dev->dev_private;
struct regmap *regmap = dc->hlcdc->regmap;
struct drm_atomic_state *state;
state = drm_atomic_helper_suspend(drm_dev);
if (IS_ERR(state))
return PTR_ERR(state);
dc->suspend.state = state;
regmap_read(regmap, ATMEL_HLCDC_IMR, &dc->suspend.imr);
regmap_write(regmap, ATMEL_HLCDC_IDR, dc->suspend.imr);
clk_disable_unprepare(dc->hlcdc->periph_clk);
return 0;
}
static int atmel_hlcdc_dc_drm_resume(struct device *dev)
{
struct drm_device *drm_dev = dev_get_drvdata(dev);
struct atmel_hlcdc_dc *dc = drm_dev->dev_private;
clk_prepare_enable(dc->hlcdc->periph_clk);
regmap_write(dc->hlcdc->regmap, ATMEL_HLCDC_IER, dc->suspend.imr);
return drm_atomic_helper_resume(drm_dev, dc->suspend.state);
}
static int exynos_drm_suspend(struct device *dev)
{
struct drm_device *drm_dev = dev_get_drvdata(dev);
struct exynos_drm_private *private = drm_dev->dev_private;
if (pm_runtime_suspended(dev) || !drm_dev)
return 0;
drm_kms_helper_poll_disable(drm_dev);
exynos_drm_fbdev_suspend(drm_dev);
private->suspend_state = drm_atomic_helper_suspend(drm_dev);
if (IS_ERR(private->suspend_state)) {
exynos_drm_fbdev_resume(drm_dev);
drm_kms_helper_poll_enable(drm_dev);
return PTR_ERR(private->suspend_state);
}
return 0;
}
static int exynos_drm_resume(struct device *dev)
{
struct drm_device *drm_dev = dev_get_drvdata(dev);
struct exynos_drm_private *private = drm_dev->dev_private;
if (pm_runtime_suspended(dev) || !drm_dev)
return 0;
drm_atomic_helper_resume(drm_dev, private->suspend_state);
exynos_drm_fbdev_resume(drm_dev);
drm_kms_helper_poll_enable(drm_dev);
return 0;
}
static int fsl_dcu_drm_pm_suspend(struct device *dev)
{
struct fsl_dcu_drm_device *fsl_dev = dev_get_drvdata(dev);
if (!fsl_dev)
return 0;
disable_irq(fsl_dev->irq);
drm_kms_helper_poll_disable(fsl_dev->drm);
console_lock();
drm_fbdev_cma_set_suspend(fsl_dev->fbdev, 1);
console_unlock();
fsl_dev->state = drm_atomic_helper_suspend(fsl_dev->drm);
if (IS_ERR(fsl_dev->state)) {
console_lock();
drm_fbdev_cma_set_suspend(fsl_dev->fbdev, 0);
console_unlock();
drm_kms_helper_poll_enable(fsl_dev->drm);
enable_irq(fsl_dev->irq);
return PTR_ERR(fsl_dev->state);
}
clk_disable_unprepare(fsl_dev->pix_clk);
clk_disable_unprepare(fsl_dev->clk);
return 0;
}
static int fsl_dcu_drm_pm_resume(struct device *dev)
{
struct fsl_dcu_drm_device *fsl_dev = dev_get_drvdata(dev);
int ret;
if (!fsl_dev)
return 0;
ret = clk_prepare_enable(fsl_dev->clk);
if (ret < 0) {
dev_err(dev, "failed to enable dcu clk\n");
return ret;
}
if (fsl_dev->tcon)
fsl_tcon_bypass_enable(fsl_dev->tcon);
fsl_dcu_drm_init_planes(fsl_dev->drm);
drm_atomic_helper_resume(fsl_dev->drm, fsl_dev->state);
console_lock();
drm_fbdev_cma_set_suspend(fsl_dev->fbdev, 0);
console_unlock();
drm_kms_helper_poll_enable(fsl_dev->drm);
enable_irq(fsl_dev->irq);
return 0;
}
static int __maybe_unused hibmc_pm_suspend(struct device *dev)
{
struct pci_dev *pdev = to_pci_dev(dev);
struct drm_device *drm_dev = pci_get_drvdata(pdev);
struct hibmc_drm_private *priv = drm_dev->dev_private;
drm_kms_helper_poll_disable(drm_dev);
priv->suspend_state = drm_atomic_helper_suspend(drm_dev);
if (IS_ERR(priv->suspend_state)) {
DRM_ERROR("drm_atomic_helper_suspend failed: %ld\n",
PTR_ERR(priv->suspend_state));
drm_kms_helper_poll_enable(drm_dev);
return PTR_ERR(priv->suspend_state);
}
return 0;
}
static int __maybe_unused hibmc_pm_resume(struct device *dev)
{
struct pci_dev *pdev = to_pci_dev(dev);
struct drm_device *drm_dev = pci_get_drvdata(pdev);
struct hibmc_drm_private *priv = drm_dev->dev_private;
drm_atomic_helper_resume(drm_dev, priv->suspend_state);
drm_kms_helper_poll_enable(drm_dev);
return 0;
}
i915 is excluded since it's rather complicated.
static int imx_drm_suspend(struct device *dev)
{
struct drm_device *drm_dev = dev_get_drvdata(dev);
struct imx_drm_device *imxdrm;
/* The drm_dev is NULL before .load hook is called */
if (drm_dev == NULL)
return 0;
drm_kms_helper_poll_disable(drm_dev);
imxdrm = drm_dev->dev_private;
imxdrm->state = drm_atomic_helper_suspend(drm_dev);
if (IS_ERR(imxdrm->state)) {
drm_kms_helper_poll_enable(drm_dev);
return PTR_ERR(imxdrm->state);
}
return 0;
}
static int imx_drm_resume(struct device *dev)
{
struct drm_device *drm_dev = dev_get_drvdata(dev);
struct imx_drm_device *imx_drm;
if (drm_dev == NULL)
return 0;
imx_drm = drm_dev->dev_private;
drm_atomic_helper_resume(drm_dev, imx_drm->state);
drm_kms_helper_poll_enable(drm_dev);
return 0;
}
static int mtk_drm_sys_suspend(struct device *dev)
{
struct mtk_drm_private *private = dev_get_drvdata(dev);
struct drm_device *drm = private->drm;
drm_kms_helper_poll_disable(drm);
private->suspend_state = drm_atomic_helper_suspend(drm);
if (IS_ERR(private->suspend_state)) {
drm_kms_helper_poll_enable(drm);
return PTR_ERR(private->suspend_state);
}
DRM_DEBUG_DRIVER("mtk_drm_sys_suspend\n");
return 0;
}
static int mtk_drm_sys_resume(struct device *dev)
{
struct mtk_drm_private *private = dev_get_drvdata(dev);
struct drm_device *drm = private->drm;
drm_atomic_helper_resume(drm, private->suspend_state);
drm_kms_helper_poll_enable(drm);
DRM_DEBUG_DRIVER("mtk_drm_sys_resume\n");
return 0;
}
nouveau is also rather complicated.
static int rockchip_drm_sys_suspend(struct device *dev)
{
struct drm_device *drm = dev_get_drvdata(dev);
struct rockchip_drm_private *priv;
if (!drm)
return 0;
drm_kms_helper_poll_disable(drm);
rockchip_drm_fb_suspend(drm);
priv = drm->dev_private;
priv->state = drm_atomic_helper_suspend(drm);
if (IS_ERR(priv->state)) {
rockchip_drm_fb_resume(drm);
drm_kms_helper_poll_enable(drm);
return PTR_ERR(priv->state);
}
return 0;
}
static int rockchip_drm_sys_resume(struct device *dev)
{
struct drm_device *drm = dev_get_drvdata(dev);
struct rockchip_drm_private *priv;
if (!drm)
return 0;
priv = drm->dev_private;
drm_atomic_helper_resume(drm, priv->state);
rockchip_drm_fb_resume(drm);
drm_kms_helper_poll_enable(drm);
return 0;
}
static int host1x_drm_suspend(struct device *dev)
{
struct drm_device *drm = dev_get_drvdata(dev);
struct tegra_drm *tegra = drm->dev_private;
drm_kms_helper_poll_disable(drm);
tegra_drm_fb_suspend(drm);
tegra->state = drm_atomic_helper_suspend(drm);
if (IS_ERR(tegra->state)) {
tegra_drm_fb_resume(drm);
drm_kms_helper_poll_enable(drm);
return PTR_ERR(tegra->state);
}
return 0;
}
static int host1x_drm_resume(struct device *dev)
{
struct drm_device *drm = dev_get_drvdata(dev);
struct tegra_drm *tegra = drm->dev_private;
drm_atomic_helper_resume(drm, tegra->state);
tegra_drm_fb_resume(drm);
drm_kms_helper_poll_enable(drm);
return 0;
}
static int tilcdc_pm_suspend(struct device *dev)
{
struct drm_device *ddev = dev_get_drvdata(dev);
struct tilcdc_drm_private *priv = ddev->dev_private;
priv->saved_state = drm_atomic_helper_suspend(ddev);
/* Select sleep pin state */
pinctrl_pm_select_sleep_state(dev);
return 0;
}
static int tilcdc_pm_resume(struct device *dev)
{
struct drm_device *ddev = dev_get_drvdata(dev);
struct tilcdc_drm_private *priv = ddev->dev_private;
int ret = 0;
/* Select default pin state */
pinctrl_pm_select_default_state(dev);
if (priv->saved_state)
ret = drm_atomic_helper_resume(ddev, priv->saved_state);
return ret;
}
int tinydrm_suspend(struct tinydrm_device *tdev)
{
struct drm_atomic_state *state;
if (tdev->suspend_state) {
DRM_ERROR("Failed to suspend: state already set\n");
return -EINVAL;
}
drm_fbdev_cma_set_suspend_unlocked(tdev->fbdev_cma, 1);
state = drm_atomic_helper_suspend(tdev->drm);
if (IS_ERR(state)) {
drm_fbdev_cma_set_suspend_unlocked(tdev->fbdev_cma, 0);
return PTR_ERR(state);
}
tdev->suspend_state = state;
return 0;
}
int tinydrm_resume(struct tinydrm_device *tdev)
{
struct drm_atomic_state *state = tdev->suspend_state;
int ret;
if (!state) {
DRM_ERROR("Failed to resume: state is not set\n");
return -EINVAL;
}
tdev->suspend_state = NULL;
ret = drm_atomic_helper_resume(tdev->drm, state);
if (ret) {
DRM_ERROR("Error resuming state: %d\n", ret);
return ret;
}
drm_fbdev_cma_set_suspend_unlocked(tdev->fbdev_cma, 0);
return 0;
}
Noralf.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: RFC: Add drm_dev_suspend/resume() ?
2017-10-22 16:52 RFC: Add drm_dev_suspend/resume() ? Noralf Trønnes
@ 2017-10-30 9:34 ` Daniel Vetter
2017-10-31 16:37 ` Noralf Trønnes
0 siblings, 1 reply; 6+ messages in thread
From: Daniel Vetter @ 2017-10-30 9:34 UTC (permalink / raw)
To: Noralf Trønnes; +Cc: dri-devel
Hi Noralf,
On Sun, Oct 22, 2017 at 06:52:41PM +0200, Noralf Trønnes wrote:
> Hi,
>
> I've spent some time in the fbdev emulation code and discovered a
> recurring pattern around suspend/resume.
> Should we add some more helpers :-)
You're maybe a bit too good at spotting these for your own good :-)
But yeah, a "suspend for dummies" is one of the things which would be nice
I think ... Especially since we now have the atomic suspend/resume
helpers.
> struct drm_device {
> /**
> * @suspend_state:
> *
> * Atomic state when suspended.
> * Set by drm_dev_suspend(), cleared by drm_dev_resume().
> */
> struct drm_atomic_state *suspend_state;
> };
Imo fits better when we put it into drm_mode_config.
> int drm_dev_suspend(struct drm_device *dev)
> {
> struct drm_atomic_state *state;
>
> if (!dev)
> return 0;
>
> drm_kms_helper_poll_disable(dev);
> drm_fb_helper_set_suspend_unlocked(dev->fb_helper, 1);
> state = drm_atomic_helper_suspend(dev);
> if (IS_ERR(state)) {
> drm_fb_helper_set_suspend_unlocked(dev->fb_helper, 0);
> drm_kms_helper_poll_enable(dev);
> return PTR_ERR(state);
> }
>
> dev->suspend_state = state;
>
> return 0;
> }
This is all about suspending the modeset side ... I'd give it a
drm_mode_config prefix instead of the drm_dev_ (that's maybe a bit too
generic), but then maybe type a general suspend/resume kernel-doc text in
the drm-internals.rst (maybe pulled in from drm_dev.c) which references
these 2 functions as the recommended way to suspend/resume the modeset
side of a driver. These won't suspend/resume a render part (if present),
so drm_dev_ seems a bit too much.
> int drm_dev_resume(struct drm_device *dev)
> {
> int ret;
>
> if (!dev || WARN_ON(!dev->suspend_state))
> return 0;
>
> ret = drm_atomic_helper_resume(dev, dev->suspend_state);
> if (ret)
> DRM_ERROR("Failed to resume (%d)\n", ret);
> dev->suspend_state = NULL;
>
> drm_fb_helper_set_suspend_unlocked(dev->fb_helper, 0);
> drm_kms_helper_poll_enable(dev);
>
> return 0;
> }
>
> The drm_kms_helper_poll_enable() docs states that it's not allowed to
> call this function if polling support has been set up, but the code
> allows it. drm_kms_helper_poll_disable() docs states that it's ok to
> call it even if polling is not enabled.
Yeah need to fix that, but need to keep a note that if you want polling,
then you need to follow that order or it goes wrong. Perhaps explain that
this is meant to avoid special-cases in suspend/resume code?
> Here are the suspend/resume code from the drivers that use
> drm_atomic_helper_suspend/resume():
Yeah, looks like a really good idea.
Thanks, Daniel
>
> static int __maybe_unused hdlcd_pm_suspend(struct device *dev)
> {
> struct drm_device *drm = dev_get_drvdata(dev);
> struct hdlcd_drm_private *hdlcd = drm ? drm->dev_private : NULL;
>
> if (!hdlcd)
> return 0;
>
> drm_kms_helper_poll_disable(drm);
>
> hdlcd->state = drm_atomic_helper_suspend(drm);
> if (IS_ERR(hdlcd->state)) {
> drm_kms_helper_poll_enable(drm);
> return PTR_ERR(hdlcd->state);
> }
>
> return 0;
> }
>
> static int __maybe_unused hdlcd_pm_resume(struct device *dev)
> {
> struct drm_device *drm = dev_get_drvdata(dev);
> struct hdlcd_drm_private *hdlcd = drm ? drm->dev_private : NULL;
>
> if (!hdlcd)
> return 0;
>
> drm_atomic_helper_resume(drm, hdlcd->state);
> drm_kms_helper_poll_enable(drm);
> pm_runtime_set_active(dev);
>
> return 0;
> }
>
>
> static int __maybe_unused malidp_pm_suspend(struct device *dev)
> {
> struct drm_device *drm = dev_get_drvdata(dev);
> struct malidp_drm *malidp = drm->dev_private;
>
> drm_kms_helper_poll_disable(drm);
> console_lock();
> drm_fbdev_cma_set_suspend(malidp->fbdev, 1);
> console_unlock();
> malidp->pm_state = drm_atomic_helper_suspend(drm);
> if (IS_ERR(malidp->pm_state)) {
> console_lock();
> drm_fbdev_cma_set_suspend(malidp->fbdev, 0);
> console_unlock();
> drm_kms_helper_poll_enable(drm);
> return PTR_ERR(malidp->pm_state);
> }
>
> return 0;
> }
>
> static int __maybe_unused malidp_pm_resume(struct device *dev)
> {
> struct drm_device *drm = dev_get_drvdata(dev);
> struct malidp_drm *malidp = drm->dev_private;
>
> drm_atomic_helper_resume(drm, malidp->pm_state);
> console_lock();
> drm_fbdev_cma_set_suspend(malidp->fbdev, 0);
> console_unlock();
> drm_kms_helper_poll_enable(drm);
>
> return 0;
> }
>
>
> static int atmel_hlcdc_dc_drm_suspend(struct device *dev)
> {
> struct drm_device *drm_dev = dev_get_drvdata(dev);
> struct atmel_hlcdc_dc *dc = drm_dev->dev_private;
> struct regmap *regmap = dc->hlcdc->regmap;
> struct drm_atomic_state *state;
>
> state = drm_atomic_helper_suspend(drm_dev);
> if (IS_ERR(state))
> return PTR_ERR(state);
>
> dc->suspend.state = state;
>
> regmap_read(regmap, ATMEL_HLCDC_IMR, &dc->suspend.imr);
> regmap_write(regmap, ATMEL_HLCDC_IDR, dc->suspend.imr);
> clk_disable_unprepare(dc->hlcdc->periph_clk);
>
> return 0;
> }
>
> static int atmel_hlcdc_dc_drm_resume(struct device *dev)
> {
> struct drm_device *drm_dev = dev_get_drvdata(dev);
> struct atmel_hlcdc_dc *dc = drm_dev->dev_private;
>
> clk_prepare_enable(dc->hlcdc->periph_clk);
> regmap_write(dc->hlcdc->regmap, ATMEL_HLCDC_IER, dc->suspend.imr);
>
> return drm_atomic_helper_resume(drm_dev, dc->suspend.state);
> }
>
>
> static int exynos_drm_suspend(struct device *dev)
> {
> struct drm_device *drm_dev = dev_get_drvdata(dev);
> struct exynos_drm_private *private = drm_dev->dev_private;
>
> if (pm_runtime_suspended(dev) || !drm_dev)
> return 0;
>
> drm_kms_helper_poll_disable(drm_dev);
> exynos_drm_fbdev_suspend(drm_dev);
> private->suspend_state = drm_atomic_helper_suspend(drm_dev);
> if (IS_ERR(private->suspend_state)) {
> exynos_drm_fbdev_resume(drm_dev);
> drm_kms_helper_poll_enable(drm_dev);
> return PTR_ERR(private->suspend_state);
> }
>
> return 0;
> }
>
> static int exynos_drm_resume(struct device *dev)
> {
> struct drm_device *drm_dev = dev_get_drvdata(dev);
> struct exynos_drm_private *private = drm_dev->dev_private;
>
> if (pm_runtime_suspended(dev) || !drm_dev)
> return 0;
>
> drm_atomic_helper_resume(drm_dev, private->suspend_state);
> exynos_drm_fbdev_resume(drm_dev);
> drm_kms_helper_poll_enable(drm_dev);
>
> return 0;
> }
>
>
> static int fsl_dcu_drm_pm_suspend(struct device *dev)
> {
> struct fsl_dcu_drm_device *fsl_dev = dev_get_drvdata(dev);
>
> if (!fsl_dev)
> return 0;
>
> disable_irq(fsl_dev->irq);
> drm_kms_helper_poll_disable(fsl_dev->drm);
>
> console_lock();
> drm_fbdev_cma_set_suspend(fsl_dev->fbdev, 1);
> console_unlock();
>
> fsl_dev->state = drm_atomic_helper_suspend(fsl_dev->drm);
> if (IS_ERR(fsl_dev->state)) {
> console_lock();
> drm_fbdev_cma_set_suspend(fsl_dev->fbdev, 0);
> console_unlock();
>
> drm_kms_helper_poll_enable(fsl_dev->drm);
> enable_irq(fsl_dev->irq);
> return PTR_ERR(fsl_dev->state);
> }
>
> clk_disable_unprepare(fsl_dev->pix_clk);
> clk_disable_unprepare(fsl_dev->clk);
>
> return 0;
> }
>
> static int fsl_dcu_drm_pm_resume(struct device *dev)
> {
> struct fsl_dcu_drm_device *fsl_dev = dev_get_drvdata(dev);
> int ret;
>
> if (!fsl_dev)
> return 0;
>
> ret = clk_prepare_enable(fsl_dev->clk);
> if (ret < 0) {
> dev_err(dev, "failed to enable dcu clk\n");
> return ret;
> }
>
> if (fsl_dev->tcon)
> fsl_tcon_bypass_enable(fsl_dev->tcon);
> fsl_dcu_drm_init_planes(fsl_dev->drm);
> drm_atomic_helper_resume(fsl_dev->drm, fsl_dev->state);
>
> console_lock();
> drm_fbdev_cma_set_suspend(fsl_dev->fbdev, 0);
> console_unlock();
>
> drm_kms_helper_poll_enable(fsl_dev->drm);
> enable_irq(fsl_dev->irq);
>
> return 0;
> }
>
>
> static int __maybe_unused hibmc_pm_suspend(struct device *dev)
> {
> struct pci_dev *pdev = to_pci_dev(dev);
> struct drm_device *drm_dev = pci_get_drvdata(pdev);
> struct hibmc_drm_private *priv = drm_dev->dev_private;
>
> drm_kms_helper_poll_disable(drm_dev);
> priv->suspend_state = drm_atomic_helper_suspend(drm_dev);
> if (IS_ERR(priv->suspend_state)) {
> DRM_ERROR("drm_atomic_helper_suspend failed: %ld\n",
> PTR_ERR(priv->suspend_state));
> drm_kms_helper_poll_enable(drm_dev);
> return PTR_ERR(priv->suspend_state);
> }
>
> return 0;
> }
>
> static int __maybe_unused hibmc_pm_resume(struct device *dev)
> {
> struct pci_dev *pdev = to_pci_dev(dev);
> struct drm_device *drm_dev = pci_get_drvdata(pdev);
> struct hibmc_drm_private *priv = drm_dev->dev_private;
>
> drm_atomic_helper_resume(drm_dev, priv->suspend_state);
> drm_kms_helper_poll_enable(drm_dev);
>
> return 0;
> }
>
>
> i915 is excluded since it's rather complicated.
>
>
> static int imx_drm_suspend(struct device *dev)
> {
> struct drm_device *drm_dev = dev_get_drvdata(dev);
> struct imx_drm_device *imxdrm;
>
> /* The drm_dev is NULL before .load hook is called */
> if (drm_dev == NULL)
> return 0;
>
> drm_kms_helper_poll_disable(drm_dev);
>
> imxdrm = drm_dev->dev_private;
> imxdrm->state = drm_atomic_helper_suspend(drm_dev);
> if (IS_ERR(imxdrm->state)) {
> drm_kms_helper_poll_enable(drm_dev);
> return PTR_ERR(imxdrm->state);
> }
>
> return 0;
> }
>
> static int imx_drm_resume(struct device *dev)
> {
> struct drm_device *drm_dev = dev_get_drvdata(dev);
> struct imx_drm_device *imx_drm;
>
> if (drm_dev == NULL)
> return 0;
>
> imx_drm = drm_dev->dev_private;
> drm_atomic_helper_resume(drm_dev, imx_drm->state);
> drm_kms_helper_poll_enable(drm_dev);
>
> return 0;
> }
>
>
> static int mtk_drm_sys_suspend(struct device *dev)
> {
> struct mtk_drm_private *private = dev_get_drvdata(dev);
> struct drm_device *drm = private->drm;
>
> drm_kms_helper_poll_disable(drm);
>
> private->suspend_state = drm_atomic_helper_suspend(drm);
> if (IS_ERR(private->suspend_state)) {
> drm_kms_helper_poll_enable(drm);
> return PTR_ERR(private->suspend_state);
> }
>
> DRM_DEBUG_DRIVER("mtk_drm_sys_suspend\n");
> return 0;
> }
>
> static int mtk_drm_sys_resume(struct device *dev)
> {
> struct mtk_drm_private *private = dev_get_drvdata(dev);
> struct drm_device *drm = private->drm;
>
> drm_atomic_helper_resume(drm, private->suspend_state);
> drm_kms_helper_poll_enable(drm);
>
> DRM_DEBUG_DRIVER("mtk_drm_sys_resume\n");
> return 0;
> }
>
>
> nouveau is also rather complicated.
>
>
> static int rockchip_drm_sys_suspend(struct device *dev)
> {
> struct drm_device *drm = dev_get_drvdata(dev);
> struct rockchip_drm_private *priv;
>
> if (!drm)
> return 0;
>
> drm_kms_helper_poll_disable(drm);
> rockchip_drm_fb_suspend(drm);
>
> priv = drm->dev_private;
> priv->state = drm_atomic_helper_suspend(drm);
> if (IS_ERR(priv->state)) {
> rockchip_drm_fb_resume(drm);
> drm_kms_helper_poll_enable(drm);
> return PTR_ERR(priv->state);
> }
>
> return 0;
> }
>
> static int rockchip_drm_sys_resume(struct device *dev)
> {
> struct drm_device *drm = dev_get_drvdata(dev);
> struct rockchip_drm_private *priv;
>
> if (!drm)
> return 0;
>
> priv = drm->dev_private;
> drm_atomic_helper_resume(drm, priv->state);
> rockchip_drm_fb_resume(drm);
> drm_kms_helper_poll_enable(drm);
>
> return 0;
> }
>
>
> static int host1x_drm_suspend(struct device *dev)
> {
> struct drm_device *drm = dev_get_drvdata(dev);
> struct tegra_drm *tegra = drm->dev_private;
>
> drm_kms_helper_poll_disable(drm);
> tegra_drm_fb_suspend(drm);
>
> tegra->state = drm_atomic_helper_suspend(drm);
> if (IS_ERR(tegra->state)) {
> tegra_drm_fb_resume(drm);
> drm_kms_helper_poll_enable(drm);
> return PTR_ERR(tegra->state);
> }
>
> return 0;
> }
>
> static int host1x_drm_resume(struct device *dev)
> {
> struct drm_device *drm = dev_get_drvdata(dev);
> struct tegra_drm *tegra = drm->dev_private;
>
> drm_atomic_helper_resume(drm, tegra->state);
> tegra_drm_fb_resume(drm);
> drm_kms_helper_poll_enable(drm);
>
> return 0;
> }
>
>
> static int tilcdc_pm_suspend(struct device *dev)
> {
> struct drm_device *ddev = dev_get_drvdata(dev);
> struct tilcdc_drm_private *priv = ddev->dev_private;
>
> priv->saved_state = drm_atomic_helper_suspend(ddev);
>
> /* Select sleep pin state */
> pinctrl_pm_select_sleep_state(dev);
>
> return 0;
> }
>
> static int tilcdc_pm_resume(struct device *dev)
> {
> struct drm_device *ddev = dev_get_drvdata(dev);
> struct tilcdc_drm_private *priv = ddev->dev_private;
> int ret = 0;
>
> /* Select default pin state */
> pinctrl_pm_select_default_state(dev);
>
> if (priv->saved_state)
> ret = drm_atomic_helper_resume(ddev, priv->saved_state);
>
> return ret;
> }
>
>
> int tinydrm_suspend(struct tinydrm_device *tdev)
> {
> struct drm_atomic_state *state;
>
> if (tdev->suspend_state) {
> DRM_ERROR("Failed to suspend: state already set\n");
> return -EINVAL;
> }
>
> drm_fbdev_cma_set_suspend_unlocked(tdev->fbdev_cma, 1);
> state = drm_atomic_helper_suspend(tdev->drm);
> if (IS_ERR(state)) {
> drm_fbdev_cma_set_suspend_unlocked(tdev->fbdev_cma, 0);
> return PTR_ERR(state);
> }
>
> tdev->suspend_state = state;
>
> return 0;
> }
>
> int tinydrm_resume(struct tinydrm_device *tdev)
> {
> struct drm_atomic_state *state = tdev->suspend_state;
> int ret;
>
> if (!state) {
> DRM_ERROR("Failed to resume: state is not set\n");
> return -EINVAL;
> }
>
> tdev->suspend_state = NULL;
>
> ret = drm_atomic_helper_resume(tdev->drm, state);
> if (ret) {
> DRM_ERROR("Error resuming state: %d\n", ret);
> return ret;
> }
>
> drm_fbdev_cma_set_suspend_unlocked(tdev->fbdev_cma, 0);
>
> return 0;
> }
>
>
> Noralf.
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: RFC: Add drm_dev_suspend/resume() ?
2017-10-30 9:34 ` Daniel Vetter
@ 2017-10-31 16:37 ` Noralf Trønnes
2017-11-01 8:47 ` Daniel Vetter
0 siblings, 1 reply; 6+ messages in thread
From: Noralf Trønnes @ 2017-10-31 16:37 UTC (permalink / raw)
To: Daniel Vetter; +Cc: dri-devel
Den 30.10.2017 10.34, skrev Daniel Vetter:
> Hi Noralf,
>
> On Sun, Oct 22, 2017 at 06:52:41PM +0200, Noralf Trønnes wrote:
>> Hi,
>>
>> I've spent some time in the fbdev emulation code and discovered a
>> recurring pattern around suspend/resume.
>> Should we add some more helpers :-)
> You're maybe a bit too good at spotting these for your own good :-)
>
> But yeah, a "suspend for dummies" is one of the things which would be nice
> I think ... Especially since we now have the atomic suspend/resume
> helpers.
>
>> struct drm_device {
>> /**
>> * @suspend_state:
>> *
>> * Atomic state when suspended.
>> * Set by drm_dev_suspend(), cleared by drm_dev_resume().
>> */
>> struct drm_atomic_state *suspend_state;
>> };
> Imo fits better when we put it into drm_mode_config.
>
>> int drm_dev_suspend(struct drm_device *dev)
>> {
>> struct drm_atomic_state *state;
>>
>> if (!dev)
>> return 0;
>>
>> drm_kms_helper_poll_disable(dev);
>> drm_fb_helper_set_suspend_unlocked(dev->fb_helper, 1);
>> state = drm_atomic_helper_suspend(dev);
>> if (IS_ERR(state)) {
>> drm_fb_helper_set_suspend_unlocked(dev->fb_helper, 0);
>> drm_kms_helper_poll_enable(dev);
>> return PTR_ERR(state);
>> }
>>
>> dev->suspend_state = state;
>>
>> return 0;
>> }
> This is all about suspending the modeset side ... I'd give it a
> drm_mode_config prefix instead of the drm_dev_ (that's maybe a bit too
> generic), but then maybe type a general suspend/resume kernel-doc text in
> the drm-internals.rst (maybe pulled in from drm_dev.c) which references
> these 2 functions as the recommended way to suspend/resume the modeset
> side of a driver. These won't suspend/resume a render part (if present),
> so drm_dev_ seems a bit too much.
I just realised that this is pulling helpers (atomic, crtc, fbdev) into
the core which IIRC is something that you didn't want?
diff --git a/drivers/gpu/drm/drm_mode_config.c
b/drivers/gpu/drm/drm_mode_config.c
index 74f6ff5df656..72f8fe1e83cb 100644
--- a/drivers/gpu/drm/drm_mode_config.c
+++ b/drivers/gpu/drm/drm_mode_config.c
@@ -20,7 +20,10 @@
* OF THIS SOFTWARE.
*/
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_crtc_helper.h>
#include <drm/drm_encoder.h>
+#include <drm/drm_fb_helper.h>
#include <drm/drm_mode_config.h>
#include <drm/drmP.h>
@@ -473,3 +476,49 @@ void drm_mode_config_cleanup(struct drm_device *dev)
drm_modeset_lock_fini(&dev->mode_config.connection_mutex);
}
EXPORT_SYMBOL(drm_mode_config_cleanup);
+
+/**
+ * drm_mode_config_suspend
+ * @dev: DRM device
+ *
+ */
+int drm_mode_config_suspend(struct drm_device *dev)
+{
+ struct drm_atomic_state *state;
+
+ if (!dev)
+ return 0;
+
+ drm_kms_helper_poll_disable(dev);
+ drm_fb_helper_set_suspend_unlocked(dev->fb_helper, 1);
+ state = drm_atomic_helper_suspend(dev);
+ if (IS_ERR(state)) {
+ drm_fb_helper_set_suspend_unlocked(dev->fb_helper, 0);
+ drm_kms_helper_poll_enable(dev);
+ return PTR_ERR(state);
+ }
+
+ dev->mode_config.suspend_state = state;
+
+ return 0;
+}
+EXPORT_SYMBOL(drm_mode_config_suspend);
+
+int drm_mode_config_resume(struct drm_device *dev)
+{
+ int ret;
+
+ if (!dev || WARN_ON(!dev->mode_config.suspend_state))
+ return 0;
+
+ ret = drm_atomic_helper_resume(dev, dev->mode_config.suspend_state);
+ if (ret)
+ DRM_ERROR("Failed to resume (%d)\n", ret);
+ dev->mode_config.suspend_state = NULL;
+
+ drm_fb_helper_set_suspend_unlocked(dev->fb_helper, 0);
+ drm_kms_helper_poll_enable(dev);
+
+ return 0;
+}
+EXPORT_SYMBOL(drm_mode_config_resume);
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index 1b37368416c8..86ef327a996d 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -766,11 +766,21 @@ struct drm_mode_config {
/* cursor size */
uint32_t cursor_width, cursor_height;
+ /**
+ * @suspend_state:
+ *
+ * Atomic state when suspended. Set by drm_mode_config_suspend(),
+ * cleared by drm_mode_config_resume().
+ */
+ struct drm_atomic_state *suspend_state;
+
const struct drm_mode_config_helper_funcs *helper_private;
};
void drm_mode_config_init(struct drm_device *dev);
void drm_mode_config_reset(struct drm_device *dev);
void drm_mode_config_cleanup(struct drm_device *dev);
+int drm_mode_config_suspend(struct drm_device *dev);
+int drm_mode_config_resume(struct drm_device *dev);
#endif
Noralf.
>> int drm_dev_resume(struct drm_device *dev)
>> {
>> int ret;
>>
>> if (!dev || WARN_ON(!dev->suspend_state))
>> return 0;
>>
>> ret = drm_atomic_helper_resume(dev, dev->suspend_state);
>> if (ret)
>> DRM_ERROR("Failed to resume (%d)\n", ret);
>> dev->suspend_state = NULL;
>>
>> drm_fb_helper_set_suspend_unlocked(dev->fb_helper, 0);
>> drm_kms_helper_poll_enable(dev);
>>
>> return 0;
>> }
>>
>> The drm_kms_helper_poll_enable() docs states that it's not allowed to
>> call this function if polling support has been set up, but the code
>> allows it. drm_kms_helper_poll_disable() docs states that it's ok to
>> call it even if polling is not enabled.
> Yeah need to fix that, but need to keep a note that if you want polling,
> then you need to follow that order or it goes wrong. Perhaps explain that
> this is meant to avoid special-cases in suspend/resume code?
>
>> Here are the suspend/resume code from the drivers that use
>> drm_atomic_helper_suspend/resume():
> Yeah, looks like a really good idea.
>
> Thanks, Daniel
>> static int __maybe_unused hdlcd_pm_suspend(struct device *dev)
>> {
>> struct drm_device *drm = dev_get_drvdata(dev);
>> struct hdlcd_drm_private *hdlcd = drm ? drm->dev_private : NULL;
>>
>> if (!hdlcd)
>> return 0;
>>
>> drm_kms_helper_poll_disable(drm);
>>
>> hdlcd->state = drm_atomic_helper_suspend(drm);
>> if (IS_ERR(hdlcd->state)) {
>> drm_kms_helper_poll_enable(drm);
>> return PTR_ERR(hdlcd->state);
>> }
>>
>> return 0;
>> }
>>
>> static int __maybe_unused hdlcd_pm_resume(struct device *dev)
>> {
>> struct drm_device *drm = dev_get_drvdata(dev);
>> struct hdlcd_drm_private *hdlcd = drm ? drm->dev_private : NULL;
>>
>> if (!hdlcd)
>> return 0;
>>
>> drm_atomic_helper_resume(drm, hdlcd->state);
>> drm_kms_helper_poll_enable(drm);
>> pm_runtime_set_active(dev);
>>
>> return 0;
>> }
>>
>>
>> static int __maybe_unused malidp_pm_suspend(struct device *dev)
>> {
>> struct drm_device *drm = dev_get_drvdata(dev);
>> struct malidp_drm *malidp = drm->dev_private;
>>
>> drm_kms_helper_poll_disable(drm);
>> console_lock();
>> drm_fbdev_cma_set_suspend(malidp->fbdev, 1);
>> console_unlock();
>> malidp->pm_state = drm_atomic_helper_suspend(drm);
>> if (IS_ERR(malidp->pm_state)) {
>> console_lock();
>> drm_fbdev_cma_set_suspend(malidp->fbdev, 0);
>> console_unlock();
>> drm_kms_helper_poll_enable(drm);
>> return PTR_ERR(malidp->pm_state);
>> }
>>
>> return 0;
>> }
>>
>> static int __maybe_unused malidp_pm_resume(struct device *dev)
>> {
>> struct drm_device *drm = dev_get_drvdata(dev);
>> struct malidp_drm *malidp = drm->dev_private;
>>
>> drm_atomic_helper_resume(drm, malidp->pm_state);
>> console_lock();
>> drm_fbdev_cma_set_suspend(malidp->fbdev, 0);
>> console_unlock();
>> drm_kms_helper_poll_enable(drm);
>>
>> return 0;
>> }
>>
>>
>> static int atmel_hlcdc_dc_drm_suspend(struct device *dev)
>> {
>> struct drm_device *drm_dev = dev_get_drvdata(dev);
>> struct atmel_hlcdc_dc *dc = drm_dev->dev_private;
>> struct regmap *regmap = dc->hlcdc->regmap;
>> struct drm_atomic_state *state;
>>
>> state = drm_atomic_helper_suspend(drm_dev);
>> if (IS_ERR(state))
>> return PTR_ERR(state);
>>
>> dc->suspend.state = state;
>>
>> regmap_read(regmap, ATMEL_HLCDC_IMR, &dc->suspend.imr);
>> regmap_write(regmap, ATMEL_HLCDC_IDR, dc->suspend.imr);
>> clk_disable_unprepare(dc->hlcdc->periph_clk);
>>
>> return 0;
>> }
>>
>> static int atmel_hlcdc_dc_drm_resume(struct device *dev)
>> {
>> struct drm_device *drm_dev = dev_get_drvdata(dev);
>> struct atmel_hlcdc_dc *dc = drm_dev->dev_private;
>>
>> clk_prepare_enable(dc->hlcdc->periph_clk);
>> regmap_write(dc->hlcdc->regmap, ATMEL_HLCDC_IER, dc->suspend.imr);
>>
>> return drm_atomic_helper_resume(drm_dev, dc->suspend.state);
>> }
>>
>>
>> static int exynos_drm_suspend(struct device *dev)
>> {
>> struct drm_device *drm_dev = dev_get_drvdata(dev);
>> struct exynos_drm_private *private = drm_dev->dev_private;
>>
>> if (pm_runtime_suspended(dev) || !drm_dev)
>> return 0;
>>
>> drm_kms_helper_poll_disable(drm_dev);
>> exynos_drm_fbdev_suspend(drm_dev);
>> private->suspend_state = drm_atomic_helper_suspend(drm_dev);
>> if (IS_ERR(private->suspend_state)) {
>> exynos_drm_fbdev_resume(drm_dev);
>> drm_kms_helper_poll_enable(drm_dev);
>> return PTR_ERR(private->suspend_state);
>> }
>>
>> return 0;
>> }
>>
>> static int exynos_drm_resume(struct device *dev)
>> {
>> struct drm_device *drm_dev = dev_get_drvdata(dev);
>> struct exynos_drm_private *private = drm_dev->dev_private;
>>
>> if (pm_runtime_suspended(dev) || !drm_dev)
>> return 0;
>>
>> drm_atomic_helper_resume(drm_dev, private->suspend_state);
>> exynos_drm_fbdev_resume(drm_dev);
>> drm_kms_helper_poll_enable(drm_dev);
>>
>> return 0;
>> }
>>
>>
>> static int fsl_dcu_drm_pm_suspend(struct device *dev)
>> {
>> struct fsl_dcu_drm_device *fsl_dev = dev_get_drvdata(dev);
>>
>> if (!fsl_dev)
>> return 0;
>>
>> disable_irq(fsl_dev->irq);
>> drm_kms_helper_poll_disable(fsl_dev->drm);
>>
>> console_lock();
>> drm_fbdev_cma_set_suspend(fsl_dev->fbdev, 1);
>> console_unlock();
>>
>> fsl_dev->state = drm_atomic_helper_suspend(fsl_dev->drm);
>> if (IS_ERR(fsl_dev->state)) {
>> console_lock();
>> drm_fbdev_cma_set_suspend(fsl_dev->fbdev, 0);
>> console_unlock();
>>
>> drm_kms_helper_poll_enable(fsl_dev->drm);
>> enable_irq(fsl_dev->irq);
>> return PTR_ERR(fsl_dev->state);
>> }
>>
>> clk_disable_unprepare(fsl_dev->pix_clk);
>> clk_disable_unprepare(fsl_dev->clk);
>>
>> return 0;
>> }
>>
>> static int fsl_dcu_drm_pm_resume(struct device *dev)
>> {
>> struct fsl_dcu_drm_device *fsl_dev = dev_get_drvdata(dev);
>> int ret;
>>
>> if (!fsl_dev)
>> return 0;
>>
>> ret = clk_prepare_enable(fsl_dev->clk);
>> if (ret < 0) {
>> dev_err(dev, "failed to enable dcu clk\n");
>> return ret;
>> }
>>
>> if (fsl_dev->tcon)
>> fsl_tcon_bypass_enable(fsl_dev->tcon);
>> fsl_dcu_drm_init_planes(fsl_dev->drm);
>> drm_atomic_helper_resume(fsl_dev->drm, fsl_dev->state);
>>
>> console_lock();
>> drm_fbdev_cma_set_suspend(fsl_dev->fbdev, 0);
>> console_unlock();
>>
>> drm_kms_helper_poll_enable(fsl_dev->drm);
>> enable_irq(fsl_dev->irq);
>>
>> return 0;
>> }
>>
>>
>> static int __maybe_unused hibmc_pm_suspend(struct device *dev)
>> {
>> struct pci_dev *pdev = to_pci_dev(dev);
>> struct drm_device *drm_dev = pci_get_drvdata(pdev);
>> struct hibmc_drm_private *priv = drm_dev->dev_private;
>>
>> drm_kms_helper_poll_disable(drm_dev);
>> priv->suspend_state = drm_atomic_helper_suspend(drm_dev);
>> if (IS_ERR(priv->suspend_state)) {
>> DRM_ERROR("drm_atomic_helper_suspend failed: %ld\n",
>> PTR_ERR(priv->suspend_state));
>> drm_kms_helper_poll_enable(drm_dev);
>> return PTR_ERR(priv->suspend_state);
>> }
>>
>> return 0;
>> }
>>
>> static int __maybe_unused hibmc_pm_resume(struct device *dev)
>> {
>> struct pci_dev *pdev = to_pci_dev(dev);
>> struct drm_device *drm_dev = pci_get_drvdata(pdev);
>> struct hibmc_drm_private *priv = drm_dev->dev_private;
>>
>> drm_atomic_helper_resume(drm_dev, priv->suspend_state);
>> drm_kms_helper_poll_enable(drm_dev);
>>
>> return 0;
>> }
>>
>>
>> i915 is excluded since it's rather complicated.
>>
>>
>> static int imx_drm_suspend(struct device *dev)
>> {
>> struct drm_device *drm_dev = dev_get_drvdata(dev);
>> struct imx_drm_device *imxdrm;
>>
>> /* The drm_dev is NULL before .load hook is called */
>> if (drm_dev == NULL)
>> return 0;
>>
>> drm_kms_helper_poll_disable(drm_dev);
>>
>> imxdrm = drm_dev->dev_private;
>> imxdrm->state = drm_atomic_helper_suspend(drm_dev);
>> if (IS_ERR(imxdrm->state)) {
>> drm_kms_helper_poll_enable(drm_dev);
>> return PTR_ERR(imxdrm->state);
>> }
>>
>> return 0;
>> }
>>
>> static int imx_drm_resume(struct device *dev)
>> {
>> struct drm_device *drm_dev = dev_get_drvdata(dev);
>> struct imx_drm_device *imx_drm;
>>
>> if (drm_dev == NULL)
>> return 0;
>>
>> imx_drm = drm_dev->dev_private;
>> drm_atomic_helper_resume(drm_dev, imx_drm->state);
>> drm_kms_helper_poll_enable(drm_dev);
>>
>> return 0;
>> }
>>
>>
>> static int mtk_drm_sys_suspend(struct device *dev)
>> {
>> struct mtk_drm_private *private = dev_get_drvdata(dev);
>> struct drm_device *drm = private->drm;
>>
>> drm_kms_helper_poll_disable(drm);
>>
>> private->suspend_state = drm_atomic_helper_suspend(drm);
>> if (IS_ERR(private->suspend_state)) {
>> drm_kms_helper_poll_enable(drm);
>> return PTR_ERR(private->suspend_state);
>> }
>>
>> DRM_DEBUG_DRIVER("mtk_drm_sys_suspend\n");
>> return 0;
>> }
>>
>> static int mtk_drm_sys_resume(struct device *dev)
>> {
>> struct mtk_drm_private *private = dev_get_drvdata(dev);
>> struct drm_device *drm = private->drm;
>>
>> drm_atomic_helper_resume(drm, private->suspend_state);
>> drm_kms_helper_poll_enable(drm);
>>
>> DRM_DEBUG_DRIVER("mtk_drm_sys_resume\n");
>> return 0;
>> }
>>
>>
>> nouveau is also rather complicated.
>>
>>
>> static int rockchip_drm_sys_suspend(struct device *dev)
>> {
>> struct drm_device *drm = dev_get_drvdata(dev);
>> struct rockchip_drm_private *priv;
>>
>> if (!drm)
>> return 0;
>>
>> drm_kms_helper_poll_disable(drm);
>> rockchip_drm_fb_suspend(drm);
>>
>> priv = drm->dev_private;
>> priv->state = drm_atomic_helper_suspend(drm);
>> if (IS_ERR(priv->state)) {
>> rockchip_drm_fb_resume(drm);
>> drm_kms_helper_poll_enable(drm);
>> return PTR_ERR(priv->state);
>> }
>>
>> return 0;
>> }
>>
>> static int rockchip_drm_sys_resume(struct device *dev)
>> {
>> struct drm_device *drm = dev_get_drvdata(dev);
>> struct rockchip_drm_private *priv;
>>
>> if (!drm)
>> return 0;
>>
>> priv = drm->dev_private;
>> drm_atomic_helper_resume(drm, priv->state);
>> rockchip_drm_fb_resume(drm);
>> drm_kms_helper_poll_enable(drm);
>>
>> return 0;
>> }
>>
>>
>> static int host1x_drm_suspend(struct device *dev)
>> {
>> struct drm_device *drm = dev_get_drvdata(dev);
>> struct tegra_drm *tegra = drm->dev_private;
>>
>> drm_kms_helper_poll_disable(drm);
>> tegra_drm_fb_suspend(drm);
>>
>> tegra->state = drm_atomic_helper_suspend(drm);
>> if (IS_ERR(tegra->state)) {
>> tegra_drm_fb_resume(drm);
>> drm_kms_helper_poll_enable(drm);
>> return PTR_ERR(tegra->state);
>> }
>>
>> return 0;
>> }
>>
>> static int host1x_drm_resume(struct device *dev)
>> {
>> struct drm_device *drm = dev_get_drvdata(dev);
>> struct tegra_drm *tegra = drm->dev_private;
>>
>> drm_atomic_helper_resume(drm, tegra->state);
>> tegra_drm_fb_resume(drm);
>> drm_kms_helper_poll_enable(drm);
>>
>> return 0;
>> }
>>
>>
>> static int tilcdc_pm_suspend(struct device *dev)
>> {
>> struct drm_device *ddev = dev_get_drvdata(dev);
>> struct tilcdc_drm_private *priv = ddev->dev_private;
>>
>> priv->saved_state = drm_atomic_helper_suspend(ddev);
>>
>> /* Select sleep pin state */
>> pinctrl_pm_select_sleep_state(dev);
>>
>> return 0;
>> }
>>
>> static int tilcdc_pm_resume(struct device *dev)
>> {
>> struct drm_device *ddev = dev_get_drvdata(dev);
>> struct tilcdc_drm_private *priv = ddev->dev_private;
>> int ret = 0;
>>
>> /* Select default pin state */
>> pinctrl_pm_select_default_state(dev);
>>
>> if (priv->saved_state)
>> ret = drm_atomic_helper_resume(ddev, priv->saved_state);
>>
>> return ret;
>> }
>>
>>
>> int tinydrm_suspend(struct tinydrm_device *tdev)
>> {
>> struct drm_atomic_state *state;
>>
>> if (tdev->suspend_state) {
>> DRM_ERROR("Failed to suspend: state already set\n");
>> return -EINVAL;
>> }
>>
>> drm_fbdev_cma_set_suspend_unlocked(tdev->fbdev_cma, 1);
>> state = drm_atomic_helper_suspend(tdev->drm);
>> if (IS_ERR(state)) {
>> drm_fbdev_cma_set_suspend_unlocked(tdev->fbdev_cma, 0);
>> return PTR_ERR(state);
>> }
>>
>> tdev->suspend_state = state;
>>
>> return 0;
>> }
>>
>> int tinydrm_resume(struct tinydrm_device *tdev)
>> {
>> struct drm_atomic_state *state = tdev->suspend_state;
>> int ret;
>>
>> if (!state) {
>> DRM_ERROR("Failed to resume: state is not set\n");
>> return -EINVAL;
>> }
>>
>> tdev->suspend_state = NULL;
>>
>> ret = drm_atomic_helper_resume(tdev->drm, state);
>> if (ret) {
>> DRM_ERROR("Error resuming state: %d\n", ret);
>> return ret;
>> }
>>
>> drm_fbdev_cma_set_suspend_unlocked(tdev->fbdev_cma, 0);
>>
>> return 0;
>> }
>>
>>
>> Noralf.
>>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: RFC: Add drm_dev_suspend/resume() ?
2017-10-31 16:37 ` Noralf Trønnes
@ 2017-11-01 8:47 ` Daniel Vetter
2017-11-01 12:59 ` Noralf Trønnes
0 siblings, 1 reply; 6+ messages in thread
From: Daniel Vetter @ 2017-11-01 8:47 UTC (permalink / raw)
To: Noralf Trønnes; +Cc: dri-devel
On Tue, Oct 31, 2017 at 05:37:23PM +0100, Noralf Trønnes wrote:
>
> Den 30.10.2017 10.34, skrev Daniel Vetter:
> > Hi Noralf,
> >
> > On Sun, Oct 22, 2017 at 06:52:41PM +0200, Noralf Trønnes wrote:
> > > Hi,
> > >
> > > I've spent some time in the fbdev emulation code and discovered a
> > > recurring pattern around suspend/resume.
> > > Should we add some more helpers :-)
> > You're maybe a bit too good at spotting these for your own good :-)
> >
> > But yeah, a "suspend for dummies" is one of the things which would be nice
> > I think ... Especially since we now have the atomic suspend/resume
> > helpers.
> >
> > > struct drm_device {
> > > /**
> > > * @suspend_state:
> > > *
> > > * Atomic state when suspended.
> > > * Set by drm_dev_suspend(), cleared by drm_dev_resume().
> > > */
> > > struct drm_atomic_state *suspend_state;
> > > };
> > Imo fits better when we put it into drm_mode_config.
> >
> > > int drm_dev_suspend(struct drm_device *dev)
> > > {
> > > struct drm_atomic_state *state;
> > >
> > > if (!dev)
> > > return 0;
> > >
> > > drm_kms_helper_poll_disable(dev);
> > > drm_fb_helper_set_suspend_unlocked(dev->fb_helper, 1);
> > > state = drm_atomic_helper_suspend(dev);
> > > if (IS_ERR(state)) {
> > > drm_fb_helper_set_suspend_unlocked(dev->fb_helper, 0);
> > > drm_kms_helper_poll_enable(dev);
> > > return PTR_ERR(state);
> > > }
> > >
> > > dev->suspend_state = state;
> > >
> > > return 0;
> > > }
> > This is all about suspending the modeset side ... I'd give it a
> > drm_mode_config prefix instead of the drm_dev_ (that's maybe a bit too
> > generic), but then maybe type a general suspend/resume kernel-doc text in
> > the drm-internals.rst (maybe pulled in from drm_dev.c) which references
> > these 2 functions as the recommended way to suspend/resume the modeset
> > side of a driver. These won't suspend/resume a render part (if present),
> > so drm_dev_ seems a bit too much.
>
> I just realised that this is pulling helpers (atomic, crtc, fbdev) into
> the core which IIRC is something that you didn't want?
Ugh right. I think starting a new drm_mode_config_helper.c for top-level
helper stuff would be a reasonable solution. Or some other name if you
have a better one.
-Daniel
>
>
> diff --git a/drivers/gpu/drm/drm_mode_config.c
> b/drivers/gpu/drm/drm_mode_config.c
> index 74f6ff5df656..72f8fe1e83cb 100644
> --- a/drivers/gpu/drm/drm_mode_config.c
> +++ b/drivers/gpu/drm/drm_mode_config.c
> @@ -20,7 +20,10 @@
> * OF THIS SOFTWARE.
> */
>
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_crtc_helper.h>
> #include <drm/drm_encoder.h>
> +#include <drm/drm_fb_helper.h>
> #include <drm/drm_mode_config.h>
> #include <drm/drmP.h>
>
> @@ -473,3 +476,49 @@ void drm_mode_config_cleanup(struct drm_device *dev)
> drm_modeset_lock_fini(&dev->mode_config.connection_mutex);
> }
> EXPORT_SYMBOL(drm_mode_config_cleanup);
> +
> +/**
> + * drm_mode_config_suspend
> + * @dev: DRM device
> + *
> + */
> +int drm_mode_config_suspend(struct drm_device *dev)
> +{
> + struct drm_atomic_state *state;
> +
> + if (!dev)
> + return 0;
> +
> + drm_kms_helper_poll_disable(dev);
> + drm_fb_helper_set_suspend_unlocked(dev->fb_helper, 1);
> + state = drm_atomic_helper_suspend(dev);
> + if (IS_ERR(state)) {
> + drm_fb_helper_set_suspend_unlocked(dev->fb_helper, 0);
> + drm_kms_helper_poll_enable(dev);
> + return PTR_ERR(state);
> + }
> +
> + dev->mode_config.suspend_state = state;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(drm_mode_config_suspend);
> +
> +int drm_mode_config_resume(struct drm_device *dev)
> +{
> + int ret;
> +
> + if (!dev || WARN_ON(!dev->mode_config.suspend_state))
> + return 0;
> +
> + ret = drm_atomic_helper_resume(dev, dev->mode_config.suspend_state);
> + if (ret)
> + DRM_ERROR("Failed to resume (%d)\n", ret);
> + dev->mode_config.suspend_state = NULL;
> +
> + drm_fb_helper_set_suspend_unlocked(dev->fb_helper, 0);
> + drm_kms_helper_poll_enable(dev);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(drm_mode_config_resume);
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index 1b37368416c8..86ef327a996d 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -766,11 +766,21 @@ struct drm_mode_config {
> /* cursor size */
> uint32_t cursor_width, cursor_height;
>
> + /**
> + * @suspend_state:
> + *
> + * Atomic state when suspended. Set by drm_mode_config_suspend(),
> + * cleared by drm_mode_config_resume().
> + */
> + struct drm_atomic_state *suspend_state;
> +
> const struct drm_mode_config_helper_funcs *helper_private;
> };
>
> void drm_mode_config_init(struct drm_device *dev);
> void drm_mode_config_reset(struct drm_device *dev);
> void drm_mode_config_cleanup(struct drm_device *dev);
> +int drm_mode_config_suspend(struct drm_device *dev);
> +int drm_mode_config_resume(struct drm_device *dev);
>
> #endif
>
>
> Noralf.
>
> > > int drm_dev_resume(struct drm_device *dev)
> > > {
> > > int ret;
> > >
> > > if (!dev || WARN_ON(!dev->suspend_state))
> > > return 0;
> > >
> > > ret = drm_atomic_helper_resume(dev, dev->suspend_state);
> > > if (ret)
> > > DRM_ERROR("Failed to resume (%d)\n", ret);
> > > dev->suspend_state = NULL;
> > >
> > > drm_fb_helper_set_suspend_unlocked(dev->fb_helper, 0);
> > > drm_kms_helper_poll_enable(dev);
> > >
> > > return 0;
> > > }
> > >
> > > The drm_kms_helper_poll_enable() docs states that it's not allowed to
> > > call this function if polling support has been set up, but the code
> > > allows it. drm_kms_helper_poll_disable() docs states that it's ok to
> > > call it even if polling is not enabled.
> > Yeah need to fix that, but need to keep a note that if you want polling,
> > then you need to follow that order or it goes wrong. Perhaps explain that
> > this is meant to avoid special-cases in suspend/resume code?
> >
> > > Here are the suspend/resume code from the drivers that use
> > > drm_atomic_helper_suspend/resume():
> > Yeah, looks like a really good idea.
> >
> > Thanks, Daniel
> > > static int __maybe_unused hdlcd_pm_suspend(struct device *dev)
> > > {
> > > struct drm_device *drm = dev_get_drvdata(dev);
> > > struct hdlcd_drm_private *hdlcd = drm ? drm->dev_private : NULL;
> > >
> > > if (!hdlcd)
> > > return 0;
> > >
> > > drm_kms_helper_poll_disable(drm);
> > >
> > > hdlcd->state = drm_atomic_helper_suspend(drm);
> > > if (IS_ERR(hdlcd->state)) {
> > > drm_kms_helper_poll_enable(drm);
> > > return PTR_ERR(hdlcd->state);
> > > }
> > >
> > > return 0;
> > > }
> > >
> > > static int __maybe_unused hdlcd_pm_resume(struct device *dev)
> > > {
> > > struct drm_device *drm = dev_get_drvdata(dev);
> > > struct hdlcd_drm_private *hdlcd = drm ? drm->dev_private : NULL;
> > >
> > > if (!hdlcd)
> > > return 0;
> > >
> > > drm_atomic_helper_resume(drm, hdlcd->state);
> > > drm_kms_helper_poll_enable(drm);
> > > pm_runtime_set_active(dev);
> > >
> > > return 0;
> > > }
> > >
> > >
> > > static int __maybe_unused malidp_pm_suspend(struct device *dev)
> > > {
> > > struct drm_device *drm = dev_get_drvdata(dev);
> > > struct malidp_drm *malidp = drm->dev_private;
> > >
> > > drm_kms_helper_poll_disable(drm);
> > > console_lock();
> > > drm_fbdev_cma_set_suspend(malidp->fbdev, 1);
> > > console_unlock();
> > > malidp->pm_state = drm_atomic_helper_suspend(drm);
> > > if (IS_ERR(malidp->pm_state)) {
> > > console_lock();
> > > drm_fbdev_cma_set_suspend(malidp->fbdev, 0);
> > > console_unlock();
> > > drm_kms_helper_poll_enable(drm);
> > > return PTR_ERR(malidp->pm_state);
> > > }
> > >
> > > return 0;
> > > }
> > >
> > > static int __maybe_unused malidp_pm_resume(struct device *dev)
> > > {
> > > struct drm_device *drm = dev_get_drvdata(dev);
> > > struct malidp_drm *malidp = drm->dev_private;
> > >
> > > drm_atomic_helper_resume(drm, malidp->pm_state);
> > > console_lock();
> > > drm_fbdev_cma_set_suspend(malidp->fbdev, 0);
> > > console_unlock();
> > > drm_kms_helper_poll_enable(drm);
> > >
> > > return 0;
> > > }
> > >
> > >
> > > static int atmel_hlcdc_dc_drm_suspend(struct device *dev)
> > > {
> > > struct drm_device *drm_dev = dev_get_drvdata(dev);
> > > struct atmel_hlcdc_dc *dc = drm_dev->dev_private;
> > > struct regmap *regmap = dc->hlcdc->regmap;
> > > struct drm_atomic_state *state;
> > >
> > > state = drm_atomic_helper_suspend(drm_dev);
> > > if (IS_ERR(state))
> > > return PTR_ERR(state);
> > >
> > > dc->suspend.state = state;
> > >
> > > regmap_read(regmap, ATMEL_HLCDC_IMR, &dc->suspend.imr);
> > > regmap_write(regmap, ATMEL_HLCDC_IDR, dc->suspend.imr);
> > > clk_disable_unprepare(dc->hlcdc->periph_clk);
> > >
> > > return 0;
> > > }
> > >
> > > static int atmel_hlcdc_dc_drm_resume(struct device *dev)
> > > {
> > > struct drm_device *drm_dev = dev_get_drvdata(dev);
> > > struct atmel_hlcdc_dc *dc = drm_dev->dev_private;
> > >
> > > clk_prepare_enable(dc->hlcdc->periph_clk);
> > > regmap_write(dc->hlcdc->regmap, ATMEL_HLCDC_IER, dc->suspend.imr);
> > >
> > > return drm_atomic_helper_resume(drm_dev, dc->suspend.state);
> > > }
> > >
> > >
> > > static int exynos_drm_suspend(struct device *dev)
> > > {
> > > struct drm_device *drm_dev = dev_get_drvdata(dev);
> > > struct exynos_drm_private *private = drm_dev->dev_private;
> > >
> > > if (pm_runtime_suspended(dev) || !drm_dev)
> > > return 0;
> > >
> > > drm_kms_helper_poll_disable(drm_dev);
> > > exynos_drm_fbdev_suspend(drm_dev);
> > > private->suspend_state = drm_atomic_helper_suspend(drm_dev);
> > > if (IS_ERR(private->suspend_state)) {
> > > exynos_drm_fbdev_resume(drm_dev);
> > > drm_kms_helper_poll_enable(drm_dev);
> > > return PTR_ERR(private->suspend_state);
> > > }
> > >
> > > return 0;
> > > }
> > >
> > > static int exynos_drm_resume(struct device *dev)
> > > {
> > > struct drm_device *drm_dev = dev_get_drvdata(dev);
> > > struct exynos_drm_private *private = drm_dev->dev_private;
> > >
> > > if (pm_runtime_suspended(dev) || !drm_dev)
> > > return 0;
> > >
> > > drm_atomic_helper_resume(drm_dev, private->suspend_state);
> > > exynos_drm_fbdev_resume(drm_dev);
> > > drm_kms_helper_poll_enable(drm_dev);
> > >
> > > return 0;
> > > }
> > >
> > >
> > > static int fsl_dcu_drm_pm_suspend(struct device *dev)
> > > {
> > > struct fsl_dcu_drm_device *fsl_dev = dev_get_drvdata(dev);
> > >
> > > if (!fsl_dev)
> > > return 0;
> > >
> > > disable_irq(fsl_dev->irq);
> > > drm_kms_helper_poll_disable(fsl_dev->drm);
> > >
> > > console_lock();
> > > drm_fbdev_cma_set_suspend(fsl_dev->fbdev, 1);
> > > console_unlock();
> > >
> > > fsl_dev->state = drm_atomic_helper_suspend(fsl_dev->drm);
> > > if (IS_ERR(fsl_dev->state)) {
> > > console_lock();
> > > drm_fbdev_cma_set_suspend(fsl_dev->fbdev, 0);
> > > console_unlock();
> > >
> > > drm_kms_helper_poll_enable(fsl_dev->drm);
> > > enable_irq(fsl_dev->irq);
> > > return PTR_ERR(fsl_dev->state);
> > > }
> > >
> > > clk_disable_unprepare(fsl_dev->pix_clk);
> > > clk_disable_unprepare(fsl_dev->clk);
> > >
> > > return 0;
> > > }
> > >
> > > static int fsl_dcu_drm_pm_resume(struct device *dev)
> > > {
> > > struct fsl_dcu_drm_device *fsl_dev = dev_get_drvdata(dev);
> > > int ret;
> > >
> > > if (!fsl_dev)
> > > return 0;
> > >
> > > ret = clk_prepare_enable(fsl_dev->clk);
> > > if (ret < 0) {
> > > dev_err(dev, "failed to enable dcu clk\n");
> > > return ret;
> > > }
> > >
> > > if (fsl_dev->tcon)
> > > fsl_tcon_bypass_enable(fsl_dev->tcon);
> > > fsl_dcu_drm_init_planes(fsl_dev->drm);
> > > drm_atomic_helper_resume(fsl_dev->drm, fsl_dev->state);
> > >
> > > console_lock();
> > > drm_fbdev_cma_set_suspend(fsl_dev->fbdev, 0);
> > > console_unlock();
> > >
> > > drm_kms_helper_poll_enable(fsl_dev->drm);
> > > enable_irq(fsl_dev->irq);
> > >
> > > return 0;
> > > }
> > >
> > >
> > > static int __maybe_unused hibmc_pm_suspend(struct device *dev)
> > > {
> > > struct pci_dev *pdev = to_pci_dev(dev);
> > > struct drm_device *drm_dev = pci_get_drvdata(pdev);
> > > struct hibmc_drm_private *priv = drm_dev->dev_private;
> > >
> > > drm_kms_helper_poll_disable(drm_dev);
> > > priv->suspend_state = drm_atomic_helper_suspend(drm_dev);
> > > if (IS_ERR(priv->suspend_state)) {
> > > DRM_ERROR("drm_atomic_helper_suspend failed: %ld\n",
> > > PTR_ERR(priv->suspend_state));
> > > drm_kms_helper_poll_enable(drm_dev);
> > > return PTR_ERR(priv->suspend_state);
> > > }
> > >
> > > return 0;
> > > }
> > >
> > > static int __maybe_unused hibmc_pm_resume(struct device *dev)
> > > {
> > > struct pci_dev *pdev = to_pci_dev(dev);
> > > struct drm_device *drm_dev = pci_get_drvdata(pdev);
> > > struct hibmc_drm_private *priv = drm_dev->dev_private;
> > >
> > > drm_atomic_helper_resume(drm_dev, priv->suspend_state);
> > > drm_kms_helper_poll_enable(drm_dev);
> > >
> > > return 0;
> > > }
> > >
> > >
> > > i915 is excluded since it's rather complicated.
> > >
> > >
> > > static int imx_drm_suspend(struct device *dev)
> > > {
> > > struct drm_device *drm_dev = dev_get_drvdata(dev);
> > > struct imx_drm_device *imxdrm;
> > >
> > > /* The drm_dev is NULL before .load hook is called */
> > > if (drm_dev == NULL)
> > > return 0;
> > >
> > > drm_kms_helper_poll_disable(drm_dev);
> > >
> > > imxdrm = drm_dev->dev_private;
> > > imxdrm->state = drm_atomic_helper_suspend(drm_dev);
> > > if (IS_ERR(imxdrm->state)) {
> > > drm_kms_helper_poll_enable(drm_dev);
> > > return PTR_ERR(imxdrm->state);
> > > }
> > >
> > > return 0;
> > > }
> > >
> > > static int imx_drm_resume(struct device *dev)
> > > {
> > > struct drm_device *drm_dev = dev_get_drvdata(dev);
> > > struct imx_drm_device *imx_drm;
> > >
> > > if (drm_dev == NULL)
> > > return 0;
> > >
> > > imx_drm = drm_dev->dev_private;
> > > drm_atomic_helper_resume(drm_dev, imx_drm->state);
> > > drm_kms_helper_poll_enable(drm_dev);
> > >
> > > return 0;
> > > }
> > >
> > >
> > > static int mtk_drm_sys_suspend(struct device *dev)
> > > {
> > > struct mtk_drm_private *private = dev_get_drvdata(dev);
> > > struct drm_device *drm = private->drm;
> > >
> > > drm_kms_helper_poll_disable(drm);
> > >
> > > private->suspend_state = drm_atomic_helper_suspend(drm);
> > > if (IS_ERR(private->suspend_state)) {
> > > drm_kms_helper_poll_enable(drm);
> > > return PTR_ERR(private->suspend_state);
> > > }
> > >
> > > DRM_DEBUG_DRIVER("mtk_drm_sys_suspend\n");
> > > return 0;
> > > }
> > >
> > > static int mtk_drm_sys_resume(struct device *dev)
> > > {
> > > struct mtk_drm_private *private = dev_get_drvdata(dev);
> > > struct drm_device *drm = private->drm;
> > >
> > > drm_atomic_helper_resume(drm, private->suspend_state);
> > > drm_kms_helper_poll_enable(drm);
> > >
> > > DRM_DEBUG_DRIVER("mtk_drm_sys_resume\n");
> > > return 0;
> > > }
> > >
> > >
> > > nouveau is also rather complicated.
> > >
> > >
> > > static int rockchip_drm_sys_suspend(struct device *dev)
> > > {
> > > struct drm_device *drm = dev_get_drvdata(dev);
> > > struct rockchip_drm_private *priv;
> > >
> > > if (!drm)
> > > return 0;
> > >
> > > drm_kms_helper_poll_disable(drm);
> > > rockchip_drm_fb_suspend(drm);
> > >
> > > priv = drm->dev_private;
> > > priv->state = drm_atomic_helper_suspend(drm);
> > > if (IS_ERR(priv->state)) {
> > > rockchip_drm_fb_resume(drm);
> > > drm_kms_helper_poll_enable(drm);
> > > return PTR_ERR(priv->state);
> > > }
> > >
> > > return 0;
> > > }
> > >
> > > static int rockchip_drm_sys_resume(struct device *dev)
> > > {
> > > struct drm_device *drm = dev_get_drvdata(dev);
> > > struct rockchip_drm_private *priv;
> > >
> > > if (!drm)
> > > return 0;
> > >
> > > priv = drm->dev_private;
> > > drm_atomic_helper_resume(drm, priv->state);
> > > rockchip_drm_fb_resume(drm);
> > > drm_kms_helper_poll_enable(drm);
> > >
> > > return 0;
> > > }
> > >
> > >
> > > static int host1x_drm_suspend(struct device *dev)
> > > {
> > > struct drm_device *drm = dev_get_drvdata(dev);
> > > struct tegra_drm *tegra = drm->dev_private;
> > >
> > > drm_kms_helper_poll_disable(drm);
> > > tegra_drm_fb_suspend(drm);
> > >
> > > tegra->state = drm_atomic_helper_suspend(drm);
> > > if (IS_ERR(tegra->state)) {
> > > tegra_drm_fb_resume(drm);
> > > drm_kms_helper_poll_enable(drm);
> > > return PTR_ERR(tegra->state);
> > > }
> > >
> > > return 0;
> > > }
> > >
> > > static int host1x_drm_resume(struct device *dev)
> > > {
> > > struct drm_device *drm = dev_get_drvdata(dev);
> > > struct tegra_drm *tegra = drm->dev_private;
> > >
> > > drm_atomic_helper_resume(drm, tegra->state);
> > > tegra_drm_fb_resume(drm);
> > > drm_kms_helper_poll_enable(drm);
> > >
> > > return 0;
> > > }
> > >
> > >
> > > static int tilcdc_pm_suspend(struct device *dev)
> > > {
> > > struct drm_device *ddev = dev_get_drvdata(dev);
> > > struct tilcdc_drm_private *priv = ddev->dev_private;
> > >
> > > priv->saved_state = drm_atomic_helper_suspend(ddev);
> > >
> > > /* Select sleep pin state */
> > > pinctrl_pm_select_sleep_state(dev);
> > >
> > > return 0;
> > > }
> > >
> > > static int tilcdc_pm_resume(struct device *dev)
> > > {
> > > struct drm_device *ddev = dev_get_drvdata(dev);
> > > struct tilcdc_drm_private *priv = ddev->dev_private;
> > > int ret = 0;
> > >
> > > /* Select default pin state */
> > > pinctrl_pm_select_default_state(dev);
> > >
> > > if (priv->saved_state)
> > > ret = drm_atomic_helper_resume(ddev, priv->saved_state);
> > >
> > > return ret;
> > > }
> > >
> > >
> > > int tinydrm_suspend(struct tinydrm_device *tdev)
> > > {
> > > struct drm_atomic_state *state;
> > >
> > > if (tdev->suspend_state) {
> > > DRM_ERROR("Failed to suspend: state already set\n");
> > > return -EINVAL;
> > > }
> > >
> > > drm_fbdev_cma_set_suspend_unlocked(tdev->fbdev_cma, 1);
> > > state = drm_atomic_helper_suspend(tdev->drm);
> > > if (IS_ERR(state)) {
> > > drm_fbdev_cma_set_suspend_unlocked(tdev->fbdev_cma, 0);
> > > return PTR_ERR(state);
> > > }
> > >
> > > tdev->suspend_state = state;
> > >
> > > return 0;
> > > }
> > >
> > > int tinydrm_resume(struct tinydrm_device *tdev)
> > > {
> > > struct drm_atomic_state *state = tdev->suspend_state;
> > > int ret;
> > >
> > > if (!state) {
> > > DRM_ERROR("Failed to resume: state is not set\n");
> > > return -EINVAL;
> > > }
> > >
> > > tdev->suspend_state = NULL;
> > >
> > > ret = drm_atomic_helper_resume(tdev->drm, state);
> > > if (ret) {
> > > DRM_ERROR("Error resuming state: %d\n", ret);
> > > return ret;
> > > }
> > >
> > > drm_fbdev_cma_set_suspend_unlocked(tdev->fbdev_cma, 0);
> > >
> > > return 0;
> > > }
> > >
> > >
> > > Noralf.
> > >
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: RFC: Add drm_dev_suspend/resume() ?
2017-11-01 8:47 ` Daniel Vetter
@ 2017-11-01 12:59 ` Noralf Trønnes
2017-11-02 9:40 ` Daniel Vetter
0 siblings, 1 reply; 6+ messages in thread
From: Noralf Trønnes @ 2017-11-01 12:59 UTC (permalink / raw)
To: Daniel Vetter; +Cc: dri-devel
Den 01.11.2017 09.47, skrev Daniel Vetter:
> On Tue, Oct 31, 2017 at 05:37:23PM +0100, Noralf Trønnes wrote:
>> Den 30.10.2017 10.34, skrev Daniel Vetter:
>>> Hi Noralf,
>>>
>>> On Sun, Oct 22, 2017 at 06:52:41PM +0200, Noralf Trønnes wrote:
>>>> Hi,
>>>>
>>>> I've spent some time in the fbdev emulation code and discovered a
>>>> recurring pattern around suspend/resume.
>>>> Should we add some more helpers :-)
>>> You're maybe a bit too good at spotting these for your own good :-)
>>>
>>> But yeah, a "suspend for dummies" is one of the things which would be nice
>>> I think ... Especially since we now have the atomic suspend/resume
>>> helpers.
>>>
>>>> struct drm_device {
>>>> /**
>>>> * @suspend_state:
>>>> *
>>>> * Atomic state when suspended.
>>>> * Set by drm_dev_suspend(), cleared by drm_dev_resume().
>>>> */
>>>> struct drm_atomic_state *suspend_state;
>>>> };
>>> Imo fits better when we put it into drm_mode_config.
>>>
>>>> int drm_dev_suspend(struct drm_device *dev)
>>>> {
>>>> struct drm_atomic_state *state;
>>>>
>>>> if (!dev)
>>>> return 0;
>>>>
>>>> drm_kms_helper_poll_disable(dev);
>>>> drm_fb_helper_set_suspend_unlocked(dev->fb_helper, 1);
>>>> state = drm_atomic_helper_suspend(dev);
>>>> if (IS_ERR(state)) {
>>>> drm_fb_helper_set_suspend_unlocked(dev->fb_helper, 0);
>>>> drm_kms_helper_poll_enable(dev);
>>>> return PTR_ERR(state);
>>>> }
>>>>
>>>> dev->suspend_state = state;
>>>>
>>>> return 0;
>>>> }
>>> This is all about suspending the modeset side ... I'd give it a
>>> drm_mode_config prefix instead of the drm_dev_ (that's maybe a bit too
>>> generic), but then maybe type a general suspend/resume kernel-doc text in
>>> the drm-internals.rst (maybe pulled in from drm_dev.c) which references
>>> these 2 functions as the recommended way to suspend/resume the modeset
>>> side of a driver. These won't suspend/resume a render part (if present),
>>> so drm_dev_ seems a bit too much.
>> I just realised that this is pulling helpers (atomic, crtc, fbdev) into
>> the core which IIRC is something that you didn't want?
> Ugh right. I think starting a new drm_mode_config_helper.c for top-level
> helper stuff would be a reasonable solution. Or some other name if you
> have a better one.
Does it fit in drm_modeset_helper.c ?
/**
* DOC: aux kms helpers
*
* This helper library contains various one-off functions which don't
really fit
* anywhere else in the DRM modeset helper library.
*/
Noralf.
> -Daniel
>
>>
>> diff --git a/drivers/gpu/drm/drm_mode_config.c
>> b/drivers/gpu/drm/drm_mode_config.c
>> index 74f6ff5df656..72f8fe1e83cb 100644
>> --- a/drivers/gpu/drm/drm_mode_config.c
>> +++ b/drivers/gpu/drm/drm_mode_config.c
>> @@ -20,7 +20,10 @@
>> * OF THIS SOFTWARE.
>> */
>>
>> +#include <drm/drm_atomic_helper.h>
>> +#include <drm/drm_crtc_helper.h>
>> #include <drm/drm_encoder.h>
>> +#include <drm/drm_fb_helper.h>
>> #include <drm/drm_mode_config.h>
>> #include <drm/drmP.h>
>>
>> @@ -473,3 +476,49 @@ void drm_mode_config_cleanup(struct drm_device *dev)
>> drm_modeset_lock_fini(&dev->mode_config.connection_mutex);
>> }
>> EXPORT_SYMBOL(drm_mode_config_cleanup);
>> +
>> +/**
>> + * drm_mode_config_suspend
>> + * @dev: DRM device
>> + *
>> + */
>> +int drm_mode_config_suspend(struct drm_device *dev)
>> +{
>> + struct drm_atomic_state *state;
>> +
>> + if (!dev)
>> + return 0;
>> +
>> + drm_kms_helper_poll_disable(dev);
>> + drm_fb_helper_set_suspend_unlocked(dev->fb_helper, 1);
>> + state = drm_atomic_helper_suspend(dev);
>> + if (IS_ERR(state)) {
>> + drm_fb_helper_set_suspend_unlocked(dev->fb_helper, 0);
>> + drm_kms_helper_poll_enable(dev);
>> + return PTR_ERR(state);
>> + }
>> +
>> + dev->mode_config.suspend_state = state;
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL(drm_mode_config_suspend);
>> +
>> +int drm_mode_config_resume(struct drm_device *dev)
>> +{
>> + int ret;
>> +
>> + if (!dev || WARN_ON(!dev->mode_config.suspend_state))
>> + return 0;
>> +
>> + ret = drm_atomic_helper_resume(dev, dev->mode_config.suspend_state);
>> + if (ret)
>> + DRM_ERROR("Failed to resume (%d)\n", ret);
>> + dev->mode_config.suspend_state = NULL;
>> +
>> + drm_fb_helper_set_suspend_unlocked(dev->fb_helper, 0);
>> + drm_kms_helper_poll_enable(dev);
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL(drm_mode_config_resume);
>> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
>> index 1b37368416c8..86ef327a996d 100644
>> --- a/include/drm/drm_mode_config.h
>> +++ b/include/drm/drm_mode_config.h
>> @@ -766,11 +766,21 @@ struct drm_mode_config {
>> /* cursor size */
>> uint32_t cursor_width, cursor_height;
>>
>> + /**
>> + * @suspend_state:
>> + *
>> + * Atomic state when suspended. Set by drm_mode_config_suspend(),
>> + * cleared by drm_mode_config_resume().
>> + */
>> + struct drm_atomic_state *suspend_state;
>> +
>> const struct drm_mode_config_helper_funcs *helper_private;
>> };
>>
>> void drm_mode_config_init(struct drm_device *dev);
>> void drm_mode_config_reset(struct drm_device *dev);
>> void drm_mode_config_cleanup(struct drm_device *dev);
>> +int drm_mode_config_suspend(struct drm_device *dev);
>> +int drm_mode_config_resume(struct drm_device *dev);
>>
>> #endif
>>
>>
>> Noralf.
>>
>>>> int drm_dev_resume(struct drm_device *dev)
>>>> {
>>>> int ret;
>>>>
>>>> if (!dev || WARN_ON(!dev->suspend_state))
>>>> return 0;
>>>>
>>>> ret = drm_atomic_helper_resume(dev, dev->suspend_state);
>>>> if (ret)
>>>> DRM_ERROR("Failed to resume (%d)\n", ret);
>>>> dev->suspend_state = NULL;
>>>>
>>>> drm_fb_helper_set_suspend_unlocked(dev->fb_helper, 0);
>>>> drm_kms_helper_poll_enable(dev);
>>>>
>>>> return 0;
>>>> }
>>>>
>>>> The drm_kms_helper_poll_enable() docs states that it's not allowed to
>>>> call this function if polling support has been set up, but the code
>>>> allows it. drm_kms_helper_poll_disable() docs states that it's ok to
>>>> call it even if polling is not enabled.
>>> Yeah need to fix that, but need to keep a note that if you want polling,
>>> then you need to follow that order or it goes wrong. Perhaps explain that
>>> this is meant to avoid special-cases in suspend/resume code?
>>>
>>>> Here are the suspend/resume code from the drivers that use
>>>> drm_atomic_helper_suspend/resume():
>>> Yeah, looks like a really good idea.
>>>
>>> Thanks, Daniel
>>>> static int __maybe_unused hdlcd_pm_suspend(struct device *dev)
>>>> {
>>>> struct drm_device *drm = dev_get_drvdata(dev);
>>>> struct hdlcd_drm_private *hdlcd = drm ? drm->dev_private : NULL;
>>>>
>>>> if (!hdlcd)
>>>> return 0;
>>>>
>>>> drm_kms_helper_poll_disable(drm);
>>>>
>>>> hdlcd->state = drm_atomic_helper_suspend(drm);
>>>> if (IS_ERR(hdlcd->state)) {
>>>> drm_kms_helper_poll_enable(drm);
>>>> return PTR_ERR(hdlcd->state);
>>>> }
>>>>
>>>> return 0;
>>>> }
>>>>
>>>> static int __maybe_unused hdlcd_pm_resume(struct device *dev)
>>>> {
>>>> struct drm_device *drm = dev_get_drvdata(dev);
>>>> struct hdlcd_drm_private *hdlcd = drm ? drm->dev_private : NULL;
>>>>
>>>> if (!hdlcd)
>>>> return 0;
>>>>
>>>> drm_atomic_helper_resume(drm, hdlcd->state);
>>>> drm_kms_helper_poll_enable(drm);
>>>> pm_runtime_set_active(dev);
>>>>
>>>> return 0;
>>>> }
>>>>
>>>>
>>>> static int __maybe_unused malidp_pm_suspend(struct device *dev)
>>>> {
>>>> struct drm_device *drm = dev_get_drvdata(dev);
>>>> struct malidp_drm *malidp = drm->dev_private;
>>>>
>>>> drm_kms_helper_poll_disable(drm);
>>>> console_lock();
>>>> drm_fbdev_cma_set_suspend(malidp->fbdev, 1);
>>>> console_unlock();
>>>> malidp->pm_state = drm_atomic_helper_suspend(drm);
>>>> if (IS_ERR(malidp->pm_state)) {
>>>> console_lock();
>>>> drm_fbdev_cma_set_suspend(malidp->fbdev, 0);
>>>> console_unlock();
>>>> drm_kms_helper_poll_enable(drm);
>>>> return PTR_ERR(malidp->pm_state);
>>>> }
>>>>
>>>> return 0;
>>>> }
>>>>
>>>> static int __maybe_unused malidp_pm_resume(struct device *dev)
>>>> {
>>>> struct drm_device *drm = dev_get_drvdata(dev);
>>>> struct malidp_drm *malidp = drm->dev_private;
>>>>
>>>> drm_atomic_helper_resume(drm, malidp->pm_state);
>>>> console_lock();
>>>> drm_fbdev_cma_set_suspend(malidp->fbdev, 0);
>>>> console_unlock();
>>>> drm_kms_helper_poll_enable(drm);
>>>>
>>>> return 0;
>>>> }
>>>>
>>>>
>>>> static int atmel_hlcdc_dc_drm_suspend(struct device *dev)
>>>> {
>>>> struct drm_device *drm_dev = dev_get_drvdata(dev);
>>>> struct atmel_hlcdc_dc *dc = drm_dev->dev_private;
>>>> struct regmap *regmap = dc->hlcdc->regmap;
>>>> struct drm_atomic_state *state;
>>>>
>>>> state = drm_atomic_helper_suspend(drm_dev);
>>>> if (IS_ERR(state))
>>>> return PTR_ERR(state);
>>>>
>>>> dc->suspend.state = state;
>>>>
>>>> regmap_read(regmap, ATMEL_HLCDC_IMR, &dc->suspend.imr);
>>>> regmap_write(regmap, ATMEL_HLCDC_IDR, dc->suspend.imr);
>>>> clk_disable_unprepare(dc->hlcdc->periph_clk);
>>>>
>>>> return 0;
>>>> }
>>>>
>>>> static int atmel_hlcdc_dc_drm_resume(struct device *dev)
>>>> {
>>>> struct drm_device *drm_dev = dev_get_drvdata(dev);
>>>> struct atmel_hlcdc_dc *dc = drm_dev->dev_private;
>>>>
>>>> clk_prepare_enable(dc->hlcdc->periph_clk);
>>>> regmap_write(dc->hlcdc->regmap, ATMEL_HLCDC_IER, dc->suspend.imr);
>>>>
>>>> return drm_atomic_helper_resume(drm_dev, dc->suspend.state);
>>>> }
>>>>
>>>>
>>>> static int exynos_drm_suspend(struct device *dev)
>>>> {
>>>> struct drm_device *drm_dev = dev_get_drvdata(dev);
>>>> struct exynos_drm_private *private = drm_dev->dev_private;
>>>>
>>>> if (pm_runtime_suspended(dev) || !drm_dev)
>>>> return 0;
>>>>
>>>> drm_kms_helper_poll_disable(drm_dev);
>>>> exynos_drm_fbdev_suspend(drm_dev);
>>>> private->suspend_state = drm_atomic_helper_suspend(drm_dev);
>>>> if (IS_ERR(private->suspend_state)) {
>>>> exynos_drm_fbdev_resume(drm_dev);
>>>> drm_kms_helper_poll_enable(drm_dev);
>>>> return PTR_ERR(private->suspend_state);
>>>> }
>>>>
>>>> return 0;
>>>> }
>>>>
>>>> static int exynos_drm_resume(struct device *dev)
>>>> {
>>>> struct drm_device *drm_dev = dev_get_drvdata(dev);
>>>> struct exynos_drm_private *private = drm_dev->dev_private;
>>>>
>>>> if (pm_runtime_suspended(dev) || !drm_dev)
>>>> return 0;
>>>>
>>>> drm_atomic_helper_resume(drm_dev, private->suspend_state);
>>>> exynos_drm_fbdev_resume(drm_dev);
>>>> drm_kms_helper_poll_enable(drm_dev);
>>>>
>>>> return 0;
>>>> }
>>>>
>>>>
>>>> static int fsl_dcu_drm_pm_suspend(struct device *dev)
>>>> {
>>>> struct fsl_dcu_drm_device *fsl_dev = dev_get_drvdata(dev);
>>>>
>>>> if (!fsl_dev)
>>>> return 0;
>>>>
>>>> disable_irq(fsl_dev->irq);
>>>> drm_kms_helper_poll_disable(fsl_dev->drm);
>>>>
>>>> console_lock();
>>>> drm_fbdev_cma_set_suspend(fsl_dev->fbdev, 1);
>>>> console_unlock();
>>>>
>>>> fsl_dev->state = drm_atomic_helper_suspend(fsl_dev->drm);
>>>> if (IS_ERR(fsl_dev->state)) {
>>>> console_lock();
>>>> drm_fbdev_cma_set_suspend(fsl_dev->fbdev, 0);
>>>> console_unlock();
>>>>
>>>> drm_kms_helper_poll_enable(fsl_dev->drm);
>>>> enable_irq(fsl_dev->irq);
>>>> return PTR_ERR(fsl_dev->state);
>>>> }
>>>>
>>>> clk_disable_unprepare(fsl_dev->pix_clk);
>>>> clk_disable_unprepare(fsl_dev->clk);
>>>>
>>>> return 0;
>>>> }
>>>>
>>>> static int fsl_dcu_drm_pm_resume(struct device *dev)
>>>> {
>>>> struct fsl_dcu_drm_device *fsl_dev = dev_get_drvdata(dev);
>>>> int ret;
>>>>
>>>> if (!fsl_dev)
>>>> return 0;
>>>>
>>>> ret = clk_prepare_enable(fsl_dev->clk);
>>>> if (ret < 0) {
>>>> dev_err(dev, "failed to enable dcu clk\n");
>>>> return ret;
>>>> }
>>>>
>>>> if (fsl_dev->tcon)
>>>> fsl_tcon_bypass_enable(fsl_dev->tcon);
>>>> fsl_dcu_drm_init_planes(fsl_dev->drm);
>>>> drm_atomic_helper_resume(fsl_dev->drm, fsl_dev->state);
>>>>
>>>> console_lock();
>>>> drm_fbdev_cma_set_suspend(fsl_dev->fbdev, 0);
>>>> console_unlock();
>>>>
>>>> drm_kms_helper_poll_enable(fsl_dev->drm);
>>>> enable_irq(fsl_dev->irq);
>>>>
>>>> return 0;
>>>> }
>>>>
>>>>
>>>> static int __maybe_unused hibmc_pm_suspend(struct device *dev)
>>>> {
>>>> struct pci_dev *pdev = to_pci_dev(dev);
>>>> struct drm_device *drm_dev = pci_get_drvdata(pdev);
>>>> struct hibmc_drm_private *priv = drm_dev->dev_private;
>>>>
>>>> drm_kms_helper_poll_disable(drm_dev);
>>>> priv->suspend_state = drm_atomic_helper_suspend(drm_dev);
>>>> if (IS_ERR(priv->suspend_state)) {
>>>> DRM_ERROR("drm_atomic_helper_suspend failed: %ld\n",
>>>> PTR_ERR(priv->suspend_state));
>>>> drm_kms_helper_poll_enable(drm_dev);
>>>> return PTR_ERR(priv->suspend_state);
>>>> }
>>>>
>>>> return 0;
>>>> }
>>>>
>>>> static int __maybe_unused hibmc_pm_resume(struct device *dev)
>>>> {
>>>> struct pci_dev *pdev = to_pci_dev(dev);
>>>> struct drm_device *drm_dev = pci_get_drvdata(pdev);
>>>> struct hibmc_drm_private *priv = drm_dev->dev_private;
>>>>
>>>> drm_atomic_helper_resume(drm_dev, priv->suspend_state);
>>>> drm_kms_helper_poll_enable(drm_dev);
>>>>
>>>> return 0;
>>>> }
>>>>
>>>>
>>>> i915 is excluded since it's rather complicated.
>>>>
>>>>
>>>> static int imx_drm_suspend(struct device *dev)
>>>> {
>>>> struct drm_device *drm_dev = dev_get_drvdata(dev);
>>>> struct imx_drm_device *imxdrm;
>>>>
>>>> /* The drm_dev is NULL before .load hook is called */
>>>> if (drm_dev == NULL)
>>>> return 0;
>>>>
>>>> drm_kms_helper_poll_disable(drm_dev);
>>>>
>>>> imxdrm = drm_dev->dev_private;
>>>> imxdrm->state = drm_atomic_helper_suspend(drm_dev);
>>>> if (IS_ERR(imxdrm->state)) {
>>>> drm_kms_helper_poll_enable(drm_dev);
>>>> return PTR_ERR(imxdrm->state);
>>>> }
>>>>
>>>> return 0;
>>>> }
>>>>
>>>> static int imx_drm_resume(struct device *dev)
>>>> {
>>>> struct drm_device *drm_dev = dev_get_drvdata(dev);
>>>> struct imx_drm_device *imx_drm;
>>>>
>>>> if (drm_dev == NULL)
>>>> return 0;
>>>>
>>>> imx_drm = drm_dev->dev_private;
>>>> drm_atomic_helper_resume(drm_dev, imx_drm->state);
>>>> drm_kms_helper_poll_enable(drm_dev);
>>>>
>>>> return 0;
>>>> }
>>>>
>>>>
>>>> static int mtk_drm_sys_suspend(struct device *dev)
>>>> {
>>>> struct mtk_drm_private *private = dev_get_drvdata(dev);
>>>> struct drm_device *drm = private->drm;
>>>>
>>>> drm_kms_helper_poll_disable(drm);
>>>>
>>>> private->suspend_state = drm_atomic_helper_suspend(drm);
>>>> if (IS_ERR(private->suspend_state)) {
>>>> drm_kms_helper_poll_enable(drm);
>>>> return PTR_ERR(private->suspend_state);
>>>> }
>>>>
>>>> DRM_DEBUG_DRIVER("mtk_drm_sys_suspend\n");
>>>> return 0;
>>>> }
>>>>
>>>> static int mtk_drm_sys_resume(struct device *dev)
>>>> {
>>>> struct mtk_drm_private *private = dev_get_drvdata(dev);
>>>> struct drm_device *drm = private->drm;
>>>>
>>>> drm_atomic_helper_resume(drm, private->suspend_state);
>>>> drm_kms_helper_poll_enable(drm);
>>>>
>>>> DRM_DEBUG_DRIVER("mtk_drm_sys_resume\n");
>>>> return 0;
>>>> }
>>>>
>>>>
>>>> nouveau is also rather complicated.
>>>>
>>>>
>>>> static int rockchip_drm_sys_suspend(struct device *dev)
>>>> {
>>>> struct drm_device *drm = dev_get_drvdata(dev);
>>>> struct rockchip_drm_private *priv;
>>>>
>>>> if (!drm)
>>>> return 0;
>>>>
>>>> drm_kms_helper_poll_disable(drm);
>>>> rockchip_drm_fb_suspend(drm);
>>>>
>>>> priv = drm->dev_private;
>>>> priv->state = drm_atomic_helper_suspend(drm);
>>>> if (IS_ERR(priv->state)) {
>>>> rockchip_drm_fb_resume(drm);
>>>> drm_kms_helper_poll_enable(drm);
>>>> return PTR_ERR(priv->state);
>>>> }
>>>>
>>>> return 0;
>>>> }
>>>>
>>>> static int rockchip_drm_sys_resume(struct device *dev)
>>>> {
>>>> struct drm_device *drm = dev_get_drvdata(dev);
>>>> struct rockchip_drm_private *priv;
>>>>
>>>> if (!drm)
>>>> return 0;
>>>>
>>>> priv = drm->dev_private;
>>>> drm_atomic_helper_resume(drm, priv->state);
>>>> rockchip_drm_fb_resume(drm);
>>>> drm_kms_helper_poll_enable(drm);
>>>>
>>>> return 0;
>>>> }
>>>>
>>>>
>>>> static int host1x_drm_suspend(struct device *dev)
>>>> {
>>>> struct drm_device *drm = dev_get_drvdata(dev);
>>>> struct tegra_drm *tegra = drm->dev_private;
>>>>
>>>> drm_kms_helper_poll_disable(drm);
>>>> tegra_drm_fb_suspend(drm);
>>>>
>>>> tegra->state = drm_atomic_helper_suspend(drm);
>>>> if (IS_ERR(tegra->state)) {
>>>> tegra_drm_fb_resume(drm);
>>>> drm_kms_helper_poll_enable(drm);
>>>> return PTR_ERR(tegra->state);
>>>> }
>>>>
>>>> return 0;
>>>> }
>>>>
>>>> static int host1x_drm_resume(struct device *dev)
>>>> {
>>>> struct drm_device *drm = dev_get_drvdata(dev);
>>>> struct tegra_drm *tegra = drm->dev_private;
>>>>
>>>> drm_atomic_helper_resume(drm, tegra->state);
>>>> tegra_drm_fb_resume(drm);
>>>> drm_kms_helper_poll_enable(drm);
>>>>
>>>> return 0;
>>>> }
>>>>
>>>>
>>>> static int tilcdc_pm_suspend(struct device *dev)
>>>> {
>>>> struct drm_device *ddev = dev_get_drvdata(dev);
>>>> struct tilcdc_drm_private *priv = ddev->dev_private;
>>>>
>>>> priv->saved_state = drm_atomic_helper_suspend(ddev);
>>>>
>>>> /* Select sleep pin state */
>>>> pinctrl_pm_select_sleep_state(dev);
>>>>
>>>> return 0;
>>>> }
>>>>
>>>> static int tilcdc_pm_resume(struct device *dev)
>>>> {
>>>> struct drm_device *ddev = dev_get_drvdata(dev);
>>>> struct tilcdc_drm_private *priv = ddev->dev_private;
>>>> int ret = 0;
>>>>
>>>> /* Select default pin state */
>>>> pinctrl_pm_select_default_state(dev);
>>>>
>>>> if (priv->saved_state)
>>>> ret = drm_atomic_helper_resume(ddev, priv->saved_state);
>>>>
>>>> return ret;
>>>> }
>>>>
>>>>
>>>> int tinydrm_suspend(struct tinydrm_device *tdev)
>>>> {
>>>> struct drm_atomic_state *state;
>>>>
>>>> if (tdev->suspend_state) {
>>>> DRM_ERROR("Failed to suspend: state already set\n");
>>>> return -EINVAL;
>>>> }
>>>>
>>>> drm_fbdev_cma_set_suspend_unlocked(tdev->fbdev_cma, 1);
>>>> state = drm_atomic_helper_suspend(tdev->drm);
>>>> if (IS_ERR(state)) {
>>>> drm_fbdev_cma_set_suspend_unlocked(tdev->fbdev_cma, 0);
>>>> return PTR_ERR(state);
>>>> }
>>>>
>>>> tdev->suspend_state = state;
>>>>
>>>> return 0;
>>>> }
>>>>
>>>> int tinydrm_resume(struct tinydrm_device *tdev)
>>>> {
>>>> struct drm_atomic_state *state = tdev->suspend_state;
>>>> int ret;
>>>>
>>>> if (!state) {
>>>> DRM_ERROR("Failed to resume: state is not set\n");
>>>> return -EINVAL;
>>>> }
>>>>
>>>> tdev->suspend_state = NULL;
>>>>
>>>> ret = drm_atomic_helper_resume(tdev->drm, state);
>>>> if (ret) {
>>>> DRM_ERROR("Error resuming state: %d\n", ret);
>>>> return ret;
>>>> }
>>>>
>>>> drm_fbdev_cma_set_suspend_unlocked(tdev->fbdev_cma, 0);
>>>>
>>>> return 0;
>>>> }
>>>>
>>>>
>>>> Noralf.
>>>>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: RFC: Add drm_dev_suspend/resume() ?
2017-11-01 12:59 ` Noralf Trønnes
@ 2017-11-02 9:40 ` Daniel Vetter
0 siblings, 0 replies; 6+ messages in thread
From: Daniel Vetter @ 2017-11-02 9:40 UTC (permalink / raw)
To: Noralf Trønnes; +Cc: dri-devel
On Wed, Nov 01, 2017 at 01:59:00PM +0100, Noralf Trønnes wrote:
>
> Den 01.11.2017 09.47, skrev Daniel Vetter:
> > On Tue, Oct 31, 2017 at 05:37:23PM +0100, Noralf Trønnes wrote:
> > > Den 30.10.2017 10.34, skrev Daniel Vetter:
> > > > Hi Noralf,
> > > >
> > > > On Sun, Oct 22, 2017 at 06:52:41PM +0200, Noralf Trønnes wrote:
> > > > > Hi,
> > > > >
> > > > > I've spent some time in the fbdev emulation code and discovered a
> > > > > recurring pattern around suspend/resume.
> > > > > Should we add some more helpers :-)
> > > > You're maybe a bit too good at spotting these for your own good :-)
> > > >
> > > > But yeah, a "suspend for dummies" is one of the things which would be nice
> > > > I think ... Especially since we now have the atomic suspend/resume
> > > > helpers.
> > > >
> > > > > struct drm_device {
> > > > > /**
> > > > > * @suspend_state:
> > > > > *
> > > > > * Atomic state when suspended.
> > > > > * Set by drm_dev_suspend(), cleared by drm_dev_resume().
> > > > > */
> > > > > struct drm_atomic_state *suspend_state;
> > > > > };
> > > > Imo fits better when we put it into drm_mode_config.
> > > >
> > > > > int drm_dev_suspend(struct drm_device *dev)
> > > > > {
> > > > > struct drm_atomic_state *state;
> > > > >
> > > > > if (!dev)
> > > > > return 0;
> > > > >
> > > > > drm_kms_helper_poll_disable(dev);
> > > > > drm_fb_helper_set_suspend_unlocked(dev->fb_helper, 1);
> > > > > state = drm_atomic_helper_suspend(dev);
> > > > > if (IS_ERR(state)) {
> > > > > drm_fb_helper_set_suspend_unlocked(dev->fb_helper, 0);
> > > > > drm_kms_helper_poll_enable(dev);
> > > > > return PTR_ERR(state);
> > > > > }
> > > > >
> > > > > dev->suspend_state = state;
> > > > >
> > > > > return 0;
> > > > > }
> > > > This is all about suspending the modeset side ... I'd give it a
> > > > drm_mode_config prefix instead of the drm_dev_ (that's maybe a bit too
> > > > generic), but then maybe type a general suspend/resume kernel-doc text in
> > > > the drm-internals.rst (maybe pulled in from drm_dev.c) which references
> > > > these 2 functions as the recommended way to suspend/resume the modeset
> > > > side of a driver. These won't suspend/resume a render part (if present),
> > > > so drm_dev_ seems a bit too much.
> > > I just realised that this is pulling helpers (atomic, crtc, fbdev) into
> > > the core which IIRC is something that you didn't want?
> > Ugh right. I think starting a new drm_mode_config_helper.c for top-level
> > helper stuff would be a reasonable solution. Or some other name if you
> > have a better one.
>
> Does it fit in drm_modeset_helper.c ?
>
> /**
> * DOC: aux kms helpers
> *
> * This helper library contains various one-off functions which don't really
> fit
> * anywhere else in the DRM modeset helper library.
> */
Even better I think, avoids another file no one remembers exists :-)
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-11-02 9:40 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-22 16:52 RFC: Add drm_dev_suspend/resume() ? Noralf Trønnes
2017-10-30 9:34 ` Daniel Vetter
2017-10-31 16:37 ` Noralf Trønnes
2017-11-01 8:47 ` Daniel Vetter
2017-11-01 12:59 ` Noralf Trønnes
2017-11-02 9:40 ` Daniel Vetter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.