Previously the heap to allocate from was selected by a mask of allowed
heap types. This may have been done as a primitive form of constraint
solving, the first heap type that matched any set bit of the heap mask
was allocated from, unless that heap was excluded by having its heap
ID bit not set in the separate passed in heap ID mask.
The heap type does not really represent constraints that should be
matched against to begin with. So the patch [0] removed the the heap type
mask matching but unfortunately left the heap ID mask check (possibly by
mistake or to preserve API). Therefor we now only have a mask of heap
IDs, but heap IDs are unique identifiers and have nothing to do with the
underlying heap, so mask matching is not useful here. This also limits us
to 32 heaps total in a system.
With the heap query API users can find the right heap based on type or
name themselves then just supply the ID for that heap. Remove heap ID
mask and allow users to specify heap ID directly by its number.
I know this is an ABI break, but we are in staging so lets get this over
with now rather than limit ourselves later.
[0] commit 2bb9f5034ec7 ("gpu: ion: Remove heapmask from client")
Signed-off-by: Andrew F. Davis <[email protected]>
---
This also means we don't need to store the available heaps in a plist,
we only operation we care about is lookup, so a better data structure
should be chosen at some point, regular list or xarray maybe?
This is base on -next as to be on top of the other taken Ion patches.
Changes from v1:
- Fix spelling in commit message
- Cleanup logic per Brian's suggestion
drivers/staging/android/ion/ion.c | 28 +++++++++++++---------------
drivers/staging/android/uapi/ion.h | 6 ++----
2 files changed, 15 insertions(+), 19 deletions(-)
diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
index 92c2914239e3..b0b0d0b587c2 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -387,7 +387,7 @@ static const struct dma_buf_ops dma_buf_ops = {
.unmap = ion_dma_buf_kunmap,
};
-static int ion_alloc(size_t len, unsigned int heap_id_mask, unsigned int flags)
+static int ion_alloc(size_t len, unsigned int heap_id, unsigned int flags)
{
struct ion_device *dev = internal_dev;
struct ion_buffer *buffer = NULL;
@@ -396,27 +396,25 @@ static int ion_alloc(size_t len, unsigned int heap_id_mask, unsigned int flags)
int fd;
struct dma_buf *dmabuf;
- pr_debug("%s: len %zu heap_id_mask %u flags %x\n", __func__,
- len, heap_id_mask, flags);
- /*
- * traverse the list of heaps available in this system in priority
- * order. If the heap type is supported by the client, and matches the
- * request of the caller allocate from it. Repeat until allocate has
- * succeeded or all heaps have been tried
- */
+ pr_debug("%s: len %zu heap_id %u flags %x\n", __func__,
+ len, heap_id, flags);
+
len = PAGE_ALIGN(len);
if (!len)
return -EINVAL;
+ /*
+ * Traverse the list of heaps available in this system. If the
+ * heap id matches the request of the caller allocate from it.
+ */
down_read(&dev->lock);
plist_for_each_entry(heap, &dev->heaps, node) {
- /* if the caller didn't specify this heap id */
- if (!((1 << heap->id) & heap_id_mask))
- continue;
- buffer = ion_buffer_create(heap, dev, len, flags);
- if (!IS_ERR(buffer))
+ /* if the caller specified this heap id */
+ if (heap->id == heap_id) {
+ buffer = ion_buffer_create(heap, dev, len, flags);
break;
+ }
}
up_read(&dev->lock);
@@ -541,7 +539,7 @@ static long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
int fd;
fd = ion_alloc(data.allocation.len,
- data.allocation.heap_id_mask,
+ data.allocation.heap_id,
data.allocation.flags);
if (fd < 0)
return fd;
diff --git a/drivers/staging/android/uapi/ion.h b/drivers/staging/android/uapi/ion.h
index 5d7009884c13..6a78a1e23251 100644
--- a/drivers/staging/android/uapi/ion.h
+++ b/drivers/staging/android/uapi/ion.h
@@ -35,8 +35,6 @@ enum ion_heap_type {
*/
};
-#define ION_NUM_HEAP_IDS (sizeof(unsigned int) * 8)
-
/**
* allocation flags - the lower 16 bits are used by core ion, the upper 16
* bits are reserved for use by the heaps themselves.
@@ -59,7 +57,7 @@ enum ion_heap_type {
/**
* struct ion_allocation_data - metadata passed from userspace for allocations
* @len: size of the allocation
- * @heap_id_mask: mask of heap ids to allocate from
+ * @heap_id: heap id to allocate from
* @flags: flags passed to heap
* @handle: pointer that will be populated with a cookie to use to
* refer to this allocation
@@ -68,7 +66,7 @@ enum ion_heap_type {
*/
struct ion_allocation_data {
__u64 len;
- __u32 heap_id_mask;
+ __u32 heap_id;
__u32 flags;
__u32 fd;
__u32 unused;
--
2.19.1
On Mon, Jan 28, 2019 at 1:44 PM Andrew F. Davis <[email protected]> wrote:
>
> Previously the heap to allocate from was selected by a mask of allowed
> heap types. This may have been done as a primitive form of constraint
> solving, the first heap type that matched any set bit of the heap mask
> was allocated from, unless that heap was excluded by having its heap
> ID bit not set in the separate passed in heap ID mask.
>
> The heap type does not really represent constraints that should be
> matched against to begin with. So the patch [0] removed the the heap type
> mask matching but unfortunately left the heap ID mask check (possibly by
> mistake or to preserve API). Therefor we now only have a mask of heap
> IDs, but heap IDs are unique identifiers and have nothing to do with the
> underlying heap, so mask matching is not useful here. This also limits us
> to 32 heaps total in a system.
>
> With the heap query API users can find the right heap based on type or
> name themselves then just supply the ID for that heap. Remove heap ID
> mask and allow users to specify heap ID directly by its number.
Sorry for the very late reply. I just dug this out of my spam box.
While this seems like a reasonable cleanup (ABI pain aside), I'm
curious as to other then the 32-heap limitation, what other benefits
this brings?
My hesitancy is that we still have fixed number to heap mapping.
Curious how this fits in (or if it would still be necessary) if we
finally moved to previously discussed per-heap device-nodes idea,
which is interesting to me as it avoids a fixed enumeration of heaps
in the kernel (and also allows for per heap permissions).
> I know this is an ABI break, but we are in staging so lets get this over
> with now rather than limit ourselves later.
>
> [0] commit 2bb9f5034ec7 ("gpu: ion: Remove heapmask from client")
>
> Signed-off-by: Andrew F. Davis <[email protected]>
> ---
>
> This also means we don't need to store the available heaps in a plist,
> we only operation we care about is lookup, so a better data structure
> should be chosen at some point, regular list or xarray maybe?
>
> This is base on -next as to be on top of the other taken Ion patches.
>
> Changes from v1:
> - Fix spelling in commit message
> - Cleanup logic per Brian's suggestion
>
Some thoughts, as this ABI break has the potential to be pretty painful.
1) Unfortunately, this ABI is exposed *through* libion via
ion_alloc/ion_alloc_fd out to gralloc implementations. Which means it
will have a wider impact to vendor userland code.
2) For patches that cause ABI breaks, it might be good to make it
clear in the commit what the userland impact looks like in userspace,
possibly with an example, so the poor folks who bisect down the change
as breaking their system in a year or so have a clear example as to
what they need to change in their code.
3) Also, its not clear how a given userland should distinguish between
the different ABIs. We already have logic in libion to distinguish
between pre-4.12 legacy and post-4.12 implementations (using implicit
ion_free() behavior). I don't see any such check we can make with this
code. Adding another ABI version may require we provide an actual
interface version ioctl.
thanks
-john
Hi John,
On Thu, Feb 14, 2019 at 09:38:29AM -0800, John Stultz wrote:
>
[snip]
> Some thoughts, as this ABI break has the potential to be pretty painful.
>
> 1) Unfortunately, this ABI is exposed *through* libion via
> ion_alloc/ion_alloc_fd out to gralloc implementations. Which means it
> will have a wider impact to vendor userland code.
I figured libion could fairly easily loop through all the set bits in
heap_mask and call the ioctl for each until it succeeds. That
preserves the old behaviour from the libion clients' perspective.
>
> 2) For patches that cause ABI breaks, it might be good to make it
> clear in the commit what the userland impact looks like in userspace,
> possibly with an example, so the poor folks who bisect down the change
> as breaking their system in a year or so have a clear example as to
> what they need to change in their code.
>
> 3) Also, its not clear how a given userland should distinguish between
> the different ABIs. We already have logic in libion to distinguish
> between pre-4.12 legacy and post-4.12 implementations (using implicit
> ion_free() behavior). I don't see any such check we can make with this
> code. Adding another ABI version may require we provide an actual
> interface version ioctl.
>
A slightly fragile/ugly approach might be to attempt a small
allocation with a heap_mask of 0xffffffff. On an "old" implementation,
you'd expect that to succeed, whereas it would/could be made to fail
in the "new" one.
Thanks,
-Brian
>
> thanks
> -john
On Fri, Feb 15, 2019 at 2:51 AM Brian Starkey <[email protected]> wrote:
>
> Hi John,
>
> On Thu, Feb 14, 2019 at 09:38:29AM -0800, John Stultz wrote:
> >
> [snip]
>
> > Some thoughts, as this ABI break has the potential to be pretty painful.
> >
> > 1) Unfortunately, this ABI is exposed *through* libion via
> > ion_alloc/ion_alloc_fd out to gralloc implementations. Which means it
> > will have a wider impact to vendor userland code.
>
> I figured libion could fairly easily loop through all the set bits in
> heap_mask and call the ioctl for each until it succeeds. That
> preserves the old behaviour from the libion clients' perspective.
Potentially, though that implicitly still caps the heaps to 32. So
I'm not sure what the net benefit would be.
> > 2) For patches that cause ABI breaks, it might be good to make it
> > clear in the commit what the userland impact looks like in userspace,
> > possibly with an example, so the poor folks who bisect down the change
> > as breaking their system in a year or so have a clear example as to
> > what they need to change in their code.
> >
> > 3) Also, its not clear how a given userland should distinguish between
> > the different ABIs. We already have logic in libion to distinguish
> > between pre-4.12 legacy and post-4.12 implementations (using implicit
> > ion_free() behavior). I don't see any such check we can make with this
> > code. Adding another ABI version may require we provide an actual
> > interface version ioctl.
> >
>
> A slightly fragile/ugly approach might be to attempt a small
> allocation with a heap_mask of 0xffffffff. On an "old" implementation,
> you'd expect that to succeed, whereas it would/could be made to fail
> in the "new" one.
Yea I think having a proper ION_IOC_VERSION is going to be necessary.
I'm hoping to send out an ugly first stab at the kernel side for
switching to per-heap devices (with a config for keeping /dev/ion for
legacy compat), which I hope will address the core issue this patch
does (moving away from heap masks to specifically requested heaps).
thanks
-john
On 2/15/19 1:01 PM, John Stultz wrote:
> On Fri, Feb 15, 2019 at 2:51 AM Brian Starkey <[email protected]> wrote:
>>
>> Hi John,
>>
>> On Thu, Feb 14, 2019 at 09:38:29AM -0800, John Stultz wrote:
>>>
>> [snip]
>>
>>> Some thoughts, as this ABI break has the potential to be pretty painful.
>>>
>>> 1) Unfortunately, this ABI is exposed *through* libion via
>>> ion_alloc/ion_alloc_fd out to gralloc implementations. Which means it
>>> will have a wider impact to vendor userland code.
>>
>> I figured libion could fairly easily loop through all the set bits in
>> heap_mask and call the ioctl for each until it succeeds. That
>> preserves the old behaviour from the libion clients' perspective.
>
> Potentially, though that implicitly still caps the heaps to 32. So
> I'm not sure what the net benefit would be.
>
That is a libion problem, libion can expose an un-capped API and users
can migrate.
>
>>> 2) For patches that cause ABI breaks, it might be good to make it
>>> clear in the commit what the userland impact looks like in userspace,
>>> possibly with an example, so the poor folks who bisect down the change
>>> as breaking their system in a year or so have a clear example as to
>>> what they need to change in their code.
>>>
>>> 3) Also, its not clear how a given userland should distinguish between
>>> the different ABIs. We already have logic in libion to distinguish
>>> between pre-4.12 legacy and post-4.12 implementations (using implicit
>>> ion_free() behavior). I don't see any such check we can make with this
>>> code. Adding another ABI version may require we provide an actual
>>> interface version ioctl.
>>>
>>
>> A slightly fragile/ugly approach might be to attempt a small
>> allocation with a heap_mask of 0xffffffff. On an "old" implementation,
>> you'd expect that to succeed, whereas it would/could be made to fail
>> in the "new" one.
>
> Yea I think having a proper ION_IOC_VERSION is going to be necessary.
>
I think that will be helpful to have ready the future just looking at
the way libdrm does things, but not right now as backwards compatibility
with staging code is not a reasonable thing to do.
> I'm hoping to send out an ugly first stab at the kernel side for
> switching to per-heap devices (with a config for keeping /dev/ion for
> legacy compat), which I hope will address the core issue this patch
> does (moving away from heap masks to specifically requested heaps).
>
Yes, that would remove the need for what this patch does.
Question though, what does the user side look like for this? With the
old /dev/ion we would:
ion_fd = open("/dev/ion")
ask for a list of heaps (ioctl on ion_fd)
iterate over the details of each heap
pick the best heap for the job
request allocation from that heap (ioctl on ion_fd)
with per-heap devs we need some way to iterate all over heap devices in
a system, and extract details from each heap device. Maybe we leave
/dev/ion but it's only job is to service ION_IOC_HEAP_QUERY requests but
instead of heap numbers it returns heap names, then device files just
match those names. Then we go allocate() from those.
If all that is addressed in your patch-set then feel free to ignore this
question :)
Andrew
> thanks
> -john
>
On Fri, Feb 15, 2019 at 11:22 AM Andrew F. Davis <[email protected]> wrote:
>
> On 2/15/19 1:01 PM, John Stultz wrote:
> > On Fri, Feb 15, 2019 at 2:51 AM Brian Starkey <[email protected]> wrote:
> >> On Thu, Feb 14, 2019 at 09:38:29AM -0800, John Stultz wrote:
> >>> 2) For patches that cause ABI breaks, it might be good to make it
> >>> clear in the commit what the userland impact looks like in userspace,
> >>> possibly with an example, so the poor folks who bisect down the change
> >>> as breaking their system in a year or so have a clear example as to
> >>> what they need to change in their code.
> >>>
> >>> 3) Also, its not clear how a given userland should distinguish between
> >>> the different ABIs. We already have logic in libion to distinguish
> >>> between pre-4.12 legacy and post-4.12 implementations (using implicit
> >>> ion_free() behavior). I don't see any such check we can make with this
> >>> code. Adding another ABI version may require we provide an actual
> >>> interface version ioctl.
> >>>
> >>
> >> A slightly fragile/ugly approach might be to attempt a small
> >> allocation with a heap_mask of 0xffffffff. On an "old" implementation,
> >> you'd expect that to succeed, whereas it would/could be made to fail
> >> in the "new" one.
> >
> > Yea I think having a proper ION_IOC_VERSION is going to be necessary.
> >
>
> I think that will be helpful to have ready the future just looking at
> the way libdrm does things, but not right now as backwards compatibility
> with staging code is not a reasonable thing to do.
I'm not sure I'm following what you mean here? While we don't have
any commitment to userland for interfaces in staging, the reality is
that there are a fair number of users affected, and we probably should
avoid causing any needless pain if possible.
Further, as part of my work, I try to keep the hikey boards with an
array of kernels (4.4, 4.9, 4.14, 4.19 and mainline) running with AOSP
master. Having hard build breaks so AOSP has to have build time
dependencies on newer or older kernels is a big pain, and the 4.12 ABI
break was not easy.
So yea, I don't think we should tie our hands in reworking the
interfaces, but it would be nice to avoid having subtle ABI changes
that don't have clear ways for userland to detect which interface
version its using.
> > I'm hoping to send out an ugly first stab at the kernel side for
> > switching to per-heap devices (with a config for keeping /dev/ion for
> > legacy compat), which I hope will address the core issue this patch
> > does (moving away from heap masks to specifically requested heaps).
> >
>
> Yes, that would remove the need for what this patch does.
> Question though, what does the user side look like for this? With the
> old /dev/ion we would:
>
> ion_fd = open("/dev/ion")
> ask for a list of heaps (ioctl on ion_fd)
> iterate over the details of each heap
> pick the best heap for the job
> request allocation from that heap (ioctl on ion_fd)
>
> with per-heap devs we need some way to iterate all over heap devices in
> a system, and extract details from each heap device. Maybe we leave
> /dev/ion but it's only job is to service ION_IOC_HEAP_QUERY requests but
> instead of heap numbers it returns heap names, then device files just
> match those names. Then we go allocate() from those.
>
So my initial thought is we simply use a /dev/ion_heaps/ dir which has
a list of heap devicenodes. /dev/ion goes away.
Currently ION_IOC_HEAP_QUERY returns:
char name[MAX_HEAP_NAME];
__u32 type;
__u32 heap_id;
The names are discoverable via "ls /dev/ion_heaps/"
The heap_id is really only useful as a handle, and after opening the
heap device, we'll have the fd to use.
The only detail we're missing is the type. I'm a little skeptical how
useful type is, but worse case we provide a QUERY ioctl on the heap
device to provide type info.
Most likely, I expect users to:
1) Open "/dev/ion_heaps/<heapname>" for the heap they want (since they
probably already know)
2) make a HEAP_ALLOCATE ioctl on the fd to allocate
But to match the use case you describe:
1) ls /dev/ion_heaps/ for a list of heaps
2) For each heap name, open the heap and make a QUERY ioctl to get
type info (for what its worth)
3) Pick best heap for the job (*handwaving magic!*)
4) Do an ALLOC ioctl on the heap's fd to allocate
Does that sound reasonable? And I don't really mean to dismiss the
dynamic picking of the best heap part, and having something like a
opaque constraints bitfield that each device and each heap export so
userland can match things up would be great. But since we don't have
any real solutions there yet(still!), it seems like most gralloc
implementations are likely to be fully knowing which heap they want at
allocation time.
thanks
-john
On 2/15/19 1:58 PM, John Stultz wrote:
> On Fri, Feb 15, 2019 at 11:22 AM Andrew F. Davis <[email protected]> wrote:
>>
>> On 2/15/19 1:01 PM, John Stultz wrote:
>>> On Fri, Feb 15, 2019 at 2:51 AM Brian Starkey <[email protected]> wrote:
>>>> On Thu, Feb 14, 2019 at 09:38:29AM -0800, John Stultz wrote:
>>>>> 2) For patches that cause ABI breaks, it might be good to make it
>>>>> clear in the commit what the userland impact looks like in userspace,
>>>>> possibly with an example, so the poor folks who bisect down the change
>>>>> as breaking their system in a year or so have a clear example as to
>>>>> what they need to change in their code.
>>>>>
>>>>> 3) Also, its not clear how a given userland should distinguish between
>>>>> the different ABIs. We already have logic in libion to distinguish
>>>>> between pre-4.12 legacy and post-4.12 implementations (using implicit
>>>>> ion_free() behavior). I don't see any such check we can make with this
>>>>> code. Adding another ABI version may require we provide an actual
>>>>> interface version ioctl.
>>>>>
>>>>
>>>> A slightly fragile/ugly approach might be to attempt a small
>>>> allocation with a heap_mask of 0xffffffff. On an "old" implementation,
>>>> you'd expect that to succeed, whereas it would/could be made to fail
>>>> in the "new" one.
>>>
>>> Yea I think having a proper ION_IOC_VERSION is going to be necessary.
>>>
>>
>> I think that will be helpful to have ready the future just looking at
>> the way libdrm does things, but not right now as backwards compatibility
>> with staging code is not a reasonable thing to do.
>
> I'm not sure I'm following what you mean here? While we don't have
> any commitment to userland for interfaces in staging, the reality is
> that there are a fair number of users affected, and we probably should
> avoid causing any needless pain if possible.
>
> Further, as part of my work, I try to keep the hikey boards with an
> array of kernels (4.4, 4.9, 4.14, 4.19 and mainline) running with AOSP
> master. Having hard build breaks so AOSP has to have build time
> dependencies on newer or older kernels is a big pain, and the 4.12 ABI
> break was not easy.
>
> So yea, I don't think we should tie our hands in reworking the
> interfaces, but it would be nice to avoid having subtle ABI changes
> that don't have clear ways for userland to detect which interface
> version its using.
>
Let me preference this by pointing out I've dealt with the same pain
internally with our Android and soon to also in AOSP for the Beagle x15
boards[0].. But my stance matches Christoph's in the other ION thread:
https://lkml.org/lkml/2019/1/19/53
The more freely we can make ABI changes here in staging the quicker we
can get this out of staging where the ABI can be locked down.
ION_IOC_VERSION should solve this anyway.
>>> I'm hoping to send out an ugly first stab at the kernel side for
>>> switching to per-heap devices (with a config for keeping /dev/ion for
>>> legacy compat), which I hope will address the core issue this patch
>>> does (moving away from heap masks to specifically requested heaps).
>>>
>>
>> Yes, that would remove the need for what this patch does.
>> Question though, what does the user side look like for this? With the
>> old /dev/ion we would:
>>
>> ion_fd = open("/dev/ion")
>> ask for a list of heaps (ioctl on ion_fd)
>> iterate over the details of each heap
>> pick the best heap for the job
>> request allocation from that heap (ioctl on ion_fd)
>>
>> with per-heap devs we need some way to iterate all over heap devices in
>> a system, and extract details from each heap device. Maybe we leave
>> /dev/ion but it's only job is to service ION_IOC_HEAP_QUERY requests but
>> instead of heap numbers it returns heap names, then device files just
>> match those names. Then we go allocate() from those.
>>
>
>
> So my initial thought is we simply use a /dev/ion_heaps/ dir which has
> a list of heap devicenodes. /dev/ion goes away.
>
> Currently ION_IOC_HEAP_QUERY returns:
> char name[MAX_HEAP_NAME];
> __u32 type;
> __u32 heap_id;
>
> The names are discoverable via "ls /dev/ion_heaps/"
>
> The heap_id is really only useful as a handle, and after opening the
> heap device, we'll have the fd to use.
>
So why have heap_id at all then?
> The only detail we're missing is the type. I'm a little skeptical how
> useful type is, but worse case we provide a QUERY ioctl on the heap
> device to provide type info.
>
> Most likely, I expect users to:
> 1) Open "/dev/ion_heaps/<heapname>" for the heap they want (since they
> probably already know)
> 2) make a HEAP_ALLOCATE ioctl on the fd to allocate
>
> But to match the use case you describe:
> 1) ls /dev/ion_heaps/ for a list of heaps
Yuk, dirent.h and friends :(
> 2) For each heap name, open the heap and make a QUERY ioctl to get
> type info (for what its worth)
> 3) Pick best heap for the job (*handwaving magic!*)
> 4) Do an ALLOC ioctl on the heap's fd to allocate
>
>
> Does that sound reasonable? And I don't really mean to dismiss the
> dynamic picking of the best heap part, and having something like a
> opaque constraints bitfield that each device and each heap export so
> userland can match things up would be great. But since we don't have
> any real solutions there yet(still!), it seems like most gralloc
> implementations are likely to be fully knowing which heap they want at
> allocation time.
>
I think you already touched on my main issue with this, the dynamic
picking not supported. Well, like you said it doesn't really exist now
either. And this doesn't look to stop one from adding it as some ioctl
extensions..
Okay, looks like you posted an RFC, lets move the discussion over to
that thread.
Andrew
[0] https://android.googlesource.com/device/ti/beagle-x15/
> thanks
> -john
>
On Fri, Feb 15, 2019 at 12:52 PM Andrew F. Davis <[email protected]> wrote:
> On 2/15/19 1:58 PM, John Stultz wrote:
> > So yea, I don't think we should tie our hands in reworking the
> > interfaces, but it would be nice to avoid having subtle ABI changes
> > that don't have clear ways for userland to detect which interface
> > version its using.
> >
>
> Let me preference this by pointing out I've dealt with the same pain
> internally with our Android and soon to also in AOSP for the Beagle x15
> boards[0].. But my stance matches Christoph's in the other ION thread:
>
> https://lkml.org/lkml/2019/1/19/53
>
> The more freely we can make ABI changes here in staging the quicker we
> can get this out of staging where the ABI can be locked down.
> ION_IOC_VERSION should solve this anyway.
If there is real momentum to destaging and locking the ABI down,
that's great! I just want to avoid lots of little incompatible changes
that don't get us fully out of staging and cause tons of pain for
folks trying to make it work (because at that point, its easier for
folks to stick to their own out of tree solutions).
> > So my initial thought is we simply use a /dev/ion_heaps/ dir which has
> > a list of heap devicenodes. /dev/ion goes away.
> >
> > Currently ION_IOC_HEAP_QUERY returns:
> > char name[MAX_HEAP_NAME];
> > __u32 type;
> > __u32 heap_id;
> >
> > The names are discoverable via "ls /dev/ion_heaps/"
> >
> > The heap_id is really only useful as a handle, and after opening the
> > heap device, we'll have the fd to use.
> >
>
> So why have heap_id at all then?
>
Good question! I'm hoping we can yank them, though internally I
didn't quite get there with my first patchset. :)
> > But to match the use case you describe:
> > 1) ls /dev/ion_heaps/ for a list of heaps
>
> Yuk, dirent.h and friends :(
Yea, its not super fun, but its a standard interface, and the ugly
bits can be done once in the helper library.
> > Does that sound reasonable? And I don't really mean to dismiss the
> > dynamic picking of the best heap part, and having something like a
> > opaque constraints bitfield that each device and each heap export so
> > userland can match things up would be great. But since we don't have
> > any real solutions there yet(still!), it seems like most gralloc
> > implementations are likely to be fully knowing which heap they want at
> > allocation time.
> >
>
> I think you already touched on my main issue with this, the dynamic
> picking not supported. Well, like you said it doesn't really exist now
> either. And this doesn't look to stop one from adding it as some ioctl
> extensions..
>
> Okay, looks like you posted an RFC, lets move the discussion over to
> that thread.
Sounds good. I look forward to your input!
thanks
-john
On Fri, Feb 15, 2019 at 11:01:59AM -0800, John Stultz wrote:
> On Fri, Feb 15, 2019 at 2:51 AM Brian Starkey <[email protected]> wrote:
> >
> > Hi John,
> >
> > On Thu, Feb 14, 2019 at 09:38:29AM -0800, John Stultz wrote:
> > >
> > [snip]
> >
> > > Some thoughts, as this ABI break has the potential to be pretty painful.
> > >
> > > 1) Unfortunately, this ABI is exposed *through* libion via
> > > ion_alloc/ion_alloc_fd out to gralloc implementations. Which means it
> > > will have a wider impact to vendor userland code.
> >
> > I figured libion could fairly easily loop through all the set bits in
> > heap_mask and call the ioctl for each until it succeeds. That
> > preserves the old behaviour from the libion clients' perspective.
>
> Potentially, though that implicitly still caps the heaps to 32. So
> I'm not sure what the net benefit would be.
>
It's purely a transitionary compatibility measure. Users of the old
API inherit the old limitation - they shouldn't care about that.
Alongside it, we'd want to add new function(s) exposing whatever the
new API is.
Cheers,
-Brian
On 2/15/19 11:01 AM, John Stultz wrote:
> On Fri, Feb 15, 2019 at 2:51 AM Brian Starkey <[email protected]> wrote:
>>
>> Hi John,
>>
>> On Thu, Feb 14, 2019 at 09:38:29AM -0800, John Stultz wrote:
>>>
>> [snip]
>>
>>> Some thoughts, as this ABI break has the potential to be pretty painful.
>>>
>>> 1) Unfortunately, this ABI is exposed *through* libion via
>>> ion_alloc/ion_alloc_fd out to gralloc implementations. Which means it
>>> will have a wider impact to vendor userland code.
>>
>> I figured libion could fairly easily loop through all the set bits in
>> heap_mask and call the ioctl for each until it succeeds. That
>> preserves the old behaviour from the libion clients' perspective.
>
> Potentially, though that implicitly still caps the heaps to 32. So
> I'm not sure what the net benefit would be.
>
>
>>> 2) For patches that cause ABI breaks, it might be good to make it
>>> clear in the commit what the userland impact looks like in userspace,
>>> possibly with an example, so the poor folks who bisect down the change
>>> as breaking their system in a year or so have a clear example as to
>>> what they need to change in their code.
>>>
>>> 3) Also, its not clear how a given userland should distinguish between
>>> the different ABIs. We already have logic in libion to distinguish
>>> between pre-4.12 legacy and post-4.12 implementations (using implicit
>>> ion_free() behavior). I don't see any such check we can make with this
>>> code. Adding another ABI version may require we provide an actual
>>> interface version ioctl.
>>>
>>
>> A slightly fragile/ugly approach might be to attempt a small
>> allocation with a heap_mask of 0xffffffff. On an "old" implementation,
>> you'd expect that to succeed, whereas it would/could be made to fail
>> in the "new" one.
>
> Yea I think having a proper ION_IOC_VERSION is going to be necessary.
>
> I'm hoping to send out an ugly first stab at the kernel side for
> switching to per-heap devices (with a config for keeping /dev/ion for
> legacy compat), which I hope will address the core issue this patch
> does (moving away from heap masks to specifically requested heaps).
>
> thanks
> -john
>
Arnd/Greg said no to this last time I tried back in 2016
https://lore.kernel.org/lkml/[email protected]/T/#u