Re: [PATCH] drm/atmel-hlcdc: Fix suspend/resume implementation

[Boris Brezillon]

On Wed,  1 Mar 2017 13:31:01 +0100
Boris Brezillon  wrote:

> The current suspend resume implementation is assuming register values are
> kept when entering suspend, which is no longer the case with the
> suspend-to-RAM on the sama5d2.
> 
> While at it, switch to the generic infrastructure to enter suspend mode
> (drm_atomic_helper_suspend/resume()).
> 

Applied to drm-misc-next.

> Signed-off-by: Boris Brezillon 
> ---
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 33 
> --
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c   | 31 
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h   |  8 ---
>  3 files changed, 21 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c 
> b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> index 6b50fb706c0e..53bfa56ca47a 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> @@ -55,14 +55,12 @@ drm_crtc_state_to_atmel_hlcdc_crtc_state(struct 
> drm_crtc_state *state)
>   * @hlcdc: pointer to the atmel_hlcdc structure provided by the MFD device
>   * @event: pointer to the current page flip event
>   * @id: CRTC id (returned by drm_crtc_index)
> - * @enabled: CRTC state
>   */
>  struct atmel_hlcdc_crtc {
>   struct drm_crtc base;
>   struct atmel_hlcdc_dc *dc;
>   struct drm_pending_vblank_event *event;
>   int id;
> - bool enabled;
>  };
>  
>  static inline struct atmel_hlcdc_crtc *
> @@ -158,9 +156,6 @@ static void atmel_hlcdc_crtc_disable(struct drm_crtc *c)
>   struct regmap *regmap = crtc->dc->hlcdc->regmap;
>   unsigned int status;
>  
> - if (!crtc->enabled)
> - return;
> -
>   drm_crtc_vblank_off(c);
>  
>   pm_runtime_get_sync(dev->dev);
> @@ -186,8 +181,6 @@ static void atmel_hlcdc_crtc_disable(struct drm_crtc *c)
>   pm_runtime_allow(dev->dev);
>  
>   pm_runtime_put_sync(dev->dev);
> -
> - crtc->enabled = false;
>  }
>  
>  static void atmel_hlcdc_crtc_enable(struct drm_crtc *c)
> @@ -197,9 +190,6 @@ static void atmel_hlcdc_crtc_enable(struct drm_crtc *c)
>   struct regmap *regmap = crtc->dc->hlcdc->regmap;
>   unsigned int status;
>  
> - if (crtc->enabled)
> - return;
> -
>   pm_runtime_get_sync(dev->dev);
>  
>   pm_runtime_forbid(dev->dev);
> @@ -226,29 +216,6 @@ static void atmel_hlcdc_crtc_enable(struct drm_crtc *c)
>   pm_runtime_put_sync(dev->dev);
>  
>   drm_crtc_vblank_on(c);
> -
> - crtc->enabled = true;
> -}
> -
> -void atmel_hlcdc_crtc_suspend(struct drm_crtc *c)
> -{
> - struct atmel_hlcdc_crtc *crtc = drm_crtc_to_atmel_hlcdc_crtc(c);
> -
> - if (crtc->enabled) {
> - atmel_hlcdc_crtc_disable(c);
> - /* save enable state for resume */
> - crtc->enabled = true;
> - }
> -}
> -
> -void atmel_hlcdc_crtc_resume(struct drm_crtc *c)
> -{
> - struct atmel_hlcdc_crtc *crtc = drm_crtc_to_atmel_hlcdc_crtc(c);
> -
> - if (crtc->enabled) {
> - crtc->enabled = false;
> - atmel_hlcdc_crtc_enable(c);
> - }
>  }
>  
>  #define ATMEL_HLCDC_RGB444_OUTPUTBIT(0)
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c 
> b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> index 970bd87d7cce..0fdb6ad0469f 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> @@ -810,31 +810,32 @@ static int atmel_hlcdc_dc_drm_remove(struct 
> platform_device *pdev)
>  static int atmel_hlcdc_dc_drm_suspend(struct device *dev)
>  {
>   struct drm_device *drm_dev = dev_get_drvdata(dev);
> - struct drm_crtc *crtc;
> + 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);
>  
> - if (pm_runtime_suspended(dev))
> - return 0;
> + dc->suspend.state = state;
> +
> + regmap_read(regmap, ATMEL_HLCDC_IMR, >suspend.imr);
> + regmap_write(regmap, ATMEL_HLCDC_IDR, dc->suspend.imr);
> + clk_disable_unprepare(dc->hlcdc->periph_clk);
>  
> - drm_modeset_lock_all(drm_dev);
> - list_for_each_entry(crtc, _dev->mode_config.crtc_list, head)
> - atmel_hlcdc_crtc_suspend(crtc);
> - drm_modeset_unlock_all(drm_dev);
>   return 0;
>  }
>  
>  static int atmel_hlcdc_dc_drm_resume(struct device *dev)
>  {
>   struct drm_device *drm_dev = dev_get_drvdata(dev);
> - struct drm_crtc *crtc;
> + struct atmel_hlcdc_dc *dc = drm_dev->dev_private;
>  
> - if (pm_runtime_suspended(dev))
> - return 0;
> + clk_prepare_enable(dc->hlcdc->periph_clk);
> + regmap_write(dc->hlcdc->regmap, ATMEL_HLCDC_IER, dc->suspend.imr);
>  
> - 

Re: [PATCH] drm/atmel-hlcdc: Fix suspend/resume implementation

[Sylvain Rochet]

Hi,

On Wed, Mar 01, 2017 at 01:31:01PM +0100, Boris Brezillon wrote:
> The current suspend resume implementation is assuming register values are
> kept when entering suspend, which is no longer the case with the
> suspend-to-RAM on the sama5d2.
> 
> While at it, switch to the generic infrastructure to enter suspend mode
> (drm_atomic_helper_suspend/resume()).
> 
> Signed-off-by: Boris Brezillon 
> ---

No regression on sama5d3, atmel_hlcdc_crtc_disable/enable are still 
called at suspend/resume and pinctrl_pm_select_sleep_state which is just 
for my unusual screen (no display pin, so I need to clamp all pads) is 
still called, thus it works for me.

Tested-by: Sylvain Rochet 

Cheers,
Sylvain
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/atmel-hlcdc: Fix suspend/resume implementation

[Daniel Vetter]

On Wed, Mar 01, 2017 at 01:31:01PM +0100, Boris Brezillon wrote:
> The current suspend resume implementation is assuming register values are
> kept when entering suspend, which is no longer the case with the
> suspend-to-RAM on the sama5d2.
> 
> While at it, switch to the generic infrastructure to enter suspend mode
> (drm_atomic_helper_suspend/resume()).
> 
> Signed-off-by: Boris Brezillon 
> ---
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 33 
> --
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c   | 31 
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h   |  8 ---
>  3 files changed, 21 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c 
> b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> index 6b50fb706c0e..53bfa56ca47a 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> @@ -55,14 +55,12 @@ drm_crtc_state_to_atmel_hlcdc_crtc_state(struct 
> drm_crtc_state *state)
>   * @hlcdc: pointer to the atmel_hlcdc structure provided by the MFD device
>   * @event: pointer to the current page flip event
>   * @id: CRTC id (returned by drm_crtc_index)
> - * @enabled: CRTC state
>   */
>  struct atmel_hlcdc_crtc {
>   struct drm_crtc base;
>   struct atmel_hlcdc_dc *dc;
>   struct drm_pending_vblank_event *event;
>   int id;
> - bool enabled;
>  };
>  
>  static inline struct atmel_hlcdc_crtc *
> @@ -158,9 +156,6 @@ static void atmel_hlcdc_crtc_disable(struct drm_crtc *c)
>   struct regmap *regmap = crtc->dc->hlcdc->regmap;
>   unsigned int status;
>  
> - if (!crtc->enabled)
> - return;

Removing bail-out code like this and switching to atomic s/r helpers is
very much how this (= atomic helpers) is supposed to work. Yay for another
driver following the common code pattern.

Acked-by: Daniel Vetter 

> -
>   drm_crtc_vblank_off(c);
>  
>   pm_runtime_get_sync(dev->dev);
> @@ -186,8 +181,6 @@ static void atmel_hlcdc_crtc_disable(struct drm_crtc *c)
>   pm_runtime_allow(dev->dev);
>  
>   pm_runtime_put_sync(dev->dev);
> -
> - crtc->enabled = false;
>  }
>  
>  static void atmel_hlcdc_crtc_enable(struct drm_crtc *c)
> @@ -197,9 +190,6 @@ static void atmel_hlcdc_crtc_enable(struct drm_crtc *c)
>   struct regmap *regmap = crtc->dc->hlcdc->regmap;
>   unsigned int status;
>  
> - if (crtc->enabled)
> - return;
> -
>   pm_runtime_get_sync(dev->dev);
>  
>   pm_runtime_forbid(dev->dev);
> @@ -226,29 +216,6 @@ static void atmel_hlcdc_crtc_enable(struct drm_crtc *c)
>   pm_runtime_put_sync(dev->dev);
>  
>   drm_crtc_vblank_on(c);
> -
> - crtc->enabled = true;
> -}
> -
> -void atmel_hlcdc_crtc_suspend(struct drm_crtc *c)
> -{
> - struct atmel_hlcdc_crtc *crtc = drm_crtc_to_atmel_hlcdc_crtc(c);
> -
> - if (crtc->enabled) {
> - atmel_hlcdc_crtc_disable(c);
> - /* save enable state for resume */
> - crtc->enabled = true;
> - }
> -}
> -
> -void atmel_hlcdc_crtc_resume(struct drm_crtc *c)
> -{
> - struct atmel_hlcdc_crtc *crtc = drm_crtc_to_atmel_hlcdc_crtc(c);
> -
> - if (crtc->enabled) {
> - crtc->enabled = false;
> - atmel_hlcdc_crtc_enable(c);
> - }
>  }
>  
>  #define ATMEL_HLCDC_RGB444_OUTPUTBIT(0)
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c 
> b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> index 970bd87d7cce..0fdb6ad0469f 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> @@ -810,31 +810,32 @@ static int atmel_hlcdc_dc_drm_remove(struct 
> platform_device *pdev)
>  static int atmel_hlcdc_dc_drm_suspend(struct device *dev)
>  {
>   struct drm_device *drm_dev = dev_get_drvdata(dev);
> - struct drm_crtc *crtc;
> + 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);
>  
> - if (pm_runtime_suspended(dev))
> - return 0;
> + dc->suspend.state = state;
> +
> + regmap_read(regmap, ATMEL_HLCDC_IMR, >suspend.imr);
> + regmap_write(regmap, ATMEL_HLCDC_IDR, dc->suspend.imr);
> + clk_disable_unprepare(dc->hlcdc->periph_clk);
>  
> - drm_modeset_lock_all(drm_dev);
> - list_for_each_entry(crtc, _dev->mode_config.crtc_list, head)
> - atmel_hlcdc_crtc_suspend(crtc);
> - drm_modeset_unlock_all(drm_dev);
>   return 0;
>  }
>  
>  static int atmel_hlcdc_dc_drm_resume(struct device *dev)
>  {
>   struct drm_device *drm_dev = dev_get_drvdata(dev);
> - struct drm_crtc *crtc;
> + struct atmel_hlcdc_dc *dc = drm_dev->dev_private;
>  
> - if