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

[Brian Starkey]

On Fri, Feb 15, 2019 at 11:01:59AM -0800, John Stultz wrote:
> On Fri, Feb 15, 2019 at 2:51 AM Brian Starkey  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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Brian Starkey]

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 0x. 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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/4] staging: android: ion: Restrict cache maintenance to dma mapped memory

[Brian Starkey]

> > +   struct ion_buffer *buffer = attachment->dmabuf->priv;
> > > > +
> > > > +   mutex_lock(>lock);
> > > > dma_unmap_sg(attachment->dev, table->sgl, table->nents, 
> > > > direction);
> > > > +   a->dma_mapped = false;
> > > > +   mutex_unlock(>lock);
> > > >  }
> > > >  
> > > >  static int ion_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma)
> > > > @@ -346,8 +360,9 @@ static int ion_dma_buf_begin_cpu_access(struct 
> > > > dma_buf *dmabuf,
> > > >  
> > > > mutex_lock(>lock);
> > > > list_for_each_entry(a, >attachments, list) {
> > > 
> > > When no devices are attached then buffer->attachments is empty and the
> > > below does not run, so if I understand this patch correctly then what
> > > you are protecting against is CPU access in the window after
> > > dma_buf_attach but before dma_buf_map.
> > > 
> > 
> > Yes
> > 
> > > This is the kind of thing that again makes me think a couple more
> > > ordering requirements on DMA-BUF ops are needed. DMA-BUFs do not require
> > > the backing memory to be allocated until map time, this is why the
> > > dma_address field would still be null as you note in the commit message.
> > > So why should the CPU be performing accesses on a buffer that is not
> > > actually backed yet?
> > > 
> > > I can think of two solutions:
> > > 
> > > 1) Only allow CPU access (mmap, kmap, {begin,end}_cpu_access) while at
> > > least one device is mapped.
> > > 
> > 
> > Would be quite limiting to clients.
> > 
> > > 2) Treat the CPU access request like the a device map request and
> > > trigger the allocation of backing memory just like if a device map had
> > > come in.
> > > 
> > 
> > Which is, as you mention pretty much what we have now (though the buffer 
> > is allocated even earlier).
> > 
> > > I know the current Ion heaps (and most other DMA-BUF exporters) all do
> > > the allocation up front so the memory is already there, but DMA-BUF was
> > > designed with late allocation in mind. I have a use-case I'm working on
> > > that finally exercises this DMA-BUF functionality and I would like to
> > > have it export through ION. This patch doesn't prevent that, but seems
> > > like it is endorsing the the idea that buffers always need to be backed,
> > > even before device attach/map is has occurred.
> > > 
> > 
> > I didn't interpret the DMA-buf contract as requiring the dma-map to be 
> > called in order for a backing store to be provided, I interpreted it as 
> > meaning there could be a backing store before the dma-map but at the 
> > dma-map call the final backing store configuration would be decided 
> > (perhaps involving migrating the memory to the final backing store).
> > I will let the dma-buf experts correct me on that.
> > 
> > Limiting userspace clients to not be able to access buffers until after 
> > they are dma-mapped seems unfortuntate to me, dma-mapping usually means a 
> > change of ownership of the memory from the CPU to the device. So generally 
> > while a buffer is dma mapped you have the device access it (though of 
> > course it is supported for CPU to access to the buffer while dma mapped) 
> > and then once the buffer is dma-unmapped the CPU can access it.  This is 
> > how the DMA APIs are frequently used, and the changes above make ION align 
> > more with the way the DMA APIs are used. Basically when the buffer is not 
> > dma-mapped the CPU doesn't need to do any CMOs to access the buffer (and 
> > ION ensures not CMOs are applied) but if the CPU does want to access the 
> > buffer while it is dma mapped then ION ensures that the appropriate CMOs 
> > are applied.
> > 
> > It seems like a legitimate uses case to me to allow clients to access the 
> > buffer before (and after) dma-mapping, example post processing of buffers.
> > 
> > 
> > > Either of the above two solutions would need to target the DMA-BUF
> > > framework,
> > > 
> > > Sumit,
> > > 
> > > Any comment?
> > > 
> 
> In a separate thread Sumit seems to have confirmed that it is not a 
> requirement for exporters to defer the allocation until first dma map.
> 
> https://lore.kernel.org/lkml/cao_48geypw0u6uwkkfgqjmmablcbm69od34qihsngewqz_a...@mail.gmail.c

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

[Brian Starkey]

On Thu, Jan 24, 2019 at 10:12:10AM -0600, Andrew F. Davis wrote:
> On 1/24/19 9:24 AM, Brian Starkey wrote:

[snip]

> > 
> > What do you think about renaming ion_allocation_data.unused to heap_id
> > and adding a flag instead? It's adding cruft to a staging API, but it
> > might soften the transition. The "old way" could get completely
> > removed just before destaging. Just a thought.
> > 
> 
> Sounds confusing, backwards compatibility in staging just doesn't seem
> like the right thing to do.
> 

Well, fair enough. It's going to cause a bit of pain - libion could
probably isolate most Android users. Laura patched that in AOSP last
time around, but I don't know how that went (or how strongly Google
feel about kernel ABI changes).

Thanks,
-Brian
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 13/14] staging: android: ion: Do not sync CPU cache on map/unmap

[Brian Starkey]

On Thu, Jan 24, 2019 at 10:04:46AM -0600, Andrew F. Davis wrote:
> On 1/23/19 11:11 AM, Brian Starkey wrote:

[snip]

> 
> I'm very new to all this, so any pointers to history in this area are
> appreciated.
> 

[snip]

> 
> > In case you didn't come across it already, the effort which seems to
> > have gained the most "air-time" recently is
> > https://github.com/cubanismo/allocator, which is still a userspace
> > module (perhaps some concepts from there could go into the kernel?),
> > but makes some attempts at generic constraint solving. It's also not
> > really moving anywhere at the moment.
> > 
> 
> Very interesting, I'm going to have to stare at this for a bit.

In which case, some reading material that might be of interest :-)

https://www.x.org/wiki/Events/XDC2016/Program/Unix_Device_Memory_Allocation.pdf
https://www.x.org/wiki/Events/XDC2017/jones_allocator.pdf
https://lists.freedesktop.org/archives/mesa-dev/2017-November/177632.html

-Brian

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Brian Starkey]

Hi Andrew,

On Wed, Jan 23, 2019 at 01:28:35PM -0600, Andrew F. Davis 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.

I checked our gralloc and it's not using the current "keep trying
until one succeeds" functionality, but it might be worth explicitly
calling out in the commit message that this will also disappear with
the new API. Maybe someone cares about that.

> 
> 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
> underling heap, so mask matching is not useful here. This also limits us

s/underling/underlying/

> 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.

What do you think about renaming ion_allocation_data.unused to heap_id
and adding a flag instead? It's adding cruft to a staging API, but it
might soften the transition. The "old way" could get completely
removed just before destaging. Just a thought.

> 
> [0] 2bb9f5034ec7 ("gpu: ion: Remove heapmask from client")
> 
> Signed-off-by: Andrew F. Davis 
> ---
> 
> 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.
> 
>  drivers/staging/android/ion/ion.c  | 21 ++---
>  drivers/staging/android/uapi/ion.h |  6 ++
>  2 files changed, 12 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/staging/android/ion/ion.c 
> b/drivers/staging/android/ion/ion.c
> index 92c2914239e3..06dd5bb10ecb 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,23 +396,22 @@ 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(>lock);
>   plist_for_each_entry(heap, >heaps, node) {
>   /* if the caller didn't specify this heap id */
> - if (!((1 << heap->id) & heap_id_mask))
> + if (heap->id != heap_id)
>   continue;
>   buffer = ion_buffer_create(heap, dev, len, flags);
>   if (!IS_ERR(buffer))

You can break unconditionally here now, though it might be neater to
refactor to:

 if (heap matches) {
buffer = alloc();
break;
 }

> @@ -541,7 +540,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
> +++ 

Re: [PATCH 13/14] staging: android: ion: Do not sync CPU cache on map/unmap

[Brian Starkey]

Hi Andrew,

On Wed, Jan 23, 2019 at 10:51:24AM -0600, Andrew F. Davis wrote:
> On 1/22/19 11:33 AM, Sumit Semwal wrote:
> > Hello everyone,
> > 
> > Sincere apologies for chiming in a bit late here, but was off due to
> > some health issues.
> > 
> 
> Hope you are feeling better friend :)
> 
> Looks like this email was a bit broken and you replied again, the
> responses are a little different in each email, so I'd like to respond
> to bits of both, I'll fix up the formatting.
> 
> > Also, adding Daniel Vetter to the mix, since he has been one of the
> > core guys who shaped up dma-buf as it is today.
> > 
> > On Tue, 22 Jan 2019 at 02:51, Andrew F. Davis  wrote:
> >>
> >> On 1/21/19 5:22 AM, Brian Starkey wrote:
> 
> [snip]
> 
> >>>
> >>> Actually I meant in the kernel, in exporters. I haven't seen anyone
> >>> using the API as it was intended (defer allocation until first map,
> >>> migrate between different attachments, etc.). Mostly, backing storage
> >>> seems to get allocated at the point of export, and device mappings are
> >>> often held persistently (e.g. the DRM prime code maps the buffer at
> >>> import time, and keeps it mapped: drm_gem_prime_import_dev).
> >>>
> >>
> > 
> > So I suppose some clarification on the 'intended use' part of dma-buf
> > about deferred allocation is due, so here it is: (Daniel, please feel
> > free to chime in with your opinion here)
> > 
> >  - dma-buf was of course designed as a framework to help intelligent
> > exporters to defer allocation until first map, and be able to migrate
> > backing storage if required etc. At the same time, it is not a
> > _requirement_ from any exporter, so exporters so far have just used it
> > as a convenient mechanism for zero-copy.
> > - ION is one of the few dma-buf exporters in kernel, which satisfies a
> > certain set of expectations from its users.
> > 
> 
> The issue here is that Ion is blocking the ability to late allocate, it
> expects its heaps to have the memory ready at allocation time. My point
> being if the DMA-BUFs intended design was to allow this then Ion should
> respect that and also allow the same from its heap exporters.
> 
> >> I haven't either, which is a shame as it allows for some really useful
> >> management strategies for shared memory resources. I'm working on one
> >> such case right now, maybe I'll get to be the first to upstream one :)
> >>
> > That will be a really good thing! Though perhaps we ought to think if
> > for what you're trying to do, is ION the right place, or should you
> > have a device-specific exporter, available to users via dma-buf apis?
> > 
> 
> I'm starting to question if Ion is the right place myself..
> 
> At a conceptual level I don't believe userspace should be picking the
> backing memory type. This is because the right type of backing memory
> for a task will change from system to system. The kernel should abstract
> away these hardware differences from userspace as much as it can to
> allow portable code.
> 
> For instance a device may need a contiguous buffer on one system but the
> same device on another may have some IOMMU. So which type of memory do
> we allocate? Same issue for cacheability and other properties.
> 
> What we need is a central allocator with full system knowledge to do the
> choosing for us. It seems many agree with the above and I take
> inspiration from your cenalloc patchset. The thing I'm not sure about is
> letting the device drivers set their constraints, because they also
> don't have the full system integration details. For cases where devices
> are behind an IOMMU it is easy enough for the device to know, but what
> about when we have external MMUs out on the bus for anyone to use (I'm
> guessing you remember TILER..).
> 
> I would propose the central allocator keep per-system knowledge (or
> fetch it from DT, or if this is considered policy then userspace) which
> it can use to directly check the attached devices and pick the right memory.
> 
> Anyway the central system allocator could handle 90% of cases I can
> think of, and this is where Ion comes back in, the other cases would
> still require the program to manually pick the right memory (maybe for
> performance reasons, etc.).
> 
> So my vision is to have Ion as the the main front-end for DMA-BUF
> allocations, and expose the central allocator through it (maybe as a
> default heap type that can be populated on a per-system basis), but also
> have other individual heap types exported for the edge cases where
> manual selecti

Re: [PATCH 4/4] staging: android: ion: Support for mapping with dma mapping attributes

[Brian Starkey]

Hi Liam,

On Fri, Jan 18, 2019 at 10:37:47AM -0800, Liam Mark wrote:
> Add support for configuring dma mapping attributes when mapping
> and unmapping memory through dma_buf_map_attachment and
> dma_buf_unmap_attachment.
> 
> For example this will allow ION clients to skip cache maintenance, by
> using DMA_ATTR_SKIP_CPU_SYNC, for buffers which are clean and haven't been
> accessed by the CPU.

How can a client know that the buffer won't be accessed by the CPU in
the future though?

I don't think we can push this decision to clients, because they are
lacking information about what else is going on with the buffer. It
needs to be done by the exporter, IMO.

Thanks,
-Brian

> 
> Signed-off-by: Liam Mark 
> ---
>  drivers/staging/android/ion/ion.c | 7 ---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/android/ion/ion.c 
> b/drivers/staging/android/ion/ion.c
> index 1fe633a7fdba..0aae845b20ba 100644
> --- a/drivers/staging/android/ion/ion.c
> +++ b/drivers/staging/android/ion/ion.c
> @@ -268,8 +268,8 @@ static struct sg_table *ion_map_dma_buf(struct 
> dma_buf_attachment *attachment,
>   table = a->table;
>  
>   mutex_lock(>lock);
> - if (!dma_map_sg(attachment->dev, table->sgl, table->nents,
> - direction)) {
> + if (!dma_map_sg_attrs(attachment->dev, table->sgl, table->nents,
> +   direction, attachment->dma_map_attrs)) {
>   mutex_unlock(>lock);
>   return ERR_PTR(-ENOMEM);
>   }
> @@ -287,7 +287,8 @@ static void ion_unmap_dma_buf(struct dma_buf_attachment 
> *attachment,
>   struct ion_buffer *buffer = attachment->dmabuf->priv;
>  
>   mutex_lock(>lock);
> - dma_unmap_sg(attachment->dev, table->sgl, table->nents, direction);
> + dma_unmap_sg_attrs(attachment->dev, table->sgl, table->nents, direction,
> +attachment->dma_map_attrs);
>   a->dma_mapped = false;
>   mutex_unlock(>lock);
>  }
> -- 
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, 
> a Linux Foundation Collaborative Project
> 
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 13/14] staging: android: ion: Do not sync CPU cache on map/unmap

[Brian Starkey]

Hi,

Sorry for being a bit sporadic on this. I was out travelling last week
with little time for email.

On Fri, Jan 18, 2019 at 11:16:31AM -0600, Andrew F. Davis wrote:
> On 1/17/19 7:11 PM, Liam Mark wrote:
> > On Thu, 17 Jan 2019, Andrew F. Davis wrote:
> > 
> >> On 1/16/19 4:54 PM, Liam Mark wrote:
> >>> On Wed, 16 Jan 2019, Andrew F. Davis wrote:
> >>>
> >>>> On 1/16/19 9:19 AM, Brian Starkey wrote:
> >>>>> Hi :-)
> >>>>>
> >>>>> On Tue, Jan 15, 2019 at 12:40:16PM -0600, Andrew F. Davis wrote:
> >>>>>> On 1/15/19 12:38 PM, Andrew F. Davis wrote:
> >>>>>>> On 1/15/19 11:45 AM, Liam Mark wrote:
> >>>>>>>> On Tue, 15 Jan 2019, Andrew F. Davis wrote:
> >>>>>>>>
> >>>>>>>>> On 1/14/19 11:13 AM, Liam Mark wrote:
> >>>>>>>>>> On Fri, 11 Jan 2019, Andrew F. Davis wrote:
> >>>>>>>>>>
> >>>>>>>>>>> Buffers may not be mapped from the CPU so skip cache maintenance 
> >>>>>>>>>>> here.
> >>>>>>>>>>> Accesses from the CPU to a cached heap should be bracketed with
> >>>>>>>>>>> {begin,end}_cpu_access calls so maintenance should not be needed 
> >>>>>>>>>>> anyway.
> >>>>>>>>>>>
> >>>>>>>>>>> Signed-off-by: Andrew F. Davis 
> >>>>>>>>>>> ---
> >>>>>>>>>>>  drivers/staging/android/ion/ion.c | 7 ---
> >>>>>>>>>>>  1 file changed, 4 insertions(+), 3 deletions(-)
> >>>>>>>>>>>
> >>>>>>>>>>> diff --git a/drivers/staging/android/ion/ion.c 
> >>>>>>>>>>> b/drivers/staging/android/ion/ion.c
> >>>>>>>>>>> index 14e48f6eb734..09cb5a8e2b09 100644
> >>>>>>>>>>> --- a/drivers/staging/android/ion/ion.c
> >>>>>>>>>>> +++ b/drivers/staging/android/ion/ion.c
> >>>>>>>>>>> @@ -261,8 +261,8 @@ static struct sg_table 
> >>>>>>>>>>> *ion_map_dma_buf(struct dma_buf_attachment *attachment,
> >>>>>>>>>>>  
> >>>>>>>>>>>   table = a->table;
> >>>>>>>>>>>  
> >>>>>>>>>>> - if (!dma_map_sg(attachment->dev, table->sgl, table->nents,
> >>>>>>>>>>> - direction))
> >>>>>>>>>>> + if (!dma_map_sg_attrs(attachment->dev, table->sgl, table->nents,
> >>>>>>>>>>> +   direction, DMA_ATTR_SKIP_CPU_SYNC))
> >>>>>>>>>>
> >>>>>>>>>> Unfortunately I don't think you can do this for a couple reasons.
> >>>>>>>>>> You can't rely on {begin,end}_cpu_access calls to do cache 
> >>>>>>>>>> maintenance.
> >>>>>>>>>> If the calls to {begin,end}_cpu_access were made before the call 
> >>>>>>>>>> to 
> >>>>>>>>>> dma_buf_attach then there won't have been a device attached so the 
> >>>>>>>>>> calls 
> >>>>>>>>>> to {begin,end}_cpu_access won't have done any cache maintenance.
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> That should be okay though, if you have no attachments (or all
> >>>>>>>>> attachments are IO-coherent) then there is no need for cache
> >>>>>>>>> maintenance. Unless you mean a sequence where a non-io-coherent 
> >>>>>>>>> device
> >>>>>>>>> is attached later after data has already been written. Does that
> >>>>>>>>> sequence need supporting? 
> >>>>>>>>
> >>>>>>>> Yes, but also I think there are cases where CPU access can happen 
> >>>>>>>> before 
> >>>>>>>> in Android, but I will focus on later for now.
> >>>>>>>>
> >>>>>>&g

Re: [PATCH 11/14] staging: android: ion: Allow heap name to be null

[Brian Starkey]

Hi Andrew,

On Fri, Jan 11, 2019 at 12:05:20PM -0600, Andrew F. Davis wrote:
> The heap name can be used for debugging but otherwise does not seem
> to be required and no other part of the code will fail if left NULL
> except here. We can make it required and check for it at some point,
> for now lets just prevent this from causing a NULL pointer exception.

I'm not so keen on this one. In the "new" API with heap querying, the
name string is the only way to identify the heap. I think Laura
mentioned at XDC2017 that it was expected that userspace should use
the strings to find the heap they want.

I'd actually be in favor of making the string a more strict UAPI than
allowing it to be empty (at least, if heap name strings is the API we
decide on for identifying heaps - which is another discussion).

Cheers,
-Brian

> 
> Signed-off-by: Andrew F. Davis 
> ---
>  drivers/staging/android/ion/ion.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/android/ion/ion.c 
> b/drivers/staging/android/ion/ion.c
> index bba5f682bc25..14e48f6eb734 100644
> --- a/drivers/staging/android/ion/ion.c
> +++ b/drivers/staging/android/ion/ion.c
> @@ -467,7 +467,7 @@ static int ion_query_heaps(struct ion_heap_query *query)
>   max_cnt = query->cnt;
>  
>   plist_for_each_entry(heap, >heaps, node) {
> - strncpy(hdata.name, heap->name, MAX_HEAP_NAME);
> + strncpy(hdata.name, heap->name ?: "(null)", MAX_HEAP_NAME);
>   hdata.name[sizeof(hdata.name) - 1] = '\0';
>   hdata.type = heap->type;
>   hdata.heap_id = heap->id;
> -- 
> 2.19.1
> 
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 13/14] staging: android: ion: Do not sync CPU cache on map/unmap

[Brian Starkey]

Hi :-)

On Tue, Jan 15, 2019 at 12:40:16PM -0600, Andrew F. Davis wrote:
> On 1/15/19 12:38 PM, Andrew F. Davis wrote:
> > On 1/15/19 11:45 AM, Liam Mark wrote:
> >> On Tue, 15 Jan 2019, Andrew F. Davis wrote:
> >>
> >>> On 1/14/19 11:13 AM, Liam Mark wrote:
>  On Fri, 11 Jan 2019, Andrew F. Davis wrote:
> 
> > Buffers may not be mapped from the CPU so skip cache maintenance here.
> > Accesses from the CPU to a cached heap should be bracketed with
> > {begin,end}_cpu_access calls so maintenance should not be needed anyway.
> >
> > Signed-off-by: Andrew F. Davis 
> > ---
> >  drivers/staging/android/ion/ion.c | 7 ---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/staging/android/ion/ion.c 
> > b/drivers/staging/android/ion/ion.c
> > index 14e48f6eb734..09cb5a8e2b09 100644
> > --- a/drivers/staging/android/ion/ion.c
> > +++ b/drivers/staging/android/ion/ion.c
> > @@ -261,8 +261,8 @@ static struct sg_table *ion_map_dma_buf(struct 
> > dma_buf_attachment *attachment,
> >  
> > table = a->table;
> >  
> > -   if (!dma_map_sg(attachment->dev, table->sgl, table->nents,
> > -   direction))
> > +   if (!dma_map_sg_attrs(attachment->dev, table->sgl, table->nents,
> > + direction, DMA_ATTR_SKIP_CPU_SYNC))
> 
>  Unfortunately I don't think you can do this for a couple reasons.
>  You can't rely on {begin,end}_cpu_access calls to do cache maintenance.
>  If the calls to {begin,end}_cpu_access were made before the call to 
>  dma_buf_attach then there won't have been a device attached so the calls 
>  to {begin,end}_cpu_access won't have done any cache maintenance.
> 
> >>>
> >>> That should be okay though, if you have no attachments (or all
> >>> attachments are IO-coherent) then there is no need for cache
> >>> maintenance. Unless you mean a sequence where a non-io-coherent device
> >>> is attached later after data has already been written. Does that
> >>> sequence need supporting? 
> >>
> >> Yes, but also I think there are cases where CPU access can happen before 
> >> in Android, but I will focus on later for now.
> >>
> >>> DMA-BUF doesn't have to allocate the backing
> >>> memory until map_dma_buf() time, and that should only happen after all
> >>> the devices have attached so it can know where to put the buffer. So we
> >>> shouldn't expect any CPU access to buffers before all the devices are
> >>> attached and mapped, right?
> >>>
> >>
> >> Here is an example where CPU access can happen later in Android.
> >>
> >> Camera device records video -> software post processing -> video device 
> >> (who does compression of raw data) and writes to a file
> >>
> >> In this example assume the buffer is cached and the devices are not 
> >> IO-coherent (quite common).
> >>
> > 
> > This is the start of the problem, having cached mappings of memory that
> > is also being accessed non-coherently is going to cause issues one way
> > or another. On top of the speculative cache fills that have to be
> > constantly fought back against with CMOs like below; some coherent
> > interconnects behave badly when you mix coherent and non-coherent access
> > (snoop filters get messed up).
> > 
> > The solution is to either always have the addresses marked non-coherent
> > (like device memory, no-map carveouts), or if you really want to use
> > regular system memory allocated at runtime, then all cached mappings of
> > it need to be dropped, even the kernel logical address (area as painful
> > as that would be).

Ouch :-( I wasn't aware about these potential interconnect issues. How
"real" is that? It seems that we aren't really hitting that today on
real devices.

> > 
> >> ION buffer is allocated.
> >>
> >> //Camera device records video
> >> dma_buf_attach
> >> dma_map_attachment (buffer needs to be cleaned)
> > 
> > Why does the buffer need to be cleaned here? I just got through reading
> > the thread linked by Laura in the other reply. I do like +Brian's
> 
> Actually +Brian this time :)
> 
> > suggestion of tracking if the buffer has had CPU access since the last
> > time and only flushing the cache if it has. As unmapped heaps never get
> > CPU mapped this would never be the case for unmapped heaps, it solves my
> > problem.
> > 
> >> [camera device writes to buffer]
> >> dma_buf_unmap_attachment (buffer needs to be invalidated)
> > 
> > It doesn't know there will be any further CPU access, it could get freed
> > after this for all we know, the invalidate can be saved until the CPU
> > requests access again.

We don't have any API to allow the invalidate to happen on CPU access
if all devices already detached. We need a struct device pointer to
give to the DMA API, otherwise on arm64 there'll be no invalidate.

I had a chat with a few people internally after the previous
discussion with Liam. One suggestion 

Re: [RFC PATCH v2] android: ion: How to properly clean caches for uncached allocations

[Brian Starkey]

On Wed, Nov 28, 2018 at 11:03:37PM -0800, Liam Mark wrote:
> On Wed, 28 Nov 2018, Brian Starkey wrote:
> > On Tue, Nov 27, 2018 at 10:46:07PM -0800, Liam Mark wrote:
> > > On Tue, 27 Nov 2018, Brian Starkey wrote:
> > > > On Mon, Nov 26, 2018 at 08:59:44PM -0800, Liam Mark wrote:
> > > > > On Tue, 20 Nov 2018, Brian Starkey wrote:

[snip]

> > > > 
> > > 
> > > Sounds like you are suggesting using carveouts to support uncached?
> > > 
> > 
> > No, I'm just saying that ion can't give out uncached _CPU_ mappings
> > for pages which are already mapped on the CPU as cached.

Probably this should have been: s/can't/shouldn't/

> > 
> 
> Okay then I guess I am not clear on where you would get this memory 
> which doesn't have a cached kernel mapping.
> It sounded like you wanted to define sections of memory in the DT as not 
> mapped in the kernel and then hand this memory to 
> dma_declare_coherent_memory (so that it can be managed) and then use an 
> ION heap as the allocator.  If the memory was defined this way it sounded 
> a lot like a carveout. But I guess you have some thoughts on how this 
> memory which doesn't have a kernel mapping can be made available for general
> use (for example available in buddy)?
> 
> Perhaps you were thinking of dynamically removing the kernel mappings 
> before handing it out as uncached, but this would have a general system 
> performance impact as this memory could come from anywhere so we would 
> quickly lose our 1GB block mappings (and likely many of our 2MB block 
> mappings as well).
> 

All I'm saying, with respect to non-cached memory and mappings, is
this:

I don't think ion should create non-cached CPU mappings of memory
which is mapped in the kernel as cached.

By extension, that means that in my opinion, the only way userspace
should be able to get a non-cached mapping, is by allocating from a
carveout.

However, I don't think this should be what we do in our complicated
media-heavy systems - carveouts are clearly impractical, as is
removing memory from the kernel map. What I think we should do, is
always do CPU access via cached mappings, for memory which is mapped
in the kernel as cached.

[snip]

> > 
> 
> I am not sure I am properly understanding as this is what my V2 patch 
> does, then when it gets an opportunity it allows the memory to be 
> re-mapped as uncached.

It's the remapping as uncached part which I'm not so keen on. It just
seems rather fragile to have mappings for the same memory with
different attributes around.

> 
> Or are you perhaps suggesting that if the memory is allocated from a 
> cached region then it always remains as cached, so only provide uncached 
> if it was allocated from an uncached region? If so I view all memory 
> available to the ION system heap for uncached allocations as having a 
> cached mapping (since it is all part of the kernel logical mappigns), so I 
> can't see how this would ever be able to support uncached allocations.

Yeah, that's exactly what I'm saying. The system heap should not
allow uncached allocations, and, memory allocated from the system heap
should always be mapped as cached for CPU accesses.

Non-cached allocations would only be allowed from carveouts (but as
discussed, I don't think carveouts are a practical solution for the
general case).

The summary of my proposal is that instead of focussing on getting
non-cached allocations, we should make cached allocations work better,
so that non-cached aliases of cached memory aren't required.

[snip]

> 
> Unfortunately if are only using cached mappings it isn't only the first 
> time you dma map the buffer you need to do cache maintenance, you need to 
> almost always do it because you don't know what CPU access happened (or 
> will happen) without a device.

I think you can always know if CPU _has_ accessed the buffer - in
begin_cpu_access, ion can set a flag, which it checks in map_dma_buf.
If that flag says it's been touched, then a cache clean is needed.

Of course you can't predict the future - there's no way to know if the
CPU _will_ access the buffer - which I think is what you're getting at
below.

> Explained more below.
> 
> > > But with this cached memory you get poor performance because you are 
> > > frequently doing cache mainteance uncessarily because there *could* be 
> > > CPU access.
> > > 
> > > The reason we want to use uncached allocations, with uncached mappings, 
> > > is 
> > > to avoid all this uncessary cache maintenance.
> > > 
> > 
> > OK I think this is the key - you don't actually care whether the
> > mappings are non-cached, you just don't want to pay a sync penalty if
> > the CPU never touched the buffer.
>

Re: [RFC PATCH v2] android: ion: How to properly clean caches for uncached allocations

[Brian Starkey]

Hi Liam,

On Tue, Nov 27, 2018 at 10:46:07PM -0800, Liam Mark wrote:
> On Tue, 27 Nov 2018, Brian Starkey wrote:
> 
> > Hi Liam,
> > 
> > On Mon, Nov 26, 2018 at 08:59:44PM -0800, Liam Mark wrote:
> > > On Tue, 20 Nov 2018, Brian Starkey wrote:
> > > 
> > > > Hi Liam,
> > > > 
> > > > I'm missing a bit of context here, but I did read the v1 thread.
> > > > Please accept my apologies if I'm re-treading trodden ground.
> > > > 
> > > > I do know we're chasing nebulous ion "problems" on our end, which
> > > > certainly seem to be related to what you're trying to fix here.
> > > > 
> > > > On Thu, Nov 01, 2018 at 03:15:06PM -0700, Liam Mark wrote:
> > > > >Based on the suggestions from Laura I created a first draft for a 
> > > > >change
> > > > >which will attempt to ensure that uncached mappings are only applied to
> > > > >ION memory who's cache lines have been cleaned.
> > > > >It does this by providing cached mappings (for uncached ION 
> > > > >allocations)
> > > > >until the ION buffer is dma mapped and successfully cleaned, then it
> > > > drops
> > > > >the userspace mappings and when pages are accessed they are faulted 
> > > > >back
> > > > >in and uncached mappings are created.
> > > > 
> > > > If I understand right, there's no way to portably clean the cache of
> > > > the kernel mapping before we map the pages into userspace. Is that
> > > > right?
> > > > 
> > > 
> > > Yes, it isn't always possible to clean the caches for an uncached mapping 
> > > because a device is required by the DMA APIs to do cache maintenance and 
> > > there isn't necessarily a device available (dma_buf_attach may not yet 
> > > have been called).
> > > 
> > > > Alternatively, can we just make ion refuse to give userspace a
> > > > non-cached mapping for pages which are mapped in the kernel as cached?
> > > 
> > > These pages will all be mapped as cached in the kernel for 64 bit (kernel 
> > > logical addresses) so you would always be refusing to create a non-cached 
> > > mapping.
> > 
> > And that might be the sane thing to do, no?
> > 
> > AFAIK there are still pages which aren't ever mapped as cached (e.g.
> > dma_declare_coherent_memory(), anything under /reserved-memory marked
> > as no-map). If those are exposed as an ion heap, then non-cached
> > mappings would be fine, and permitted.
> > 
> 
> Sounds like you are suggesting using carveouts to support uncached?
> 

No, I'm just saying that ion can't give out uncached _CPU_ mappings
for pages which are already mapped on the CPU as cached.

> We have many multimedia use cases which use very large amounts of uncached
> memory, uncached memory is used as a performance optimization because CPU
> access won't happen so it allows us to skip cache maintenance for all the
> dma map and dma unmap calls. To create carveouts large enough to support
> to support the worst case scenarios could result in very large carveouts.
> 
> Large carveouts like this would likely result in poor memory utilizations
> (since they are tuned for worst case) which would likely have significant
> performance impacts (more limited memory causes more frequent memory
> reclaim ect...).
> 
> Also estimating for worst case could be difficult since the amount of
> uncached memory could be app dependent.
> Unfortunately I don't think this would make for a very scalable solution.
> 

Sure, I understand the desire not to use carveouts. I'm not suggesting
carveouts are a viable alternative.

> > > 
> > > > Would userspace using the dma-buf sync ioctl around its accesses do
> > > > the "right thing" in that case?
> > > > 
> > > 
> > > I don't think so, the dma-buf sync ioctl require a device to peform cache 
> > > maintenance, but as mentioned above a device may not be available.
> > > 
> > 
> > If a device didn't attach yet, then no cache maintenance is
> > necessary. The only thing accessing the memory is the CPU, via a
> > cached mapping, which should work just fine. So far so good.
> > 
> 
> Unfortunately not.
> Scenario:
> - Client allocates uncached memory.
> - Client calls the DMA_BUF_IOCTL_SYNC IOCT IOCTL with flags
> DMA_BUF_SYNC_START (but this doesn't do any cache maintenance since there
> isn't any device)
> - Client mmap the memory (ION creates uncached mapping)

Re: [RFC PATCH v2] android: ion: How to properly clean caches for uncached allocations

[Brian Starkey]

Hi Liam,

On Mon, Nov 26, 2018 at 08:59:44PM -0800, Liam Mark wrote:
> On Tue, 20 Nov 2018, Brian Starkey wrote:
> 
> > Hi Liam,
> > 
> > I'm missing a bit of context here, but I did read the v1 thread.
> > Please accept my apologies if I'm re-treading trodden ground.
> > 
> > I do know we're chasing nebulous ion "problems" on our end, which
> > certainly seem to be related to what you're trying to fix here.
> > 
> > On Thu, Nov 01, 2018 at 03:15:06PM -0700, Liam Mark wrote:
> > >Based on the suggestions from Laura I created a first draft for a change
> > >which will attempt to ensure that uncached mappings are only applied to
> > >ION memory who's cache lines have been cleaned.
> > >It does this by providing cached mappings (for uncached ION allocations)
> > >until the ION buffer is dma mapped and successfully cleaned, then it
> > drops
> > >the userspace mappings and when pages are accessed they are faulted back
> > >in and uncached mappings are created.
> > 
> > If I understand right, there's no way to portably clean the cache of
> > the kernel mapping before we map the pages into userspace. Is that
> > right?
> > 
> 
> Yes, it isn't always possible to clean the caches for an uncached mapping 
> because a device is required by the DMA APIs to do cache maintenance and 
> there isn't necessarily a device available (dma_buf_attach may not yet 
> have been called).
> 
> > Alternatively, can we just make ion refuse to give userspace a
> > non-cached mapping for pages which are mapped in the kernel as cached?
> 
> These pages will all be mapped as cached in the kernel for 64 bit (kernel 
> logical addresses) so you would always be refusing to create a non-cached 
> mapping.

And that might be the sane thing to do, no?

AFAIK there are still pages which aren't ever mapped as cached (e.g.
dma_declare_coherent_memory(), anything under /reserved-memory marked
as no-map). If those are exposed as an ion heap, then non-cached
mappings would be fine, and permitted.

> 
> > Would userspace using the dma-buf sync ioctl around its accesses do
> > the "right thing" in that case?
> > 
> 
> I don't think so, the dma-buf sync ioctl require a device to peform cache 
> maintenance, but as mentioned above a device may not be available.
> 

If a device didn't attach yet, then no cache maintenance is
necessary. The only thing accessing the memory is the CPU, via a
cached mapping, which should work just fine. So far so good.

If there are already attachments, then ion_dma_buf_begin_cpu_access()
will sync for CPU access against all of the attached devices, and
again the CPU should see the right thing.

In the other direction, ion_dma_buf_end_cpu_access() will sync for
device access for all currently attached devices. If there's no
attached devices yet, then there's nothing to do until there is (only
thing accessing is CPU via a CPU-cached mapping).

When the first (or another) device attaches, then when it maps the
buffer, the map_dma_buf callback should do whatever sync-ing is needed
for that device.

I might be way off with my understanding of the various DMA APIs, but
this is how I think they're meant to work.

> > Given that as you pointed out, the kernel does still have a cached
> > mapping to these pages, trying to give the CPU a non-cached mapping of
> > those same pages while preserving consistency seems fraught. Wouldn't
> > it be better to make sure all CPU mappings are cached, and have CPU
> > clients use the dma_buf_{begin,end}_cpu_access() hooks to get
> > consistency where needed?
> > 
> 
> It is fraught, but unfortunately you can't rely on 
> dma_buf_{begin,end}_cpu_access() to do cache maintenance as these calls 
> require a device, and a device is not always available.

As above, if there's really no device, then no syncing is needed
because only the CPU is accessing the buffer, and only ever via cached
mappings.

> 
> > >
> > >This change has the following potential disadvantages:
> > >- It assumes that userpace clients won't attempt to access the buffer
> > >while it is being mapped as we are removing the userpspace mappings at
> > >this point (though it is okay for them to have it mapped)
> > >- It assumes that kernel clients won't hold a kernel mapping to the
> > buffer
> > >(ie dma_buf_kmap) while it is being dma-mapped. What should we do if
> > there
> > >is a kernel mapping at the time of dma mapping, fail the mapping, warn?
> > >- There may be a performance penalty as a result of having to fault in
> > the
> > >pages after removing the userspace mappings.
> > 
> > I wonder

Re: [RFC PATCH v2] android: ion: How to properly clean caches for uncached allocations

[Brian Starkey]

Hi Liam,

I'm missing a bit of context here, but I did read the v1 thread.
Please accept my apologies if I'm re-treading trodden ground.

I do know we're chasing nebulous ion "problems" on our end, which
certainly seem to be related to what you're trying to fix here.

On Thu, Nov 01, 2018 at 03:15:06PM -0700, Liam Mark wrote:
>Based on the suggestions from Laura I created a first draft for a change
>which will attempt to ensure that uncached mappings are only applied to
>ION memory who's cache lines have been cleaned.
>It does this by providing cached mappings (for uncached ION allocations)
>until the ION buffer is dma mapped and successfully cleaned, then it drops
>the userspace mappings and when pages are accessed they are faulted back
>in and uncached mappings are created.

If I understand right, there's no way to portably clean the cache of
the kernel mapping before we map the pages into userspace. Is that
right?

Alternatively, can we just make ion refuse to give userspace a
non-cached mapping for pages which are mapped in the kernel as cached?
Would userspace using the dma-buf sync ioctl around its accesses do
the "right thing" in that case?

Given that as you pointed out, the kernel does still have a cached
mapping to these pages, trying to give the CPU a non-cached mapping of
those same pages while preserving consistency seems fraught. Wouldn't
it be better to make sure all CPU mappings are cached, and have CPU
clients use the dma_buf_{begin,end}_cpu_access() hooks to get
consistency where needed?

>
>This change has the following potential disadvantages:
>- It assumes that userpace clients won't attempt to access the buffer
>while it is being mapped as we are removing the userpspace mappings at
>this point (though it is okay for them to have it mapped)
>- It assumes that kernel clients won't hold a kernel mapping to the buffer
>(ie dma_buf_kmap) while it is being dma-mapped. What should we do if there
>is a kernel mapping at the time of dma mapping, fail the mapping, warn?
>- There may be a performance penalty as a result of having to fault in the
>pages after removing the userspace mappings.

I wonder if the dma-buf sync ioctl might provide a way for userspace
to opt-in to when the zap/fault happens. Zap on (DMA_BUF_SYNC_WRITE |
DMA_BUF_SYNC_WRITE_END) and fault on (DMA_BUF_SYNC_READ |
DMA_BUF_SYNC_START)

>
>It passes basic testing involving reading writing and reading from
>uncached system heap allocations before and after dma mapping.
>
>Please let me know if this is heading in the right direction and if there
>are any concerns.
>
>Signed-off-by: Liam Mark 
>---
> drivers/staging/android/ion/ion.c | 146 +-
> drivers/staging/android/ion/ion.h |   9 +++
> 2 files changed, 152 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/staging/android/ion/ion.c 
>b/drivers/staging/android/ion/ion.c
>index 99073325b0c0..3dc0f5a265bf 100644
>--- a/drivers/staging/android/ion/ion.c
>+++ b/drivers/staging/android/ion/ion.c
>@@ -96,6 +96,7 @@ static struct ion_buffer *ion_buffer_create(struct ion_heap 
>*heap,
>   }
>
>   INIT_LIST_HEAD(>attachments);
>+  INIT_LIST_HEAD(>vmas);
>   mutex_init(>lock);
>   mutex_lock(>buffer_lock);
>   ion_buffer_add(dev, buffer);
>@@ -117,6 +118,7 @@ void ion_buffer_destroy(struct ion_buffer *buffer)
>   buffer->heap->ops->unmap_kernel(buffer->heap, buffer);
>   }
>   buffer->heap->ops->free(buffer);
>+  vfree(buffer->pages);
>   kfree(buffer);
> }
>
>@@ -245,11 +247,29 @@ static void ion_dma_buf_detatch(struct dma_buf *dmabuf,
>   kfree(a);
> }
>
>+static bool ion_buffer_uncached_clean(struct ion_buffer *buffer)
>+{
>+  return buffer->uncached_clean;
>+}

nit: The function name sounds like a verb to me - as in "calling this
will clean the buffer". I feel ion_buffer_is_uncached_clean() would
read better.

Thanks,
-Brian

>+
>+/* expect buffer->lock to be already taken */
>+static void ion_buffer_zap_mappings(struct ion_buffer *buffer)
>+{
>+  struct ion_vma_list *vma_list;
>+
>+  list_for_each_entry(vma_list, >vmas, list) {
>+  struct vm_area_struct *vma = vma_list->vma;
>+
>+  zap_page_range(vma, vma->vm_start, vma->vm_end - vma->vm_start);
>+  }
>+}
>+
> static struct sg_table *ion_map_dma_buf(struct dma_buf_attachment *attachment,
>   enum dma_data_direction direction)
> {
>   struct ion_dma_buf_attachment *a = attachment->priv;
>   struct sg_table *table;
>+  struct ion_buffer *buffer = attachment->dmabuf->priv;
>
>   table = a->table;
>
>@@ -257,6 +277,19 @@ static struct sg_table *ion_map_dma_buf(struct 
>dma_buf_attachment *attachment,
>   direction))
>   return ERR_PTR(-ENOMEM);
>
>+  if (!ion_buffer_cached(buffer)) {
>+  mutex_lock(>lock);
>+  if (!ion_buffer_uncached_clean(buffer)) {
>+  

Re: [RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging

[Brian Starkey]

On Sun, Mar 12, 2017 at 02:34:14PM +0100, Benjamin Gaignard wrote:

2017-03-09 18:38 GMT+01:00 Laura Abbott :

On 03/09/2017 02:00 AM, Benjamin Gaignard wrote:

2017-03-06 17:04 GMT+01:00 Daniel Vetter :

On Mon, Mar 06, 2017 at 11:58:05AM +0100, Mark Brown wrote:

On Mon, Mar 06, 2017 at 11:40:41AM +0100, Daniel Vetter wrote:


No one gave a thing about android in upstream, so Greg KH just dumped it
all into staging/android/. We've discussed ION a bunch of times, recorded
anything we'd like to fix in staging/android/TODO, and Laura's patch
series here addresses a big chunk of that.



This is pretty much the same approach we (gpu folks) used to de-stage the
syncpt stuff.


Well, there's also the fact that quite a few people have issues with the
design (like Laurent).  It seems like a lot of them have either got more
comfortable with it over time, or at least not managed to come up with
any better ideas in the meantime.


See the TODO, it has everything a really big group (look at the patch for
the full Cc: list) figured needs to be improved at LPC 2015. We don't just
merge stuff because merging stuff is fun :-)

Laurent was even in that group ...
-Daniel


For me those patches are going in the right direction.

I still have few questions:
- since alignment management has been remove from ion-core, should it
be also removed from ioctl structure ?


Yes, I think I'm going to go with the suggestion to fixup the ABI
so we don't need the compat layer and as part of that I'm also
dropping the align argument.


- can you we ride off ion_handle (at least in userland) and only
export a dma-buf descriptor ?


Yes, I think this is the right direction given we're breaking
everything anyway. I was debating trying to keep the two but
moving to only dma bufs is probably cleaner. The only reason
I could see for keeping the handles is running out of file
descriptors for dma-bufs but that seems unlikely.


In the future how can we add new heaps ?
Some platforms have very specific memory allocation
requirements (just have a look in the number of gem custom allocator in drm)
Do you plan to add heap type/mask for each ?


Yes, that was my thinking.


My concern is about the policy to adding heaps, will you accept
"customs" heap per
platforms ? per devices ? or only generic ones ?
If you are too strict, we will have lot of out-of-tree heaps and if
you accept of of them
it will be a nightmare to maintain



Are you concerned about actual heaps (e.g. a carveout at 0x8000 vs
a carveout at 0x6000) or heap types?

For heap types, I think the policy can be strict - if it's generally
useful then it should live in-tree in ion. Otherwise, it would be
out-of-tree. I'd expect most "custom" heaps to be parameterisable to
the point of being generally useful.

For actual heap instances, I would expect them to be communicated via
reserved-memory regions or something similar, and so the maintenance
burden is pretty low.

The existing query ioctl can allow heap IDs to get assigned
dynamically at runtime, so there's no need to reserve "bit 6" for
"CUSTOM_ACME_HEAP_1"


Another point is how can we put secure rules (like selinux policy) on
heaps since all the allocations
go to the same device (/dev/ion) ? For example, until now, in Android
we have to give the same
access rights to all the process that use ION.
It will become problem when we will add secure heaps because we won't
be able to distinguish secure
processes to standard ones or set specific policy per heaps.
Maybe I'm wrong here but I have never see selinux policy checking an
ioctl field but if that
exist it could be a solution.



I might be thinking of a different type of "secure", but...

Should the security of secure heaps be enforced by OS-level
permissions? I don't know about other architectures, but at least on
arm/arm64 this is enforced in hardware; it doesn't matter who has
access to the ion heap, because only secure devices (or the CPU
running a secure process) is physically able to access the memory
backing the buffer.

In fact, in the use-cases I know of, the process asking for the ion
allocation is not a secure process, and so we wouldn't *want* to
restrict the secure heap to be allocated from only by secure
processes.

-Brian





Benjamin



Thanks,
Laura


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging

[Brian Starkey]

On Fri, Mar 10, 2017 at 11:46:42AM +, Robin Murphy wrote:

On 10/03/17 10:31, Brian Starkey wrote:

Hi,

On Thu, Mar 09, 2017 at 09:38:49AM -0800, Laura Abbott wrote:

On 03/09/2017 02:00 AM, Benjamin Gaignard wrote:


[snip]



For me those patches are going in the right direction.

I still have few questions:
- since alignment management has been remove from ion-core, should it
be also removed from ioctl structure ?


Yes, I think I'm going to go with the suggestion to fixup the ABI
so we don't need the compat layer and as part of that I'm also
dropping the align argument.



Is the only motivation for removing the alignment parameter that
no-one got around to using it for something useful yet?
The original comment was true - different devices do have different
alignment requirements.

Better alignment can help SMMUs use larger blocks when mapping,
reducing TLB pressure and the chance of a page table walk causing
display underruns.


For that use-case, though, alignment alone doesn't necessarily help -
you need the whole allocation granularity to match your block size (i.e.
given a 1MB block size, asking for 17KB and getting back 17KB starting
at a 1MB boundary doesn't help much - that whole 1MB needs to be
allocated and everyone needs to know it to ensure that the whole lot can
be mapped safely). Now, whether it's down to the callers or the heap
implementations to decide and enforce that granularity is another
question, but provided allocations are at least naturally aligned to
whatever the granularity is (which is a reasonable assumption to bake
in) then it's all good.

Robin.


Agreed, alignment alone isn't enough. But lets assume that an app
knows what a "good" granularity is, and always asks for allocation
sizes which are suitably rounded to allow blocks to be used. Currently
it looks like a "standard" ION_HEAP_TYPE_CARVEOUT heap would give me
back just a PAGE_SIZE aligned buffer. So even *if* the caller knows
its desired block size, there's no way for it to get guaranteed better
alignment, which wouldn't be a bad feature to have.

Anyway as Daniel and Rob say, if the interface is designed properly
this kind of extension would be possible later, or you can have a
special heap with a larger granule.

I suppose it makes sense to remove it while there's no-one actually
implementing it, in case an alternate method proves more usable.

-Brian





-Brian

___
linux-arm-kernel mailing list
linux-arm-ker...@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel



___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging

[Brian Starkey]

Hi,

On Thu, Mar 09, 2017 at 09:38:49AM -0800, Laura Abbott wrote:

On 03/09/2017 02:00 AM, Benjamin Gaignard wrote:


[snip]



For me those patches are going in the right direction.

I still have few questions:
- since alignment management has been remove from ion-core, should it
be also removed from ioctl structure ?


Yes, I think I'm going to go with the suggestion to fixup the ABI
so we don't need the compat layer and as part of that I'm also
dropping the align argument.



Is the only motivation for removing the alignment parameter that
no-one got around to using it for something useful yet?
The original comment was true - different devices do have different
alignment requirements.

Better alignment can help SMMUs use larger blocks when mapping,
reducing TLB pressure and the chance of a page table walk causing
display underruns.

-Brian
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCHv2 1/4] staging: android: ion: Drop heap type masks

[Brian Starkey]

On Tue, Sep 06, 2016 at 03:16:52PM -0700, Laura Abbott wrote:

On 09/05/2016 04:20 AM, Brian Starkey wrote:

Hi,

On Fri, Sep 02, 2016 at 12:36:25PM -0700, Laura Abbott wrote:

On 09/02/2016 06:41 AM, Brian Starkey wrote:

Hi Laura,

On Thu, Sep 01, 2016 at 03:40:41PM -0700, Laura Abbott wrote:


There is no advantage to having heap types be a mask. The ion client has
long since dropped the mask. Drop the notion of heap type masks as well.



I know this is the same patch you sent last time, so sorry for not
picking this up then - but I'm curious what "The" ion client is here?



ion_client_create used to take a mask to indicate what heap types it
could allocate from. This hasn't been the case since 2bb9f5034ec7
("gpu: ion: Remove heapmask from client"). "The ion client" probably
should have been "struct ion_client"


Ah I see, the in-kernel ion_client. Sorry, I completely forgot that
even existed (because it's totally useless - how is a driver meant to
find the global ion_device?)




Our ion client(s) certainly still use these masks, and it's still
used as a mask within ion itself - even if the relationship between a
mask and a heap type has been somewhat lost.


Where is it used in Ion? I don't see it in tree unless I missed something
and I'm not eager to keep this around for out of tree code. What's the
actual use for this?


You're certainly right that these heap-ID-to-allocation-mask macros
are unused in the kernel, but I don't really see the reason for
removing them - they are convenient (for now).

Example: I'm using the dummy ion driver, and I want to allocate from
the SYSTEM_CONTIG heap - the ION_HEAP_SYSTEM_CONTIG_MASK gives me the
exact mask I need for that.

It seems your opinion is that heap-IDs are already, and should be,
completely decoupled from their type. That sounds like a good idea to
me, but it's not true (yet) - again check out the dummy driver.



Good point, I need to clean up the dummy driver to stop using heap
types as the id ;)

I get that it's convenient but it's a bad practice to conflate the
namespaces.


At the moment, heap-IDs are assigned by ion drivers in any way they
see fit. For as long as that stays the case there's always going to
be heap-masks hard-coded in UAPI kernel headers (in-tree or not), so
removing these particular masks seems a bit fruitless.



It's not fruitless, the concept of type as mask makes no sense. They
are two different name spaces and I've found Ion users have a hard
time keeping them separate and pass in the heap type mask when using
non dummy


You can add me to that group :-)




I'd rather see driver-assigned heap-IDs disappear completely, and have
them assigned by ion core from an idr or something. At that point
these macros really *are* meaningless, and I'd be totally fine with
removing them (and userspace won't be able to depend on hard-coded
allocation masks any more - it will have to use the query ioctl,
which I assume is the whole point?).



Ideally yes we'd be able to get rid of the hard coded device IDs.
I consider the query ioctl a stepping stone to that, depending on
how enthusiastic people are about Ion.


IMO it's not the right time to remove these macros, because they still
have meaning and usefulness.



I still think they should be deleted to avoid namespace polution.



If you nuke type-as-ID in dummy, and put a clear comment on the
ion_heap_type enum stating that heap-type and heap-ID are strictly
different, then I'm happy.
I think without that there'll still be confusion.

Thanks!
Brian


Cheers,
Brian





Thanks,
Brian


Signed-off-by: Laura Abbott <labb...@redhat.com>
---
drivers/staging/android/uapi/ion.h | 6 --
1 file changed, 6 deletions(-)

diff --git a/drivers/staging/android/uapi/ion.h 
b/drivers/staging/android/uapi/ion.h
index 0a8e40f..a9c4e8b 100644
--- a/drivers/staging/android/uapi/ion.h
+++ b/drivers/staging/android/uapi/ion.h
@@ -44,14 +44,8 @@ enum ion_heap_type {
 * must be last so device specific heaps always
 * are at the end of this enum
 */
-ION_NUM_HEAPS = 16,
};

-#define ION_HEAP_SYSTEM_MASK(1 << ION_HEAP_TYPE_SYSTEM)
-#define ION_HEAP_SYSTEM_CONTIG_MASK(1 << ION_HEAP_TYPE_SYSTEM_CONTIG)
-#define ION_HEAP_CARVEOUT_MASK(1 << ION_HEAP_TYPE_CARVEOUT)
-#define ION_HEAP_TYPE_DMA_MASK(1 << ION_HEAP_TYPE_DMA)
-
#define ION_NUM_HEAP_IDS(sizeof(unsigned int) * 8)

/**
--
2.7.4






___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCHv2 1/4] staging: android: ion: Drop heap type masks

[Brian Starkey]

Hi,

On Fri, Sep 02, 2016 at 12:36:25PM -0700, Laura Abbott wrote:

On 09/02/2016 06:41 AM, Brian Starkey wrote:

Hi Laura,

On Thu, Sep 01, 2016 at 03:40:41PM -0700, Laura Abbott wrote:


There is no advantage to having heap types be a mask. The ion client has
long since dropped the mask. Drop the notion of heap type masks as well.



I know this is the same patch you sent last time, so sorry for not
picking this up then - but I'm curious what "The" ion client is here?



ion_client_create used to take a mask to indicate what heap types it
could allocate from. This hasn't been the case since 2bb9f5034ec7
("gpu: ion: Remove heapmask from client"). "The ion client" probably
should have been "struct ion_client"


Ah I see, the in-kernel ion_client. Sorry, I completely forgot that
even existed (because it's totally useless - how is a driver meant to
find the global ion_device?)




Our ion client(s) certainly still use these masks, and it's still
used as a mask within ion itself - even if the relationship between a
mask and a heap type has been somewhat lost.


Where is it used in Ion? I don't see it in tree unless I missed something
and I'm not eager to keep this around for out of tree code. What's the
actual use for this?


You're certainly right that these heap-ID-to-allocation-mask macros
are unused in the kernel, but I don't really see the reason for
removing them - they are convenient (for now).

Example: I'm using the dummy ion driver, and I want to allocate from
the SYSTEM_CONTIG heap - the ION_HEAP_SYSTEM_CONTIG_MASK gives me the
exact mask I need for that.

It seems your opinion is that heap-IDs are already, and should be,
completely decoupled from their type. That sounds like a good idea to
me, but it's not true (yet) - again check out the dummy driver.

At the moment, heap-IDs are assigned by ion drivers in any way they
see fit. For as long as that stays the case there's always going to
be heap-masks hard-coded in UAPI kernel headers (in-tree or not), so
removing these particular masks seems a bit fruitless.

I'd rather see driver-assigned heap-IDs disappear completely, and have
them assigned by ion core from an idr or something. At that point
these macros really *are* meaningless, and I'd be totally fine with
removing them (and userspace won't be able to depend on hard-coded
allocation masks any more - it will have to use the query ioctl,
which I assume is the whole point?).

IMO it's not the right time to remove these macros, because they still
have meaning and usefulness.

Cheers,
Brian





Thanks,
Brian


Signed-off-by: Laura Abbott <labb...@redhat.com>
---
drivers/staging/android/uapi/ion.h | 6 --
1 file changed, 6 deletions(-)

diff --git a/drivers/staging/android/uapi/ion.h 
b/drivers/staging/android/uapi/ion.h
index 0a8e40f..a9c4e8b 100644
--- a/drivers/staging/android/uapi/ion.h
+++ b/drivers/staging/android/uapi/ion.h
@@ -44,14 +44,8 @@ enum ion_heap_type {
  * must be last so device specific heaps always
  * are at the end of this enum
  */
-ION_NUM_HEAPS = 16,
};

-#define ION_HEAP_SYSTEM_MASK(1 << ION_HEAP_TYPE_SYSTEM)
-#define ION_HEAP_SYSTEM_CONTIG_MASK(1 << ION_HEAP_TYPE_SYSTEM_CONTIG)
-#define ION_HEAP_CARVEOUT_MASK(1 << ION_HEAP_TYPE_CARVEOUT)
-#define ION_HEAP_TYPE_DMA_MASK(1 << ION_HEAP_TYPE_DMA)
-
#define ION_NUM_HEAP_IDS(sizeof(unsigned int) * 8)

/**
--
2.7.4




___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCHv2 1/4] staging: android: ion: Drop heap type masks

[Brian Starkey]

Hi Laura,

On Thu, Sep 01, 2016 at 03:40:41PM -0700, Laura Abbott wrote:


There is no advantage to having heap types be a mask. The ion client has
long since dropped the mask. Drop the notion of heap type masks as well.



I know this is the same patch you sent last time, so sorry for not
picking this up then - but I'm curious what "The" ion client is here?

Our ion client(s) certainly still use these masks, and it's still
used as a mask within ion itself - even if the relationship between a
mask and a heap type has been somewhat lost.

Thanks,
Brian


Signed-off-by: Laura Abbott 
---
drivers/staging/android/uapi/ion.h | 6 --
1 file changed, 6 deletions(-)

diff --git a/drivers/staging/android/uapi/ion.h 
b/drivers/staging/android/uapi/ion.h
index 0a8e40f..a9c4e8b 100644
--- a/drivers/staging/android/uapi/ion.h
+++ b/drivers/staging/android/uapi/ion.h
@@ -44,14 +44,8 @@ enum ion_heap_type {
   * must be last so device specific heaps always
   * are at the end of this enum
   */
-   ION_NUM_HEAPS = 16,
};

-#define ION_HEAP_SYSTEM_MASK   (1 << ION_HEAP_TYPE_SYSTEM)
-#define ION_HEAP_SYSTEM_CONTIG_MASK(1 << ION_HEAP_TYPE_SYSTEM_CONTIG)
-#define ION_HEAP_CARVEOUT_MASK (1 << ION_HEAP_TYPE_CARVEOUT)
-#define ION_HEAP_TYPE_DMA_MASK (1 << ION_HEAP_TYPE_DMA)
-
#define ION_NUM_HEAP_IDS(sizeof(unsigned int) * 8)

/**
--
2.7.4


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [RFC][PATCH 6/6] staging: android: ion: Introduce new ioctls for dynamic heaps

[Brian Starkey]

Hi,

I'm finding "usage_id" a bit confusing - there's not a very clear
distinction between usage_id and heap ID.

For instance, ION_IOC_USAGE_CNT claims to return the number of usage
IDs, but seems to return the number of heaps (i.e. number heap IDs, some
of which might be usage_ids).

Similarly, alloc2 claims to allocate by usage_id, but will just as
happily allocate by heap ID.

Maybe this should just be described as a single heap ID namespace, where
some IDs represent groups of heap IDs.

I've a few inline comments below.

-Brian

On Mon, Jun 06, 2016 at 11:23:33AM -0700, Laura Abbott wrote:

From: Laura Abbott 


The Ion ABI for heaps is limiting to work with for more complex systems.
Heaps have to be registered at boot time with known ids available to
userspace. This becomes a tight ABI which is prone to breakage.

Introduce a new mechanism for registering heap ids dynamically. A base
set of heap ids are registered at boot time but there is no knowledge
of fallbacks. Fallback information can be remapped and changed
dynamically. Information about available heaps can also be queried with
an ioctl to avoid the need to have heap ids be an ABI with userspace.

Signed-off-by: Laura Abbott 
---
drivers/staging/android/ion/ion-ioctl.c | 109 +--
drivers/staging/android/ion/ion.c   | 184 ++--
drivers/staging/android/ion/ion_priv.h  |  15 +++
drivers/staging/android/uapi/ion.h  | 135 +++
4 files changed, 426 insertions(+), 17 deletions(-)

diff --git a/drivers/staging/android/ion/ion-ioctl.c 
b/drivers/staging/android/ion/ion-ioctl.c
index 45b89e8..169cad8 100644
--- a/drivers/staging/android/ion/ion-ioctl.c
+++ b/drivers/staging/android/ion/ion-ioctl.c
@@ -22,6 +22,49 @@
#include "ion_priv.h"
#include "compat_ion.h"

+union ion_ioctl_arg {
+   struct ion_fd_data fd;
+   struct ion_allocation_data allocation;
+   struct ion_handle_data handle;
+   struct ion_custom_data custom;
+   struct ion_abi_version abi_version;
+   struct ion_new_alloc_data allocation2;
+   struct ion_usage_id_map id_map;
+   struct ion_usage_cnt usage_cnt;
+   struct ion_heap_query query;
+};
+
+static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
+{
+   int ret = 0;
+
+   switch (cmd) {
+   case ION_IOC_ABI_VERSION:
+   ret =  arg->abi_version.reserved != 0;
+   break;
+   case ION_IOC_ALLOC2:
+   ret = arg->allocation2.reserved0 != 0;
+   ret |= arg->allocation2.reserved1 != 0;
+   ret |= arg->allocation2.reserved2 != 0;
+   break;
+   case ION_IOC_ID_MAP:
+   ret = arg->id_map.reserved0 != 0;
+   ret |= arg->id_map.reserved1 != 0;
+   break;
+   case ION_IOC_USAGE_CNT:
+   ret = arg->usage_cnt.reserved != 0;
+   break;
+   case ION_IOC_HEAP_QUERY:
+   ret = arg->query.reserved0 != 0;
+   ret |= arg->query.reserved1 != 0;
+   ret |= arg->query.reserved2 != 0;
+   break;
+   default:
+   break;
+   }
+   return ret ? -EINVAL : 0;
+}
+
/* fix up the cases where the ioctl direction bits are incorrect */
static unsigned int ion_ioctl_dir(unsigned int cmd)
{
@@ -42,14 +85,7 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned 
long arg)
struct ion_handle *cleanup_handle = NULL;
int ret = 0;
unsigned int dir;
-
-   union {
-   struct ion_fd_data fd;
-   struct ion_allocation_data allocation;
-   struct ion_handle_data handle;
-   struct ion_custom_data custom;
-   struct ion_abi_version abi_version;
-   } data;
+   union ion_ioctl_arg data;

dir = ion_ioctl_dir(cmd);

@@ -60,7 +96,12 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned 
long arg)
if (copy_from_user(, (void __user *)arg, _IOC_SIZE(cmd)))
return -EFAULT;

+   ret = validate_ioctl_arg(cmd, );
+   if (ret)
+   return ret;
+
switch (cmd) {
+   /* Old ioctl */
case ION_IOC_ALLOC:
{
struct ion_handle *handle;
@@ -77,6 +118,7 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned 
long arg)
cleanup_handle = handle;
break;
}
+   /* Old ioctl */
case ION_IOC_FREE:
{
struct ion_handle *handle;
@@ -92,6 +134,7 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned 
long arg)
mutex_unlock(>lock);
break;
}
+   /* Old ioctl */
case ION_IOC_SHARE:
case ION_IOC_MAP:
{
@@ -106,6 +149,7 @@ long ion_ioctl(struct file *filp, unsigned int cmd, 
unsigned long arg)
ret = data.fd.fd;
   

Re: [RFC][PATCH 0/6] ion: improved ABI

[Brian Starkey]

Hi Laura,

On Mon, Jun 06, 2016 at 11:23:27AM -0700, Laura Abbott wrote:

The ABI for Ion's ioctl interface are a pain to work with. The heap IDs
are a 32-bit non-discoverable namespace that form part of the ABI. There's
no way to determine what ABI version is in use which leads to problems
if the ABI changes or needs to be updated.

This series is a first approach to give a better ABI for Ion. This includes:

- Following the advice in botching-up-ioctls.txt
- Ioctl for ABI version
- Dynamic assignment of heap ids
- queryable heap ids
- Runtime mapping of heap ids, including fallbacks. This avoids the need to
 encode the fallbacks as an ABI.

I'm most interested in feedback if this ABI is actually an improvement and
usable. The heap id map/query interface seems error prone but I didn't have
a cleaner solution. There aren't any kernel APIs for the new features as the
focus was on a userspace API but I anticipate that following easily once
the userspace API is established.



If I understand it right, the big improvement here is that userspace can
find out what heap IDs are available, and what type of heap they
correspond to? This seems good.

I'm not sure about how userspace is meant to use the usage mappings
though. Who calls ION_IOC_ID_MAP?

(also, map/mapping is pretty overloaded. How about groups/groupings?)

If I assume that the thing calling ION_IOC_ID_MAP is the same thing
doing the allocating, then there doesn't seem to be much need for
creating mappings. The combined mapper/allocator would necessarily need
some knowledge about which types can satisfy which usage, and so could
follow something like this:
 1. The heaps can be queried, finding their IDs and types
 2. A mask of heap IDs can be created which satisfies a "usage", based
on the queried types
 3. Allocation operations can then simply use this constructed mask

On the other hand, if I assume that the thing doing the allocating is
different to the thing doing the usage mapping (i.e. the allocator
doesn't need to know about heap _types_, only usages); then I can't see
a way for the allocator to figure out which usage_id it's meant to
allocate with - which puts it right back to the old problem of opaque
heap IDs (-> opaque usage_ids), except it's worse because they can
change dynamically.

Thanks,
Brian



Thanks,
Laura

P.S. Not to turn this into a bike shedding session but if you have suggestions
for a name for this framework other than Ion I would be interested to hear
them. Too many other things are already named Ion.

Laura Abbott (6):
 staging: android: ion: return error value for ion_device_add_heap
 staging: android: ion: Switch to using an idr to manage heaps
 staging: android: ion: Drop heap type masks
 staging: android: ion: Pull out ion ioctls to a separate file
 staging: android: ion: Add an ioctl for ABI checking
 staging: android: ion: Introduce new ioctls for dynamic heaps

drivers/staging/android/ion/Makefile|   3 +-
drivers/staging/android/ion/ion-ioctl.c | 243 ++
drivers/staging/android/ion/ion.c   | 438 
drivers/staging/android/ion/ion_priv.h  | 109 +++-
drivers/staging/android/uapi/ion.h  | 164 +++-
5 files changed, 728 insertions(+), 229 deletions(-)
create mode 100644 drivers/staging/android/ion/ion-ioctl.c

-- 2.5.5

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel