Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH v2] staging: android: ion: Allocate from heap ID directly
 without mask
To:     John Stultz <john.stultz@linaro.org>,
        Brian Starkey <Brian.Starkey@arm.com>
CC:     Laura Abbott <labbott@redhat.com>,
        Sumit Semwal <sumit.semwal@linaro.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        =?UTF-8?Q?Arve_Hj=c3=b8nnev=c3=a5g?= <arve@android.com>,
        Christoph Hellwig <hch@infradead.org>,
        Liam Mark <lmark@codeaurora.org>,
        "devel@driverdev.osuosl.org" <devel@driverdev.osuosl.org>,
        lkml <linux-kernel@vger.kernel.org>,
        dri-devel <dri-devel@lists.freedesktop.org>, nd <nd@arm.com>
References: <20190128214408.25442-1-afd@ti.com>
 <CALAqxLXb53nsBJtb958sMzpr-fnXxqFE0s2B6YJzoGcswv50wg@mail.gmail.com>
 <20190215105057.jujgm4k77rhkvmo7@DESKTOP-E1NTVVP.localdomain>
 <CALAqxLU=L3Y7KJEo9c-Nu1rS3EquofGf1UGWBWqAO+Qke72Ekw@mail.gmail.com>
From:   "Andrew F. Davis" <afd@ti.com>
Message-ID: <0cc81357-034b-9c74-8172-2a7a26e77804@ti.com>
Date:   Fri, 15 Feb 2019 13:22:31 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.2.1
MIME-Version: 1.0
In-Reply-To: <CALAqxLU=L3Y7KJEo9c-Nu1rS3EquofGf1UGWBWqAO+Qke72Ekw@mail.gmail.com>
Content-Type: text/plain; charset="utf-8"
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 2/15/19 1:01 PM, John Stultz wrote:
> On Fri, Feb 15, 2019 at 2:51 AM Brian Starkey <Brian.Starkey@arm.com> 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
>