[PATCH] ARM: PXA: Make PXA27x/PXA3xx overlay actually work

[PATCH] ARM: PXA: Make PXA27x/PXA3xx overlay actually work

[Vasily Khoruzhick]

From: "Russell King - ARM Linux" <linux@arm.linux.org.uk>

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 <anarsoul@gmail.com>
---
 drivers/video/pxafb.c |   86 ++++++++++++++++++++++++++++++------------------
 drivers/video/pxafb.h |    2 +-
 2 files changed, 55 insertions(+), 33 deletions(-)

diff --git a/drivers/video/pxafb.c b/drivers/video/pxafb.c
index 825b665..33f607a 100644
--- a/drivers/video/pxafb.c
+++ b/drivers/video/pxafb.c
@@ -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);
diff --git a/drivers/video/pxafb.h b/drivers/video/pxafb.h
index 2353521..84e3ae1 100644
--- a/drivers/video/pxafb.h
+++ b/drivers/video/pxafb.h
@@ -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

Re: [PATCH] ARM: PXA: Make PXA27x/PXA3xx overlay actually work

[Russell King - ARM Linux]

Eric - ping?

On Wed, Feb 02, 2011 at 10:46:59PM +0200, Vasily Khoruzhick wrote:
> From: "Russell King - ARM Linux" <linux@arm.linux.org.uk>
> 
> 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 <anarsoul@gmail.com>
> ---
>  drivers/video/pxafb.c |   86 ++++++++++++++++++++++++++++++------------------
>  drivers/video/pxafb.h |    2 +-
>  2 files changed, 55 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/video/pxafb.c b/drivers/video/pxafb.c
> index 825b665..33f607a 100644
> --- a/drivers/video/pxafb.c
> +++ b/drivers/video/pxafb.c
> @@ -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);
> diff --git a/drivers/video/pxafb.h b/drivers/video/pxafb.h
> index 2353521..84e3ae1 100644
> --- a/drivers/video/pxafb.h
> +++ b/drivers/video/pxafb.h
> @@ -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
> 

Re: [PATCH] ARM: PXA: Make PXA27x/PXA3xx overlay actually work

[Eric Miao]

On Thu, Feb 3, 2011 at 4:46 AM, Vasily Khoruzhick <anarsoul@gmail.com> wrote:
> From: "Russell King - ARM Linux" <linux@arm.linux.org.uk>
>
> 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.

That's indeed an issue.

>
> 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.

The patch looks generally OK to me, some points for discussion
below though:

>
> Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> ---
>  drivers/video/pxafb.c |   86 ++++++++++++++++++++++++++++++------------------
>  drivers/video/pxafb.h |    2 +-
>  2 files changed, 55 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/video/pxafb.c b/drivers/video/pxafb.c
> index 825b665..33f607a 100644
> --- a/drivers/video/pxafb.c
> +++ b/drivers/video/pxafb.c
> @@ -629,16 +629,19 @@ static void overlay1fb_disable(struct pxafb_layer *ofb)
>  {
>        uint32_t lccr5 = lcd_readl(ofb->fbi, LCCR5);
>

Or maybe simply:

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

this makes this block of change much cleaner.

> -       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);

Ditto.

>
> -       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);

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?

>
> -       /* 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]);

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.

>        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
> +

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

>        init_waitqueue_head(&fbi->ctrlr_wait);
>        INIT_WORK(&fbi->task, pxafb_task);
>        mutex_init(&fbi->ctrlr_lock);
> diff --git a/drivers/video/pxafb.h b/drivers/video/pxafb.h
> index 2353521..84e3ae1 100644
> --- a/drivers/video/pxafb.h
> +++ b/drivers/video/pxafb.h
> @@ -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
>
>

Re: [PATCH] ARM: PXA: Make PXA27x/PXA3xx overlay actually work

[Vasily Khoruzhick]

On Tuesday 15 February 2011 09:35:44 Eric Miao wrote:

> Or maybe simply:
> 
> 	if (lcd_readl(ofb->fbi, OVL1C1) & OVLxC1_OEN)
> 		return;

You mean if (!lcd_readl.....) return? Then OK.

> this makes this block of change much cleaner.
> 
> > -       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] &

<skip>

> >  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);
> 
> 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.

For multiple users driver needs some rework indeed.

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

I prefer to fix it later.

> >        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.

> > +#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.
 
Regards
Vasily

Re: [PATCH] ARM: PXA: Make PXA27x/PXA3xx overlay actually work

[Eric Miao]

>> >        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.

>> > +#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.

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()?

Re: [PATCH] ARM: PXA: Make PXA27x/PXA3xx overlay actually work

[Vasily Khoruzhick]

On Tuesday 15 February 2011 13:51:06 Eric Miao wrote:
> >> >        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.
> 
> >> > +#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.
> 
> 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

Re: [PATCH] ARM: PXA: Make PXA27x/PXA3xx overlay actually work

[Eric Miao]

On Tue, Feb 15, 2011 at 7:58 PM, Vasily Khoruzhick <anarsoul@gmail.com> wrote:
> On Tuesday 15 February 2011 13:51:06 Eric Miao wrote:
>> >> >        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.
>>
>> >> > +#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.
>>
>> 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.
>

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

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

        /* 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;


> Regards
> Vasily
>

Re: [PATCH] ARM: PXA: Make PXA27x/PXA3xx overlay actually work

[Vasily Khoruzhick]

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)
> 
> @@ -925,6 +925,8 @@ static int __devinit pxafb_overlay_init(struct
> pxafb_info *fbi)
> 
>         /* 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?

Re: [PATCH] ARM: PXA: Make PXA27x/PXA3xx overlay actually work

[Eric Miao]

On Tue, Feb 15, 2011 at 9:51 PM, Vasily Khoruzhick <anarsoul@gmail.com> wrote:
> 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)
>>
>> @@ -925,6 +925,8 @@ static int __devinit pxafb_overlay_init(struct
>> pxafb_info *fbi)
>>
>>         /* 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

Re: [PATCH] ARM: PXA: Make PXA27x/PXA3xx overlay actually work

[Vasily Khoruzhick]

On Tuesday 15 February 2011 17:12:10 Eric Miao wrote:
> On Tue, Feb 15, 2011 at 9:51 PM, Vasily Khoruzhick <anarsoul@gmail.com> 
wrote:
> > 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)
> >> 
> >> @@ -925,6 +925,8 @@ static int __devinit pxafb_overlay_init(struct
> >> pxafb_info *fbi)
> >> 
> >>         /* 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?

Ok, will send patches tonight

> Thanks
> - eric

[PATCH 1/2] ARM: PXA: PXAFB: Fix double-free issue.

[Vasily Khoruzhick]

From: Russell King - ARM Linux <linux@arm.linux.org.uk>

From: Russell King - ARM Linux <linux@arm.linux.org.uk>

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 <anarsoul@gmail.com>
---
 drivers/video/pxafb.c |   55 +++++++++++++++++++++++++++++++++---------------
 drivers/video/pxafb.h |    2 +-
 2 files changed, 39 insertions(+), 18 deletions(-)

diff --git a/drivers/video/pxafb.c b/drivers/video/pxafb.c
index 825b665..c6aad56 100644
--- a/drivers/video/pxafb.c
+++ b/drivers/video/pxafb.c
@@ -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[] = {
@@ -720,12 +728,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 +739,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 +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);
@@ -891,7 +910,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);
 }
@@ -1368,7 +1387,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 +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);
 }
 
diff --git a/drivers/video/pxafb.h b/drivers/video/pxafb.h
index 2353521..84e3ae1 100644
--- a/drivers/video/pxafb.h
+++ b/drivers/video/pxafb.h
@@ -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

[PATCH 2/2] ARM: PXA: PXAFB: fix plane Z-ordering problem

[Vasily Khoruzhick]

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

Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
---
 drivers/video/pxafb.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/video/pxafb.c b/drivers/video/pxafb.c
index c6aad56..590a99a 100644
--- a/drivers/video/pxafb.c
+++ b/drivers/video/pxafb.c
@@ -942,8 +942,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;
 }
@@ -1827,6 +1825,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

Re: [PATCH 1/2] ARM: PXA: PXAFB: Fix double-free issue.

[Russell King - ARM Linux]

On Thu, Feb 17, 2011 at 09:43:07AM +0200, Vasily Khoruzhick wrote:
> From: Russell King - ARM Linux <linux@arm.linux.org.uk>
> 
> From: Russell King - ARM Linux <linux@arm.linux.org.uk>

No need for two From: lines.

> @@ -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.

> @@ -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;
> +

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

> @@ -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__);

No need for this change.

Re: [PATCH 1/2] ARM: PXA: PXAFB: Fix double-free issue.

[Vasily Khoruzhick]

On Thursday 17 February 2011 13:03:21 Russell King - ARM Linux wrote:
> On Thu, Feb 17, 2011 at 09:43:07AM +0200, Vasily Khoruzhick wrote:
> > From: Russell King - ARM Linux <linux@arm.linux.org.uk>
> > 
> > From: Russell King - ARM Linux <linux@arm.linux.org.uk>
> 
> No need for two From: lines.

It was added accidently, sorry

> > @@ -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.
> 
> > @@ -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;
> > +
> 
> You don't describe this change in the change log, and this wasn't in my
> patch.

I did, "- Disable overlay only if it was enabled."

> > @@ -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__);
> 
> No need for this change.

Ok

[PATCH v2 1/3] ARM: PXA: PXAFB: Fix double-free issue

[Vasily Khoruzhick]

From: Russell King - ARM Linux <linux@arm.linux.org.uk>

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 <anarsoul@gmail.com>
---
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(-)

diff --git a/drivers/video/pxafb.c b/drivers/video/pxafb.c
index 825b665..41a499f 100644
--- a/drivers/video/pxafb.c
+++ b/drivers/video/pxafb.c
@@ -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));
@@ -687,6 +690,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));
@@ -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);
 }
@@ -1368,7 +1385,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 +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);
 }
 
diff --git a/drivers/video/pxafb.h b/drivers/video/pxafb.h
index 2353521..84e3ae1 100644
--- a/drivers/video/pxafb.h
+++ b/drivers/video/pxafb.h
@@ -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

[PATCH v2 2/3] ARM: PXA: PXAFB: Fix plane Z-ordering problem

[Vasily Khoruzhick]

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

Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
---
 drivers/video/pxafb.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/video/pxafb.c b/drivers/video/pxafb.c
index 41a499f..54a9ab8 100644
--- a/drivers/video/pxafb.c
+++ b/drivers/video/pxafb.c
@@ -940,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;
 }
@@ -1825,6 +1823,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

[PATCH v2 3/3] ARM: PXA: PXAFB: Fix typo in ypos assignment

[Vasily Khoruzhick]

Sascha Hauer <s.hauer@pengutronix.de> pointed that
ypos takes value of xpos due to typo.

Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
---
 drivers/video/pxafb.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/video/pxafb.c b/drivers/video/pxafb.c
index 54a9ab8..80ea7e6 100644
--- a/drivers/video/pxafb.c
+++ b/drivers/video/pxafb.c
@@ -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) |
-- 
1.7.4

Re: [PATCH v2 3/3] ARM: PXA: PXAFB: Fix typo in ypos assignment

[Marek Vasut]

On Sunday 20 February 2011 16:02:27 Vasily Khoruzhick wrote:
> Sascha Hauer <s.hauer@pengutronix.de> pointed that
> ypos takes value of xpos due to typo.

Looks good,

Acked-by: Marek Vasut <marek.vasut@gmail.com>
> 
> Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> ---
>  drivers/video/pxafb.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/video/pxafb.c b/drivers/video/pxafb.c
> index 54a9ab8..80ea7e6 100644
> --- a/drivers/video/pxafb.c
> +++ b/drivers/video/pxafb.c
> @@ -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) |

Re: [PATCH v2 1/3] ARM: PXA: PXAFB: Fix double-free issue

[Marek Vasut]

On Sunday 20 February 2011 16:02:25 Vasily Khoruzhick wrote:
> From: Russell King - ARM Linux <linux@arm.linux.org.uk>
> 
> 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 <anarsoul@gmail.com>
> ---
> 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(-)
> 
> diff --git a/drivers/video/pxafb.c b/drivers/video/pxafb.c
> index 825b665..41a499f 100644
> --- a/drivers/video/pxafb.c
> +++ b/drivers/video/pxafb.c
> @@ -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;
> +

Maybe you can even avoid reading LCCR5 above ;-)

>  	lcd_writel(ofb->fbi, OVL1C1, ofb->control[0] & ~OVLxC1_OEN);
> 
>  	lcd_writel(ofb->fbi, LCSR1, LCSR1_BS(1));
> @@ -687,6 +690,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;
> +

Hm?

>  	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);
>  }
> @@ -1368,7 +1385,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 +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);
>  }
> 
> diff --git a/drivers/video/pxafb.h b/drivers/video/pxafb.h
> index 2353521..84e3ae1 100644
> --- a/drivers/video/pxafb.h
> +++ b/drivers/video/pxafb.h
> @@ -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;

Re: [PATCH v2 1/3] ARM: PXA: PXAFB: Fix double-free issue

[Vasily Khoruzhick]

On Sunday 20 February 2011 20:46:20 Marek Vasut wrote:

> > @@ -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;
> > +
> 
> Maybe you can even avoid reading LCCR5 above ;-)

Agreed.

Will resend soon.

Re: [PATCH v2 1/3] ARM: PXA: PXAFB: Fix double-free issue

[Vasily Khoruzhick]

Please discard this series.

PXA270/3xx overlay memory management is still broken - it was not good 
decision to allocate memory on open/close - after some time OS just does not 
have 115kb of contiguous memory, so it fails to allocate overlay fb memory 
after ~1h of uptime.

I'll rework it and resend soon.

Regards
Vasily

[PATCH 1/4] ARM: PXA: PXAFB: rework pxafb overlay memory management

[Vasily Khoruzhick]

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 <anarsoul@gmail.com>
---
 drivers/video/pxafb.c |  121 ++++++++++++++++++++++++++++++++-----------------
 drivers/video/pxafb.h |    3 +-
 2 files changed, 81 insertions(+), 43 deletions(-)

diff --git a/drivers/video/pxafb.c b/drivers/video/pxafb.c
index 825b665..0764759 100644
--- a/drivers/video/pxafb.c
+++ b/drivers/video/pxafb.c
@@ -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);
 
@@ -720,12 +730,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 +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)
@@ -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);
 }
@@ -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 */
 	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) ||
 	    (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);
 }
 
diff --git a/drivers/video/pxafb.h b/drivers/video/pxafb.h
index 2353521..26ba9fa 100644
--- a/drivers/video/pxafb.h
+++ b/drivers/video/pxafb.h
@@ -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;
-- 
1.7.4.1

[PATCH 2/4] ARM: PXA: PXAFB: Fix plane Z-ordering problem

[Vasily Khoruzhick]

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

Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
---
 drivers/video/pxafb.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/video/pxafb.c b/drivers/video/pxafb.c
index 0764759..e2f643e 100644
--- a/drivers/video/pxafb.c
+++ b/drivers/video/pxafb.c
@@ -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");
 }
 
@@ -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

Re: [PATCH 2/4] ARM: PXA: PXAFB: Fix plane Z-ordering problem

[Eric Miao]

Applied.

On Fri, Mar 11, 2011 at 5:20 PM, Vasily Khoruzhick <anarsoul@gmail.com> wrote:
> pxafb_overlay_init is not right place to change Z-ordering,
> move it to main plane initialization.
>
> Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> ---
>  drivers/video/pxafb.c |    8 ++++++--
>  1 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/video/pxafb.c b/drivers/video/pxafb.c
> index 0764759..e2f643e 100644
> --- a/drivers/video/pxafb.c
> +++ b/drivers/video/pxafb.c
> @@ -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");
>  }
>
> @@ -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
>
>

[PATCH 3/4] ARM: PXA: PXAFB: Fix typo in ypos assignment

[Vasily Khoruzhick]

Sascha Hauer <s.hauer@pengutronix.de> pointed that
ypos takes value of xpos due to typo.

Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
---
 drivers/video/pxafb.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/video/pxafb.c b/drivers/video/pxafb.c
index e2f643e..a3bdcc1 100644
--- a/drivers/video/pxafb.c
+++ b/drivers/video/pxafb.c
@@ -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

Re: [PATCH 3/4] ARM: PXA: PXAFB: Fix typo in ypos assignment

[Marek Vasut]

On Friday 11 March 2011 10:20:49 Vasily Khoruzhick wrote:
> Sascha Hauer <s.hauer@pengutronix.de> pointed that
> ypos takes value of xpos due to typo.

You can mark this patch a resend if it already went through LAKML :)

> 
> Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> ---
>  drivers/video/pxafb.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/video/pxafb.c b/drivers/video/pxafb.c
> index e2f643e..a3bdcc1 100644
> --- a/drivers/video/pxafb.c
> +++ b/drivers/video/pxafb.c
> @@ -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) |

Re: [PATCH 3/4] ARM: PXA: PXAFB: Fix typo in ypos assignment

[Eric Miao]

Applied.

On Fri, Mar 11, 2011 at 5:20 PM, Vasily Khoruzhick <anarsoul@gmail.com> wrote:
> Sascha Hauer <s.hauer@pengutronix.de> pointed that
> ypos takes value of xpos due to typo.
>
> Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> ---
>  drivers/video/pxafb.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/video/pxafb.c b/drivers/video/pxafb.c
> index e2f643e..a3bdcc1 100644
> --- a/drivers/video/pxafb.c
> +++ b/drivers/video/pxafb.c
> @@ -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
>
>

[PATCH 4/4] ARM: PXA: PXAFB: don't disable controller on cpufreq transition if overlay is in use

[Vasily Khoruzhick]

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 <anarsoul@gmail.com>
---
 drivers/video/pxafb.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/video/pxafb.c b/drivers/video/pxafb.c
index a3bdcc1..a2e5b51 100644
--- a/drivers/video/pxafb.c
+++ b/drivers/video/pxafb.c
@@ -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

Re: [PATCH 4/4] ARM: PXA: PXAFB: don't disable controller on cpufreq

[Eric Miao]

Applied.

On Fri, Mar 11, 2011 at 5:20 PM, Vasily Khoruzhick <anarsoul@gmail.com> wrote:
> 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 <anarsoul@gmail.com>
> ---
>  drivers/video/pxafb.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/video/pxafb.c b/drivers/video/pxafb.c
> index a3bdcc1..a2e5b51 100644
> --- a/drivers/video/pxafb.c
> +++ b/drivers/video/pxafb.c
> @@ -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@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

Re: [PATCH 1/4] ARM: PXA: PXAFB: rework pxafb overlay memory management

[Marek Vasut]

On Friday 11 March 2011 10:20:47 Vasily Khoruzhick wrote:
> 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 <anarsoul@gmail.com>
> ---
>  drivers/video/pxafb.c |  121
> ++++++++++++++++++++++++++++++++----------------- drivers/video/pxafb.h | 
>   3 +-
>  2 files changed, 81 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/video/pxafb.c b/drivers/video/pxafb.c
> index 825b665..0764759 100644
> --- a/drivers/video/pxafb.c
> +++ b/drivers/video/pxafb.c
> @@ -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);
> 
> @@ -720,12 +730,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)

TBH I don't like this notation, it feels hard to read. Can you split that ofb-
>usage++ into two parts ?

Cheers

> +		/* 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)
> @@ -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);
>  }
> @@ -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 */
>  	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) ||
>  	    (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);
>  }
> 
> diff --git a/drivers/video/pxafb.h b/drivers/video/pxafb.h
> index 2353521..26ba9fa 100644
> --- a/drivers/video/pxafb.h
> +++ b/drivers/video/pxafb.h
> @@ -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;

Re: [PATCH 1/4] ARM: PXA: PXAFB: rework pxafb overlay memory management

[Vasily Khoruzhick]

On Friday 11 March 2011 23:34:10 Marek Vasut wrote:
> > +	if (ofb->usage++ = 0)
> 
> TBH I don't like this notation, it feels hard to read. Can you split that
> ofb-
> 
> >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 :)

Regards
Vasily

Re: [PATCH 1/4] ARM: PXA: PXAFB: rework pxafb overlay memory management

[Eric Miao]

2011/3/12 Vasily Khoruzhick <anarsoul@gmail.com>:
> On Friday 11 March 2011 23:34:10 Marek Vasut wrote:
>> > +   if (ofb->usage++ = 0)
>>
>> TBH I don't like this notation, it feels hard to read. Can you split that
>> ofb-
>>
>> >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 :)
>

I'm OK with the it. The whole series merged into -devel.

Thanks.
- eric

Re: [PATCH 1/2] ARM: PXA: PXAFB: Fix double-free issue.

[Marek Vasut]

On Thursday 17 February 2011 08:43:07 Vasily Khoruzhick wrote:
> From: Russell King - ARM Linux <linux@arm.linux.org.uk>
> 
> From: Russell King - ARM Linux <linux@arm.linux.org.uk>
> 
> 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 <anarsoul@gmail.com>
> ---
>  drivers/video/pxafb.c |   55
> +++++++++++++++++++++++++++++++++--------------- drivers/video/pxafb.h |  
>  2 +-
>  2 files changed, 39 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/video/pxafb.c b/drivers/video/pxafb.c
> index 825b665..c6aad56 100644
> --- a/drivers/video/pxafb.c
> +++ b/drivers/video/pxafb.c
> @@ -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[] = {
> @@ -720,12 +728,10 @@ static int overlayfb_open(struct fb_info *info, int
> user) if (user = 0)
>  		return -ENODEV;
> 

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.

> -	/* 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 +739,24 @@ static int overlayfb_release(struct fb_info *info,
> int user) {
>  	struct pxafb_layer *ofb = (struct pxafb_layer*) info;
> 

DTTO, why no atomic?

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

DTTO

> -	atomic_set(&ofb->usage, 0);
> +	ofb->usage = 0;
>  	ofb->fbi = fbi;
>  	init_completion(&ofb->branch_done);
>  }
> @@ -1368,7 +1387,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 +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);
>  }
> 
> diff --git a/drivers/video/pxafb.h b/drivers/video/pxafb.h
> index 2353521..84e3ae1 100644
> --- a/drivers/video/pxafb.h
> +++ b/drivers/video/pxafb.h
> @@ -92,7 +92,7 @@ struct pxafb_layer_ops {
>  struct pxafb_layer {
>  	struct fb_info		fb;
>  	int			id;

DTTO, here.

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

Re: [PATCH 1/2] ARM: PXA: PXAFB: Fix double-free issue.

[Russell King - ARM Linux]

On Thu, Feb 17, 2011 at 07:17:41PM +0100, Marek Vasut wrote:
> 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.

The open and release functions are called with a mutex held.  Only
_one_ thread can be inside these at any one time.  So what use does
additionally doing an atomic operation within an already thread-safe
environment gain you?

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

No.  You're not understanding the code.  This is equivalent to:

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

And if you write it like that, then it is obvious.  It's your understanding
of what a post-increment looks like which is suspicious here.

> > @@ -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?

Because this is already a thread-safe code region.

> >  	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

An initializing store by which a machine can write the entire contents in
one instruction _is_ by its very nature atomic.

atomic_t is one of the most over(ab)used types because people just don't
think about the code they're writing. ;(

Re: [PATCH 1/2] ARM: PXA: PXAFB: Fix double-free issue.

[Eric Miao]

On Fri, Feb 18, 2011 at 2:56 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Thu, Feb 17, 2011 at 07:17:41PM +0100, Marek Vasut wrote:
>> 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.

Yep, I completely agree that's useless here.

Re: [PATCH] ARM: PXA: Make PXA27x/PXA3xx overlay actually work

[Russell King - ARM Linux]

On Tue, Feb 15, 2011 at 03:35:44PM +0800, Eric Miao wrote:
> > @@ -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);
> 
> 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.

Re: [PATCH] ARM: PXA: Make PXA27x/PXA3xx overlay actually work

[Eric Miao]

On Tue, Feb 15, 2011 at 5:48 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Tue, Feb 15, 2011 at 03:35:44PM +0800, Eric Miao wrote:
>> > @@ -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);
>>
>> 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.
>

OK, let's go ahead fix the racing conditions later.

Re: [PATCH] ARM: PXA: Make PXA27x/PXA3xx overlay actually work

[Sascha Hauer]

Hi Vasily,

On Wed, Feb 02, 2011 at 10:46:59PM +0200, Vasily Khoruzhick wrote:
> From: "Russell King - ARM Linux" <linux@arm.linux.org.uk>
> 
> 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.

While at pxa overlay support you also might to fix that the xpos field
in var->nonstd is also used for ypos in two places:

ypos = NONSTD_TO_XPOS(var->nonstd);
                ^^^

I stumbled upon this but was not interested enough that moment to create
a patch.

-- 
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 |