From: "Russell King - ARM Linux" <redacted>
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(-)
@@ -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);return0;}
@@ -733,12 +737,24 @@ static int overlayfb_release(struct fb_info *info, int user){structpxafb_layer*ofb=(structpxafb_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;+}+}return0;}
@@ -817,7 +833,8 @@ static int overlayfb_map_video_memory(struct pxafb_layer *ofb)if(ofb->video_mem_size>=size)return0;-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);
@@ -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");return0;}
@@ -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);return0;
From: Russell King - ARM Linux <hidden> Date: 2011-02-13 13:31:16
Eric - ping?
On Wed, Feb 02, 2011 at 10:46:59PM +0200, Vasily Khoruzhick wrote:
quoted hunk
From: "Russell King - ARM Linux" <redacted>
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(-)
@@ -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);return0;}
@@ -733,12 +737,24 @@ static int overlayfb_release(struct fb_info *info, int user){structpxafb_layer*ofb=(structpxafb_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;+}+}return0;}
@@ -817,7 +833,8 @@ static int overlayfb_map_video_memory(struct pxafb_layer *ofb)if(ofb->video_mem_size>=size)return0;-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);
@@ -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");return0;}
@@ -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);return0;
From: Eric Miao <hidden> Date: 2011-02-15 07:35:44
On Thu, Feb 3, 2011 at 4:46 AM, Vasily Khoruzhick [off-list ref] wrote:
From: "Russell King - ARM Linux" <redacted>
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:
@@ -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?
quoted hunk
- /* 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)
@@ -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,
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.
@@ -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?
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.
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.
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.
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.
From: Eric Miao <hidden> Date: 2011-02-15 15:12:10
On Tue, Feb 15, 2011 at 9:51 PM, Vasily Khoruzhick [off-list ref] wrote:
On Tuesday 15 February 2011 15:36:07 you wrote:
quoted
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
On Tuesday 15 February 2011 17:12:10 Eric Miao wrote:
On Tue, Feb 15, 2011 at 9:51 PM, Vasily Khoruzhick [off-list ref]
wrote:
quoted
On Tuesday 15 February 2011 15:36:07 you wrote:
quoted
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?
From: Russell King - ARM Linux <redacted>
From: Russell King - ARM Linux <redacted>
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(-)
@@ -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);return0;}
@@ -733,12 +739,24 @@ static int overlayfb_release(struct fb_info *info, int user){structpxafb_layer*ofb=(structpxafb_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;+}+}return0;}
@@ -817,7 +835,8 @@ static int overlayfb_map_video_memory(struct pxafb_layer *ofb)if(ofb->video_mem_size>=size)return0;-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);
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(-)
@@ -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");return0;}
@@ -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);
From: Russell King - ARM Linux <redacted>
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(-)
@@ -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);return0;}
@@ -733,12 +737,24 @@ static int overlayfb_release(struct fb_info *info, int user){structpxafb_layer*ofb=(structpxafb_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;+}+}return0;}
@@ -817,7 +833,8 @@ static int overlayfb_map_video_memory(struct pxafb_layer *ofb)if(ofb->video_mem_size>=size)return0;-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);
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(-)
@@ -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");return0;}
@@ -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);
From: Marek Vasut <hidden> Date: 2011-02-20 18:46:20
On Sunday 20 February 2011 16:02:25 Vasily Khoruzhick wrote:
quoted hunk
From: Russell King - ARM Linux <redacted>
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(-)
@@ -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,
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
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(-)
@@ -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);return0;}
@@ -733,12 +741,15 @@ static int overlayfb_release(struct fb_info *info, int user){structpxafb_layer*ofb=(structpxafb_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--;+}return0;}
@@ -794,7 +805,7 @@ static int overlayfb_check_var(struct fb_var_screeninfo *var,return0;}-staticintoverlayfb_map_video_memory(structpxafb_layer*ofb)+staticintoverlayfb_check_video_memory(structpxafb_layer*ofb){structfb_var_screeninfo*var=&ofb->fb.var;intpfor=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)return0;--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;-return0;+return-EINVAL;}staticintoverlayfb_set_par(structfb_info*info)
@@ -841,7 +836,7 @@ static int overlayfb_set_par(struct fb_info *info)structfb_var_screeninfo*var=&info->var;intxpos,ypos,pfor,bpp,ret;-ret=overlayfb_map_video_memory(ofb);+ret=overlayfb_check_video_memory(ofb);if(ret)returnret;
@@ -904,20 +899,54 @@ static inline int pxafb_overlay_supported(void)return0;}-staticint__devinitpxafb_overlay_init(structpxafb_info*fbi)+staticint__devinitpxafb_overlay_map_video_memory(structpxafb_info*pxafb,+structpxafb_layer*ofb)+{+/* We assume that user will use at most video_mem_size for overlay fb,+*anyway,it'suselesstouse16bppmainplaneand24bppoverlay+*/+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;++return0;+}++staticvoid__devinitpxafb_overlay_init(structpxafb_info*fbi){inti,ret;if(!pxafb_overlay_supported())-return0;+return;for(i=0;i<2;i++){-init_pxafb_overlay(fbi,&fbi->overlay[i],i);-ret=register_framebuffer(&fbi->overlay[i].fb);+structpxafb_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);-returnret;+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");-return0;}staticvoid__devexitpxafb_overlay_exit(structpxafb_info*fbi)
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(-)
@@ -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);
From: Eric Miao <hidden> Date: 2011-03-16 13:16:29
Applied.
On Fri, Mar 11, 2011 at 5:20 PM, Vasily Khoruzhick [off-list ref] wrote:
quoted hunk
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(-)
/* 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");
}
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
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(-)
From: Eric Miao <hidden> Date: 2011-03-16 13:16:42
Applied.
On Fri, Mar 11, 2011 at 5:20 PM, Vasily Khoruzhick [off-list ref] wrote:
quoted hunk
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(-)
From: Marek Vasut <hidden> Date: 2011-03-11 21:34:10
On Friday 11 March 2011 10:20:47 Vasily Khoruzhick wrote:
quoted hunk
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(-)
@@ -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
quoted hunk
+ /* 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,
From: Marek Vasut <hidden> Date: 2011-02-17 18:17:41
On Thursday 17 February 2011 08:43:07 Vasily Khoruzhick wrote:
quoted hunk
From: Russell King - ARM Linux <redacted>
From: Russell King - ARM Linux <redacted>
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(-)
@@ -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.
quoted hunk
- /* 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;
From: Russell King - ARM Linux <hidden> Date: 2011-02-17 18:56:09
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.
quoted
@@ -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.
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. ;(
From: Russell King - ARM Linux <hidden> Date: 2011-02-15 09:48:08
On Tue, Feb 15, 2011 at 03:35:44PM +0800, Eric Miao wrote:
quoted
@@ -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.
From: Eric Miao <hidden> Date: 2011-02-15 11:43:07
On Tue, Feb 15, 2011 at 5:48 PM, Russell King - ARM Linux
[off-list ref] wrote:
On Tue, Feb 15, 2011 at 03:35:44PM +0800, Eric Miao wrote:
quoted
quoted
@@ -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.
Hi Vasily,
On Wed, Feb 02, 2011 at 10:46:59PM +0200, Vasily Khoruzhick wrote:
From: "Russell King - ARM Linux" <redacted>
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 |