| Message ID | 1355215912-28715-1-git-send-email-johan.mossberg@stericsson.com |
|---|---|
| State | New |
| Headers | show |
Johan, It should be heap->id only. You can always have two heaps with different ids, but of the same type. For eg: two CMA heaps or two carveout heaps. User may control which heap should be used for buffer allocation by setting appropriate bits in the heap_mask. - Nishanth Peethambaran +91-9448074166 On Tue, Dec 11, 2012 at 2:21 PM, Johan Mossberg <johan.mossberg@stericsson.com> wrote: > The heap id is compared against the heap mask in ion_alloc which is > incorrect, the heap type should be used. > > Signed-off-by: Johan Mossberg <johan.mossberg@stericsson.com> > --- > drivers/gpu/ion/ion.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/ion/ion.c b/drivers/gpu/ion/ion.c > index fc152b9..c811380f 100644 > --- a/drivers/gpu/ion/ion.c > +++ b/drivers/gpu/ion/ion.c > @@ -414,7 +414,7 @@ struct ion_handle *ion_alloc(struct ion_client *client, size_t len, > if (!((1 << heap->type) & client->heap_mask)) > continue; > /* if the caller didn't specify this heap type */ > - if (!((1 << heap->id) & heap_mask)) > + if (!((1 << heap->type) & heap_mask)) > continue; > buffer = ion_buffer_create(heap, dev, len, align, flags); > if (!IS_ERR_OR_NULL(buffer)) > -- > 1.8.0 > > > _______________________________________________ > Linaro-mm-sig mailing list > Linaro-mm-sig@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/linaro-mm-sig
On 12/11/2012 10:55 AM, Nishanth Peethambaran wrote: > Johan, > > It should be heap->id only. You can always have two heaps with > different ids, but of the same type. For eg: two CMA heaps or two > carveout heaps. > User may control which heap should be used for buffer allocation by > setting appropriate bits in the heap_mask. I don't understand, please elaborate. Using the heap id here makes no sense to me. We are trying to check if the heap matches the supplied heap type mask, just like we checked if the heap matched the client's heap type mask in the line before. > > - Nishanth Peethambaran > +91-9448074166 > > > > On Tue, Dec 11, 2012 at 2:21 PM, Johan Mossberg > <johan.mossberg@stericsson.com> wrote: >> The heap id is compared against the heap mask in ion_alloc which is >> incorrect, the heap type should be used. >> >> Signed-off-by: Johan Mossberg <johan.mossberg@stericsson.com> >> --- >> drivers/gpu/ion/ion.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/ion/ion.c b/drivers/gpu/ion/ion.c >> index fc152b9..c811380f 100644 >> --- a/drivers/gpu/ion/ion.c >> +++ b/drivers/gpu/ion/ion.c >> @@ -414,7 +414,7 @@ struct ion_handle *ion_alloc(struct ion_client *client, size_t len, >> if (!((1 << heap->type) & client->heap_mask)) >> continue; >> /* if the caller didn't specify this heap type */ >> - if (!((1 << heap->id) & heap_mask)) >> + if (!((1 << heap->type) & heap_mask)) >> continue; >> buffer = ion_buffer_create(heap, dev, len, align, flags); >> if (!IS_ERR_OR_NULL(buffer)) >> -- >> 1.8.0 >> >> >> _______________________________________________ >> Linaro-mm-sig mailing list >> Linaro-mm-sig@lists.linaro.org >> http://lists.linaro.org/mailman/listinfo/linaro-mm-sig
Consider having two carveout/cma heaps partitioned into different banks of SDRAM. Say, the heap->id iof carveout0 is 0 and heap->id of carveout1 is 1. But both would have the heap->type as ION_HEAP_TYPE_CARVEOUT (2) Userspace ion client would be created with a heap_mask of -1 (any heap type is ok). At allocation time, user process can pass heap_mask as 1 to allocate from carveout0; heap_mask as 2 to allocate from carveout1; heap_mask as 3 to try allocating from carveout0 first and then carveout1 next;. If the check is based on heap->type, you would miss that flexibiility. On Tue, Dec 11, 2012 at 4:34 PM, Johan Mossberg <johan.mossberg@stericsson.com> wrote: > On 12/11/2012 10:55 AM, Nishanth Peethambaran wrote: >> Johan, >> >> It should be heap->id only. You can always have two heaps with >> different ids, but of the same type. For eg: two CMA heaps or two >> carveout heaps. >> User may control which heap should be used for buffer allocation by >> setting appropriate bits in the heap_mask. > > I don't understand, please elaborate. Using the heap id here makes no > sense to me. We are trying to check if the heap matches the supplied > heap type mask, just like we checked if the heap matched the client's > heap type mask in the line before. > >> >> - Nishanth Peethambaran >> +91-9448074166 >> >> >> >> On Tue, Dec 11, 2012 at 2:21 PM, Johan Mossberg >> <johan.mossberg@stericsson.com> wrote: >>> The heap id is compared against the heap mask in ion_alloc which is >>> incorrect, the heap type should be used. >>> >>> Signed-off-by: Johan Mossberg <johan.mossberg@stericsson.com> >>> --- >>> drivers/gpu/ion/ion.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/ion/ion.c b/drivers/gpu/ion/ion.c >>> index fc152b9..c811380f 100644 >>> --- a/drivers/gpu/ion/ion.c >>> +++ b/drivers/gpu/ion/ion.c >>> @@ -414,7 +414,7 @@ struct ion_handle *ion_alloc(struct ion_client *client, size_t len, >>> if (!((1 << heap->type) & client->heap_mask)) >>> continue; >>> /* if the caller didn't specify this heap type */ >>> - if (!((1 << heap->id) & heap_mask)) >>> + if (!((1 << heap->type) & heap_mask)) >>> continue; >>> buffer = ion_buffer_create(heap, dev, len, align, flags); >>> if (!IS_ERR_OR_NULL(buffer)) >>> -- >>> 1.8.0 >>> >>> >>> _______________________________________________ >>> Linaro-mm-sig mailing list >>> Linaro-mm-sig@lists.linaro.org >>> http://lists.linaro.org/mailman/listinfo/linaro-mm-sig > - Nishanth Peethambaran
I see what you mean. In my opinion it is very hard to understand that it works this way. heap_mask means one thing in ion_client_create and another thing in ion_alloc. I can't find any information about heap ids being a power of two either. We should probably consider updating the documentation and variable names to make it more clear how this works. On 12/11/2012 12:50 PM, Nishanth Peethambaran wrote: > Consider having two carveout/cma heaps partitioned into different > banks of SDRAM. Say, the heap->id iof carveout0 is 0 and heap->id of > carveout1 is 1. But both would have the heap->type as > ION_HEAP_TYPE_CARVEOUT (2) > > Userspace ion client would be created with a heap_mask of -1 (any heap > type is ok). At allocation time, user process can pass heap_mask as 1 > to allocate from carveout0; heap_mask as 2 to allocate from carveout1; > heap_mask as 3 to try allocating from carveout0 first and then > carveout1 next;. If the check is based on heap->type, you would miss > that flexibiility. > > > On Tue, Dec 11, 2012 at 4:34 PM, Johan Mossberg > <johan.mossberg@stericsson.com> wrote: >> On 12/11/2012 10:55 AM, Nishanth Peethambaran wrote: >>> Johan, >>> >>> It should be heap->id only. You can always have two heaps with >>> different ids, but of the same type. For eg: two CMA heaps or two >>> carveout heaps. >>> User may control which heap should be used for buffer allocation by >>> setting appropriate bits in the heap_mask. >> >> I don't understand, please elaborate. Using the heap id here makes no >> sense to me. We are trying to check if the heap matches the supplied >> heap type mask, just like we checked if the heap matched the client's >> heap type mask in the line before. >> >>> >>> - Nishanth Peethambaran >>> +91-9448074166 >>> >>> >>> >>> On Tue, Dec 11, 2012 at 2:21 PM, Johan Mossberg >>> <johan.mossberg@stericsson.com> wrote: >>>> The heap id is compared against the heap mask in ion_alloc which is >>>> incorrect, the heap type should be used. >>>> >>>> Signed-off-by: Johan Mossberg <johan.mossberg@stericsson.com> >>>> --- >>>> drivers/gpu/ion/ion.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/gpu/ion/ion.c b/drivers/gpu/ion/ion.c >>>> index fc152b9..c811380f 100644 >>>> --- a/drivers/gpu/ion/ion.c >>>> +++ b/drivers/gpu/ion/ion.c >>>> @@ -414,7 +414,7 @@ struct ion_handle *ion_alloc(struct ion_client *client, size_t len, >>>> if (!((1 << heap->type) & client->heap_mask)) >>>> continue; >>>> /* if the caller didn't specify this heap type */ >>>> - if (!((1 << heap->id) & heap_mask)) >>>> + if (!((1 << heap->type) & heap_mask)) >>>> continue; >>>> buffer = ion_buffer_create(heap, dev, len, align, flags); >>>> if (!IS_ERR_OR_NULL(buffer)) >>>> -- >>>> 1.8.0 >>>> >>>> >>>> _______________________________________________ >>>> Linaro-mm-sig mailing list >>>> Linaro-mm-sig@lists.linaro.org >>>> http://lists.linaro.org/mailman/listinfo/linaro-mm-sig >> > > - Nishanth Peethambaran
Heap ids need not be a power of 2. The ids need to be unique and maximum 16 heaps can co-exist. From the source code, my inference is that heap_mask passed at allocation time is a bitmask of heap ids. You are right that the heap id and heap type gets mixed up. The ion_debug_heap_total, client create heap_mask relies on heap type. The ion_debug_client_show and allocation heap_mask relies on heap id. I would prefer all of them using heap id instead of heap type. I had earlier submitted a patch to update ion_debug_heap_total() to use heap id instead of heap type. But, did not see any response. :) On Tue, Dec 11, 2012 at 6:20 PM, Johan Mossberg <johan.mossberg@stericsson.com> wrote: > I see what you mean. In my opinion it is very hard to understand that it > works this way. heap_mask means one thing in ion_client_create and > another thing in ion_alloc. I can't find any information about heap ids > being a power of two either. We should probably consider updating the > documentation and variable names to make it more clear how this works. > > On 12/11/2012 12:50 PM, Nishanth Peethambaran wrote: >> Consider having two carveout/cma heaps partitioned into different >> banks of SDRAM. Say, the heap->id iof carveout0 is 0 and heap->id of >> carveout1 is 1. But both would have the heap->type as >> ION_HEAP_TYPE_CARVEOUT (2) >> >> Userspace ion client would be created with a heap_mask of -1 (any heap >> type is ok). At allocation time, user process can pass heap_mask as 1 >> to allocate from carveout0; heap_mask as 2 to allocate from carveout1; >> heap_mask as 3 to try allocating from carveout0 first and then >> carveout1 next;. If the check is based on heap->type, you would miss >> that flexibiility. >> >> >> On Tue, Dec 11, 2012 at 4:34 PM, Johan Mossberg >> <johan.mossberg@stericsson.com> wrote: >>> On 12/11/2012 10:55 AM, Nishanth Peethambaran wrote: >>>> Johan, >>>> >>>> It should be heap->id only. You can always have two heaps with >>>> different ids, but of the same type. For eg: two CMA heaps or two >>>> carveout heaps. >>>> User may control which heap should be used for buffer allocation by >>>> setting appropriate bits in the heap_mask. >>> >>> I don't understand, please elaborate. Using the heap id here makes no >>> sense to me. We are trying to check if the heap matches the supplied >>> heap type mask, just like we checked if the heap matched the client's >>> heap type mask in the line before. >>> >>>> >>>> - Nishanth Peethambaran >>>> +91-9448074166 >>>> >>>> >>>> >>>> On Tue, Dec 11, 2012 at 2:21 PM, Johan Mossberg >>>> <johan.mossberg@stericsson.com> wrote: >>>>> The heap id is compared against the heap mask in ion_alloc which is >>>>> incorrect, the heap type should be used. >>>>> >>>>> Signed-off-by: Johan Mossberg <johan.mossberg@stericsson.com> >>>>> --- >>>>> drivers/gpu/ion/ion.c | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/gpu/ion/ion.c b/drivers/gpu/ion/ion.c >>>>> index fc152b9..c811380f 100644 >>>>> --- a/drivers/gpu/ion/ion.c >>>>> +++ b/drivers/gpu/ion/ion.c >>>>> @@ -414,7 +414,7 @@ struct ion_handle *ion_alloc(struct ion_client *client, size_t len, >>>>> if (!((1 << heap->type) & client->heap_mask)) >>>>> continue; >>>>> /* if the caller didn't specify this heap type */ >>>>> - if (!((1 << heap->id) & heap_mask)) >>>>> + if (!((1 << heap->type) & heap_mask)) >>>>> continue; >>>>> buffer = ion_buffer_create(heap, dev, len, align, flags); >>>>> if (!IS_ERR_OR_NULL(buffer)) >>>>> -- >>>>> 1.8.0 >>>>> >>>>> >>>>> _______________________________________________ >>>>> Linaro-mm-sig mailing list >>>>> Linaro-mm-sig@lists.linaro.org >>>>> http://lists.linaro.org/mailman/listinfo/linaro-mm-sig >>> >> >> - Nishanth Peethambaran > - Nishanth Peethambaran
Nishanth is correct. The intention was to use the heap_id's so you could have several heaps of the same type -- this is a fairly common configuration when creating carveouts for secure playback. I agree the variable naming is pretty confusing -- I would be happy to take a patch that clarifies it a bit, otherwise I'll take a look at doing that myself. Apologies on not getting feedback to your earlier patches Nishanth. If you want to repost them against the latest android-3.4 I'll take a look. Rebecca On Tue, Dec 11, 2012 at 5:58 AM, Nishanth Peethambaran <nishanth.p@gmail.com> wrote: > Heap ids need not be a power of 2. The ids need to be unique and > maximum 16 heaps can co-exist. From the source code, my inference is > that heap_mask passed at allocation time is a bitmask of heap ids. > > You are right that the heap id and heap type gets mixed up. The > ion_debug_heap_total, client create heap_mask relies on heap type. The > ion_debug_client_show and allocation heap_mask relies on heap id. I > would prefer all of them using heap id instead of heap type. I had > earlier submitted a patch to update ion_debug_heap_total() to use heap > id instead of heap type. But, did not see any response. :) > > > On Tue, Dec 11, 2012 at 6:20 PM, Johan Mossberg > <johan.mossberg@stericsson.com> wrote: >> I see what you mean. In my opinion it is very hard to understand that it >> works this way. heap_mask means one thing in ion_client_create and >> another thing in ion_alloc. I can't find any information about heap ids >> being a power of two either. We should probably consider updating the >> documentation and variable names to make it more clear how this works. >> >> On 12/11/2012 12:50 PM, Nishanth Peethambaran wrote: >>> Consider having two carveout/cma heaps partitioned into different >>> banks of SDRAM. Say, the heap->id iof carveout0 is 0 and heap->id of >>> carveout1 is 1. But both would have the heap->type as >>> ION_HEAP_TYPE_CARVEOUT (2) >>> >>> Userspace ion client would be created with a heap_mask of -1 (any heap >>> type is ok). At allocation time, user process can pass heap_mask as 1 >>> to allocate from carveout0; heap_mask as 2 to allocate from carveout1; >>> heap_mask as 3 to try allocating from carveout0 first and then >>> carveout1 next;. If the check is based on heap->type, you would miss >>> that flexibiility. >>> >>> >>> On Tue, Dec 11, 2012 at 4:34 PM, Johan Mossberg >>> <johan.mossberg@stericsson.com> wrote: >>>> On 12/11/2012 10:55 AM, Nishanth Peethambaran wrote: >>>>> Johan, >>>>> >>>>> It should be heap->id only. You can always have two heaps with >>>>> different ids, but of the same type. For eg: two CMA heaps or two >>>>> carveout heaps. >>>>> User may control which heap should be used for buffer allocation by >>>>> setting appropriate bits in the heap_mask. >>>> >>>> I don't understand, please elaborate. Using the heap id here makes no >>>> sense to me. We are trying to check if the heap matches the supplied >>>> heap type mask, just like we checked if the heap matched the client's >>>> heap type mask in the line before. >>>> >>>>> >>>>> - Nishanth Peethambaran >>>>> +91-9448074166 >>>>> >>>>> >>>>> >>>>> On Tue, Dec 11, 2012 at 2:21 PM, Johan Mossberg >>>>> <johan.mossberg@stericsson.com> wrote: >>>>>> The heap id is compared against the heap mask in ion_alloc which is >>>>>> incorrect, the heap type should be used. >>>>>> >>>>>> Signed-off-by: Johan Mossberg <johan.mossberg@stericsson.com> >>>>>> --- >>>>>> drivers/gpu/ion/ion.c | 2 +- >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/drivers/gpu/ion/ion.c b/drivers/gpu/ion/ion.c >>>>>> index fc152b9..c811380f 100644 >>>>>> --- a/drivers/gpu/ion/ion.c >>>>>> +++ b/drivers/gpu/ion/ion.c >>>>>> @@ -414,7 +414,7 @@ struct ion_handle *ion_alloc(struct ion_client *client, size_t len, >>>>>> if (!((1 << heap->type) & client->heap_mask)) >>>>>> continue; >>>>>> /* if the caller didn't specify this heap type */ >>>>>> - if (!((1 << heap->id) & heap_mask)) >>>>>> + if (!((1 << heap->type) & heap_mask)) >>>>>> continue; >>>>>> buffer = ion_buffer_create(heap, dev, len, align, flags); >>>>>> if (!IS_ERR_OR_NULL(buffer)) >>>>>> -- >>>>>> 1.8.0 >>>>>> >>>>>> >>>>>> _______________________________________________ >>>>>> Linaro-mm-sig mailing list >>>>>> Linaro-mm-sig@lists.linaro.org >>>>>> http://lists.linaro.org/mailman/listinfo/linaro-mm-sig >>>> >>> >>> - Nishanth Peethambaran >> > > - Nishanth Peethambaran > > _______________________________________________ > Linaro-mm-sig mailing list > Linaro-mm-sig@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/linaro-mm-sig
I have posted a new patch which unifies the meaning of heap_mask and fixes the debugfs show functions. I have assumed that the heap_mask passed during client creation is also intended to be a bitmask of heap ids. If the intention is to pass bitmask of heap->types during client creation and a bitmask of heap->ids during buffer creation, we can rename the client specific parameter, say heap_type_mask. - Nishanth Peethambaran On Wed, Dec 12, 2012 at 1:10 AM, Rebecca Schultz Zavin <rebecca@android.com> wrote: > Nishanth is correct. The intention was to use the heap_id's so you > could have several heaps of the same type -- this is a fairly common > configuration when creating carveouts for secure playback. I agree > the variable naming is pretty confusing -- I would be happy to take a > patch that clarifies it a bit, otherwise I'll take a look at doing > that myself. > > Apologies on not getting feedback to your earlier patches Nishanth. > If you want to repost them against the latest android-3.4 I'll take a > look. > > Rebecca > > On Tue, Dec 11, 2012 at 5:58 AM, Nishanth Peethambaran > <nishanth.p@gmail.com> wrote: >> Heap ids need not be a power of 2. The ids need to be unique and >> maximum 16 heaps can co-exist. From the source code, my inference is >> that heap_mask passed at allocation time is a bitmask of heap ids. >> >> You are right that the heap id and heap type gets mixed up. The >> ion_debug_heap_total, client create heap_mask relies on heap type. The >> ion_debug_client_show and allocation heap_mask relies on heap id. I >> would prefer all of them using heap id instead of heap type. I had >> earlier submitted a patch to update ion_debug_heap_total() to use heap >> id instead of heap type. But, did not see any response. :) >> >> >> On Tue, Dec 11, 2012 at 6:20 PM, Johan Mossberg >> <johan.mossberg@stericsson.com> wrote: >>> I see what you mean. In my opinion it is very hard to understand that it >>> works this way. heap_mask means one thing in ion_client_create and >>> another thing in ion_alloc. I can't find any information about heap ids >>> being a power of two either. We should probably consider updating the >>> documentation and variable names to make it more clear how this works. >>> >>> On 12/11/2012 12:50 PM, Nishanth Peethambaran wrote: >>>> Consider having two carveout/cma heaps partitioned into different >>>> banks of SDRAM. Say, the heap->id iof carveout0 is 0 and heap->id of >>>> carveout1 is 1. But both would have the heap->type as >>>> ION_HEAP_TYPE_CARVEOUT (2) >>>> >>>> Userspace ion client would be created with a heap_mask of -1 (any heap >>>> type is ok). At allocation time, user process can pass heap_mask as 1 >>>> to allocate from carveout0; heap_mask as 2 to allocate from carveout1; >>>> heap_mask as 3 to try allocating from carveout0 first and then >>>> carveout1 next;. If the check is based on heap->type, you would miss >>>> that flexibiility. >>>> >>>> >>>> On Tue, Dec 11, 2012 at 4:34 PM, Johan Mossberg >>>> <johan.mossberg@stericsson.com> wrote: >>>>> On 12/11/2012 10:55 AM, Nishanth Peethambaran wrote: >>>>>> Johan, >>>>>> >>>>>> It should be heap->id only. You can always have two heaps with >>>>>> different ids, but of the same type. For eg: two CMA heaps or two >>>>>> carveout heaps. >>>>>> User may control which heap should be used for buffer allocation by >>>>>> setting appropriate bits in the heap_mask. >>>>> >>>>> I don't understand, please elaborate. Using the heap id here makes no >>>>> sense to me. We are trying to check if the heap matches the supplied >>>>> heap type mask, just like we checked if the heap matched the client's >>>>> heap type mask in the line before. >>>>> >>>>>> >>>>>> - Nishanth Peethambaran >>>>>> +91-9448074166 >>>>>> >>>>>> >>>>>> >>>>>> On Tue, Dec 11, 2012 at 2:21 PM, Johan Mossberg >>>>>> <johan.mossberg@stericsson.com> wrote: >>>>>>> The heap id is compared against the heap mask in ion_alloc which is >>>>>>> incorrect, the heap type should be used. >>>>>>> >>>>>>> Signed-off-by: Johan Mossberg <johan.mossberg@stericsson.com> >>>>>>> --- >>>>>>> drivers/gpu/ion/ion.c | 2 +- >>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>>> >>>>>>> diff --git a/drivers/gpu/ion/ion.c b/drivers/gpu/ion/ion.c >>>>>>> index fc152b9..c811380f 100644 >>>>>>> --- a/drivers/gpu/ion/ion.c >>>>>>> +++ b/drivers/gpu/ion/ion.c >>>>>>> @@ -414,7 +414,7 @@ struct ion_handle *ion_alloc(struct ion_client *client, size_t len, >>>>>>> if (!((1 << heap->type) & client->heap_mask)) >>>>>>> continue; >>>>>>> /* if the caller didn't specify this heap type */ >>>>>>> - if (!((1 << heap->id) & heap_mask)) >>>>>>> + if (!((1 << heap->type) & heap_mask)) >>>>>>> continue; >>>>>>> buffer = ion_buffer_create(heap, dev, len, align, flags); >>>>>>> if (!IS_ERR_OR_NULL(buffer)) >>>>>>> -- >>>>>>> 1.8.0 >>>>>>> >>>>>>> >>>>>>> _______________________________________________ >>>>>>> Linaro-mm-sig mailing list >>>>>>> Linaro-mm-sig@lists.linaro.org >>>>>>> http://lists.linaro.org/mailman/listinfo/linaro-mm-sig >>>>> >>>> >>>> - Nishanth Peethambaran >>> >> >> - Nishanth Peethambaran >> >> _______________________________________________ >> Linaro-mm-sig mailing list >> Linaro-mm-sig@lists.linaro.org >> http://lists.linaro.org/mailman/listinfo/linaro-mm-sig
I have a patch that removes the bitmask passed on client creation. The idea was you'd specify the heaps that were safe to use from this client, but in practice everyone seems to pass -1 and the drivers should probably know which heaps to use. Rebecca On Tue, Dec 11, 2012 at 5:01 PM, Nishanth Peethambaran <nishanth.p@gmail.com> wrote: > I have posted a new patch which unifies the meaning of heap_mask and > fixes the debugfs show functions. > I have assumed that the heap_mask passed during client creation is > also intended to be a bitmask of heap ids. If the intention is to pass > bitmask of heap->types during client creation and a bitmask of > heap->ids during buffer creation, we can rename the client specific > parameter, say heap_type_mask. > > - Nishanth Peethambaran > > > On Wed, Dec 12, 2012 at 1:10 AM, Rebecca Schultz Zavin > <rebecca@android.com> wrote: >> Nishanth is correct. The intention was to use the heap_id's so you >> could have several heaps of the same type -- this is a fairly common >> configuration when creating carveouts for secure playback. I agree >> the variable naming is pretty confusing -- I would be happy to take a >> patch that clarifies it a bit, otherwise I'll take a look at doing >> that myself. >> >> Apologies on not getting feedback to your earlier patches Nishanth. >> If you want to repost them against the latest android-3.4 I'll take a >> look. >> >> Rebecca >> >> On Tue, Dec 11, 2012 at 5:58 AM, Nishanth Peethambaran >> <nishanth.p@gmail.com> wrote: >>> Heap ids need not be a power of 2. The ids need to be unique and >>> maximum 16 heaps can co-exist. From the source code, my inference is >>> that heap_mask passed at allocation time is a bitmask of heap ids. >>> >>> You are right that the heap id and heap type gets mixed up. The >>> ion_debug_heap_total, client create heap_mask relies on heap type. The >>> ion_debug_client_show and allocation heap_mask relies on heap id. I >>> would prefer all of them using heap id instead of heap type. I had >>> earlier submitted a patch to update ion_debug_heap_total() to use heap >>> id instead of heap type. But, did not see any response. :) >>> >>> >>> On Tue, Dec 11, 2012 at 6:20 PM, Johan Mossberg >>> <johan.mossberg@stericsson.com> wrote: >>>> I see what you mean. In my opinion it is very hard to understand that it >>>> works this way. heap_mask means one thing in ion_client_create and >>>> another thing in ion_alloc. I can't find any information about heap ids >>>> being a power of two either. We should probably consider updating the >>>> documentation and variable names to make it more clear how this works. >>>> >>>> On 12/11/2012 12:50 PM, Nishanth Peethambaran wrote: >>>>> Consider having two carveout/cma heaps partitioned into different >>>>> banks of SDRAM. Say, the heap->id iof carveout0 is 0 and heap->id of >>>>> carveout1 is 1. But both would have the heap->type as >>>>> ION_HEAP_TYPE_CARVEOUT (2) >>>>> >>>>> Userspace ion client would be created with a heap_mask of -1 (any heap >>>>> type is ok). At allocation time, user process can pass heap_mask as 1 >>>>> to allocate from carveout0; heap_mask as 2 to allocate from carveout1; >>>>> heap_mask as 3 to try allocating from carveout0 first and then >>>>> carveout1 next;. If the check is based on heap->type, you would miss >>>>> that flexibiility. >>>>> >>>>> >>>>> On Tue, Dec 11, 2012 at 4:34 PM, Johan Mossberg >>>>> <johan.mossberg@stericsson.com> wrote: >>>>>> On 12/11/2012 10:55 AM, Nishanth Peethambaran wrote: >>>>>>> Johan, >>>>>>> >>>>>>> It should be heap->id only. You can always have two heaps with >>>>>>> different ids, but of the same type. For eg: two CMA heaps or two >>>>>>> carveout heaps. >>>>>>> User may control which heap should be used for buffer allocation by >>>>>>> setting appropriate bits in the heap_mask. >>>>>> >>>>>> I don't understand, please elaborate. Using the heap id here makes no >>>>>> sense to me. We are trying to check if the heap matches the supplied >>>>>> heap type mask, just like we checked if the heap matched the client's >>>>>> heap type mask in the line before. >>>>>> >>>>>>> >>>>>>> - Nishanth Peethambaran >>>>>>> +91-9448074166 >>>>>>> >>>>>>> >>>>>>> >>>>>>> On Tue, Dec 11, 2012 at 2:21 PM, Johan Mossberg >>>>>>> <johan.mossberg@stericsson.com> wrote: >>>>>>>> The heap id is compared against the heap mask in ion_alloc which is >>>>>>>> incorrect, the heap type should be used. >>>>>>>> >>>>>>>> Signed-off-by: Johan Mossberg <johan.mossberg@stericsson.com> >>>>>>>> --- >>>>>>>> drivers/gpu/ion/ion.c | 2 +- >>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>>>> >>>>>>>> diff --git a/drivers/gpu/ion/ion.c b/drivers/gpu/ion/ion.c >>>>>>>> index fc152b9..c811380f 100644 >>>>>>>> --- a/drivers/gpu/ion/ion.c >>>>>>>> +++ b/drivers/gpu/ion/ion.c >>>>>>>> @@ -414,7 +414,7 @@ struct ion_handle *ion_alloc(struct ion_client *client, size_t len, >>>>>>>> if (!((1 << heap->type) & client->heap_mask)) >>>>>>>> continue; >>>>>>>> /* if the caller didn't specify this heap type */ >>>>>>>> - if (!((1 << heap->id) & heap_mask)) >>>>>>>> + if (!((1 << heap->type) & heap_mask)) >>>>>>>> continue; >>>>>>>> buffer = ion_buffer_create(heap, dev, len, align, flags); >>>>>>>> if (!IS_ERR_OR_NULL(buffer)) >>>>>>>> -- >>>>>>>> 1.8.0 >>>>>>>> >>>>>>>> >>>>>>>> _______________________________________________ >>>>>>>> Linaro-mm-sig mailing list >>>>>>>> Linaro-mm-sig@lists.linaro.org >>>>>>>> http://lists.linaro.org/mailman/listinfo/linaro-mm-sig >>>>>> >>>>> >>>>> - Nishanth Peethambaran >>>> >>> >>> - Nishanth Peethambaran >>> >>> _______________________________________________ >>> Linaro-mm-sig mailing list >>> Linaro-mm-sig@lists.linaro.org >>> http://lists.linaro.org/mailman/listinfo/linaro-mm-sig
True. A superset heap_mask at client creation time was redundant and userspace does not have this provision at all. I shall resubmit a patch with updates only to debugfs show functions. The issue with the heap and client show functions is that it shows the sum of all heaps of the same type instead of showing individual heap information. - Nishanth Peethambaran On Wed, Dec 12, 2012 at 6:42 AM, Rebecca Schultz Zavin <rebecca@android.com> wrote: > I have a patch that removes the bitmask passed on client creation. > The idea was you'd specify the heaps that were safe to use from this > client, but in practice everyone seems to pass -1 and the drivers > should probably know which heaps to use. > > Rebecca > > On Tue, Dec 11, 2012 at 5:01 PM, Nishanth Peethambaran > <nishanth.p@gmail.com> wrote: >> I have posted a new patch which unifies the meaning of heap_mask and >> fixes the debugfs show functions. >> I have assumed that the heap_mask passed during client creation is >> also intended to be a bitmask of heap ids. If the intention is to pass >> bitmask of heap->types during client creation and a bitmask of >> heap->ids during buffer creation, we can rename the client specific >> parameter, say heap_type_mask. >> >> - Nishanth Peethambaran >> >> >> On Wed, Dec 12, 2012 at 1:10 AM, Rebecca Schultz Zavin >> <rebecca@android.com> wrote: >>> Nishanth is correct. The intention was to use the heap_id's so you >>> could have several heaps of the same type -- this is a fairly common >>> configuration when creating carveouts for secure playback. I agree >>> the variable naming is pretty confusing -- I would be happy to take a >>> patch that clarifies it a bit, otherwise I'll take a look at doing >>> that myself. >>> >>> Apologies on not getting feedback to your earlier patches Nishanth. >>> If you want to repost them against the latest android-3.4 I'll take a >>> look. >>> >>> Rebecca >>> >>> On Tue, Dec 11, 2012 at 5:58 AM, Nishanth Peethambaran >>> <nishanth.p@gmail.com> wrote: >>>> Heap ids need not be a power of 2. The ids need to be unique and >>>> maximum 16 heaps can co-exist. From the source code, my inference is >>>> that heap_mask passed at allocation time is a bitmask of heap ids. >>>> >>>> You are right that the heap id and heap type gets mixed up. The >>>> ion_debug_heap_total, client create heap_mask relies on heap type. The >>>> ion_debug_client_show and allocation heap_mask relies on heap id. I >>>> would prefer all of them using heap id instead of heap type. I had >>>> earlier submitted a patch to update ion_debug_heap_total() to use heap >>>> id instead of heap type. But, did not see any response. :) >>>> >>>> >>>> On Tue, Dec 11, 2012 at 6:20 PM, Johan Mossberg >>>> <johan.mossberg@stericsson.com> wrote: >>>>> I see what you mean. In my opinion it is very hard to understand that it >>>>> works this way. heap_mask means one thing in ion_client_create and >>>>> another thing in ion_alloc. I can't find any information about heap ids >>>>> being a power of two either. We should probably consider updating the >>>>> documentation and variable names to make it more clear how this works. >>>>> >>>>> On 12/11/2012 12:50 PM, Nishanth Peethambaran wrote: >>>>>> Consider having two carveout/cma heaps partitioned into different >>>>>> banks of SDRAM. Say, the heap->id iof carveout0 is 0 and heap->id of >>>>>> carveout1 is 1. But both would have the heap->type as >>>>>> ION_HEAP_TYPE_CARVEOUT (2) >>>>>> >>>>>> Userspace ion client would be created with a heap_mask of -1 (any heap >>>>>> type is ok). At allocation time, user process can pass heap_mask as 1 >>>>>> to allocate from carveout0; heap_mask as 2 to allocate from carveout1; >>>>>> heap_mask as 3 to try allocating from carveout0 first and then >>>>>> carveout1 next;. If the check is based on heap->type, you would miss >>>>>> that flexibiility. >>>>>> >>>>>> >>>>>> On Tue, Dec 11, 2012 at 4:34 PM, Johan Mossberg >>>>>> <johan.mossberg@stericsson.com> wrote: >>>>>>> On 12/11/2012 10:55 AM, Nishanth Peethambaran wrote: >>>>>>>> Johan, >>>>>>>> >>>>>>>> It should be heap->id only. You can always have two heaps with >>>>>>>> different ids, but of the same type. For eg: two CMA heaps or two >>>>>>>> carveout heaps. >>>>>>>> User may control which heap should be used for buffer allocation by >>>>>>>> setting appropriate bits in the heap_mask. >>>>>>> >>>>>>> I don't understand, please elaborate. Using the heap id here makes no >>>>>>> sense to me. We are trying to check if the heap matches the supplied >>>>>>> heap type mask, just like we checked if the heap matched the client's >>>>>>> heap type mask in the line before. >>>>>>> >>>>>>>> >>>>>>>> - Nishanth Peethambaran >>>>>>>> +91-9448074166 >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> On Tue, Dec 11, 2012 at 2:21 PM, Johan Mossberg >>>>>>>> <johan.mossberg@stericsson.com> wrote: >>>>>>>>> The heap id is compared against the heap mask in ion_alloc which is >>>>>>>>> incorrect, the heap type should be used. >>>>>>>>> >>>>>>>>> Signed-off-by: Johan Mossberg <johan.mossberg@stericsson.com> >>>>>>>>> --- >>>>>>>>> drivers/gpu/ion/ion.c | 2 +- >>>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>>>>> >>>>>>>>> diff --git a/drivers/gpu/ion/ion.c b/drivers/gpu/ion/ion.c >>>>>>>>> index fc152b9..c811380f 100644 >>>>>>>>> --- a/drivers/gpu/ion/ion.c >>>>>>>>> +++ b/drivers/gpu/ion/ion.c >>>>>>>>> @@ -414,7 +414,7 @@ struct ion_handle *ion_alloc(struct ion_client *client, size_t len, >>>>>>>>> if (!((1 << heap->type) & client->heap_mask)) >>>>>>>>> continue; >>>>>>>>> /* if the caller didn't specify this heap type */ >>>>>>>>> - if (!((1 << heap->id) & heap_mask)) >>>>>>>>> + if (!((1 << heap->type) & heap_mask)) >>>>>>>>> continue; >>>>>>>>> buffer = ion_buffer_create(heap, dev, len, align, flags); >>>>>>>>> if (!IS_ERR_OR_NULL(buffer)) >>>>>>>>> -- >>>>>>>>> 1.8.0 >>>>>>>>> >>>>>>>>> >>>>>>>>> _______________________________________________ >>>>>>>>> Linaro-mm-sig mailing list >>>>>>>>> Linaro-mm-sig@lists.linaro.org >>>>>>>>> http://lists.linaro.org/mailman/listinfo/linaro-mm-sig >>>>>>> >>>>>> >>>>>> - Nishanth Peethambaran >>>>> >>>> >>>> - Nishanth Peethambaran >>>> >>>> _______________________________________________ >>>> Linaro-mm-sig mailing list >>>> Linaro-mm-sig@lists.linaro.org >>>> http://lists.linaro.org/mailman/listinfo/linaro-mm-sig
diff --git a/drivers/gpu/ion/ion.c b/drivers/gpu/ion/ion.c index fc152b9..c811380f 100644 --- a/drivers/gpu/ion/ion.c +++ b/drivers/gpu/ion/ion.c @@ -414,7 +414,7 @@ struct ion_handle *ion_alloc(struct ion_client *client, size_t len, if (!((1 << heap->type) & client->heap_mask)) continue; /* if the caller didn't specify this heap type */ - if (!((1 << heap->id) & heap_mask)) + if (!((1 << heap->type) & heap_mask)) continue; buffer = ion_buffer_create(heap, dev, len, align, flags); if (!IS_ERR_OR_NULL(buffer))
The heap id is compared against the heap mask in ion_alloc which is incorrect, the heap type should be used. Signed-off-by: Johan Mossberg <johan.mossberg@stericsson.com> --- drivers/gpu/ion/ion.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)