ARM: PXA: Make PXA27x/PXA3xx overlay actually work

From: "Russell King - ARM Linux" <[email protected]>

Release callback tries to free memory even if it was not allocated in map_video_memory. Fix PXA27x/3xx overlay memory management and make overlay actually work.

Added by Vasily Khoruzhick:

- Move overlay Z-ordering selection into main fb initialization, otherwise plane ordering is wrong.
- Clear x_res/y_res fields of fb.var on release, to make sure our callback will be called on next FBIOPUT_VSCREENINFO ioctl.
- Disable overlay only if it was enabled.

Signed-off-by Vasily Khoruzhick <[email protected]>

---
 drivers/video/pxafb.c |   86 ++++++++++++++++++++++++++++++------------------
 drivers/video/pxafb.h |    2 +-
 2 files changed, 55 insertions(+), 33 deletions(-)

drivers/video/pxafb.c +54 -32

index 825b665..33f607a 100644
@@ -629,16 +629,19 @@ static void overlay1fb_disable(struct pxafb_layer *ofb)
 {
 	uint32_t lccr5 = lcd_readl(ofb->fbi, LCCR5);
 
-	lcd_writel(ofb->fbi, OVL1C1, ofb->control[0] & ~OVLxC1_OEN);
+	if (lcd_readl(ofb->fbi, OVL1C1) & OVLxC1_OEN) {
+		lcd_writel(ofb->fbi, OVL1C1, ofb->control[0] & ~OVLxC1_OEN);
 
-	lcd_writel(ofb->fbi, LCSR1, LCSR1_BS(1));
-	lcd_writel(ofb->fbi, LCCR5, lccr5 & ~LCSR1_BS(1));
-	lcd_writel(ofb->fbi, FBR1, ofb->fbi->fdadr[DMA_OV1] | 0x3);
+		lcd_writel(ofb->fbi, LCSR1, LCSR1_BS(1));
+		lcd_writel(ofb->fbi, LCCR5, lccr5 & ~LCSR1_BS(1));
+		lcd_writel(ofb->fbi, FBR1, ofb->fbi->fdadr[DMA_OV1] | 0x3);
 
-	if (wait_for_completion_timeout(&ofb->branch_done, 1 * HZ) == 0)
-		pr_warning("%s: timeout disabling overlay1\n", __func__);
+		if (wait_for_completion_timeout(&ofb->branch_done, 1 * HZ) == 0)
+			pr_warning("%s: timeout disabling overlay1\n",
+				__func__);
 
-	lcd_writel(ofb->fbi, LCCR5, lccr5);
+		lcd_writel(ofb->fbi, LCCR5, lccr5);
+	}
 }
 
 static void overlay2fb_setup(struct pxafb_layer *ofb)

@@ -687,16 +690,19 @@ static void overlay2fb_disable(struct pxafb_layer *ofb)
 {
 	uint32_t lccr5 = lcd_readl(ofb->fbi, LCCR5);
 
-	lcd_writel(ofb->fbi, OVL2C1, ofb->control[0] & ~OVLxC1_OEN);
+	if (lcd_readl(ofb->fbi, OVL2C1) & OVLxC1_OEN) {
+		lcd_writel(ofb->fbi, OVL2C1, ofb->control[0] & ~OVLxC1_OEN);
 
-	lcd_writel(ofb->fbi, LCSR1, LCSR1_BS(2));
-	lcd_writel(ofb->fbi, LCCR5, lccr5 & ~LCSR1_BS(2));
-	lcd_writel(ofb->fbi, FBR2, ofb->fbi->fdadr[DMA_OV2_Y]  | 0x3);
-	lcd_writel(ofb->fbi, FBR3, ofb->fbi->fdadr[DMA_OV2_Cb] | 0x3);
-	lcd_writel(ofb->fbi, FBR4, ofb->fbi->fdadr[DMA_OV2_Cr] | 0x3);
+		lcd_writel(ofb->fbi, LCSR1, LCSR1_BS(2));
+		lcd_writel(ofb->fbi, LCCR5, lccr5 & ~LCSR1_BS(2));
+		lcd_writel(ofb->fbi, FBR2, ofb->fbi->fdadr[DMA_OV2_Y]  | 0x3);
+		lcd_writel(ofb->fbi, FBR3, ofb->fbi->fdadr[DMA_OV2_Cb] | 0x3);
+		lcd_writel(ofb->fbi, FBR4, ofb->fbi->fdadr[DMA_OV2_Cr] | 0x3);
 
-	if (wait_for_completion_timeout(&ofb->branch_done, 1 * HZ) == 0)
-		pr_warning("%s: timeout disabling overlay2\n", __func__);
+		if (wait_for_completion_timeout(&ofb->branch_done, 1 * HZ) == 0)
+			pr_warning("%s: timeout disabling overlay2\n",
+				__func__);
+	}
 }
 
 static struct pxafb_layer_ops ofb_ops[] = {

@@ -720,12 +726,10 @@ static int overlayfb_open(struct fb_info *info, int user)
 	if (user == 0)
 		return -ENODEV;
 
-	/* allow only one user at a time */
-	if (atomic_inc_and_test(&ofb->usage))
-		return -EBUSY;
+	if (ofb->usage++ == 0)
+		/* unblank the base framebuffer */
+		fb_blank(&ofb->fbi->fb, FB_BLANK_UNBLANK);
 
-	/* unblank the base framebuffer */
-	fb_blank(&ofb->fbi->fb, FB_BLANK_UNBLANK);
 	return 0;
 }
 

@@ -733,12 +737,24 @@ static int overlayfb_release(struct fb_info *info, int user)
 {
 	struct pxafb_layer *ofb = (struct pxafb_layer*) info;
 
-	atomic_dec(&ofb->usage);
-	ofb->ops->disable(ofb);
-
-	free_pages_exact(ofb->video_mem, ofb->video_mem_size);
-	ofb->video_mem = NULL;
-	ofb->video_mem_size = 0;
+	if (--ofb->usage == 0) {
+		ofb->ops->disable(ofb);
+		ofb->fb.var.height	= -1;
+		ofb->fb.var.width	= -1;
+		ofb->fb.var.xres = ofb->fb.var.xres_virtual = 0;
+		ofb->fb.var.yres = ofb->fb.var.yres_virtual = 0;
+
+		mutex_lock(&ofb->fb.mm_lock);
+		ofb->fb.fix.smem_start	= 0;
+		ofb->fb.fix.smem_len	= 0;
+		mutex_unlock(&ofb->fb.mm_lock);
+
+		if (ofb->video_mem) {
+			free_pages_exact(ofb->video_mem, ofb->video_mem_size);
+			ofb->video_mem = NULL;
+			ofb->video_mem_size = 0;
+		}
+	}
 	return 0;
 }
 

@@ -817,7 +833,8 @@ static int overlayfb_map_video_memory(struct pxafb_layer *ofb)
 		if (ofb->video_mem_size >= size)
 			return 0;
 
-		free_pages_exact(ofb->video_mem, ofb->video_mem_size);
+		/* don't re-allocate: userspace may have the buffer mapped */
+		return -EINVAL;
 	}
 
 	ofb->video_mem = alloc_pages_exact(size, GFP_KERNEL | __GFP_ZERO);

@@ -891,7 +908,7 @@ static void __devinit init_pxafb_overlay(struct pxafb_info *fbi,
 
 	ofb->id = id;
 	ofb->ops = &ofb_ops[id];
-	atomic_set(&ofb->usage, 0);
+	ofb->usage = 0;
 	ofb->fbi = fbi;
 	init_completion(&ofb->branch_done);
 }

@@ -923,8 +940,6 @@ static int __devinit pxafb_overlay_init(struct pxafb_info *fbi)
 	/* mask all IU/BS/EOF/SOF interrupts */
 	lcd_writel(fbi, LCCR5, ~0);
 
-	/* place overlay(s) on top of base */
-	fbi->lccr0 |= LCCR0_OUC;
 	pr_info("PXA Overlay driver loaded successfully!\n");
 	return 0;
 }

@@ -1368,7 +1383,8 @@ static int pxafb_activate_var(struct fb_var_screeninfo *var,
 	    (lcd_readl(fbi, LCCR3) != fbi->reg_lccr3) ||
 	    (lcd_readl(fbi, LCCR4) != fbi->reg_lccr4) ||
 	    (lcd_readl(fbi, FDADR0) != fbi->fdadr[0]) ||
-	    (lcd_readl(fbi, FDADR1) != fbi->fdadr[1]))
+	    ((fbi->lccr0 & LCCR0_SDS) &&
+	    (lcd_readl(fbi, FDADR1) != fbi->fdadr[1])))
 		pxafb_schedule_work(fbi, C_REENABLE);
 
 	return 0;

@@ -1420,7 +1436,8 @@ static void pxafb_enable_controller(struct pxafb_info *fbi)
 	lcd_writel(fbi, LCCR0, fbi->reg_lccr0 & ~LCCR0_ENB);
 
 	lcd_writel(fbi, FDADR0, fbi->fdadr[0]);
-	lcd_writel(fbi, FDADR1, fbi->fdadr[1]);
+	if (fbi->lccr0 & LCCR0_SDS)
+		lcd_writel(fbi, FDADR1, fbi->fdadr[1]);
 	lcd_writel(fbi, LCCR0, fbi->reg_lccr0 | LCCR0_ENB);
 }
 

@@ -1806,6 +1823,11 @@ static struct pxafb_info * __devinit pxafb_init_fbinfo(struct device *dev)
 
 	pxafb_decode_mach_info(fbi, inf);
 
+#ifdef CONFIG_FB_PXA_OVERLAY
+	if (cpu_is_pxa27x())
+		fbi->lccr0 |= LCCR0_OUC;
+#endif
+
 	init_waitqueue_head(&fbi->ctrlr_wait);
 	INIT_WORK(&fbi->task, pxafb_task);
 	mutex_init(&fbi->ctrlr_lock);

drivers/video/pxafb.h +1 -1

index 2353521..84e3ae1 100644
@@ -92,7 +92,7 @@ struct pxafb_layer_ops {
 struct pxafb_layer {
 	struct fb_info		fb;
 	int			id;
-	atomic_t		usage;
+	uint32_t		usage;
 	uint32_t		control[2];
 
 	struct pxafb_layer_ops	*ops;

Signature · 1 line

1.7.4.rc1

From: "Russell King - ARM Linux" <[email protected]>

Release callback tries to free memory even if it was not allocated in map_video_memory. Fix PXA27x/3xx overlay memory management and make overlay actually work.

Added by Vasily Khoruzhick:

- Move overlay Z-ordering selection into main fb initialization, otherwise plane ordering is wrong.
- Clear x_res/y_res fields of fb.var on release, to make sure our callback will be called on next FBIOPUT_VSCREENINFO ioctl.
- Disable overlay only if it was enabled.

Signed-off-by Vasily Khoruzhick <[email protected]>

---
?drivers/video/pxafb.c | ? 86 ++++++++++++++++++++++++++++++------------------ ?drivers/video/pxafb.h | ? ?2 +-
?2 files changed, 55 insertions(+), 33 deletions(-)

drivers/video/pxafb.c +0 -0

index 825b665..33f607a 100644

drivers/video/pxafb.c +10 -7

- ? ? ? lcd_writel(ofb->fbi, OVL1C1, ofb->control[0] & ~OVLxC1_OEN);
+ ? ? ? if (lcd_readl(ofb->fbi, OVL1C1) & OVLxC1_OEN) {
+ ? ? ? ? ? ? ? lcd_writel(ofb->fbi, OVL1C1, ofb->control[0] & ~OVLxC1_OEN);
 
- ? ? ? lcd_writel(ofb->fbi, LCSR1, LCSR1_BS(1));
- ? ? ? lcd_writel(ofb->fbi, LCCR5, lccr5 & ~LCSR1_BS(1));
- ? ? ? lcd_writel(ofb->fbi, FBR1, ofb->fbi->fdadr[DMA_OV1] | 0x3);
+ ? ? ? ? ? ? ? lcd_writel(ofb->fbi, LCSR1, LCSR1_BS(1));
+ ? ? ? ? ? ? ? lcd_writel(ofb->fbi, LCCR5, lccr5 & ~LCSR1_BS(1));
+ ? ? ? ? ? ? ? lcd_writel(ofb->fbi, FBR1, ofb->fbi->fdadr[DMA_OV1] | 0x3);
 
- ? ? ? if (wait_for_completion_timeout(&ofb->branch_done, 1 * HZ) == 0)
- ? ? ? ? ? ? ? pr_warning("%s: timeout disabling overlay1\n", __func__);
+ ? ? ? ? ? ? ? if (wait_for_completion_timeout(&ofb->branch_done, 1 * HZ) == 0)
+ ? ? ? ? ? ? ? ? ? ? ? pr_warning("%s: timeout disabling overlay1\n",
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? __func__);
 
- ? ? ? lcd_writel(ofb->fbi, LCCR5, lccr5);
+ ? ? ? ? ? ? ? lcd_writel(ofb->fbi, LCCR5, lccr5);
+ ? ? ? }
 ?}
 
 ?static void overlay2fb_setup(struct pxafb_layer *ofb)

drivers/video/pxafb.c +9 -7

- ? ? ? lcd_writel(ofb->fbi, LCSR1, LCSR1_BS(2));
- ? ? ? lcd_writel(ofb->fbi, LCCR5, lccr5 & ~LCSR1_BS(2));
- ? ? ? lcd_writel(ofb->fbi, FBR2, ofb->fbi->fdadr[DMA_OV2_Y] ?| 0x3);
- ? ? ? lcd_writel(ofb->fbi, FBR3, ofb->fbi->fdadr[DMA_OV2_Cb] | 0x3);
- ? ? ? lcd_writel(ofb->fbi, FBR4, ofb->fbi->fdadr[DMA_OV2_Cr] | 0x3);
+ ? ? ? ? ? ? ? lcd_writel(ofb->fbi, LCSR1, LCSR1_BS(2));
+ ? ? ? ? ? ? ? lcd_writel(ofb->fbi, LCCR5, lccr5 & ~LCSR1_BS(2));
+ ? ? ? ? ? ? ? lcd_writel(ofb->fbi, FBR2, ofb->fbi->fdadr[DMA_OV2_Y] ?| 0x3);
+ ? ? ? ? ? ? ? lcd_writel(ofb->fbi, FBR3, ofb->fbi->fdadr[DMA_OV2_Cb] | 0x3);
+ ? ? ? ? ? ? ? lcd_writel(ofb->fbi, FBR4, ofb->fbi->fdadr[DMA_OV2_Cr] | 0x3);
 
- ? ? ? if (wait_for_completion_timeout(&ofb->branch_done, 1 * HZ) == 0)
- ? ? ? ? ? ? ? pr_warning("%s: timeout disabling overlay2\n", __func__);
+ ? ? ? ? ? ? ? if (wait_for_completion_timeout(&ofb->branch_done, 1 * HZ) == 0)
+ ? ? ? ? ? ? ? ? ? ? ? pr_warning("%s: timeout disabling overlay2\n",
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? __func__);
+ ? ? ? }
 ?}
 
 ?static struct pxafb_layer_ops ofb_ops[] = {

drivers/video/pxafb.c +21 -10

- ? ? ? /* unblank the base framebuffer */
- ? ? ? fb_blank(&ofb->fbi->fb, FB_BLANK_UNBLANK);
 ? ? ? ?return 0;
 ?}
 

@@ -733,12 +737,24 @@ static int overlayfb_release(struct fb_info *info, int user)
 ?{
 ? ? ? ?struct pxafb_layer *ofb = (struct pxafb_layer*) info;
 
- ? ? ? atomic_dec(&ofb->usage);
- ? ? ? ofb->ops->disable(ofb);
-
- ? ? ? free_pages_exact(ofb->video_mem, ofb->video_mem_size);
- ? ? ? ofb->video_mem = NULL;
- ? ? ? ofb->video_mem_size = 0;
+ ? ? ? if (--ofb->usage == 0) {
+ ? ? ? ? ? ? ? ofb->ops->disable(ofb);
+ ? ? ? ? ? ? ? ofb->fb.var.height ? ? ?= -1;
+ ? ? ? ? ? ? ? ofb->fb.var.width ? ? ? = -1;
+ ? ? ? ? ? ? ? ofb->fb.var.xres = ofb->fb.var.xres_virtual = 0;
+ ? ? ? ? ? ? ? ofb->fb.var.yres = ofb->fb.var.yres_virtual = 0;
+
+ ? ? ? ? ? ? ? mutex_lock(&ofb->fb.mm_lock);
+ ? ? ? ? ? ? ? ofb->fb.fix.smem_start ?= 0;
+ ? ? ? ? ? ? ? ofb->fb.fix.smem_len ? ?= 0;
+ ? ? ? ? ? ? ? mutex_unlock(&ofb->fb.mm_lock);
+
+ ? ? ? ? ? ? ? if (ofb->video_mem) {
+ ? ? ? ? ? ? ? ? ? ? ? free_pages_exact(ofb->video_mem, ofb->video_mem_size);
+ ? ? ? ? ? ? ? ? ? ? ? ofb->video_mem = NULL;
+ ? ? ? ? ? ? ? ? ? ? ? ofb->video_mem_size = 0;
+ ? ? ? ? ? ? ? }
+ ? ? ? }
 ? ? ? ?return 0;
 ?}
 

@@ -817,7 +833,8 @@ static int overlayfb_map_video_memory(struct pxafb_layer *ofb)
 ? ? ? ? ? ? ? ?if (ofb->video_mem_size >= size)
 ? ? ? ? ? ? ? ? ? ? ? ?return 0;
 
- ? ? ? ? ? ? ? free_pages_exact(ofb->video_mem, ofb->video_mem_size);
+ ? ? ? ? ? ? ? /* don't re-allocate: userspace may have the buffer mapped */
+ ? ? ? ? ? ? ? return -EINVAL;
 ? ? ? ?}
 
 ? ? ? ?ofb->video_mem = alloc_pages_exact(size, GFP_KERNEL | __GFP_ZERO);

@@ -891,7 +908,7 @@ static void __devinit init_pxafb_overlay(struct pxafb_info *fbi,
 
 ? ? ? ?ofb->id = id;
 ? ? ? ?ofb->ops = &ofb_ops[id];
- ? ? ? atomic_set(&ofb->usage, 0);
+ ? ? ? ofb->usage = 0;
 ? ? ? ?ofb->fbi = fbi;
 ? ? ? ?init_completion(&ofb->branch_done);
 ?}

@@ -923,8 +940,6 @@ static int __devinit pxafb_overlay_init(struct pxafb_info *fbi)

? ? ? ?/* mask all IU/BS/EOF/SOF interrupts */
? ? ? ?lcd_writel(fbi, LCCR5, ~0);

- ? ? ? /* place overlay(s) on top of base */
- ? ? ? fbi->lccr0 |= LCCR0_OUC;
? ? ? ?pr_info("PXA Overlay driver loaded successfully!\n"); ? ? ? ?return 0;
?}

drivers/video/pxafb.c +0 -0

@@ -1368,7 +1383,8 @@ static int pxafb_activate_var(struct fb_var_screeninfo *var,

? ? ? ? ? ?(lcd_readl(fbi, LCCR3) != fbi->reg_lccr3) ||
? ? ? ? ? ?(lcd_readl(fbi, LCCR4) != fbi->reg_lccr4) ||
? ? ? ? ? ?(lcd_readl(fbi, FDADR0) != fbi->fdadr[0]) ||

- ? ? ? ? ? (lcd_readl(fbi, FDADR1) != fbi->fdadr[1]))
+ ? ? ? ? ? ((fbi->lccr0 & LCCR0_SDS) &&
+ ? ? ? ? ? (lcd_readl(fbi, FDADR1) != fbi->fdadr[1]))) ? ? ? ? ? ? ? ?pxafb_schedule_work(fbi, C_REENABLE);

? ? ? ?return 0;

drivers/video/pxafb.c +0 -0

 ? ? ? ?init_waitqueue_head(&fbi->ctrlr_wait);
 ? ? ? ?INIT_WORK(&fbi->task, pxafb_task);
 ? ? ? ?mutex_init(&fbi->ctrlr_lock);

drivers/video/pxafb.h +1 -2

index 2353521..84e3ae1 100644
@@ -92,7 +92,7 @@ struct pxafb_layer_ops {
 ?struct pxafb_layer {
 ? ? ? ?struct fb_info ? ? ? ? ?fb;
 ? ? ? ?int ? ? ? ? ? ? ? ? ? ? id;
- ? ? ? atomic_t ? ? ? ? ? ? ? ?usage;
+ ? ? ? uint32_t ? ? ? ? ? ? ? ?usage;
 ? ? ? ?uint32_t ? ? ? ? ? ? ? ?control[2];
 
 ? ? ? ?struct pxafb_layer_ops ?*ops;
--

1.7.4.rc1

Or maybe simply:

	if (lcd_readl(ofb->fbi, OVL1C1) & OVLxC1_OEN)
		return;

this makes this block of change much cleaner.

Quoted 3 lines

drivers/video/pxafb.c +2 -1

-       lcd_writel(ofb->fbi, OVL1C1, ofb->control[0] & ~OVLxC1_OEN);
+       if (lcd_readl(ofb->fbi, OVL1C1) & OVLxC1_OEN) {
+               lcd_writel(ofb->fbi, OVL1C1, ofb->control[0] &

Quoted 5 lines

static struct pxafb_layer_ops ofb_ops[] = {

drivers/video/pxafb.c +0 -0

@@ -720,12 +726,10 @@ static int overlayfb_open(struct fb_info *info, int
 user) if (user == 0)
               return -ENODEV;
 

drivers/video/pxafb.c static int overlayfb_open(struct fb_info *info, int user) · line 731

Quoted 6 lines

drivers/video/pxafb.c +3 -3

-       /* allow only one user at a time */
-       if (atomic_inc_and_test(&ofb->usage))
-               return -EBUSY;
+       if (ofb->usage++ == 0)
+               /* unblank the base framebuffer */
+               fb_blank(&ofb->fbi->fb, FB_BLANK_UNBLANK);

The change above allows multiple user at a time? Then I guess some other places need to be changed accordingly to avoid the racing conditions.

If this is a feature request, can we postpone it to subsequent patches?

drivers/video/pxafb.c static void pxafb_enable_controller(struct pxafb_info *fbi) · line 1440

Quoted 4 lines

drivers/video/pxafb.c +2 -1

       lcd_writel(fbi, FDADR0, fbi->fdadr[0]);
-       lcd_writel(fbi, FDADR1, fbi->fdadr[1]);
+       if (fbi->lccr0 & LCCR0_SDS)
+               lcd_writel(fbi, FDADR1, fbi->fdadr[1]);

My original intention was to simplify the code a bit by ignoring LCCR0_SDS, as FDADR1 would not take effect if not enabled even if it's being read/written.

drivers/video/pxafb.c static struct pxafb_info * __devinit pxafb_init_fbinfo(struct device *dev) · line 1830

Quoted 5 lines

drivers/video/pxafb.c +5 -0

+#ifdef CONFIG_FB_PXA_OVERLAY
+       if (cpu_is_pxa27x())
+               fbi->lccr0 |= LCCR0_OUC;
+#endif
+

I seem to remember LCCR0_OUC is still valid on pxa3xx, did you do some test on pxa3xx as well?

Quoted 5 lines

Quoted 4 lines

? ? ? ?lcd_writel(fbi, FDADR0, fbi->fdadr[0]);
- ? ? ? lcd_writel(fbi, FDADR1, fbi->fdadr[1]);
+ ? ? ? if (fbi->lccr0 & LCCR0_SDS)
+ ? ? ? ? ? ? ? lcd_writel(fbi, FDADR1, fbi->fdadr[1]);

Quoted 3 lines

My original intention was to simplify the code a bit by ignoring LCCR0_SDS, as FDADR1 would not take effect if not enabled even if it's being read/written.

It leads to potential race condition when you try to reconfigure main plane and overlay1 simultaneously.

Quoted 8 lines

drivers/video/pxafb.c static struct pxafb_info * __devinit pxafb_init_fbinfo(struct device *dev) · line 1830

Quoted 5 lines

drivers/video/pxafb.c +5 -0

+#ifdef CONFIG_FB_PXA_OVERLAY
+ ? ? ? if (cpu_is_pxa27x())
+ ? ? ? ? ? ? ? fbi->lccr0 |= LCCR0_OUC;
+#endif
+

I seem to remember LCCR0_OUC is still valid on pxa3xx, did you do some test on pxa3xx as well?

Sorry, I have no any pxa3xx boards.

Quoted 11 lines

Quoted 8 lines

drivers/video/pxafb.c static void pxafb_enable_controller(struct pxafb_info *fbi) · line 1440

Quoted 4 lines

       lcd_writel(fbi, FDADR0, fbi->fdadr[0]);

drivers/video/pxafb.c +2 -1

-       lcd_writel(fbi, FDADR1, fbi->fdadr[1]);
+       if (fbi->lccr0 & LCCR0_SDS)
+               lcd_writel(fbi, FDADR1, fbi->fdadr[1]);

My original intention was to simplify the code a bit by ignoring LCCR0_SDS, as FDADR1 would not take effect if not enabled even if it's being read/written.

It leads to potential race condition when you try to reconfigure main plane and overlay1 simultaneously.

You are right on this.

Quoted 9 lines

Quoted 8 lines

drivers/video/pxafb.c static struct pxafb_info * __devinit pxafb_init_fbinfo(struct device *dev) · line 1830

Quoted 5 lines

drivers/video/pxafb.c +5 -0

+#ifdef CONFIG_FB_PXA_OVERLAY
+       if (cpu_is_pxa27x())
+               fbi->lccr0 |= LCCR0_OUC;
+#endif
+

I seem to remember LCCR0_OUC is still valid on pxa3xx, did you do some test on pxa3xx as well?

Quoted 1 line

Sorry, I have no any pxa3xx boards.

That's all right, I can give it a test later. The point is, why did you move the code here from pxafb_overlay_init()?

Quoted 23 lines · On Tuesday 15 February 2011 13:51:06 Eric Miao wrote:

Quoted 11 lines

Quoted 8 lines

Quoted 4 lines

? ? ? ?lcd_writel(fbi, FDADR0, fbi->fdadr[0]);
- ? ? ? lcd_writel(fbi, FDADR1, fbi->fdadr[1]);
+ ? ? ? if (fbi->lccr0 & LCCR0_SDS)
+ ? ? ? ? ? ? ? lcd_writel(fbi, FDADR1, fbi->fdadr[1]);

My original intention was to simplify the code a bit by ignoring LCCR0_SDS, as FDADR1 would not take effect if not enabled even if it's being read/written.

It leads to potential race condition when you try to reconfigure main plane and overlay1 simultaneously.

You are right on this.

Quoted 9 lines

Quoted 8 lines

drivers/video/pxafb.c static struct pxafb_info * __devinit pxafb_init_fbinfo(struct device *dev) · line 1830

Quoted 5 lines

drivers/video/pxafb.c +5 -0

+#ifdef CONFIG_FB_PXA_OVERLAY
+ ? ? ? if (cpu_is_pxa27x())
+ ? ? ? ? ? ? ? fbi->lccr0 |= LCCR0_OUC;
+#endif
+

I seem to remember LCCR0_OUC is still valid on pxa3xx, did you do some test on pxa3xx as well?

Quoted 4 lines

Quoted 1 line

Sorry, I have no any pxa3xx boards.

That's all right, I can give it a test later. The point is, why did you move the code here from pxafb_overlay_init()?

Because otherwise correct plane order (overlays on top) will be selected only on next main plane reconfigure.

Regards
Vasily

Quoted 6 lines · On Tuesday 15 February 2011 15:36:07 you wrote:

Then maybe in this way? (I'd rather keep this bit in overlay specific code, and make it valid not only to pxa27x)

(quoted diff) +0 -0

@@ -925,6 +925,8 @@ static int __devinit pxafb_overlay_init(struct
 pxafb_info *fbi)
 

Quoted 6 lines

? ? ? ? /* place overlay(s) on top of base */
? ? ? ? fbi->lccr0 |= LCCR0_OUC;
+ ? ? ? lcd_writel(fbi, LCCR0, fbi->lccr0 & ~LCCR0_ENB); +
? ? ? ? pr_info("PXA Overlay driver loaded successfully!\n"); ? ? ? ? return 0;

I tried it, it doesn't work that way (I got garbage on screen). Maybe it's not right time to modify LCCR0 reg?

On Tue, Feb 15, 2011 at 9:51 PM, Vasily Khoruzhick <[email protected]>

Quoted 7 lines · On Tuesday 15 February 2011 15:36:07 you wrote:

Quoted 6 lines

Then maybe in this way? (I'd rather keep this bit in overlay specific code, and make it valid not only to pxa27x)

(quoted diff) +0 -0

@@ -925,6 +925,8 @@ static int __devinit pxafb_overlay_init(struct
 pxafb_info *fbi)
 

Quoted 9 lines

Review static int __devinit pxafb_overlay_init(struct · line 932

Quoted 6 lines

(quoted diff) +2 -0

        /* place overlay(s) on top of base */
        fbi->lccr0 |= LCCR0_OUC;
+       lcd_writel(fbi, LCCR0, fbi->lccr0 & ~LCCR0_ENB);
+
        pr_info("PXA Overlay driver loaded successfully!\n");
        return 0;

I tried it, it doesn't work that way (I got garbage on screen). Maybe it's not right time to modify LCCR0 reg?

I guess that's because OUC bit needs to be set when the panel is not enabled. But re-enabling here is a bit hackish as well.

I don't mind move the code a bit, but can we separate this change into another patch from the rest?

Thanks
- eric

From: Russell King - ARM Linux <[email protected]>

From: Russell King - ARM Linux <[email protected]>

Quoted 3 lines · Vasily Khoruzhick

From: Russell King - ARM Linux <[email protected]>

From: Russell King - ARM Linux <[email protected]>

No need for two From: lines.

drivers/video/pxafb.c static void overlay1fb_disable(struct pxafb_layer · line 646

Quoted 10 lines

drivers/video/pxafb.c +2 -1

@@ -636,7 +639,8 @@ static void overlay1fb_disable(struct pxafb_layer
 *ofb)
 
 	lcd_writel(ofb->fbi, FBR1, ofb->fbi->fdadr[DMA_OV1] | 0x3);
 
 	if (wait_for_completion_timeout(&ofb->branch_done, 1 * HZ) == 0)
 
-		pr_warning("%s: timeout disabling overlay1\n", __func__);
+		pr_warning("%s: timeout disabling overlay1\n",
+			__func__);

No need for this change.

Quoted 5 lines

(quoted diff) +0 -0

@@ -687,6 +691,9 @@ static void overlay2fb_disable(struct pxafb_layer
 *ofb)
 
 {
 

drivers/video/pxafb.c static void overlay2fb_disable(struct pxafb_layer *ofb) · line 696

Quoted 5 lines

drivers/video/pxafb.c +3 -0

 	uint32_t lccr5 = lcd_readl(ofb->fbi, LCCR5);
 
+	if (!(lcd_readl(ofb->fbi, OVL2C1) & OVLxC1_OEN))
+		return;
+

You don't describe this change in the change log, and this wasn't in my patch.

Quoted 5 lines

drivers/video/pxafb.c +0 -0

@@ -696,7 +703,8 @@ static void overlay2fb_disable(struct pxafb_layer
 *ofb)
 
 	lcd_writel(ofb->fbi, FBR4, ofb->fbi->fdadr[DMA_OV2_Cr] | 0x3);
 

drivers/video/pxafb.c static void overlay2fb_disable(struct pxafb_layer *ofb) · line 707

Quoted 5 lines

drivers/video/pxafb.c +2 -1

 	if (wait_for_completion_timeout(&ofb->branch_done, 1 * HZ) == 0)
 
-		pr_warning("%s: timeout disabling overlay2\n", __func__);
+		pr_warning("%s: timeout disabling overlay2\n",
+			__func__);

No need for this change.

pxafb_overlay_init is not right place to change Z-ordering, move it to main plane initialization.

Signed-off-by Vasily Khoruzhick <[email protected]>

---
?drivers/video/pxafb.c | ? ?8 ++++++--
?1 files changed, 6 insertions(+), 2 deletions(-)

drivers/video/pxafb.c +0 -0

index 0764759..e2f643e 100644
@@ -952,8 +952,6 @@ static void __devinit pxafb_overlay_init(struct pxafb_info *fbi)

? ? ? ?/* mask all IU/BS/EOF/SOF interrupts */
? ? ? ?lcd_writel(fbi, LCCR5, ~0);

- ? ? ? /* place overlay(s) on top of base */
- ? ? ? fbi->lccr0 |= LCCR0_OUC;
? ? ? ?pr_info("PXA Overlay driver loaded successfully!\n"); ?}

drivers/video/pxafb.c +6 -1

@@ -1843,6 +1841,12 @@ static struct pxafb_info * __devinit pxafb_init_fbinfo(struct device *dev)
 
 ? ? ? ?pxafb_decode_mach_info(fbi, inf);
 
+#ifdef CONFIG_FB_PXA_OVERLAY
+ ? ? ? /* place overlay(s) on top of base */
+ ? ? ? if (pxafb_overlay_supported())
+ ? ? ? ? ? ? ? fbi->lccr0 |= LCCR0_OUC;
+#endif
+
 ? ? ? ?init_waitqueue_head(&fbi->ctrlr_wait);
 ? ? ? ?INIT_WORK(&fbi->task, pxafb_task);
 ? ? ? ?mutex_init(&fbi->ctrlr_lock);
--

1.7.4.1

Sascha Hauer <[email protected]> pointed that ypos takes value of xpos due to typo.

Signed-off-by Vasily Khoruzhick <[email protected]>

---
 drivers/video/pxafb.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

drivers/video/pxafb.c +2 -2

index e2f643e..a3bdcc1 100644
@@ -761,7 +761,7 @@ static int overlayfb_check_var(struct fb_var_screeninfo
 *var, int xpos, ypos, pfor, bpp;
 
 	xpos = NONSTD_TO_XPOS(var->nonstd);
-	ypos = NONSTD_TO_XPOS(var->nonstd);
+	ypos = NONSTD_TO_YPOS(var->nonstd);
 	pfor = NONSTD_TO_PFOR(var->nonstd);
 
 	bpp = pxafb_var_to_bpp(var);

@@ -842,7 +842,7 @@ static int overlayfb_set_par(struct fb_info *info)
 
 	bpp  = pxafb_var_to_bpp(var);
 	xpos = NONSTD_TO_XPOS(var->nonstd);
-	ypos = NONSTD_TO_XPOS(var->nonstd);
+	ypos = NONSTD_TO_YPOS(var->nonstd);
 	pfor = NONSTD_TO_PFOR(var->nonstd);
 
 	ofb->control[0] = OVLxC1_PPL(var->xres) | OVLxC1_LPO(var->yres) |

Sascha Hauer <[email protected]> pointed that ypos takes value of xpos due to typo.

Signed-off-by Vasily Khoruzhick <[email protected]>

---
?drivers/video/pxafb.c | ? ?4 ++--
?1 files changed, 2 insertions(+), 2 deletions(-)

drivers/video/pxafb.c +2 -2

index e2f643e..a3bdcc1 100644
@@ -761,7 +761,7 @@ static int overlayfb_check_var(struct fb_var_screeninfo *var,
 ? ? ? ?int xpos, ypos, pfor, bpp;
 
 ? ? ? ?xpos = NONSTD_TO_XPOS(var->nonstd);
- ? ? ? ypos = NONSTD_TO_XPOS(var->nonstd);
+ ? ? ? ypos = NONSTD_TO_YPOS(var->nonstd);
 ? ? ? ?pfor = NONSTD_TO_PFOR(var->nonstd);
 
 ? ? ? ?bpp = pxafb_var_to_bpp(var);

@@ -842,7 +842,7 @@ static int overlayfb_set_par(struct fb_info *info)
 
 ? ? ? ?bpp ?= pxafb_var_to_bpp(var);
 ? ? ? ?xpos = NONSTD_TO_XPOS(var->nonstd);
- ? ? ? ypos = NONSTD_TO_XPOS(var->nonstd);
+ ? ? ? ypos = NONSTD_TO_YPOS(var->nonstd);
 ? ? ? ?pfor = NONSTD_TO_PFOR(var->nonstd);
 

? ? ? ?ofb->control[0] = OVLxC1_PPL(var->xres) | OVLxC1_LPO(var->yres) | --
1.7.4.1

It's not safe to disable controller if overlay(s) is enabled (results in system hang). So we avoid to disable controller in this case. Userspace should choose proper governor to avoid freq changing when overlay is in use, otherwise LCD may blink.

Signed-off-by Vasily Khoruzhick <[email protected]>

---
?drivers/video/pxafb.c | ? ?3 ++-
?1 files changed, 2 insertions(+), 1 deletions(-)

drivers/video/pxafb.c +0 -0

index a3bdcc1..a2e5b51 100644
@@ -1648,7 +1648,8 @@ pxafb_freq_transition(struct notifier_block *nb, unsigned long val, void *data)
 
 ? ? ? ?switch (val) {

? ? ? ?case CPUFREQ_PRECHANGE:
- ? ? ? ? ? ? ? set_ctrlr_state(fbi, C_DISABLE_CLKCHANGE);
+ ? ? ? ? ? ? ? if (!fbi->overlay[0].usage && !fbi->overlay[1].usage)
+ ? ? ? ? ? ? ? ? ? ? ? set_ctrlr_state(fbi, C_DISABLE_CLKCHANGE); ? ? ? ? ? ? ? ?break;

? ? ? ?case CPUFREQ_POSTCHANGE:
--
1.7.4.1

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

PXAFB overlay memory management is something messy:
- it allocates memory dynamically on open/release, and it results in memory allocation failure after ~1h of uptime (system does not have 115k of physically contiguous memory)
- in release callback it tries to free memory even if it was not allocated.

Also driver touches FDADR1 on main plane reconfiguration, and it can cause problems if overlay1 is enabled.

This patch attempts to fix those issues.

Patch is based on Russell King's work.

Signed-off-by Vasily Khoruzhick <[email protected]>

---
 drivers/video/pxafb.c |  121
++++++++++++++++++++++++++++++++----------------- drivers/video/pxafb.h | 
  3 +-
 2 files changed, 81 insertions(+), 43 deletions(-)

drivers/video/pxafb.c +12 -2

index 825b665..0764759 100644
@@ -627,7 +627,12 @@ static void overlay1fb_enable(struct pxafb_layer *ofb)
 
 static void overlay1fb_disable(struct pxafb_layer *ofb)
 {
-	uint32_t lccr5 = lcd_readl(ofb->fbi, LCCR5);
+	uint32_t lccr5;
+
+	if (!(lcd_readl(ofb->fbi, OVL1C1) & OVLxC1_OEN))
+		return;
+
+	lccr5 = lcd_readl(ofb->fbi, LCCR5);
 
 	lcd_writel(ofb->fbi, OVL1C1, ofb->control[0] & ~OVLxC1_OEN);
 

@@ -685,7 +690,12 @@ static void overlay2fb_enable(struct pxafb_layer *ofb)
 
 static void overlay2fb_disable(struct pxafb_layer *ofb)
 {
-	uint32_t lccr5 = lcd_readl(ofb->fbi, LCCR5);
+	uint32_t lccr5;
+
+	if (!(lcd_readl(ofb->fbi, OVL2C1) & OVLxC1_OEN))
+		return;
+
+	lccr5 = lcd_readl(ofb->fbi, LCCR5);
 
 	lcd_writel(ofb->fbi, OVL2C1, ofb->control[0] & ~OVLxC1_OEN);
 

usage++ into two parts ?

drivers/video/pxafb.c +12 -25

+		/* unblank the base framebuffer */
+		fb_blank(&ofb->fbi->fb, FB_BLANK_UNBLANK);
 
-	/* unblank the base framebuffer */
-	fb_blank(&ofb->fbi->fb, FB_BLANK_UNBLANK);
 	return 0;
 }

@@ -733,12 +741,15 @@ static int overlayfb_release(struct fb_info *info,
 int user) {
 	struct pxafb_layer *ofb = (struct pxafb_layer*) info;
 
-	atomic_dec(&ofb->usage);
-	ofb->ops->disable(ofb);
+	if (ofb->usage == 1) {
+		ofb->ops->disable(ofb);
+		ofb->fb.var.height	= -1;
+		ofb->fb.var.width	= -1;
+		ofb->fb.var.xres = ofb->fb.var.xres_virtual = 0;
+		ofb->fb.var.yres = ofb->fb.var.yres_virtual = 0;
 
-	free_pages_exact(ofb->video_mem, ofb->video_mem_size);
-	ofb->video_mem = NULL;
-	ofb->video_mem_size = 0;
+		ofb->usage--;
+	}
 	return 0;
 }
 

@@ -794,7 +805,7 @@ static int overlayfb_check_var(struct fb_var_screeninfo
 *var, return 0;
 }
 
-static int overlayfb_map_video_memory(struct pxafb_layer *ofb)
+static int overlayfb_check_video_memory(struct pxafb_layer *ofb)
 {
 	struct fb_var_screeninfo *var = &ofb->fb.var;
 	int pfor = NONSTD_TO_PFOR(var->nonstd);

@@ -812,27 +823,11 @@ static int overlayfb_map_video_memory(struct
 pxafb_layer *ofb)
 
 	size = PAGE_ALIGN(ofb->fb.fix.line_length * var->yres_virtual);
 
-	/* don't re-allocate if the original video memory is enough */
 	if (ofb->video_mem) {
 		if (ofb->video_mem_size >= size)
 			return 0;
-
-		free_pages_exact(ofb->video_mem, ofb->video_mem_size);
 	}
-
-	ofb->video_mem = alloc_pages_exact(size, GFP_KERNEL | __GFP_ZERO);
-	if (ofb->video_mem == NULL)
-		return -ENOMEM;
-
-	ofb->video_mem_phys = virt_to_phys(ofb->video_mem);
-	ofb->video_mem_size = size;
-
-	mutex_lock(&ofb->fb.mm_lock);
-	ofb->fb.fix.smem_start	= ofb->video_mem_phys;
-	ofb->fb.fix.smem_len	= ofb->fb.fix.line_length * var->yres_virtual;
-	mutex_unlock(&ofb->fb.mm_lock);
-	ofb->fb.screen_base	= ofb->video_mem;
-	return 0;
+	return -EINVAL;
 }
 

static int overlayfb_set_par(struct fb_info *info)

drivers/video/pxafb.c +2 -2

@@ -841,7 +836,7 @@ static int overlayfb_set_par(struct fb_info *info)
 	struct fb_var_screeninfo *var = &info->var;
 	int xpos, ypos, pfor, bpp, ret;
 
-	ret = overlayfb_map_video_memory(ofb);
+	ret = overlayfb_check_video_memory(ofb);
 	if (ret)
 		return ret;
 

@@ -891,7 +886,7 @@ static void __devinit init_pxafb_overlay(struct
 pxafb_info *fbi,
 
 	ofb->id = id;
 	ofb->ops = &ofb_ops[id];
-	atomic_set(&ofb->usage, 0);
+	ofb->usage = 0;
 	ofb->fbi = fbi;
 	init_completion(&ofb->branch_done);

}

drivers/video/pxafb.c +38 -5

@@ -904,20 +899,54 @@ static inline int pxafb_overlay_supported(void)
 	return 0;
 }
 
-static int __devinit pxafb_overlay_init(struct pxafb_info *fbi)
+static int __devinit pxafb_overlay_map_video_memory(struct pxafb_info
 *pxafb, +	struct pxafb_layer *ofb)
+{
+	/* We assume that user will use at most video_mem_size for overlay fb,
+	 * anyway, it's useless to use 16bpp main plane and 24bpp overlay
+	 */
+	ofb->video_mem = alloc_pages_exact(PAGE_ALIGN(pxafb->video_mem_size),
+		GFP_KERNEL | __GFP_ZERO);
+	if (ofb->video_mem == NULL)
+		return -ENOMEM;
+
+	ofb->video_mem_phys = virt_to_phys(ofb->video_mem);
+	ofb->video_mem_size = PAGE_ALIGN(pxafb->video_mem_size);
+
+	mutex_lock(&ofb->fb.mm_lock);
+	ofb->fb.fix.smem_start	= ofb->video_mem_phys;
+	ofb->fb.fix.smem_len	= pxafb->video_mem_size;
+	mutex_unlock(&ofb->fb.mm_lock);
+
+	ofb->fb.screen_base	= ofb->video_mem;
+
+	return 0;
+}
+
+static void __devinit pxafb_overlay_init(struct pxafb_info *fbi)
 {
 	int i, ret;
 
 	if (!pxafb_overlay_supported())
-		return 0;
+		return;
 
 	for (i = 0; i < 2; i++) {
-		init_pxafb_overlay(fbi, &fbi->overlay[i], i);
-		ret = register_framebuffer(&fbi->overlay[i].fb);
+		struct pxafb_layer *ofb = &fbi->overlay[i];
+		init_pxafb_overlay(fbi, ofb, i);
+		ret = register_framebuffer(&ofb->fb);
 		if (ret) {
 			dev_err(fbi->dev, "failed to register overlay %d\n", i);
-			return ret;
+			continue;
+		}
+		ret = pxafb_overlay_map_video_memory(fbi, ofb);
+		if (ret) {
+			dev_err(fbi->dev,
+				"failed to map video memory for overlay %d\n",
+				i);
+			unregister_framebuffer(&ofb->fb);
+			continue;
 		}
+		ofb->registered = 1;
 	}
 
 	/* mask all IU/BS/EOF/SOF interrupts */

@@ -926,7 +955,6 @@ static int __devinit pxafb_overlay_init(struct

pxafb_info *fbi) /* place overlay(s) on top of base */

drivers/video/pxafb.c +9 -3

 	fbi->lccr0 |= LCCR0_OUC;
 	pr_info("PXA Overlay driver loaded successfully!\n");
-	return 0;
 }
 
 static void __devexit pxafb_overlay_exit(struct pxafb_info *fbi)

@@ -936,8 +964,15 @@ static void __devexit pxafb_overlay_exit(struct
 pxafb_info *fbi) if (!pxafb_overlay_supported())
 		return;
 
-	for (i = 0; i < 2; i++)
-		unregister_framebuffer(&fbi->overlay[i].fb);
+	for (i = 0; i < 2; i++) {
+		struct pxafb_layer *ofb = &fbi->overlay[i];
+		if (ofb->registered) {
+			if (ofb->video_mem)
+				free_pages_exact(ofb->video_mem,
+					ofb->video_mem_size);
+			unregister_framebuffer(&ofb->fb);
+		}
+	}
 }
 #else
 static inline void pxafb_overlay_init(struct pxafb_info *fbi) {}

@@ -1368,7 +1403,8 @@ static int pxafb_activate_var(struct

fb_var_screeninfo *var, (lcd_readl(fbi, LCCR3) != fbi->reg_lccr3) ||

drivers/video/pxafb.c +4 -2

 	    (lcd_readl(fbi, LCCR4) != fbi->reg_lccr4) ||
 	    (lcd_readl(fbi, FDADR0) != fbi->fdadr[0]) ||
-	    (lcd_readl(fbi, FDADR1) != fbi->fdadr[1]))
+	    ((fbi->lccr0 & LCCR0_SDS) &&
+	    (lcd_readl(fbi, FDADR1) != fbi->fdadr[1])))
 		pxafb_schedule_work(fbi, C_REENABLE);
 
 	return 0;

@@ -1420,7 +1456,8 @@ static void pxafb_enable_controller(struct pxafb_info
 *fbi) lcd_writel(fbi, LCCR0, fbi->reg_lccr0 & ~LCCR0_ENB);
 
 	lcd_writel(fbi, FDADR0, fbi->fdadr[0]);
-	lcd_writel(fbi, FDADR1, fbi->fdadr[1]);
+	if (fbi->lccr0 & LCCR0_SDS)
+		lcd_writel(fbi, FDADR1, fbi->fdadr[1]);
 	lcd_writel(fbi, LCCR0, fbi->reg_lccr0 | LCCR0_ENB);
 }
 

drivers/video/pxafb.h +2 -1

index 2353521..26ba9fa 100644
@@ -92,7 +92,8 @@ struct pxafb_layer_ops {
 struct pxafb_layer {
 	struct fb_info		fb;
 	int			id;
-	atomic_t		usage;
+	int			registered;
+	uint32_t		usage;
 	uint32_t		control[2];
 
 	struct pxafb_layer_ops	*ops;

drivers/video/pxafb.c static int overlayfb_open(struct fb_info *info, int user) · line 733

Quoted 1 line

drivers/video/pxafb.c +1 -0

+	if (ofb->usage++ == 0)

TBH I don't like this notation, it feels hard to read. Can you split that ofb-

Quoted 1 line

usage++ into two parts ?

Cheers

Quoted 5 lines

Quoted 1 line

+ ? if (ofb->usage++ == 0)

TBH I don't like this notation, it feels hard to read. Can you split that ofb-

Quoted 3 lines

Quoted 1 line

usage++ into two parts ?

Cheers

It's not deprecated by kernel codestyle, looks ok and works ok, but I'll change it if one more person argues against it :)

From: Russell King - ARM Linux <[email protected]>

Release callback tries to free memory even if it was not allocated in map_video_memory, fix it.

Added by Vasily Khoruzhick:

- Clear x_res/y_res fields of fb.var on release, to make sure our callback will be called on next FBIOPUT_VSCREENINFO ioctl.
- Disable overlay only if it was enabled.
- Don't touch FDADR1 if it's not necessary

Signed-off-by Vasily Khoruzhick <[email protected]>

---
v2: remove unnecessary newlines, add comment about FDADR1

 drivers/video/pxafb.c |   49
++++++++++++++++++++++++++++++++++--------------- drivers/video/pxafb.h | 
  2 +-
 2 files changed, 35 insertions(+), 16 deletions(-)

drivers/video/pxafb.c +0 -0

index 825b665..41a499f 100644

drivers/video/pxafb.c +24 -13

 	lcd_writel(ofb->fbi, OVL2C1, ofb->control[0] & ~OVLxC1_OEN);
 
 	lcd_writel(ofb->fbi, LCSR1, LCSR1_BS(2));

@@ -720,12 +726,10 @@ static int overlayfb_open(struct fb_info *info, int
 user) if (user == 0)
 		return -ENODEV;
 
-	/* allow only one user at a time */
-	if (atomic_inc_and_test(&ofb->usage))
-		return -EBUSY;
+	if (ofb->usage++ == 0)
+		/* unblank the base framebuffer */
+		fb_blank(&ofb->fbi->fb, FB_BLANK_UNBLANK);
 
-	/* unblank the base framebuffer */
-	fb_blank(&ofb->fbi->fb, FB_BLANK_UNBLANK);
 	return 0;
 }
 

@@ -733,12 +737,24 @@ static int overlayfb_release(struct fb_info *info,
 int user) {
 	struct pxafb_layer *ofb = (struct pxafb_layer*) info;
 
-	atomic_dec(&ofb->usage);
-	ofb->ops->disable(ofb);
-
-	free_pages_exact(ofb->video_mem, ofb->video_mem_size);
-	ofb->video_mem = NULL;
-	ofb->video_mem_size = 0;
+	if (--ofb->usage == 0) {
+		ofb->ops->disable(ofb);
+		ofb->fb.var.height	= -1;
+		ofb->fb.var.width	= -1;
+		ofb->fb.var.xres = ofb->fb.var.xres_virtual = 0;
+		ofb->fb.var.yres = ofb->fb.var.yres_virtual = 0;
+
+		mutex_lock(&ofb->fb.mm_lock);
+		ofb->fb.fix.smem_start	= 0;
+		ofb->fb.fix.smem_len	= 0;
+		mutex_unlock(&ofb->fb.mm_lock);
+
+		if (ofb->video_mem) {
+			free_pages_exact(ofb->video_mem, ofb->video_mem_size);
+			ofb->video_mem = NULL;
+			ofb->video_mem_size = 0;
+		}
+	}
 	return 0;
 }
 

@@ -817,7 +833,8 @@ static int overlayfb_map_video_memory(struct
 pxafb_layer *ofb) if (ofb->video_mem_size >= size)
 			return 0;
 
-		free_pages_exact(ofb->video_mem, ofb->video_mem_size);
+		/* don't re-allocate: userspace may have the buffer mapped */
+		return -EINVAL;
 	}
 
 	ofb->video_mem = alloc_pages_exact(size, GFP_KERNEL | __GFP_ZERO);

@@ -891,7 +908,7 @@ static void __devinit init_pxafb_overlay(struct
 pxafb_info *fbi,
 
 	ofb->id = id;
 	ofb->ops = &ofb_ops[id];
-	atomic_set(&ofb->usage, 0);
+	ofb->usage = 0;
 	ofb->fbi = fbi;
 	init_completion(&ofb->branch_done);

}

drivers/video/pxafb.c +0 -0

@@ -1368,7 +1385,8 @@ static int pxafb_activate_var(struct

fb_var_screeninfo *var, (lcd_readl(fbi, LCCR3) != fbi->reg_lccr3) ||

drivers/video/pxafb.c +4 -2

 	    (lcd_readl(fbi, LCCR4) != fbi->reg_lccr4) ||
 	    (lcd_readl(fbi, FDADR0) != fbi->fdadr[0]) ||
-	    (lcd_readl(fbi, FDADR1) != fbi->fdadr[1]))
+	    ((fbi->lccr0 & LCCR0_SDS) &&
+	    (lcd_readl(fbi, FDADR1) != fbi->fdadr[1])))
 		pxafb_schedule_work(fbi, C_REENABLE);
 
 	return 0;

@@ -1420,7 +1438,8 @@ static void pxafb_enable_controller(struct pxafb_info
 *fbi) lcd_writel(fbi, LCCR0, fbi->reg_lccr0 & ~LCCR0_ENB);
 
 	lcd_writel(fbi, FDADR0, fbi->fdadr[0]);
-	lcd_writel(fbi, FDADR1, fbi->fdadr[1]);
+	if (fbi->lccr0 & LCCR0_SDS)
+		lcd_writel(fbi, FDADR1, fbi->fdadr[1]);
 	lcd_writel(fbi, LCCR0, fbi->reg_lccr0 | LCCR0_ENB);
 }
 

drivers/video/pxafb.h +1 -1

index 2353521..84e3ae1 100644
@@ -92,7 +92,7 @@ struct pxafb_layer_ops {
 struct pxafb_layer {
 	struct fb_info		fb;
 	int			id;
-	atomic_t		usage;
+	uint32_t		usage;
 	uint32_t		control[2];
 
 	struct pxafb_layer_ops	*ops;

Quoted 5 lines

(quoted diff) +0 -0

@@ -629,6 +629,9 @@ static void overlay1fb_disable(struct pxafb_layer
 *ofb)
 
 {
 

drivers/video/pxafb.c static void overlay1fb_disable(struct pxafb_layer *ofb) · line 634

Quoted 5 lines

drivers/video/pxafb.c +3 -0

 	uint32_t lccr5 = lcd_readl(ofb->fbi, LCCR5);
 
+	if (!(lcd_readl(ofb->fbi, OVL1C1) & OVLxC1_OEN))
+		return;
+

Maybe you can even avoid reading LCCR5 above ;-)

Sascha Hauer <[email protected]> pointed that ypos takes value of xpos due to typo.

Signed-off-by Vasily Khoruzhick <[email protected]>

---
 drivers/video/pxafb.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

drivers/video/pxafb.c +2 -2

index 54a9ab8..80ea7e6 100644
@@ -766,7 +766,7 @@ static int overlayfb_check_var(struct fb_var_screeninfo
 *var, int xpos, ypos, pfor, bpp;
 
 	xpos = NONSTD_TO_XPOS(var->nonstd);
-	ypos = NONSTD_TO_XPOS(var->nonstd);
+	ypos = NONSTD_TO_YPOS(var->nonstd);
 	pfor = NONSTD_TO_PFOR(var->nonstd);
 
 	bpp = pxafb_var_to_bpp(var);

@@ -864,7 +864,7 @@ static int overlayfb_set_par(struct fb_info *info)
 
 	bpp  = pxafb_var_to_bpp(var);
 	xpos = NONSTD_TO_XPOS(var->nonstd);
-	ypos = NONSTD_TO_XPOS(var->nonstd);
+	ypos = NONSTD_TO_YPOS(var->nonstd);
 	pfor = NONSTD_TO_PFOR(var->nonstd);
 
 	ofb->control[0] = OVLxC1_PPL(var->xres) | OVLxC1_LPO(var->yres) |

From: Russell King - ARM Linux <[email protected]>

From: Russell King - ARM Linux <[email protected]>

Release callback tries to free memory even if it was not allocated in map_video_memory, fix it.

Added by Vasily Khoruzhick:

- Clear x_res/y_res fields of fb.var on release, to make sure our callback will be called on next FBIOPUT_VSCREENINFO ioctl.
- Disable overlay only if it was enabled.

Signed-off-by Vasily Khoruzhick <[email protected]>

---
 drivers/video/pxafb.c |   55
+++++++++++++++++++++++++++++++++--------------- drivers/video/pxafb.h |  
 2 +-
 2 files changed, 39 insertions(+), 18 deletions(-)

drivers/video/pxafb.c +10 -2

index 825b665..c6aad56 100644
@@ -629,6 +629,9 @@ static void overlay1fb_disable(struct pxafb_layer *ofb)
 {
 	uint32_t lccr5 = lcd_readl(ofb->fbi, LCCR5);
 
+	if (!(lcd_readl(ofb->fbi, OVL1C1) & OVLxC1_OEN))
+		return;
+
 	lcd_writel(ofb->fbi, OVL1C1, ofb->control[0] & ~OVLxC1_OEN);
 
 	lcd_writel(ofb->fbi, LCSR1, LCSR1_BS(1));

@@ -636,7 +639,8 @@ static void overlay1fb_disable(struct pxafb_layer *ofb)
 	lcd_writel(ofb->fbi, FBR1, ofb->fbi->fdadr[DMA_OV1] | 0x3);
 
 	if (wait_for_completion_timeout(&ofb->branch_done, 1 * HZ) == 0)
-		pr_warning("%s: timeout disabling overlay1\n", __func__);
+		pr_warning("%s: timeout disabling overlay1\n",
+			__func__);
 
 	lcd_writel(ofb->fbi, LCCR5, lccr5);
 }

@@ -687,6 +691,9 @@ static void overlay2fb_disable(struct pxafb_layer *ofb)
 {
 	uint32_t lccr5 = lcd_readl(ofb->fbi, LCCR5);
 
+	if (!(lcd_readl(ofb->fbi, OVL2C1) & OVLxC1_OEN))
+		return;
+
 	lcd_writel(ofb->fbi, OVL2C1, ofb->control[0] & ~OVLxC1_OEN);
 
 	lcd_writel(ofb->fbi, LCSR1, LCSR1_BS(2));

@@ -696,7 +703,8 @@ static void overlay2fb_disable(struct pxafb_layer *ofb)
 	lcd_writel(ofb->fbi, FBR4, ofb->fbi->fdadr[DMA_OV2_Cr] | 0x3);
 
 	if (wait_for_completion_timeout(&ofb->branch_done, 1 * HZ) == 0)
-		pr_warning("%s: timeout disabling overlay2\n", __func__);
+		pr_warning("%s: timeout disabling overlay2\n",
+			__func__);
 }
 
 static struct pxafb_layer_ops ofb_ops[] = {

drivers/video/pxafb.c +3 -5

-	/* allow only one user at a time */
-	if (atomic_inc_and_test(&ofb->usage))
-		return -EBUSY;
+	if (ofb->usage++ == 0)
+		/* unblank the base framebuffer */
+		fb_blank(&ofb->fbi->fb, FB_BLANK_UNBLANK);
 
-	/* unblank the base framebuffer */
-	fb_blank(&ofb->fbi->fb, FB_BLANK_UNBLANK);
 	return 0;
 }
 

drivers/video/pxafb.c +20 -7

-	atomic_dec(&ofb->usage);
-	ofb->ops->disable(ofb);
-
-	free_pages_exact(ofb->video_mem, ofb->video_mem_size);
-	ofb->video_mem = NULL;
-	ofb->video_mem_size = 0;
+	if (--ofb->usage == 0) {
+		ofb->ops->disable(ofb);
+		ofb->fb.var.height	= -1;
+		ofb->fb.var.width	= -1;
+		ofb->fb.var.xres = ofb->fb.var.xres_virtual = 0;
+		ofb->fb.var.yres = ofb->fb.var.yres_virtual = 0;
+
+		mutex_lock(&ofb->fb.mm_lock);
+		ofb->fb.fix.smem_start	= 0;
+		ofb->fb.fix.smem_len	= 0;
+		mutex_unlock(&ofb->fb.mm_lock);
+
+		if (ofb->video_mem) {
+			free_pages_exact(ofb->video_mem, ofb->video_mem_size);
+			ofb->video_mem = NULL;
+			ofb->video_mem_size = 0;
+		}
+	}
 	return 0;
 }
 

@@ -817,7 +835,8 @@ static int overlayfb_map_video_memory(struct
 pxafb_layer *ofb) if (ofb->video_mem_size >= size)
 			return 0;
 
-		free_pages_exact(ofb->video_mem, ofb->video_mem_size);
+		/* don't re-allocate: userspace may have the buffer mapped */
+		return -EINVAL;
 	}
 
 	ofb->video_mem = alloc_pages_exact(size, GFP_KERNEL | __GFP_ZERO);

drivers/video/pxafb.c +1 -1

-	atomic_set(&ofb->usage, 0);
+	ofb->usage = 0;
 	ofb->fbi = fbi;
 	init_completion(&ofb->branch_done);

}

drivers/video/pxafb.c +0 -0

@@ -1368,7 +1387,8 @@ static int pxafb_activate_var(struct

fb_var_screeninfo *var, (lcd_readl(fbi, LCCR3) != fbi->reg_lccr3) ||

drivers/video/pxafb.c +4 -2

 	    (lcd_readl(fbi, LCCR4) != fbi->reg_lccr4) ||
 	    (lcd_readl(fbi, FDADR0) != fbi->fdadr[0]) ||
-	    (lcd_readl(fbi, FDADR1) != fbi->fdadr[1]))
+	    ((fbi->lccr0 & LCCR0_SDS) &&
+	    (lcd_readl(fbi, FDADR1) != fbi->fdadr[1])))
 		pxafb_schedule_work(fbi, C_REENABLE);
 
 	return 0;

@@ -1420,7 +1440,8 @@ static void pxafb_enable_controller(struct pxafb_info
 *fbi) lcd_writel(fbi, LCCR0, fbi->reg_lccr0 & ~LCCR0_ENB);
 
 	lcd_writel(fbi, FDADR0, fbi->fdadr[0]);
-	lcd_writel(fbi, FDADR1, fbi->fdadr[1]);
+	if (fbi->lccr0 & LCCR0_SDS)
+		lcd_writel(fbi, FDADR1, fbi->fdadr[1]);
 	lcd_writel(fbi, LCCR0, fbi->reg_lccr0 | LCCR0_ENB);
 }
 

drivers/video/pxafb.h +0 -0

index 2353521..84e3ae1 100644

drivers/video/pxafb.h +1 -1

-	atomic_t		usage;
+	uint32_t		usage;
 	uint32_t		control[2];
 
 	struct pxafb_layer_ops	*ops;

Why are you getting rid of the atomic operations ?

Besides, "if (ofb->usage++ == 0)" looks suspicious, especially if you later declare it as uint32_t.

drivers/video/pxafb.c static int overlayfb_release(struct fb_info *info, · line 741

Quoted 4 lines

drivers/video/pxafb.c +0 -0

@@ -733,12 +739,24 @@ static int overlayfb_release(struct fb_info *info,
 int user) {
 	struct pxafb_layer *ofb = (struct pxafb_layer*) info;
 

DTTO, why no atomic?

drivers/video/pxafb.c static void __devinit init_pxafb_overlay(struct · line 913

Quoted 6 lines

drivers/video/pxafb.c +0 -0

 	ofb->video_mem = alloc_pages_exact(size, GFP_KERNEL | __GFP_ZERO);

@@ -891,7 +910,7 @@ static void __devinit init_pxafb_overlay(struct
 pxafb_info *fbi,
 
 	ofb->id = id;
 	ofb->ops = &ofb_ops[id];

DTTO

Quoted 1 line · Marek Vasut

Why are you getting rid of the atomic operations ?

Because they're idiotic. ?Just because something is called "atomic" doesn't make it so, and this is one instance where it's absolutely useless.

Quoted 4 lines

(quoted diff) +0 -0

@@ -720,12 +726,10 @@ static int overlayfb_open(struct fb_info *info, int user)
 ? ? ? ?if (user == 0)
 ? ? ? ? ? ? ? ?return -ENODEV;
 

Review static int overlayfb_open(struct fb_info *info, int user) · line 731

Quoted 6 lines

(quoted diff) +3 -3

- ? ? ? /* allow only one user at a time */
- ? ? ? if (atomic_inc_and_test(&ofb->usage))
- ? ? ? ? ? ? ? return -EBUSY;
+ ? ? ? if (ofb->usage++ == 0)
+ ? ? ? ? ? ? ? /* unblank the base framebuffer */
+ ? ? ? ? ? ? ? fb_blank(&ofb->fbi->fb, FB_BLANK_UNBLANK);

The change above allows multiple user at a time? Then I guess some other places need to be changed accordingly to avoid the racing conditions.

Quoted 4 lines · Eric Miao

Quoted 4 lines

(quoted diff) +0 -0

@@ -720,12 +726,10 @@ static int overlayfb_open(struct fb_info *info, int user)
 ? ? ? ?if (user == 0)
 ? ? ? ? ? ? ? ?return -ENODEV;
 

Quoted 10 lines

Review static int overlayfb_open(struct fb_info *info, int user) · line 731

Quoted 6 lines

(quoted diff) +3 -3

- ? ? ? /* allow only one user at a time */
- ? ? ? if (atomic_inc_and_test(&ofb->usage))
- ? ? ? ? ? ? ? return -EBUSY;
+ ? ? ? if (ofb->usage++ == 0)
+ ? ? ? ? ? ? ? /* unblank the base framebuffer */
+ ? ? ? ? ? ? ? fb_blank(&ofb->fbi->fb, FB_BLANK_UNBLANK);

The change above allows multiple user at a time? Then I guess some other places need to be changed accordingly to avoid the racing conditions.

You can't prevent multiple users. ?Think threaded applications which share the same set of fds.

Any driver which tries to do so by restricting the number of open()s is simply buggy.

From: "Russell King - ARM Linux" <[email protected]>

Release callback tries to free memory even if it was not allocated in map_video_memory. Fix PXA27x/3xx overlay memory management and make overlay actually work.