[PATCH 0/7] drm/fsl-dcu: fixes and enhancements

[PATCH 0/7] drm/fsl-dcu: fixes and enhancements

[Stefan Agner]

During testing the DCU DRM driver on the Freescale Vybrid platform
I came across some (platform independent) bugs and problems which
this patchset addresses.

Note: To use the driver on Vybrid some platform/device-tree
enhancements are needed which are not part of this patchset.
I still need to clean those up.

Stefan Agner (7):
  drm/fsl-dcu: specify volatile registers
  drm/fsl-dcu: remove regmap return value checks
  drm/fsl-dcu: avoid memory leak on errors
  drm/fsl-dcu: handle initialization errors properly
  drm/fsl-dcu: mask all interrupts on initialization
  drm/fsl-dcu: fix alpha blending
  drm/fsl-dcu: use mode flags for hsync/vsync pixelclk polarity

 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c  | 143 +++++++++++-----------------
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c   |  65 ++++++-------
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h   |   8 +-
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_kms.c   |  24 ++++-
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c | 137 ++++++++++++--------------
 drivers/gpu/drm/panel/panel-simple.c        |   2 +
 6 files changed, 171 insertions(+), 208 deletions(-)

-- 
2.6.2

[PATCH 0/7] drm/fsl-dcu: fixes and enhancements

[Stefan Agner]

During testing the DCU DRM driver on the Freescale Vybrid platform
I came across some (platform independent) bugs and problems which
this patchset addresses.

Note: To use the driver on Vybrid some platform/device-tree
enhancements are needed which are not part of this patchset.
I still need to clean those up.

Stefan Agner (7):
  drm/fsl-dcu: specify volatile registers
  drm/fsl-dcu: remove regmap return value checks
  drm/fsl-dcu: avoid memory leak on errors
  drm/fsl-dcu: handle initialization errors properly
  drm/fsl-dcu: mask all interrupts on initialization
  drm/fsl-dcu: fix alpha blending
  drm/fsl-dcu: use mode flags for hsync/vsync pixelclk polarity

 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c  | 143 +++++++++++-----------------
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c   |  65 ++++++-------
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h   |   8 +-
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_kms.c   |  24 ++++-
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c | 137 ++++++++++++--------------
 drivers/gpu/drm/panel/panel-simple.c        |   2 +
 6 files changed, 171 insertions(+), 208 deletions(-)

-- 
2.6.2

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

[PATCH 1/7] drm/fsl-dcu: specify volatile registers

[Stefan Agner]

Since we are using cached registers, we need to specify volatile
registers explicitly to avoid reading their value from the cache.
This allows to read the correct interrupt status in fsl_dcu_drm_irq
and clear the asserted bits only.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

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..d6e27af 100644
--- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
+++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
@@ -28,11 +28,21 @@
 #include "fsl_dcu_drm_crtc.h"
 #include "fsl_dcu_drm_drv.h"
 
+static bool fsl_dcu_drm_is_volatile_reg(struct device *dev, unsigned int reg)
+{
+	if (reg == DCU_INT_STATUS || reg == DCU_UPDATE_MODE)
+		return true;
+
+	return false;
+}
+
 static const struct regmap_config fsl_dcu_regmap_config = {
 	.reg_bits = 32,
 	.reg_stride = 4,
 	.val_bits = 32,
 	.cache_type = REGCACHE_RBTREE,
+
+	.volatile_reg = fsl_dcu_drm_is_volatile_reg,
 };
 
 static int fsl_dcu_drm_irq_init(struct drm_device *dev)
@@ -129,7 +139,7 @@ static irqreturn_t fsl_dcu_drm_irq(int irq, void *arg)
 	if (int_status & DCU_INT_STATUS_VBLANK)
 		drm_handle_vblank(dev, 0);
 
-	ret = regmap_write(fsl_dev->regmap, DCU_INT_STATUS, 0xffffffff);
+	ret = regmap_write(fsl_dev->regmap, DCU_INT_STATUS, int_status);
 	if (ret)
 		dev_err(dev->dev, "set DCU_INT_STATUS failed\n");
 	ret = regmap_write(fsl_dev->regmap, DCU_UPDATE_MODE,
-- 
2.6.2

[PATCH 1/7] drm/fsl-dcu: specify volatile registers

[Stefan Agner]

Since we are using cached registers, we need to specify volatile
registers explicitly to avoid reading their value from the cache.
This allows to read the correct interrupt status in fsl_dcu_drm_irq
and clear the asserted bits only.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

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..d6e27af 100644
--- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
+++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
@@ -28,11 +28,21 @@
 #include "fsl_dcu_drm_crtc.h"
 #include "fsl_dcu_drm_drv.h"
 
+static bool fsl_dcu_drm_is_volatile_reg(struct device *dev, unsigned int reg)
+{
+	if (reg == DCU_INT_STATUS || reg == DCU_UPDATE_MODE)
+		return true;
+
+	return false;
+}
+
 static const struct regmap_config fsl_dcu_regmap_config = {
 	.reg_bits = 32,
 	.reg_stride = 4,
 	.val_bits = 32,
 	.cache_type = REGCACHE_RBTREE,
+
+	.volatile_reg = fsl_dcu_drm_is_volatile_reg,
 };
 
 static int fsl_dcu_drm_irq_init(struct drm_device *dev)
@@ -129,7 +139,7 @@ static irqreturn_t fsl_dcu_drm_irq(int irq, void *arg)
 	if (int_status & DCU_INT_STATUS_VBLANK)
 		drm_handle_vblank(dev, 0);
 
-	ret = regmap_write(fsl_dev->regmap, DCU_INT_STATUS, 0xffffffff);
+	ret = regmap_write(fsl_dev->regmap, DCU_INT_STATUS, int_status);
 	if (ret)
 		dev_err(dev->dev, "set DCU_INT_STATUS failed\n");
 	ret = regmap_write(fsl_dev->regmap, DCU_UPDATE_MODE,
-- 
2.6.2

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

[PATCH 2/7] drm/fsl-dcu: remove regmap return value checks

[Stefan Agner]

It is not common to do regmap return value checks, especially not
for memory mapped device. We can rule out most error returns since
the conditions are static and we know they are ok (e.g. offset
aligned to register stride). Also without proper error handling
they are not really valuable for the user. Hence remove most of
them.

The check in the interrupt handler is worth keeping since a
volatile register won't be readable in case register caching is
still enabled.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c  | 124 +++++++++-------------------
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c   |  54 ++++--------
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c | 109 +++++++++---------------
 3 files changed, 99 insertions(+), 188 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 82a3d31..adf9212 100644
--- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c
+++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c
@@ -42,34 +42,24 @@ static void fsl_dcu_drm_disable_crtc(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
 	struct fsl_dcu_drm_device *fsl_dev = dev->dev_private;
-	int ret;
 
-	ret = regmap_update_bits(fsl_dev->regmap, DCU_DCU_MODE,
-				 DCU_MODE_DCU_MODE_MASK,
-				 DCU_MODE_DCU_MODE(DCU_MODE_OFF));
-	if (ret)
-		dev_err(fsl_dev->dev, "Disable CRTC failed\n");
-	ret = regmap_write(fsl_dev->regmap, DCU_UPDATE_MODE,
-			   DCU_UPDATE_MODE_READREG);
-	if (ret)
-		dev_err(fsl_dev->dev, "Enable CRTC failed\n");
+	regmap_update_bits(fsl_dev->regmap, DCU_DCU_MODE,
+			   DCU_MODE_DCU_MODE_MASK,
+			   DCU_MODE_DCU_MODE(DCU_MODE_OFF));
+	regmap_write(fsl_dev->regmap, DCU_UPDATE_MODE,
+		     DCU_UPDATE_MODE_READREG);
 }
 
 static void fsl_dcu_drm_crtc_enable(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
 	struct fsl_dcu_drm_device *fsl_dev = dev->dev_private;
-	int ret;
 
-	ret = regmap_update_bits(fsl_dev->regmap, DCU_DCU_MODE,
-				 DCU_MODE_DCU_MODE_MASK,
-				 DCU_MODE_DCU_MODE(DCU_MODE_NORMAL));
-	if (ret)
-		dev_err(fsl_dev->dev, "Enable CRTC failed\n");
-	ret = regmap_write(fsl_dev->regmap, DCU_UPDATE_MODE,
-			   DCU_UPDATE_MODE_READREG);
-	if (ret)
-		dev_err(fsl_dev->dev, "Enable CRTC failed\n");
+	regmap_update_bits(fsl_dev->regmap, DCU_DCU_MODE,
+			   DCU_MODE_DCU_MODE_MASK,
+			   DCU_MODE_DCU_MODE(DCU_MODE_NORMAL));
+	regmap_write(fsl_dev->regmap, DCU_UPDATE_MODE,
+		     DCU_UPDATE_MODE_READREG);
 }
 
 static bool fsl_dcu_drm_crtc_mode_fixup(struct drm_crtc *crtc,
@@ -86,7 +76,6 @@ static void fsl_dcu_drm_crtc_mode_set_nofb(struct drm_crtc *crtc)
 	struct drm_display_mode *mode = &crtc->state->mode;
 	unsigned int hbp, hfp, hsw, vbp, vfp, vsw, div, index;
 	unsigned long dcuclk;
-	int ret;
 
 	index = drm_crtc_index(crtc);
 	dcuclk = clk_get_rate(fsl_dev->clk);
@@ -100,51 +89,31 @@ static void fsl_dcu_drm_crtc_mode_set_nofb(struct drm_crtc *crtc)
 	vfp = mode->vsync_start - mode->vdisplay;
 	vsw = mode->vsync_end - mode->vsync_start;
 
-	ret = regmap_write(fsl_dev->regmap, DCU_HSYN_PARA,
-			   DCU_HSYN_PARA_BP(hbp) |
-			   DCU_HSYN_PARA_PW(hsw) |
-			   DCU_HSYN_PARA_FP(hfp));
-	if (ret)
-		goto set_failed;
-	ret = regmap_write(fsl_dev->regmap, DCU_VSYN_PARA,
-			   DCU_VSYN_PARA_BP(vbp) |
-			   DCU_VSYN_PARA_PW(vsw) |
-			   DCU_VSYN_PARA_FP(vfp));
-	if (ret)
-		goto set_failed;
-	ret = regmap_write(fsl_dev->regmap, DCU_DISP_SIZE,
-			   DCU_DISP_SIZE_DELTA_Y(mode->vdisplay) |
-			   DCU_DISP_SIZE_DELTA_X(mode->hdisplay));
-	if (ret)
-		goto set_failed;
-	ret = regmap_write(fsl_dev->regmap, DCU_DIV_RATIO, div);
-	if (ret)
-		goto set_failed;
-	ret = regmap_write(fsl_dev->regmap, DCU_SYN_POL,
-			   DCU_SYN_POL_INV_VS_LOW | DCU_SYN_POL_INV_HS_LOW);
-	if (ret)
-		goto set_failed;
-	ret = regmap_write(fsl_dev->regmap, DCU_BGND, DCU_BGND_R(0) |
-			   DCU_BGND_G(0) | DCU_BGND_B(0));
-	if (ret)
-		goto set_failed;
-	ret = regmap_write(fsl_dev->regmap, DCU_DCU_MODE,
-			   DCU_MODE_BLEND_ITER(1) | DCU_MODE_RASTER_EN);
-	if (ret)
-		goto set_failed;
-	ret = regmap_write(fsl_dev->regmap, DCU_THRESHOLD,
-			   DCU_THRESHOLD_LS_BF_VS(BF_VS_VAL) |
-			   DCU_THRESHOLD_OUT_BUF_HIGH(BUF_MAX_VAL) |
-			   DCU_THRESHOLD_OUT_BUF_LOW(BUF_MIN_VAL));
-	if (ret)
-		goto set_failed;
-	ret = regmap_write(fsl_dev->regmap, DCU_UPDATE_MODE,
-			   DCU_UPDATE_MODE_READREG);
-	if (ret)
-		goto set_failed;
+	regmap_write(fsl_dev->regmap, DCU_HSYN_PARA,
+		     DCU_HSYN_PARA_BP(hbp) |
+		     DCU_HSYN_PARA_PW(hsw) |
+		     DCU_HSYN_PARA_FP(hfp));
+	regmap_write(fsl_dev->regmap, DCU_VSYN_PARA,
+		     DCU_VSYN_PARA_BP(vbp) |
+		     DCU_VSYN_PARA_PW(vsw) |
+		     DCU_VSYN_PARA_FP(vfp));
+	regmap_write(fsl_dev->regmap, DCU_DISP_SIZE,
+		     DCU_DISP_SIZE_DELTA_Y(mode->vdisplay) |
+		     DCU_DISP_SIZE_DELTA_X(mode->hdisplay));
+	regmap_write(fsl_dev->regmap, DCU_DIV_RATIO, div);
+	regmap_write(fsl_dev->regmap, DCU_SYN_POL,
+		     DCU_SYN_POL_INV_VS_LOW | DCU_SYN_POL_INV_HS_LOW);
+	regmap_write(fsl_dev->regmap, DCU_BGND, DCU_BGND_R(0) |
+		     DCU_BGND_G(0) | DCU_BGND_B(0));
+	regmap_write(fsl_dev->regmap, DCU_DCU_MODE,
+		     DCU_MODE_BLEND_ITER(1) | DCU_MODE_RASTER_EN);
+	regmap_write(fsl_dev->regmap, DCU_THRESHOLD,
+		     DCU_THRESHOLD_LS_BF_VS(BF_VS_VAL) |
+		     DCU_THRESHOLD_OUT_BUF_HIGH(BUF_MAX_VAL) |
+		     DCU_THRESHOLD_OUT_BUF_LOW(BUF_MIN_VAL));
+	regmap_write(fsl_dev->regmap, DCU_UPDATE_MODE,
+		     DCU_UPDATE_MODE_READREG);
 	return;
-set_failed:
-	dev_err(dev->dev, "set DCU register failed\n");
 }
 
 static const struct drm_crtc_helper_funcs fsl_dcu_drm_crtc_helper_funcs = {
@@ -186,25 +155,14 @@ int fsl_dcu_drm_crtc_create(struct fsl_dcu_drm_device *fsl_dev)
 	else
 		reg_num = VF610_LAYER_REG_NUM;
 	for (i = 0; i <= fsl_dev->soc->total_layer; i++) {
-		for (j = 0; j < reg_num; j++) {
-			ret = regmap_write(fsl_dev->regmap,
-					   DCU_CTRLDESCLN(i, j), 0);
-			if (ret)
-				goto init_failed;
-		}
+		for (j = 0; j < reg_num; j++)
+			regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN(i, j), 0);
 	}
-	ret = regmap_update_bits(fsl_dev->regmap, DCU_DCU_MODE,
-				 DCU_MODE_DCU_MODE_MASK,
-				 DCU_MODE_DCU_MODE(DCU_MODE_OFF));
-	if (ret)
-		goto init_failed;
-	ret = regmap_write(fsl_dev->regmap, DCU_UPDATE_MODE,
-			   DCU_UPDATE_MODE_READREG);
-	if (ret)
-		goto init_failed;
+	regmap_update_bits(fsl_dev->regmap, DCU_DCU_MODE,
+			   DCU_MODE_DCU_MODE_MASK,
+			   DCU_MODE_DCU_MODE(DCU_MODE_OFF));
+	regmap_write(fsl_dev->regmap, DCU_UPDATE_MODE,
+		     DCU_UPDATE_MODE_READREG);
 
 	return 0;
-init_failed:
-	dev_err(fsl_dev->dev, "init DCU register failed\n");
-	return ret;
 }
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 d6e27af..d37ff0e 100644
--- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
+++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
@@ -55,20 +55,12 @@ static int fsl_dcu_drm_irq_init(struct drm_device *dev)
 	if (ret < 0)
 		dev_err(dev->dev, "failed to install IRQ handler\n");
 
-	ret = regmap_write(fsl_dev->regmap, DCU_INT_STATUS, 0);
-	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");
+	regmap_write(fsl_dev->regmap, DCU_INT_STATUS, 0);
+	regmap_read(fsl_dev->regmap, DCU_INT_MASK, &value);
 	value &= DCU_INT_MASK_VBLANK;
-	ret = regmap_write(fsl_dev->regmap, DCU_INT_MASK, value);
-	if (ret)
-		dev_err(dev->dev, "set DCU_INT_MASK failed\n");
-	ret = regmap_write(fsl_dev->regmap, DCU_UPDATE_MODE,
-			   DCU_UPDATE_MODE_READREG);
-	if (ret)
-		dev_err(dev->dev, "set DCU_UPDATE_MODE failed\n");
+	regmap_write(fsl_dev->regmap, DCU_INT_MASK, value);
+	regmap_write(fsl_dev->regmap, DCU_UPDATE_MODE,
+		     DCU_UPDATE_MODE_READREG);
 
 	return ret;
 }
@@ -134,18 +126,17 @@ static irqreturn_t fsl_dcu_drm_irq(int irq, void *arg)
 	int ret;
 
 	ret = regmap_read(fsl_dev->regmap, DCU_INT_STATUS, &int_status);
-	if (ret)
-		dev_err(dev->dev, "set DCU_INT_STATUS failed\n");
+	if (ret) {
+		dev_err(dev->dev, "read DCU_INT_STATUS failed\n");
+		return IRQ_NONE;
+	}
+
 	if (int_status & DCU_INT_STATUS_VBLANK)
 		drm_handle_vblank(dev, 0);
 
-	ret = regmap_write(fsl_dev->regmap, DCU_INT_STATUS, int_status);
-	if (ret)
-		dev_err(dev->dev, "set DCU_INT_STATUS failed\n");
-	ret = regmap_write(fsl_dev->regmap, DCU_UPDATE_MODE,
-			   DCU_UPDATE_MODE_READREG);
-	if (ret)
-		dev_err(dev->dev, "set DCU_UPDATE_MODE failed\n");
+	regmap_write(fsl_dev->regmap, DCU_INT_STATUS, int_status);
+	regmap_write(fsl_dev->regmap, DCU_UPDATE_MODE,
+		     DCU_UPDATE_MODE_READREG);
 
 	return IRQ_HANDLED;
 }
@@ -154,15 +145,11 @@ 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");
+	regmap_read(fsl_dev->regmap, DCU_INT_MASK, &value);
 	value &= ~DCU_INT_MASK_VBLANK;
-	ret = regmap_write(fsl_dev->regmap, DCU_INT_MASK, value);
-	if (ret)
-		dev_err(dev->dev, "set DCU_INT_MASK failed\n");
+	regmap_write(fsl_dev->regmap, DCU_INT_MASK, value);
+
 	return 0;
 }
 
@@ -171,15 +158,10 @@ static void fsl_dcu_drm_disable_vblank(struct drm_device *dev,
 {
 	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");
+	regmap_read(fsl_dev->regmap, DCU_INT_MASK, &value);
 	value |= DCU_INT_MASK_VBLANK;
-	ret = regmap_write(fsl_dev->regmap, DCU_INT_MASK, value);
-	if (ret)
-		dev_err(dev->dev, "set DCU_INT_MASK failed\n");
+	regmap_write(fsl_dev->regmap, DCU_INT_MASK, value);
 }
 
 static const struct file_operations fsl_dcu_drm_fops = {
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..92e606a 100644
--- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c
+++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c
@@ -62,19 +62,15 @@ static void fsl_dcu_drm_plane_atomic_disable(struct drm_plane *plane,
 {
 	struct fsl_dcu_drm_device *fsl_dev = plane->dev->dev_private;
 	unsigned int value;
-	int index, ret;
+	int index;
 
 	index = fsl_dcu_drm_plane_index(plane);
 	if (index < 0)
 		return;
 
-	ret = regmap_read(fsl_dev->regmap, DCU_CTRLDESCLN(index, 4), &value);
-	if (ret)
-		dev_err(fsl_dev->dev, "read DCU_INT_MASK failed\n");
+	regmap_read(fsl_dev->regmap, DCU_CTRLDESCLN(index, 4), &value);
 	value &= ~DCU_LAYER_EN;
-	ret = regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN(index, 4), value);
-	if (ret)
-		dev_err(fsl_dev->dev, "set DCU register failed\n");
+	regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN(index, 4), value);
 }
 
 static void fsl_dcu_drm_plane_atomic_update(struct drm_plane *plane,
@@ -86,7 +82,7 @@ static void fsl_dcu_drm_plane_atomic_update(struct drm_plane *plane,
 	struct drm_framebuffer *fb = plane->state->fb;
 	struct drm_gem_cma_object *gem;
 	unsigned int alpha, bpp;
-	int index, ret;
+	int index;
 
 	index = fsl_dcu_drm_plane_index(plane);
 	if (index < 0)
@@ -123,70 +119,45 @@ static void fsl_dcu_drm_plane_atomic_update(struct drm_plane *plane,
 		return;
 	}
 
-	ret = regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN(index, 1),
-			   DCU_LAYER_HEIGHT(state->crtc_h) |
-			   DCU_LAYER_WIDTH(state->crtc_w));
-	if (ret)
-		goto set_failed;
-	ret = regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN(index, 2),
-			   DCU_LAYER_POSY(state->crtc_y) |
-			   DCU_LAYER_POSX(state->crtc_x));
-	if (ret)
-		goto set_failed;
-	ret = regmap_write(fsl_dev->regmap,
-			   DCU_CTRLDESCLN(index, 3), gem->paddr);
-	if (ret)
-		goto set_failed;
-	ret = regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN(index, 4),
-			   DCU_LAYER_EN |
-			   DCU_LAYER_TRANS(alpha) |
-			   DCU_LAYER_BPP(bpp) |
-			   DCU_LAYER_AB(0));
-	if (ret)
-		goto set_failed;
-	ret = regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN(index, 5),
-			   DCU_LAYER_CKMAX_R(0xFF) |
-			   DCU_LAYER_CKMAX_G(0xFF) |
-			   DCU_LAYER_CKMAX_B(0xFF));
-	if (ret)
-		goto set_failed;
-	ret = regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN(index, 6),
-			   DCU_LAYER_CKMIN_R(0) |
-			   DCU_LAYER_CKMIN_G(0) |
-			   DCU_LAYER_CKMIN_B(0));
-	if (ret)
-		goto set_failed;
-	ret = regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN(index, 7), 0);
-	if (ret)
-		goto set_failed;
-	ret = regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN(index, 8),
-			   DCU_LAYER_FG_FCOLOR(0));
-	if (ret)
-		goto set_failed;
-	ret = regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN(index, 9),
-			   DCU_LAYER_BG_BCOLOR(0));
-	if (ret)
-		goto set_failed;
+	regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN(index, 1),
+		     DCU_LAYER_HEIGHT(state->crtc_h) |
+		     DCU_LAYER_WIDTH(state->crtc_w));
+	regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN(index, 2),
+		     DCU_LAYER_POSY(state->crtc_y) |
+		     DCU_LAYER_POSX(state->crtc_x));
+	regmap_write(fsl_dev->regmap,
+		     DCU_CTRLDESCLN(index, 3), gem->paddr);
+	regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN(index, 4),
+		     DCU_LAYER_EN |
+		     DCU_LAYER_TRANS(alpha) |
+		     DCU_LAYER_BPP(bpp) |
+		     DCU_LAYER_AB(0));
+	regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN(index, 5),
+		     DCU_LAYER_CKMAX_R(0xFF) |
+		     DCU_LAYER_CKMAX_G(0xFF) |
+		     DCU_LAYER_CKMAX_B(0xFF));
+	regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN(index, 6),
+		     DCU_LAYER_CKMIN_R(0) |
+		     DCU_LAYER_CKMIN_G(0) |
+		     DCU_LAYER_CKMIN_B(0));
+	regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN(index, 7), 0);
+	regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN(index, 8),
+		     DCU_LAYER_FG_FCOLOR(0));
+	regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN(index, 9),
+		     DCU_LAYER_BG_BCOLOR(0));
+
 	if (!strcmp(fsl_dev->soc->name, "ls1021a")) {
-		ret = regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN(index, 10),
-				   DCU_LAYER_POST_SKIP(0) |
-				   DCU_LAYER_PRE_SKIP(0));
-		if (ret)
-			goto set_failed;
+		regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN(index, 10),
+			     DCU_LAYER_POST_SKIP(0) |
+			     DCU_LAYER_PRE_SKIP(0));
 	}
-	ret = regmap_update_bits(fsl_dev->regmap, DCU_DCU_MODE,
-				 DCU_MODE_DCU_MODE_MASK,
-				 DCU_MODE_DCU_MODE(DCU_MODE_NORMAL));
-	if (ret)
-		goto set_failed;
-	ret = regmap_write(fsl_dev->regmap,
-			   DCU_UPDATE_MODE, DCU_UPDATE_MODE_READREG);
-	if (ret)
-		goto set_failed;
-	return;
+	regmap_update_bits(fsl_dev->regmap, DCU_DCU_MODE,
+			   DCU_MODE_DCU_MODE_MASK,
+			   DCU_MODE_DCU_MODE(DCU_MODE_NORMAL));
+	regmap_write(fsl_dev->regmap,
+		     DCU_UPDATE_MODE, DCU_UPDATE_MODE_READREG);
 
-set_failed:
-	dev_err(fsl_dev->dev, "set DCU register failed\n");
+	return;
 }
 
 static void
-- 
2.6.2

[PATCH 2/7] drm/fsl-dcu: remove regmap return value checks

[Stefan Agner]

It is not common to do regmap return value checks, especially not
for memory mapped device. We can rule out most error returns since
the conditions are static and we know they are ok (e.g. offset
aligned to register stride). Also without proper error handling
they are not really valuable for the user. Hence remove most of
them.

The check in the interrupt handler is worth keeping since a
volatile register won't be readable in case register caching is
still enabled.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c  | 124 +++++++++-------------------
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c   |  54 ++++--------
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c | 109 +++++++++---------------
 3 files changed, 99 insertions(+), 188 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 82a3d31..adf9212 100644
--- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c
+++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c
@@ -42,34 +42,24 @@ static void fsl_dcu_drm_disable_crtc(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
 	struct fsl_dcu_drm_device *fsl_dev = dev->dev_private;
-	int ret;
 
-	ret = regmap_update_bits(fsl_dev->regmap, DCU_DCU_MODE,
-				 DCU_MODE_DCU_MODE_MASK,
-				 DCU_MODE_DCU_MODE(DCU_MODE_OFF));
-	if (ret)
-		dev_err(fsl_dev->dev, "Disable CRTC failed\n");
-	ret = regmap_write(fsl_dev->regmap, DCU_UPDATE_MODE,
-			   DCU_UPDATE_MODE_READREG);
-	if (ret)
-		dev_err(fsl_dev->dev, "Enable CRTC failed\n");
+	regmap_update_bits(fsl_dev->regmap, DCU_DCU_MODE,
+			   DCU_MODE_DCU_MODE_MASK,
+			   DCU_MODE_DCU_MODE(DCU_MODE_OFF));
+	regmap_write(fsl_dev->regmap, DCU_UPDATE_MODE,
+		     DCU_UPDATE_MODE_READREG);
 }
 
 static void fsl_dcu_drm_crtc_enable(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
 	struct fsl_dcu_drm_device *fsl_dev = dev->dev_private;
-	int ret;
 
-	ret = regmap_update_bits(fsl_dev->regmap, DCU_DCU_MODE,
-				 DCU_MODE_DCU_MODE_MASK,
-				 DCU_MODE_DCU_MODE(DCU_MODE_NORMAL));
-	if (ret)
-		dev_err(fsl_dev->dev, "Enable CRTC failed\n");
-	ret = regmap_write(fsl_dev->regmap, DCU_UPDATE_MODE,
-			   DCU_UPDATE_MODE_READREG);
-	if (ret)
-		dev_err(fsl_dev->dev, "Enable CRTC failed\n");
+	regmap_update_bits(fsl_dev->regmap, DCU_DCU_MODE,
+			   DCU_MODE_DCU_MODE_MASK,
+			   DCU_MODE_DCU_MODE(DCU_MODE_NORMAL));
+	regmap_write(fsl_dev->regmap, DCU_UPDATE_MODE,
+		     DCU_UPDATE_MODE_READREG);
 }
 
 static bool fsl_dcu_drm_crtc_mode_fixup(struct drm_crtc *crtc,
@@ -86,7 +76,6 @@ static void fsl_dcu_drm_crtc_mode_set_nofb(struct drm_crtc *crtc)
 	struct drm_display_mode *mode = &crtc->state->mode;
 	unsigned int hbp, hfp, hsw, vbp, vfp, vsw, div, index;
 	unsigned long dcuclk;
-	int ret;
 
 	index = drm_crtc_index(crtc);
 	dcuclk = clk_get_rate(fsl_dev->clk);
@@ -100,51 +89,31 @@ static void fsl_dcu_drm_crtc_mode_set_nofb(struct drm_crtc *crtc)
 	vfp = mode->vsync_start - mode->vdisplay;
 	vsw = mode->vsync_end - mode->vsync_start;
 
-	ret = regmap_write(fsl_dev->regmap, DCU_HSYN_PARA,
-			   DCU_HSYN_PARA_BP(hbp) |
-			   DCU_HSYN_PARA_PW(hsw) |
-			   DCU_HSYN_PARA_FP(hfp));
-	if (ret)
-		goto set_failed;
-	ret = regmap_write(fsl_dev->regmap, DCU_VSYN_PARA,
-			   DCU_VSYN_PARA_BP(vbp) |
-			   DCU_VSYN_PARA_PW(vsw) |
-			   DCU_VSYN_PARA_FP(vfp));
-	if (ret)
-		goto set_failed;
-	ret = regmap_write(fsl_dev->regmap, DCU_DISP_SIZE,
-			   DCU_DISP_SIZE_DELTA_Y(mode->vdisplay) |
-			   DCU_DISP_SIZE_DELTA_X(mode->hdisplay));
-	if (ret)
-		goto set_failed;
-	ret = regmap_write(fsl_dev->regmap, DCU_DIV_RATIO, div);
-	if (ret)
-		goto set_failed;
-	ret = regmap_write(fsl_dev->regmap, DCU_SYN_POL,
-			   DCU_SYN_POL_INV_VS_LOW | DCU_SYN_POL_INV_HS_LOW);
-	if (ret)
-		goto set_failed;
-	ret = regmap_write(fsl_dev->regmap, DCU_BGND, DCU_BGND_R(0) |
-			   DCU_BGND_G(0) | DCU_BGND_B(0));
-	if (ret)
-		goto set_failed;
-	ret = regmap_write(fsl_dev->regmap, DCU_DCU_MODE,
-			   DCU_MODE_BLEND_ITER(1) | DCU_MODE_RASTER_EN);
-	if (ret)
-		goto set_failed;
-	ret = regmap_write(fsl_dev->regmap, DCU_THRESHOLD,
-			   DCU_THRESHOLD_LS_BF_VS(BF_VS_VAL) |
-			   DCU_THRESHOLD_OUT_BUF_HIGH(BUF_MAX_VAL) |
-			   DCU_THRESHOLD_OUT_BUF_LOW(BUF_MIN_VAL));
-	if (ret)
-		goto set_failed;
-	ret = regmap_write(fsl_dev->regmap, DCU_UPDATE_MODE,
-			   DCU_UPDATE_MODE_READREG);
-	if (ret)
-		goto set_failed;
+	regmap_write(fsl_dev->regmap, DCU_HSYN_PARA,
+		     DCU_HSYN_PARA_BP(hbp) |
+		     DCU_HSYN_PARA_PW(hsw) |
+		     DCU_HSYN_PARA_FP(hfp));
+	regmap_write(fsl_dev->regmap, DCU_VSYN_PARA,
+		     DCU_VSYN_PARA_BP(vbp) |
+		     DCU_VSYN_PARA_PW(vsw) |
+		     DCU_VSYN_PARA_FP(vfp));
+	regmap_write(fsl_dev->regmap, DCU_DISP_SIZE,
+		     DCU_DISP_SIZE_DELTA_Y(mode->vdisplay) |
+		     DCU_DISP_SIZE_DELTA_X(mode->hdisplay));
+	regmap_write(fsl_dev->regmap, DCU_DIV_RATIO, div);
+	regmap_write(fsl_dev->regmap, DCU_SYN_POL,
+		     DCU_SYN_POL_INV_VS_LOW | DCU_SYN_POL_INV_HS_LOW);
+	regmap_write(fsl_dev->regmap, DCU_BGND, DCU_BGND_R(0) |
+		     DCU_BGND_G(0) | DCU_BGND_B(0));
+	regmap_write(fsl_dev->regmap, DCU_DCU_MODE,
+		     DCU_MODE_BLEND_ITER(1) | DCU_MODE_RASTER_EN);
+	regmap_write(fsl_dev->regmap, DCU_THRESHOLD,
+		     DCU_THRESHOLD_LS_BF_VS(BF_VS_VAL) |
+		     DCU_THRESHOLD_OUT_BUF_HIGH(BUF_MAX_VAL) |
+		     DCU_THRESHOLD_OUT_BUF_LOW(BUF_MIN_VAL));
+	regmap_write(fsl_dev->regmap, DCU_UPDATE_MODE,
+		     DCU_UPDATE_MODE_READREG);
 	return;
-set_failed:
-	dev_err(dev->dev, "set DCU register failed\n");
 }
 
 static const struct drm_crtc_helper_funcs fsl_dcu_drm_crtc_helper_funcs = {
@@ -186,25 +155,14 @@ int fsl_dcu_drm_crtc_create(struct fsl_dcu_drm_device *fsl_dev)
 	else
 		reg_num = VF610_LAYER_REG_NUM;
 	for (i = 0; i <= fsl_dev->soc->total_layer; i++) {
-		for (j = 0; j < reg_num; j++) {
-			ret = regmap_write(fsl_dev->regmap,
-					   DCU_CTRLDESCLN(i, j), 0);
-			if (ret)
-				goto init_failed;
-		}
+		for (j = 0; j < reg_num; j++)
+			regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN(i, j), 0);
 	}
-	ret = regmap_update_bits(fsl_dev->regmap, DCU_DCU_MODE,
-				 DCU_MODE_DCU_MODE_MASK,
-				 DCU_MODE_DCU_MODE(DCU_MODE_OFF));
-	if (ret)
-		goto init_failed;
-	ret = regmap_write(fsl_dev->regmap, DCU_UPDATE_MODE,
-			   DCU_UPDATE_MODE_READREG);
-	if (ret)
-		goto init_failed;
+	regmap_update_bits(fsl_dev->regmap, DCU_DCU_MODE,
+			   DCU_MODE_DCU_MODE_MASK,
+			   DCU_MODE_DCU_MODE(DCU_MODE_OFF));
+	regmap_write(fsl_dev->regmap, DCU_UPDATE_MODE,
+		     DCU_UPDATE_MODE_READREG);
 
 	return 0;
-init_failed:
-	dev_err(fsl_dev->dev, "init DCU register failed\n");
-	return ret;
 }
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 d6e27af..d37ff0e 100644
--- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
+++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
@@ -55,20 +55,12 @@ static int fsl_dcu_drm_irq_init(struct drm_device *dev)
 	if (ret < 0)
 		dev_err(dev->dev, "failed to install IRQ handler\n");
 
-	ret = regmap_write(fsl_dev->regmap, DCU_INT_STATUS, 0);
-	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");
+	regmap_write(fsl_dev->regmap, DCU_INT_STATUS, 0);
+	regmap_read(fsl_dev->regmap, DCU_INT_MASK, &value);
 	value &= DCU_INT_MASK_VBLANK;
-	ret = regmap_write(fsl_dev->regmap, DCU_INT_MASK, value);
-	if (ret)
-		dev_err(dev->dev, "set DCU_INT_MASK failed\n");
-	ret = regmap_write(fsl_dev->regmap, DCU_UPDATE_MODE,
-			   DCU_UPDATE_MODE_READREG);
-	if (ret)
-		dev_err(dev->dev, "set DCU_UPDATE_MODE failed\n");
+	regmap_write(fsl_dev->regmap, DCU_INT_MASK, value);
+	regmap_write(fsl_dev->regmap, DCU_UPDATE_MODE,
+		     DCU_UPDATE_MODE_READREG);
 
 	return ret;
 }
@@ -134,18 +126,17 @@ static irqreturn_t fsl_dcu_drm_irq(int irq, void *arg)
 	int ret;
 
 	ret = regmap_read(fsl_dev->regmap, DCU_INT_STATUS, &int_status);
-	if (ret)
-		dev_err(dev->dev, "set DCU_INT_STATUS failed\n");
+	if (ret) {
+		dev_err(dev->dev, "read DCU_INT_STATUS failed\n");
+		return IRQ_NONE;
+	}
+
 	if (int_status & DCU_INT_STATUS_VBLANK)
 		drm_handle_vblank(dev, 0);
 
-	ret = regmap_write(fsl_dev->regmap, DCU_INT_STATUS, int_status);
-	if (ret)
-		dev_err(dev->dev, "set DCU_INT_STATUS failed\n");
-	ret = regmap_write(fsl_dev->regmap, DCU_UPDATE_MODE,
-			   DCU_UPDATE_MODE_READREG);
-	if (ret)
-		dev_err(dev->dev, "set DCU_UPDATE_MODE failed\n");
+	regmap_write(fsl_dev->regmap, DCU_INT_STATUS, int_status);
+	regmap_write(fsl_dev->regmap, DCU_UPDATE_MODE,
+		     DCU_UPDATE_MODE_READREG);
 
 	return IRQ_HANDLED;
 }
@@ -154,15 +145,11 @@ 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");
+	regmap_read(fsl_dev->regmap, DCU_INT_MASK, &value);
 	value &= ~DCU_INT_MASK_VBLANK;
-	ret = regmap_write(fsl_dev->regmap, DCU_INT_MASK, value);
-	if (ret)
-		dev_err(dev->dev, "set DCU_INT_MASK failed\n");
+	regmap_write(fsl_dev->regmap, DCU_INT_MASK, value);
+
 	return 0;
 }
 
@@ -171,15 +158,10 @@ static void fsl_dcu_drm_disable_vblank(struct drm_device *dev,
 {
 	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");
+	regmap_read(fsl_dev->regmap, DCU_INT_MASK, &value);
 	value |= DCU_INT_MASK_VBLANK;
-	ret = regmap_write(fsl_dev->regmap, DCU_INT_MASK, value);
-	if (ret)
-		dev_err(dev->dev, "set DCU_INT_MASK failed\n");
+	regmap_write(fsl_dev->regmap, DCU_INT_MASK, value);
 }
 
 static const struct file_operations fsl_dcu_drm_fops = {
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..92e606a 100644
--- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c
+++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c
@@ -62,19 +62,15 @@ static void fsl_dcu_drm_plane_atomic_disable(struct drm_plane *plane,
 {
 	struct fsl_dcu_drm_device *fsl_dev = plane->dev->dev_private;
 	unsigned int value;
-	int index, ret;
+	int index;
 
 	index = fsl_dcu_drm_plane_index(plane);
 	if (index < 0)
 		return;
 
-	ret = regmap_read(fsl_dev->regmap, DCU_CTRLDESCLN(index, 4), &value);
-	if (ret)
-		dev_err(fsl_dev->dev, "read DCU_INT_MASK failed\n");
+	regmap_read(fsl_dev->regmap, DCU_CTRLDESCLN(index, 4), &value);
 	value &= ~DCU_LAYER_EN;
-	ret = regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN(index, 4), value);
-	if (ret)
-		dev_err(fsl_dev->dev, "set DCU register failed\n");
+	regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN(index, 4), value);
 }
 
 static void fsl_dcu_drm_plane_atomic_update(struct drm_plane *plane,
@@ -86,7 +82,7 @@ static void fsl_dcu_drm_plane_atomic_update(struct drm_plane *plane,
 	struct drm_framebuffer *fb = plane->state->fb;
 	struct drm_gem_cma_object *gem;
 	unsigned int alpha, bpp;
-	int index, ret;
+	int index;
 
 	index = fsl_dcu_drm_plane_index(plane);
 	if (index < 0)
@@ -123,70 +119,45 @@ static void fsl_dcu_drm_plane_atomic_update(struct drm_plane *plane,
 		return;
 	}
 
-	ret = regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN(index, 1),
-			   DCU_LAYER_HEIGHT(state->crtc_h) |
-			   DCU_LAYER_WIDTH(state->crtc_w));
-	if (ret)
-		goto set_failed;
-	ret = regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN(index, 2),
-			   DCU_LAYER_POSY(state->crtc_y) |
-			   DCU_LAYER_POSX(state->crtc_x));
-	if (ret)
-		goto set_failed;
-	ret = regmap_write(fsl_dev->regmap,
-			   DCU_CTRLDESCLN(index, 3), gem->paddr);
-	if (ret)
-		goto set_failed;
-	ret = regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN(index, 4),
-			   DCU_LAYER_EN |
-			   DCU_LAYER_TRANS(alpha) |
-			   DCU_LAYER_BPP(bpp) |
-			   DCU_LAYER_AB(0));
-	if (ret)
-		goto set_failed;
-	ret = regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN(index, 5),
-			   DCU_LAYER_CKMAX_R(0xFF) |
-			   DCU_LAYER_CKMAX_G(0xFF) |
-			   DCU_LAYER_CKMAX_B(0xFF));
-	if (ret)
-		goto set_failed;
-	ret = regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN(index, 6),
-			   DCU_LAYER_CKMIN_R(0) |
-			   DCU_LAYER_CKMIN_G(0) |
-			   DCU_LAYER_CKMIN_B(0));
-	if (ret)
-		goto set_failed;
-	ret = regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN(index, 7), 0);
-	if (ret)
-		goto set_failed;
-	ret = regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN(index, 8),
-			   DCU_LAYER_FG_FCOLOR(0));
-	if (ret)
-		goto set_failed;
-	ret = regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN(index, 9),
-			   DCU_LAYER_BG_BCOLOR(0));
-	if (ret)
-		goto set_failed;
+	regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN(index, 1),
+		     DCU_LAYER_HEIGHT(state->crtc_h) |
+		     DCU_LAYER_WIDTH(state->crtc_w));
+	regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN(index, 2),
+		     DCU_LAYER_POSY(state->crtc_y) |
+		     DCU_LAYER_POSX(state->crtc_x));
+	regmap_write(fsl_dev->regmap,
+		     DCU_CTRLDESCLN(index, 3), gem->paddr);
+	regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN(index, 4),
+		     DCU_LAYER_EN |
+		     DCU_LAYER_TRANS(alpha) |
+		     DCU_LAYER_BPP(bpp) |
+		     DCU_LAYER_AB(0));
+	regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN(index, 5),
+		     DCU_LAYER_CKMAX_R(0xFF) |
+		     DCU_LAYER_CKMAX_G(0xFF) |
+		     DCU_LAYER_CKMAX_B(0xFF));
+	regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN(index, 6),
+		     DCU_LAYER_CKMIN_R(0) |
+		     DCU_LAYER_CKMIN_G(0) |
+		     DCU_LAYER_CKMIN_B(0));
+	regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN(index, 7), 0);
+	regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN(index, 8),
+		     DCU_LAYER_FG_FCOLOR(0));
+	regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN(index, 9),
+		     DCU_LAYER_BG_BCOLOR(0));
+
 	if (!strcmp(fsl_dev->soc->name, "ls1021a")) {
-		ret = regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN(index, 10),
-				   DCU_LAYER_POST_SKIP(0) |
-				   DCU_LAYER_PRE_SKIP(0));
-		if (ret)
-			goto set_failed;
+		regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN(index, 10),
+			     DCU_LAYER_POST_SKIP(0) |
+			     DCU_LAYER_PRE_SKIP(0));
 	}
-	ret = regmap_update_bits(fsl_dev->regmap, DCU_DCU_MODE,
-				 DCU_MODE_DCU_MODE_MASK,
-				 DCU_MODE_DCU_MODE(DCU_MODE_NORMAL));
-	if (ret)
-		goto set_failed;
-	ret = regmap_write(fsl_dev->regmap,
-			   DCU_UPDATE_MODE, DCU_UPDATE_MODE_READREG);
-	if (ret)
-		goto set_failed;
-	return;
+	regmap_update_bits(fsl_dev->regmap, DCU_DCU_MODE,
+			   DCU_MODE_DCU_MODE_MASK,
+			   DCU_MODE_DCU_MODE(DCU_MODE_NORMAL));
+	regmap_write(fsl_dev->regmap,
+		     DCU_UPDATE_MODE, DCU_UPDATE_MODE_READREG);
 
-set_failed:
-	dev_err(fsl_dev->dev, "set DCU register failed\n");
+	return;
 }
 
 static void
-- 
2.6.2

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

[PATCH 3/7] drm/fsl-dcu: avoid memory leak on errors

[Stefan Agner]

Improve error handling during CRTC initialization. Especially avoid
memory leaks in the primary plane initialization error path.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c  | 7 ++++++-
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c | 1 +
 2 files changed, 7 insertions(+), 1 deletion(-)

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 adf9212..b2f56e4 100644
--- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c
+++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c
@@ -143,10 +143,15 @@ int fsl_dcu_drm_crtc_create(struct fsl_dcu_drm_device *fsl_dev)
 	int ret;
 
 	primary = fsl_dcu_drm_primary_create_plane(fsl_dev->drm);
+	if (!primary)
+		return -ENOMEM;
+
 	ret = drm_crtc_init_with_planes(fsl_dev->drm, crtc, primary, NULL,
 					&fsl_dcu_drm_crtc_funcs);
-	if (ret < 0)
+	if (ret) {
+		primary->funcs->destroy(primary);
 		return ret;
+	}
 
 	drm_crtc_helper_add(crtc, &fsl_dcu_drm_crtc_helper_funcs);
 
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 92e606a..1cb6295 100644
--- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c
+++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c
@@ -184,6 +184,7 @@ static const struct drm_plane_helper_funcs fsl_dcu_drm_plane_helper_funcs = {
 static void fsl_dcu_drm_plane_destroy(struct drm_plane *plane)
 {
 	drm_plane_cleanup(plane);
+	kfree(plane);
 }
 
 static const struct drm_plane_funcs fsl_dcu_drm_plane_funcs = {
-- 
2.6.2

[PATCH 3/7] drm/fsl-dcu: avoid memory leak on errors

[Stefan Agner]

Improve error handling during CRTC initialization. Especially avoid
memory leaks in the primary plane initialization error path.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c  | 7 ++++++-
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c | 1 +
 2 files changed, 7 insertions(+), 1 deletion(-)

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 adf9212..b2f56e4 100644
--- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c
+++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c
@@ -143,10 +143,15 @@ int fsl_dcu_drm_crtc_create(struct fsl_dcu_drm_device *fsl_dev)
 	int ret;
 
 	primary = fsl_dcu_drm_primary_create_plane(fsl_dev->drm);
+	if (!primary)
+		return -ENOMEM;
+
 	ret = drm_crtc_init_with_planes(fsl_dev->drm, crtc, primary, NULL,
 					&fsl_dcu_drm_crtc_funcs);
-	if (ret < 0)
+	if (ret) {
+		primary->funcs->destroy(primary);
 		return ret;
+	}
 
 	drm_crtc_helper_add(crtc, &fsl_dcu_drm_crtc_helper_funcs);
 
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 92e606a..1cb6295 100644
--- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c
+++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c
@@ -184,6 +184,7 @@ static const struct drm_plane_helper_funcs fsl_dcu_drm_plane_helper_funcs = {
 static void fsl_dcu_drm_plane_destroy(struct drm_plane *plane)
 {
 	drm_plane_cleanup(plane);
+	kfree(plane);
 }
 
 static const struct drm_plane_funcs fsl_dcu_drm_plane_funcs = {
-- 
2.6.2

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

[PATCH 4/7] drm/fsl-dcu: handle initialization errors properly

[Stefan Agner]

If initialization fails (e.g. due to missing panel node or deferred
probe) make sure to roll-back all operations and return the error
code.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_kms.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_kms.c b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_kms.c
index 0ef5959..c564ec6 100644
--- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_kms.c
+++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_kms.c
@@ -25,6 +25,8 @@ static const struct drm_mode_config_funcs fsl_dcu_drm_mode_config_funcs = {
 
 int fsl_dcu_drm_modeset_init(struct fsl_dcu_drm_device *fsl_dev)
 {
+	int ret;
+
 	drm_mode_config_init(fsl_dev->drm);
 
 	fsl_dev->drm->mode_config.min_width = 0;
@@ -33,11 +35,25 @@ int fsl_dcu_drm_modeset_init(struct fsl_dcu_drm_device *fsl_dev)
 	fsl_dev->drm->mode_config.max_height = 2047;
 	fsl_dev->drm->mode_config.funcs = &fsl_dcu_drm_mode_config_funcs;
 
-	drm_kms_helper_poll_init(fsl_dev->drm);
-	fsl_dcu_drm_crtc_create(fsl_dev);
-	fsl_dcu_drm_encoder_create(fsl_dev, &fsl_dev->crtc);
-	fsl_dcu_drm_connector_create(fsl_dev, &fsl_dev->encoder);
+	ret = fsl_dcu_drm_crtc_create(fsl_dev);
+	if (ret)
+		return ret;
+
+	ret = fsl_dcu_drm_encoder_create(fsl_dev, &fsl_dev->crtc);
+	if (ret)
+		goto fail_encoder;
+
+	ret = fsl_dcu_drm_connector_create(fsl_dev, &fsl_dev->encoder);
+	if (ret)
+		goto fail_connector;
+
 	drm_mode_config_reset(fsl_dev->drm);
+	drm_kms_helper_poll_init(fsl_dev->drm);
 
 	return 0;
+fail_encoder:
+	fsl_dev->crtc.funcs->destroy(&fsl_dev->crtc);
+fail_connector:
+	fsl_dev->encoder.funcs->destroy(&fsl_dev->encoder);
+	return ret;
 }
-- 
2.6.2

[PATCH 4/7] drm/fsl-dcu: handle initialization errors properly

[Stefan Agner]

If initialization fails (e.g. due to missing panel node or deferred
probe) make sure to roll-back all operations and return the error
code.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_kms.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_kms.c b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_kms.c
index 0ef5959..c564ec6 100644
--- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_kms.c
+++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_kms.c
@@ -25,6 +25,8 @@ static const struct drm_mode_config_funcs fsl_dcu_drm_mode_config_funcs = {
 
 int fsl_dcu_drm_modeset_init(struct fsl_dcu_drm_device *fsl_dev)
 {
+	int ret;
+
 	drm_mode_config_init(fsl_dev->drm);
 
 	fsl_dev->drm->mode_config.min_width = 0;
@@ -33,11 +35,25 @@ int fsl_dcu_drm_modeset_init(struct fsl_dcu_drm_device *fsl_dev)
 	fsl_dev->drm->mode_config.max_height = 2047;
 	fsl_dev->drm->mode_config.funcs = &fsl_dcu_drm_mode_config_funcs;
 
-	drm_kms_helper_poll_init(fsl_dev->drm);
-	fsl_dcu_drm_crtc_create(fsl_dev);
-	fsl_dcu_drm_encoder_create(fsl_dev, &fsl_dev->crtc);
-	fsl_dcu_drm_connector_create(fsl_dev, &fsl_dev->encoder);
+	ret = fsl_dcu_drm_crtc_create(fsl_dev);
+	if (ret)
+		return ret;
+
+	ret = fsl_dcu_drm_encoder_create(fsl_dev, &fsl_dev->crtc);
+	if (ret)
+		goto fail_encoder;
+
+	ret = fsl_dcu_drm_connector_create(fsl_dev, &fsl_dev->encoder);
+	if (ret)
+		goto fail_connector;
+
 	drm_mode_config_reset(fsl_dev->drm);
+	drm_kms_helper_poll_init(fsl_dev->drm);
 
 	return 0;
+fail_encoder:
+	fsl_dev->crtc.funcs->destroy(&fsl_dev->crtc);
+fail_connector:
+	fsl_dev->encoder.funcs->destroy(&fsl_dev->encoder);
+	return ret;
 }
-- 
2.6.2

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

[PATCH 5/7] drm/fsl-dcu: mask all interrupts on initialization

[Stefan Agner]

The state of the interrupt mask register on initialization is
unknown, e.g. U-Boot could already used the DCU. So depending on
the boot loader, the outcome of the interrupt mask register could
be different. A defined state is much more preferable. Also, there
is no value in keeping interrupts enabled which we don't need.
Therefor, mask all interrupts on initialization.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c | 5 +----
 1 file changed, 1 insertion(+), 4 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 d37ff0e..e01c813 100644
--- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
+++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
@@ -48,7 +48,6 @@ 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);
@@ -56,9 +55,7 @@ static int fsl_dcu_drm_irq_init(struct drm_device *dev)
 		dev_err(dev->dev, "failed to install IRQ handler\n");
 
 	regmap_write(fsl_dev->regmap, DCU_INT_STATUS, 0);
-	regmap_read(fsl_dev->regmap, DCU_INT_MASK, &value);
-	value &= DCU_INT_MASK_VBLANK;
-	regmap_write(fsl_dev->regmap, DCU_INT_MASK, value);
+	regmap_write(fsl_dev->regmap, DCU_INT_MASK, ~0);
 	regmap_write(fsl_dev->regmap, DCU_UPDATE_MODE,
 		     DCU_UPDATE_MODE_READREG);
 
-- 
2.6.2

[PATCH 5/7] drm/fsl-dcu: mask all interrupts on initialization

[Stefan Agner]

The state of the interrupt mask register on initialization is
unknown, e.g. U-Boot could already used the DCU. So depending on
the boot loader, the outcome of the interrupt mask register could
be different. A defined state is much more preferable. Also, there
is no value in keeping interrupts enabled which we don't need.
Therefor, mask all interrupts on initialization.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c | 5 +----
 1 file changed, 1 insertion(+), 4 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 d37ff0e..e01c813 100644
--- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
+++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
@@ -48,7 +48,6 @@ 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);
@@ -56,9 +55,7 @@ static int fsl_dcu_drm_irq_init(struct drm_device *dev)
 		dev_err(dev->dev, "failed to install IRQ handler\n");
 
 	regmap_write(fsl_dev->regmap, DCU_INT_STATUS, 0);
-	regmap_read(fsl_dev->regmap, DCU_INT_MASK, &value);
-	value &= DCU_INT_MASK_VBLANK;
-	regmap_write(fsl_dev->regmap, DCU_INT_MASK, value);
+	regmap_write(fsl_dev->regmap, DCU_INT_MASK, ~0);
 	regmap_write(fsl_dev->regmap, DCU_UPDATE_MODE,
 		     DCU_UPDATE_MODE_READREG);
 
-- 
2.6.2

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

[PATCH 6/7] drm/fsl-dcu: fix alpha blending

[Stefan Agner]

Fix alpha blending by enabling alpha blending for the whole frame if
a color mode with alpha channel is selected (DRM_FORMAT_ARGB*). Also
support color modes without alpha channel (DRM_FORMAT_XRGB*) by just
not enabling alpha blending on layer level.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h   |  4 +++-
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c | 31 +++++++++++++++++++----------
 2 files changed, 23 insertions(+), 12 deletions(-)

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 579b9e4..6413ac9 100644
--- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
+++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
@@ -133,7 +133,9 @@
 #define DCU_LAYER_RLE_EN		BIT(15)
 #define DCU_LAYER_LUOFFS(x)		((x) << 4)
 #define DCU_LAYER_BB_ON			BIT(2)
-#define DCU_LAYER_AB(x)			(x)
+#define DCU_LAYER_AB_NONE		0
+#define DCU_LAYER_AB_CHROMA_KEYING	1
+#define DCU_LAYER_AB_WHOLE_FRAME	2
 
 #define DCU_LAYER_CKMAX_R(x)		((x) << 16)
 #define DCU_LAYER_CKMAX_G(x)		((x) << 8)
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 1cb6295..feb7986 100644
--- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c
+++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c
@@ -47,8 +47,11 @@ static int fsl_dcu_drm_plane_atomic_check(struct drm_plane *plane,
 	switch (fb->pixel_format) {
 	case DRM_FORMAT_RGB565:
 	case DRM_FORMAT_RGB888:
+	case DRM_FORMAT_XRGB8888:
 	case DRM_FORMAT_ARGB8888:
-	case DRM_FORMAT_BGRA4444:
+	case DRM_FORMAT_XRGB4444:
+	case DRM_FORMAT_ARGB4444:
+	case DRM_FORMAT_XRGB1555:
 	case DRM_FORMAT_ARGB1555:
 	case DRM_FORMAT_YUV422:
 		return 0;
@@ -81,7 +84,7 @@ static void fsl_dcu_drm_plane_atomic_update(struct drm_plane *plane,
 	struct drm_plane_state *state = plane->state;
 	struct drm_framebuffer *fb = plane->state->fb;
 	struct drm_gem_cma_object *gem;
-	unsigned int alpha, bpp;
+	unsigned int alpha = DCU_LAYER_AB_NONE, bpp;
 	int index;
 
 	index = fsl_dcu_drm_plane_index(plane);
@@ -93,27 +96,30 @@ static void fsl_dcu_drm_plane_atomic_update(struct drm_plane *plane,
 	switch (fb->pixel_format) {
 	case DRM_FORMAT_RGB565:
 		bpp = FSL_DCU_RGB565;
-		alpha = 0xff;
 		break;
 	case DRM_FORMAT_RGB888:
 		bpp = FSL_DCU_RGB888;
-		alpha = 0xff;
 		break;
 	case DRM_FORMAT_ARGB8888:
+		alpha = DCU_LAYER_AB_WHOLE_FRAME;
+		/* fall-through */
+	case DRM_FORMAT_XRGB8888:
 		bpp = FSL_DCU_ARGB8888;
-		alpha = 0xff;
 		break;
-	case DRM_FORMAT_BGRA4444:
+	case DRM_FORMAT_ARGB4444:
+		alpha = DCU_LAYER_AB_WHOLE_FRAME;
+		/* fall-through */
+	case DRM_FORMAT_XRGB4444:
 		bpp = FSL_DCU_ARGB4444;
-		alpha = 0xff;
 		break;
 	case DRM_FORMAT_ARGB1555:
+		alpha = DCU_LAYER_AB_WHOLE_FRAME;
+		/* fall-through */
+	case DRM_FORMAT_XRGB1555:
 		bpp = FSL_DCU_ARGB1555;
-		alpha = 0xff;
 		break;
 	case DRM_FORMAT_YUV422:
 		bpp = FSL_DCU_YUV422;
-		alpha = 0xff;
 		break;
 	default:
 		return;
@@ -129,9 +135,9 @@ static void fsl_dcu_drm_plane_atomic_update(struct drm_plane *plane,
 		     DCU_CTRLDESCLN(index, 3), gem->paddr);
 	regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN(index, 4),
 		     DCU_LAYER_EN |
-		     DCU_LAYER_TRANS(alpha) |
+		     DCU_LAYER_TRANS(0xff) |
 		     DCU_LAYER_BPP(bpp) |
-		     DCU_LAYER_AB(0));
+		     alpha);
 	regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN(index, 5),
 		     DCU_LAYER_CKMAX_R(0xFF) |
 		     DCU_LAYER_CKMAX_G(0xFF) |
@@ -199,8 +205,11 @@ static const struct drm_plane_funcs fsl_dcu_drm_plane_funcs = {
 static const u32 fsl_dcu_drm_plane_formats[] = {
 	DRM_FORMAT_RGB565,
 	DRM_FORMAT_RGB888,
+	DRM_FORMAT_XRGB8888,
 	DRM_FORMAT_ARGB8888,
+	DRM_FORMAT_XRGB4444,
 	DRM_FORMAT_ARGB4444,
+	DRM_FORMAT_XRGB1555,
 	DRM_FORMAT_ARGB1555,
 	DRM_FORMAT_YUV422,
 };
-- 
2.6.2

[PATCH 6/7] drm/fsl-dcu: fix alpha blending

[Stefan Agner]

Fix alpha blending by enabling alpha blending for the whole frame if
a color mode with alpha channel is selected (DRM_FORMAT_ARGB*). Also
support color modes without alpha channel (DRM_FORMAT_XRGB*) by just
not enabling alpha blending on layer level.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h   |  4 +++-
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c | 31 +++++++++++++++++++----------
 2 files changed, 23 insertions(+), 12 deletions(-)

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 579b9e4..6413ac9 100644
--- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
+++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
@@ -133,7 +133,9 @@
 #define DCU_LAYER_RLE_EN		BIT(15)
 #define DCU_LAYER_LUOFFS(x)		((x) << 4)
 #define DCU_LAYER_BB_ON			BIT(2)
-#define DCU_LAYER_AB(x)			(x)
+#define DCU_LAYER_AB_NONE		0
+#define DCU_LAYER_AB_CHROMA_KEYING	1
+#define DCU_LAYER_AB_WHOLE_FRAME	2
 
 #define DCU_LAYER_CKMAX_R(x)		((x) << 16)
 #define DCU_LAYER_CKMAX_G(x)		((x) << 8)
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 1cb6295..feb7986 100644
--- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c
+++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c
@@ -47,8 +47,11 @@ static int fsl_dcu_drm_plane_atomic_check(struct drm_plane *plane,
 	switch (fb->pixel_format) {
 	case DRM_FORMAT_RGB565:
 	case DRM_FORMAT_RGB888:
+	case DRM_FORMAT_XRGB8888:
 	case DRM_FORMAT_ARGB8888:
-	case DRM_FORMAT_BGRA4444:
+	case DRM_FORMAT_XRGB4444:
+	case DRM_FORMAT_ARGB4444:
+	case DRM_FORMAT_XRGB1555:
 	case DRM_FORMAT_ARGB1555:
 	case DRM_FORMAT_YUV422:
 		return 0;
@@ -81,7 +84,7 @@ static void fsl_dcu_drm_plane_atomic_update(struct drm_plane *plane,
 	struct drm_plane_state *state = plane->state;
 	struct drm_framebuffer *fb = plane->state->fb;
 	struct drm_gem_cma_object *gem;
-	unsigned int alpha, bpp;
+	unsigned int alpha = DCU_LAYER_AB_NONE, bpp;
 	int index;
 
 	index = fsl_dcu_drm_plane_index(plane);
@@ -93,27 +96,30 @@ static void fsl_dcu_drm_plane_atomic_update(struct drm_plane *plane,
 	switch (fb->pixel_format) {
 	case DRM_FORMAT_RGB565:
 		bpp = FSL_DCU_RGB565;
-		alpha = 0xff;
 		break;
 	case DRM_FORMAT_RGB888:
 		bpp = FSL_DCU_RGB888;
-		alpha = 0xff;
 		break;
 	case DRM_FORMAT_ARGB8888:
+		alpha = DCU_LAYER_AB_WHOLE_FRAME;
+		/* fall-through */
+	case DRM_FORMAT_XRGB8888:
 		bpp = FSL_DCU_ARGB8888;
-		alpha = 0xff;
 		break;
-	case DRM_FORMAT_BGRA4444:
+	case DRM_FORMAT_ARGB4444:
+		alpha = DCU_LAYER_AB_WHOLE_FRAME;
+		/* fall-through */
+	case DRM_FORMAT_XRGB4444:
 		bpp = FSL_DCU_ARGB4444;
-		alpha = 0xff;
 		break;
 	case DRM_FORMAT_ARGB1555:
+		alpha = DCU_LAYER_AB_WHOLE_FRAME;
+		/* fall-through */
+	case DRM_FORMAT_XRGB1555:
 		bpp = FSL_DCU_ARGB1555;
-		alpha = 0xff;
 		break;
 	case DRM_FORMAT_YUV422:
 		bpp = FSL_DCU_YUV422;
-		alpha = 0xff;
 		break;
 	default:
 		return;
@@ -129,9 +135,9 @@ static void fsl_dcu_drm_plane_atomic_update(struct drm_plane *plane,
 		     DCU_CTRLDESCLN(index, 3), gem->paddr);
 	regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN(index, 4),
 		     DCU_LAYER_EN |
-		     DCU_LAYER_TRANS(alpha) |
+		     DCU_LAYER_TRANS(0xff) |
 		     DCU_LAYER_BPP(bpp) |
-		     DCU_LAYER_AB(0));
+		     alpha);
 	regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN(index, 5),
 		     DCU_LAYER_CKMAX_R(0xFF) |
 		     DCU_LAYER_CKMAX_G(0xFF) |
@@ -199,8 +205,11 @@ static const struct drm_plane_funcs fsl_dcu_drm_plane_funcs = {
 static const u32 fsl_dcu_drm_plane_formats[] = {
 	DRM_FORMAT_RGB565,
 	DRM_FORMAT_RGB888,
+	DRM_FORMAT_XRGB8888,
 	DRM_FORMAT_ARGB8888,
+	DRM_FORMAT_XRGB4444,
 	DRM_FORMAT_ARGB4444,
+	DRM_FORMAT_XRGB1555,
 	DRM_FORMAT_ARGB1555,
 	DRM_FORMAT_YUV422,
 };
-- 
2.6.2

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

[PATCH 7/7] drm/fsl-dcu: use mode flags for hsync/vsync pixelclk polarity

[Stefan Agner]

The current default configuration is as follows:
- Display samples data on the falling edge
- Invert VSYNC signal (active LOW)
- Invert HSYNC signal (active LOW)

The mode flags allow to specify the required polarity per
display. Furthermore, none of the current driver settings is
actually a standard polarity.

This patch applies the current driver standard polarities as
explicit flags to the display which has been introduced with
the driver (NEC WQVGA "nec,nl4827hc19-05b"). The driver now
also parses the flags field and applies the configuration
accordingly, by using the following values as defaults (e.g.
if no flags are specified):
- Display samples data on the rising edge
- VSYNC signal not inverted (active HIGH)
- HSYNC signal not inverted (active HIGH)

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c | 16 +++++++++++++---
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h  |  4 ++--
 drivers/gpu/drm/panel/panel-simple.c       |  2 ++
 3 files changed, 17 insertions(+), 5 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 b2f56e4..db69725 100644
--- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c
+++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c
@@ -18,6 +18,8 @@
 #include <drm/drm_crtc.h>
 #include <drm/drm_crtc_helper.h>
 
+#include <video/display_timing.h>
+
 #include "fsl_dcu_drm_crtc.h"
 #include "fsl_dcu_drm_drv.h"
 #include "fsl_dcu_drm_plane.h"
@@ -74,7 +76,7 @@ static void fsl_dcu_drm_crtc_mode_set_nofb(struct drm_crtc *crtc)
 	struct drm_device *dev = crtc->dev;
 	struct fsl_dcu_drm_device *fsl_dev = dev->dev_private;
 	struct drm_display_mode *mode = &crtc->state->mode;
-	unsigned int hbp, hfp, hsw, vbp, vfp, vsw, div, index;
+	unsigned int hbp, hfp, hsw, vbp, vfp, vsw, div, index, pol = 0;
 	unsigned long dcuclk;
 
 	index = drm_crtc_index(crtc);
@@ -89,6 +91,15 @@ static void fsl_dcu_drm_crtc_mode_set_nofb(struct drm_crtc *crtc)
 	vfp = mode->vsync_start - mode->vdisplay;
 	vsw = mode->vsync_end - mode->vsync_start;
 
+	if (!(mode->flags & DISPLAY_FLAGS_PIXDATA_POSEDGE))
+		pol |= DCU_SYN_POL_INV_PXCK_FALL;
+
+	if (mode->flags & DRM_MODE_FLAG_NHSYNC)
+		pol |= DCU_SYN_POL_INV_HS_LOW;
+
+	if (mode->flags & DRM_MODE_FLAG_NHSYNC)
+		pol |= DCU_SYN_POL_INV_VS_LOW;
+
 	regmap_write(fsl_dev->regmap, DCU_HSYN_PARA,
 		     DCU_HSYN_PARA_BP(hbp) |
 		     DCU_HSYN_PARA_PW(hsw) |
@@ -101,8 +112,7 @@ static void fsl_dcu_drm_crtc_mode_set_nofb(struct drm_crtc *crtc)
 		     DCU_DISP_SIZE_DELTA_Y(mode->vdisplay) |
 		     DCU_DISP_SIZE_DELTA_X(mode->hdisplay));
 	regmap_write(fsl_dev->regmap, DCU_DIV_RATIO, div);
-	regmap_write(fsl_dev->regmap, DCU_SYN_POL,
-		     DCU_SYN_POL_INV_VS_LOW | DCU_SYN_POL_INV_HS_LOW);
+	regmap_write(fsl_dev->regmap, DCU_SYN_POL, pol);
 	regmap_write(fsl_dev->regmap, DCU_BGND, DCU_BGND_R(0) |
 		     DCU_BGND_G(0) | DCU_BGND_B(0));
 	regmap_write(fsl_dev->regmap, DCU_DCU_MODE,
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 6413ac9..2a724f3 100644
--- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
+++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
@@ -47,8 +47,8 @@
 #define DCU_VSYN_PARA_FP(x)		(x)
 
 #define DCU_SYN_POL			0x0024
-#define DCU_SYN_POL_INV_PXCK_FALL	(0 << 6)
-#define DCU_SYN_POL_NEG_REMAIN		(0 << 5)
+#define DCU_SYN_POL_INV_PXCK_FALL	BIT(6)
+#define DCU_SYN_POL_NEG_REMAIN		BIT(5)
 #define DCU_SYN_POL_INV_VS_LOW		BIT(1)
 #define DCU_SYN_POL_INV_HS_LOW		BIT(0)
 
diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index f97b73e..fa68b56 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -960,6 +960,8 @@ static const struct drm_display_mode nec_nl4827hc19_05b_mode = {
 	.vsync_end = 272 + 2 + 4,
 	.vtotal = 272 + 2 + 4 + 2,
 	.vrefresh = 74,
+	.flags = DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC |
+		 DISPLAY_FLAGS_PIXDATA_POSEDGE,
 };
 
 static const struct panel_desc nec_nl4827hc19_05b = {
-- 
2.6.2

[PATCH 7/7] drm/fsl-dcu: use mode flags for hsync/vsync pixelclk polarity

[Stefan Agner]

The current default configuration is as follows:
- Display samples data on the falling edge
- Invert VSYNC signal (active LOW)
- Invert HSYNC signal (active LOW)

The mode flags allow to specify the required polarity per
display. Furthermore, none of the current driver settings is
actually a standard polarity.

This patch applies the current driver standard polarities as
explicit flags to the display which has been introduced with
the driver (NEC WQVGA "nec,nl4827hc19-05b"). The driver now
also parses the flags field and applies the configuration
accordingly, by using the following values as defaults (e.g.
if no flags are specified):
- Display samples data on the rising edge
- VSYNC signal not inverted (active HIGH)
- HSYNC signal not inverted (active HIGH)

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c | 16 +++++++++++++---
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h  |  4 ++--
 drivers/gpu/drm/panel/panel-simple.c       |  2 ++
 3 files changed, 17 insertions(+), 5 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 b2f56e4..db69725 100644
--- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c
+++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c
@@ -18,6 +18,8 @@
 #include <drm/drm_crtc.h>
 #include <drm/drm_crtc_helper.h>
 
+#include <video/display_timing.h>
+
 #include "fsl_dcu_drm_crtc.h"
 #include "fsl_dcu_drm_drv.h"
 #include "fsl_dcu_drm_plane.h"
@@ -74,7 +76,7 @@ static void fsl_dcu_drm_crtc_mode_set_nofb(struct drm_crtc *crtc)
 	struct drm_device *dev = crtc->dev;
 	struct fsl_dcu_drm_device *fsl_dev = dev->dev_private;
 	struct drm_display_mode *mode = &crtc->state->mode;
-	unsigned int hbp, hfp, hsw, vbp, vfp, vsw, div, index;
+	unsigned int hbp, hfp, hsw, vbp, vfp, vsw, div, index, pol = 0;
 	unsigned long dcuclk;
 
 	index = drm_crtc_index(crtc);
@@ -89,6 +91,15 @@ static void fsl_dcu_drm_crtc_mode_set_nofb(struct drm_crtc *crtc)
 	vfp = mode->vsync_start - mode->vdisplay;
 	vsw = mode->vsync_end - mode->vsync_start;
 
+	if (!(mode->flags & DISPLAY_FLAGS_PIXDATA_POSEDGE))
+		pol |= DCU_SYN_POL_INV_PXCK_FALL;
+
+	if (mode->flags & DRM_MODE_FLAG_NHSYNC)
+		pol |= DCU_SYN_POL_INV_HS_LOW;
+
+	if (mode->flags & DRM_MODE_FLAG_NHSYNC)
+		pol |= DCU_SYN_POL_INV_VS_LOW;
+
 	regmap_write(fsl_dev->regmap, DCU_HSYN_PARA,
 		     DCU_HSYN_PARA_BP(hbp) |
 		     DCU_HSYN_PARA_PW(hsw) |
@@ -101,8 +112,7 @@ static void fsl_dcu_drm_crtc_mode_set_nofb(struct drm_crtc *crtc)
 		     DCU_DISP_SIZE_DELTA_Y(mode->vdisplay) |
 		     DCU_DISP_SIZE_DELTA_X(mode->hdisplay));
 	regmap_write(fsl_dev->regmap, DCU_DIV_RATIO, div);
-	regmap_write(fsl_dev->regmap, DCU_SYN_POL,
-		     DCU_SYN_POL_INV_VS_LOW | DCU_SYN_POL_INV_HS_LOW);
+	regmap_write(fsl_dev->regmap, DCU_SYN_POL, pol);
 	regmap_write(fsl_dev->regmap, DCU_BGND, DCU_BGND_R(0) |
 		     DCU_BGND_G(0) | DCU_BGND_B(0));
 	regmap_write(fsl_dev->regmap, DCU_DCU_MODE,
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 6413ac9..2a724f3 100644
--- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
+++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
@@ -47,8 +47,8 @@
 #define DCU_VSYN_PARA_FP(x)		(x)
 
 #define DCU_SYN_POL			0x0024
-#define DCU_SYN_POL_INV_PXCK_FALL	(0 << 6)
-#define DCU_SYN_POL_NEG_REMAIN		(0 << 5)
+#define DCU_SYN_POL_INV_PXCK_FALL	BIT(6)
+#define DCU_SYN_POL_NEG_REMAIN		BIT(5)
 #define DCU_SYN_POL_INV_VS_LOW		BIT(1)
 #define DCU_SYN_POL_INV_HS_LOW		BIT(0)
 
diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index f97b73e..fa68b56 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -960,6 +960,8 @@ static const struct drm_display_mode nec_nl4827hc19_05b_mode = {
 	.vsync_end = 272 + 2 + 4,
 	.vtotal = 272 + 2 + 4 + 2,
 	.vrefresh = 74,
+	.flags = DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC |
+		 DISPLAY_FLAGS_PIXDATA_POSEDGE,
 };
 
 static const struct panel_desc nec_nl4827hc19_05b = {
-- 
2.6.2

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

Re: [PATCH 7/7] drm/fsl-dcu: use mode flags for hsync/vsync pixelclk polarity

[Stefan Agner]

Hi Dave, Hi Thierry,

Not sure how to handle this patch: it contains a little change in
panel-simple.c. I think this should be in one patch, since it changes
the associated logic in the driver... Can I send that through my tree?

--
Stefan

On 2015-11-18 18:42, Stefan Agner wrote:
> The current default configuration is as follows:
> - Display samples data on the falling edge
> - Invert VSYNC signal (active LOW)
> - Invert HSYNC signal (active LOW)
> 
> The mode flags allow to specify the required polarity per
> display. Furthermore, none of the current driver settings is
> actually a standard polarity.
> 
> This patch applies the current driver standard polarities as
> explicit flags to the display which has been introduced with
> the driver (NEC WQVGA "nec,nl4827hc19-05b"). The driver now
> also parses the flags field and applies the configuration
> accordingly, by using the following values as defaults (e.g.
> if no flags are specified):
> - Display samples data on the rising edge
> - VSYNC signal not inverted (active HIGH)
> - HSYNC signal not inverted (active HIGH)
> 
> Signed-off-by: Stefan Agner <stefan@agner.ch>
> ---
>  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c | 16 +++++++++++++---
>  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h  |  4 ++--
>  drivers/gpu/drm/panel/panel-simple.c       |  2 ++
>  3 files changed, 17 insertions(+), 5 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 b2f56e4..db69725 100644
> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c
> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c
> @@ -18,6 +18,8 @@
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_crtc_helper.h>
>  
> +#include <video/display_timing.h>
> +
>  #include "fsl_dcu_drm_crtc.h"
>  #include "fsl_dcu_drm_drv.h"
>  #include "fsl_dcu_drm_plane.h"
> @@ -74,7 +76,7 @@ static void fsl_dcu_drm_crtc_mode_set_nofb(struct
> drm_crtc *crtc)
>  	struct drm_device *dev = crtc->dev;
>  	struct fsl_dcu_drm_device *fsl_dev = dev->dev_private;
>  	struct drm_display_mode *mode = &crtc->state->mode;
> -	unsigned int hbp, hfp, hsw, vbp, vfp, vsw, div, index;
> +	unsigned int hbp, hfp, hsw, vbp, vfp, vsw, div, index, pol = 0;
>  	unsigned long dcuclk;
>  
>  	index = drm_crtc_index(crtc);
> @@ -89,6 +91,15 @@ static void fsl_dcu_drm_crtc_mode_set_nofb(struct
> drm_crtc *crtc)
>  	vfp = mode->vsync_start - mode->vdisplay;
>  	vsw = mode->vsync_end - mode->vsync_start;
>  
> +	if (!(mode->flags & DISPLAY_FLAGS_PIXDATA_POSEDGE))
> +		pol |= DCU_SYN_POL_INV_PXCK_FALL;
> +
> +	if (mode->flags & DRM_MODE_FLAG_NHSYNC)
> +		pol |= DCU_SYN_POL_INV_HS_LOW;
> +
> +	if (mode->flags & DRM_MODE_FLAG_NHSYNC)
> +		pol |= DCU_SYN_POL_INV_VS_LOW;
> +
>  	regmap_write(fsl_dev->regmap, DCU_HSYN_PARA,
>  		     DCU_HSYN_PARA_BP(hbp) |
>  		     DCU_HSYN_PARA_PW(hsw) |
> @@ -101,8 +112,7 @@ static void fsl_dcu_drm_crtc_mode_set_nofb(struct
> drm_crtc *crtc)
>  		     DCU_DISP_SIZE_DELTA_Y(mode->vdisplay) |
>  		     DCU_DISP_SIZE_DELTA_X(mode->hdisplay));
>  	regmap_write(fsl_dev->regmap, DCU_DIV_RATIO, div);
> -	regmap_write(fsl_dev->regmap, DCU_SYN_POL,
> -		     DCU_SYN_POL_INV_VS_LOW | DCU_SYN_POL_INV_HS_LOW);
> +	regmap_write(fsl_dev->regmap, DCU_SYN_POL, pol);
>  	regmap_write(fsl_dev->regmap, DCU_BGND, DCU_BGND_R(0) |
>  		     DCU_BGND_G(0) | DCU_BGND_B(0));
>  	regmap_write(fsl_dev->regmap, DCU_DCU_MODE,
> 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 6413ac9..2a724f3 100644
> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
> @@ -47,8 +47,8 @@
>  #define DCU_VSYN_PARA_FP(x)		(x)
>  
>  #define DCU_SYN_POL			0x0024
> -#define DCU_SYN_POL_INV_PXCK_FALL	(0 << 6)
> -#define DCU_SYN_POL_NEG_REMAIN		(0 << 5)
> +#define DCU_SYN_POL_INV_PXCK_FALL	BIT(6)
> +#define DCU_SYN_POL_NEG_REMAIN		BIT(5)
>  #define DCU_SYN_POL_INV_VS_LOW		BIT(1)
>  #define DCU_SYN_POL_INV_HS_LOW		BIT(0)
>  
> diff --git a/drivers/gpu/drm/panel/panel-simple.c
> b/drivers/gpu/drm/panel/panel-simple.c
> index f97b73e..fa68b56 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -960,6 +960,8 @@ static const struct drm_display_mode
> nec_nl4827hc19_05b_mode = {
>  	.vsync_end = 272 + 2 + 4,
>  	.vtotal = 272 + 2 + 4 + 2,
>  	.vrefresh = 74,
> +	.flags = DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC |
> +		 DISPLAY_FLAGS_PIXDATA_POSEDGE,
>  };
>  
>  static const struct panel_desc nec_nl4827hc19_05b = {

Re: [PATCH 7/7] drm/fsl-dcu: use mode flags for hsync/vsync pixelclk polarity

[Stefan Agner]

Hi Dave, Hi Thierry,

Not sure how to handle this patch: it contains a little change in
panel-simple.c. I think this should be in one patch, since it changes
the associated logic in the driver... Can I send that through my tree?

--
Stefan

On 2015-11-18 18:42, Stefan Agner wrote:
> The current default configuration is as follows:
> - Display samples data on the falling edge
> - Invert VSYNC signal (active LOW)
> - Invert HSYNC signal (active LOW)
> 
> The mode flags allow to specify the required polarity per
> display. Furthermore, none of the current driver settings is
> actually a standard polarity.
> 
> This patch applies the current driver standard polarities as
> explicit flags to the display which has been introduced with
> the driver (NEC WQVGA "nec,nl4827hc19-05b"). The driver now
> also parses the flags field and applies the configuration
> accordingly, by using the following values as defaults (e.g.
> if no flags are specified):
> - Display samples data on the rising edge
> - VSYNC signal not inverted (active HIGH)
> - HSYNC signal not inverted (active HIGH)
> 
> Signed-off-by: Stefan Agner <stefan@agner.ch>
> ---
>  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c | 16 +++++++++++++---
>  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h  |  4 ++--
>  drivers/gpu/drm/panel/panel-simple.c       |  2 ++
>  3 files changed, 17 insertions(+), 5 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 b2f56e4..db69725 100644
> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c
> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c
> @@ -18,6 +18,8 @@
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_crtc_helper.h>
>  
> +#include <video/display_timing.h>
> +
>  #include "fsl_dcu_drm_crtc.h"
>  #include "fsl_dcu_drm_drv.h"
>  #include "fsl_dcu_drm_plane.h"
> @@ -74,7 +76,7 @@ static void fsl_dcu_drm_crtc_mode_set_nofb(struct
> drm_crtc *crtc)
>  	struct drm_device *dev = crtc->dev;
>  	struct fsl_dcu_drm_device *fsl_dev = dev->dev_private;
>  	struct drm_display_mode *mode = &crtc->state->mode;
> -	unsigned int hbp, hfp, hsw, vbp, vfp, vsw, div, index;
> +	unsigned int hbp, hfp, hsw, vbp, vfp, vsw, div, index, pol = 0;
>  	unsigned long dcuclk;
>  
>  	index = drm_crtc_index(crtc);
> @@ -89,6 +91,15 @@ static void fsl_dcu_drm_crtc_mode_set_nofb(struct
> drm_crtc *crtc)
>  	vfp = mode->vsync_start - mode->vdisplay;
>  	vsw = mode->vsync_end - mode->vsync_start;
>  
> +	if (!(mode->flags & DISPLAY_FLAGS_PIXDATA_POSEDGE))
> +		pol |= DCU_SYN_POL_INV_PXCK_FALL;
> +
> +	if (mode->flags & DRM_MODE_FLAG_NHSYNC)
> +		pol |= DCU_SYN_POL_INV_HS_LOW;
> +
> +	if (mode->flags & DRM_MODE_FLAG_NHSYNC)
> +		pol |= DCU_SYN_POL_INV_VS_LOW;
> +
>  	regmap_write(fsl_dev->regmap, DCU_HSYN_PARA,
>  		     DCU_HSYN_PARA_BP(hbp) |
>  		     DCU_HSYN_PARA_PW(hsw) |
> @@ -101,8 +112,7 @@ static void fsl_dcu_drm_crtc_mode_set_nofb(struct
> drm_crtc *crtc)
>  		     DCU_DISP_SIZE_DELTA_Y(mode->vdisplay) |
>  		     DCU_DISP_SIZE_DELTA_X(mode->hdisplay));
>  	regmap_write(fsl_dev->regmap, DCU_DIV_RATIO, div);
> -	regmap_write(fsl_dev->regmap, DCU_SYN_POL,
> -		     DCU_SYN_POL_INV_VS_LOW | DCU_SYN_POL_INV_HS_LOW);
> +	regmap_write(fsl_dev->regmap, DCU_SYN_POL, pol);
>  	regmap_write(fsl_dev->regmap, DCU_BGND, DCU_BGND_R(0) |
>  		     DCU_BGND_G(0) | DCU_BGND_B(0));
>  	regmap_write(fsl_dev->regmap, DCU_DCU_MODE,
> 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 6413ac9..2a724f3 100644
> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
> @@ -47,8 +47,8 @@
>  #define DCU_VSYN_PARA_FP(x)		(x)
>  
>  #define DCU_SYN_POL			0x0024
> -#define DCU_SYN_POL_INV_PXCK_FALL	(0 << 6)
> -#define DCU_SYN_POL_NEG_REMAIN		(0 << 5)
> +#define DCU_SYN_POL_INV_PXCK_FALL	BIT(6)
> +#define DCU_SYN_POL_NEG_REMAIN		BIT(5)
>  #define DCU_SYN_POL_INV_VS_LOW		BIT(1)
>  #define DCU_SYN_POL_INV_HS_LOW		BIT(0)
>  
> diff --git a/drivers/gpu/drm/panel/panel-simple.c
> b/drivers/gpu/drm/panel/panel-simple.c
> index f97b73e..fa68b56 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -960,6 +960,8 @@ static const struct drm_display_mode
> nec_nl4827hc19_05b_mode = {
>  	.vsync_end = 272 + 2 + 4,
>  	.vtotal = 272 + 2 + 4 + 2,
>  	.vrefresh = 74,
> +	.flags = DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC |
> +		 DISPLAY_FLAGS_PIXDATA_POSEDGE,
>  };
>  
>  static const struct panel_desc nec_nl4827hc19_05b = {
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 7/7] drm/fsl-dcu: use mode flags for hsync/vsync pixelclk polarity

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 933 bytes --]

On Wed, Jan 27, 2016 at 06:46:50PM -0800, Stefan Agner wrote:
[...]
> > diff --git a/drivers/gpu/drm/panel/panel-simple.c
> > b/drivers/gpu/drm/panel/panel-simple.c
> > index f97b73e..fa68b56 100644
> > --- a/drivers/gpu/drm/panel/panel-simple.c
> > +++ b/drivers/gpu/drm/panel/panel-simple.c
> > @@ -960,6 +960,8 @@ static const struct drm_display_mode
> > nec_nl4827hc19_05b_mode = {
> >  	.vsync_end = 272 + 2 + 4,
> >  	.vtotal = 272 + 2 + 4 + 2,
> >  	.vrefresh = 74,
> > +	.flags = DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC |
> > +		 DISPLAY_FLAGS_PIXDATA_POSEDGE,

It doesn't seem like these two types of flags should be mixed because
they overlap. DISPLAY_FLAGS_PIXDATA_POSEDGE has the same value as the
DRM_MODE_FLAG_CSYNC define.

The definition of the DISPLAY_FLAGS_PIXDATA_POSEDGE is also not very
clear to me. I don't think we have an equivalent DRM_MODE_FLAG_* but
we could add one if there's really a need.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 7/7] drm/fsl-dcu: use mode flags for hsync/vsync pixelclk polarity

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 933 bytes --]

On Wed, Jan 27, 2016 at 06:46:50PM -0800, Stefan Agner wrote:
[...]
> > diff --git a/drivers/gpu/drm/panel/panel-simple.c
> > b/drivers/gpu/drm/panel/panel-simple.c
> > index f97b73e..fa68b56 100644
> > --- a/drivers/gpu/drm/panel/panel-simple.c
> > +++ b/drivers/gpu/drm/panel/panel-simple.c
> > @@ -960,6 +960,8 @@ static const struct drm_display_mode
> > nec_nl4827hc19_05b_mode = {
> >  	.vsync_end = 272 + 2 + 4,
> >  	.vtotal = 272 + 2 + 4 + 2,
> >  	.vrefresh = 74,
> > +	.flags = DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC |
> > +		 DISPLAY_FLAGS_PIXDATA_POSEDGE,

It doesn't seem like these two types of flags should be mixed because
they overlap. DISPLAY_FLAGS_PIXDATA_POSEDGE has the same value as the
DRM_MODE_FLAG_CSYNC define.

The definition of the DISPLAY_FLAGS_PIXDATA_POSEDGE is also not very
clear to me. I don't think we have an equivalent DRM_MODE_FLAG_* but
we could add one if there's really a need.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 7/7] drm/fsl-dcu: use mode flags for hsync/vsync pixelclk polarity

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1578 bytes --]

On Wed, Jan 27, 2016 at 06:46:50PM -0800, Stefan Agner wrote:
[...]
> On 2015-11-18 18:42, Stefan Agner wrote:
[...]
> > diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c
[...]
> > @@ -89,6 +91,15 @@ static void fsl_dcu_drm_crtc_mode_set_nofb(struct
> > drm_crtc *crtc)
> >  	vfp = mode->vsync_start - mode->vdisplay;
> >  	vsw = mode->vsync_end - mode->vsync_start;
> >  
> > +	if (!(mode->flags & DISPLAY_FLAGS_PIXDATA_POSEDGE))
> > +		pol |= DCU_SYN_POL_INV_PXCK_FALL;
> > +
> > +	if (mode->flags & DRM_MODE_FLAG_NHSYNC)
> > +		pol |= DCU_SYN_POL_INV_HS_LOW;
> > +
> > +	if (mode->flags & DRM_MODE_FLAG_NHSYNC)
> > +		pol |= DCU_SYN_POL_INV_VS_LOW;

I suspect that you want to check for DRM_MODE_FLAG_NVSYNC here instead.

> > 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 6413ac9..2a724f3 100644
> > --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
> > +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
> > @@ -47,8 +47,8 @@
> >  #define DCU_VSYN_PARA_FP(x)		(x)
> >  
> >  #define DCU_SYN_POL			0x0024
> > -#define DCU_SYN_POL_INV_PXCK_FALL	(0 << 6)
> > -#define DCU_SYN_POL_NEG_REMAIN		(0 << 5)
> > +#define DCU_SYN_POL_INV_PXCK_FALL	BIT(6)
> > +#define DCU_SYN_POL_NEG_REMAIN		BIT(5)

I don't understand these changes. You're in fact changing the values for
these defines, but it's not mentioned in the commit message. It also
seems like it should be a separate patch because it isn't related to the
mode flags changes in the remainder of the patch.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 7/7] drm/fsl-dcu: use mode flags for hsync/vsync pixelclk polarity

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1578 bytes --]

On Wed, Jan 27, 2016 at 06:46:50PM -0800, Stefan Agner wrote:
[...]
> On 2015-11-18 18:42, Stefan Agner wrote:
[...]
> > diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c
[...]
> > @@ -89,6 +91,15 @@ static void fsl_dcu_drm_crtc_mode_set_nofb(struct
> > drm_crtc *crtc)
> >  	vfp = mode->vsync_start - mode->vdisplay;
> >  	vsw = mode->vsync_end - mode->vsync_start;
> >  
> > +	if (!(mode->flags & DISPLAY_FLAGS_PIXDATA_POSEDGE))
> > +		pol |= DCU_SYN_POL_INV_PXCK_FALL;
> > +
> > +	if (mode->flags & DRM_MODE_FLAG_NHSYNC)
> > +		pol |= DCU_SYN_POL_INV_HS_LOW;
> > +
> > +	if (mode->flags & DRM_MODE_FLAG_NHSYNC)
> > +		pol |= DCU_SYN_POL_INV_VS_LOW;

I suspect that you want to check for DRM_MODE_FLAG_NVSYNC here instead.

> > 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 6413ac9..2a724f3 100644
> > --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
> > +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
> > @@ -47,8 +47,8 @@
> >  #define DCU_VSYN_PARA_FP(x)		(x)
> >  
> >  #define DCU_SYN_POL			0x0024
> > -#define DCU_SYN_POL_INV_PXCK_FALL	(0 << 6)
> > -#define DCU_SYN_POL_NEG_REMAIN		(0 << 5)
> > +#define DCU_SYN_POL_INV_PXCK_FALL	BIT(6)
> > +#define DCU_SYN_POL_NEG_REMAIN		BIT(5)

I don't understand these changes. You're in fact changing the values for
these defines, but it's not mentioned in the commit message. It also
seems like it should be a separate patch because it isn't related to the
mode flags changes in the remainder of the patch.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 7/7] drm/fsl-dcu: use mode flags for hsync/vsync pixelclk polarity

[Stefan Agner]

On 2016-02-03 06:00, Thierry Reding wrote:
> On Wed, Jan 27, 2016 at 06:46:50PM -0800, Stefan Agner wrote:
> [...]
>> > diff --git a/drivers/gpu/drm/panel/panel-simple.c
>> > b/drivers/gpu/drm/panel/panel-simple.c
>> > index f97b73e..fa68b56 100644
>> > --- a/drivers/gpu/drm/panel/panel-simple.c
>> > +++ b/drivers/gpu/drm/panel/panel-simple.c
>> > @@ -960,6 +960,8 @@ static const struct drm_display_mode
>> > nec_nl4827hc19_05b_mode = {
>> >  	.vsync_end = 272 + 2 + 4,
>> >  	.vtotal = 272 + 2 + 4 + 2,
>> >  	.vrefresh = 74,
>> > +	.flags = DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC |
>> > +		 DISPLAY_FLAGS_PIXDATA_POSEDGE,
> 
> It doesn't seem like these two types of flags should be mixed because
> they overlap. DISPLAY_FLAGS_PIXDATA_POSEDGE has the same value as the
> DRM_MODE_FLAG_CSYNC define.

There are other entries such as hannstar_hsd100pxn1_timing which make
use of DISPLAY_FLAGS too, hence I did not bother further. Having a
conflict is definitely not what we want.

> The definition of the DISPLAY_FLAGS_PIXDATA_POSEDGE is also not very
> clear to me. I don't think we have an equivalent DRM_MODE_FLAG_* but
> we could add one if there's really a need.

E.g. assuming a parallel video signal, the pixel data signals could be
changing on positive or negative edge relative to the clock signal. Most
displays _sample_ the data on rising edge, hence controllers should
normally drive data on falling edge.

However, as always, there are exceptions to the rule, and one of the
exception is Freescales default display for the Tower evaluation board.
It samples data on falling edge, hence the controller should drive data
on rising edge...

Yeah I also did not found an equivalent in DRM_MODE_FLAG_*. I have here
also other displays where we would need this flag. The display-timings
binds and enum display_flags support it too, hence I guess we should
have it here too.

I will split out this patch from the patchset (I already applied the
rest), add another patch to add the flag, make use of the flag in this
patch and resend it as v2.

--
Stefan

Re: [PATCH 7/7] drm/fsl-dcu: use mode flags for hsync/vsync pixelclk polarity

[Stefan Agner]

On 2016-02-03 06:00, Thierry Reding wrote:
> On Wed, Jan 27, 2016 at 06:46:50PM -0800, Stefan Agner wrote:
> [...]
>> > diff --git a/drivers/gpu/drm/panel/panel-simple.c
>> > b/drivers/gpu/drm/panel/panel-simple.c
>> > index f97b73e..fa68b56 100644
>> > --- a/drivers/gpu/drm/panel/panel-simple.c
>> > +++ b/drivers/gpu/drm/panel/panel-simple.c
>> > @@ -960,6 +960,8 @@ static const struct drm_display_mode
>> > nec_nl4827hc19_05b_mode = {
>> >  	.vsync_end = 272 + 2 + 4,
>> >  	.vtotal = 272 + 2 + 4 + 2,
>> >  	.vrefresh = 74,
>> > +	.flags = DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC |
>> > +		 DISPLAY_FLAGS_PIXDATA_POSEDGE,
> 
> It doesn't seem like these two types of flags should be mixed because
> they overlap. DISPLAY_FLAGS_PIXDATA_POSEDGE has the same value as the
> DRM_MODE_FLAG_CSYNC define.

There are other entries such as hannstar_hsd100pxn1_timing which make
use of DISPLAY_FLAGS too, hence I did not bother further. Having a
conflict is definitely not what we want.

> The definition of the DISPLAY_FLAGS_PIXDATA_POSEDGE is also not very
> clear to me. I don't think we have an equivalent DRM_MODE_FLAG_* but
> we could add one if there's really a need.

E.g. assuming a parallel video signal, the pixel data signals could be
changing on positive or negative edge relative to the clock signal. Most
displays _sample_ the data on rising edge, hence controllers should
normally drive data on falling edge.

However, as always, there are exceptions to the rule, and one of the
exception is Freescales default display for the Tower evaluation board.
It samples data on falling edge, hence the controller should drive data
on rising edge...

Yeah I also did not found an equivalent in DRM_MODE_FLAG_*. I have here
also other displays where we would need this flag. The display-timings
binds and enum display_flags support it too, hence I guess we should
have it here too.

I will split out this patch from the patchset (I already applied the
rest), add another patch to add the flag, make use of the flag in this
patch and resend it as v2.

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

Re: [PATCH 7/7] drm/fsl-dcu: use mode flags for hsync/vsync pixelclk polarity

[Stefan Agner]

On 2016-02-03 06:04, Thierry Reding wrote:
> On Wed, Jan 27, 2016 at 06:46:50PM -0800, Stefan Agner wrote:
> [...]
>> On 2015-11-18 18:42, Stefan Agner wrote:
> [...]
>> > diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c
> [...]
>> > @@ -89,6 +91,15 @@ static void fsl_dcu_drm_crtc_mode_set_nofb(struct
>> > drm_crtc *crtc)
>> >  	vfp = mode->vsync_start - mode->vdisplay;
>> >  	vsw = mode->vsync_end - mode->vsync_start;
>> >
>> > +	if (!(mode->flags & DISPLAY_FLAGS_PIXDATA_POSEDGE))
>> > +		pol |= DCU_SYN_POL_INV_PXCK_FALL;
>> > +
>> > +	if (mode->flags & DRM_MODE_FLAG_NHSYNC)
>> > +		pol |= DCU_SYN_POL_INV_HS_LOW;
>> > +
>> > +	if (mode->flags & DRM_MODE_FLAG_NHSYNC)
>> > +		pol |= DCU_SYN_POL_INV_VS_LOW;
> 
> I suspect that you want to check for DRM_MODE_FLAG_NVSYNC here instead.
> 

Ups, yes.

>> > 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 6413ac9..2a724f3 100644
>> > --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
>> > +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
>> > @@ -47,8 +47,8 @@
>> >  #define DCU_VSYN_PARA_FP(x)		(x)
>> >
>> >  #define DCU_SYN_POL			0x0024
>> > -#define DCU_SYN_POL_INV_PXCK_FALL	(0 << 6)
>> > -#define DCU_SYN_POL_NEG_REMAIN		(0 << 5)
>> > +#define DCU_SYN_POL_INV_PXCK_FALL	BIT(6)
>> > +#define DCU_SYN_POL_NEG_REMAIN		BIT(5)
> 
> I don't understand these changes. You're in fact changing the values for
> these defines, but it's not mentioned in the commit message. It also
> seems like it should be a separate patch because it isn't related to the
> mode flags changes in the remainder of the patch.

Yes, in order to set them, I need them to be positive, there is no use
if they are zero... They haven't been used at all so far, so there is no
issue in changing their value. Just realized that their naming is
actually related to the 0 value, so I probably should rename them to
just reflect the field name

#define DCU_SYN_POL_INV_PXCK	BIT(6)
#define DCU_SYN_POL_NEG		BIT(5)

--
Stefan

Re: [PATCH 7/7] drm/fsl-dcu: use mode flags for hsync/vsync pixelclk polarity

[Stefan Agner]

On 2016-02-03 06:04, Thierry Reding wrote:
> On Wed, Jan 27, 2016 at 06:46:50PM -0800, Stefan Agner wrote:
> [...]
>> On 2015-11-18 18:42, Stefan Agner wrote:
> [...]
>> > diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c
> [...]
>> > @@ -89,6 +91,15 @@ static void fsl_dcu_drm_crtc_mode_set_nofb(struct
>> > drm_crtc *crtc)
>> >  	vfp = mode->vsync_start - mode->vdisplay;
>> >  	vsw = mode->vsync_end - mode->vsync_start;
>> >
>> > +	if (!(mode->flags & DISPLAY_FLAGS_PIXDATA_POSEDGE))
>> > +		pol |= DCU_SYN_POL_INV_PXCK_FALL;
>> > +
>> > +	if (mode->flags & DRM_MODE_FLAG_NHSYNC)
>> > +		pol |= DCU_SYN_POL_INV_HS_LOW;
>> > +
>> > +	if (mode->flags & DRM_MODE_FLAG_NHSYNC)
>> > +		pol |= DCU_SYN_POL_INV_VS_LOW;
> 
> I suspect that you want to check for DRM_MODE_FLAG_NVSYNC here instead.
> 

Ups, yes.

>> > 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 6413ac9..2a724f3 100644
>> > --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
>> > +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
>> > @@ -47,8 +47,8 @@
>> >  #define DCU_VSYN_PARA_FP(x)		(x)
>> >
>> >  #define DCU_SYN_POL			0x0024
>> > -#define DCU_SYN_POL_INV_PXCK_FALL	(0 << 6)
>> > -#define DCU_SYN_POL_NEG_REMAIN		(0 << 5)
>> > +#define DCU_SYN_POL_INV_PXCK_FALL	BIT(6)
>> > +#define DCU_SYN_POL_NEG_REMAIN		BIT(5)
> 
> I don't understand these changes. You're in fact changing the values for
> these defines, but it's not mentioned in the commit message. It also
> seems like it should be a separate patch because it isn't related to the
> mode flags changes in the remainder of the patch.

Yes, in order to set them, I need them to be positive, there is no use
if they are zero... They haven't been used at all so far, so there is no
issue in changing their value. Just realized that their naming is
actually related to the 0 value, so I probably should rename them to
just reflect the field name

#define DCU_SYN_POL_INV_PXCK	BIT(6)
#define DCU_SYN_POL_NEG		BIT(5)

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

Re: [PATCH 7/7] drm/fsl-dcu: use mode flags for hsync/vsync pixelclk polarity

[Stefan Agner]

On 2016-02-03 15:18, Stefan Agner wrote:
> On 2016-02-03 06:00, Thierry Reding wrote:
>> On Wed, Jan 27, 2016 at 06:46:50PM -0800, Stefan Agner wrote:
>> [...]
>>> > diff --git a/drivers/gpu/drm/panel/panel-simple.c
>>> > b/drivers/gpu/drm/panel/panel-simple.c
>>> > index f97b73e..fa68b56 100644
>>> > --- a/drivers/gpu/drm/panel/panel-simple.c
>>> > +++ b/drivers/gpu/drm/panel/panel-simple.c
>>> > @@ -960,6 +960,8 @@ static const struct drm_display_mode
>>> > nec_nl4827hc19_05b_mode = {
>>> >  	.vsync_end = 272 + 2 + 4,
>>> >  	.vtotal = 272 + 2 + 4 + 2,
>>> >  	.vrefresh = 74,
>>> > +	.flags = DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC |
>>> > +		 DISPLAY_FLAGS_PIXDATA_POSEDGE,
>>
>> It doesn't seem like these two types of flags should be mixed because
>> they overlap. DISPLAY_FLAGS_PIXDATA_POSEDGE has the same value as the
>> DRM_MODE_FLAG_CSYNC define.
> 
> There are other entries such as hannstar_hsd100pxn1_timing which make
> use of DISPLAY_FLAGS too, hence I did not bother further. Having a
> conflict is definitely not what we want.

Oh, just realized, hannstar_hsd100pxn1_timing is actually a different
type of struct, and for this struct the DISPALY_FLAGS make sense...

> 
>> The definition of the DISPLAY_FLAGS_PIXDATA_POSEDGE is also not very
>> clear to me. I don't think we have an equivalent DRM_MODE_FLAG_* but
>> we could add one if there's really a need.
> 
> E.g. assuming a parallel video signal, the pixel data signals could be
> changing on positive or negative edge relative to the clock signal. Most
> displays _sample_ the data on rising edge, hence controllers should
> normally drive data on falling edge.
> 
> However, as always, there are exceptions to the rule, and one of the
> exception is Freescales default display for the Tower evaluation board.
> It samples data on falling edge, hence the controller should drive data
> on rising edge...
> 
> Yeah I also did not found an equivalent in DRM_MODE_FLAG_*. I have here
> also other displays where we would need this flag. The display-timings
> binds and enum display_flags support it too, hence I guess we should
> have it here too.
> 
> I will split out this patch from the patchset (I already applied the
> rest), add another patch to add the flag, make use of the flag in this
> patch and resend it as v2.

I realized that after this, there are only two flags which are not yet
in DRM_MODE_FLAG: DE_LOW/DE_HIGH.

Thierry, what do you think, should I add these remaining two flags too?

--
Stefan

Re: [PATCH 7/7] drm/fsl-dcu: use mode flags for hsync/vsync pixelclk polarity

[Stefan Agner]

On 2016-02-03 15:18, Stefan Agner wrote:
> On 2016-02-03 06:00, Thierry Reding wrote:
>> On Wed, Jan 27, 2016 at 06:46:50PM -0800, Stefan Agner wrote:
>> [...]
>>> > diff --git a/drivers/gpu/drm/panel/panel-simple.c
>>> > b/drivers/gpu/drm/panel/panel-simple.c
>>> > index f97b73e..fa68b56 100644
>>> > --- a/drivers/gpu/drm/panel/panel-simple.c
>>> > +++ b/drivers/gpu/drm/panel/panel-simple.c
>>> > @@ -960,6 +960,8 @@ static const struct drm_display_mode
>>> > nec_nl4827hc19_05b_mode = {
>>> >  	.vsync_end = 272 + 2 + 4,
>>> >  	.vtotal = 272 + 2 + 4 + 2,
>>> >  	.vrefresh = 74,
>>> > +	.flags = DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC |
>>> > +		 DISPLAY_FLAGS_PIXDATA_POSEDGE,
>>
>> It doesn't seem like these two types of flags should be mixed because
>> they overlap. DISPLAY_FLAGS_PIXDATA_POSEDGE has the same value as the
>> DRM_MODE_FLAG_CSYNC define.
> 
> There are other entries such as hannstar_hsd100pxn1_timing which make
> use of DISPLAY_FLAGS too, hence I did not bother further. Having a
> conflict is definitely not what we want.

Oh, just realized, hannstar_hsd100pxn1_timing is actually a different
type of struct, and for this struct the DISPALY_FLAGS make sense...

> 
>> The definition of the DISPLAY_FLAGS_PIXDATA_POSEDGE is also not very
>> clear to me. I don't think we have an equivalent DRM_MODE_FLAG_* but
>> we could add one if there's really a need.
> 
> E.g. assuming a parallel video signal, the pixel data signals could be
> changing on positive or negative edge relative to the clock signal. Most
> displays _sample_ the data on rising edge, hence controllers should
> normally drive data on falling edge.
> 
> However, as always, there are exceptions to the rule, and one of the
> exception is Freescales default display for the Tower evaluation board.
> It samples data on falling edge, hence the controller should drive data
> on rising edge...
> 
> Yeah I also did not found an equivalent in DRM_MODE_FLAG_*. I have here
> also other displays where we would need this flag. The display-timings
> binds and enum display_flags support it too, hence I guess we should
> have it here too.
> 
> I will split out this patch from the patchset (I already applied the
> rest), add another patch to add the flag, make use of the flag in this
> patch and resend it as v2.

I realized that after this, there are only two flags which are not yet
in DRM_MODE_FLAG: DE_LOW/DE_HIGH.

Thierry, what do you think, should I add these remaining two flags too?

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

Re: [PATCH 0/7] drm/fsl-dcu: fixes and enhancements

[Stefan Agner]

On 2015-11-18 18:42, Stefan Agner wrote:
> During testing the DCU DRM driver on the Freescale Vybrid platform
> I came across some (platform independent) bugs and problems which
> this patchset addresses.
> 
> Note: To use the driver on Vybrid some platform/device-tree
> enhancements are needed which are not part of this patchset.
> I still need to clean those up.

Applied 1-6 to my tree:
http://git.agner.ch/gitweb/?p=linux-drm-fsl-dcu.git;a=shortlog;h=refs/heads/for-next

Will send out a pull request for that tree soonish.

> 
> Stefan Agner (7):
>   drm/fsl-dcu: specify volatile registers
>   drm/fsl-dcu: remove regmap return value checks
>   drm/fsl-dcu: avoid memory leak on errors
>   drm/fsl-dcu: handle initialization errors properly
>   drm/fsl-dcu: mask all interrupts on initialization
>   drm/fsl-dcu: fix alpha blending
>   drm/fsl-dcu: use mode flags for hsync/vsync pixelclk polarity
> 
>  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c  | 143 +++++++++++-----------------
>  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c   |  65 ++++++-------
>  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h   |   8 +-
>  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_kms.c   |  24 ++++-
>  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c | 137 ++++++++++++--------------
>  drivers/gpu/drm/panel/panel-simple.c        |   2 +
>  6 files changed, 171 insertions(+), 208 deletions(-)

Re: [PATCH 0/7] drm/fsl-dcu: fixes and enhancements

[Stefan Agner]

On 2015-11-18 18:42, Stefan Agner wrote:
> During testing the DCU DRM driver on the Freescale Vybrid platform
> I came across some (platform independent) bugs and problems which
> this patchset addresses.
> 
> Note: To use the driver on Vybrid some platform/device-tree
> enhancements are needed which are not part of this patchset.
> I still need to clean those up.

Applied 1-6 to my tree:
http://git.agner.ch/gitweb/?p=linux-drm-fsl-dcu.git;a=shortlog;h=refs/heads/for-next

Will send out a pull request for that tree soonish.

> 
> Stefan Agner (7):
>   drm/fsl-dcu: specify volatile registers
>   drm/fsl-dcu: remove regmap return value checks
>   drm/fsl-dcu: avoid memory leak on errors
>   drm/fsl-dcu: handle initialization errors properly
>   drm/fsl-dcu: mask all interrupts on initialization
>   drm/fsl-dcu: fix alpha blending
>   drm/fsl-dcu: use mode flags for hsync/vsync pixelclk polarity
> 
>  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c  | 143 +++++++++++-----------------
>  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c   |  65 ++++++-------
>  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h   |   8 +-
>  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_kms.c   |  24 ++++-
>  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c | 137 ++++++++++++--------------
>  drivers/gpu/drm/panel/panel-simple.c        |   2 +
>  6 files changed, 171 insertions(+), 208 deletions(-)
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel