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