Re: [PATCH v2 26/26] drm/exynos: Consolidate suspend/resume in drm_drv

On Wed, Oct 16, 2013 at 3:26 PM, Sean Paul seanpaul@chromium.org wrote:

This patch removes all of the suspend/resume logic from the individual drivers and consolidates it in drm_drv. This consolidation reduces the number of functions which enable/disable the hardware to just one -- the dpms callback. This ensures that we always power up/down in a consistent manner.

Signed-off-by: Sean Paul seanpaul@chromium.org

Changes in v2: - Added to the patchset

drivers/gpu/drm/exynos/exynos_drm_drv.c | 97 ++++++++++++++++++++++++++++++++ drivers/gpu/drm/exynos/exynos_drm_fimd.c | 91 +++++------------------------- drivers/gpu/drm/exynos/exynos_hdmi.c | 82 +-------------------------- drivers/gpu/drm/exynos/exynos_mixer.c | 75 +++++------------------- 4 files changed, 127 insertions(+), 218 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c index 03caa3a..91d6863 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c @@ -11,6 +11,7 @@

• option) any later version.

*/

+#include <linux/pm_runtime.h> #include <drm/drmP.h> #include <drm/drm_crtc_helper.h>

@@ -51,6 +52,7 @@ static int exynos_drm_load(struct drm_device *dev, unsigned long flags) return -ENOMEM;

    INIT_LIST_HEAD(&private->pageflip_event_list);

•   dev_set_drvdata(dev->dev, dev);
    dev->dev_private = (void *)private;
  
    /*
  

@@ -155,6 +157,41 @@ static int exynos_drm_unload(struct drm_device *dev) return 0; }

+static int exynos_drm_suspend(struct drm_device *dev, pm_message_t state) +{

•   struct drm_connector *connector;
  

•   drm_modeset_lock_all(dev);
  

•   list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
  

•           int old_dpms = connector->dpms;
  

•           if (connector->funcs->dpms)
  

•                   connector->funcs->dpms(connector, DRM_MODE_DPMS_OFF);
  

•           /* Set the old mode back to the connector for resume */
  

•           connector->dpms = old_dpms;
  

•   }
  

•   drm_modeset_unlock_all(dev);
  

•   return 0;
  

+}

+static int exynos_drm_resume(struct drm_device *dev) +{

•   struct drm_connector *connector;
  

•   drm_modeset_lock_all(dev);
  

•   list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
  

•           if (connector->funcs->dpms)
  

•                   connector->funcs->dpms(connector, connector->dpms);
  

•   }
  

•   drm_helper_resume_force_mode(dev);
  

•   drm_modeset_unlock_all(dev);
  

•   return 0;
  

+}

static int exynos_drm_open(struct drm_device *dev, struct drm_file *file) { struct drm_exynos_file_private *file_priv; @@ -262,6 +299,8 @@ static struct drm_driver exynos_drm_driver = { DRIVER_GEM | DRIVER_PRIME, .load = exynos_drm_load, .unload = exynos_drm_unload,

•   .suspend                = exynos_drm_suspend,
  

•   .resume                 = exynos_drm_resume,
    .open                   = exynos_drm_open,
    .preclose               = exynos_drm_preclose,
    .lastclose              = exynos_drm_lastclose,
  

@@ -293,6 +332,9 @@ static int exynos_drm_platform_probe(struct platform_device *pdev) { pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32);

•   pm_runtime_enable(&pdev->dev);
  

•   pm_runtime_get_sync(&pdev->dev);
  

•   return drm_platform_init(&exynos_drm_driver, pdev);
  

}

@@ -303,12 +345,67 @@ static int exynos_drm_platform_remove(struct platform_device *pdev) return 0; }

+#ifdef CONFIG_PM_SLEEP +static int exynos_drm_sys_suspend(struct device *dev) +{

•   struct drm_device *drm_dev = dev_get_drvdata(dev);
  

•   pm_message_t message;
  

•   if (pm_runtime_suspended(dev))
  

•           return 0;
  

•   message.event = PM_EVENT_SUSPEND;
  

•   return exynos_drm_suspend(drm_dev, message);
  

+}

+static int exynos_drm_sys_resume(struct device *dev) +{

•   struct drm_device *drm_dev = dev_get_drvdata(dev);
  

•   if (pm_runtime_suspended(dev))
  

•           return 0;
  

•   return exynos_drm_resume(drm_dev);
  

+} +#endif

+#ifdef CONFIG_PM_RUNTIME +static int exynos_drm_runtime_suspend(struct device *dev) +{

•   struct drm_device *drm_dev = dev_get_drvdata(dev);
  

•   pm_message_t message;
  

•   if (pm_runtime_suspended(dev))
  

•           return 0;
  

•   message.event = PM_EVENT_SUSPEND;
  

•   return exynos_drm_suspend(drm_dev, message);
  

+}

+static int exynos_drm_runtime_resume(struct device *dev) +{

•   struct drm_device *drm_dev = dev_get_drvdata(dev);
  

•   if (!pm_runtime_suspended(dev))
  

•           return 0;
  

•   return exynos_drm_resume(drm_dev);
  

+} +#endif

+static const struct dev_pm_ops exynos_drm_pm_ops = {

•   SET_SYSTEM_SLEEP_PM_OPS(exynos_drm_sys_suspend, exynos_drm_sys_resume)
  

•   SET_RUNTIME_PM_OPS(exynos_drm_runtime_suspend,
  

•                   exynos_drm_runtime_resume, NULL)
  

+};

static struct platform_driver exynos_drm_platform_driver = { .probe = exynos_drm_platform_probe, .remove = exynos_drm_platform_remove, .driver = { .owner = THIS_MODULE, .name = "exynos-drm",

•           .pm     = &exynos_drm_pm_ops,
    },
  

};

diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c index ba12916..208e013 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c @@ -741,6 +741,8 @@ static int fimd_poweron(struct exynos_drm_manager *mgr)

    ctx->suspended = false;

•   pm_runtime_get_sync(ctx->dev);
  

•   ret = clk_prepare_enable(ctx->bus_clk);
    if (ret < 0) {
            DRM_ERROR("Failed to prepare_enable the bus clk [%d]\n", ret);
  

@@ -794,32 +796,24 @@ static int fimd_poweroff(struct exynos_drm_manager *mgr) clk_disable_unprepare(ctx->lcd_clk); clk_disable_unprepare(ctx->bus_clk);

•   pm_runtime_put_sync(ctx->dev);
  

•   ctx->suspended = true;
    return 0;
  

}

static void fimd_dpms(struct exynos_drm_manager *mgr, int mode) {

•   struct fimd_context *ctx = mgr->ctx;
  

•   DRM_DEBUG_KMS("%s, %d\n", __FILE__, mode);
  
    switch (mode) {
    case DRM_MODE_DPMS_ON:
  

•           /*
  

•            * enable fimd hardware only if suspended status.
  

•            *
  

•            * P.S. fimd_dpms function would be called at booting time so
  

•            * clk_enable could be called double time.
  

•            */
  

•           if (ctx->suspended)
  

•                   pm_runtime_get_sync(ctx->dev);
  

•           fimd_poweron(mgr);
            break;
    case DRM_MODE_DPMS_STANDBY:
    case DRM_MODE_DPMS_SUSPEND:
    case DRM_MODE_DPMS_OFF:
  

•           if (!ctx->suspended)
  

•                   pm_runtime_put_sync(ctx->dev);
  

•           fimd_poweroff(mgr);
            break;
    default:
            DRM_DEBUG_KMS("unspecified mode %d\n", mode);
  

@@ -950,8 +944,14 @@ static int fimd_probe(struct platform_device *pdev) fimd_manager.ctx = ctx; exynos_drm_manager_register(&fimd_manager);

•   /*
  

•    * We need to runtime pm to enable/disable sysmmu since it is a child of
  

•    * this driver. Ideally, this would hang off the drm driver's runtime
  

•    * operations, but we're not quite there yet.
  

•    *
  

•    * Tracked in crbug.com/264312
  

Argh, this (and the similar comment below) slipped by me, sorry about that.

Can you remove, or would you like to to send a new version?

Sean

•    */
    pm_runtime_enable(dev);
  

•   pm_runtime_get_sync(dev);
  
    for (win = 0; win < WINDOWS_NR; win++)
            fimd_clear_win(ctx, win);
  

@@ -961,84 +961,23 @@ static int fimd_probe(struct platform_device *pdev)

static int fimd_remove(struct platform_device *pdev) {

•   struct device *dev = &pdev->dev;
    struct exynos_drm_manager *mgr = platform_get_drvdata(pdev);
  

•   struct fimd_context *ctx = mgr->ctx;
  
    exynos_drm_manager_unregister(&fimd_manager);
  
  

•   if (ctx->suspended)
  

•           goto out;
  

•   pm_runtime_set_suspended(dev);
  

•   pm_runtime_put_sync(dev);
  

•   fimd_dpms(mgr, DRM_MODE_DPMS_OFF);
  
  

-out:

•   pm_runtime_disable(dev);
  

•   pm_runtime_disable(&pdev->dev);
  
    return 0;
  

}

-#ifdef CONFIG_PM_SLEEP -static int fimd_suspend(struct device *dev) -{

•   struct exynos_drm_manager *mgr = get_fimd_manager(dev);
  

•   /*
  

•    * do not use pm_runtime_suspend(). if pm_runtime_suspend() is
  

•    * called here, an error would be returned by that interface
  

•    * because the usage_count of pm runtime is more than 1.
  

•    */
  

•   if (!pm_runtime_suspended(dev))
  

•           return fimd_poweroff(mgr);
  

•   return 0;
  

-}

-static int fimd_resume(struct device *dev) -{

•   struct exynos_drm_manager *mgr = get_fimd_manager(dev);
  

•   /*
  

•    * if entered to sleep when lcd panel was on, the usage_count
  

•    * of pm runtime would still be 1 so in this case, fimd driver
  

•    * should be on directly not drawing on pm runtime interface.
  

•    */
  

•   if (pm_runtime_suspended(dev))
  

•           return 0;
  

•   return fimd_poweron(mgr);
  

-} -#endif

-#ifdef CONFIG_PM_RUNTIME -static int fimd_runtime_suspend(struct device *dev) -{

•   struct exynos_drm_manager *mgr = get_fimd_manager(dev);
  

•   return fimd_poweroff(mgr);
  

-}

-static int fimd_runtime_resume(struct device *dev) -{

•   struct exynos_drm_manager *mgr = get_fimd_manager(dev);
  

•   return fimd_poweron(mgr);
  

-} -#endif

-static const struct dev_pm_ops fimd_pm_ops = {

•   SET_SYSTEM_SLEEP_PM_OPS(fimd_suspend, fimd_resume)
  

•   SET_RUNTIME_PM_OPS(fimd_runtime_suspend, fimd_runtime_resume, NULL)
  

-};

struct platform_driver fimd_driver = { .probe = fimd_probe, .remove = fimd_remove, .driver = { .name = "exynos4-fb", .owner = THIS_MODULE,

•           .pm     = &fimd_pm_ops,
            .of_match_table = fimd_driver_dt_match,
    },
  

}; diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c index afc83c9..130b109 100644 --- a/drivers/gpu/drm/exynos/exynos_hdmi.c +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c @@ -28,7 +28,6 @@ #include <linux/interrupt.h> #include <linux/irq.h> #include <linux/delay.h> -#include <linux/pm_runtime.h> #include <linux/clk.h> #include <linux/regulator/consumer.h> #include <linux/io.h> @@ -1771,7 +1770,6 @@ static void hdmi_poweroff(struct exynos_drm_display *display) regulator_bulk_disable(res->regul_count, res->regul_bulk);

    mutex_lock(&hdata->hdmi_mutex);

•   hdata->powered = false;
  
  

out: @@ -1780,20 +1778,16 @@ out:

static void hdmi_dpms(struct exynos_drm_display *display, int mode) {

•   struct hdmi_context *hdata = display->ctx;
  

•   DRM_DEBUG_KMS("mode %d\n", mode);
  
    switch (mode) {
    case DRM_MODE_DPMS_ON:
  

•           if (pm_runtime_suspended(hdata->dev))
  

•                   pm_runtime_get_sync(hdata->dev);
  

•           hdmi_poweron(display);
            break;
    case DRM_MODE_DPMS_STANDBY:
    case DRM_MODE_DPMS_SUSPEND:
    case DRM_MODE_DPMS_OFF:
  

•           if (!pm_runtime_suspended(hdata->dev))
  

•                   pm_runtime_put_sync(hdata->dev);
  

•           hdmi_poweroff(display);
            break;
    default:
            DRM_DEBUG_KMS("unknown dpms mode: %d\n", mode);
  

@@ -2036,8 +2030,6 @@ static int hdmi_probe(struct platform_device *pdev) hdmi_display.ctx = hdata; exynos_drm_display_register(&hdmi_display);

•   pm_runtime_enable(dev);
  

•   return 0;
  
  

err_hdmiphy: @@ -2053,88 +2045,18 @@ static int hdmi_remove(struct platform_device *pdev) struct exynos_drm_display *display = get_hdmi_display(dev); struct hdmi_context *hdata = display->ctx;

•   pm_runtime_disable(dev);
  

•   put_device(&hdata->hdmiphy_port->dev);
    put_device(&hdata->ddc_port->dev);
  
    return 0;
  

}

-#ifdef CONFIG_PM_SLEEP -static int hdmi_suspend(struct device *dev) -{

•   struct exynos_drm_display *display = get_hdmi_display(dev);
  

•   struct hdmi_context *hdata = display->ctx;
  

•   disable_irq(hdata->irq);
  

•   hdata->hpd = false;
  

•   if (hdata->drm_dev)
  

•           drm_helper_hpd_irq_event(hdata->drm_dev);
  

•   if (pm_runtime_suspended(dev)) {
  

•           DRM_DEBUG_KMS("Already suspended\n");
  

•           return 0;
  

•   }
  

•   hdmi_poweroff(display);
  

•   return 0;
  

-}

-static int hdmi_resume(struct device *dev) -{

•   struct exynos_drm_display *display = get_hdmi_display(dev);
  

•   struct hdmi_context *hdata = display->ctx;
  

•   hdata->hpd = gpio_get_value(hdata->hpd_gpio);
  

•   enable_irq(hdata->irq);
  

•   if (!pm_runtime_suspended(dev)) {
  

•           DRM_DEBUG_KMS("Already resumed\n");
  

•           return 0;
  

•   }
  

•   hdmi_poweron(display);
  

•   return 0;
  

-} -#endif

-#ifdef CONFIG_PM_RUNTIME -static int hdmi_runtime_suspend(struct device *dev) -{

•   struct exynos_drm_display *display = get_hdmi_display(dev);
  

•   hdmi_poweroff(display);
  

•   return 0;
  

-}

-static int hdmi_runtime_resume(struct device *dev) -{

•   struct exynos_drm_display *display = get_hdmi_display(dev);
  

•   hdmi_poweron(display);
  

•   return 0;
  

-} -#endif

-static const struct dev_pm_ops hdmi_pm_ops = {

•   SET_SYSTEM_SLEEP_PM_OPS(hdmi_suspend, hdmi_resume)
  

•   SET_RUNTIME_PM_OPS(hdmi_runtime_suspend, hdmi_runtime_resume, NULL)
  

-};

struct platform_driver hdmi_driver = { .probe = hdmi_probe, .remove = hdmi_remove, .driver = { .name = "exynos-hdmi", .owner = THIS_MODULE,

•           .pm     = &hdmi_pm_ops,
            .of_match_table = hdmi_match_types,
    },
  

}; diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c index 39aed3e..985391d 100644 --- a/drivers/gpu/drm/exynos/exynos_mixer.c +++ b/drivers/gpu/drm/exynos/exynos_mixer.c @@ -902,6 +902,8 @@ static void mixer_poweron(struct exynos_drm_manager *mgr) ctx->powered = true; mutex_unlock(&ctx->mixer_mutex);

•   pm_runtime_get_sync(ctx->dev);
  

•   clk_prepare_enable(res->mixer);
    if (ctx->vp_enabled) {
            clk_prepare_enable(res->vp);
  

@@ -934,6 +936,8 @@ static void mixer_poweroff(struct exynos_drm_manager *mgr) clk_disable_unprepare(res->sclk_mixer); }

•   pm_runtime_put_sync(ctx->dev);
  

•   mutex_lock(&ctx->mixer_mutex);
    ctx->powered = false;
  
  

@@ -943,18 +947,14 @@ out:

static void mixer_dpms(struct exynos_drm_manager *mgr, int mode) {

•   struct mixer_context *mixer_ctx = mgr->ctx;
  

•   switch (mode) {
    case DRM_MODE_DPMS_ON:
  

•           if (pm_runtime_suspended(mixer_ctx->dev))
  

•                   pm_runtime_get_sync(mixer_ctx->dev);
  

•           mixer_poweron(mgr);
            break;
    case DRM_MODE_DPMS_STANDBY:
    case DRM_MODE_DPMS_SUSPEND:
    case DRM_MODE_DPMS_OFF:
  

•           if (!pm_runtime_suspended(mixer_ctx->dev))
  

•                   pm_runtime_put_sync(mixer_ctx->dev);
  

•           mixer_poweroff(mgr);
            break;
    default:
            DRM_DEBUG_KMS("unknown dpms mode: %d\n", mode);
  

@@ -1239,6 +1239,13 @@ static int mixer_probe(struct platform_device *pdev) platform_set_drvdata(pdev, &mixer_manager); exynos_drm_manager_register(&mixer_manager);

•   /*
  

•    * We need to runtime pm to enable/disable sysmmu since it is a child of
  

•    * this driver. Ideally, this would hang off the drm driver's runtime
  

•    * operations, but we're not quite there yet.
  

•    *
  

•    * Tracked in crbug.com/264312
  

•    */
    pm_runtime_enable(dev);
  
    return 0;
  

@@ -1258,66 +1265,10 @@ static int mixer_remove(struct platform_device *pdev) return 0; }

-#ifdef CONFIG_PM_SLEEP -static int mixer_suspend(struct device *dev) -{

•   struct exynos_drm_manager *mgr = get_mixer_manager(dev);
  

•   if (pm_runtime_suspended(dev)) {
  

•           DRM_DEBUG_KMS("Already suspended\n");
  

•           return 0;
  

•   }
  

•   mixer_poweroff(mgr);
  

•   return 0;
  

-}

-static int mixer_resume(struct device *dev) -{

•   struct exynos_drm_manager *mgr = get_mixer_manager(dev);
  

•   if (!pm_runtime_suspended(dev)) {
  

•           DRM_DEBUG_KMS("Already resumed\n");
  

•           return 0;
  

•   }
  

•   mixer_poweron(mgr);
  

•   return 0;
  

-} -#endif

-#ifdef CONFIG_PM_RUNTIME -static int mixer_runtime_suspend(struct device *dev) -{

•   struct exynos_drm_manager *mgr = get_mixer_manager(dev);
  

•   mixer_poweroff(mgr);
  

•   return 0;
  

-}

-static int mixer_runtime_resume(struct device *dev) -{

•   struct exynos_drm_manager *mgr = get_mixer_manager(dev);
  

•   mixer_poweron(mgr);
  

•   return 0;
  

-} -#endif

-static const struct dev_pm_ops mixer_pm_ops = {

•   SET_SYSTEM_SLEEP_PM_OPS(mixer_suspend, mixer_resume)
  

•   SET_RUNTIME_PM_OPS(mixer_runtime_suspend, mixer_runtime_resume, NULL)
  

-};

struct platform_driver mixer_driver = { .driver = { .name = "exynos-mixer", .owner = THIS_MODULE,

•           .pm = &mixer_pm_ops,
            .of_match_table = mixer_match_types,
    },
    .probe = mixer_probe,
  

-- 1.8.4