PXA270 overlay problem

PXA270 overlay problem

[Vasily Khoruzhick]

Hi, I'm experiencing problems with overlay1/overlay2 on PXA270 using pxafb 
driver. Main problem is overlays just don't work for some reason, and even 
more - after enabling any overlay something weird happens (LCD blinks for a 
0.5 second, and then main plane comes back, no overlay plane is visible), I'm 
getting following messages on dmesg:

[   93.679574] overlay1fb_disable: timeout disabling overlay1
[   95.601537] BUG: Bad page state in process sh  pfn:a1b60
[   95.601645] page:c0456c00 count:0 mapcount:0 mapping:  (null) index:0x0
[   95.601698] page flags: 0x200(arch_1)
[   95.601917] [<c0029610>] (unwind_backtrace+0x0/0xe4) from [<c0073088>] 
(bad_page+0xc8/0xf4)
[   95.602042] [<c0073088>] (bad_page+0xc8/0xf4) from [<c0074088>] 
(get_page_from_freelist+0x378/0x494)
[   95.602167] [<c0074088>] (get_page_from_freelist+0x378/0x494) from 
[<c007426c>] (__alloc_pages_nodemask+0xc8/0x50c)
[   95.602290] [<c007426c>] (__alloc_pages_nodemask+0xc8/0x50c) from 
[<c00746c0>] (__get_free_pages+0x10/0x3c)
[   95.602424] [<c00746c0>] (__get_free_pages+0x10/0x3c) from [<c002b780>] 
(pgd_alloc+0x14/0xd0)
[   95.602581] [<c002b780>] (pgd_alloc+0x14/0xd0) from [<c0036ce0>] 
(mm_init.clone.54+0x98/0xd8)
[   95.602702] [<c0036ce0>] (mm_init.clone.54+0x98/0xd8) from [<c0036edc>] 
(dup_mm+0x60/0x4e0)
[   95.602819] [<c0036edc>] (dup_mm+0x60/0x4e0) from [<c0037a60>] 
(copy_process+0x6cc/0xde4)
[   95.602933] [<c0037a60>] (copy_process+0x6cc/0xde4) from [<c00382fc>] 
(do_fork+0x160/0x334)
[   95.603042] [<c00382fc>] (do_fork+0x160/0x334) from [<c002804c>] 
(sys_fork+0x20/0x24)
[   95.603196] [<c002804c>] (sys_fork+0x20/0x24) from [<c0024fc0>] 
(ret_fast_syscall+0x0/0x2c)
[   95.603264] Disabling lock debugging due to kernel taint
[   95.607171] BUG: Bad page state in process sh  pfn:a1b64
[   95.607282] page:c0456c80 count:0 mapcount:0 mapping:  (null) index:0x0
[   95.607335] page flags: 0x200(arch_1)
[   95.607556] [<c0029610>] (unwind_backtrace+0x0/0xe4) from [<c0073088>] 
(bad_page+0xc8/0xf4)
[   95.607680] [<c0073088>] (bad_page+0xc8/0xf4) from [<c0074088>] 
(get_page_from_freelist+0x378/0x494)
[   95.607805] [<c0074088>] (get_page_from_freelist+0x378/0x494) from 
[<c007426c>] (__alloc_pages_nodemask+0xc8/0x50c)
[   95.607955] [<c007426c>] (__alloc_pages_nodemask+0xc8/0x50c) from 
[<c0083890>] (__pte_alloc+0x20/0x1a4)
[   95.608080] [<c0083890>] (__pte_alloc+0x20/0x1a4) from [<c0084864>] 
(copy_page_range+0xf4/0x42c)
[   95.608236] [<c0084864>] (copy_page_range+0xf4/0x42c) from [<c0037178>] 
(dup_mm+0x2fc/0x4e0)
[   95.608355] [<c0037178>] (dup_mm+0x2fc/0x4e0) from [<c0037a60>] 
(copy_process+0x6cc/0xde4)
[   95.608471] [<c0037a60>] (copy_process+0x6cc/0xde4) from [<c00382fc>] 
(do_fork+0x160/0x334)
[   95.608580] [<c00382fc>] (do_fork+0x160/0x334) from [<c002804c>] 
(sys_fork+0x20/0x24)
[   95.608735] [<c002804c>] (sys_fork+0x20/0x24) from [<c0024fc0>] 
(ret_fast_syscall+0x0/0x2c)

And kernel hangs in few minutes after that.

Sample code is attached, it tries to enable 1st overlay and fill it with some 
color.

Any ideas what can be wrong?

Regards
Vasily
-------------- next part --------------
A non-text attachment was scrubbed...
Name: main.c
Type: text/x-csrc
Size: 1483 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110126/8737df7f/attachment.bin>

PXA270 overlay problem

[Vasily Khoruzhick]

On Monday 31 January 2011 14:37:16 Marek Vasut wrote:
> On Wednesday 26 January 2011 21:46:00 Vasily Khoruzhick wrote:
> > Hi, I'm experiencing problems with overlay1/overlay2 on PXA270 using
> > pxafb driver. Main problem is overlays just don't work for some reason,
> > and even more - after enabling any overlay something weird happens (LCD
> > blinks for a 0.5 second, and then main plane comes back, no overlay
> > plane is visible), I'm getting following messages on dmesg:
> > 
> > [   93.679574] overlay1fb_disable: timeout disabling overlay1
> > [   95.601537] BUG: Bad page state in process sh  pfn:a1b60
> > [   95.601645] page:c0456c00 count:0 mapcount:0 mapping:  (null)
> > index:0x0 [   95.601698] page flags: 0x200(arch_1)
> > [   95.601917] [<c0029610>] (unwind_backtrace+0x0/0xe4) from [<c0073088>]
> > (bad_page+0xc8/0xf4)
> > [   95.602042] [<c0073088>] (bad_page+0xc8/0xf4) from [<c0074088>]
> > (get_page_from_freelist+0x378/0x494)
> > [   95.602167] [<c0074088>] (get_page_from_freelist+0x378/0x494) from
> > [<c007426c>] (__alloc_pages_nodemask+0xc8/0x50c)
> > [   95.602290] [<c007426c>] (__alloc_pages_nodemask+0xc8/0x50c) from
> > [<c00746c0>] (__get_free_pages+0x10/0x3c)
> > [   95.602424] [<c00746c0>] (__get_free_pages+0x10/0x3c) from
> > [<c002b780>] (pgd_alloc+0x14/0xd0)
> > [   95.602581] [<c002b780>] (pgd_alloc+0x14/0xd0) from [<c0036ce0>]
> > (mm_init.clone.54+0x98/0xd8)
> > [   95.602702] [<c0036ce0>] (mm_init.clone.54+0x98/0xd8) from
> > [<c0036edc>] (dup_mm+0x60/0x4e0)
> > [   95.602819] [<c0036edc>] (dup_mm+0x60/0x4e0) from [<c0037a60>]
> > (copy_process+0x6cc/0xde4)
> > [   95.602933] [<c0037a60>] (copy_process+0x6cc/0xde4) from [<c00382fc>]
> > (do_fork+0x160/0x334)
> > [   95.603042] [<c00382fc>] (do_fork+0x160/0x334) from [<c002804c>]
> > (sys_fork+0x20/0x24)
> > [   95.603196] [<c002804c>] (sys_fork+0x20/0x24) from [<c0024fc0>]
> > (ret_fast_syscall+0x0/0x2c)
> > [   95.603264] Disabling lock debugging due to kernel taint
> > [   95.607171] BUG: Bad page state in process sh  pfn:a1b64
> > [   95.607282] page:c0456c80 count:0 mapcount:0 mapping:  (null)
> > index:0x0 [   95.607335] page flags: 0x200(arch_1)
> > [   95.607556] [<c0029610>] (unwind_backtrace+0x0/0xe4) from [<c0073088>]
> > (bad_page+0xc8/0xf4)
> > [   95.607680] [<c0073088>] (bad_page+0xc8/0xf4) from [<c0074088>]
> > (get_page_from_freelist+0x378/0x494)
> > [   95.607805] [<c0074088>] (get_page_from_freelist+0x378/0x494) from
> > [<c007426c>] (__alloc_pages_nodemask+0xc8/0x50c)
> > [   95.607955] [<c007426c>] (__alloc_pages_nodemask+0xc8/0x50c) from
> > [<c0083890>] (__pte_alloc+0x20/0x1a4)
> > [   95.608080] [<c0083890>] (__pte_alloc+0x20/0x1a4) from [<c0084864>]
> > (copy_page_range+0xf4/0x42c)
> > [   95.608236] [<c0084864>] (copy_page_range+0xf4/0x42c) from
> > [<c0037178>] (dup_mm+0x2fc/0x4e0)
> > [   95.608355] [<c0037178>] (dup_mm+0x2fc/0x4e0) from [<c0037a60>]
> > (copy_process+0x6cc/0xde4)
> > [   95.608471] [<c0037a60>] (copy_process+0x6cc/0xde4) from [<c00382fc>]
> > (do_fork+0x160/0x334)
> > [   95.608580] [<c00382fc>] (do_fork+0x160/0x334) from [<c002804c>]
> > (sys_fork+0x20/0x24)
> > [   95.608735] [<c002804c>] (sys_fork+0x20/0x24) from [<c0024fc0>]
> > (ret_fast_syscall+0x0/0x2c)
> > 
> > And kernel hangs in few minutes after that.
> > 
> > Sample code is attached, it tries to enable 1st overlay and fill it with
> > some color.
> > 
> > Any ideas what can be wrong?
> > 
> > Regards
> > Vasily
> 
> Do you -- by any chance -- have any weird changes to the pxafb driver?

Nope, no changes to pxafb driver.

PXA270 overlay problem

[Marek Vasut]

On Wednesday 26 January 2011 21:46:00 Vasily Khoruzhick wrote:
> Hi, I'm experiencing problems with overlay1/overlay2 on PXA270 using pxafb
> driver. Main problem is overlays just don't work for some reason, and even
> more - after enabling any overlay something weird happens (LCD blinks for a
> 0.5 second, and then main plane comes back, no overlay plane is visible),
> I'm getting following messages on dmesg:
> 
> [   93.679574] overlay1fb_disable: timeout disabling overlay1
> [   95.601537] BUG: Bad page state in process sh  pfn:a1b60
> [   95.601645] page:c0456c00 count:0 mapcount:0 mapping:  (null) index:0x0
> [   95.601698] page flags: 0x200(arch_1)
> [   95.601917] [<c0029610>] (unwind_backtrace+0x0/0xe4) from [<c0073088>]
> (bad_page+0xc8/0xf4)
> [   95.602042] [<c0073088>] (bad_page+0xc8/0xf4) from [<c0074088>]
> (get_page_from_freelist+0x378/0x494)
> [   95.602167] [<c0074088>] (get_page_from_freelist+0x378/0x494) from
> [<c007426c>] (__alloc_pages_nodemask+0xc8/0x50c)
> [   95.602290] [<c007426c>] (__alloc_pages_nodemask+0xc8/0x50c) from
> [<c00746c0>] (__get_free_pages+0x10/0x3c)
> [   95.602424] [<c00746c0>] (__get_free_pages+0x10/0x3c) from [<c002b780>]
> (pgd_alloc+0x14/0xd0)
> [   95.602581] [<c002b780>] (pgd_alloc+0x14/0xd0) from [<c0036ce0>]
> (mm_init.clone.54+0x98/0xd8)
> [   95.602702] [<c0036ce0>] (mm_init.clone.54+0x98/0xd8) from [<c0036edc>]
> (dup_mm+0x60/0x4e0)
> [   95.602819] [<c0036edc>] (dup_mm+0x60/0x4e0) from [<c0037a60>]
> (copy_process+0x6cc/0xde4)
> [   95.602933] [<c0037a60>] (copy_process+0x6cc/0xde4) from [<c00382fc>]
> (do_fork+0x160/0x334)
> [   95.603042] [<c00382fc>] (do_fork+0x160/0x334) from [<c002804c>]
> (sys_fork+0x20/0x24)
> [   95.603196] [<c002804c>] (sys_fork+0x20/0x24) from [<c0024fc0>]
> (ret_fast_syscall+0x0/0x2c)
> [   95.603264] Disabling lock debugging due to kernel taint
> [   95.607171] BUG: Bad page state in process sh  pfn:a1b64
> [   95.607282] page:c0456c80 count:0 mapcount:0 mapping:  (null) index:0x0
> [   95.607335] page flags: 0x200(arch_1)
> [   95.607556] [<c0029610>] (unwind_backtrace+0x0/0xe4) from [<c0073088>]
> (bad_page+0xc8/0xf4)
> [   95.607680] [<c0073088>] (bad_page+0xc8/0xf4) from [<c0074088>]
> (get_page_from_freelist+0x378/0x494)
> [   95.607805] [<c0074088>] (get_page_from_freelist+0x378/0x494) from
> [<c007426c>] (__alloc_pages_nodemask+0xc8/0x50c)
> [   95.607955] [<c007426c>] (__alloc_pages_nodemask+0xc8/0x50c) from
> [<c0083890>] (__pte_alloc+0x20/0x1a4)
> [   95.608080] [<c0083890>] (__pte_alloc+0x20/0x1a4) from [<c0084864>]
> (copy_page_range+0xf4/0x42c)
> [   95.608236] [<c0084864>] (copy_page_range+0xf4/0x42c) from [<c0037178>]
> (dup_mm+0x2fc/0x4e0)
> [   95.608355] [<c0037178>] (dup_mm+0x2fc/0x4e0) from [<c0037a60>]
> (copy_process+0x6cc/0xde4)
> [   95.608471] [<c0037a60>] (copy_process+0x6cc/0xde4) from [<c00382fc>]
> (do_fork+0x160/0x334)
> [   95.608580] [<c00382fc>] (do_fork+0x160/0x334) from [<c002804c>]
> (sys_fork+0x20/0x24)
> [   95.608735] [<c002804c>] (sys_fork+0x20/0x24) from [<c0024fc0>]
> (ret_fast_syscall+0x0/0x2c)
> 
> And kernel hangs in few minutes after that.
> 
> Sample code is attached, it tries to enable 1st overlay and fill it with
> some color.
> 
> Any ideas what can be wrong?
> 
> Regards
> Vasily

Do you -- by any chance -- have any weird changes to the pxafb driver?

PXA270 overlay problem

[Russell King - ARM Linux]

On Wed, Jan 26, 2011 at 10:46:00PM +0200, Vasily Khoruzhick wrote:
> Hi, I'm experiencing problems with overlay1/overlay2 on PXA270 using pxafb 
> driver. Main problem is overlays just don't work for some reason, and even 
> more - after enabling any overlay something weird happens (LCD blinks for a 
> 0.5 second, and then main plane comes back, no overlay plane is visible), I'm 
> getting following messages on dmesg:
> 
> [   93.679574] overlay1fb_disable: timeout disabling overlay1
> [   95.601537] BUG: Bad page state in process sh  pfn:a1b60
> [   95.601645] page:c0456c00 count:0 mapcount:0 mapping:  (null) index:0x0
> [   95.601698] page flags: 0x200(arch_1)

Ouch.  PG_arch_1 is our 'dcache clean' bit, which we set to indicate
that the page is clean.  This should never be set on a newly allocated
page.

It's cleared by generic code whenever a page enters the free lists, so
newly allocated pages should never have the bit set.

What your report means is that someone did DMA cache maintainence
(specifically, unmapping the page), copied the page as a result of
a COW fault, or called flush_dcache_page() on an already free'd page.

Maybe the pages were mapped into userspace, meanwhile someone free'd
the pages.

And yes, I can see one way that this could happen:

- open overlay
- map buffer
- set framebuffer parameters
   (free's mapped buffer, leaving the mapped one in place, creates new buffer)
- close overlay

Maybe another way:

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

So if two users open the overlay, both map it, and then one closes, the
memory backing the overlay gets freed - meanwhile the other user still
has it mapped etc.

The alloc/free stuff in there just looks really dangerous to me.

PXA270 overlay problem

[Vasily Khoruzhick]

On Monday 31 January 2011 15:04:14 Russell King - ARM Linux wrote:
> On Wed, Jan 26, 2011 at 10:46:00PM +0200, Vasily Khoruzhick wrote:
> > Hi, I'm experiencing problems with overlay1/overlay2 on PXA270 using
> > pxafb driver. Main problem is overlays just don't work for some reason,
> > and even more - after enabling any overlay something weird happens (LCD
> > blinks for a 0.5 second, and then main plane comes back, no overlay
> > plane is visible), I'm getting following messages on dmesg:
> > 
> > [   93.679574] overlay1fb_disable: timeout disabling overlay1
> > [   95.601537] BUG: Bad page state in process sh  pfn:a1b60
> > [   95.601645] page:c0456c00 count:0 mapcount:0 mapping:  (null)
> > index:0x0 [   95.601698] page flags: 0x200(arch_1)
> 
> Ouch.  PG_arch_1 is our 'dcache clean' bit, which we set to indicate
> that the page is clean.  This should never be set on a newly allocated
> page.
> 
> It's cleared by generic code whenever a page enters the free lists, so
> newly allocated pages should never have the bit set.
> 
> What your report means is that someone did DMA cache maintainence
> (specifically, unmapping the page), copied the page as a result of
> a COW fault, or called flush_dcache_page() on an already free'd page.
> 
> Maybe the pages were mapped into userspace, meanwhile someone free'd
> the pages.
> 
> And yes, I can see one way that this could happen:
> 
> - open overlay
> - map buffer
> - set framebuffer parameters
>    (free's mapped buffer, leaving the mapped one in place, creates new
> buffer) - close overlay

But I map framebuffer only after FBIOPUT_VSCREENINFO ioctl.

> Maybe another way:
> 
> 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);
> 
> So if two users open the overlay, both map it, and then one closes, the
> memory backing the overlay gets freed - meanwhile the other user still
> has it mapped etc.

Again, there's only one user - my app.

> The alloc/free stuff in there just looks really dangerous to me.

Yep, it looks dangerous.

Regards
Vasily

PXA270 overlay problem

[Russell King - ARM Linux]

On Mon, Jan 31, 2011 at 07:08:48PM +0200, Vasily Khoruzhick wrote:
> On Monday 31 January 2011 15:04:14 Russell King - ARM Linux wrote:
> > On Wed, Jan 26, 2011 at 10:46:00PM +0200, Vasily Khoruzhick wrote:
> > > Hi, I'm experiencing problems with overlay1/overlay2 on PXA270 using
> > > pxafb driver. Main problem is overlays just don't work for some reason,
> > > and even more - after enabling any overlay something weird happens (LCD
> > > blinks for a 0.5 second, and then main plane comes back, no overlay
> > > plane is visible), I'm getting following messages on dmesg:
> > > 
> > > [   93.679574] overlay1fb_disable: timeout disabling overlay1
> > > [   95.601537] BUG: Bad page state in process sh  pfn:a1b60
> > > [   95.601645] page:c0456c00 count:0 mapcount:0 mapping:  (null)
> > > index:0x0 [   95.601698] page flags: 0x200(arch_1)
> > 
> > Ouch.  PG_arch_1 is our 'dcache clean' bit, which we set to indicate
> > that the page is clean.  This should never be set on a newly allocated
> > page.
> > 
> > It's cleared by generic code whenever a page enters the free lists, so
> > newly allocated pages should never have the bit set.
> > 
> > What your report means is that someone did DMA cache maintainence
> > (specifically, unmapping the page), copied the page as a result of
> > a COW fault, or called flush_dcache_page() on an already free'd page.
> > 
> > Maybe the pages were mapped into userspace, meanwhile someone free'd
> > the pages.
> > 
> > And yes, I can see one way that this could happen:
> > 
> > - open overlay
> > - map buffer
> > - set framebuffer parameters
> >    (free's mapped buffer, leaving the mapped one in place, creates new
> > buffer) - close overlay
> 
> But I map framebuffer only after FBIOPUT_VSCREENINFO ioctl.
> 
> > Maybe another way:
> > 
> > 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);
> > 
> > So if two users open the overlay, both map it, and then one closes, the
> > memory backing the overlay gets freed - meanwhile the other user still
> > has it mapped etc.
> 
> Again, there's only one user - my app.

I didn't look any deeper so I can't say - but it feels very much like this
kind of thing is responsible for your problem.

Things actually get worse if I look at the driver:

static int overlayfb_open(struct fb_info *info, int user)
{
        /* allow only one user at a time */
        if (atomic_inc_and_test(&ofb->usage))
                return -EBUSY;

This is rubbish.  atomic_inc_and_test(v) does:

	val = *v;
	val += 1;
	*v = val;

	return val == 0;

So this doesn't stop multiple opens (and arguably you _can't_ prevent
multiple opens anyway.)

Anyway, I think it would be worth fixing this, and seeing what the effect
is.  Note that one of the side effects of one of this changes is that you
only get one attempt at increasing the memory size in FBIOPUT_VSCREENINFO.
Once the buffer has been allocated, we never change it - as there is no
way of knowing whether it's mapped or not.

The other change is that we properly remove all references to the
allocated memory when closing the device - which ensures that an open()
followed by mmap() with no FBIOPUT_VSCREENINFO call will always fail.

Lastly, it does allow concurrent opens, but makes sure that we have the
necessary number of closes before freeing the buffer.

Please give this a try and see whether it makes any difference for you.

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

diff --git a/drivers/video/pxafb.c b/drivers/video/pxafb.c
index 825b665..f73227f 100644
--- a/drivers/video/pxafb.c
+++ b/drivers/video/pxafb.c
@@ -720,12 +720,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 +731,18 @@ 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 == 0) {
+		ofb->ops->disable(ofb);
 
-	free_pages_exact(ofb->video_mem, ofb->video_mem_size);
-	ofb->video_mem = NULL;
-	ofb->video_mem_size = 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);
+
+		free_pages_exact(ofb->video_mem, ofb->video_mem_size);
+		ofb->video_mem = NULL;
+		ofb->video_mem_size = 0;
+	}
 	return 0;
 }
 
@@ -817,7 +821,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 +896,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);
 }
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;

PXA270 overlay problem

[Vasily Khoruzhick]

On Monday 31 January 2011 19:39:29 Russell King - ARM Linux wrote:
> On Mon, Jan 31, 2011 at 07:08:48PM +0200, Vasily Khoruzhick wrote:
> > On Monday 31 January 2011 15:04:14 Russell King - ARM Linux wrote:
> > > On Wed, Jan 26, 2011 at 10:46:00PM +0200, Vasily Khoruzhick wrote:
> > > > Hi, I'm experiencing problems with overlay1/overlay2 on PXA270 using
> > > > pxafb driver. Main problem is overlays just don't work for some
> > > > reason, and even more - after enabling any overlay something weird
> > > > happens (LCD blinks for a 0.5 second, and then main plane comes
> > > > back, no overlay plane is visible), I'm getting following messages
> > > > on dmesg:
> > > > 
> > > > [   93.679574] overlay1fb_disable: timeout disabling overlay1
> > > > [   95.601537] BUG: Bad page state in process sh  pfn:a1b60
> > > > [   95.601645] page:c0456c00 count:0 mapcount:0 mapping:  (null)
> > > > index:0x0 [   95.601698] page flags: 0x200(arch_1)
> > > 
> > > Ouch.  PG_arch_1 is our 'dcache clean' bit, which we set to indicate
> > > that the page is clean.  This should never be set on a newly allocated
> > > page.
> > > 
> > > It's cleared by generic code whenever a page enters the free lists, so
> > > newly allocated pages should never have the bit set.
> > > 
> > > What your report means is that someone did DMA cache maintainence
> > > (specifically, unmapping the page), copied the page as a result of
> > > a COW fault, or called flush_dcache_page() on an already free'd page.
> > > 
> > > Maybe the pages were mapped into userspace, meanwhile someone free'd
> > > the pages.
> > > 
> > > And yes, I can see one way that this could happen:
> > > 
> > > - open overlay
> > > - map buffer
> > > - set framebuffer parameters
> > > 
> > >    (free's mapped buffer, leaving the mapped one in place, creates new
> > > 
> > > buffer) - close overlay
> > 
> > But I map framebuffer only after FBIOPUT_VSCREENINFO ioctl.
> > 
> > > Maybe another way:
> > > 
> > > 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);
> > > 
> > > So if two users open the overlay, both map it, and then one closes, the
> > > memory backing the overlay gets freed - meanwhile the other user still
> > > has it mapped etc.
> > 
> > Again, there's only one user - my app.
> 
> I didn't look any deeper so I can't say - but it feels very much like this
> kind of thing is responsible for your problem.
> 
> Things actually get worse if I look at the driver:
> 
> static int overlayfb_open(struct fb_info *info, int user)
> {
>         /* allow only one user at a time */
>         if (atomic_inc_and_test(&ofb->usage))
>                 return -EBUSY;
> 
> This is rubbish.  atomic_inc_and_test(v) does:
> 
> 	val = *v;
> 	val += 1;
> 	*v = val;
> 
> 	return val == 0;
> 
> So this doesn't stop multiple opens (and arguably you _can't_ prevent
> multiple opens anyway.)
> 
> Anyway, I think it would be worth fixing this, and seeing what the effect
> is.  Note that one of the side effects of one of this changes is that you
> only get one attempt at increasing the memory size in FBIOPUT_VSCREENINFO.
> Once the buffer has been allocated, we never change it - as there is no
> way of knowing whether it's mapped or not.
> 
> The other change is that we properly remove all references to the
> allocated memory when closing the device - which ensures that an open()
> followed by mmap() with no FBIOPUT_VSCREENINFO call will always fail.
> 
> Lastly, it does allow concurrent opens, but makes sure that we have the
> necessary number of closes before freeing the buffer.
> 
> Please give this a try and see whether it makes any difference for you.

Overlay still does not work, but now it does not crash system. Driver 
complains:

[   36.062235] overlay1fb_disable: timeout disabling overlay1

Regards
Vasily

PXA270 overlay problem

[Vasily Khoruzhick]

On Monday 31 January 2011 20:35:05 Vasily Khoruzhick wrote:
> On Monday 31 January 2011 19:39:29 Russell King - ARM Linux wrote:
> > On Mon, Jan 31, 2011 at 07:08:48PM +0200, Vasily Khoruzhick wrote:
> > > On Monday 31 January 2011 15:04:14 Russell King - ARM Linux wrote:
> > > > On Wed, Jan 26, 2011 at 10:46:00PM +0200, Vasily Khoruzhick wrote:
> > > > > Hi, I'm experiencing problems with overlay1/overlay2 on PXA270
> > > > > using pxafb driver. Main problem is overlays just don't work for
> > > > > some reason, and even more - after enabling any overlay something
> > > > > weird happens (LCD blinks for a 0.5 second, and then main plane
> > > > > comes back, no overlay plane is visible), I'm getting following
> > > > > messages on dmesg:
> > > > > 
> > > > > [   93.679574] overlay1fb_disable: timeout disabling overlay1
> > > > > [   95.601537] BUG: Bad page state in process sh  pfn:a1b60
> > > > > [   95.601645] page:c0456c00 count:0 mapcount:0 mapping:  (null)
> > > > > index:0x0 [   95.601698] page flags: 0x200(arch_1)
> > > > 
> > > > Ouch.  PG_arch_1 is our 'dcache clean' bit, which we set to indicate
> > > > that the page is clean.  This should never be set on a newly
> > > > allocated page.
> > > > 
> > > > It's cleared by generic code whenever a page enters the free lists,
> > > > so newly allocated pages should never have the bit set.
> > > > 
> > > > What your report means is that someone did DMA cache maintainence
> > > > (specifically, unmapping the page), copied the page as a result of
> > > > a COW fault, or called flush_dcache_page() on an already free'd page.
> > > > 
> > > > Maybe the pages were mapped into userspace, meanwhile someone free'd
> > > > the pages.
> > > > 
> > > > And yes, I can see one way that this could happen:
> > > > 
> > > > - open overlay
> > > > - map buffer
> > > > - set framebuffer parameters
> > > > 
> > > >    (free's mapped buffer, leaving the mapped one in place, creates
> > > >    new
> > > > 
> > > > buffer) - close overlay
> > > 
> > > But I map framebuffer only after FBIOPUT_VSCREENINFO ioctl.
> > > 
> > > > Maybe another way:
> > > > 
> > > > 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);
> > > > 
> > > > So if two users open the overlay, both map it, and then one closes,
> > > > the memory backing the overlay gets freed - meanwhile the other user
> > > > still has it mapped etc.
> > > 
> > > Again, there's only one user - my app.
> > 
> > I didn't look any deeper so I can't say - but it feels very much like
> > this kind of thing is responsible for your problem.
> > 
> > Things actually get worse if I look at the driver:
> > 
> > static int overlayfb_open(struct fb_info *info, int user)
> > {
> > 
> >         /* allow only one user at a time */
> >         if (atomic_inc_and_test(&ofb->usage))
> >         
> >                 return -EBUSY;
> > 
> > This is rubbish.  atomic_inc_and_test(v) does:
> > 	val = *v;
> > 	val += 1;
> > 	*v = val;
> > 	
> > 	return val == 0;
> > 
> > So this doesn't stop multiple opens (and arguably you _can't_ prevent
> > multiple opens anyway.)
> > 
> > Anyway, I think it would be worth fixing this, and seeing what the effect
> > is.  Note that one of the side effects of one of this changes is that you
> > only get one attempt at increasing the memory size in
> > FBIOPUT_VSCREENINFO. Once the buffer has been allocated, we never change
> > it - as there is no way of knowing whether it's mapped or not.
> > 
> > The other change is that we properly remove all references to the
> > allocated memory when closing the device - which ensures that an open()
> > followed by mmap() with no FBIOPUT_VSCREENINFO call will always fail.
> > 
> > Lastly, it does allow concurrent opens, but makes sure that we have the
> > necessary number of closes before freeing the buffer.
> > 
> > Please give this a try and see whether it makes any difference for you.
> 
> Overlay still does not work, but now it does not crash system. Driver
> complains:
> 
> [   36.062235] overlay1fb_disable: timeout disabling overlay1
> 
> Regards
> Vasily

I got it working and now understand why this bug happens. pxafb driver tries 
to enable overlay (and set its params, allocates memory) in fb_set_par 
callback, but it will be called only if vscreeninfo is changed. But it 
disables overlay (and frees memory) on release. I'll will send a patch as soon 
as I fix it.

Regards
Vasily