Various s3c-fb updates

The following problems were found in the above situation:

sfb->windows[win] was being assigned at the end of s3c_fb_probe_win only. This resulted in passing a NULL to s3c_fb_release_win if probe_win returned early and a memory leak.

dma_free_writecombine does not allow its third argument to be NULL.

fb_dealloc_cmap does not verify whether its argument is not NULL.

Signed-off-by Pawel Osciak <[email protected]>

Signed-off-by Kyungmin Park <[email protected]>

---
 drivers/video/s3c-fb.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

drivers/video/s3c-fb.c +2 -1

index b00c064..d998324 100644
@@ -804,7 +804,8 @@ static void s3c_fb_free_memory(struct s3c_fb *sfb, struct s3c_fb_win *win)
 {
 	struct fb_info *fbi = win->fbinfo;
 
-	dma_free_writecombine(sfb->dev, PAGE_ALIGN(fbi->fix.smem_len),
+	if (fbi->screen_base)
+		dma_free_writecombine(sfb->dev, PAGE_ALIGN(fbi->fix.smem_len),
 			      fbi->screen_base, fbi->fix.smem_start);
 }
 

drivers/video/s3c-fb.c +1 -1

 		s3c_fb_free_memory(sfb, win);
 		framebuffer_release(win->fbinfo);
 	}

@@ -865,6 +867,7 @@ static int __devinit s3c_fb_probe_win(struct s3c_fb *sfb, unsigned int win_no,
 	WARN_ON(windata->win_mode.yres == 0);
 
 	win = fbinfo->par;
+	*res = win;
 	var = &fbinfo->var;
 	win->variant = *variant;
 	win->fbinfo = fbinfo;

@@ -939,7 +942,6 @@ static int __devinit s3c_fb_probe_win(struct s3c_fb *sfb, unsigned int win_no,
 		return ret;
 	}
 
-	*res = win;
 	dev_info(sfb->dev, "window %d: fb %s\n", win_no, fbinfo->fix.id);
 
 	return 0;

Ben Dooks <[email protected]> wrote:

Review static void s3c_fb_release_win(struct s3c_fb *sfb,

Quoted 1 line · Pawel Osciak

(quoted diff) +0 -0

@@ -819,7 +820,8 @@ static void s3c_fb_release_win(struct s3c_fb *sfb,

struct s3c_fb_win *win)

drivers/video/s3c-fb.c static void s3c_fb_release_win(struct s3c_fb *sfb, struct s3c_fb_win *win) · line 824

Quoted 6 lines

drivers/video/s3c-fb.c +2 -1

 {
 	if (win->fbinfo) {
 		unregister_framebuffer(win->fbinfo);
-		fb_dealloc_cmap(&win->fbinfo->cmap);
+		if (&win->fbinfo->cmap)
+			fb_dealloc_cmap(&win->fbinfo->cmap);

did you really mean &win->fbinfo->cmap? surely that will end up always evaluating to true?

Quoted 30 lines

The following problems were found in the above situation:

sfb->windows[win] was being assigned at the end of s3c_fb_probe_win only. This resulted in passing a NULL to s3c_fb_release_win if probe_win returned early and a memory leak.

dma_free_writecombine does not allow its third argument to be NULL.

fb_dealloc_cmap does not verify whether its argument is not NULL.

Signed-off-by Pawel Osciak <[email protected]>

Signed-off-by Kyungmin Park <[email protected]>

---
 drivers/video/s3c-fb.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

drivers/video/s3c-fb.c +2 -1

index b00c064..d998324 100644
@@ -804,7 +804,8 @@ static void s3c_fb_free_memory(struct s3c_fb *sfb, struct s3c_fb_win *win)
 {
 	struct fb_info *fbi = win->fbinfo;
 
-	dma_free_writecombine(sfb->dev, PAGE_ALIGN(fbi->fix.smem_len),
+	if (fbi->screen_base)
+		dma_free_writecombine(sfb->dev, PAGE_ALIGN(fbi->fix.smem_len),
 			      fbi->screen_base, fbi->fix.smem_start);
 }
 

drivers/video/s3c-fb.c static void s3c_fb_release_win(struct s3c_fb *sfb, struct s3c_fb_win *win) · line 824

Quoted 7 lines

drivers/video/s3c-fb.c +2 -1

@@ -819,7 +820,8 @@ static void s3c_fb_release_win(struct s3c_fb *sfb, struct s3c_fb_win *win)
 {
 	if (win->fbinfo) {
 		unregister_framebuffer(win->fbinfo);
-		fb_dealloc_cmap(&win->fbinfo->cmap);
+		if (&win->fbinfo->cmap)
+			fb_dealloc_cmap(&win->fbinfo->cmap);

did you really mean &win->fbinfo->cmap? surely that will end up always evaluating to true?

Quoted 12 lines

drivers/video/s3c-fb.c +1 -0

 		s3c_fb_free_memory(sfb, win);
 		framebuffer_release(win->fbinfo);
 	}

@@ -865,6 +867,7 @@ static int __devinit s3c_fb_probe_win(struct s3c_fb *sfb, unsigned int win_no,
 	WARN_ON(windata->win_mode.yres == 0);
 
 	win = fbinfo->par;
+	*res = win;
 	var = &fbinfo->var;
 	win->variant = *variant;
 	win->fbinfo = fbinfo;

drivers/video/s3c-fb.c static int __devinit s3c_fb_probe_win(struct s3c_fb *sfb, unsigned int win_no, · line 947

Quoted 7 lines

drivers/video/s3c-fb.c +0 -1

@@ -939,7 +942,6 @@ static int __devinit s3c_fb_probe_win(struct s3c_fb *sfb, unsigned int win_no,
 		return ret;
 	}
 
-	*res = win;
 	dev_info(sfb->dev, "window %d: fb %s\n", win_no, fbinfo->fix.id);
 
 	return 0;

--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

FRAMESEL1 bitfield starts on 13th bit, not on 14th.

Signed-off-by Pawel Osciak <[email protected]>

Signed-off-by Kyungmin Park <[email protected]>

S5PC100 and S5PV210 framebuffer devices differ slightly in terms of available registers and their driver data structures have to be separate.

Ben Dooks <[email protected]> wrote:

Quoted 2 lines · Pawel Osciak

S5PC100 and S5PV210 framebuffer devices differ slightly in terms of available registers and their driver data structures have to be separate.

Would have been nice to note which ones changes, otherwise ok.

Add framebuffer device name initialization calls for S3C2443, S3C64xx and S5P machines.

Supports all bpp modes.

The PRTCON register is used to disable in-hardware updates of registers that store start and end addresses of framebuffer memory. This prevents display corruption in case we do not make it before VSYNC with updating them atomically. With this feature there is no need to wait for a VSYNC interrupt before each such update.

Signed-off-by Pawel Osciak <[email protected]>

Signed-off-by Kyungmin Park <[email protected]>

---
 arch/arm/plat-samsung/include/plat/regs-fb.h |    5 ++
 drivers/video/s3c-fb.c                       |   71 ++++++++++++++++++++++++++
 2 files changed, 76 insertions(+), 0 deletions(-)

arch/arm/plat-samsung/include/plat/regs-fb.h +5 -0

index ac10013..f454e32 100644
@@ -112,6 +112,11 @@
 #define VIDCON2_ORGYCbCr			(1 << 8)
 #define VIDCON2_YUVORDCrCb			(1 << 7)
 
+/* PRTCON (S3C6410, S5PC100) */
+
+#define PRTCON					(0x0c)
+#define PRTCON_PROTECT				(1 << 11)
+
 /* VIDTCON0 */
 
 #define VIDTCON0_VBPDE_MASK			(0xff << 24)

drivers/video/s3c-fb.c +3 -0

index 59ac76a..4f3680d 100644
@@ -71,6 +71,7 @@ struct s3c_fb;
  * @buf_end: Offset of buffer end registers.
  * @osd: The base for the OSD registers.
  * @palette: Address of palette memory, or 0 if none.
+ * @has_prtcon: Set if has PRTCON register.
  */
 struct s3c_fb_variant {
 	unsigned int	is_2443:1;

@@ -85,6 +86,8 @@ struct s3c_fb_variant {
 	unsigned short	osd;
 	unsigned short	osd_stride;
 	unsigned short	palette[S3C_FB_MAX_WIN];
+
+	unsigned int	has_prtcon:1;
 };
 
 /**

drivers/video/s3c-fb.c +66 -0

+
 	/* disable the window whilst we update it */
 	writel(0, regs + WINCON(win_no));
 

@@ -735,6 +741,66 @@ static int s3c_fb_blank(int blank_mode, struct fb_info *info)
 	return 0;
 }
 
+/**
+ * s3c_fb_pan_display() - Pan the display.
+ *
+ * Note that the offsets can be written to the device at any time, as their
+ * values are latched at each vsync automatically. This also means that only
+ * the last call to this function will have any effect on next vsync, but
+ * there is no need to sleep waiting for it to prevent tearing.
+ *
+ * @var: The screen information to verify.
+ * @info: The framebuffer device.
+ */
+static int s3c_fb_pan_display(struct fb_var_screeninfo *var,
+			      struct fb_info *info)
+{
+	struct s3c_fb_win *win	= info->par;
+	struct s3c_fb *sfb	= win->parent;
+	void __iomem *buf	= sfb->regs + win->index * 8;
+	unsigned int start_byte_offset, end_byte_offset;
+
+	/* Offset in bytes to the start of the displayed area */
+	start_byte_offset = var->yoffset * info->fix.line_length;
+	/* X offset depends on the current bpp */
+	if (info->var.bits_per_pixel >= 8) {
+		start_byte_offset +=
+			var->xoffset * (info->var.bits_per_pixel >> 3);
+	} else {
+		switch (info->var.bits_per_pixel) {
+		case 4:
+			start_byte_offset += var->xoffset >> 1;
+			break;
+		case 2:
+			start_byte_offset += var->xoffset >> 2;
+			break;
+		case 1:
+			start_byte_offset += var->xoffset >> 3;
+			break;
+		default:
+			dev_err(sfb->dev, "invalid bpp\n");
+			return -EINVAL;
+		}
+	}
+	/* Offset in bytes to the end of the displayed area */
+	end_byte_offset = start_byte_offset + var->yres * info->fix.line_length;
+
+	/* Temporarily turn off per-vsync update from shadow registers until
+	 * both start and end addresses are updated to prevent corruption */
+	if (sfb->variant.has_prtcon)
+		writel(PRTCON_PROTECT, sfb->regs + PRTCON);
+
+	writel(info->fix.smem_start + start_byte_offset,
+		buf + sfb->variant.buf_start);
+	writel(info->fix.smem_start + end_byte_offset,
+		buf + sfb->variant.buf_end);
+
+	if (sfb->variant.has_prtcon)
+		writel(0, sfb->regs + PRTCON);
+
+	return 0;
+}
+
 static struct fb_ops s3c_fb_ops = {
 	.owner		= THIS_MODULE,
 	.fb_check_var	= s3c_fb_check_var,

@@ -744,6 +810,7 @@ static struct fb_ops s3c_fb_ops = {
 	.fb_fillrect	= cfb_fillrect,
 	.fb_copyarea	= cfb_copyarea,
 	.fb_imageblit	= cfb_imageblit,
+	.fb_pan_display	= s3c_fb_pan_display,
 };
 
 /**

@@ -1243,6 +1310,8 @@ static struct s3c_fb_driverdata s3c_fb_data_64xx __devinitdata = {
 			[3] = 0x320,
 			[4] = 0x340,
 		},
+
+		.has_prtcon	= 1,
 	},
 	.win[0]	= &s3c_fb_data_64xx_wins[0],
 	.win[1]	= &s3c_fb_data_64xx_wins[1],

@@ -1271,6 +1340,8 @@ static struct s3c_fb_driverdata s3c_fb_data_s5pc100 __devinitdata = {
 			[3] = 0x3000,
 			[4] = 0x3400,
 		},
+
+		.has_prtcon	= 1,
 	},
 	.win[0]	= &s3c_fb_data_64xx_wins[0],
 	.win[1]	= &s3c_fb_data_64xx_wins[1],

Op 28-06-10 10:08, Pawel Osciak schreef:

Quoted 54 lines

Supports all bpp modes.

The PRTCON register is used to disable in-hardware updates of registers that store start and end addresses of framebuffer memory. This prevents display corruption in case we do not make it before VSYNC with updating them atomically. With this feature there is no need to wait for a VSYNC interrupt before each such update.

Signed-off-by Pawel Osciak <[email protected]>

Signed-off-by Kyungmin Park <[email protected]>

---
 arch/arm/plat-samsung/include/plat/regs-fb.h |    5 ++
 drivers/video/s3c-fb.c                       |   71 ++++++++++++++++++++++++++
 2 files changed, 76 insertions(+), 0 deletions(-)

arch/arm/plat-samsung/include/plat/regs-fb.h +5 -0

index ac10013..f454e32 100644
@@ -112,6 +112,11 @@
 #define VIDCON2_ORGYCbCr			(1 << 8)
 #define VIDCON2_YUVORDCrCb			(1 << 7)
 
+/* PRTCON (S3C6410, S5PC100) */
+
+#define PRTCON					(0x0c)
+#define PRTCON_PROTECT				(1 << 11)
+
 /* VIDTCON0 */
 
 #define VIDTCON0_VBPDE_MASK			(0xff << 24)

drivers/video/s3c-fb.c +3 -0

index 59ac76a..4f3680d 100644
@@ -71,6 +71,7 @@ struct s3c_fb;
  * @buf_end: Offset of buffer end registers.
  * @osd: The base for the OSD registers.
  * @palette: Address of palette memory, or 0 if none.
+ * @has_prtcon: Set if has PRTCON register.
  */
 struct s3c_fb_variant {
 	unsigned int	is_2443:1;

@@ -85,6 +86,8 @@ struct s3c_fb_variant {
 	unsigned short	osd;
 	unsigned short	osd_stride;
 	unsigned short	palette[S3C_FB_MAX_WIN];
+
+	unsigned int	has_prtcon:1;
 };
 
 /**

@@ -379,6 +382,9 @@ static int s3c_fb_set_par(struct fb_info *info)
 

drivers/video/s3c-fb.c static int s3c_fb_set_par(struct fb_info *info) · line 386

Quoted 4 lines

drivers/video/s3c-fb.c +2 -0

 	info->fix.line_length = (var->xres_virtual * var->bits_per_pixel) / 8;
 
+	info->fix.xpanstep = info->var.xres_virtual > info->var.xres ? 1 : 0;
+	info->fix.ypanstep = info->var.yres_virtual > info->var.yres ? 1 : 0;

No need for the '? 1 : 0'

Quoted 55 lines

Op 28-06-10 10:08, Pawel Osciak schreef:

Quoted 54 lines

Supports all bpp modes.

The PRTCON register is used to disable in-hardware updates of registers that store start and end addresses of framebuffer memory. This prevents display corruption in case we do not make it before VSYNC with updating them atomically. With this feature there is no need to wait for a VSYNC interrupt before each such update.

Signed-off-by Pawel Osciak <[email protected]>

Signed-off-by Kyungmin Park <[email protected]>

---
 arch/arm/plat-samsung/include/plat/regs-fb.h |    5 ++
 drivers/video/s3c-fb.c                       |   71 ++++++++++++++++++++++++++
 2 files changed, 76 insertions(+), 0 deletions(-)

arch/arm/plat-samsung/include/plat/regs-fb.h +5 -0

index ac10013..f454e32 100644
@@ -112,6 +112,11 @@
 #define VIDCON2_ORGYCbCr			(1 << 8)
 #define VIDCON2_YUVORDCrCb			(1 << 7)
 
+/* PRTCON (S3C6410, S5PC100) */
+
+#define PRTCON					(0x0c)
+#define PRTCON_PROTECT				(1 << 11)
+
 /* VIDTCON0 */
 
 #define VIDTCON0_VBPDE_MASK			(0xff << 24)

drivers/video/s3c-fb.c +3 -0

index 59ac76a..4f3680d 100644
@@ -71,6 +71,7 @@ struct s3c_fb;
  * @buf_end: Offset of buffer end registers.
  * @osd: The base for the OSD registers.
  * @palette: Address of palette memory, or 0 if none.
+ * @has_prtcon: Set if has PRTCON register.
  */
 struct s3c_fb_variant {
 	unsigned int	is_2443:1;

@@ -85,6 +86,8 @@ struct s3c_fb_variant {
 	unsigned short	osd;
 	unsigned short	osd_stride;
 	unsigned short	palette[S3C_FB_MAX_WIN];
+
+	unsigned int	has_prtcon:1;
 };
 
 /**

@@ -379,6 +382,9 @@ static int s3c_fb_set_par(struct fb_info *info)
 

Quoted 5 lines

drivers/video/s3c-fb.c static int s3c_fb_set_par(struct fb_info *info) · line 386

Quoted 4 lines

drivers/video/s3c-fb.c +2 -0

 	info->fix.line_length = (var->xres_virtual * var->bits_per_pixel) / 8;
 
+	info->fix.xpanstep = info->var.xres_virtual > info->var.xres ? 1 : 0;
+	info->fix.ypanstep = info->var.yres_virtual > info->var.yres ? 1 : 0;

No need for the '? 1 : 0'

I tihnk there is, IIRC there is no neccessity for a compiler to produce one for true, just that the result be !0

Quoted 1 line · Maurus Cuelenaere

No need for the '? 1 : 0'

I tihnk there is, IIRC there is no neccessity for a compiler to produce one for true, just that the result be !0

Supports all bpp modes.

The PRTCON register is used to disable in-hardware updates of registers that store start and end addresses of framebuffer memory. This prevents display corruption in case we do not make it before VSYNC with updating them atomically. With this feature there is no need to wait for a VSYNC interrupt before each such update.

Signed-off-by Pawel Osciak <[email protected]>

Signed-off-by Kyungmin Park <[email protected]>

---
 arch/arm/plat-samsung/include/plat/regs-fb.h |    5 ++
 drivers/video/s3c-fb.c                       |   71 ++++++++++++++++++++++++++
 2 files changed, 76 insertions(+), 0 deletions(-)

arch/arm/plat-samsung/include/plat/regs-fb.h +0 -0

index ac10013..f454e32 100644

arch/arm/plat-samsung/include/plat/regs-fb.h +3 -0

+#define PRTCON					(0x0c)
+#define PRTCON_PROTECT				(1 << 11)
+
 /* VIDTCON0 */
 
 #define VIDTCON0_VBPDE_MASK			(0xff << 24)

drivers/video/s3c-fb.c +14 -0

index 59ac76a..4f3680d 100644
@@ -71,6 +71,7 @@ struct s3c_fb;
  * @buf_end: Offset of buffer end registers.
  * @osd: The base for the OSD registers.
  * @palette: Address of palette memory, or 0 if none.
+ * @has_prtcon: Set if has PRTCON register.
  */
 struct s3c_fb_variant {
 	unsigned int	is_2443:1;

@@ -85,6 +86,8 @@ struct s3c_fb_variant {
 	unsigned short	osd;
 	unsigned short	osd_stride;
 	unsigned short	palette[S3C_FB_MAX_WIN];
+
+	unsigned int	has_prtcon:1;
 };
 
 /**

@@ -379,6 +382,9 @@ static int s3c_fb_set_par(struct fb_info *info)
 
 	info->fix.line_length = (var->xres_virtual * var->bits_per_pixel) / 8;
 
+	info->fix.xpanstep = info->var.xres_virtual > info->var.xres ? 1 : 0;
+	info->fix.ypanstep = info->var.yres_virtual > info->var.yres ? 1 : 0;
+
 	/* disable the window whilst we update it */
 	writel(0, regs + WINCON(win_no));
 

@@ -735,6 +741,66 @@ static int s3c_fb_blank(int blank_mode, struct fb_info *info)
 	return 0;
 }
 
+/**
+ * s3c_fb_pan_display() - Pan the display.
+ *
+ * Note that the offsets can be written to the device at any time, as their
+ * values are latched at each vsync automatically. This also means that only
+ * the last call to this function will have any effect on next vsync, but
+ * there is no need to sleep waiting for it to prevent tearing.
+ *

drivers/video/s3c-fb.c +12 -0

+	/* Offset in bytes to the start of the displayed area */
+	start_byte_offset = var->yoffset * info->fix.line_length;
+	/* X offset depends on the current bpp */
+	if (info->var.bits_per_pixel >= 8) {
+		start_byte_offset +=
+			var->xoffset * (info->var.bits_per_pixel >> 3);
+	} else {
+		switch (info->var.bits_per_pixel) {
+		case 4:
+			start_byte_offset += var->xoffset >> 1;
+			break;
+		case 2:

drivers/video/s3c-fb.c +24 -0

+	/* Offset in bytes to the end of the displayed area */
+	end_byte_offset = start_byte_offset + var->yres * info->fix.line_length;
+
+	/* Temporarily turn off per-vsync update from shadow registers until
+	 * both start and end addresses are updated to prevent corruption */
+	if (sfb->variant.has_prtcon)
+		writel(PRTCON_PROTECT, sfb->regs + PRTCON);
+
+	writel(info->fix.smem_start + start_byte_offset,
+		buf + sfb->variant.buf_start);
+	writel(info->fix.smem_start + end_byte_offset,
+		buf + sfb->variant.buf_end);
+
+	if (sfb->variant.has_prtcon)
+		writel(0, sfb->regs + PRTCON);
+
+	return 0;
+}
+
 static struct fb_ops s3c_fb_ops = {
 	.owner		= THIS_MODULE,
 	.fb_check_var	= s3c_fb_check_var,

@@ -744,6 +810,7 @@ static struct fb_ops s3c_fb_ops = {
 	.fb_fillrect	= cfb_fillrect,
 	.fb_copyarea	= cfb_copyarea,
 	.fb_imageblit	= cfb_imageblit,
+	.fb_pan_display	= s3c_fb_pan_display,
 };
 
 /**

@@ -1243,6 +1310,8 @@ static struct s3c_fb_driverdata s3c_fb_data_64xx __devinitdata = {
 			[3] = 0x320,
 			[4] = 0x340,
 		},
+
+		.has_prtcon	= 1,
 	},
 	.win[0]	= &s3c_fb_data_64xx_wins[0],
 	.win[1]	= &s3c_fb_data_64xx_wins[1],

@@ -1271,6 +1340,8 @@ static struct s3c_fb_driverdata s3c_fb_data_s5pc100 __devinitdata = {
 			[3] = 0x3000,
 			[4] = 0x3400,
 		},
+
+		.has_prtcon	= 1,
 	},
 	.win[0]	= &s3c_fb_data_64xx_wins[0],
 	.win[1]	= &s3c_fb_data_64xx_wins[1],

Ben Dooks <[email protected]> wrote:

arch/arm/plat-

Quoted 1 line · Pawel Osciak

arch/arm/plat- renamed from arch/arm/plat-samsung/include/plat/regs-fb.h +0 -0

samsung/include/plat/regs-fb.h

Quoted 7 lines

index ac10013..f454e32 100644

arch/arm/plat-samsung/include/plat/regs-fb.h +0 -0

@@ -112,6 +112,11 @@
 #define VIDCON2_ORGYCbCr			(1 << 8)
 #define VIDCON2_YUVORDCrCb			(1 << 7)
 

arch/arm/plat-samsung/include/plat/regs-fb.h line 115

Quoted 1 line

arch/arm/plat-samsung/include/plat/regs-fb.h +1 -0

+/* PRTCON (S3C6410, S5PC100) */

Not listed in my S3C6410 manual?

arch/arm/plat-samsung/include/plat/regs-fb.h static int s3c_fb_blank(int blank_mode, struct fb_info

Quoted 1 line

arch/arm/plat-samsung/include/plat/regs-fb.h +0 -0

@@ -735,6 +741,66 @@ static int s3c_fb_blank(int blank_mode, struct fb_info

*info)

arch/arm/plat-samsung/include/plat/regs-fb.h static int s3c_fb_blank(int blank_mode, struct fb_info · line 747

Quoted 7 lines

arch/arm/plat-samsung/include/plat/regs-fb.h +4 -0

 	return 0;
 }
 
+/**
+ * s3c_fb_pan_display() - Pan the display.
+ *
+ * Note that the offsets can be written to the device at any time, as

their

arch/arm/plat-samsung/include/plat/regs-fb.h static int s3c_fb_blank(int blank_mode, struct fb_info · line 748

Quoted 1 line

arch/arm/plat-samsung/include/plat/regs-fb.h +1 -0

+ * values are latched at each vsync automatically. This also means that

only

arch/arm/plat-samsung/include/plat/regs-fb.h static int s3c_fb_blank(int blank_mode, struct fb_info · line 761

Quoted 13 lines

arch/arm/plat-samsung/include/plat/regs-fb.h +13 -0

+ * the last call to this function will have any effect on next vsync, but
+ * there is no need to sleep waiting for it to prevent tearing.
+ *
+ * @var: The screen information to verify.
+ * @info: The framebuffer device.
+ */
+static int s3c_fb_pan_display(struct fb_var_screeninfo *var,
+			      struct fb_info *info)
+{
+	struct s3c_fb_win *win	= info->par;
+	struct s3c_fb *sfb	= win->parent;
+	void __iomem *buf	= sfb->regs + win->index * 8;
+	unsigned int start_byte_offset, end_byte_offset;

could have shortened to start and end, might have made some of the calculation easier.

Quoted 17 lines

arch/arm/plat-samsung/include/plat/regs-fb.h +17 -0

+	/* Offset in bytes to the start of the displayed area */
+	start_byte_offset = var->yoffset * info->fix.line_length;
+	/* X offset depends on the current bpp */
+	if (info->var.bits_per_pixel >= 8) {
+		start_byte_offset +=
+			var->xoffset * (info->var.bits_per_pixel >> 3);
+	} else {
+		switch (info->var.bits_per_pixel) {
+		case 4:
+			start_byte_offset += var->xoffset >> 1;
+			break;
+		case 2:
+			start_byte_offset += var->xoffset >> 2;
+			break;
+		case 1:
+			start_byte_offset += var->xoffset >> 3;
+			break;

arch/arm/plat-samsung/include/plat/regs-fb.h static int s3c_fb_blank(int blank_mode, struct fb_info · line 783

Quoted 5 lines

arch/arm/plat-samsung/include/plat/regs-fb.h +5 -0

+		default:
+			dev_err(sfb->dev, "invalid bpp\n");
+			return -EINVAL;
+		}
+	}

this could have been done by changing it to start_byte_offset += var->xoffset / (8 / info->var.bits_per_pixel);

note, you always set pan step to 1, evne if the case of bpp less than 8.

Quoted 12 lines

Ben Dooks <[email protected]> wrote:

arch/arm/plat-

Quoted 1 line · Pawel Osciak

arch/arm/plat- renamed from arch/arm/plat-samsung/include/plat/regs-fb.h +0 -0

samsung/include/plat/regs-fb.h

Quoted 7 lines

index ac10013..f454e32 100644

arch/arm/plat-samsung/include/plat/regs-fb.h +0 -0

@@ -112,6 +112,11 @@
 #define VIDCON2_ORGYCbCr			(1 << 8)
 #define VIDCON2_YUVORDCrCb			(1 << 7)
 

Quoted 4 lines

arch/arm/plat-samsung/include/plat/regs-fb.h line 115

Quoted 1 line

arch/arm/plat-samsung/include/plat/regs-fb.h +1 -0

+/* PRTCON (S3C6410, S5PC100) */

Not listed in my S3C6410 manual?

Not in my either. It is there in the S5PC100 docs though. This is not an "official statement", but I've been testing panning on my 6410 *very* intensively and experienced a lot of crashes without it. Not a single one with it though. So it's either almost impossible luck or it is there, undocumented. There is a hole at this address anyway, so this shouldn't hurt anything.

Add VSYNC interrupt support and an ioctl that allows waiting for it. Interrupts are turned on only when needed.

Signed-off-by Pawel Osciak <[email protected]>

Signed-off-by Kyungmin Park <[email protected]>

---
 arch/arm/plat-samsung/include/plat/regs-fb.h |    1 +
 drivers/video/s3c-fb.c                       |  167 +++++++++++++++++++++++++-
 2 files changed, 167 insertions(+), 1 deletions(-)

arch/arm/plat-samsung/include/plat/regs-fb.h +1 -0

index f454e32..5bcdd09 100644
@@ -298,6 +298,7 @@
 #define VIDINTCON0_FRAMESEL0_FRONTPORCH		(0x3 << 15)
 
 #define VIDINTCON0_FRAMESEL1			(1 << 13)
+#define VIDINTCON0_FRAMESEL1_MASK		(0x3 << 13)
 #define VIDINTCON0_FRAMESEL1_NONE		(0x0 << 13)
 #define VIDINTCON0_FRAMESEL1_BACKPORCH		(0x1 << 13)
 #define VIDINTCON0_FRAMESEL1_VSYNC		(0x2 << 13)

drivers/video/s3c-fb.c +38 -0

index 4f3680d..6131ebb 100644
@@ -21,6 +21,8 @@
 #include <linux/clk.h>
 #include <linux/fb.h>
 #include <linux/io.h>
+#include <linux/uaccess.h>
+#include <linux/interrupt.h>
 
 #include <mach/map.h>
 #include <plat/regs-fb-v4.h>

@@ -48,6 +50,11 @@
 	__raw_writel(v, r); } while(0)
 #endif /* FB_S3C_DEBUG_REGWRITE */
 
+/* irq_flags bits */
+#define S3C_FB_VSYNC_IRQ_EN	0
+
+#define VSYNC_TIMEOUT_MSEC 50
+
 struct s3c_fb;
 
 #define VALID_BPP(x) (1 << ((x) - 1))

@@ -156,6 +163,16 @@ struct s3c_fb_win {
 };
 
 /**
+ * struct s3c_fb_vsync - vsync information
+ * @wait:	a queue for processes waiting for vsync
+ * @count:	vsync interrupt count
+ */
+struct s3c_fb_vsync {
+	wait_queue_head_t	wait;
+	unsigned int		count;
+};
+
+/**
  * struct s3c_fb - overall hardware state of the hardware
  * @dev: The device that we bound to, for printing, etc.
  * @regs_res: The resource we claimed for the IO registers.

@@ -165,6 +182,9 @@ struct s3c_fb_win {
  * @enabled: A bitmask of enabled hardware windows.
  * @pdata: The platform configuration data passed with the device.
  * @windows: The hardware windows that have been claimed.
+ * @irq_no: IRQ line number
+ * @irq_flags: irq flags
+ * @vsync_info: VSYNC-related information (count, queues...)
  */
 struct s3c_fb {
 	struct device		*dev;

@@ -177,6 +197,10 @@ struct s3c_fb {
 
 	struct s3c_fb_platdata	*pdata;
 	struct s3c_fb_win	*windows[S3C_FB_MAX_WIN];
+
+	int			 irq_no;
+	unsigned long		 irq_flags;
+	struct s3c_fb_vsync	 vsync_info;
 };
 
 /**

@@ -801,6 +825,124 @@ static int s3c_fb_pan_display(struct fb_var_screeninfo *var,
 	return 0;
 }
 
+/**
+ * s3c_fb_enable_irq() - enable framebuffer interrupts
+ * @sfb: main hardware state
+ */
+static void s3c_fb_enable_irq(struct s3c_fb *sfb)
+{
+	void __iomem *regs = sfb->regs;
+	u32 irq_ctrl_reg;
+
+	if (!test_and_set_bit(S3C_FB_VSYNC_IRQ_EN, &sfb->irq_flags)) {
+		/* IRQ disabled, enable it */
+		irq_ctrl_reg = readl(regs + VIDINTCON0);
+
+		irq_ctrl_reg |= VIDINTCON0_INT_ENABLE;

drivers/video/s3c-fb.c +51 -0

+
+/**
+ * s3c_fb_disable_irq() - disable framebuffer interrupts
+ * @sfb: main hardware state
+ */
+static void s3c_fb_disable_irq(struct s3c_fb *sfb)
+{
+	void __iomem *regs = sfb->regs;
+	u32 irq_ctrl_reg;
+
+	if (test_and_clear_bit(S3C_FB_VSYNC_IRQ_EN, &sfb->irq_flags)) {
+		/* IRQ enabled, disable it */
+		irq_ctrl_reg = readl(regs + VIDINTCON0);
+
+		irq_ctrl_reg &= ~VIDINTCON0_INT_FRAME;
+		irq_ctrl_reg &= ~VIDINTCON0_INT_ENABLE;
+
+		writel(irq_ctrl_reg, regs + VIDINTCON0);
+	}
+}
+
+static irqreturn_t s3c_fb_irq(int irq, void *dev_id)
+{
+	struct s3c_fb *sfb = dev_id;
+	void __iomem  *regs = sfb->regs;
+	u32 irq_sts_reg;
+
+	irq_sts_reg = readl(regs + VIDINTCON1);
+
+	if (irq_sts_reg & VIDINTCON1_INT_FRAME) {
+
+		/* VSYNC interrupt, accept it */
+		writel(VIDINTCON1_INT_FRAME, regs + VIDINTCON1);
+
+		sfb->vsync_info.count++;
+		wake_up_interruptible(&sfb->vsync_info.wait);
+	}
+
+	/* We only support waiting for VSYNC for now, so it's safe
+	 * to always disable irqs here.
+	 */
+	s3c_fb_disable_irq(sfb);
+
+	return IRQ_HANDLED;
+}
+
+/**
+ * s3c_fb_wait_for_vsync() - sleep until next VSYNC interrupt or timeout
+ * @sfb: main hardware state
+ * @crtc: head index.
+ */

drivers/video/s3c-fb.c +13 -0

+	ret = wait_event_interruptible_timeout(sfb->vsync_info.wait,
+				       count != sfb->vsync_info.count,
+				       msecs_to_jiffies(VSYNC_TIMEOUT_MSEC));
+	if (ret == 0)
+		return -ETIMEDOUT;
+
+	return 0;
+}
+
+static int s3c_fb_ioctl(struct fb_info *info, unsigned int cmd,
+			unsigned long arg)
+{
+	struct s3c_fb_win *win = info->par;

drivers/video/s3c-fb.c +28 -1

+	}
+
+	return ret;
+}
+
 static struct fb_ops s3c_fb_ops = {
 	.owner		= THIS_MODULE,
 	.fb_check_var	= s3c_fb_check_var,

@@ -811,6 +953,7 @@ static struct fb_ops s3c_fb_ops = {
 	.fb_copyarea	= cfb_copyarea,
 	.fb_imageblit	= cfb_imageblit,
 	.fb_pan_display	= s3c_fb_pan_display,
+	.fb_ioctl	= s3c_fb_ioctl,
 };
 
 /**

@@ -917,6 +1060,8 @@ static int __devinit s3c_fb_probe_win(struct s3c_fb *sfb, unsigned int win_no,
 
 	dev_dbg(sfb->dev, "probing window %d, variant %p\n", win_no, variant);
 
+	init_waitqueue_head(&sfb->vsync_info.wait);
+
 	palette_size = variant->palette_sz * 4;
 
 	fbinfo = framebuffer_alloc(sizeof(struct s3c_fb_win) +

@@ -1096,6 +1241,20 @@ static int __devinit s3c_fb_probe(struct platform_device *pdev)
 		goto err_req_region;
 	}
 
+	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+	if (!res) {
+		dev_err(dev, "failed to acquire irq resource\n");
+		ret = -ENOENT;
+		goto err_ioremap;
+	}
+	sfb->irq_no = res->start;
+	ret = request_irq(sfb->irq_no, s3c_fb_irq,
+			  0, "s3c_fb", sfb);
+	if (ret) {
+		dev_err(dev, "irq request failed\n");
+		goto err_ioremap;
+	}
+
 	dev_dbg(dev, "got resources (regs %p), probing windows\n", sfb->regs);
 
 	/* setup gpio and output polarity controls */

@@ -1130,7 +1289,7 @@ static int __devinit s3c_fb_probe(struct platform_device *pdev)
 			dev_err(dev, "failed to create window %d\n", win);
 			for (; win >= 0; win--)
 				s3c_fb_release_win(sfb, sfb->windows[win]);
-			goto err_ioremap;
+			goto err_irq;
 		}
 	}
 

@@ -1138,6 +1297,9 @@ static int __devinit s3c_fb_probe(struct platform_device *pdev)
 
 	return 0;
 
+err_irq:
+	free_irq(sfb->irq_no, sfb);
+
 err_ioremap:
 	iounmap(sfb->regs);
 

@@ -1170,6 +1332,8 @@ static int __devexit s3c_fb_remove(struct platform_device *pdev)
 		if (sfb->windows[win])
 			s3c_fb_release_win(sfb, sfb->windows[win]);
 
+	free_irq(sfb->irq_no, sfb);
+
 	iounmap(sfb->regs);
 
 	clk_disable(sfb->bus_clk);

drivers/video/s3c-fb.c +0 -0

 	.win[0]	= &s3c_fb_data_64xx_wins[0],
 	.win[1]	= &s3c_fb_data_64xx_wins[1],

Ben Dooks <mailto:[email protected]> wrote:

Quoted 24 lines · Pawel Osciak

drivers/video/s3c-fb.c +20 -0

@@ -801,6 +825,124 @@ static int s3c_fb_pan_display(struct fb_var_screeninfo *var,
 	return 0;
 }
 
+/**
+ * s3c_fb_enable_irq() - enable framebuffer interrupts
+ * @sfb: main hardware state
+ */
+static void s3c_fb_enable_irq(struct s3c_fb *sfb)
+{
+	void __iomem *regs = sfb->regs;
+	u32 irq_ctrl_reg;
+
+	if (!test_and_set_bit(S3C_FB_VSYNC_IRQ_EN, &sfb->irq_flags)) {
+		/* IRQ disabled, enable it */
+		irq_ctrl_reg = readl(regs + VIDINTCON0);
+
+		irq_ctrl_reg |= VIDINTCON0_INT_ENABLE;
+		irq_ctrl_reg |= VIDINTCON0_INT_FRAME;
+
+		irq_ctrl_reg &= ~VIDINTCON0_FRAMESEL0_MASK;
+		irq_ctrl_reg |= VIDINTCON0_FRAMESEL0_VSYNC;
+		irq_ctrl_reg &= ~VIDINTCON0_FRAMESEL1_MASK;
+		irq_ctrl_reg |= VIDINTCON0_FRAMESEL1_NONE;

drivers/video/s3c-fb.c static int s3c_fb_pan_display(struct fb_var_screeninfo *var, · line 851

Quoted 4 lines

drivers/video/s3c-fb.c +4 -0

+
+		writel(irq_ctrl_reg, regs + VIDINTCON0);
+	}
+}

there should probably be some form of locking so that an irq doesn't come through and disable this. possibly in the call that queues the request to reduce the likelyhood of any races.

Quoted 55 lines

drivers/video/s3c-fb.c +55 -0

+
+/**
+ * s3c_fb_disable_irq() - disable framebuffer interrupts
+ * @sfb: main hardware state
+ */
+static void s3c_fb_disable_irq(struct s3c_fb *sfb)
+{
+	void __iomem *regs = sfb->regs;
+	u32 irq_ctrl_reg;
+
+	if (test_and_clear_bit(S3C_FB_VSYNC_IRQ_EN, &sfb->irq_flags)) {
+		/* IRQ enabled, disable it */
+		irq_ctrl_reg = readl(regs + VIDINTCON0);
+
+		irq_ctrl_reg &= ~VIDINTCON0_INT_FRAME;
+		irq_ctrl_reg &= ~VIDINTCON0_INT_ENABLE;
+
+		writel(irq_ctrl_reg, regs + VIDINTCON0);
+	}
+}
+
+static irqreturn_t s3c_fb_irq(int irq, void *dev_id)
+{
+	struct s3c_fb *sfb = dev_id;
+	void __iomem  *regs = sfb->regs;
+	u32 irq_sts_reg;
+
+	irq_sts_reg = readl(regs + VIDINTCON1);
+
+	if (irq_sts_reg & VIDINTCON1_INT_FRAME) {
+
+		/* VSYNC interrupt, accept it */
+		writel(VIDINTCON1_INT_FRAME, regs + VIDINTCON1);
+
+		sfb->vsync_info.count++;
+		wake_up_interruptible(&sfb->vsync_info.wait);
+	}
+
+	/* We only support waiting for VSYNC for now, so it's safe
+	 * to always disable irqs here.
+	 */
+	s3c_fb_disable_irq(sfb);
+
+	return IRQ_HANDLED;
+}
+
+/**
+ * s3c_fb_wait_for_vsync() - sleep until next VSYNC interrupt or timeout
+ * @sfb: main hardware state
+ * @crtc: head index.
+ */
+static int s3c_fb_wait_for_vsync(struct s3c_fb *sfb, u32 crtc)
+{
+	unsigned long count;
+	int ret;

drivers/video/s3c-fb.c static int s3c_fb_pan_display(struct fb_var_screeninfo *var, · line 912

Quoted 6 lines

drivers/video/s3c-fb.c +6 -0

+
+	if (crtc != 0)
+		return -ENODEV;
+
+	s3c_fb_enable_irq(sfb);
+	count = sfb->vsync_info.count;

possibly doing count before enabling the irq.

Quoted 17 lines

drivers/video/s3c-fb.c +17 -0

+	ret = wait_event_interruptible_timeout(sfb->vsync_info.wait,
+				       count != sfb->vsync_info.count,
+				       msecs_to_jiffies(VSYNC_TIMEOUT_MSEC));
+	if (ret == 0)
+		return -ETIMEDOUT;
+
+	return 0;
+}
+
+static int s3c_fb_ioctl(struct fb_info *info, unsigned int cmd,
+			unsigned long arg)
+{
+	struct s3c_fb_win *win = info->par;
+	struct s3c_fb *sfb = win->parent;
+	int ret;
+	u32 crtc;
+

drivers/video/s3c-fb.c static int s3c_fb_pan_display(struct fb_var_screeninfo *var, · line 935

Quoted 6 lines

drivers/video/s3c-fb.c +6 -0

+	switch (cmd) {
+	case FBIO_WAITFORVSYNC:
+		if (get_user(crtc, (u32 __user *)arg)) {
+			ret = -EFAULT;
+			break;
+		}

can't find any info on what the argument is meant to be.

drivers/video/s3c-fb.c static int s3c_fb_pan_display(struct fb_var_screeninfo *var, · line 940

Quoted 5 lines

drivers/video/s3c-fb.c +5 -0

+
+		ret = s3c_fb_wait_for_vsync(sfb, crtc);
+		break;
+	default:
+		ret = -ENOTTY;

Can someone else confirm this is the correct err?

Review static struct s3c_fb_driverdata s3c_fb_data_s5pv210

Quoted 1 line

(quoted diff) +0 -0

@@ -1370,6 +1534,7 @@ static struct s3c_fb_driverdata s3c_fb_data_s5pv210

__devinitdata = {

drivers/video/s3c-fb.c static struct s3c_fb_driverdata s3c_fb_data_s5pv210 __devinitdata = { · line 1538

Quoted 5 lines

drivers/video/s3c-fb.c +1 -0

 			[3] = 0x3000,
 			[4] = 0x3400,
 		},
+
 	},

oops, added whitespace.

S5PV210 allows per-window locking of register value updates from shadow registers.

Signed-off-by Pawel Osciak <[email protected]>

Signed-off-by Kyungmin Park <[email protected]>

---
 arch/arm/plat-samsung/include/plat/regs-fb.h |    3 ++
 drivers/video/s3c-fb.c                       |   39 +++++++++++++++++++++++--
 2 files changed, 38 insertions(+), 4 deletions(-)

arch/arm/plat-samsung/include/plat/regs-fb.h +0 -0

index 5bcdd09..da54b64 100644

arch/arm/plat-samsung/include/plat/regs-fb.h +0 -0

 #define VIDOSDxA_TOPLEFT_X_MASK			(0x7ff << 11)
 #define VIDOSDxA_TOPLEFT_X_SHIFT		(11)

drivers/video/s3c-fb.c +2 -0

index 0a93fca..94423c5 100644
@@ -79,6 +79,7 @@ struct s3c_fb;
  * @osd: The base for the OSD registers.
  * @palette: Address of palette memory, or 0 if none.
  * @has_prtcon: Set if has PRTCON register.
+ * @has_shadowcon: Set if has SHADOWCON register.
  */
 struct s3c_fb_variant {
 	unsigned int	is_2443:1;

@@ -95,6 +96,7 @@ struct s3c_fb_variant {
 	unsigned short	palette[S3C_FB_MAX_WIN];
 
 	unsigned int	has_prtcon:1;
+	unsigned int	has_shadowcon:1;
 };
 
 /**

Ben Dooks <[email protected]> wrote:

Quoted 4 lines · Pawel Osciak

arch/arm/plat-samsung/include/plat/regs-fb.h +0 -0

@@ -218,6 +218,9 @@
 #define WINCON1_BPPMODE_25BPP_A1888		(0xd << 2)
 #define WINCON1_BPPMODE_28BPP_A4888		(0xd << 2)
 

arch/arm/plat-samsung/include/plat/regs-fb.h line 223

Quoted 3 lines

arch/arm/plat-samsung/include/plat/regs-fb.h +3 -0

+/* S5PV210 */
+#define SHADOWCON				(0x34)
+#define SHADOWCON_WINx_PROTECT(_win)		(1 << (10 + _win))

you should have () around the _win arg.

drivers/video/s3c-fb.c static int s3c_fb_align_word(unsigned int bpp,

Quoted 24 lines

arch/arm/plat-samsung/include/plat/regs-fb.h +0 -0

 #define VIDOSDxA_TOPLEFT_X_MASK			(0x7ff << 11)
 #define VIDOSDxA_TOPLEFT_X_SHIFT		(11)

drivers/video/s3c-fb.c +2 -0

index 0a93fca..94423c5 100644
@@ -79,6 +79,7 @@ struct s3c_fb;
  * @osd: The base for the OSD registers.
  * @palette: Address of palette memory, or 0 if none.
  * @has_prtcon: Set if has PRTCON register.
+ * @has_shadowcon: Set if has SHADOWCON register.
  */
 struct s3c_fb_variant {
 	unsigned int	is_2443:1;

@@ -95,6 +96,7 @@ struct s3c_fb_variant {
 	unsigned short	palette[S3C_FB_MAX_WIN];
 
 	unsigned int	has_prtcon:1;
+	unsigned int	has_shadowcon:1;
 };
 
 /**

@@ -363,6 +365,36 @@ static int s3c_fb_align_word(unsigned int bpp,

unsigned int pix)

drivers/video/s3c-fb.c static int s3c_fb_align_word(unsigned int bpp, · line 368

Quoted 4 lines

drivers/video/s3c-fb.c +1 -0

 }
 
 /**
+ * shadow_protect_win() - disable updating values from shadow registers at

vsync

drivers/video/s3c-fb.c static int s3c_fb_align_word(unsigned int bpp, unsigned int pix) · line 373

Quoted 5 lines

drivers/video/s3c-fb.c +5 -0

+ *
+ * @win: window to protect registers for
+ * @protect: 1 to protect (disable updates)
+ */
+static void shadow_protect_win(struct s3c_fb_win *win, int protect)

technically, protect should be of type 'bool'

S3C64xx and S5P OSD registers for OSD size and alpha are as follows:
VIDOSDC: win 0 - size, win 1-4: alpha
VIDOSDD: win 1-2 - size; not present for windows 0, 3 and 4

Signed-off-by Pawel Osciak <[email protected]>

Signed-off-by Kyungmin Park <[email protected]>

---
 drivers/video/s3c-fb.c |   58 ++++++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 51 insertions(+), 7 deletions(-)

drivers/video/s3c-fb.c +0 -0

index 94423c5..3b2c7fe 100644

drivers/video/s3c-fb.c +38 -7

 /**
  * struct s3c_fb_variant - fb variant information

@@ -112,7 +115,10 @@ struct s3c_fb_variant {
 struct s3c_fb_win_variant {
 	unsigned int	has_osd_c:1;
 	unsigned int	has_osd_d:1;
+	unsigned int	has_osd_size:1;
+	unsigned int	has_osd_alpha:1;
 	unsigned int	palette_16bpp:1;
+	unsigned short	osd_size_off;
 	unsigned short	palette_sz;
 	u32		valid_bpp;
 };

@@ -365,6 +371,36 @@ static int s3c_fb_align_word(unsigned int bpp, unsigned int pix)
 }
 
 /**
+ * vidosd_set_size() - set OSD size for a window
+ *
+ * @win: the window to set OSD size for
+ * @size: OSD size register value
+ */
+static void vidosd_set_size(struct s3c_fb_win *win, u32 size)
+{
+	struct s3c_fb *sfb = win->parent;
+
+	if (win->variant.has_osd_size)
+		writel(size, sfb->regs + VIDOSD_SIZE(win->index, sfb->variant,
+							win->variant));
+}
+
+/**
+ * vidosd_set_alpha() - set alpha transparency for a window
+ *
+ * @win: the window to set OSD size for
+ * @alpha: alpha register value
+ */
+static void vidosd_set_alpha(struct s3c_fb_win *win, u32 alpha)
+{
+	struct s3c_fb *sfb = win->parent;
+
+	if (win->variant.has_osd_alpha)
+		writel(alpha, sfb->regs + VIDOSD_ALPHA(win->index,
+						sfb->variant, win->variant));
+}
+
+/**
  * shadow_protect_win() - disable updating values from shadow registers at vsync
  *
  * @win: window to protect registers for

@@ -408,7 +444,7 @@ static int s3c_fb_set_par(struct fb_info *info)
 	void __iomem *regs = sfb->regs;
 	void __iomem *buf = regs;
 	int win_no = win->index;
-	u32 osdc_data = 0;
+	u32 alpha = 0;
 	u32 data;
 	u32 pagewidth;
 	int clkdiv;

@@ -511,15 +547,12 @@ static int s3c_fb_set_par(struct fb_info *info)
 
 	data = var->xres * var->yres;
 
-	osdc_data = VIDISD14C_ALPHA1_R(0xf) |
+	alpha = VIDISD14C_ALPHA1_R(0xf) |
 		VIDISD14C_ALPHA1_G(0xf) |
 		VIDISD14C_ALPHA1_B(0xf);
 
-	if (win->variant.has_osd_d) {
-		writel(data, regs + VIDOSD_D(win_no, sfb->variant));
-		writel(osdc_data, regs + VIDOSD_C(win_no, sfb->variant));
-	} else
-		writel(data, regs + VIDOSD_C(win_no, sfb->variant));
+	vidosd_set_alpha(win, alpha);
+	vidosd_set_size(win, data);
 
 	data = WINCONx_ENWIN;
 

@@ -1445,12 +1478,17 @@ static int s3c_fb_resume(struct platform_device *pdev)
 static struct s3c_fb_win_variant s3c_fb_data_64xx_wins[] __devinitdata = {
 	[0] = {
 		.has_osd_c	= 1,
+		.has_osd_size	= 1,

Ben Dooks <[email protected]> wrote:

Quoted 8 lines · Pawel Osciak

S3C64xx and S5P OSD registers for OSD size and alpha are as follows:
VIDOSDC: win 0 - size, win 1-4: alpha
VIDOSDD: win 1-2 - size; not present for windows 0, 3 and 4

Signed-off-by Pawel Osciak <[email protected]>

Signed-off-by Kyungmin Park <[email protected]>

---
 drivers/video/s3c-fb.c |   58 ++++++++++++++++++++++++++++++++++++++++++--

---

drivers/video/s3c-fb.c struct s3c_fb; · line 69

Quoted 13 lines

 1 files changed, 51 insertions(+), 7 deletions(-)

drivers/video/s3c-fb.c +3 -0

index 94423c5..3b2c7fe 100644
@@ -64,6 +64,9 @@ struct s3c_fb;
 #define VIDOSD_B(win, variant) (OSD_BASE(win, variant) + 0x04)
 #define VIDOSD_C(win, variant) (OSD_BASE(win, variant) + 0x08)
 #define VIDOSD_D(win, variant) (OSD_BASE(win, variant) + 0x0C)
+#define VIDOSD_SIZE(win, variant, win_variant) \
+	(OSD_BASE(win, variant) + (win_variant).osd_size_off)
+#define VIDOSD_ALPHA(win, variant, win_variant) VIDOSD_C(win, variant)

hmm, this is becoming a bit complicated. if we have a function to set it then maybe we should just calculate it there.

drivers/video/s3c-fb.c static int s3c_fb_resume(struct platform_device

Quoted 1 line

drivers/video/s3c-fb.c +0 -0

@@ -1445,12 +1478,17 @@ static int s3c_fb_resume(struct platform_device

*pdev)

Quoted 4 lines

drivers/video/s3c-fb.c +1 -0

 static struct s3c_fb_win_variant s3c_fb_data_64xx_wins[] __devinitdata = {
 	[0] = {
 		.has_osd_c	= 1,
+		.has_osd_size	= 1,

drivers/video/s3c-fb.c static int s3c_fb_resume(struct platform_device *pdev) · line 1491

Quoted 10 lines

drivers/video/s3c-fb.c +4 -0

+		.osd_size_off	= 0x8,
 		.palette_sz	= 256,
 		.valid_bpp	= VALID_BPP1248 | VALID_BPP(16) | VALID_BPP(24),
 	},
 	[1] = {
 		.has_osd_c	= 1,
 		.has_osd_d	= 1,
+		.has_osd_size	= 1,
+		.osd_size_off	= 0x12,
+		.has_osd_alpha	= 1,

how about osd_size_off !=0 => has_osd_size ?

do we need to change the osd c and d definitions?