diff mbox series

[EARLY,RFC,3/4] ion: Add HEAP_INFO ioctl to be able to fetch heap type

Message ID 1550262252-15558-4-git-send-email-john.stultz@linaro.org
State New
Headers show
Series ION per heap devices | expand

Commit Message

John Stultz Feb. 15, 2019, 8:24 p.m. UTC
The per-device heaps don't support HEAP_QUERY ioctl, since
the name is provided in the devnode path and the heapid isn't
useful with the new interface (one uses the fd of heapdevice).

But, one missing bit of functionality is a way to find the
heap type. So provide a HEAP_INFO ioctl which exposes the
heap type out so there is the potential for some sort of
dynamic heap matching/discovery.

Most likely this IOCTL will be useful when extended to allow
some sort of opaque constraint bitfield to be shared so userland
can match heaps with devices in a fully dynamic way.

Cc: Laura Abbott <labbott@redhat.com>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: Liam Mark <lmark@codeaurora.org>
Cc: Brian Starkey <Brian.Starkey@arm.com>
Cc: Andrew F. Davis <afd@ti.com>
Cc: Alistair Strachan <astrachan@google.com>
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: John Stultz <john.stultz@linaro.org>

---
 drivers/staging/android/ion/ion-ioctl.c | 12 ++++++++++++
 drivers/staging/android/uapi/ion.h      | 22 ++++++++++++++++++++++
 2 files changed, 34 insertions(+)

-- 
2.7.4

Comments

Laura Abbott Feb. 19, 2019, 9:13 p.m. UTC | #1
On 2/15/19 12:24 PM, John Stultz wrote:
> The per-device heaps don't support HEAP_QUERY ioctl, since
> the name is provided in the devnode path and the heapid isn't
> useful with the new interface (one uses the fd of heapdevice).
> 
> But, one missing bit of functionality is a way to find the
> heap type. So provide a HEAP_INFO ioctl which exposes the
> heap type out so there is the potential for some sort of
> dynamic heap matching/discovery.
> 
> Most likely this IOCTL will be useful when extended to allow
> some sort of opaque constraint bitfield to be shared so userland
> can match heaps with devices in a fully dynamic way.
> 

We've been waiting on the constraint solving for a while and
it's never really happened :(

It certainly works but I'm concerned about adding this and
then finding (yet again) that it doesn't work. We're
getting the heap name now but do we lose anything if we
don't expose it as part of the ABI?

> Cc: Laura Abbott <labbott@redhat.com>
> Cc: Sumit Semwal <sumit.semwal@linaro.org>
> Cc: Liam Mark <lmark@codeaurora.org>
> Cc: Brian Starkey <Brian.Starkey@arm.com>
> Cc: Andrew F. Davis <afd@ti.com>
> Cc: Alistair Strachan <astrachan@google.com>
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
>   drivers/staging/android/ion/ion-ioctl.c | 12 ++++++++++++
>   drivers/staging/android/uapi/ion.h      | 22 ++++++++++++++++++++++
>   2 files changed, 34 insertions(+)
> 
> diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c
> index ea8d263..6db5969 100644
> --- a/drivers/staging/android/ion/ion-ioctl.c
> +++ b/drivers/staging/android/ion/ion-ioctl.c
> @@ -14,6 +14,7 @@ union ion_ioctl_arg {
>   	struct ion_allocation_data allocation;
>   	struct ion_heap_allocation_data heap_allocation;
>   	struct ion_heap_query query;
> +	struct ion_heap_info heap_info;
>   	u32 version;
>   };
>   
> @@ -149,6 +150,17 @@ long ion_heap_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>   
>   		break;
>   	}
> +	case ION_IOC_HEAP_INFO:
> +	{
> +		struct miscdevice *miscdev = filp->private_data;
> +		struct ion_heap *heap;
> +
> +		heap = container_of(miscdev, struct ion_heap, heap_dev);
> +
> +		data.heap_info.type = heap->type;
> +
> +		break;
> +	}
>   	case ION_IOC_VERSION:
>   		data.version = ION_VERSION;
>   		break;
> diff --git a/drivers/staging/android/uapi/ion.h b/drivers/staging/android/uapi/ion.h
> index 20db09f..1b3ca1e 100644
> --- a/drivers/staging/android/uapi/ion.h
> +++ b/drivers/staging/android/uapi/ion.h
> @@ -111,6 +111,19 @@ struct ion_heap_data {
>   };
>   
>   /**
> + * struct ion_heap_info - Info about the heap
> + *
> + */
> +struct ion_heap_info {
> +	__u32 type;
> +	__u32 reserved0;
> +	__u32 reserved1;
> +	__u32 reserved2;
> +	__u32 reserved3;
> +	__u32 reserved4;
> +};
> +
> +/**
>    * struct ion_heap_query - collection of data about all heaps
>    * @cnt - total number of heaps to be copied
>    * @heaps - buffer to copy heap data
> @@ -159,4 +172,13 @@ struct ion_heap_query {
>   #define ION_IOC_HEAP_ALLOC	_IOWR(ION_IOC_MAGIC, 10, \
>   				      struct ion_heap_allocation_data)
>   
> +/**
> + * DOC: ION_IOC_HEAP_INFO - allocate memory from heap
> + *
> + * Takes an ion_heap_query structure and populates information about
> + * available Ion heaps.
> + */
> +#define ION_IOC_HEAP_INFO	_IOWR(ION_IOC_MAGIC, 11, \
> +				      struct ion_heap_allocation_data)
> +
>   #endif /* _UAPI_LINUX_ION_H */
>
Andrew Davis Feb. 19, 2019, 9:39 p.m. UTC | #2
On 2/19/19 3:13 PM, Laura Abbott wrote:
> On 2/15/19 12:24 PM, John Stultz wrote:
>> The per-device heaps don't support HEAP_QUERY ioctl, since
>> the name is provided in the devnode path and the heapid isn't
>> useful with the new interface (one uses the fd of heapdevice).
>>
>> But, one missing bit of functionality is a way to find the
>> heap type. So provide a HEAP_INFO ioctl which exposes the
>> heap type out so there is the potential for some sort of
>> dynamic heap matching/discovery.
>>
>> Most likely this IOCTL will be useful when extended to allow
>> some sort of opaque constraint bitfield to be shared so userland
>> can match heaps with devices in a fully dynamic way.
>>
> 
> We've been waiting on the constraint solving for a while and
> it's never really happened :(
> 

Most likely there will never be a one-size-fits-all solution here. So
allowing for an extensible ABI that permits new information to be
exported as needed will be important.

> It certainly works but I'm concerned about adding this and
> then finding (yet again) that it doesn't work. We're
> getting the heap name now but do we lose anything if we
> don't expose it as part of the ABI?
> 

We can always add more ioctls, we cant go back and remove the old ones
if we make them too clunky and have to remove something they expose. A
simple starting ABI seems to make the most sense here. Even heap type
doesn't look like a good thing to expose, it is just as static and
one-off as heap name, I don't see it having all that much use :/

Andrew

>> Cc: Laura Abbott <labbott@redhat.com>
>> Cc: Sumit Semwal <sumit.semwal@linaro.org>
>> Cc: Liam Mark <lmark@codeaurora.org>
>> Cc: Brian Starkey <Brian.Starkey@arm.com>
>> Cc: Andrew F. Davis <afd@ti.com>
>> Cc: Alistair Strachan <astrachan@google.com>
>> Cc: dri-devel@lists.freedesktop.org
>> Signed-off-by: John Stultz <john.stultz@linaro.org>
>> ---
>>   drivers/staging/android/ion/ion-ioctl.c | 12 ++++++++++++
>>   drivers/staging/android/uapi/ion.h      | 22 ++++++++++++++++++++++
>>   2 files changed, 34 insertions(+)
>>
>> diff --git a/drivers/staging/android/ion/ion-ioctl.c
>> b/drivers/staging/android/ion/ion-ioctl.c
>> index ea8d263..6db5969 100644
>> --- a/drivers/staging/android/ion/ion-ioctl.c
>> +++ b/drivers/staging/android/ion/ion-ioctl.c
>> @@ -14,6 +14,7 @@ union ion_ioctl_arg {
>>       struct ion_allocation_data allocation;
>>       struct ion_heap_allocation_data heap_allocation;
>>       struct ion_heap_query query;
>> +    struct ion_heap_info heap_info;
>>       u32 version;
>>   };
>>   @@ -149,6 +150,17 @@ long ion_heap_ioctl(struct file *filp, unsigned
>> int cmd, unsigned long arg)
>>             break;
>>       }
>> +    case ION_IOC_HEAP_INFO:
>> +    {
>> +        struct miscdevice *miscdev = filp->private_data;
>> +        struct ion_heap *heap;
>> +
>> +        heap = container_of(miscdev, struct ion_heap, heap_dev);
>> +
>> +        data.heap_info.type = heap->type;
>> +
>> +        break;
>> +    }
>>       case ION_IOC_VERSION:
>>           data.version = ION_VERSION;
>>           break;
>> diff --git a/drivers/staging/android/uapi/ion.h
>> b/drivers/staging/android/uapi/ion.h
>> index 20db09f..1b3ca1e 100644
>> --- a/drivers/staging/android/uapi/ion.h
>> +++ b/drivers/staging/android/uapi/ion.h
>> @@ -111,6 +111,19 @@ struct ion_heap_data {
>>   };
>>     /**
>> + * struct ion_heap_info - Info about the heap
>> + *
>> + */
>> +struct ion_heap_info {
>> +    __u32 type;
>> +    __u32 reserved0;
>> +    __u32 reserved1;
>> +    __u32 reserved2;
>> +    __u32 reserved3;
>> +    __u32 reserved4;
>> +};
>> +
>> +/**
>>    * struct ion_heap_query - collection of data about all heaps
>>    * @cnt - total number of heaps to be copied
>>    * @heaps - buffer to copy heap data
>> @@ -159,4 +172,13 @@ struct ion_heap_query {
>>   #define ION_IOC_HEAP_ALLOC    _IOWR(ION_IOC_MAGIC, 10, \
>>                         struct ion_heap_allocation_data)
>>   +/**
>> + * DOC: ION_IOC_HEAP_INFO - allocate memory from heap
>> + *
>> + * Takes an ion_heap_query structure and populates information about
>> + * available Ion heaps.
>> + */
>> +#define ION_IOC_HEAP_INFO    _IOWR(ION_IOC_MAGIC, 11, \
>> +                      struct ion_heap_allocation_data)
>> +
>>   #endif /* _UAPI_LINUX_ION_H */
>>
>
Laura Abbott Feb. 19, 2019, 9:46 p.m. UTC | #3
On 2/19/19 1:39 PM, Andrew F. Davis wrote:
> On 2/19/19 3:13 PM, Laura Abbott wrote:
>> On 2/15/19 12:24 PM, John Stultz wrote:
>>> The per-device heaps don't support HEAP_QUERY ioctl, since
>>> the name is provided in the devnode path and the heapid isn't
>>> useful with the new interface (one uses the fd of heapdevice).
>>>
>>> But, one missing bit of functionality is a way to find the
>>> heap type. So provide a HEAP_INFO ioctl which exposes the
>>> heap type out so there is the potential for some sort of
>>> dynamic heap matching/discovery.
>>>
>>> Most likely this IOCTL will be useful when extended to allow
>>> some sort of opaque constraint bitfield to be shared so userland
>>> can match heaps with devices in a fully dynamic way.
>>>
>>
>> We've been waiting on the constraint solving for a while and
>> it's never really happened :(
>>
> 
> Most likely there will never be a one-size-fits-all solution here. So
> allowing for an extensible ABI that permits new information to be
> exported as needed will be important.
> 
>> It certainly works but I'm concerned about adding this and
>> then finding (yet again) that it doesn't work. We're
>> getting the heap name now but do we lose anything if we
>> don't expose it as part of the ABI?
>>
> 
> We can always add more ioctls, we cant go back and remove the old ones
> if we make them too clunky and have to remove something they expose. A
> simple starting ABI seems to make the most sense here. Even heap type
> doesn't look like a good thing to expose, it is just as static and
> one-off as heap name, I don't see it having all that much use :/
> 

That's my point though, why are we adding this ioctl now if we
don't have a good idea of its use case or why we want the heap
type exposed? If we come up with a good use later we can
add the ioctl then with better requirements.

> Andrew
> 
>>> Cc: Laura Abbott <labbott@redhat.com>
>>> Cc: Sumit Semwal <sumit.semwal@linaro.org>
>>> Cc: Liam Mark <lmark@codeaurora.org>
>>> Cc: Brian Starkey <Brian.Starkey@arm.com>
>>> Cc: Andrew F. Davis <afd@ti.com>
>>> Cc: Alistair Strachan <astrachan@google.com>
>>> Cc: dri-devel@lists.freedesktop.org
>>> Signed-off-by: John Stultz <john.stultz@linaro.org>
>>> ---
>>>    drivers/staging/android/ion/ion-ioctl.c | 12 ++++++++++++
>>>    drivers/staging/android/uapi/ion.h      | 22 ++++++++++++++++++++++
>>>    2 files changed, 34 insertions(+)
>>>
>>> diff --git a/drivers/staging/android/ion/ion-ioctl.c
>>> b/drivers/staging/android/ion/ion-ioctl.c
>>> index ea8d263..6db5969 100644
>>> --- a/drivers/staging/android/ion/ion-ioctl.c
>>> +++ b/drivers/staging/android/ion/ion-ioctl.c
>>> @@ -14,6 +14,7 @@ union ion_ioctl_arg {
>>>        struct ion_allocation_data allocation;
>>>        struct ion_heap_allocation_data heap_allocation;
>>>        struct ion_heap_query query;
>>> +    struct ion_heap_info heap_info;
>>>        u32 version;
>>>    };
>>>    @@ -149,6 +150,17 @@ long ion_heap_ioctl(struct file *filp, unsigned
>>> int cmd, unsigned long arg)
>>>              break;
>>>        }
>>> +    case ION_IOC_HEAP_INFO:
>>> +    {
>>> +        struct miscdevice *miscdev = filp->private_data;
>>> +        struct ion_heap *heap;
>>> +
>>> +        heap = container_of(miscdev, struct ion_heap, heap_dev);
>>> +
>>> +        data.heap_info.type = heap->type;
>>> +
>>> +        break;
>>> +    }
>>>        case ION_IOC_VERSION:
>>>            data.version = ION_VERSION;
>>>            break;
>>> diff --git a/drivers/staging/android/uapi/ion.h
>>> b/drivers/staging/android/uapi/ion.h
>>> index 20db09f..1b3ca1e 100644
>>> --- a/drivers/staging/android/uapi/ion.h
>>> +++ b/drivers/staging/android/uapi/ion.h
>>> @@ -111,6 +111,19 @@ struct ion_heap_data {
>>>    };
>>>      /**
>>> + * struct ion_heap_info - Info about the heap
>>> + *
>>> + */
>>> +struct ion_heap_info {
>>> +    __u32 type;
>>> +    __u32 reserved0;
>>> +    __u32 reserved1;
>>> +    __u32 reserved2;
>>> +    __u32 reserved3;
>>> +    __u32 reserved4;
>>> +};
>>> +
>>> +/**
>>>     * struct ion_heap_query - collection of data about all heaps
>>>     * @cnt - total number of heaps to be copied
>>>     * @heaps - buffer to copy heap data
>>> @@ -159,4 +172,13 @@ struct ion_heap_query {
>>>    #define ION_IOC_HEAP_ALLOC    _IOWR(ION_IOC_MAGIC, 10, \
>>>                          struct ion_heap_allocation_data)
>>>    +/**
>>> + * DOC: ION_IOC_HEAP_INFO - allocate memory from heap
>>> + *
>>> + * Takes an ion_heap_query structure and populates information about
>>> + * available Ion heaps.
>>> + */
>>> +#define ION_IOC_HEAP_INFO    _IOWR(ION_IOC_MAGIC, 11, \
>>> +                      struct ion_heap_allocation_data)
>>> +
>>>    #endif /* _UAPI_LINUX_ION_H */
>>>
>>
John Stultz Feb. 19, 2019, 9:47 p.m. UTC | #4
On Tue, Feb 19, 2019 at 1:13 PM Laura Abbott <labbott@redhat.com> wrote:
>
> On 2/15/19 12:24 PM, John Stultz wrote:
> > The per-device heaps don't support HEAP_QUERY ioctl, since
> > the name is provided in the devnode path and the heapid isn't
> > useful with the new interface (one uses the fd of heapdevice).
> >
> > But, one missing bit of functionality is a way to find the
> > heap type. So provide a HEAP_INFO ioctl which exposes the
> > heap type out so there is the potential for some sort of
> > dynamic heap matching/discovery.
> >
> > Most likely this IOCTL will be useful when extended to allow
> > some sort of opaque constraint bitfield to be shared so userland
> > can match heaps with devices in a fully dynamic way.
> >
>
> We've been waiting on the constraint solving for a while and
> it's never really happened :(
>

Yea. I'm not trying to open that up again.

> It certainly works but I'm concerned about adding this and
> then finding (yet again) that it doesn't work. We're
> getting the heap name now but do we lose anything if we
> don't expose it as part of the ABI?

Right. So all we're exporting in this patch is the heap_type. This was
somewhat of an afterthought for me, as practically, I suspect the
gralloc users of ion will know which heap they want by name, and won't
do any sort of dynamic heap finding.

That said, ion's current API provides the QUERY interface which gives
you a list of heap ids/names/types, so if you wanted something that on
any random system was able to find a ION_HEAP_TYPE_DMA heap and use
it, you could.

So this HEAP_INFO ioctl provides a way to do the same. That's it.

That said, I could envision the ioctl to be extended (via the reserved
values) to provide some sort of constraint cookie to allow for
constraint solving, but that's still a unsolved issue at large.

Given the handwaving at the constraints part, and that the heap type
is a pretty coarse grained enum (only 6 types, as of now - one being
catchall "custom"), I'm fine holding off on this bit unless folks
really see it as valuable.

thanks
-john
John Stultz Feb. 19, 2019, 9:50 p.m. UTC | #5
On Tue, Feb 19, 2019 at 1:46 PM Laura Abbott <labbott@redhat.com> wrote:
>
> On 2/19/19 1:39 PM, Andrew F. Davis wrote:
> > On 2/19/19 3:13 PM, Laura Abbott wrote:
> >> On 2/15/19 12:24 PM, John Stultz wrote:
> >>> The per-device heaps don't support HEAP_QUERY ioctl, since
> >>> the name is provided in the devnode path and the heapid isn't
> >>> useful with the new interface (one uses the fd of heapdevice).
> >>>
> >>> But, one missing bit of functionality is a way to find the
> >>> heap type. So provide a HEAP_INFO ioctl which exposes the
> >>> heap type out so there is the potential for some sort of
> >>> dynamic heap matching/discovery.
> >>>
> >>> Most likely this IOCTL will be useful when extended to allow
> >>> some sort of opaque constraint bitfield to be shared so userland
> >>> can match heaps with devices in a fully dynamic way.
> >>>
> >>
> >> We've been waiting on the constraint solving for a while and
> >> it's never really happened :(
> >>
> >
> > Most likely there will never be a one-size-fits-all solution here. So
> > allowing for an extensible ABI that permits new information to be
> > exported as needed will be important.
> >
> >> It certainly works but I'm concerned about adding this and
> >> then finding (yet again) that it doesn't work. We're
> >> getting the heap name now but do we lose anything if we
> >> don't expose it as part of the ABI?
> >>
> >
> > We can always add more ioctls, we cant go back and remove the old ones
> > if we make them too clunky and have to remove something they expose. A
> > simple starting ABI seems to make the most sense here. Even heap type
> > doesn't look like a good thing to expose, it is just as static and
> > one-off as heap name, I don't see it having all that much use :/
> >
>
> That's my point though, why are we adding this ioctl now if we
> don't have a good idea of its use case or why we want the heap
> type exposed? If we come up with a good use later we can
> add the ioctl then with better requirements.

Ok. I think we three are in agreement here.  Best to drop this bit and
leave it till someone has a clear need/use.

thanks
-john
Brian Starkey Feb. 20, 2019, 3:07 p.m. UTC | #6
On Tue, Feb 19, 2019 at 01:47:36PM -0800, John Stultz wrote:
> On Tue, Feb 19, 2019 at 1:13 PM Laura Abbott <labbott@redhat.com> wrote:
> >
> > On 2/15/19 12:24 PM, John Stultz wrote:
> > > The per-device heaps don't support HEAP_QUERY ioctl, since
> > > the name is provided in the devnode path and the heapid isn't
> > > useful with the new interface (one uses the fd of heapdevice).
> > >
> > > But, one missing bit of functionality is a way to find the
> > > heap type. So provide a HEAP_INFO ioctl which exposes the
> > > heap type out so there is the potential for some sort of
> > > dynamic heap matching/discovery.
> > >
> > > Most likely this IOCTL will be useful when extended to allow
> > > some sort of opaque constraint bitfield to be shared so userland
> > > can match heaps with devices in a fully dynamic way.
> > >
> >
> > We've been waiting on the constraint solving for a while and
> > it's never really happened :(
> >
> 
> Yea. I'm not trying to open that up again.
> 
> > It certainly works but I'm concerned about adding this and
> > then finding (yet again) that it doesn't work. We're
> > getting the heap name now but do we lose anything if we
> > don't expose it as part of the ABI?
> 
> Right. So all we're exporting in this patch is the heap_type. This was
> somewhat of an afterthought for me, as practically, I suspect the
> gralloc users of ion will know which heap they want by name, and won't
> do any sort of dynamic heap finding.
> 
> That said, ion's current API provides the QUERY interface which gives
> you a list of heap ids/names/types, so if you wanted something that on
> any random system was able to find a ION_HEAP_TYPE_DMA heap and use
> it, you could.
> 
> So this HEAP_INFO ioctl provides a way to do the same. That's it.
> 
> That said, I could envision the ioctl to be extended (via the reserved
> values) to provide some sort of constraint cookie to allow for
> constraint solving, but that's still a unsolved issue at large.
> 
> Given the handwaving at the constraints part, and that the heap type
> is a pretty coarse grained enum (only 6 types, as of now - one being
> catchall "custom"), I'm fine holding off on this bit unless folks
> really see it as valuable.

What do people think about putting the heap type into the device name?
The whole query API is then replaced with `ls`

-Brian

> 
> thanks
> -john
John Stultz Feb. 20, 2019, 5:29 p.m. UTC | #7
On Wed, Feb 20, 2019 at 7:07 AM Brian Starkey <Brian.Starkey@arm.com> wrote:
>
> On Tue, Feb 19, 2019 at 01:47:36PM -0800, John Stultz wrote:
> > On Tue, Feb 19, 2019 at 1:13 PM Laura Abbott <labbott@redhat.com> wrote:
> > >
> > > On 2/15/19 12:24 PM, John Stultz wrote:
> > > > The per-device heaps don't support HEAP_QUERY ioctl, since
> > > > the name is provided in the devnode path and the heapid isn't
> > > > useful with the new interface (one uses the fd of heapdevice).
> > > >
> > > > But, one missing bit of functionality is a way to find the
> > > > heap type. So provide a HEAP_INFO ioctl which exposes the
> > > > heap type out so there is the potential for some sort of
> > > > dynamic heap matching/discovery.
> > > >
> > > > Most likely this IOCTL will be useful when extended to allow
> > > > some sort of opaque constraint bitfield to be shared so userland
> > > > can match heaps with devices in a fully dynamic way.
> > > >
> > >
> > > We've been waiting on the constraint solving for a while and
> > > it's never really happened :(
> > >
> >
> > Yea. I'm not trying to open that up again.
> >
> > > It certainly works but I'm concerned about adding this and
> > > then finding (yet again) that it doesn't work. We're
> > > getting the heap name now but do we lose anything if we
> > > don't expose it as part of the ABI?
> >
> > Right. So all we're exporting in this patch is the heap_type. This was
> > somewhat of an afterthought for me, as practically, I suspect the
> > gralloc users of ion will know which heap they want by name, and won't
> > do any sort of dynamic heap finding.
> >
> > That said, ion's current API provides the QUERY interface which gives
> > you a list of heap ids/names/types, so if you wanted something that on
> > any random system was able to find a ION_HEAP_TYPE_DMA heap and use
> > it, you could.
> >
> > So this HEAP_INFO ioctl provides a way to do the same. That's it.
> >
> > That said, I could envision the ioctl to be extended (via the reserved
> > values) to provide some sort of constraint cookie to allow for
> > constraint solving, but that's still a unsolved issue at large.
> >
> > Given the handwaving at the constraints part, and that the heap type
> > is a pretty coarse grained enum (only 6 types, as of now - one being
> > catchall "custom"), I'm fine holding off on this bit unless folks
> > really see it as valuable.
>
> What do people think about putting the heap type into the device name?
> The whole query API is then replaced with `ls`

Hrm. I'm a bit hesitant on that, as that enshrines a naming convention as abi.

Also I personally don't think the heap-types are particularly well
defined. The memory itself is either contiguous or not, 32bit dma
accessible or not, etc. -  the specific implementation "type"
[system-contig, carveout, chunk, cma] isn't as useful from userland
other then conventions of how the heaps may be partitioned between
users.

So I'm probably happier to hide that detail.  If someone wants to do
dynamic heap discovery, we probably need a proper constraint solution.

For now, a potential generic gralloc would just have to have some
device specific policy file that defines which heap device to use for
which type of allocation it wants. Not completely unlike an fstab.

thanks
-john
diff mbox series

Patch

diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c
index ea8d263..6db5969 100644
--- a/drivers/staging/android/ion/ion-ioctl.c
+++ b/drivers/staging/android/ion/ion-ioctl.c
@@ -14,6 +14,7 @@  union ion_ioctl_arg {
 	struct ion_allocation_data allocation;
 	struct ion_heap_allocation_data heap_allocation;
 	struct ion_heap_query query;
+	struct ion_heap_info heap_info;
 	u32 version;
 };
 
@@ -149,6 +150,17 @@  long ion_heap_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 
 		break;
 	}
+	case ION_IOC_HEAP_INFO:
+	{
+		struct miscdevice *miscdev = filp->private_data;
+		struct ion_heap *heap;
+
+		heap = container_of(miscdev, struct ion_heap, heap_dev);
+
+		data.heap_info.type = heap->type;
+
+		break;
+	}
 	case ION_IOC_VERSION:
 		data.version = ION_VERSION;
 		break;
diff --git a/drivers/staging/android/uapi/ion.h b/drivers/staging/android/uapi/ion.h
index 20db09f..1b3ca1e 100644
--- a/drivers/staging/android/uapi/ion.h
+++ b/drivers/staging/android/uapi/ion.h
@@ -111,6 +111,19 @@  struct ion_heap_data {
 };
 
 /**
+ * struct ion_heap_info - Info about the heap
+ *
+ */
+struct ion_heap_info {
+	__u32 type;
+	__u32 reserved0;
+	__u32 reserved1;
+	__u32 reserved2;
+	__u32 reserved3;
+	__u32 reserved4;
+};
+
+/**
  * struct ion_heap_query - collection of data about all heaps
  * @cnt - total number of heaps to be copied
  * @heaps - buffer to copy heap data
@@ -159,4 +172,13 @@  struct ion_heap_query {
 #define ION_IOC_HEAP_ALLOC	_IOWR(ION_IOC_MAGIC, 10, \
 				      struct ion_heap_allocation_data)
 
+/**
+ * DOC: ION_IOC_HEAP_INFO - allocate memory from heap
+ *
+ * Takes an ion_heap_query structure and populates information about
+ * available Ion heaps.
+ */
+#define ION_IOC_HEAP_INFO	_IOWR(ION_IOC_MAGIC, 11, \
+				      struct ion_heap_allocation_data)
+
 #endif /* _UAPI_LINUX_ION_H */