Re: [PATCH 2/4] mm: cma: Contiguous Memory Allocator added

Re: [PATCH 2/4] mm: cma: Contiguous Memory Allocator added

[Daniel Walker]

On Tue, 2010-07-20 at 17:51 +0200, Michal Nazarewicz wrote:
> +** Use cases
> +
> +    Lets analyse some imaginary system that uses the CMA to see how
> +    the framework can be used and configured.
> +
> +
> +    We have a platform with a hardware video decoder and a camera
> each
> +    needing 20 MiB of memory in worst case.  Our system is written in
> +    such a way though that the two devices are never used at the same
> +    time and memory for them may be shared.  In such a system the
> +    following two command line arguments would be used:
> +
> +        cma=r=20M cma_map=video,camera=r 

This seems inelegant to me.. It seems like these should be connected
with the drivers themselves vs. doing it on the command like for
everything. You could have the video driver declare it needs 20megs, and
the the camera does the same but both indicate it's shared ..

If you have this disconnected from the drivers it will just cause
confusion, since few will know what these parameters should be for a
given driver set. It needs to be embedded in the kernel.

Daniel

-- 
Sent by an consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH 2/4] mm: cma: Contiguous Memory Allocator added

[Michał Nazarewicz]

On Tue, 20 Jul 2010 20:15:24 +0200, Daniel Walker <dwalker@codeaurora.org> wrote:

> On Tue, 2010-07-20 at 17:51 +0200, Michal Nazarewicz wrote:
>> +** Use cases
>> +
>> +    Lets analyse some imaginary system that uses the CMA to see how
>> +    the framework can be used and configured.
>> +
>> +
>> +    We have a platform with a hardware video decoder and a camera
>> each
>> +    needing 20 MiB of memory in worst case.  Our system is written in
>> +    such a way though that the two devices are never used at the same
>> +    time and memory for them may be shared.  In such a system the
>> +    following two command line arguments would be used:
>> +
>> +        cma=r=20M cma_map=video,camera=r
>
> This seems inelegant to me.. It seems like these should be connected
> with the drivers themselves vs. doing it on the command like for
> everything. You could have the video driver declare it needs 20megs, and
> the the camera does the same but both indicate it's shared ..
>
> If you have this disconnected from the drivers it will just cause
> confusion, since few will know what these parameters should be for a
> given driver set. It needs to be embedded in the kernel.

I see your point but the problem is that devices drivers don't know the
rest of the system neither they know what kind of use cases the system
should support.


Lets say, we have a camera, a JPEG encoder, a video decoder and
scaler (ie. devices that scales raw image).  We want to support the
following 3 use cases:

1. Camera's output is scaled and displayed in real-time.
2. Single frame is taken from camera and saved as JPEG image.
3. A video file is decoded, scaled and displayed.

What is apparent is that camera and video decoder are never running
at the same time.  The same situation is with JPEG encoder and scaler.
 From this knowledge we can construct the following:

   cma=a=10M;b=10M cma_map=camera,video=a;jpeg,scaler=b

This may be a silly example but it shows that the configuration of
memory regions and device->regions mapping should be done after
some investigation rather then from devices which may have not enough
knowledge.


One of the purposes of the CMA framework is to make it let device
drivers completely forget about the memory management and enjoy
a simple API.


CMA core has a cma_defaults() function which can be called from
platform initialisation code.  It makes it easy to provide default
values for the cma and cma_map parameters.  This makes it possible
to provide a default which will work in many/most cases even if
user does not provide custom cma and/or cma_map parameters.


Having said that, some way of letting device drivers request
a region if one has not been defined for them may be a good idea.
I'll have to think about it...

-- 
Best regards,                                        _     _
| Humble Liege of Serenely Enlightened Majesty of  o' \,=./ `o
| Computer Science,  Michał "mina86" Nazarewicz       (o o)
+----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo--

Re: [PATCH 2/4] mm: cma: Contiguous Memory Allocator added

[Daniel Walker]

On Tue, 2010-07-20 at 21:14 +0200, Michał Nazarewicz wrote:
> On Tue, 20 Jul 2010 20:15:24 +0200, Daniel Walker <dwalker@codeaurora.org> wrote:
> 
> > On Tue, 2010-07-20 at 17:51 +0200, Michal Nazarewicz wrote:
> >> +** Use cases
> >> +
> >> +    Lets analyse some imaginary system that uses the CMA to see how
> >> +    the framework can be used and configured.
> >> +
> >> +
> >> +    We have a platform with a hardware video decoder and a camera
> >> each
> >> +    needing 20 MiB of memory in worst case.  Our system is written in
> >> +    such a way though that the two devices are never used at the same
> >> +    time and memory for them may be shared.  In such a system the
> >> +    following two command line arguments would be used:
> >> +
> >> +        cma=r=20M cma_map=video,camera=r
> >
> > This seems inelegant to me.. It seems like these should be connected
> > with the drivers themselves vs. doing it on the command like for
> > everything. You could have the video driver declare it needs 20megs, and
> > the the camera does the same but both indicate it's shared ..
> >
> > If you have this disconnected from the drivers it will just cause
> > confusion, since few will know what these parameters should be for a
> > given driver set. It needs to be embedded in the kernel.
> 
> I see your point but the problem is that devices drivers don't know the
> rest of the system neither they know what kind of use cases the system
> should support.
> 
> 
> Lets say, we have a camera, a JPEG encoder, a video decoder and
> scaler (ie. devices that scales raw image).  We want to support the
> following 3 use cases:
> 
> 1. Camera's output is scaled and displayed in real-time.
> 2. Single frame is taken from camera and saved as JPEG image.
> 3. A video file is decoded, scaled and displayed.
> 
> What is apparent is that camera and video decoder are never running
> at the same time.  The same situation is with JPEG encoder and scaler.
>  From this knowledge we can construct the following:
> 
>    cma=a=10M;b=10M cma_map=camera,video=a;jpeg,scaler=b

It should be implicit tho. If the video driver isn't using the memory
then it should tell your framework that the memory is not used. That way
something else can use it.

(btw, these strings your creating yikes, talk about confusing ..)

> One of the purposes of the CMA framework is to make it let device
> drivers completely forget about the memory management and enjoy
> a simple API.

The driver, and it's maintainer, are really the best people to know how
much memory they need and when it's used/unused. You don't really want
to architect them out.

Daniel

-- 
Sent by an consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH 2/4] mm: cma: Contiguous Memory Allocator added

[Michał Nazarewicz]

On Tue, 20 Jul 2010 21:38:18 +0200, Daniel Walker <dwalker@codeaurora.org> wrote:

> On Tue, 2010-07-20 at 21:14 +0200, Michał Nazarewicz wrote:
>> On Tue, 20 Jul 2010 20:15:24 +0200, Daniel Walker <dwalker@codeaurora.org> wrote:
>>
>> > On Tue, 2010-07-20 at 17:51 +0200, Michal Nazarewicz wrote:
>> >> +** Use cases
>> >> +
>> >> +    Lets analyse some imaginary system that uses the CMA to see how
>> >> +    the framework can be used and configured.
>> >> +
>> >> +
>> >> +    We have a platform with a hardware video decoder and a camera
>> >> each
>> >> +    needing 20 MiB of memory in worst case.  Our system is written in
>> >> +    such a way though that the two devices are never used at the same
>> >> +    time and memory for them may be shared.  In such a system the
>> >> +    following two command line arguments would be used:
>> >> +
>> >> +        cma=r=20M cma_map=video,camera=r
>> >
>> > This seems inelegant to me.. It seems like these should be connected
>> > with the drivers themselves vs. doing it on the command like for
>> > everything. You could have the video driver declare it needs 20megs, and
>> > the the camera does the same but both indicate it's shared ..
>> >
>> > If you have this disconnected from the drivers it will just cause
>> > confusion, since few will know what these parameters should be for a
>> > given driver set. It needs to be embedded in the kernel.
>>
>> I see your point but the problem is that devices drivers don't know the
>> rest of the system neither they know what kind of use cases the system
>> should support.
>>
>>
>> Lets say, we have a camera, a JPEG encoder, a video decoder and
>> scaler (ie. devices that scales raw image).  We want to support the
>> following 3 use cases:
>>
>> 1. Camera's output is scaled and displayed in real-time.
>> 2. Single frame is taken from camera and saved as JPEG image.
>> 3. A video file is decoded, scaled and displayed.
>>
>> What is apparent is that camera and video decoder are never running
>> at the same time.  The same situation is with JPEG encoder and scaler.
>>  From this knowledge we can construct the following:
>>
>>    cma=a=10M;b=10M cma_map=camera,video=a;jpeg,scaler=b
>
> It should be implicit tho. If the video driver isn't using the memory
> then it should tell your framework that the memory is not used. That way
> something else can use it.

What you are asking for is:

	cma=a=100M cma_map=*/*=a

All devices will share the same region so that "if the video driver isn't
using the memory" then "something else can use it". (please excuse me quoting
you, it was stronger then me ;) ).

Driver has to little information to say whether it really stopped using
memory.  Maybe the next call will be to allocate buffers for frames and
initialise the chip?  Sure, some “good enough” defaults can be provided
(and the framework allows that) but still platform architect might need
more power.

> (btw, these strings your creating yikes, talk about confusing ..)

They are not that scary really.  Let's look at cma:

	a=10M;b=10M

Split it on semicolon:

	a=10M
	b=10M

and you see that it defines two regions (a and b) 10M each.

As of cma_map:

	camera,video=a;jpeg,scaler=b

Again split it on semicolon:

	camera,video=a
	jpeg,scaler=b

Now, substitute equal sign by "use(s) region(s)":

	camera,video	use(s) region(s):	a
	jpeg,scaler	use(s) region(s):	b

No black magic here. ;)

>> One of the purposes of the CMA framework is to make it let device
>> drivers completely forget about the memory management and enjoy
>> a simple API.
>
> The driver, and it's maintainer, are really the best people to know how
> much memory they need and when it's used/unused. You don't really want
> to architect them out.

This might be true if there is only one device but even then it's not
always the case.  If many devices need physically-contiguous memory
there is no way for them to communicate and share memory.  For best
performance someone must look at them and say who gets what.

Still, with updated version it will be possible for drivers to use
private regions.

-- 
Best regards,                                        _     _
| Humble Liege of Serenely Enlightened Majesty of  o' \,=./ `o
| Computer Science,  Michał "mina86" Nazarewicz       (o o)
+----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo--

Re: [PATCH 2/4] mm: cma: Contiguous Memory Allocator added

[Mark Brown]

On Tue, Jul 20, 2010 at 09:14:58PM +0200, Micha?? Nazarewicz wrote:
> On Tue, 20 Jul 2010 20:15:24 +0200, Daniel Walker <dwalker@codeaurora.org> wrote:

> > If you have this disconnected from the drivers it will just cause
> > confusion, since few will know what these parameters should be for a
> > given driver set. It needs to be embedded in the kernel.

> I see your point but the problem is that devices drivers don't know the
> rest of the system neither they know what kind of use cases the system
> should support.

If this does need to be configured per system would having platform data
of some kind in the kernel not be a sensible a place to do it, or even
having a way of configuring this at runtime (after all, the set of
currently active users may vary depending on the current configuration
and keeping everything allocated all the time may be wasteful)?

Re: [PATCH 2/4] mm: cma: Contiguous Memory Allocator added

[Michał Nazarewicz]

On Wed, 21 Jul 2010 15:52:30 +0200, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:

> On Tue, Jul 20, 2010 at 09:14:58PM +0200, Micha?? Nazarewicz wrote:
>> On Tue, 20 Jul 2010 20:15:24 +0200, Daniel Walker <dwalker@codeaurora.org> wrote:
>
>> > If you have this disconnected from the drivers it will just cause
>> > confusion, since few will know what these parameters should be for a
>> > given driver set. It needs to be embedded in the kernel.
>
>> I see your point but the problem is that devices drivers don't know the
>> rest of the system neither they know what kind of use cases the system
>> should support.
>
> If this does need to be configured per system would having platform data
> of some kind in the kernel not be a sensible a place to do it,

The current version (and the next version I'm working on) of the code
has cma_defaults() call.  It is intended to be called from platform
initialisation code to provide defaults.

> or even
> having a way of configuring this at runtime (after all, the set of
> currently active users may vary depending on the current configuration
> and keeping everything allocated all the time may be wasteful)?

I am currently working on making the whole thing more dynamic.  I imagine
the list of regions would stay pretty much the same after kernel has
started (that's because one cannot reliably allocate new big contiguous
memory regions) but it will be possible to change the set of rules, etc.


-- 
Best regards,                                        _     _
| Humble Liege of Serenely Enlightened Majesty of  o' \,=./ `o
| Computer Science,  Michał "mina86" Nazarewicz       (o o)
+----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo--

Re: [PATCH 2/4] mm: cma: Contiguous Memory Allocator added

[Daniel Walker]

On Wed, 2010-07-21 at 14:01 +0200, Michał Nazarewicz wrote:

> What you are asking for is:
> 
> 	cma=a=100M cma_map=*/*=a
> 
> All devices will share the same region so that "if the video driver isn't
> using the memory" then "something else can use it". (please excuse me quoting
> you, it was stronger then me ;) ).

Ok ..

> Driver has to little information to say whether it really stopped using
> memory.  Maybe the next call will be to allocate buffers for frames and
> initialise the chip?  Sure, some “good enough” defaults can be provided
> (and the framework allows that) but still platform architect might need
> more power.

I think your talking more about optimization .. You can take that into
account ..

> > (btw, these strings your creating yikes, talk about confusing ..)
> 
> They are not that scary really.  Let's look at cma:
> 
> 	a=10M;b=10M
> 
> Split it on semicolon:
> 
> 	a=10M
> 	b=10M
> 
> and you see that it defines two regions (a and b) 10M each.

I think your assuming a lot .. I've never seen the notation before I
wouldn't assuming there's regions or whatever ..

> As of cma_map:
> 
> 	camera,video=a;jpeg,scaler=b
> 
> Again split it on semicolon:
> 
> 	camera,video=a
> 	jpeg,scaler=b
> 
> Now, substitute equal sign by "use(s) region(s)":
> 
> 	camera,video	use(s) region(s):	a
> 	jpeg,scaler	use(s) region(s):	b
> 
> No black magic here. ;)

It way too complicated .. Users (i.e. not programmers) has to use
this ..

> >> One of the purposes of the CMA framework is to make it let device
> >> drivers completely forget about the memory management and enjoy
> >> a simple API.
> >
> > The driver, and it's maintainer, are really the best people to know how
> > much memory they need and when it's used/unused. You don't really want
> > to architect them out.
> 
> This might be true if there is only one device but even then it's not
> always the case.  If many devices need physically-contiguous memory
> there is no way for them to communicate and share memory.  For best
> performance someone must look at them and say who gets what.

How do you think regular memory allocation work? I mean there's many
devices that all need different amounts of memory and they get along.
Yet your saying it's not possible .

Daniel


-- 
Sent by an consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH 2/4] mm: cma: Contiguous Memory Allocator added

[Michał Nazarewicz]

On Wed, 21 Jul 2010 19:35:50 +0200, Daniel Walker <dwalker@codeaurora.org> wrote:

> On Wed, 2010-07-21 at 14:01 +0200, Michał Nazarewicz wrote:
>
>> What you are asking for is:
>>
>> 	cma=a=100M cma_map=*/*=a
>>
>> All devices will share the same region so that "if the video driver isn't
>> using the memory" then "something else can use it". (please excuse me quoting
>> you, it was stronger then me ;) ).
>
> Ok ..
>
>> Driver has to little information to say whether it really stopped using
>> memory.  Maybe the next call will be to allocate buffers for frames and
>> initialise the chip?  Sure, some “good enough” defaults can be provided
>> (and the framework allows that) but still platform architect might need
>> more power.
>
> I think your talking more about optimization .. You can take that into
> account ..

Well, yes, that's one of the points: to minimise amount of memory reserved
for devices.

>> > (btw, these strings your creating yikes, talk about confusing ..)
>>
>> They are not that scary really.  Let's look at cma:
>>
>> 	a=10M;b=10M
>>
>> Split it on semicolon:
>>
>> 	a=10M
>> 	b=10M
>>
>> and you see that it defines two regions (a and b) 10M each.
>
> I think your assuming a lot .. I've never seen the notation before I
> wouldn't assuming there's regions or whatever ..

That's why there is documentation with grammar included. :)

>> As of cma_map:
>>
>> 	camera,video=a;jpeg,scaler=b
>>
>> Again split it on semicolon:
>>
>> 	camera,video=a
>> 	jpeg,scaler=b
>>
>> Now, substitute equal sign by "use(s) region(s)":
>>
>> 	camera,video	use(s) region(s):	a
>> 	jpeg,scaler	use(s) region(s):	b
>>
>> No black magic here. ;)
>
> It way too complicated .. Users (i.e. not programmers) has to use
> this ..

Not really.  This will probably be used mostly on embedded systems
where users don't have much to say as far as hardware included on the
platform is concerned, etc.  Once a phone, tablet, etc. is released
users will have little need for customising those strings.

On desktop computers on the other hand, the whole framework may be
completely useless as devices are more likely to have IO map or scatter/getter
capabilities.

Plus, as I mentioned above, some “good enough” defaults can be provided.

>> >> One of the purposes of the CMA framework is to make it let device
>> >> drivers completely forget about the memory management and enjoy
>> >> a simple API.
>> >
>> > The driver, and it's maintainer, are really the best people to know how
>> > much memory they need and when it's used/unused. You don't really want
>> > to architect them out.
>>
>> This might be true if there is only one device but even then it's not
>> always the case.  If many devices need physically-contiguous memory
>> there is no way for them to communicate and share memory.  For best
>> performance someone must look at them and say who gets what.
>
> How do you think regular memory allocation work? I mean there's many
> devices that all need different amounts of memory and they get along.
> Yet your saying it's not possible .

Regular memory allocation either does not allow you to allocate big chunks
of memory (kmalloc) or uses MMU (vmalloc).  The purpose of CMA is to provide
a framework for allocators of big physically-contiguous chunks of memory.

If a driver needs several KiB it just uses kmalloc() which handles such
allocations just fine.  However, we are taking about 6MiB full-HD frame
or a photo from 5 megapixel camera.

Currently, drivers are developed which create their own mechanism for
allocating such chunks of memory.  Often based on bootmem.  CMA will unify
all those mechanism and let it easier to manage them plus will allow for
many drivers to share regions.

-- 
Best regards,                                        _     _
| Humble Liege of Serenely Enlightened Majesty of  o' \,=./ `o
| Computer Science,  Michał "mina86" Nazarewicz       (o o)
+----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo--

Re: [PATCH 2/4] mm: cma: Contiguous Memory Allocator added

[Daniel Walker]

On Wed, 2010-07-21 at 20:11 +0200, Michał Nazarewicz wrote:

> >> > (btw, these strings your creating yikes, talk about confusing ..)
> >>
> >> They are not that scary really.  Let's look at cma:
> >>
> >> 	a=10M;b=10M
> >>
> >> Split it on semicolon:
> >>
> >> 	a=10M
> >> 	b=10M
> >>
> >> and you see that it defines two regions (a and b) 10M each.
> >
> > I think your assuming a lot .. I've never seen the notation before I
> > wouldn't assuming there's regions or whatever ..
> 
> That's why there is documentation with grammar included. :)
> 
> >> As of cma_map:
> >>
> >> 	camera,video=a;jpeg,scaler=b
> >>
> >> Again split it on semicolon:
> >>
> >> 	camera,video=a
> >> 	jpeg,scaler=b
> >>
> >> Now, substitute equal sign by "use(s) region(s)":
> >>
> >> 	camera,video	use(s) region(s):	a
> >> 	jpeg,scaler	use(s) region(s):	b
> >>
> >> No black magic here. ;)
> >
> > It way too complicated .. Users (i.e. not programmers) has to use
> > this ..
> 
> Not really.  This will probably be used mostly on embedded systems
> where users don't have much to say as far as hardware included on the
> platform is concerned, etc.  Once a phone, tablet, etc. is released
> users will have little need for customising those strings.

You can't assume that user won't want to reflash their own kernel on the
device. Your assuming way too much.

If you assume they do want their own kernel then they would need this
string from someplace. If your right and this wouldn't need to change,
why bother allowing it to be configured at all ?

Daniel

-- 
Sent by an consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH 2/4] mm: cma: Contiguous Memory Allocator added

[Mark Brown]

On Wed, Jul 21, 2010 at 04:31:35PM +0200, Micha?? Nazarewicz wrote:
> On Wed, 21 Jul 2010 15:52:30 +0200, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:

> > If this does need to be configured per system would having platform data
> > of some kind in the kernel not be a sensible a place to do it,

> The current version (and the next version I'm working on) of the code
> has cma_defaults() call.  It is intended to be called from platform
> initialisation code to provide defaults.

So the command line is just a way of overriding that?  That makes things
a lot nicer - normally the device would use the defaults and the command
line would be used in development.

> > or even
> > having a way of configuring this at runtime (after all, the set of
> > currently active users may vary depending on the current configuration
> > and keeping everything allocated all the time may be wasteful)?

> I am currently working on making the whole thing more dynamic.  I imagine
> the list of regions would stay pretty much the same after kernel has
> started (that's because one cannot reliably allocate new big contiguous
> memory regions) but it will be possible to change the set of rules, etc.

Yes, I think it will be much easier to be able to grab the regions at
startup but hopefully the allocation within those regions can be made
much more dynamic.  This would render most of the configuration syntax
unneeded.

Re: [PATCH 2/4] mm: cma: Contiguous Memory Allocator added

[Michał Nazarewicz]

> On Wed, 2010-07-21 at 20:11 +0200, Michał Nazarewicz wrote:
>> Not really.  This will probably be used mostly on embedded systems
>> where users don't have much to say as far as hardware included on the
>> platform is concerned, etc.  Once a phone, tablet, etc. is released
>> users will have little need for customising those strings.

On Wed, 21 Jul 2010 20:19:08 +0200, Daniel Walker <dwalker@codeaurora.org> wrote:
> You can't assume that user won't want to reflash their own kernel on the
> device. Your assuming way too much.

If user is clever enough to reflash a phone she will find the strings
easy especially that they are provided from: (i) bootloader which is
even less likely to be reflashed and if someone do reflash bootloader
she is a guru who'd know how to make the strings; or (ii) platform
defaults which will be available with the rest of the source code
for the platform.

> If you assume they do want their own kernel then they would need this
> string from someplace. If your right and this wouldn't need to change,
> why bother allowing it to be configured at all ?

Imagine a developer who needs to recompile the kernel and reflash the
device each time she wants to change the configuration...  Command line
arguments seems a better option for development.

And the configuration is needed because it is platform-dependent
so it needs to be set for each platform.

-- 
Best regards,                                        _     _
| Humble Liege of Serenely Enlightened Majesty of  o' \,=./ `o
| Computer Science,  Michał "mina86" Nazarewicz       (o o)
+----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo--

Re: [PATCH 2/4] mm: cma: Contiguous Memory Allocator added

[Michał Nazarewicz]

On Wed, 21 Jul 2010 20:24:58 +0200, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:

> On Wed, Jul 21, 2010 at 04:31:35PM +0200, Micha?? Nazarewicz wrote:
>> On Wed, 21 Jul 2010 15:52:30 +0200, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:
>
>> > If this does need to be configured per system would having platform data
>> > of some kind in the kernel not be a sensible a place to do it,
>
>> The current version (and the next version I'm working on) of the code
>> has cma_defaults() call.  It is intended to be called from platform
>> initialisation code to provide defaults.
>
> So the command line is just a way of overriding that?  That makes things
> a lot nicer - normally the device would use the defaults and the command
> line would be used in development.

Correct.

>> > or even
>> > having a way of configuring this at runtime (after all, the set of
>> > currently active users may vary depending on the current configuration
>> > and keeping everything allocated all the time may be wasteful)?
>
>> I am currently working on making the whole thing more dynamic.  I imagine
>> the list of regions would stay pretty much the same after kernel has
>> started (that's because one cannot reliably allocate new big contiguous
>> memory regions) but it will be possible to change the set of rules, etc.
>
> Yes, I think it will be much easier to be able to grab the regions at
> startup but hopefully the allocation within those regions can be made
> much more dynamic.  This would render most of the configuration syntax
> unneeded.

Not sure what you mean by the last sentence.  Maybe we have different
things in mind?

-- 
Best regards,                                        _     _
| Humble Liege of Serenely Enlightened Majesty of  o' \,=./ `o
| Computer Science,  Michał "mina86" Nazarewicz       (o o)
+----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo--

Re: [PATCH 2/4] mm: cma: Contiguous Memory Allocator added

[Daniel Walker]

On Wed, 2010-07-21 at 20:38 +0200, Michał Nazarewicz wrote:
> > On Wed, 2010-07-21 at 20:11 +0200, Michał Nazarewicz wrote:
> >> Not really.  This will probably be used mostly on embedded systems
> >> where users don't have much to say as far as hardware included on the
> >> platform is concerned, etc.  Once a phone, tablet, etc. is released
> >> users will have little need for customising those strings.
> 
> On Wed, 21 Jul 2010 20:19:08 +0200, Daniel Walker <dwalker@codeaurora.org> wrote:
> > You can't assume that user won't want to reflash their own kernel on the
> > device. Your assuming way too much.
> 
> If user is clever enough to reflash a phone she will find the strings
> easy especially that they are provided from: (i) bootloader which is
> even less likely to be reflashed and if someone do reflash bootloader
> she is a guru who'd know how to make the strings; or (ii) platform
> defaults which will be available with the rest of the source code
> for the platform.

Your, again, assuming all sorts of stuff .. On my phone for example it
is very easy to reflash, personally, I think most devices will be like
that in the future. so you don't _need_ to be clever to reflash the
device.

> > If you assume they do want their own kernel then they would need this
> > string from someplace. If your right and this wouldn't need to change,
> > why bother allowing it to be configured at all ?
> 
> Imagine a developer who needs to recompile the kernel and reflash the
> device each time she wants to change the configuration...  Command line
> arguments seems a better option for development.

So make it a default off debug configuration option ..

Daniel

-- 
Sent by an consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH 2/4] mm: cma: Contiguous Memory Allocator added

[Michał Nazarewicz]

On Wed, 21 Jul 2010 20:58:08 +0200, Daniel Walker <dwalker@codeaurora.org> wrote:

> On Wed, 2010-07-21 at 20:38 +0200, Michał Nazarewicz wrote:
>> > On Wed, 2010-07-21 at 20:11 +0200, Michał Nazarewicz wrote:
>> >> Not really.  This will probably be used mostly on embedded systems
>> >> where users don't have much to say as far as hardware included on the
>> >> platform is concerned, etc.  Once a phone, tablet, etc. is released
>> >> users will have little need for customising those strings.
>>
>> On Wed, 21 Jul 2010 20:19:08 +0200, Daniel Walker <dwalker@codeaurora.org> wrote:
>> > You can't assume that user won't want to reflash their own kernel on the
>> > device. Your assuming way too much.
>>
>> If user is clever enough to reflash a phone she will find the strings
>> easy especially that they are provided from: (i) bootloader which is
>> even less likely to be reflashed and if someone do reflash bootloader
>> she is a guru who'd know how to make the strings; or (ii) platform
>> defaults which will be available with the rest of the source code
>> for the platform.
>
> Your, again, assuming all sorts of stuff .. On my phone for example it
> is very easy to reflash, personally, I think most devices will be like
> that in the future. so you don't _need_ to be clever to reflash the
> device.

Bottom line is: if you reflash the device you (i) get an image from
somewhere and it has the strings in it, (ii) reflash the kernel and
parameters are provided by bootloader so they still remain, (iii)
use platform default strings which you get with the source codes and
include when kernel is built, or (iv) are a guru who knows what to
do.

>> > If you assume they do want their own kernel then they would need this
>> > string from someplace. If your right and this wouldn't need to change,
>> > why bother allowing it to be configured at all ?
>>
>> Imagine a developer who needs to recompile the kernel and reflash the
>> device each time she wants to change the configuration...  Command line
>> arguments seems a better option for development.
>
> So make it a default off debug configuration option ..

I don't really see the point of doing that.  Adding the command line
parameters is really a minor cost so there will be no benefits from
removing it.

-- 
Best regards,                                        _     _
| Humble Liege of Serenely Enlightened Majesty of  o' \,=./ `o
| Computer Science,  Michał "mina86" Nazarewicz       (o o)
+----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo--

Re: [PATCH 2/4] mm: cma: Contiguous Memory Allocator added

[Daniel Walker]

On Wed, 2010-07-21 at 21:21 +0200, Michał Nazarewicz wrote:
> On Wed, 21 Jul 2010 20:58:08 +0200, Daniel Walker <dwalker@codeaurora.org> wrote:
> 
> > On Wed, 2010-07-21 at 20:38 +0200, Michał Nazarewicz wrote:
> >> > On Wed, 2010-07-21 at 20:11 +0200, Michał Nazarewicz wrote:
> >> >> Not really.  This will probably be used mostly on embedded systems
> >> >> where users don't have much to say as far as hardware included on the
> >> >> platform is concerned, etc.  Once a phone, tablet, etc. is released
> >> >> users will have little need for customising those strings.
> >>
> >> On Wed, 21 Jul 2010 20:19:08 +0200, Daniel Walker <dwalker@codeaurora.org> wrote:
> >> > You can't assume that user won't want to reflash their own kernel on the
> >> > device. Your assuming way too much.
> >>
> >> If user is clever enough to reflash a phone she will find the strings
> >> easy especially that they are provided from: (i) bootloader which is
> >> even less likely to be reflashed and if someone do reflash bootloader
> >> she is a guru who'd know how to make the strings; or (ii) platform
> >> defaults which will be available with the rest of the source code
> >> for the platform.
> >
> > Your, again, assuming all sorts of stuff .. On my phone for example it
> > is very easy to reflash, personally, I think most devices will be like
> > that in the future. so you don't _need_ to be clever to reflash the
> > device.
> 
> Bottom line is: if you reflash the device you (i) get an image from
> somewhere and it has the strings in it, (ii) reflash the kernel and
> parameters are provided by bootloader so they still remain, (iii)
> use platform default strings which you get with the source codes and
> include when kernel is built, or (iv) are a guru who knows what to
> do.

What makes you assume that the bootloader would have these strings?
Do your devices have these strings? Maybe mine don't have them.

Assume the strings are gone and you can't find them, or have no idea
what they should be. What do you do then?

> >> > If you assume they do want their own kernel then they would need this
> >> > string from someplace. If your right and this wouldn't need to change,
> >> > why bother allowing it to be configured at all ?
> >>
> >> Imagine a developer who needs to recompile the kernel and reflash the
> >> device each time she wants to change the configuration...  Command line
> >> arguments seems a better option for development.
> >
> > So make it a default off debug configuration option ..
> 
> I don't really see the point of doing that.  Adding the command line
> parameters is really a minor cost so there will be no benefits from
> removing it.

Well, I like my kernel minus bloat so that's a good reason. I don't see
a good reason to keep the interface in a production situation .. Maybe
during development , but really I don't see even a developer needing to
make the kind of changes your suggesting very often.

Daniel

-- 
Sent by an consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH 2/4] mm: cma: Contiguous Memory Allocator added

[Michał Nazarewicz]

On Wed, 21 Jul 2010 21:37:09 +0200, Daniel Walker <dwalker@codeaurora.org> wrote:
> What makes you assume that the bootloader would have these strings?
> Do your devices have these strings? Maybe mine don't have them.

I don't assume.  I only state it as one of the possibilities.

> Assume the strings are gone and you can't find them, or have no idea
> what they should be. What do you do then?

Ask Google?

I have a better question for you though: assume the "mem" parameter is
lost and you have no idea what it should be?  There are many parameters
passed to kernel by bootloader and you could ask about all of them.

Passing cma configuration via command line is one of the possibilities
-- especially convenient during development -- but I would expect platform
defaults in a final product so you may well not need to worry about it.

Bottom line: if you destroyed your device, you are screwed.

>>>> Imagine a developer who needs to recompile the kernel and reflash the
>>>> device each time she wants to change the configuration...  Command line
>>>> arguments seems a better option for development.

>>> So make it a default off debug configuration option ..

>> I don't really see the point of doing that.  Adding the command line
>> parameters is really a minor cost so there will be no benefits from
>> removing it.

> Well, I like my kernel minus bloat so that's a good reason. I don't see
> a good reason to keep the interface in a production situation .. Maybe
> during development , but really I don't see even a developer needing to
> make the kind of changes your suggesting very often.

As I've said, removing the command line parameters would not benefit the
kernel that much.  It's like 1% of the code or less.  On the other hand,
it would add complexity to the CMA framework which is a good reason not
to do that.

Would you also argue about removing all the other kernel parameters as
well? I bet you don't use most of them.  Still they are there because
removing them would add too much complexity to the code (conditional
compilation, etc.).

I'm not saying that removing “bloat” (or unused options if you will)
 from the kernel is a bad thing but there is a line where costs of
doing so negate the benefits.

-- 
Best regards,                                        _     _
| Humble Liege of Serenely Enlightened Majesty of  o' \,=./ `o
| Computer Science,  Michał "mina86" Nazarewicz       (o o)
+----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo--

Re: [PATCH 2/4] mm: cma: Contiguous Memory Allocator added

[Daniel Walker]

On Wed, 2010-07-21 at 21:53 +0200, Michał Nazarewicz wrote:
> On Wed, 21 Jul 2010 21:37:09 +0200, Daniel Walker <dwalker@codeaurora.org> wrote:
> > What makes you assume that the bootloader would have these strings?
> > Do your devices have these strings? Maybe mine don't have them.
> 
> I don't assume.  I only state it as one of the possibilities.
> 
> > Assume the strings are gone and you can't find them, or have no idea
> > what they should be. What do you do then?
> 
> Ask Google?

Exactly, that's why they need to be in the kernel ..

> I have a better question for you though: assume the "mem" parameter is
> lost and you have no idea what it should be?  There are many parameters
> passed to kernel by bootloader and you could ask about all of them.

That's hardware based tho. Of course you need info on what your hardware
is. What your doing isn't based on hardware specifics, it's based on
optimizations.

> Passing cma configuration via command line is one of the possibilities
> -- especially convenient during development -- but I would expect platform
> defaults in a final product so you may well not need to worry about it.

I honestly don't thing the "development" angle flies either , but if you
keep this there's no way it should be enabled for anything but debug.

> > Well, I like my kernel minus bloat so that's a good reason. I don't see
> > a good reason to keep the interface in a production situation .. Maybe
> > during development , but really I don't see even a developer needing to
> > make the kind of changes your suggesting very often.
> 
> As I've said, removing the command line parameters would not benefit the
> kernel that much.  It's like 1% of the code or less.  On the other hand,
> it would add complexity to the CMA framework which is a good reason not
> to do that.

If we allowed everyone to add there little tiny bit of bloat where would
we be?

> Would you also argue about removing all the other kernel parameters as
> well? I bet you don't use most of them.  Still they are there because
> removing them would add too much complexity to the code (conditional
> compilation, etc.).

Your is at a different level of complexity ..

Daniel


-- 
Sent by an consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH 2/4] mm: cma: Contiguous Memory Allocator added

[Michał Nazarewicz]

On Wed, 21 Jul 2010 22:03:24 +0200, Daniel Walker <dwalker@codeaurora.org> wrote:

> On Wed, 2010-07-21 at 21:53 +0200, Michał Nazarewicz wrote:
>> On Wed, 21 Jul 2010 21:37:09 +0200, Daniel Walker <dwalker@codeaurora.org> wrote:
>> > What makes you assume that the bootloader would have these strings?
>> > Do your devices have these strings? Maybe mine don't have them.
>>
>> I don't assume.  I only state it as one of the possibilities.
>>
>> > Assume the strings are gone and you can't find them, or have no idea
>> > what they should be. What do you do then?
>>
>> Ask Google?
>
> Exactly, that's why they need to be in the kernel ..

Right.....  Please show me a place where I've written that it won't be in
the kernel? I keep repeating command line is only one of the possibilities.
I would imagine that in final product defaults from platform would be used
and bootloader would be left alone.

>> I have a better question for you though: assume the "mem" parameter is
>> lost and you have no idea what it should be?  There are many parameters
>> passed to kernel by bootloader and you could ask about all of them.
>
> That's hardware based tho. Of course you need info on what your hardware
> is. What your doing isn't based on hardware specifics, it's based on
> optimizations.
>
>> Passing cma configuration via command line is one of the possibilities
>> -- especially convenient during development -- but I would expect platform
>> defaults in a final product so you may well not need to worry about it.
>
> I honestly don't thing the "development" angle flies either , but if you
> keep this there's no way it should be enabled for anything but debug.

If you are developing the whole platform and optimising the allocators,
etc. it's very convenient.  If you develop something else then it's not
needed but then again it's usually the case that if you develop “foo”
then “bar” is not needed.

>> Would you also argue about removing all the other kernel parameters as
>> well? I bet you don't use most of them.  Still they are there because
>> removing them would add too much complexity to the code (conditional
>> compilation, etc.).
>
> Your is at a different level of complexity ..

Which most of will remain even if the command line parameters were to
be removed.  One needs to specify this configuration somehow and no
matter how you do it it will be complex in one way or another.  In my
code the complexity is parsing of the strings, in a different approach
it would be complex in a different way.

At the same time, the fact that the parameters can be provided via command
line is has a minimal impact on the code.

-- 
Best regards,                                        _     _
| Humble Liege of Serenely Enlightened Majesty of  o' \,=./ `o
| Computer Science,  Michał "mina86" Nazarewicz       (o o)
+----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo--

Re: [PATCH 2/4] mm: cma: Contiguous Memory Allocator added

[Daniel Walker]

On Wed, 2010-07-21 at 22:22 +0200, Michał Nazarewicz wrote:
> On Wed, 21 Jul 2010 22:03:24 +0200, Daniel Walker <dwalker@codeaurora.org> wrote:
> 
> > On Wed, 2010-07-21 at 21:53 +0200, Michał Nazarewicz wrote:
> >> On Wed, 21 Jul 2010 21:37:09 +0200, Daniel Walker <dwalker@codeaurora.org> wrote:
> >> > What makes you assume that the bootloader would have these strings?
> >> > Do your devices have these strings? Maybe mine don't have them.
> >>
> >> I don't assume.  I only state it as one of the possibilities.
> >>
> >> > Assume the strings are gone and you can't find them, or have no idea
> >> > what they should be. What do you do then?
> >>
> >> Ask Google?
> >
> > Exactly, that's why they need to be in the kernel ..
> 
> Right.....  Please show me a place where I've written that it won't be in
> the kernel? I keep repeating command line is only one of the possibilities.
> I would imagine that in final product defaults from platform would be used
> and bootloader would be left alone.

It should never be anyplace else.

Daniel

-- 
Sent by an consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH 2/4] mm: cma: Contiguous Memory Allocator added

[Michał Nazarewicz]

> On Wed, 2010-07-21 at 22:22 +0200, Michał Nazarewicz wrote:
>> Right.....  Please show me a place where I've written that it won't be in
>> the kernel? I keep repeating command line is only one of the possibilities.
>> I would imagine that in final product defaults from platform would be used
>> and bootloader would be left alone.

On Wed, 21 Jul 2010 22:34:32 +0200, Daniel Walker <dwalker@codeaurora.org> wrote:
> It should never be anyplace else.

I disagree.  There are countless “dubug_level” kernel parameters or even
some “printk” related parameters.  Those are completely hardware-independent.
There are also parameters that are hardware dependent but most users won't
care to set them.  That's how the things are: there are some defaults but
you can override them by command line parameters.

-- 
Best regards,                                        _     _
| Humble Liege of Serenely Enlightened Majesty of  o' \,=./ `o
| Computer Science,  Michał "mina86" Nazarewicz       (o o)
+----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo--

Re: [PATCH 2/4] mm: cma: Contiguous Memory Allocator added

[Daniel Walker]

On Wed, 2010-07-21 at 22:43 +0200, Michał Nazarewicz wrote:
> > On Wed, 2010-07-21 at 22:22 +0200, Michał Nazarewicz wrote:
> >> Right.....  Please show me a place where I've written that it won't be in
> >> the kernel? I keep repeating command line is only one of the possibilities.
> >> I would imagine that in final product defaults from platform would be used
> >> and bootloader would be left alone.
> 
> On Wed, 21 Jul 2010 22:34:32 +0200, Daniel Walker <dwalker@codeaurora.org> wrote:
> > It should never be anyplace else.
> 
> I disagree.  There are countless “dubug_level” kernel parameters or even
> some “printk” related parameters.  Those are completely hardware-independent.
> There are also parameters that are hardware dependent but most users won't
> care to set them.  That's how the things are: there are some defaults but
> you can override them by command line parameters.

Your not hearing the issues.. IT'S TOO COMPLEX! Please remove it.

Daniel

-- 
Sent by an consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH 2/4] mm: cma: Contiguous Memory Allocator added

[Michał Nazarewicz]

On Wed, 21 Jul 2010 22:45:43 +0200, Daniel Walker <dwalker@codeaurora.org> wrote:
> Your not hearing the issues.. IT'S TOO COMPLEX! Please remove it.

Remove what exactly?

The command line parameter? It's like 50 lines of code, so I don't
see any benefits.

The possibility to specify the configuration? It would defy the whole
purpose of CMA, so I won't do that.

The complexity has to be there one way or the other and even though
I am aware that less complex code is the better and am trying to
remove unnecessary complexity but saying “remove it” won't make any
good unless you provide me with a better alternative.  At this point,
I am still convinced that the string parameters are the best option.

-- 
Best regards,                                        _     _
| Humble Liege of Serenely Enlightened Majesty of  o' \,=./ `o
| Computer Science,  Michał "mina86" Nazarewicz       (o o)
+----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo--

Re: [PATCH 2/4] mm: cma: Contiguous Memory Allocator added

[Daniel Walker]

On Wed, 2010-07-21 at 22:56 +0200, Michał Nazarewicz wrote:
> On Wed, 21 Jul 2010 22:45:43 +0200, Daniel Walker <dwalker@codeaurora.org> wrote:
> > Your not hearing the issues.. IT'S TOO COMPLEX! Please remove it.
> 
> Remove what exactly?

Remove the command line option and all related code, or make it all a
debug option.

Arguing with me isn't going to help your cause.

Daniel

-- 
Sent by an consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH 2/4] mm: cma: Contiguous Memory Allocator added

[Mark Brown]

On Wed, Jul 21, 2010 at 08:41:12PM +0200, Micha?? Nazarewicz wrote:
> On Wed, 21 Jul 2010 20:24:58 +0200, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:

> >> I am currently working on making the whole thing more dynamic.  I imagine

> > Yes, I think it will be much easier to be able to grab the regions at
> > startup but hopefully the allocation within those regions can be made
> > much more dynamic.  This would render most of the configuration syntax
> > unneeded.

> Not sure what you mean by the last sentence.  Maybe we have different
> things in mind?

I mean that if the drivers are able to request things dynamically and
have some knowledge of their own requirements then that removes the need
to manually specify exactly which regions go to which drivers which
means that most of the complexity of the existing syntax is not needed
since it can be figured out at runtime.

RE: [PATCH 2/4] mm: cma: Contiguous Memory Allocator added

[Marek Szyprowski]

Hello,

On Thursday, July 22, 2010 11:06 AM Mark Brown wrote:

> On Wed, Jul 21, 2010 at 08:41:12PM +0200, Micha?? Nazarewicz wrote:
> > On Wed, 21 Jul 2010 20:24:58 +0200, Mark Brown
> <broonie@opensource.wolfsonmicro.com> wrote:
> 
> > >> I am currently working on making the whole thing more dynamic.  I
> imagine
> 
> > > Yes, I think it will be much easier to be able to grab the regions at
> > > startup but hopefully the allocation within those regions can be made
> > > much more dynamic.  This would render most of the configuration syntax
> > > unneeded.
> 
> > Not sure what you mean by the last sentence.  Maybe we have different
> > things in mind?
> 
> I mean that if the drivers are able to request things dynamically and
> have some knowledge of their own requirements then that removes the need
> to manually specify exactly which regions go to which drivers which
> means that most of the complexity of the existing syntax is not needed
> since it can be figured out at runtime.

The driver may specify memory requirements (like memory address range or
alignment), but it cannot provide enough information to avoid or reduce
memory fragmentation. More than one memory region can be perfectly used
to reduce memory fragmentation IF common usage patterns are known. In
embedded world usually not all integrated device are being used at the
same time. This way some memory regions can be shared by 2 or more devices. 

Just assume that gfx accelerator allocates memory is rather small chunks,
but keeps it while relevant surface is being displayed or processed by
application. It is not surprising that GUI (accelerated by the hardware
engine) is used almost all the time on a mobile device. This usage pattern
would produce a lot of fragmentation in the memory pool that is used by gfx
accelerator. Then we want to run a camera capture device to take a 8Mpix
photo. This require a large contiguous buffer. If we try to allocate it from
common pool it might happen that it is not possible (because of the
fragmentation).

With CMA approach we can create 2 memory regions for this case. One for gfx
accelerator and the other for camera capture device, video decoder or jpeg
decoder, because common usage analysis showed that these 3 devices usually
are not used at the same time.

Best regards
--
Marek Szyprowski
Samsung Poland R&D Center

Re: [PATCH 2/4] mm: cma: Contiguous Memory Allocator added

[Michał Nazarewicz]

On Wed, 21 Jul 2010 23:01:42 +0200, Daniel Walker <dwalker@codeaurora.org> wrote:

> On Wed, 2010-07-21 at 22:56 +0200, Michał Nazarewicz wrote:
>> On Wed, 21 Jul 2010 22:45:43 +0200, Daniel Walker <dwalker@codeaurora.org> wrote:
>> > Your not hearing the issues.. IT'S TOO COMPLEX! Please remove it.
>>
>> Remove what exactly?
>
> Remove the command line option and all related code, or make it all a
> debug option.

How convenient... you have stripped the part of my mail where I described
why this is request have no sense.  I'll quote myself then:

>> The command line parameter? It's like 50 lines of code, so I don't
>> see any benefits.

As such, I'm not going to add bunch of #ifdefs just to remove 50 lines
of code.

>> The possibility to specify the configuration? It would defy the whole
>> purpose of CMA, so I won't do that.

Simply as that.  We work with a platform where whole of the functionality
provided by CMA is required (many regions, region start address, region
alignment, device->region mapping).

This means, what I keep repeating and you keep ignoring, that the complexity
will be there if not as a parsing code then moved to the platform
initialisation code and drivers code.

One of the purposes of CMA is to hide the complexity inside CMA framework so
device driver authors and platform maintainers can use a simpler interface.


Some time age (like year or two) I've posted some other solution to the
problem which served our purpose just well and had very little complexity
in it.  Unfortunately, customising that solution was quite hard (required
changes to a header file and adding modifying code for reserving space).

Also, in this old solution, adding or removing regions required device
drivers to be modified.

This was not nice, not nice at all.  True, however, the core wasn't complex.


So when you say remove the complicity I say: I have been there, it's ugly.


> Arguing with me isn't going to help your cause.

It's you who keep repeating “remove it, it's to complex” without
hearing my arguments.  I keep trying to show that all of the
functionality is required and is being used on our development
platform.

If your hardware does not require that complexity... well, you're one
lucky man.  Unfortunately, we are not, and we need a complex solution
to work with complex hardware.

-- 
Best regards,                                        _     _
| Humble Liege of Serenely Enlightened Majesty of  o' \,=./ `o
| Computer Science,  Michał "mina86" Nazarewicz       (o o)
+----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo--

Re: [PATCH 2/4] mm: cma: Contiguous Memory Allocator added

[Mark Brown]

On Thu, Jul 22, 2010 at 11:25:48AM +0200, Marek Szyprowski wrote:

> The driver may specify memory requirements (like memory address range or
> alignment), but it cannot provide enough information to avoid or reduce
> memory fragmentation. More than one memory region can be perfectly used
> to reduce memory fragmentation IF common usage patterns are known. In
> embedded world usually not all integrated device are being used at the
> same time. This way some memory regions can be shared by 2 or more devices. 

I do have some passing familiarity with the area, typically a lot of the
features of a SoC won't get used at all at runtime on any given system.

> Just assume that gfx accelerator allocates memory is rather small chunks,
> but keeps it while relevant surface is being displayed or processed by
> application. It is not surprising that GUI (accelerated by the hardware
> engine) is used almost all the time on a mobile device. This usage pattern
> would produce a lot of fragmentation in the memory pool that is used by gfx
> accelerator. Then we want to run a camera capture device to take a 8Mpix

I'd expect that the devices would be able to reserve blocks of memory to
play with separately to the actual allocations (ie, allocate regions
like those on the command line) and things like the GPU would make use
of that.  I think you're already doing part of this?

> photo. This require a large contiguous buffer. If we try to allocate it from
> common pool it might happen that it is not possible (because of the
> fragmentation).

Sure, but none of this is saying to me that it's specifically important
to supply a static configuration via this textual configuration language
on the command line - half the problem is that you're trying to write
the configuration down in a format which is fairly tightly constrained
by needing to be there.  If the configuration is more dynamic there's a
lot more flexibility to either allow the system to figure things out
dynamically (which will hopefully work a lot of the time, for example in
your use case only the GPU really needs memory reserving).

Remember also that if you can configure this at runtime (as you say
you're working towards) then even if you have a fairly static
configuration you can inject it into the kernel from the application
layer rather than having to either hard code it in the image or bodge it
in via the command line.  This keeps the resource allocation joined up
with the application layer (which is after all what determines the
resource usage).

Re: [PATCH 2/4] mm: cma: Contiguous Memory Allocator added

[Michał Nazarewicz]

On Thu, 22 Jul 2010 12:52:03 +0200, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:
> I'd expect that the devices would be able to reserve blocks of memory to
> play with separately to the actual allocations (ie, allocate regions
> like those on the command line) and things like the GPU would make use
> of that.  I think you're already doing part of this?

In the patchset I've sent it is not possible but I already have a version that
supports this.  Regions can be registered at any time.  What's more, such
regions can be completely private to drivers that register them.

> Sure, but none of this is saying to me that it's specifically important
> to supply a static configuration via this textual configuration language
> on the command line - half the problem is that you're trying to write
> the configuration down in a format which is fairly tightly constrained
> by needing to be there.  If the configuration is more dynamic there's a
> lot more flexibility to either allow the system to figure things out
> dynamically (which will hopefully work a lot of the time, for example in
> your use case only the GPU really needs memory reserving).
>
> Remember also that if you can configure this at runtime (as you say
> you're working towards) then even if you have a fairly static
> configuration you can inject it into the kernel from the application
> layer rather than having to either hard code it in the image or bodge it
> in via the command line.  This keeps the resource allocation joined up
> with the application layer (which is after all what determines the
> resource usage).

There are two command line arguments to consider: cma and cma_map.


The first one, I believe, should be there as to specify the regions
that are to be reserved.  Drivers and platform will still be able to
add their own regions but I believe that in vest majority of cases,
it will be enough to just pass the list of region on a command line.

Alternatively, instead of the textual description of platform could
provide an array of regions it want reserved.  It would remove like
50 lines of code from CMA core (in the version I have on my drive at
least, where part of the syntax was simplified) however it would
remove the possibility to easily change the configuration from
command line (ie. no need to recompile which is handy when you need
to optimise this and test various configurations) and would add more
code to the platform initialisation code, ie: instead of:

	cma_defaults("reg1=20M;reg2=20M", NULL);

one would have to define an array with the regions descriptors.
Personally, I don't see much benefits from this.


As of the second parameter, "cma_map", which validating and parsing
is like 150 lines of code, I consider it handy because you can manage
all the memory regions in one place and it moves some of the complexity
 from device drivers to CMA.  I'm also working on providing a sysfs
entry so that the it would be possible to change the mapping at runtime.

For example, consider a driver I have mentioned before: video decoder
that needs to allocate memory from 3 different regions (for firmware,
the first bank of memory and the second bank of memory).  With CMA you
define the regions:

	cma=vf=1M/128K;a=20M;b=20M@512M;

and then map video driver to them like so:

	cma_map=video/a=a;video/b=b;video/f=vf

I agree that parsing it is not nice but thanks to it, all you need to
do in the driver is:

	cma_alloc(dev, "a", ...)
	cma_alloc(dev, "b", ...)
	cma_alloc(dev, "f", ...)

Without cma_map you'd have to pass names of the region to the driver
and make the driver use those.

It would also make it impossible or hard to change the mapping once
the driver is loaded.


What I'm trying to say is that I'm trying to move complexity out of
the drivers into the framework (as I believe that's what frameworks
are for).


As of dynamic, runtime, automatic configuration, I don't really see
that.  I'm still wondering how to make as little configuration
necessary as possible but I don't think everything can be done
in such a way.

-- 
Best regards,                                        _     _
| Humble Liege of Serenely Enlightened Majesty of  o' \,=./ `o
| Computer Science,  Michał "mina86" Nazarewicz       (o o)
+----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo--

Re: [PATCH 2/4] mm: cma: Contiguous Memory Allocator added

[Mark Brown]

On Thu, Jul 22, 2010 at 01:30:52PM +0200, Michał Nazarewicz wrote:

> The first one, I believe, should be there as to specify the regions
> that are to be reserved.  Drivers and platform will still be able to
> add their own regions but I believe that in vest majority of cases,
> it will be enough to just pass the list of region on a command line.

The command line is a real pain for stuff like this since it's not
usually committed to revision control so robustly and it's normally more
painful to change the bootloader to pass the desired command line in
than it is to change either the kernel or userspace (some bootloaders
are just completely unconfigurable without reflashing, and if your only
recovery mechanism is JTAG that can be a bit of a concern).

> Alternatively, instead of the textual description of platform could
> provide an array of regions it want reserved.  It would remove like
> 50 lines of code from CMA core (in the version I have on my drive at
> least, where part of the syntax was simplified) however it would
> remove the possibility to easily change the configuration from
> command line (ie. no need to recompile which is handy when you need
> to optimise this and test various configurations) and would add more
> code to the platform initialisation code, ie: instead of:

> 	cma_defaults("reg1=20M;reg2=20M", NULL);

> one would have to define an array with the regions descriptors.
> Personally, I don't see much benefits from this.

I think it'd be vastly more legible, especially if the list of regions
gets large.  I had thought the only reason for the text format was to
put it onto the command line.

> I agree that parsing it is not nice but thanks to it, all you need to
> do in the driver is:

> 	cma_alloc(dev, "a", ...)
> 	cma_alloc(dev, "b", ...)
> 	cma_alloc(dev, "f", ...)

> Without cma_map you'd have to pass names of the region to the driver
> and make the driver use those.

I agree that a mapping facility for the names is essential, especially
if drivers need to share regions.

> What I'm trying to say is that I'm trying to move complexity out of
> the drivers into the framework (as I believe that's what frameworks
> are for).

It sounds like apart from the way you're passing the configuration in
you're doing roughly what I'd suggest.  I'd expect that in a lot of
cases the map could be satisfied from the default region so there'd be
no need to explicitly set one up.

Re: [PATCH 2/4] mm: cma: Contiguous Memory Allocator added

[Michał Nazarewicz]

> On Thu, Jul 22, 2010 at 01:30:52PM +0200, Michał Nazarewicz wrote:
>> The first one, I believe, should be there as to specify the regions
>> that are to be reserved.  Drivers and platform will still be able to
>> add their own regions but I believe that in vest majority of cases,
>> it will be enough to just pass the list of region on a command line.

On Thu, 22 Jul 2010 14:46:00 +0200, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:
> The command line is a real pain for stuff like this since it's not
> usually committed to revision control so robustly and it's normally more
> painful to change the bootloader to pass the desired command line in
> than it is to change either the kernel or userspace (some bootloaders
> are just completely unconfigurable without reflashing, and if your only
> recovery mechanism is JTAG that can be a bit of a concern).

That's why command line is only intended as a way to overwrite the
defaults which are provided by the platform.  In a final product,
configuration should be specified in platform code and not on
command line.

>> Alternatively, instead of the textual description of platform could
>> provide an array of regions it want reserved.  It would remove like
>> 50 lines of code from CMA core (in the version I have on my drive at
>> least, where part of the syntax was simplified) however it would
>> remove the possibility to easily change the configuration from
>> command line (ie. no need to recompile which is handy when you need
>> to optimise this and test various configurations) and would add more
>> code to the platform initialisation code, ie: instead of:
>>
>> 	cma_defaults("reg1=20M;reg2=20M", NULL);
>
>> one would have to define an array with the regions descriptors.
>> Personally, I don't see much benefits from this.
>
> I think it'd be vastly more legible, especially if the list of regions
> gets large.  I had thought the only reason for the text format was to
> put it onto the command line.

Command line was one of the reasons for using textual interface.  I surely
wouldn't go with parsing the strings if I could manage without it allowing
easy platform-level configuration at the same time.

>> I agree that parsing it is not nice but thanks to it, all you need to
>> do in the driver is:
>>
>> 	cma_alloc(dev, "a", ...)
>> 	cma_alloc(dev, "b", ...)
>> 	cma_alloc(dev, "f", ...)
>>
>> Without cma_map you'd have to pass names of the region to the driver
>> and make the driver use those.
>
> I agree that a mapping facility for the names is essential, especially
> if drivers need to share regions.
>
>> What I'm trying to say is that I'm trying to move complexity out of
>> the drivers into the framework (as I believe that's what frameworks
>> are for).
>
> It sounds like apart from the way you're passing the configuration in
> you're doing roughly what I'd suggest.  I'd expect that in a lot of
> cases the map could be satisfied from the default region so there'd be
> no need to explicitly set one up.

Platform can specify something like:

	cma_defaults("reg=20M", "*/*=reg");

which would make all the drivers share 20 MiB region by default.  I'm also
thinking if something like:

	cma_defaults("reg=20M", "*/*=*");

(ie. asterisk instead of list of regions) should be allowed.  It would make
the default to be that all allocations are performed from all named regions.
I'll see how much coding is that and maybe add it.

-- 
Best regards,                                        _     _
| Humble Liege of Serenely Enlightened Majesty of  o' \,=./ `o
| Computer Science,  Michał "mina86" Nazarewicz       (o o)
+----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo--

Re: [PATCH 2/4] mm: cma: Contiguous Memory Allocator added

[Mark Brown]

On Thu, Jul 22, 2010 at 03:24:26PM +0200, Michał Nazarewicz wrote:

> That's why command line is only intended as a way to overwrite the
> defaults which are provided by the platform.  In a final product,
> configuration should be specified in platform code and not on
> command line.

Yeah, agreed though I'm not convinced we can't do it via userspace
(initrd would give us a chance to do stuff early) or just kernel
rebuilds.

> >It sounds like apart from the way you're passing the configuration in
> >you're doing roughly what I'd suggest.  I'd expect that in a lot of
> >cases the map could be satisfied from the default region so there'd be
> >no need to explicitly set one up.

> Platform can specify something like:

> 	cma_defaults("reg=20M", "*/*=reg");

> which would make all the drivers share 20 MiB region by default.  I'm also
> thinking if something like:

Yes, exactly - probably you can even have a default region backed by
normal vmalloc() RAM which would at least be able to take a stab at
working by default.

Re: [PATCH 2/4] mm: cma: Contiguous Memory Allocator added

[Michał Nazarewicz]

On Thu, 22 Jul 2010 15:40:56 +0200, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:

> On Thu, Jul 22, 2010 at 03:24:26PM +0200, Michał Nazarewicz wrote:
>
>> That's why command line is only intended as a way to overwrite the
>> defaults which are provided by the platform.  In a final product,
>> configuration should be specified in platform code and not on
>> command line.
>
> Yeah, agreed though I'm not convinced we can't do it via userspace
> (initrd would give us a chance to do stuff early) or just kernel
> rebuilds.

If there's any other easy way of overwriting platform's default I'm happy
to listen. :)

>> >It sounds like apart from the way you're passing the configuration in
>> >you're doing roughly what I'd suggest.  I'd expect that in a lot of
>> >cases the map could be satisfied from the default region so there'd be
>> >no need to explicitly set one up.
>
>> Platform can specify something like:
>
>> 	cma_defaults("reg=20M", "*/*=reg");
>
>> which would make all the drivers share 20 MiB region by default.
>
> Yes, exactly - probably you can even have a default region backed by
> normal vmalloc() RAM which would at least be able to take a stab at
> working by default.

Not sure what you mean here.  vmalloc() allocated buffers cannot be used
with CMA since they are not contiguous in memory.

-- 
Best regards,                                        _     _
| Humble Liege of Serenely Enlightened Majesty of  o' \,=./ `o
| Computer Science,  Michał "mina86" Nazarewicz       (o o)
+----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo--

Re: [PATCH 2/4] mm: cma: Contiguous Memory Allocator added

[Mark Brown]

On Thu, Jul 22, 2010 at 04:58:43PM +0200, Michał Nazarewicz wrote:
> On Thu, 22 Jul 2010 15:40:56 +0200, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:

> >Yeah, agreed though I'm not convinced we can't do it via userspace
> >(initrd would give us a chance to do stuff early) or just kernel
> >rebuilds.

> If there's any other easy way of overwriting platform's default I'm happy
> to listen. :)

Netlink or similar, for example?

> >Yes, exactly - probably you can even have a default region backed by
> >normal vmalloc() RAM which would at least be able to take a stab at
> >working by default.

> Not sure what you mean here.  vmalloc() allocated buffers cannot be used
> with CMA since they are not contiguous in memory.

Sorry, thinko - I just meant allocated at runtime.  It'd fail a a lot of
the time so might not be worth bothering.