[PATCH] video IPUv1 fixes for different display connections

[PATCH] video IPUv1 fixes for different display connections

[Sascha Hauer]

Hi,

The following series fixes the display connections to the IPU on
i.MX3 boards. The connection to the display is completely independend
of the internal pixel format in the framebuffer. The driver instead
changes the IPU <-> Display mapping dependent on the pixelformat
which is wrong.
The following patch makes 16bpp and 32bpp modes possible on both
RGB666 and RGB888 connected displays.
The burstsize setting for 32bpp was configured wrong, so 32bpp
modes have obviously never been tested.

Not for stable since all in kernel boards seem to work.

Sascha Hauer (2):
      i.MX IPU DMA: Fix wrong burstsize settings
      video i.MX IPU: Fix display connections

 arch/arm/plat-mxc/include/mach/mx3fb.h |   15 ++++++++
 drivers/dma/ipu/ipu_idmac.c            |   25 +------------
 drivers/video/mx3fb.c                  |   61 +++++++++++--------------------
 3 files changed, 38 insertions(+), 63 deletions(-)

[PATCH 1/2] i.MX IPU DMA: Fix wrong burstsize settings

[Sascha Hauer]

The burstsize (npb in struct chan_param_mem) is set in
ipu_ch_param_set_size() once. The number of allowed
pixels in a burst depend on the pixel format and the
rotation mode. For 16bit formats 16 pixels are allowed
whereas for 32bit formats only 8 pixels are allowed.
Set these values correctly in ipu_ch_param_set_size()
and do not overwrite them afterwards.
We do not support rotation right now, so ignore this
case.
This patch fixes the wrong burstsize setting of 16 pixels
for 32bpp.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/dma/ipu/ipu_idmac.c |   25 +------------------------
 1 files changed, 1 insertions(+), 24 deletions(-)

diff --git a/drivers/dma/ipu/ipu_idmac.c b/drivers/dma/ipu/ipu_idmac.c
index 0e5ef33..ccdc806 100644
--- a/drivers/dma/ipu/ipu_idmac.c
+++ b/drivers/dma/ipu/ipu_idmac.c
@@ -312,7 +312,7 @@ static void ipu_ch_param_set_size(union chan_param_mem *params,
 	case IPU_PIX_FMT_RGB565:
 		params->ip.bpp	= 2;
 		params->ip.pfs	= 4;
-		params->ip.npb	= 7;
+		params->ip.npb	= 15;
 		params->ip.sat	= 2;		/* SAT = 32-bit access */
 		params->ip.ofs0	= 0;		/* Red bit offset */
 		params->ip.ofs1	= 5;		/* Green bit offset */
@@ -422,12 +422,6 @@ static void ipu_ch_param_set_size(union chan_param_mem *params,
 	params->pp.nsb = 1;
 }
 
-static void ipu_ch_param_set_burst_size(union chan_param_mem *params,
-					uint16_t burst_pixels)
-{
-	params->pp.npb = burst_pixels - 1;
-}
-
 static void ipu_ch_param_set_buffer(union chan_param_mem *params,
 				    dma_addr_t buf0, dma_addr_t buf1)
 {
@@ -690,23 +684,6 @@ static int ipu_init_channel_buffer(struct idmac_channel *ichan,
 	ipu_ch_param_set_size(&params, pixel_fmt, width, height, stride_bytes);
 	ipu_ch_param_set_buffer(&params, phyaddr_0, phyaddr_1);
 	ipu_ch_param_set_rotation(&params, rot_mode);
-	/* Some channels (rotation) have restriction on burst length */
-	switch (channel) {
-	case IDMAC_IC_7:	/* Hangs with burst 8, 16, other values
-				   invalid - Table 44-30 */
-/*
-		ipu_ch_param_set_burst_size(&params, 8);
- */
-		break;
-	case IDMAC_SDC_0:
-	case IDMAC_SDC_1:
-		/* In original code only IPU_PIX_FMT_RGB565 was setting burst */
-		ipu_ch_param_set_burst_size(&params, 16);
-		break;
-	case IDMAC_IC_0:
-	default:
-		break;
-	}
 
 	spin_lock_irqsave(&ipu->lock, flags);
 
-- 
1.7.7.1

[PATCH 2/2] video i.MX IPU: Fix display connections

[Sascha Hauer]

The IPU internally works on 32bit colors. It can arbitrarily map
between pixel formats and internal representation and also between
internal representation and the physical connection to the display.
The driver used to change the mapping between internal representation
and display connection depending on the user selected bpp which is
wrong. Instead, the mapping is specified by the hardware, so an
additional field in platform data is added to describe the connection
between i.MX and the display. The default for this field is RGB666
which seems to be the only configuration which works without this
patch, so I assumed that all in Kernel boards are connected this
way.
This patch has been tested on a RGB666 connected display and a
RGB888 connected display in both 16bpp and 32bpp modes.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 arch/arm/plat-mxc/include/mach/mx3fb.h |   15 ++++++++
 drivers/video/mx3fb.c                  |   61 +++++++++++--------------------
 2 files changed, 37 insertions(+), 39 deletions(-)

diff --git a/arch/arm/plat-mxc/include/mach/mx3fb.h b/arch/arm/plat-mxc/include/mach/mx3fb.h
index ac24c5c..fdbe600 100644
--- a/arch/arm/plat-mxc/include/mach/mx3fb.h
+++ b/arch/arm/plat-mxc/include/mach/mx3fb.h
@@ -22,6 +22,20 @@
 #define FB_SYNC_SWAP_RGB	0x04000000
 #define FB_SYNC_CLK_SEL_EN	0x02000000
 
+/*
+ * Specify the way your display is connected. The IPU can arbitrarily
+ * map the internal colors to the external data lines. We only support
+ * the following mappings at the moment.
+ */
+enum disp_data_mapping {
+	/* blue -> d[0..5], green -> d[6..11], red -> d[12..17] */
+	IPU_DISP_DATA_MAPPING_RGB666,
+	/* blue -> d[0..4], green -> d[5..10], red -> d[11..15] */
+	IPU_DISP_DATA_MAPPING_RGB565,
+	/* blue -> d[0..7], green -> d[8..15], red -> d[16..23] */
+	IPU_DISP_DATA_MAPPING_RGB888,
+};
+
 /**
  * struct mx3fb_platform_data - mx3fb platform data
  *
@@ -33,6 +47,7 @@ struct mx3fb_platform_data {
 	const char			*name;
 	const struct fb_videomode	*mode;
 	int				num_modes;
+	enum disp_data_mapping		disp_data_fmt;
 };
 
 #endif
diff --git a/drivers/video/mx3fb.c b/drivers/video/mx3fb.c
index e3406ab..f1383c8 100644
--- a/drivers/video/mx3fb.c
+++ b/drivers/video/mx3fb.c
@@ -245,6 +245,7 @@ struct mx3fb_data {
 
 	uint32_t		h_start_width;
 	uint32_t		v_start_width;
+	enum disp_data_mapping	disp_data_fmt;
 };
 
 struct dma_chan_request {
@@ -287,11 +288,14 @@ static void mx3fb_write_reg(struct mx3fb_data *mx3fb, u32 value, unsigned long r
 	__raw_writel(value, mx3fb->reg_base + reg);
 }
 
-static const uint32_t di_mappings[] = {
-	0x1600AAAA, 0x00E05555, 0x00070000, 3,	/* RGB888 */
-	0x0005000F, 0x000B000F, 0x0011000F, 1,	/* RGB666 */
-	0x0011000F, 0x000B000F, 0x0005000F, 1,	/* BGR666 */
-	0x0004003F, 0x000A000F, 0x000F003F, 1	/* RGB565 */
+struct di_mapping {
+	uint32_t b0, b1, b2;
+};
+
+static const struct di_mapping di_mappings[] = {
+	[IPU_DISP_DATA_MAPPING_RGB666] = { 0x0005000f, 0x000b000f, 0x0011000f },
+	[IPU_DISP_DATA_MAPPING_RGB565] = { 0x0004003f, 0x000a000f, 0x000f003f },
+	[IPU_DISP_DATA_MAPPING_RGB888] = { 0x00070000, 0x000f0000, 0x00170000 },
 };
 
 static void sdc_fb_init(struct mx3fb_info *fbi)
@@ -425,7 +429,6 @@ static int sdc_set_window_pos(struct mx3fb_data *mx3fb, enum ipu_channel channel
  * @pixel_clk:		desired pixel clock frequency in Hz.
  * @width:		width of panel in pixels.
  * @height:		height of panel in pixels.
- * @pixel_fmt:		pixel format of buffer as FOURCC ASCII code.
  * @h_start_width:	number of pixel clocks between the HSYNC signal pulse
  *			and the start of valid data.
  * @h_sync_width:	width of the HSYNC signal in units of pixel clocks.
@@ -442,7 +445,6 @@ static int sdc_set_window_pos(struct mx3fb_data *mx3fb, enum ipu_channel channel
 static int sdc_init_panel(struct mx3fb_data *mx3fb, enum ipu_panel panel,
 			  uint32_t pixel_clk,
 			  uint16_t width, uint16_t height,
-			  enum pixel_fmt pixel_fmt,
 			  uint16_t h_start_width, uint16_t h_sync_width,
 			  uint16_t h_end_width, uint16_t v_start_width,
 			  uint16_t v_sync_width, uint16_t v_end_width,
@@ -453,6 +455,7 @@ static int sdc_init_panel(struct mx3fb_data *mx3fb, enum ipu_panel panel,
 	uint32_t old_conf;
 	uint32_t div;
 	struct clk *ipu_clk;
+	const struct di_mapping *map;
 
 	dev_dbg(mx3fb->dev, "panel size = %d x %d", width, height);
 
@@ -540,36 +543,10 @@ static int sdc_init_panel(struct mx3fb_data *mx3fb, enum ipu_panel panel,
 		sig.Vsync_pol << DI_D3_VSYNC_POL_SHIFT;
 	mx3fb_write_reg(mx3fb, old_conf, DI_DISP_SIG_POL);
 
-	switch (pixel_fmt) {
-	case IPU_PIX_FMT_RGB24:
-		mx3fb_write_reg(mx3fb, di_mappings[0], DI_DISP3_B0_MAP);
-		mx3fb_write_reg(mx3fb, di_mappings[1], DI_DISP3_B1_MAP);
-		mx3fb_write_reg(mx3fb, di_mappings[2], DI_DISP3_B2_MAP);
-		mx3fb_write_reg(mx3fb, mx3fb_read_reg(mx3fb, DI_DISP_ACC_CC) |
-			     ((di_mappings[3] - 1) << 12), DI_DISP_ACC_CC);
-		break;
-	case IPU_PIX_FMT_RGB666:
-		mx3fb_write_reg(mx3fb, di_mappings[4], DI_DISP3_B0_MAP);
-		mx3fb_write_reg(mx3fb, di_mappings[5], DI_DISP3_B1_MAP);
-		mx3fb_write_reg(mx3fb, di_mappings[6], DI_DISP3_B2_MAP);
-		mx3fb_write_reg(mx3fb, mx3fb_read_reg(mx3fb, DI_DISP_ACC_CC) |
-			     ((di_mappings[7] - 1) << 12), DI_DISP_ACC_CC);
-		break;
-	case IPU_PIX_FMT_BGR666:
-		mx3fb_write_reg(mx3fb, di_mappings[8], DI_DISP3_B0_MAP);
-		mx3fb_write_reg(mx3fb, di_mappings[9], DI_DISP3_B1_MAP);
-		mx3fb_write_reg(mx3fb, di_mappings[10], DI_DISP3_B2_MAP);
-		mx3fb_write_reg(mx3fb, mx3fb_read_reg(mx3fb, DI_DISP_ACC_CC) |
-			     ((di_mappings[11] - 1) << 12), DI_DISP_ACC_CC);
-		break;
-	default:
-		mx3fb_write_reg(mx3fb, di_mappings[12], DI_DISP3_B0_MAP);
-		mx3fb_write_reg(mx3fb, di_mappings[13], DI_DISP3_B1_MAP);
-		mx3fb_write_reg(mx3fb, di_mappings[14], DI_DISP3_B2_MAP);
-		mx3fb_write_reg(mx3fb, mx3fb_read_reg(mx3fb, DI_DISP_ACC_CC) |
-			     ((di_mappings[15] - 1) << 12), DI_DISP_ACC_CC);
-		break;
-	}
+	map = &di_mappings[mx3fb->disp_data_fmt];
+	mx3fb_write_reg(mx3fb, map->b0, DI_DISP3_B0_MAP);
+	mx3fb_write_reg(mx3fb, map->b1, DI_DISP3_B1_MAP);
+	mx3fb_write_reg(mx3fb, map->b2, DI_DISP3_B2_MAP);
 
 	spin_unlock_irqrestore(&mx3fb->lock, lock_flags);
 
@@ -780,8 +757,6 @@ static int __set_par(struct fb_info *fbi, bool lock)
 		if (sdc_init_panel(mx3fb, mode,
 				   (PICOS2KHZ(fbi->var.pixclock)) * 1000UL,
 				   fbi->var.xres, fbi->var.yres,
-				   (fbi->var.sync & FB_SYNC_SWAP_RGB) ?
-				   IPU_PIX_FMT_BGR666 : IPU_PIX_FMT_RGB666,
 				   fbi->var.left_margin,
 				   fbi->var.hsync_len,
 				   fbi->var.right_margin +
@@ -1349,6 +1324,12 @@ static int init_fb_chan(struct mx3fb_data *mx3fb, struct idmac_channel *ichan)
 	const struct fb_videomode *mode;
 	int ret, num_modes;
 
+	if (mx3fb_pdata->disp_data_fmt >= ARRAY_SIZE(di_mappings)) {
+		dev_err(dev, "Illegal display data format %d\n",
+				mx3fb_pdata->disp_data_fmt);
+		return -EINVAL;
+	}
+
 	ichan->client = mx3fb;
 	irq = ichan->eof_irq;
 
@@ -1402,6 +1383,8 @@ static int init_fb_chan(struct mx3fb_data *mx3fb, struct idmac_channel *ichan)
 	mx3fbi->mx3fb		= mx3fb;
 	mx3fbi->blank		= FB_BLANK_NORMAL;
 
+	mx3fb->disp_data_fmt	= mx3fb_pdata->disp_data_fmt;
+
 	init_completion(&mx3fbi->flip_cmpl);
 	disable_irq(ichan->eof_irq);
 	dev_dbg(mx3fb->dev, "disabling irq %d\n", ichan->eof_irq);
-- 
1.7.7.1

Re: [PATCH] video IPUv1 fixes for different display connections

[Vinod Koul]

On Thu, 2011-12-01 at 14:58 +0100, Sascha Hauer wrote:
> Hi,
> 
> The following series fixes the display connections to the IPU on
> i.MX3 boards. The connection to the display is completely independend
> of the internal pixel format in the framebuffer. The driver instead
> changes the IPU <-> Display mapping dependent on the pixelformat
> which is wrong.
> The following patch makes 16bpp and 32bpp modes possible on both
> RGB666 and RGB888 connected displays.
> The burstsize setting for 32bpp was configured wrong, so 32bpp
> modes have obviously never been tested.
> 
> Not for stable since all in kernel boards seem to work.
Applied thanks.

Couldn't help but notice that :
@@ -312,7 +312,7 @@ static void ipu_ch_param_set_size(union
chan_param_mem *params,
        case IPU_PIX_FMT_RGB565:
                params->ip.bpp  = 2;
                params->ip.pfs  = 4;

is not really good way. Someone else who is using this driver has no way
of knowing what these magic numbers mean and how to use them
Perhaps you can add meaning to these numbers and some note on why they
should be set to these values.


-- 
~Vinod

Re: [PATCH] video IPUv1 fixes for different display connections

[Sascha Hauer]

On Thu, Dec 08, 2011 at 01:03:26PM +0530, Vinod Koul wrote:
> On Thu, 2011-12-01 at 14:58 +0100, Sascha Hauer wrote:
> > Hi,
> > 
> > The following series fixes the display connections to the IPU on
> > i.MX3 boards. The connection to the display is completely independend
> > of the internal pixel format in the framebuffer. The driver instead
> > changes the IPU <-> Display mapping dependent on the pixelformat
> > which is wrong.
> > The following patch makes 16bpp and 32bpp modes possible on both
> > RGB666 and RGB888 connected displays.
> > The burstsize setting for 32bpp was configured wrong, so 32bpp
> > modes have obviously never been tested.
> > 
> > Not for stable since all in kernel boards seem to work.
> Applied thanks.
> 
> Couldn't help but notice that :
> @@ -312,7 +312,7 @@ static void ipu_ch_param_set_size(union
> chan_param_mem *params,
>         case IPU_PIX_FMT_RGB565:
>                 params->ip.bpp  = 2;
>                 params->ip.pfs  = 4;
> 
> is not really good way. Someone else who is using this driver has no way
> of knowing what these magic numbers mean and how to use them
> Perhaps you can add meaning to these numbers and some note on why they
> should be set to these values.

Yes, you are right. Adding defines for these really makes this better
readable. BTW did you queue both patches or only the first one?

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] video IPUv1 fixes for different display connections

[Vinod Koul]

On Thu, 2011-12-08 at 10:39 +0100, Sascha Hauer wrote:
> Yes, you are right. Adding defines for these really makes this better
> readable. BTW did you queue both patches or only the first one?
Both :)


-- 
~Vinod