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

[Laura Abbott]

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 at redhat.com>
>>> Cc: Sumit Semwal <sumit.semwal at linaro.org>
>>> Cc: Liam Mark <lmark at codeaurora.org>
>>> Cc: Brian Starkey <Brian.Starkey at arm.com>
>>> Cc: Andrew F. Davis <afd at ti.com>
>>> Cc: Alistair Strachan <astrachan at google.com>
>>> Cc: dri-devel at lists.freedesktop.org
>>> Signed-off-by: John Stultz <john.stultz at 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 */
>>>
>>