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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
