Date:	Sat, 4 Jun 2016 01:13:15 +0200
From:	Daniel Vetter <daniel@...ll.ch>
To:	Stefan Agner <stefan@...er.ch>
Cc:	thierry.reding@...il.com, airlied@...ux.ie,
	dri-devel@...ts.freedesktop.org, jianwei.wang.chn@...il.com,
	meng.yi@....com, alexander.stein@...tec-electronic.com,
	linux-kernel@...r.kernel.org, alison.wang@...escale.com
Subject: Re: [PATCH v2 5/6] drm/fsl-dcu: implement suspend/resume using
 atomic helpers

On Fri, Jun 03, 2016 at 03:56:43PM -0700, Stefan Agner wrote:
> Use the drm_atomic_helper_suspend() and drm_atomic_helper_resume()
> helpers to implement subsystem-level suspend/resume. This replaces
> the (non-functional) regmap cache based suspend resume functionality.
> 
> Signed-off-by: Stefan Agner <stefan@...er.ch>
> ---
>  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c | 40 ++++++++++++++++++++++++++-----
>  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h |  1 +
>  2 files changed, 35 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
> index 06a4d01..2bc4fa2 100644
> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
> @@ -11,6 +11,7 @@
>  
>  #include <linux/clk.h>
>  #include <linux/clk-provider.h>
> +#include <linux/console.h>
>  #include <linux/io.h>
>  #include <linux/mfd/syscon.h>
>  #include <linux/mm.h>
> @@ -22,6 +23,7 @@
>  #include <linux/regmap.h>
>  
>  #include <drm/drmP.h>
> +#include <drm/drm_atomic_helper.h>
>  #include <drm/drm_crtc_helper.h>
>  #include <drm/drm_fb_cma_helper.h>
>  #include <drm/drm_gem_cma_helper.h>
> @@ -42,10 +44,8 @@ static const struct regmap_config fsl_dcu_regmap_config = {
>  	.reg_bits = 32,
>  	.reg_stride = 4,
>  	.val_bits = 32,
> -	.cache_type = REGCACHE_FLAT,
>  
>  	.volatile_reg = fsl_dcu_drm_is_volatile_reg,
> -	.max_register = 0x11fc,
>  };
>  
>  static int fsl_dcu_drm_irq_init(struct drm_device *dev)
> @@ -229,9 +229,25 @@ static int fsl_dcu_drm_pm_suspend(struct device *dev)
>  	if (!fsl_dev)
>  		return 0;
>  
> +	disable_irq(fsl_dev->irq);
>  	drm_kms_helper_poll_disable(fsl_dev->drm);
> -	regcache_cache_only(fsl_dev->regmap, true);
> -	regcache_mark_dirty(fsl_dev->regmap);
> +
> +	console_lock();
> +	drm_fbdev_cma_set_suspend(fsl_dev->fbdev, 1);
> +	console_unlock();

Hm, it would be nice to push that console_lock into the helper itself.
Plus maybe reuse the trick i915 uses to put the resume side into a work
item to avoid taking console_lock in a blocking way.

The reason for that is that in a normal suspend/resume operation
console_lock is highly contended due to all the prinkt noise flying
around. If someone disabled fbdev emulation (either in Kconfig or at
runtime) we really, really want to avoid taking it. And without fbdev
there's no need to call fb_set_suspend at all.

Signed up for that little bit of polish? Imo totally ok as a follow-up,
but would be real nice to have.
-Daniel

> +
> +	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;
> @@ -251,9 +267,21 @@ static int fsl_dcu_drm_pm_resume(struct device *dev)
>  		return ret;
>  	}
>  
> +	ret = clk_prepare_enable(fsl_dev->pix_clk);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to enable pix clk\n");
> +		return ret;
> +	}
> +
> +	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);
> -	regcache_cache_only(fsl_dev->regmap, false);
> -	regcache_sync(fsl_dev->regmap);
> +	enable_irq(fsl_dev->irq);
>  
>  	return 0;
>  }
> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
> index b1bba3a..3b371fe7 100644
> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
> @@ -194,6 +194,7 @@ struct fsl_dcu_drm_device {
>  	struct drm_encoder encoder;
>  	struct fsl_dcu_drm_connector connector;
>  	const struct fsl_dcu_soc_data *soc;
> +	struct drm_atomic_state *state;
>  };
>  
>  void fsl_dcu_fbdev_init(struct drm_device *dev);
> -- 
> 2.8.2
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@...ts.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Powered by blists - more mailing lists