RFC: Add drm_dev_suspend/resume() ?

Noralf Trønnes noralf at tronnes.org
Tue Oct 31 16:37:23 UTC 2017


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.
>>



More information about the dri-devel mailing list