[RESEND 2/3] drm :fsl-dcu: Cleanup vblank interrupt mask and status setting code

[RESEND 2/3] drm :fsl-dcu: Cleanup vblank interrupt mask and status setting code

[Dongsheng Wang]

From: Jianwei Wang <jianwei.wang.chn@gmail.com>

Switch update interrupt mask bit with regmap_update_bits, and clear
interrupt status by writing 1 to relevant bit before setting mask in
fsl_dcu_drm_irq_init function.

Signed-off-by: Jianwei Wang <jianwei.wang.chn@gmail.com>
Signed-off-by: Yi Meng <b56799@freescale.com>
Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com>

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 1930234..5c29ff7 100644
--- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
+++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
@@ -38,21 +38,17 @@ static const struct regmap_config fsl_dcu_regmap_config = {
 static int fsl_dcu_drm_irq_init(struct drm_device *dev)
 {
 	struct fsl_dcu_drm_device *fsl_dev = dev->dev_private;
-	unsigned int value;
 	int ret;
 
 	ret = drm_irq_install(dev, fsl_dev->irq);
 	if (ret < 0)
 		dev_err(dev->dev, "failed to install IRQ handler\n");
 
-	ret = regmap_write(fsl_dev->regmap, DCU_INT_STATUS, 0);
+	ret = regmap_write(fsl_dev->regmap, DCU_INT_STATUS, 0xffffffff);
 	if (ret)
 		dev_err(dev->dev, "set DCU_INT_STATUS failed\n");
-	ret = regmap_read(fsl_dev->regmap, DCU_INT_MASK, &value);
-	if (ret)
-		dev_err(dev->dev, "read DCU_INT_MASK failed\n");
-	value &= DCU_INT_MASK_VBLANK;
-	ret = regmap_write(fsl_dev->regmap, DCU_INT_MASK, value);
+	ret = regmap_update_bits(fsl_dev->regmap, DCU_INT_MASK,
+				 DCU_INT_MASK_VBLANK, ~DCU_INT_MASK_VBLANK);
 	if (ret)
 		dev_err(dev->dev, "set DCU_INT_MASK failed\n");
 	ret = regmap_write(fsl_dev->regmap, DCU_UPDATE_MODE,
@@ -143,14 +139,10 @@ static irqreturn_t fsl_dcu_drm_irq(int irq, void *arg)
 static int fsl_dcu_drm_enable_vblank(struct drm_device *dev, unsigned int pipe)
 {
 	struct fsl_dcu_drm_device *fsl_dev = dev->dev_private;
-	unsigned int value;
 	int ret;
 
-	ret = regmap_read(fsl_dev->regmap, DCU_INT_MASK, &value);
-	if (ret)
-		dev_err(dev->dev, "read DCU_INT_MASK failed\n");
-	value &= ~DCU_INT_MASK_VBLANK;
-	ret = regmap_write(fsl_dev->regmap, DCU_INT_MASK, value);
+	ret = regmap_update_bits(fsl_dev->regmap, DCU_INT_MASK,
+				 DCU_INT_MASK_VBLANK, ~DCU_INT_MASK_VBLANK);
 	if (ret)
 		dev_err(dev->dev, "set DCU_INT_MASK failed\n");
 	return 0;
@@ -160,14 +152,10 @@ static void fsl_dcu_drm_disable_vblank(struct drm_device *dev,
 				       unsigned int pipe)
 {
 	struct fsl_dcu_drm_device *fsl_dev = dev->dev_private;
-	unsigned int value;
 	int ret;
 
-	ret = regmap_read(fsl_dev->regmap, DCU_INT_MASK, &value);
-	if (ret)
-		dev_err(dev->dev, "read DCU_INT_MASK failed\n");
-	value |= DCU_INT_MASK_VBLANK;
-	ret = regmap_write(fsl_dev->regmap, DCU_INT_MASK, value);
+	ret = regmap_update_bits(fsl_dev->regmap, DCU_INT_MASK,
+				 DCU_INT_MASK_VBLANK, DCU_INT_MASK_VBLANK);
 	if (ret)
 		dev_err(dev->dev, "set DCU_INT_MASK failed\n");
 }
-- 
2.1.0.27.g96db324

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[RESEND 3/3] drm :fsl-dcu: Add multi layers support

[Dongsheng Wang]

From: Jianwei Wang <jianwei.wang.chn@gmail.com>

For DCU support atmost 16 layers(on ls1021a) or 64 layers(on vf610),
add (total_layer - 1) overlay planes.

Signed-off-by: Jianwei Wang <jianwei.wang.chn@gmail.com>
Signed-off-by: Yi Meng <b56799@freescale.com>
Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com>

diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c
index a8932a8..7ed1a7e 100644
--- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c
+++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c
@@ -235,7 +235,10 @@ static const u32 fsl_dcu_drm_plane_formats[] = {
 
 struct drm_plane *fsl_dcu_drm_primary_create_plane(struct drm_device *dev)
 {
+	struct fsl_dcu_drm_device *fsl_dev = dev->dev_private;
 	struct drm_plane *primary;
+	struct drm_plane *overlay;
+	unsigned int total_layer, formats_size, i;
 	int ret;
 
 	primary = kzalloc(sizeof(*primary), GFP_KERNEL);
@@ -244,11 +247,12 @@ struct drm_plane *fsl_dcu_drm_primary_create_plane(struct drm_device *dev)
 		return NULL;
 	}
 
+	formats_size = ARRAY_SIZE(fsl_dcu_drm_plane_formats);
 	/* possible_crtc's will be filled in later by crtc_init */
 	ret = drm_universal_plane_init(dev, primary, 0,
 				       &fsl_dcu_drm_plane_funcs,
 				       fsl_dcu_drm_plane_formats,
-				       ARRAY_SIZE(fsl_dcu_drm_plane_formats),
+				       formats_size,
 				       DRM_PLANE_TYPE_PRIMARY);
 	if (ret) {
 		kfree(primary);
@@ -256,5 +260,26 @@ struct drm_plane *fsl_dcu_drm_primary_create_plane(struct drm_device *dev)
 	}
 	drm_plane_helper_add(primary, &fsl_dcu_drm_plane_helper_funcs);
 
+	total_layer = fsl_dev->soc->total_layer;
+	for (i = 0; i < total_layer - 1; i++) {
+		overlay = kzalloc(sizeof(*overlay), GFP_KERNEL);
+		if (!overlay) {
+			DRM_DEBUG_KMS("Failed to allocate overlay plane\n");
+			goto out;
+		}
+
+		ret = drm_universal_plane_init(dev, overlay, 1,
+					       &fsl_dcu_drm_plane_funcs,
+					       fsl_dcu_drm_plane_formats,
+					       formats_size,
+					       DRM_PLANE_TYPE_OVERLAY);
+		if (ret) {
+			kfree(overlay);
+			goto out;
+		}
+		drm_plane_helper_add(overlay, &fsl_dcu_drm_plane_helper_funcs);
+	}
+
+out:
 	return primary;
 }
-- 
2.1.0.27.g96db324

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RESEND 3/3] drm :fsl-dcu: Add multi layers support

[Stefan Agner]

Hi Dongsheng,

On 2015-12-01 00:16, Dongsheng Wang wrote:
> From: Jianwei Wang <jianwei.wang.chn@gmail.com>
> 
> For DCU support atmost 16 layers(on ls1021a) or 64 layers(on vf610),
> add (total_layer - 1) overlay planes.
> 
> Signed-off-by: Jianwei Wang <jianwei.wang.chn@gmail.com>
> Signed-off-by: Yi Meng <b56799@freescale.com>
> Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com>
> 
> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c
> b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c
> index a8932a8..7ed1a7e 100644
> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c
> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c
> @@ -235,7 +235,10 @@ static const u32 fsl_dcu_drm_plane_formats[] = {
>  
>  struct drm_plane *fsl_dcu_drm_primary_create_plane(struct drm_device *dev)
>  {

It feels wrong to me to add multi layer support in a function called
fsl_dcu_drm_primary_create_plane...

I suggest to either rename the function, or better create a new function
for the overlay planes. The only shared variable is formats_size, which
is anyway statically determined by the compiler.

--
Stefan

> +	struct fsl_dcu_drm_device *fsl_dev = dev->dev_private;
>  	struct drm_plane *primary;
> +	struct drm_plane *overlay;
> +	unsigned int total_layer, formats_size, i;
>  	int ret;
>  
>  	primary = kzalloc(sizeof(*primary), GFP_KERNEL);
> @@ -244,11 +247,12 @@ struct drm_plane
> *fsl_dcu_drm_primary_create_plane(struct drm_device *dev)
>  		return NULL;
>  	}
>  
> +	formats_size = ARRAY_SIZE(fsl_dcu_drm_plane_formats);
>  	/* possible_crtc's will be filled in later by crtc_init */
>  	ret = drm_universal_plane_init(dev, primary, 0,
>  				       &fsl_dcu_drm_plane_funcs,
>  				       fsl_dcu_drm_plane_formats,
> -				       ARRAY_SIZE(fsl_dcu_drm_plane_formats),
> +				       formats_size,
>  				       DRM_PLANE_TYPE_PRIMARY);
>  	if (ret) {
>  		kfree(primary);
> @@ -256,5 +260,26 @@ struct drm_plane
> *fsl_dcu_drm_primary_create_plane(struct drm_device *dev)
>  	}
>  	drm_plane_helper_add(primary, &fsl_dcu_drm_plane_helper_funcs);
>  
> +	total_layer = fsl_dev->soc->total_layer;
> +	for (i = 0; i < total_layer - 1; i++) {
> +		overlay = kzalloc(sizeof(*overlay), GFP_KERNEL);
> +		if (!overlay) {
> +			DRM_DEBUG_KMS("Failed to allocate overlay plane\n");
> +			goto out;
> +		}
> +
> +		ret = drm_universal_plane_init(dev, overlay, 1,
> +					       &fsl_dcu_drm_plane_funcs,
> +					       fsl_dcu_drm_plane_formats,
> +					       formats_size,
> +					       DRM_PLANE_TYPE_OVERLAY);
> +		if (ret) {
> +			kfree(overlay);

You basically allow to initialize only some layers, which is probably
fine. But I think we should add at least an error message here... 

--
Stefan

> +			goto out;
> +		}
> +		drm_plane_helper_add(overlay, &fsl_dcu_drm_plane_helper_funcs);
> +	}
> +
> +out:
>  	return primary;
>  }
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RESEND 2/3] drm :fsl-dcu: Cleanup vblank interrupt mask and status setting code

[Stefan Agner]

[also added Jianwei Wang, since he is the current maintainer of this
driver]

On 2015-12-01 00:16, Dongsheng Wang wrote:
> From: Jianwei Wang <jianwei.wang.chn@gmail.com>
> 
> Switch update interrupt mask bit with regmap_update_bits, and clear
> interrupt status by writing 1 to relevant bit before setting mask in
> fsl_dcu_drm_irq_init function.
> 
> Signed-off-by: Jianwei Wang <jianwei.wang.chn@gmail.com>
> Signed-off-by: Yi Meng <b56799@freescale.com>
> Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com>
> 
> 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 1930234..5c29ff7 100644
> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
> @@ -38,21 +38,17 @@ static const struct regmap_config fsl_dcu_regmap_config = {
>  static int fsl_dcu_drm_irq_init(struct drm_device *dev)
>  {
>  	struct fsl_dcu_drm_device *fsl_dev = dev->dev_private;
> -	unsigned int value;
>  	int ret;
>  
>  	ret = drm_irq_install(dev, fsl_dev->irq);
>  	if (ret < 0)
>  		dev_err(dev->dev, "failed to install IRQ handler\n");
>  
> -	ret = regmap_write(fsl_dev->regmap, DCU_INT_STATUS, 0);
> +	ret = regmap_write(fsl_dev->regmap, DCU_INT_STATUS, 0xffffffff);
>  	if (ret)
>  		dev_err(dev->dev, "set DCU_INT_STATUS failed\n");
> -	ret = regmap_read(fsl_dev->regmap, DCU_INT_MASK, &value);
> -	if (ret)
> -		dev_err(dev->dev, "read DCU_INT_MASK failed\n");
> -	value &= DCU_INT_MASK_VBLANK;
> -	ret = regmap_write(fsl_dev->regmap, DCU_INT_MASK, value);
> +	ret = regmap_update_bits(fsl_dev->regmap, DCU_INT_MASK,
> +				 DCU_INT_MASK_VBLANK, ~DCU_INT_MASK_VBLANK);

Just realized that patch tries to solve the issue:
https://lkml.org/lkml/2015/11/18/951

However, in my variant, VBLANK is not enabled on init. Afaik,
enable_vblank will be called anyway, hence there is no need to unmask
VBLANK here...

This patchset and my patchset have some overlap, we need to serialize
this patches.

@Jianwei/DRM maintainers, what do you think, how do we proceed on this?

--
Stefan

>  	if (ret)
>  		dev_err(dev->dev, "set DCU_INT_MASK failed\n");
>  	ret = regmap_write(fsl_dev->regmap, DCU_UPDATE_MODE,
> @@ -143,14 +139,10 @@ static irqreturn_t fsl_dcu_drm_irq(int irq, void *arg)
>  static int fsl_dcu_drm_enable_vblank(struct drm_device *dev, unsigned int pipe)
>  {
>  	struct fsl_dcu_drm_device *fsl_dev = dev->dev_private;
> -	unsigned int value;
>  	int ret;
>  
> -	ret = regmap_read(fsl_dev->regmap, DCU_INT_MASK, &value);
> -	if (ret)
> -		dev_err(dev->dev, "read DCU_INT_MASK failed\n");
> -	value &= ~DCU_INT_MASK_VBLANK;
> -	ret = regmap_write(fsl_dev->regmap, DCU_INT_MASK, value);
> +	ret = regmap_update_bits(fsl_dev->regmap, DCU_INT_MASK,
> +				 DCU_INT_MASK_VBLANK, ~DCU_INT_MASK_VBLANK);
>  	if (ret)
>  		dev_err(dev->dev, "set DCU_INT_MASK failed\n");
>  	return 0;
> @@ -160,14 +152,10 @@ static void fsl_dcu_drm_disable_vblank(struct
> drm_device *dev,
>  				       unsigned int pipe)
>  {
>  	struct fsl_dcu_drm_device *fsl_dev = dev->dev_private;
> -	unsigned int value;
>  	int ret;
>  
> -	ret = regmap_read(fsl_dev->regmap, DCU_INT_MASK, &value);
> -	if (ret)
> -		dev_err(dev->dev, "read DCU_INT_MASK failed\n");
> -	value |= DCU_INT_MASK_VBLANK;
> -	ret = regmap_write(fsl_dev->regmap, DCU_INT_MASK, value);
> +	ret = regmap_update_bits(fsl_dev->regmap, DCU_INT_MASK,
> +				 DCU_INT_MASK_VBLANK, DCU_INT_MASK_VBLANK);
>  	if (ret)
>  		dev_err(dev->dev, "set DCU_INT_MASK failed\n");
>  }
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [RESEND 3/3] drm :fsl-dcu: Add multi layers support

[Wang Dongsheng]

Hi Stefan,

Thanks, now I'm working on PSCIv1.0 for u-boot, and PCIe PM patches.
Will reply your comments later(Maybe next week).

Regards,
-Dongsheng

> 
> Hi Dongsheng,
> 
> On 2015-12-01 00:16, Dongsheng Wang wrote:
> > From: Jianwei Wang <jianwei.wang.chn@gmail.com>
> >
> > For DCU support atmost 16 layers(on ls1021a) or 64 layers(on vf610),
> > add (total_layer - 1) overlay planes.
> >
> > Signed-off-by: Jianwei Wang <jianwei.wang.chn@gmail.com>
> > Signed-off-by: Yi Meng <b56799@freescale.com>
> > Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com>
> >
> > diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c
> > b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c
> > index a8932a8..7ed1a7e 100644
> > --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c
> > +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c
> > @@ -235,7 +235,10 @@ static const u32 fsl_dcu_drm_plane_formats[] = {
> >
> >  struct drm_plane *fsl_dcu_drm_primary_create_plane(struct drm_device
> > *dev)  {
> 
> It feels wrong to me to add multi layer support in a function called
> fsl_dcu_drm_primary_create_plane...
> 
> I suggest to either rename the function, or better create a new function for the
> overlay planes. The only shared variable is formats_size, which is anyway
> statically determined by the compiler.
> 
> --
> Stefan
> 
> > +	struct fsl_dcu_drm_device *fsl_dev = dev->dev_private;
> >  	struct drm_plane *primary;
> > +	struct drm_plane *overlay;
> > +	unsigned int total_layer, formats_size, i;
> >  	int ret;
> >
> >  	primary = kzalloc(sizeof(*primary), GFP_KERNEL); @@ -244,11 +247,12
> > @@ struct drm_plane *fsl_dcu_drm_primary_create_plane(struct
> > drm_device *dev)
> >  		return NULL;
> >  	}
> >
> > +	formats_size = ARRAY_SIZE(fsl_dcu_drm_plane_formats);
> >  	/* possible_crtc's will be filled in later by crtc_init */
> >  	ret = drm_universal_plane_init(dev, primary, 0,
> >  				       &fsl_dcu_drm_plane_funcs,
> >  				       fsl_dcu_drm_plane_formats,
> > -				       ARRAY_SIZE(fsl_dcu_drm_plane_formats),
> > +				       formats_size,
> >  				       DRM_PLANE_TYPE_PRIMARY);
> >  	if (ret) {
> >  		kfree(primary);
> > @@ -256,5 +260,26 @@ struct drm_plane
> > *fsl_dcu_drm_primary_create_plane(struct drm_device *dev)
> >  	}
> >  	drm_plane_helper_add(primary, &fsl_dcu_drm_plane_helper_funcs);
> >
> > +	total_layer = fsl_dev->soc->total_layer;
> > +	for (i = 0; i < total_layer - 1; i++) {
> > +		overlay = kzalloc(sizeof(*overlay), GFP_KERNEL);
> > +		if (!overlay) {
> > +			DRM_DEBUG_KMS("Failed to allocate overlay plane\n");
> > +			goto out;
> > +		}
> > +
> > +		ret = drm_universal_plane_init(dev, overlay, 1,
> > +					       &fsl_dcu_drm_plane_funcs,
> > +					       fsl_dcu_drm_plane_formats,
> > +					       formats_size,
> > +					       DRM_PLANE_TYPE_OVERLAY);
> > +		if (ret) {
> > +			kfree(overlay);
> 
> You basically allow to initialize only some layers, which is probably fine. But
> I think we should add at least an error message here...
> 
> --
> Stefan
> 
> > +			goto out;
> > +		}
> > +		drm_plane_helper_add(overlay, &fsl_dcu_drm_plane_helper_funcs);
> > +	}
> > +
> > +out:
> >  	return primary;
> >  }
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel