gpu: ion: Fix incorrect usage of heap id in ion_alloc

Message ID 1355215912-28715-1-git-send-email-johan.mossberg@stericsson.com
State New
Headers show

Commit Message

Johan Mossberg Dec. 11, 2012, 8:51 a.m.
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(-)

Comments

Nishanth Peethambaran Dec. 11, 2012, 9:55 a.m. | #1
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
Johan Mossberg Dec. 11, 2012, 11:04 a.m. | #2
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 Dec. 11, 2012, 11:50 a.m. | #3
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
Johan Mossberg Dec. 11, 2012, 12:50 p.m. | #4
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 Dec. 11, 2012, 1:58 p.m. | #5
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
Rebecca Schultz Zavin Dec. 11, 2012, 7:40 p.m. | #6
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
Nishanth Peethambaran Dec. 12, 2012, 1:01 a.m. | #7
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
Rebecca Schultz Zavin Dec. 12, 2012, 1:12 a.m. | #8
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
Nishanth Peethambaran Dec. 12, 2012, 1:36 a.m. | #9
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

Patch hide | download patch | download mbox

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