Re: [PATCH v2] staging: android: ion: Allocate from heap ID directly without mask

[John Stultz]

On Fri, Feb 15, 2019 at 11:22 AM Andrew F. Davis <afd@xxxxxx> wrote:
>
> On 2/15/19 1:01 PM, John Stultz wrote:
> > On Fri, Feb 15, 2019 at 2:51 AM Brian Starkey <Brian.Starkey@xxxxxxx> 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