Re: [PATCH v2 2/2] drm/fsl-dcu: use flat regmap cache

[Stefan Agner]

On 2016-02-02 17:06, Stefan Agner wrote:
> Using flat regmap cache instead of RB-tree to avoid the following
> lockdep warning on driver load:
> [    0.697285] WARNING: CPU: 0 PID: 1 at kernel/locking/lockdep.c:2755
> lockdep_trace_alloc+0x15c/0x160()
> [    0.697449] DEBUG_LOCKS_WARN_ON(irqs_disabled_flags(flags))
> 
> The RB-tree regmap cache needs to allocate new space on first
> writes. However, allocations in an atomic context (e.g. when a
> spinlock is held) are not allowed. The function regmap_write
> calls map->lock, which acquires a spinlock in the fast_io case.
> Since the FSL DCU driver uses MMIO, the regmap bus of type
> regmap_mmio is being used which has fast_io set to true.
> 
> The MMIO space of the DCU driver is reasonable condense, hence
> using the much faster flat regmap cache is anyway the better
> choice.

As discussed with Thierry on IRC, using regmap cache often does not
really make sense in the context of display controllers. While trying to
use the suspend/resume, I realized that the current implementation in
the DCU driver has bugs and does not work (e.g. an explicit READREG is
missing, but even with that in place, restoring all registers and enable
the controller then seem not to work reliable). Using the new
suspend/resume helpers seem to work better...

Therefor I NACK my own patch here and vote for removing the regcache
entirely (and use the new DRM supsend/resume helpers to implement
suspend/resume)...

--
Stefan 

> 
> Signed-off-by: Stefan Agner <stefan@xxxxxxxx>
> Cc: Mark Brown <broonie@xxxxxxxxxx>
> ---
> Changes since v1:
> - Do not move drm_dev_alloc
> 
>  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c | 18 +++++++++++-------
>  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h |  6 ++++--
>  2 files changed, 15 insertions(+), 9 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 e01c813..9c21aad 100644
> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
> @@ -36,11 +36,11 @@ static bool fsl_dcu_drm_is_volatile_reg(struct
> device *dev, unsigned int reg)
>  	return false;
>  }
>  
> -static const struct regmap_config fsl_dcu_regmap_config = {
> +static struct regmap_config fsl_dcu_regmap_config = {
>  	.reg_bits = 32,
>  	.reg_stride = 4,
>  	.val_bits = 32,
> -	.cache_type = REGCACHE_RBTREE,
> +	.cache_type = REGCACHE_FLAT,
>  
>  	.volatile_reg = fsl_dcu_drm_is_volatile_reg,
>  };
> @@ -260,12 +260,14 @@ static const struct fsl_dcu_soc_data
> fsl_dcu_ls1021a_data = {
>  	.name = "ls1021a",
>  	.total_layer = 16,
>  	.max_layer = 4,
> +	.max_register = LS1021A_DCU_MAX_REGISTER,
>  };
>  
>  static const struct fsl_dcu_soc_data fsl_dcu_vf610_data = {
>  	.name = "vf610",
>  	.total_layer = 64,
>  	.max_layer = 6,
> +	.max_register = VF610_DCU_MAX_REGISTER,
>  };
>  
>  static const struct of_device_id fsl_dcu_of_match[] = {
> @@ -331,6 +333,13 @@ static int fsl_dcu_drm_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	id = of_match_node(fsl_dcu_of_match, pdev->dev.of_node);
> +	if (!id)
> +		return -ENODEV;
> +
> +	fsl_dev->soc = id->data;
> +
> +	fsl_dcu_regmap_config.max_register = fsl_dev->soc->max_register;
>  	fsl_dev->regmap = devm_regmap_init_mmio(dev, base,
>  			&fsl_dcu_regmap_config);
>  	if (IS_ERR(fsl_dev->regmap)) {
> @@ -338,11 +347,6 @@ static int fsl_dcu_drm_probe(struct platform_device *pdev)
>  		return PTR_ERR(fsl_dev->regmap);
>  	}
>  
> -	id = of_match_node(fsl_dcu_of_match, pdev->dev.of_node);
> -	if (!id)
> -		return -ENODEV;
> -	fsl_dev->soc = id->data;
> -
>  	drm = drm_dev_alloc(driver, dev);
>  	if (!drm)
>  		return -ENOMEM;
> 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 2a724f3..7c296a0 100644
> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
> @@ -114,8 +114,6 @@
>  #define DCU_UPDATE_MODE_MODE		BIT(31)
>  #define DCU_UPDATE_MODE_READREG		BIT(30)
>  
> -#define DCU_DCFB_MAX			0x300
> -
>  #define DCU_CTRLDESCLN(layer, reg)	(0x200 + (reg - 1) * 4 + (layer) * 0x40)
>  
>  #define DCU_LAYER_HEIGHT(x)		((x) << 16)
> @@ -155,6 +153,9 @@
>  #define DCU_LAYER_POST_SKIP(x)		((x) << 16)
>  #define DCU_LAYER_PRE_SKIP(x)		(x)
>  
> +#define VF610_DCU_MAX_REGISTER		0x11fc
> +#define LS1021A_DCU_MAX_REGISTER	0x5fc
> +
>  #define FSL_DCU_RGB565			4
>  #define FSL_DCU_RGB888			5
>  #define FSL_DCU_ARGB8888		6
> @@ -175,6 +176,7 @@ struct fsl_dcu_soc_data {
>  	unsigned int total_layer;
>  	/*max layer number DCU supported*/
>  	unsigned int max_layer;
> +	unsigned int max_register;
>  };
>  
>  struct fsl_dcu_drm_device {