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

[Inki Dae <inki.dae@samsung.com>]

> -----Original Message-----
> From: Sean Paul [mailto:seanpaul@chromium.org]
> Sent: Thursday, October 17, 2013 5:40 AM
> To: dri-devel; InKi Dae
> Cc: Dave Airlie; Tomasz Figa; Stéphane Marchesin; Sean Paul
> Subject: 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?
> 


I can do it.

Thanks,
Inki Dae

P.s. we didn't really think about this way. Actually, DRM driver contains
all device drivers; fimd, hdmi, g2d, and so on. So it's good idea that core
driver handles PM, and other sub drivers should take care of their own power
domain and clock according to dpms request from core driver. And with this
way, it seems that we could resolve power ordering issue when sleep ->
wakeup


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