[PATCH v2 1/2] drm/fsl-dcu: fix register initialization

[PATCH v2 1/2] drm/fsl-dcu: fix register initialization

[Stefan Agner]

The layer enumeration start with 0 (0-15 for LS1021a and 0-63 for
Vybrid) whereas the register enumeration start from 1 (1-10 for
LS1021a and 1-9 for Vybrid). The loop started off from 0 for both
iterations and initialized the number of layers inclusive, which
is one layer too many.

All extensively written registers seem to be unassigned, it seems
that the write to those registers did not do any harm in practice.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
This two patches apply cleanly on top of my earlier DCU fixes                    
patchset:                                                                        
https://lkml.org/lkml/2015/11/18/953

Changes since v1:
- (none)

 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c
index db69725..3c92889 100644
--- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c
+++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c
@@ -169,8 +169,8 @@ int fsl_dcu_drm_crtc_create(struct fsl_dcu_drm_device *fsl_dev)
 		reg_num = LS1021A_LAYER_REG_NUM;
 	else
 		reg_num = VF610_LAYER_REG_NUM;
-	for (i = 0; i <= fsl_dev->soc->total_layer; i++) {
-		for (j = 0; j < reg_num; j++)
+	for (i = 0; i < fsl_dev->soc->total_layer; i++) {
+		for (j = 1; j <= reg_num; j++)
 			regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN(i, j), 0);
 	}
 	regmap_update_bits(fsl_dev->regmap, DCU_DCU_MODE,
-- 
2.7.0

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

[Stefan Agner]

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.

Signed-off-by: Stefan Agner <stefan@agner.ch>
Cc: Mark Brown <broonie@kernel.org>
---
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 {
-- 
2.7.0

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@agner.ch>
> Cc: Mark Brown <broonie@kernel.org>
> ---
> 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 {

Re: [PATCH v2 1/2] drm/fsl-dcu: fix register initialization

[Stefan Agner]

On 2016-02-02 17:06, Stefan Agner wrote:
> The layer enumeration start with 0 (0-15 for LS1021a and 0-63 for
> Vybrid) whereas the register enumeration start from 1 (1-10 for
> LS1021a and 1-9 for Vybrid). The loop started off from 0 for both
> iterations and initialized the number of layers inclusive, which
> is one layer too many.
> 
> All extensively written registers seem to be unassigned, it seems
> that the write to those registers did not do any harm in practice.
> 
> Signed-off-by: Stefan Agner <stefan@agner.ch>

Applied.

> ---
> This two patches apply cleanly on top of my earlier DCU fixes         
>
> patchset:                                                             
>
> https://lkml.org/lkml/2015/11/18/953
> 
> Changes since v1:
> - (none)
> 
>  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c
> b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c
> index db69725..3c92889 100644
> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c
> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c
> @@ -169,8 +169,8 @@ int fsl_dcu_drm_crtc_create(struct
> fsl_dcu_drm_device *fsl_dev)
>  		reg_num = LS1021A_LAYER_REG_NUM;
>  	else
>  		reg_num = VF610_LAYER_REG_NUM;
> -	for (i = 0; i <= fsl_dev->soc->total_layer; i++) {
> -		for (j = 0; j < reg_num; j++)
> +	for (i = 0; i < fsl_dev->soc->total_layer; i++) {
> +		for (j = 1; j <= reg_num; j++)
>  			regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN(i, j), 0);
>  	}
>  	regmap_update_bits(fsl_dev->regmap, DCU_DCU_MODE,