Re: Mainline OMAP3 breakage (and other OMAP?)

Re: Mainline OMAP3 breakage (and other OMAP?)

[Tony Lindgren]

* Russell King - ARM Linux <linux@arm.linux.org.uk> [101202 13:04]:
> This has been around since October:
> 
> drivers/video/omap2/vram.c: In function ■omap_vram_reserve_sdram_memblock■:
> drivers/video/omap2/vram.c:573: error: ■MEMBLOCK_REAL_LIMIT■ undeclared (first use in this function)
> drivers/video/omap2/vram.c:573: error: (Each undeclared identifier is reported only once
> drivers/video/omap2/vram.c:573: error: for each function it appears in.)
> 
> This requires a trivial one-liner compile fix:
> 
> diff --git a/drivers/video/omap2/vram.c b/drivers/video/omap2/vram.c
> index fed2a72..a8973f0 100644
> --- a/drivers/video/omap2/vram.c
> +++ b/drivers/video/omap2/vram.c
> @@ -570,7 +570,7 @@ void __init omap_vram_reserve_sdram_memblock(void)
>  			return;
>  		}
>  	} else {
> -		paddr = memblock_alloc_base(size, PAGE_SIZE, MEMBLOCK_REAL_LIMIT);
> +		paddr = memblock_alloc(size, PAGE_SIZE);
>  	}
>  
>  	omap_vram_add_region(paddr, size);
> 
> which restores the old behaviour before the X86 memblock changes went
> in.  Yes, there may be other changes due to the ioremap stuff, but
> that's really no excuse for not fixing the compile error itself.

Great. Adding fbdev and Tomi to Cc.

Acked-by: Tony Lindgren <tony@atomide.com>

Re: Mainline OMAP3 breakage (and other OMAP?)

[Russell King - ARM Linux]

On Thu, Dec 02, 2010 at 01:58:38PM -0800, Tony Lindgren wrote:
> * Russell King - ARM Linux <linux@arm.linux.org.uk> [101202 13:04]:
> > This has been around since October:
> > 
> > drivers/video/omap2/vram.c: In function ■omap_vram_reserve_sdram_memblock■:
> > drivers/video/omap2/vram.c:573: error: ■MEMBLOCK_REAL_LIMIT■ undeclared (first use in this function)
> > drivers/video/omap2/vram.c:573: error: (Each undeclared identifier is reported only once
> > drivers/video/omap2/vram.c:573: error: for each function it appears in.)
> > 
> > This requires a trivial one-liner compile fix:
> > 
> > diff --git a/drivers/video/omap2/vram.c b/drivers/video/omap2/vram.c
> > index fed2a72..a8973f0 100644
> > --- a/drivers/video/omap2/vram.c
> > +++ b/drivers/video/omap2/vram.c
> > @@ -570,7 +570,7 @@ void __init omap_vram_reserve_sdram_memblock(void)
> >  			return;
> >  		}
> >  	} else {
> > -		paddr = memblock_alloc_base(size, PAGE_SIZE, MEMBLOCK_REAL_LIMIT);
> > +		paddr = memblock_alloc(size, PAGE_SIZE);
> >  	}
> >  
> >  	omap_vram_add_region(paddr, size);
> > 
> > which restores the old behaviour before the X86 memblock changes went
> > in.  Yes, there may be other changes due to the ioremap stuff, but
> > that's really no excuse for not fixing the compile error itself.
> 
> Great. Adding fbdev and Tomi to Cc.
> 
> Acked-by: Tony Lindgren <tony@atomide.com>

http://marc.info/?l=linux-omap&w=2&r=1&s=MEMBLOCK_REAL_LIMIT%20vram&q=b

There have been patches posted throughout November to fix this, but
the problem is they're not making it to mainline.  It needs chasing
until someone does the right thing and sends one variant of the above
patch, rather than just leaving it until the ioremap fixes hit
mainline during the next merge window.

Re: Mainline OMAP3 breakage (and other OMAP?)

[Tony Lindgren]

* Russell King - ARM Linux <linux@arm.linux.org.uk> [101202 14:06]:
> On Thu, Dec 02, 2010 at 01:58:38PM -0800, Tony Lindgren wrote:
> > * Russell King - ARM Linux <linux@arm.linux.org.uk> [101202 13:04]:
> > > This has been around since October:
> > > 
> > > drivers/video/omap2/vram.c: In function ■omap_vram_reserve_sdram_memblock■:
> > > drivers/video/omap2/vram.c:573: error: ■MEMBLOCK_REAL_LIMIT■ undeclared (first use in this function)
> > > drivers/video/omap2/vram.c:573: error: (Each undeclared identifier is reported only once
> > > drivers/video/omap2/vram.c:573: error: for each function it appears in.)
> > > 
> > > This requires a trivial one-liner compile fix:
> > > 
> > > diff --git a/drivers/video/omap2/vram.c b/drivers/video/omap2/vram.c
> > > index fed2a72..a8973f0 100644
> > > --- a/drivers/video/omap2/vram.c
> > > +++ b/drivers/video/omap2/vram.c
> > > @@ -570,7 +570,7 @@ void __init omap_vram_reserve_sdram_memblock(void)
> > >  			return;
> > >  		}
> > >  	} else {
> > > -		paddr = memblock_alloc_base(size, PAGE_SIZE, MEMBLOCK_REAL_LIMIT);
> > > +		paddr = memblock_alloc(size, PAGE_SIZE);
> > >  	}
> > >  
> > >  	omap_vram_add_region(paddr, size);
> > > 
> > > which restores the old behaviour before the X86 memblock changes went
> > > in.  Yes, there may be other changes due to the ioremap stuff, but
> > > that's really no excuse for not fixing the compile error itself.
> > 
> > Great. Adding fbdev and Tomi to Cc.
> > 
> > Acked-by: Tony Lindgren <tony@atomide.com>
> 
> http://marc.info/?l=linux-omap&w=2&r=1&s=MEMBLOCK_REAL_LIMIT%20vram&q=b
> 
> There have been patches posted throughout November to fix this, but
> the problem is they're not making it to mainline.  It needs chasing
> until someone does the right thing and sends one variant of the above
> patch, rather than just leaving it until the ioremap fixes hit
> mainline during the next merge window.

Yes this should go in during the -rc for sure.

I suggest you merge this but let's wait a bit and check if Tomi
already has a similar fix queued for the -rc series.

Regards,

Tony

Re: Mainline OMAP3 breakage (and other OMAP?)

[Paul Mundt]

On Thu, Dec 02, 2010 at 02:32:08PM -0800, Tony Lindgren wrote:
> * Russell King - ARM Linux <linux@arm.linux.org.uk> [101202 14:06]:
> > On Thu, Dec 02, 2010 at 01:58:38PM -0800, Tony Lindgren wrote:
> > > * Russell King - ARM Linux <linux@arm.linux.org.uk> [101202 13:04]:
> > > > This has been around since October:
> > > > 
> > > > drivers/video/omap2/vram.c: In function ???omap_vram_reserve_sdram_memblock???:
> > > > drivers/video/omap2/vram.c:573: error: ???MEMBLOCK_REAL_LIMIT??? undeclared (first use in this function)
> > > > drivers/video/omap2/vram.c:573: error: (Each undeclared identifier is reported only once
> > > > drivers/video/omap2/vram.c:573: error: for each function it appears in.)
> > > > 
> > > > This requires a trivial one-liner compile fix:
> > > > 
> > > > diff --git a/drivers/video/omap2/vram.c b/drivers/video/omap2/vram.c
> > > > index fed2a72..a8973f0 100644
> > > > --- a/drivers/video/omap2/vram.c
> > > > +++ b/drivers/video/omap2/vram.c
> > > > @@ -570,7 +570,7 @@ void __init omap_vram_reserve_sdram_memblock(void)
> > > >  			return;
> > > >  		}
> > > >  	} else {
> > > > -		paddr = memblock_alloc_base(size, PAGE_SIZE, MEMBLOCK_REAL_LIMIT);
> > > > +		paddr = memblock_alloc(size, PAGE_SIZE);
> > > >  	}
> > > >  
> > > >  	omap_vram_add_region(paddr, size);
> > > > 
> > > > which restores the old behaviour before the X86 memblock changes went
> > > > in.  Yes, there may be other changes due to the ioremap stuff, but
> > > > that's really no excuse for not fixing the compile error itself.
> > > 
> > > Great. Adding fbdev and Tomi to Cc.
> > > 
> > > Acked-by: Tony Lindgren <tony@atomide.com>
> > 
> > http://marc.info/?l=linux-omap&w=2&r=1&s=MEMBLOCK_REAL_LIMIT%20vram&q=b
> > 
> > There have been patches posted throughout November to fix this, but
> > the problem is they're not making it to mainline.  It needs chasing
> > until someone does the right thing and sends one variant of the above
> > patch, rather than just leaving it until the ioremap fixes hit
> > mainline during the next merge window.
> 
> Yes this should go in during the -rc for sure.
> 
> I suggest you merge this but let's wait a bit and check if Tomi
> already has a similar fix queued for the -rc series.
> 
This has been fixed since -rc2.

Re: Mainline OMAP3 breakage (and other OMAP?)

[Russell King - ARM Linux]

On Fri, Dec 03, 2010 at 12:09:55PM +0900, Paul Mundt wrote:
> On Thu, Dec 02, 2010 at 02:32:08PM -0800, Tony Lindgren wrote:
> > * Russell King - ARM Linux <linux@arm.linux.org.uk> [101202 14:06]:
> > > On Thu, Dec 02, 2010 at 01:58:38PM -0800, Tony Lindgren wrote:
> > > > * Russell King - ARM Linux <linux@arm.linux.org.uk> [101202 13:04]:
> > > > > This has been around since October:
> > > > > 
> > > > > drivers/video/omap2/vram.c: In function ???omap_vram_reserve_sdram_memblock???:
> > > > > drivers/video/omap2/vram.c:573: error: ???MEMBLOCK_REAL_LIMIT??? undeclared (first use in this function)
> > > > > drivers/video/omap2/vram.c:573: error: (Each undeclared identifier is reported only once
> > > > > drivers/video/omap2/vram.c:573: error: for each function it appears in.)
> > > > > 
> > > > > This requires a trivial one-liner compile fix:
> > > > > 
> > > > > diff --git a/drivers/video/omap2/vram.c b/drivers/video/omap2/vram.c
> > > > > index fed2a72..a8973f0 100644
> > > > > --- a/drivers/video/omap2/vram.c
> > > > > +++ b/drivers/video/omap2/vram.c
> > > > > @@ -570,7 +570,7 @@ void __init omap_vram_reserve_sdram_memblock(void)
> > > > >  			return;
> > > > >  		}
> > > > >  	} else {
> > > > > -		paddr = memblock_alloc_base(size, PAGE_SIZE, MEMBLOCK_REAL_LIMIT);
> > > > > +		paddr = memblock_alloc(size, PAGE_SIZE);
> > > > >  	}
> > > > >  
> > > > >  	omap_vram_add_region(paddr, size);
> > > > > 
> > > > > which restores the old behaviour before the X86 memblock changes went
> > > > > in.  Yes, there may be other changes due to the ioremap stuff, but
> > > > > that's really no excuse for not fixing the compile error itself.
> > > > 
> > > > Great. Adding fbdev and Tomi to Cc.
> > > > 
> > > > Acked-by: Tony Lindgren <tony@atomide.com>
> > > 
> > > http://marc.info/?l=linux-omap&w=2&r=1&s=MEMBLOCK_REAL_LIMIT%20vram&q=b
> > > 
> > > There have been patches posted throughout November to fix this, but
> > > the problem is they're not making it to mainline.  It needs chasing
> > > until someone does the right thing and sends one variant of the above
> > > patch, rather than just leaving it until the ioremap fixes hit
> > > mainline during the next merge window.
> > 
> > Yes this should go in during the -rc for sure.
> > 
> > I suggest you merge this but let's wait a bit and check if Tomi
> > already has a similar fix queued for the -rc series.
> > 
> This has been fixed since -rc2.

So it is.  However, the ioremap fix is wrong.

        } else {
                paddr = memblock_alloc(size, PAGE_SIZE);
        }

        memblock_free(paddr, size);
        memblock_remove(paddr, size);

Didn't I say that such blocks were supposed to be aligned to a 2MB
boundary - and the size also so aligned?

http://lists.arm.linux.org.uk/lurker/message/20101011.152516.893a6088.en.html

It at least needs to be 1MB as that's the section size.

Re: Mainline OMAP3 breakage (and other OMAP?)

[Felipe Contreras]

On Fri, Dec 3, 2010 at 10:41 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Fri, Dec 03, 2010 at 12:09:55PM +0900, Paul Mundt wrote:
>> This has been fixed since -rc2.
>
> So it is.  However, the ioremap fix is wrong.
>
>        } else {
>                paddr = memblock_alloc(size, PAGE_SIZE);
>        }
>
>        memblock_free(paddr, size);
>        memblock_remove(paddr, size);
>
> Didn't I say that such blocks were supposed to be aligned to a 2MB
> boundary - and the size also so aligned?

I asked you if there was something special and I didn't receive any response:
http://article.gmane.org/gmane.linux.kernel/1048434

So, why SZ_2M?

-- 
Felipe Contreras

Re: Mainline OMAP3 breakage (and other OMAP?)

[Russell King - ARM Linux]

On Fri, Dec 03, 2010 at 02:10:23PM +0200, Felipe Contreras wrote:
> On Fri, Dec 3, 2010 at 10:41 AM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Fri, Dec 03, 2010 at 12:09:55PM +0900, Paul Mundt wrote:
> >> This has been fixed since -rc2.
> >
> > So it is.  However, the ioremap fix is wrong.
> >
> >        } else {
> >                paddr = memblock_alloc(size, PAGE_SIZE);
> >        }
> >
> >        memblock_free(paddr, size);
> >        memblock_remove(paddr, size);
> >
> > Didn't I say that such blocks were supposed to be aligned to a 2MB
> > boundary - and the size also so aligned?
> 
> I asked you if there was something special and I didn't receive any response:
> http://article.gmane.org/gmane.linux.kernel/1048434

So you thought you could ride rough-shod over my statement and ignore it?
Thank you for treating me like shit, I'll remember that for future and
treat you in a similar manner.

> So, why SZ_2M?

Firstly, that's the granularity which we allocate page tables - one
Linux page table covers 2MB of memory.  We want to avoid creating page
tables for the main memory mapping as that increases TLB pressure through
the use of additional TLB entries, and more page table walks.

Plus, we never used to allow the kernel's direct memory mapping to be
mapped at anything less than section size - this restriction has since
been lifted due to OMAP SRAM problems, but I'd rather we stuck with it
to ensure that we have proper behaviour from all parts of the system.

Secondly, we don't want to end up with lots of fragmentation at the end
of the memory mapping as that'll reduce performance, not only by making
the pfn_valid() search more expensive.

Emsuring a minimum allocation size and alignment makes sure that the
regions can be coalesced together into one block, and minimises run-time
expenses.

So please, 2MB, or if you object, at the _very_ _least_ 1MB.  But
definitely not PAGE_SIZE.

Re: Mainline OMAP3 breakage (and other OMAP?)

[Felipe Contreras]

On Fri, Dec 3, 2010 at 3:07 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Fri, Dec 03, 2010 at 02:10:23PM +0200, Felipe Contreras wrote:
>> On Fri, Dec 3, 2010 at 10:41 AM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>> > On Fri, Dec 03, 2010 at 12:09:55PM +0900, Paul Mundt wrote:
>> >> This has been fixed since -rc2.
>> >
>> > So it is.  However, the ioremap fix is wrong.
>> >
>> >        } else {
>> >                paddr = memblock_alloc(size, PAGE_SIZE);
>> >        }
>> >
>> >        memblock_free(paddr, size);
>> >        memblock_remove(paddr, size);
>> >
>> > Didn't I say that such blocks were supposed to be aligned to a 2MB
>> > boundary - and the size also so aligned?
>>
>> I asked you if there was something special and I didn't receive any response:
>> http://article.gmane.org/gmane.linux.kernel/1048434
>
> So you thought you could ride rough-shod over my statement and ignore it?
> Thank you for treating me like shit, I'll remember that for future and
> treat you in a similar manner.

No, you didn't make any statement, just provided code. I thought SZ_2M
might have just been an example for something that required that
alignment, thus my request for clarification.

>> So, why SZ_2M?
>
> Firstly, that's the granularity which we allocate page tables - one
> Linux page table covers 2MB of memory.  We want to avoid creating page
> tables for the main memory mapping as that increases TLB pressure through
> the use of additional TLB entries, and more page table walks.
>
> Plus, we never used to allow the kernel's direct memory mapping to be
> mapped at anything less than section size - this restriction has since
> been lifted due to OMAP SRAM problems, but I'd rather we stuck with it
> to ensure that we have proper behaviour from all parts of the system.
>
> Secondly, we don't want to end up with lots of fragmentation at the end
> of the memory mapping as that'll reduce performance, not only by making
> the pfn_valid() search more expensive.
>
> Emsuring a minimum allocation size and alignment makes sure that the
> regions can be coalesced together into one block, and minimises run-time
> expenses.
>
> So please, 2MB, or if you object, at the _very_ _least_ 1MB.  But
> definitely not PAGE_SIZE.

Fair enough. If nobody beats me to it I would send patches to fix
that, although perhaps it would make sense to write a helper function
that does exactly this.

Cheers.

-- 
Felipe Contreras

Re: Mainline OMAP3 breakage (and other OMAP?)

[Tomi Valkeinen]

Hi,

On Fri, 2010-12-03 at 13:07 +0000, ext Russell King - ARM Linux wrote:

> So please, 2MB, or if you object, at the _very_ _least_ 1MB.  But
> definitely not PAGE_SIZE.

Here's a patch for this. Works for me on OMAP3430SDP. If the patch is
ok, I'll send a pull request to Paul.



From 6c54704730626e2683e05574b3cbba966980c956 Mon Sep 17 00:00:00 2001
From: Tomi Valkeinen <tomi.valkeinen@nokia.com>
Date: Tue, 14 Dec 2010 14:16:59 +0200
Subject: [PATCH] OMAP: DSS: VRAM: Align start & size of vram to 2M

Align the start address and size of VRAM area to 2M as per comments from
Russell King:

> > So, why SZ_2M?
>
> Firstly, that's the granularity which we allocate page tables - one
> Linux page table covers 2MB of memory.  We want to avoid creating page
> tables for the main memory mapping as that increases TLB pressure through
> the use of additional TLB entries, and more page table walks.
>
> Plus, we never used to allow the kernel's direct memory mapping to be
> mapped at anything less than section size - this restriction has since
> been lifted due to OMAP SRAM problems, but I'd rather we stuck with it
> to ensure that we have proper behaviour from all parts of the system.
>
> Secondly, we don't want to end up with lots of fragmentation at the end
> of the memory mapping as that'll reduce performance, not only by making
> the pfn_valid() search more expensive.
>
> Emsuring a minimum allocation size and alignment makes sure that the
> regions can be coalesced together into one block, and minimises run-time
> expenses.
>
> So please, 2MB, or if you object, at the _very_ _least_ 1MB.  But
> definitely not PAGE_SIZE.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@nokia.com>
---
 drivers/video/omap2/vram.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/video/omap2/vram.c b/drivers/video/omap2/vram.c
index 2fd7e52..9441e2e 100644
--- a/drivers/video/omap2/vram.c
+++ b/drivers/video/omap2/vram.c
@@ -551,7 +551,7 @@ void __init omap_vram_reserve_sdram_memblock(void)
 	if (!size)
 		return;
 
-	size = PAGE_ALIGN(size);
+	size = ALIGN(size, SZ_2M);
 
 	if (paddr) {
 		if (paddr & ~PAGE_MASK) {
@@ -576,7 +576,7 @@ void __init omap_vram_reserve_sdram_memblock(void)
 			return;
 		}
 	} else {
-		paddr = memblock_alloc(size, PAGE_SIZE);
+		paddr = memblock_alloc(size, SZ_2M);
 	}
 
 	memblock_free(paddr, size);
-- 
1.7.1

Re: Mainline OMAP3 breakage (and other OMAP?)

[Tony Lindgren]

* Tomi Valkeinen <tomi.valkeinen@nokia.com> [101214 04:23]:
> >
> > So please, 2MB, or if you object, at the _very_ _least_ 1MB.  But
> > definitely not PAGE_SIZE.

Acked-by: Tony Lindgren <tony@atomide.com>
 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@nokia.com>
> ---
>  drivers/video/omap2/vram.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/video/omap2/vram.c b/drivers/video/omap2/vram.c
> index 2fd7e52..9441e2e 100644
> --- a/drivers/video/omap2/vram.c
> +++ b/drivers/video/omap2/vram.c
> @@ -551,7 +551,7 @@ void __init omap_vram_reserve_sdram_memblock(void)
>  	if (!size)
>  		return;
>  
> -	size = PAGE_ALIGN(size);
> +	size = ALIGN(size, SZ_2M);
>  
>  	if (paddr) {
>  		if (paddr & ~PAGE_MASK) {
> @@ -576,7 +576,7 @@ void __init omap_vram_reserve_sdram_memblock(void)
>  			return;
>  		}
>  	} else {
> -		paddr = memblock_alloc(size, PAGE_SIZE);
> +		paddr = memblock_alloc(size, SZ_2M);
>  	}
>  
>  	memblock_free(paddr, size);
> -- 
> 1.7.1
> 
> 
>