Re: [PATCH] staging: ion: remove from the tree

[Laura Abbott]

On 8/27/20 8:36 AM, Greg Kroah-Hartman wrote:

The ION android code has long been marked to be removed, now that we
dma-buf support merged into the real part of the kernel.

It was thought that we could wait to remove the ion kernel at a later
time, but as the out-of-tree Android fork of the ion code has diverged
quite a bit, and any Android device using the ion interface uses that
forked version and not this in-tree version, the in-tree copy of the
code is abandonded and not used by anyone.

Combine this abandoned codebase with the need to make changes to it in
order to keep the kernel building properly, which then causes merge
issues when merging those changes into the out-of-tree Android code, and
you end up with two different groups of people (the in-kernel-tree
developers, and the Android kernel developers) who are both annoyed at
the current situation.  Because of this problem, just drop the in-kernel
copy of the ion code now, as it's not used, and is only causing problems
for everyone involved.

Cc: "Arve Hjønnevåg" 
Cc: "Christian König" 
Cc: Christian Brauner 
Cc: Christoph Hellwig 
Cc: Hridya Valsaraju 
Cc: Joel Fernandes 
Cc: John Stultz 
Cc: Laura Abbott 
Cc: Martijn Coenen 
Cc: Shuah Khan 
Cc: Sumit Semwal 
Cc: Suren Baghdasaryan 
Cc: Todd Kjos 
Cc: de...@driverdev.osuosl.org
Cc: dri-de...@lists.freedesktop.org
Cc: linaro-mm-...@lists.linaro.org
Signed-off-by: Greg Kroah-Hartman 


We discussed this at the Android MC on Monday and the plan was to
remove it after the next LTS release.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 04/15] staging/android/ion: delete dma_buf->kmap/unmap implemenation

[Laura Abbott]

On 11/18/19 5:35 AM, Daniel Vetter wrote:

There's no callers in-tree anymore.

For merging probably best to stuff this into drm-misc, since that's
where the dma-buf heaps will land too. And the resulting conflict
hopefully ensures that dma-buf heaps wont have a new ->kmap/unmap
implemenation.

Signed-off-by: Daniel Vetter 
Cc: Laura Abbott 
Cc: Sumit Semwal 
Cc: de...@driverdev.osuosl.org
Cc: linaro-mm-...@lists.linaro.org
---
  drivers/staging/android/ion/ion.c | 14 --
  1 file changed, 14 deletions(-)

diff --git a/drivers/staging/android/ion/ion.c 
b/drivers/staging/android/ion/ion.c
index e6b1ca141b93..bb4ededc1150 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -274,18 +274,6 @@ static void ion_dma_buf_release(struct dma_buf *dmabuf)
_ion_buffer_destroy(buffer);
  }
  
-static void *ion_dma_buf_kmap(struct dma_buf *dmabuf, unsigned long offset)

-{
-   struct ion_buffer *buffer = dmabuf->priv;
-
-   return buffer->vaddr + offset * PAGE_SIZE;
-}
-
-static void ion_dma_buf_kunmap(struct dma_buf *dmabuf, unsigned long offset,
-  void *ptr)
-{
-}
-
  static int ion_dma_buf_begin_cpu_access(struct dma_buf *dmabuf,
enum dma_data_direction direction)
  {
@@ -349,8 +337,6 @@ static const struct dma_buf_ops dma_buf_ops = {
.detach = ion_dma_buf_detatch,
.begin_cpu_access = ion_dma_buf_begin_cpu_access,
.end_cpu_access = ion_dma_buf_end_cpu_access,
-   .map = ion_dma_buf_kmap,
-   .unmap = ion_dma_buf_kunmap,
  };
  
  static int ion_alloc(size_t len, unsigned int heap_id_mask, unsigned int flags)




Acked-by: Laura Abbott 

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

Re: [PATCH] ion_system_heap: support X86 archtecture

[Laura Abbott]

On 9/29/19 3:28 AM, jun.zh...@intel.com wrote:

From: zhang jun 

we see tons of warning like:
[   45.846872] x86/PAT: NDK MediaCodec_:3753 map pfn RAM range req
write-combining for [mem 0x1e7a8-0x1e7a87fff], got write-back
[   45.848827] x86/PAT: .vorbis.decoder:4088 map pfn RAM range req
write-combining for [mem 0x1e7a58000-0x1e7a58fff], got write-back
[   45.848875] x86/PAT: NDK MediaCodec_:3753 map pfn RAM range req
write-combining for [mem 0x1e7a48000-0x1e7a4], got write-back
[   45.849403] x86/PAT: .vorbis.decoder:4088 map pfn RAM range
req write-combining for [mem 0x1e7a7-0x1e7a70fff], got write-back

check the kernel Documentation/x86/pat.txt, it says:
A. Exporting pages to users with remap_pfn_range, io_remap_pfn_range,
vm_insert_pfn
Drivers wanting to export some pages to userspace do it by using
mmap interface and a combination of
1) pgprot_noncached()
2) io_remap_pfn_range() or remap_pfn_range() or vm_insert_pfn()
With PAT support, a new API pgprot_writecombine is being added.
So, drivers can continue to use the above sequence, with either
pgprot_noncached() or pgprot_writecombine() in step 1, followed by step 2.

In addition, step 2 internally tracks the region as UC or WC in
memtype list in order to ensure no conflicting mapping.

Note that this set of APIs only works with IO (non RAM) regions.
If driver ants to export a RAM region, it has to do set_memory_uc() or
set_memory_wc() as step 0 above and also track the usage of those pages
and use set_memory_wb() before the page is freed to free pool.

the fix follow the pat document, do set_memory_wc() as step 0 and
use the set_memory_wb() before the page is freed.



All this work needs to be done on the new dma-buf heap rework and I
don't think it makes sense to put it on the staging version

https://lore.kernel.org/lkml/20190906184712.91980-1-john.stu...@linaro.org/

(I also continue to question the value of uncached buffers, especially on
x86)


Signed-off-by: he, bo 
Signed-off-by: zhang jun 
Signed-off-by: Bai, Jie A 
---
  drivers/staging/android/ion/ion_system_heap.c | 28 ++-
  1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/android/ion/ion_system_heap.c 
b/drivers/staging/android/ion/ion_system_heap.c
index b83a1d16bd89..d298b8194820 100644
--- a/drivers/staging/android/ion/ion_system_heap.c
+++ b/drivers/staging/android/ion/ion_system_heap.c
@@ -13,6 +13,7 @@
  #include 
  #include 
  #include 
+#include 
  
  #include "ion.h"
  
@@ -134,6 +135,13 @@ static int ion_system_heap_allocate(struct ion_heap *heap,

sg = table->sgl;
list_for_each_entry_safe(page, tmp_page, , lru) {
sg_set_page(sg, page, page_size(page), 0);
+
+#ifdef CONFIG_X86
+   if (!(buffer->flags & ION_FLAG_CACHED))
+   set_memory_wc((unsigned long)page_address(sg_page(sg)),
+ PAGE_ALIGN(sg->length) >> PAGE_SHIFT);
+#endif
+
sg = sg_next(sg);
list_del(>lru);
}
@@ -162,8 +170,15 @@ static void ion_system_heap_free(struct ion_buffer *buffer)
if (!(buffer->private_flags & ION_PRIV_FLAG_SHRINKER_FREE))
ion_heap_buffer_zero(buffer);
  
-	for_each_sg(table->sgl, sg, table->nents, i)

+   for_each_sg(table->sgl, sg, table->nents, i) {
+#ifdef CONFIG_X86
+   if (!(buffer->flags & ION_FLAG_CACHED))
+   set_memory_wb((unsigned long)page_address(sg_page(sg)),
+ PAGE_ALIGN(sg->length) >> PAGE_SHIFT);
+#endif
+
free_buffer_page(sys_heap, buffer, sg_page(sg));
+   }
sg_free_table(table);
kfree(table);
  }
@@ -316,6 +331,12 @@ static int ion_system_contig_heap_allocate(struct ion_heap 
*heap,
  
  	buffer->sg_table = table;
  
+#ifdef CONFIG_X86

+   if (!(buffer->flags & ION_FLAG_CACHED))
+   set_memory_wc((unsigned long)page_address(page),
+ PAGE_ALIGN(len) >> PAGE_SHIFT);
+#endif
+
return 0;
  
  free_table:

@@ -334,6 +355,11 @@ static void ion_system_contig_heap_free(struct ion_buffer 
*buffer)
unsigned long pages = PAGE_ALIGN(buffer->size) >> PAGE_SHIFT;
unsigned long i;
  
+#ifdef CONFIG_X86

+   if (!(buffer->flags & ION_FLAG_CACHED))
+   set_memory_wb((unsigned long)page_address(page), pages);
+#endif
+
for (i = 0; i < pages; i++)
__free_page(page + i);
sg_free_table(table);



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

Re: [PATCH] staging: android: ion: cma heap: Limit size of allocated buffer

[Laura Abbott]

On 8/26/19 3:55 AM, Alexey Skidanov wrote:


On 8/26/19 11:36 AM, Laura Abbott wrote:

On 8/23/19 10:28 PM, Alexey Skidanov wrote:

In ion_cma_heap, the allocated buffer is represented by a single
struct scatterlist instance. The length field of this struct is
32 bit, hence the maximal size of requested buffer should be
less than 4GB.

The len paramer of the allocation function is 64 bit (on 64 bit systems).
Hence the requested size might be greater than 4GB and in this case
the field length of the struct scatterlist is initialized incorrectly.

To fix this, we check that requested size may fit into
the field length of the struct scatterlist



Is this a real issue that's actually possible to hit? Allocating
more than a 4GB region of CMA seems ill advised and likely to
throw off all the accounting.


Yes. Not sure why it seems ill advised - most of the buffers are small or 
middle size ones, but sometimes really huge one is requested.



Mostly I'm surprised allocating 4GB of CMA actually succeeds. When
I was last doing work on CMA, the reliability just wasn't there
for larger regions. It's been several years since then so maybe
things have changed. My concern about the accounting is that
most of the math is done such that there are a few CMA pages
vs more pages across the rest of the system which seems likely to
hit performance issues. CMA was designed with targets with smaller
memory footprints so anything that has 4GB of memory to go around is
much larger than what CMA was originally designed for.

The check added should only be applicable if we can reliably
allocate more than 4GB of CMA. If you can confirm you have a setup
where you are actually able to allocate 4GB of CMA, I'd rather
just have the check be for 4GB explicitly instead of tying
it to just the scatterlist length. It's a reasonable restriction
to make and it's easier to review.


Signed-off-by: Alexey Skidanov 
---
   drivers/staging/android/ion/ion.h  | 5 +
   drivers/staging/android/ion/ion_cma_heap.c | 3 +++
   2 files changed, 8 insertions(+)

diff --git a/drivers/staging/android/ion/ion.h 
b/drivers/staging/android/ion/ion.h
index e291299..9dd7e20 100644
--- a/drivers/staging/android/ion/ion.h
+++ b/drivers/staging/android/ion/ion.h
@@ -21,6 +21,11 @@
     #include "../uapi/ion.h"
   +#define MAX_SCATTERLIST_LEN ({\
+    typeof(((struct scatterlist *)0)->length) v;\
+    v = -1;\
+    })
+
   /**
    * struct ion_buffer - metadata for a particular buffer
    * @node:    node in the ion_device buffers tree
diff --git a/drivers/staging/android/ion/ion_cma_heap.c 
b/drivers/staging/android/ion/ion_cma_heap.c
index bf65e67..d069719 100644
--- a/drivers/staging/android/ion/ion_cma_heap.c
+++ b/drivers/staging/android/ion/ion_cma_heap.c
@@ -36,6 +36,9 @@ static int ion_cma_allocate(struct ion_heap *heap, struct 
ion_buffer *buffer,
   unsigned long align = get_order(size);
   int ret;
   +    if (size > MAX_SCATTERLIST_LEN)
+    return -EINVAL;
+
   if (align > CONFIG_CMA_ALIGNMENT)
   align = CONFIG_CMA_ALIGNMENT;
  




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

Re: [PATCH] staging: android: ion: cma heap: Limit size of allocated buffer

[Laura Abbott]

On 8/23/19 10:28 PM, Alexey Skidanov wrote:

In ion_cma_heap, the allocated buffer is represented by a single
struct scatterlist instance. The length field of this struct is
32 bit, hence the maximal size of requested buffer should be
less than 4GB.

The len paramer of the allocation function is 64 bit (on 64 bit systems).
Hence the requested size might be greater than 4GB and in this case
the field length of the struct scatterlist is initialized incorrectly.

To fix this, we check that requested size may fit into
the field length of the struct scatterlist



Is this a real issue that's actually possible to hit? Allocating
more than a 4GB region of CMA seems ill advised and likely to
throw off all the accounting.


Signed-off-by: Alexey Skidanov 
---
  drivers/staging/android/ion/ion.h  | 5 +
  drivers/staging/android/ion/ion_cma_heap.c | 3 +++
  2 files changed, 8 insertions(+)

diff --git a/drivers/staging/android/ion/ion.h 
b/drivers/staging/android/ion/ion.h
index e291299..9dd7e20 100644
--- a/drivers/staging/android/ion/ion.h
+++ b/drivers/staging/android/ion/ion.h
@@ -21,6 +21,11 @@
  
  #include "../uapi/ion.h"
  
+#define MAX_SCATTERLIST_LEN ({\

+   typeof(((struct scatterlist *)0)->length) v;\
+   v = -1;\
+   })
+
  /**
   * struct ion_buffer - metadata for a particular buffer
   * @node: node in the ion_device buffers tree
diff --git a/drivers/staging/android/ion/ion_cma_heap.c 
b/drivers/staging/android/ion/ion_cma_heap.c
index bf65e67..d069719 100644
--- a/drivers/staging/android/ion/ion_cma_heap.c
+++ b/drivers/staging/android/ion/ion_cma_heap.c
@@ -36,6 +36,9 @@ static int ion_cma_allocate(struct ion_heap *heap, struct 
ion_buffer *buffer,
unsigned long align = get_order(size);
int ret;
  
+	if (size > MAX_SCATTERLIST_LEN)

+   return -EINVAL;
+
if (align > CONFIG_CMA_ALIGNMENT)
align = CONFIG_CMA_ALIGNMENT;
  



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

Re: Limits for ION Memory Allocator

[Laura Abbott]

On 7/17/19 12:31 PM, Alexander Popov wrote:

Hello!

The syzkaller [1] has a trouble with fuzzing the Linux kernel with ION Memory
Allocator.

Syzkaller uses several methods [2] to limit memory consumption of the userspace
processes calling the syscalls for testing the kernel:
  - setrlimit(),
  - cgroups,
  - various sysctl.
But these methods don't work for ION Memory Allocator, so any userspace process
that has access to /dev/ion can bring the system to the out-of-memory state.

An example of a program doing that:


#include 
#include 
#include 
#include 
#include 
#include 

#define ION_IOC_MAGIC   'I'
#define ION_IOC_ALLOC   _IOWR(ION_IOC_MAGIC, 0, \
  struct ion_allocation_data)

struct ion_allocation_data {
__u64 len;
__u32 heap_id_mask;
__u32 flags;
__u32 fd;
__u32 unused;
};

int main(void)
{
unsigned long i = 0;
int fd = -1;
struct ion_allocation_data data = {
.len = 0x13f65d8c,
.heap_id_mask = 1,
.flags = 0,
.fd = -1,
.unused = 0
};

fd = open("/dev/ion", 0);
if (fd == -1) {
perror("[-] open /dev/ion");
return 1;
}

while (1) {
printf("iter %lu\n", i);
ioctl(fd, ION_IOC_ALLOC, );
i++;
}

return 0;
}


I looked through the code of ion_alloc() and didn't find any limit checks.
Is it currently possible to limit ION kernel allocations for some process?

If not, is it a right idea to do that?
Thanks!



Yes, I do think that's the right approach. We're working on moving Ion
out of staging and this is something I mentioned to John Stultz. I don't
think we've thought too hard about how to do the actual limiting so
suggestions are welcome.

Thanks,
Laura


Best regards,
Alexander


[1]: https://github.com/google/syzkaller
[2]: https://github.com/google/syzkaller/blob/master/executor/common_linux.h



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

Re: [PATCH] staging: android: ion: Remove unused rbtree for ion_buffer

[Laura Abbott]

On 7/12/19 4:47 AM, Lecopzer Chen wrote:

ion_buffer_add() insert ion_buffer into rbtree every time creating
an ion_buffer but never use it after ION reworking.
Also, buffer_lock protects only rbtree operation, remove it together.

Signed-off-by: Lecopzer Chen 
Cc: YJ Chiang 
Cc: Lecopzer Chen 
---
  drivers/staging/android/ion/ion.c | 36 ---
  drivers/staging/android/ion/ion.h | 10 +
  2 files changed, 1 insertion(+), 45 deletions(-)

diff --git a/drivers/staging/android/ion/ion.c 
b/drivers/staging/android/ion/ion.c
index 92c2914239e3..e6b1ca141b93 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -29,32 +29,6 @@
  static struct ion_device *internal_dev;
  static int heap_id;
  
-/* this function should only be called while dev->lock is held */

-static void ion_buffer_add(struct ion_device *dev,
-  struct ion_buffer *buffer)
-{
-   struct rb_node **p = >buffers.rb_node;
-   struct rb_node *parent = NULL;
-   struct ion_buffer *entry;
-
-   while (*p) {
-   parent = *p;
-   entry = rb_entry(parent, struct ion_buffer, node);
-
-   if (buffer < entry) {
-   p = &(*p)->rb_left;
-   } else if (buffer > entry) {
-   p = &(*p)->rb_right;
-   } else {
-   pr_err("%s: buffer already found.", __func__);
-   BUG();
-   }
-   }
-
-   rb_link_node(>node, parent, p);
-   rb_insert_color(>node, >buffers);
-}
-
  /* this function should only be called while dev->lock is held */
  static struct ion_buffer *ion_buffer_create(struct ion_heap *heap,
struct ion_device *dev,
@@ -100,9 +74,6 @@ static struct ion_buffer *ion_buffer_create(struct ion_heap 
*heap,
  
  	INIT_LIST_HEAD(>attachments);

mutex_init(>lock);
-   mutex_lock(>buffer_lock);
-   ion_buffer_add(dev, buffer);
-   mutex_unlock(>buffer_lock);
return buffer;
  
  err1:

@@ -131,11 +102,6 @@ void ion_buffer_destroy(struct ion_buffer *buffer)
  static void _ion_buffer_destroy(struct ion_buffer *buffer)
  {
struct ion_heap *heap = buffer->heap;
-   struct ion_device *dev = buffer->dev;
-
-   mutex_lock(>buffer_lock);
-   rb_erase(>node, >buffers);
-   mutex_unlock(>buffer_lock);
  
  	if (heap->flags & ION_HEAP_FLAG_DEFER_FREE)

ion_heap_freelist_add(heap, buffer);
@@ -694,8 +660,6 @@ static int ion_device_create(void)
}
  
  	idev->debug_root = debugfs_create_dir("ion", NULL);

-   idev->buffers = RB_ROOT;
-   mutex_init(>buffer_lock);
init_rwsem(>lock);
plist_head_init(>heaps);
internal_dev = idev;
diff --git a/drivers/staging/android/ion/ion.h 
b/drivers/staging/android/ion/ion.h
index e291299fd35f..74914a266e25 100644
--- a/drivers/staging/android/ion/ion.h
+++ b/drivers/staging/android/ion/ion.h
@@ -23,7 +23,6 @@
  
  /**

   * struct ion_buffer - metadata for a particular buffer
- * @node:  node in the ion_device buffers tree
   * @list: element in list of deferred freeable buffers
   * @dev:  back pointer to the ion_device
   * @heap: back pointer to the heap the buffer came from
@@ -39,10 +38,7 @@
   * @attachments:  list of devices attached to this buffer
   */
  struct ion_buffer {
-   union {
-   struct rb_node node;
-   struct list_head list;
-   };
+   struct list_head list;
struct ion_device *dev;
struct ion_heap *heap;
unsigned long flags;
@@ -61,14 +57,10 @@ void ion_buffer_destroy(struct ion_buffer *buffer);
  /**
   * struct ion_device - the metadata of the ion device node
   * @dev:  the actual misc device
- * @buffers:   an rb tree of all the existing buffers
- * @buffer_lock:   lock protecting the tree of buffers
   * @lock: rwsem protecting the tree of heaps and clients
   */
  struct ion_device {
struct miscdevice dev;
-   struct rb_root buffers;
-   struct mutex buffer_lock;
    struct rw_semaphore lock;
struct plist_head heaps;
struct dentry *debug_root;



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

Re: [PATCH 2/2] staging: android: ion: Remove file ion_chunk_heap.c

[Laura Abbott]

On 7/3/19 4:18 AM, Nishka Dasgupta wrote:

Remove file ion_chunk_heap.c as its functions and definitions are not
used anywhere else.
Issue found with Coccinelle.



Acked-by: Laura Abbott 


Signed-off-by: Nishka Dasgupta 
---
  drivers/staging/android/ion/Kconfig  |   9 --
  drivers/staging/android/ion/Makefile |   1 -
  drivers/staging/android/ion/ion_chunk_heap.c | 147 ---
  3 files changed, 157 deletions(-)
  delete mode 100644 drivers/staging/android/ion/ion_chunk_heap.c

diff --git a/drivers/staging/android/ion/Kconfig 
b/drivers/staging/android/ion/Kconfig
index dff641451a89..989fe84a9f9d 100644
--- a/drivers/staging/android/ion/Kconfig
+++ b/drivers/staging/android/ion/Kconfig
@@ -18,15 +18,6 @@ config ION_SYSTEM_HEAP
  Choose this option to enable the Ion system heap. The system heap
  is backed by pages from the buddy allocator. If in doubt, say Y.
  
-config ION_CHUNK_HEAP

-   bool "Ion chunk heap support"
-   depends on ION
-   help
-  Choose this option to enable chunk heaps with Ion. This heap is
- similar in function the carveout heap but memory is broken down
- into smaller chunk sizes, typically corresponding to a TLB size.
- Unless you know your system has these regions, you should say N here.
-
  config ION_CMA_HEAP
bool "Ion CMA heap support"
depends on ION && DMA_CMA
diff --git a/drivers/staging/android/ion/Makefile 
b/drivers/staging/android/ion/Makefile
index 0ac5465e2841..5f4487b1a224 100644
--- a/drivers/staging/android/ion/Makefile
+++ b/drivers/staging/android/ion/Makefile
@@ -1,5 +1,4 @@
  # SPDX-License-Identifier: GPL-2.0
  obj-$(CONFIG_ION) += ion.o ion_heap.o
  obj-$(CONFIG_ION_SYSTEM_HEAP) += ion_system_heap.o ion_page_pool.o
-obj-$(CONFIG_ION_CHUNK_HEAP) += ion_chunk_heap.o
  obj-$(CONFIG_ION_CMA_HEAP) += ion_cma_heap.o
diff --git a/drivers/staging/android/ion/ion_chunk_heap.c 
b/drivers/staging/android/ion/ion_chunk_heap.c
deleted file mode 100644
index 1e869f4bad45..
--- a/drivers/staging/android/ion/ion_chunk_heap.c
+++ /dev/null
@@ -1,147 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/*
- * ION memory allocator chunk heap helper
- *
- * Copyright (C) 2012 Google, Inc.
- */
-
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-
-#include "ion.h"
-
-struct ion_chunk_heap {
-   struct ion_heap heap;
-   struct gen_pool *pool;
-   unsigned long chunk_size;
-   unsigned long size;
-   unsigned long allocated;
-};
-
-static int ion_chunk_heap_allocate(struct ion_heap *heap,
-  struct ion_buffer *buffer,
-  unsigned long size,
-  unsigned long flags)
-{
-   struct ion_chunk_heap *chunk_heap =
-   container_of(heap, struct ion_chunk_heap, heap);
-   struct sg_table *table;
-   struct scatterlist *sg;
-   int ret, i;
-   unsigned long num_chunks;
-   unsigned long allocated_size;
-
-   allocated_size = ALIGN(size, chunk_heap->chunk_size);
-   num_chunks = allocated_size / chunk_heap->chunk_size;
-
-   if (allocated_size > chunk_heap->size - chunk_heap->allocated)
-   return -ENOMEM;
-
-   table = kmalloc(sizeof(*table), GFP_KERNEL);
-   if (!table)
-   return -ENOMEM;
-   ret = sg_alloc_table(table, num_chunks, GFP_KERNEL);
-   if (ret) {
-   kfree(table);
-   return ret;
-   }
-
-   sg = table->sgl;
-   for (i = 0; i < num_chunks; i++) {
-   unsigned long paddr = gen_pool_alloc(chunk_heap->pool,
-chunk_heap->chunk_size);
-   if (!paddr)
-   goto err;
-   sg_set_page(sg, pfn_to_page(PFN_DOWN(paddr)),
-   chunk_heap->chunk_size, 0);
-   sg = sg_next(sg);
-   }
-
-   buffer->sg_table = table;
-   chunk_heap->allocated += allocated_size;
-   return 0;
-err:
-   sg = table->sgl;
-   for (i -= 1; i >= 0; i--) {
-   gen_pool_free(chunk_heap->pool, page_to_phys(sg_page(sg)),
- sg->length);
-   sg = sg_next(sg);
-   }
-   sg_free_table(table);
-   kfree(table);
-   return -ENOMEM;
-}
-
-static void ion_chunk_heap_free(struct ion_buffer *buffer)
-{
-   struct ion_heap *heap = buffer->heap;
-   struct ion_chunk_heap *chunk_heap =
-   container_of(heap, struct ion_chunk_heap, heap);
-   struct sg_table *table = buffer->sg_table;
-   struct scatterlist *sg;
-   int i;
-   unsigned long allocated_size;
-
-   allocated_size = ALIGN(buffer->size, chunk_heap->chunk_size);
-
-   ion_heap_buffer_zero(buffer);
-
-   for_each_sg(table-&g

Re: [PATCH 1/2] staging: android: ion: Remove file ion_carveout_heap.c

[Laura Abbott]

On 7/3/19 5:50 AM, Daniel Vetter wrote:

On Wed, Jul 3, 2019 at 10:37 AM Greg KH  wrote:


On Wed, Jul 03, 2019 at 01:48:41PM +0530, Nishka Dasgupta wrote:

Remove file ion_carveout_heap.c as its functions and definitions are not
used anywhere.
Issue found with Coccinelle.

Signed-off-by: Nishka Dasgupta 
---
  drivers/staging/android/ion/Kconfig   |   9 --
  drivers/staging/android/ion/Makefile  |   1 -
  .../staging/android/ion/ion_carveout_heap.c   | 133 --


I keep trying to do this, but others point out that the ion code is
"going to be fixed up soon" and that people rely on this interface now.
Well, "code outside of the kernel tree" relies on this, which is not ok,
but the "soon" people keep insisting on it...

Odds are I should just delete all of ION, as there hasn't been any
forward progress on it in a long time.

Hopefully that wakes some people up...


John Stultz has done a steady stream on ion destaging patch series
past few months, und the heading of "DMA-BUF Heaps", targeting
drivers/dma-buf. I'm not following the details, and it seems a bit a
crawl, but there's definitely work going on ... Just probably not
in-place in staging I think.
-Daniel




https://lists.freedesktop.org/archives/dri-devel/2019-June/223705.html

It is making slow and steady progress. Part of this is trying to
make sure we actually get this right before moving anything
out of staging.

That said, I think we're at the point where nobody wants the
carveout and chunk heaps so I'd actually be okay with removing
those files. Just to be explicit:

Acked-by: Laura Abbott 

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

Re: [PATCH] staging: android: ion: refactory ion_alloc for kernel driver use

[Laura Abbott]

On 3/29/19 7:26 PM, Zengtao (B) wrote:

Hi laura:


-Original Message-
From: Laura Abbott [mailto:labb...@redhat.com]
Sent: Friday, March 29, 2019 9:27 PM
To: Zengtao (B) ; sumit.sem...@linaro.org
Cc: Greg Kroah-Hartman ; Arve Hjønnevåg
; Todd Kjos ; Martijn Coenen
; Joel Fernandes ;
Christian Brauner ; de...@driverdev.osuosl.org;
dri-de...@lists.freedesktop.org; linaro-mm-...@lists.linaro.org;
linux-ker...@vger.kernel.org
Subject: Re: [PATCH] staging: android: ion: refactory ion_alloc for kernel
driver use

On 3/29/19 11:40 AM, Zeng Tao wrote:

There are two reasons for this patch:
1. There are some potential requirements for ion_alloc in kernel
space, some media drivers need to allocate media buffers from ion
instead of buddy or dma framework, this is more convient and clean
very for media drivers. And In that case, ion is the only media buffer
provider, it's more easier to maintain.
2. Fd is only needed by user processes, not the kernel space, so
dma_buf should be returned instead of fd for kernel space, and
dma_buf_fd should be called only for userspace api.



I really want to just NAK this because it doesn't seem like something
that's necessary. The purpose of Ion is to provide buffers to userspace
because there's no other way for userspace to get access to the memory.
The kernel already has other APIs to access the memory. This also
complicates the re-work that's been happening where the requirement is
only userspace.

Can you be more detailed about which media drivers you are referring to
and why they can't just use other APIs?



I think I 've got your point, the ION is designed for usespace, but for kernel
  space, we are really lacking of someone which plays the same role,(allocate
media memory, share the memory using dma_buf, provide debug and statistics
for media memory).

In fact, for kernel space, we have the dma framework, dma-buf, etc..
And we can work on top of such apis, but some duplicate jobs(everyone has
  to maintain its own buffer sharing, debug and statistics).
So we need to have some to do the common things(ION's the best choice now)



Keep in mind that Ion is a thin shell of what it was as most of the
debugging and statistics was removed because it was buggy. Most of that
should end up going at the dma_buf layer since it's really a dma_buf allocation
API.


When the ION was introduced, a lot of media memory frameworks existed, the
dma framework was not so good, so ION heaps, integrated buffer sharing, 
statistics
and usespace api were the required features, but now dma framework is more 
powerful,
we don't even need ION heaps now, but the userspace api, buffer sharing, 
statistics are
still needed, and the buffer sharing, statistics can be re-worked and export to 
kernel space,
not only used by userspace, , and that is my point.



I see what you are getting at but I don't think the same thing
applies to the kernel as it does userspace. We can enforce a
single way of using the dma_buf fd in userspace but the kernel
has a variety of ways to use dma_buf because each driver and
framework has its own needs. I'm still not convinced that adding
Ion APIs in the kernel is the right option since as you point out
we don't really need the heaps. That mostly leaves Ion as a wrapper
to handle doing the export. Maybe we could benefit from that
but I think it might require more thought.

I'd rather see a proposal in the media API itself showing what
you think is necessary but without using Ion. That would be
a good start so we could fully review what might make sense to
pull out of Ion into something common.

Thanks,
Laura




Signed-off-by: Zeng Tao 
---
   drivers/staging/android/ion/ion.c | 32

+---

   1 file changed, 17 insertions(+), 15 deletions(-)

diff --git a/drivers/staging/android/ion/ion.c
b/drivers/staging/android/ion/ion.c
index 92c2914..e93fb49 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -387,13 +387,13 @@ 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)
+struct dma_buf *ion_alloc(size_t len, unsigned int heap_id_mask,
+ unsigned int flags)
   {
struct ion_device *dev = internal_dev;
struct ion_buffer *buffer = NULL;
struct ion_heap *heap;
DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
-   int fd;
struct dma_buf *dmabuf;

pr_debug("%s: len %zu heap_id_mask %u flags %x\n", __func__,

@@

-407,7 +407,7 @@ static int ion_alloc(size_t len, unsigned int

heap_id_mask, unsigned int flags)

len = PAGE_ALIGN(len);

if (!len)
-   return -EINVAL;
+   return ERR_PTR(-EINVAL);

down_read(>lock);
plist_for_each_entry(heap, >heaps, node) { @@ -421,10

+421,10

@@ static int ion_alloc(size_t len, unsigned int heap_id_mask,

unsigned int flags)

 

Re: [PATCH] staging: android: ion: refactory ion_alloc for kernel driver use

[Laura Abbott]

On 3/29/19 11:40 AM, Zeng Tao wrote:

There are two reasons for this patch:
1. There are some potential requirements for ion_alloc in kernel space,
some media drivers need to allocate media buffers from ion instead of
buddy or dma framework, this is more convient and clean very for media
drivers. And In that case, ion is the only media buffer provider, it's
more easier to maintain.
2. Fd is only needed by user processes, not the kernel space, so dma_buf
should be returned instead of fd for kernel space, and dma_buf_fd should
be called only for userspace api.



I really want to just NAK this because it doesn't seem like something
that's necessary. The purpose of Ion is to provide buffers to userspace
because there's no other way for userspace to get access to the memory.
The kernel already has other APIs to access the memory. This also
complicates the re-work that's been happening where the requirement
is only userspace.

Can you be more detailed about which media drivers you are referring
to and why they can't just use other APIs?



Signed-off-by: Zeng Tao 
---
  drivers/staging/android/ion/ion.c | 32 +---
  1 file changed, 17 insertions(+), 15 deletions(-)

diff --git a/drivers/staging/android/ion/ion.c 
b/drivers/staging/android/ion/ion.c
index 92c2914..e93fb49 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -387,13 +387,13 @@ 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)

+struct dma_buf *ion_alloc(size_t len, unsigned int heap_id_mask,
+ unsigned int flags)
  {
struct ion_device *dev = internal_dev;
struct ion_buffer *buffer = NULL;
struct ion_heap *heap;
DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
-   int fd;
struct dma_buf *dmabuf;
  
  	pr_debug("%s: len %zu heap_id_mask %u flags %x\n", __func__,

@@ -407,7 +407,7 @@ static int ion_alloc(size_t len, unsigned int heap_id_mask, 
unsigned int flags)
len = PAGE_ALIGN(len);
  
  	if (!len)

-   return -EINVAL;
+   return ERR_PTR(-EINVAL);
  
  	down_read(>lock);

plist_for_each_entry(heap, >heaps, node) {
@@ -421,10 +421,10 @@ static int ion_alloc(size_t len, unsigned int 
heap_id_mask, unsigned int flags)
up_read(>lock);
  
  	if (!buffer)

-   return -ENODEV;
+   return ERR_PTR(-ENODEV);
  
  	if (IS_ERR(buffer))

-   return PTR_ERR(buffer);
+   return ERR_PTR(PTR_ERR(buffer));
  
  	exp_info.ops = _buf_ops;

exp_info.size = buffer->size;
@@ -432,17 +432,12 @@ static int ion_alloc(size_t len, unsigned int 
heap_id_mask, unsigned int flags)
exp_info.priv = buffer;
  
  	dmabuf = dma_buf_export(_info);

-   if (IS_ERR(dmabuf)) {
+   if (IS_ERR(dmabuf))
_ion_buffer_destroy(buffer);
-   return PTR_ERR(dmabuf);
-   }
  
-	fd = dma_buf_fd(dmabuf, O_CLOEXEC);

-   if (fd < 0)
-   dma_buf_put(dmabuf);
-
-   return fd;
+   return dmabuf;
  }
+EXPORT_SYMBOL(ion_alloc);
  
  static int ion_query_heaps(struct ion_heap_query *query)

  {
@@ -539,12 +534,19 @@ static long ion_ioctl(struct file *filp, unsigned int 
cmd, unsigned long arg)
case ION_IOC_ALLOC:
{
int fd;
+   struct dma_buf *dmabuf;
  
-		fd = ion_alloc(data.allocation.len,

+   dmabuf = ion_alloc(data.allocation.len,
   data.allocation.heap_id_mask,
   data.allocation.flags);
-   if (fd < 0)
+   if (IS_ERR(dmabuf))
+   return PTR_ERR(dmabuf);
+
+   fd = dma_buf_fd(dmabuf, O_CLOEXEC);
+   if (fd < 0) {
+   dma_buf_put(dmabuf);
return fd;
+   }
  
  		data.allocation.fd = fd;
  



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

Re: [PATCH] driver : staging : ion: optimization for decreasing memory fragmentaion

[Laura Abbott]

On 3/20/19 7:23 AM, Vlastimil Babka wrote:

You should have CC'd the ION maintainers/lists per
./scripts/get_maintainer.pl - CCing now.

On 3/14/19 12:06 PM, Zhaoyang Huang wrote:

From: Zhaoyang Huang 

Two action for this patch:
1. set a batch size for system heap's shrinker, which can have it buffer
reasonable page blocks in pool for future allocation.
2. reverse the order sequence when free page blocks, the purpose is also
to have system heap keep as more big blocks as it can.

By testing on an android system with 2G RAM, the changes with setting
batch = 48MB can help reduce the fragmentation obviously and improve
big block allocation speed for 15%.

Signed-off-by: Zhaoyang Huang 
---
  drivers/staging/android/ion/ion_heap.c| 12 +++-
  drivers/staging/android/ion/ion_system_heap.c |  2 +-
  2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/android/ion/ion_heap.c 
b/drivers/staging/android/ion/ion_heap.c
index 31db510..9e9caf2 100644
--- a/drivers/staging/android/ion/ion_heap.c
+++ b/drivers/staging/android/ion/ion_heap.c
@@ -16,6 +16,8 @@
  #include 
  #include "ion.h"
  
+unsigned long ion_heap_batch = 0;

+
  void *ion_heap_map_kernel(struct ion_heap *heap,
  struct ion_buffer *buffer)
  {
@@ -303,7 +305,15 @@ int ion_heap_init_shrinker(struct ion_heap *heap)
heap->shrinker.count_objects = ion_heap_shrink_count;
heap->shrinker.scan_objects = ion_heap_shrink_scan;
heap->shrinker.seeks = DEFAULT_SEEKS;
-   heap->shrinker.batch = 0;
+   heap->shrinker.batch = ion_heap_batch;
  
  	return register_shrinker(>shrinker);

  }
+
+static int __init ion_system_heap_batch_init(char *arg)
+{
+ion_heap_batch = memparse(arg, NULL);
+
+   return 0;
+}
+early_param("ion_batch", ion_system_heap_batch_init);
diff --git a/drivers/staging/android/ion/ion_system_heap.c 
b/drivers/staging/android/ion/ion_system_heap.c
index 701eb9f..d249f8d 100644
--- a/drivers/staging/android/ion/ion_system_heap.c
+++ b/drivers/staging/android/ion/ion_system_heap.c
@@ -182,7 +182,7 @@ static int ion_system_heap_shrink(struct ion_heap *heap, 
gfp_t gfp_mask,
if (!nr_to_scan)
only_scan = 1;
  
-	for (i = 0; i < NUM_ORDERS; i++) {

+   for (i = NUM_ORDERS - 1; i >= 0; i--) {
pool = sys_heap->pools[i];
  
  		if (only_scan) {






We're in the process of significantly reworking Ion so I
don't think it makes sense to take these as we work to
get things out of staging. You can resubmit this later,
but when you do please split this into two separate
patches since it's actually two independent changes.

Thanks,
Laura

___
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

[Laura Abbott]

On 1/24/19 8:44 AM, Brian Starkey wrote:

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



In some respects this is more a question of "what is the purpose
of Ion". Once upon a time, Ion was going to do constraint solving
but that never really happened and I figured Ion would get deprecated.
People keep coming out of the woodwork with new uses for Ion so
its stuck around. This is why I've primarily focused on Ion as a
framework for exposing available memory types to userspace and leave
the constraint solving to someone else, since that's what most
users seem to want out of Ion ("I know I want memory type X please
give it to me"). That's not to say that this was a perfect or
even the correct approach, just what made the most sense based
on users.

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

Re: [PATCH] staging: android: ion: fix sys heap pool's gfp_flags

[Laura Abbott]

On 1/31/19 10:59 PM, Qing Xia wrote:

In the first loop, gfp_flags will be modified to high_order_gfp_flags,
and there will be no chance to change back to low_order_gfp_flags.

Fixes: e7f63771 ("ION: Sys_heap: Add cached pool to spead up cached buffer 
alloc")
Signed-off-by: Qing Xia 
---
  drivers/staging/android/ion/ion_system_heap.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/android/ion/ion_system_heap.c 
b/drivers/staging/android/ion/ion_system_heap.c
index 0383f75..20f2103 100644
--- a/drivers/staging/android/ion/ion_system_heap.c
+++ b/drivers/staging/android/ion/ion_system_heap.c
@@ -223,10 +223,10 @@ static void ion_system_heap_destroy_pools(struct 
ion_page_pool **pools)
  static int ion_system_heap_create_pools(struct ion_page_pool **pools)
  {
int i;
-   gfp_t gfp_flags = low_order_gfp_flags;
  
  	for (i = 0; i < NUM_ORDERS; i++) {

struct ion_page_pool *pool;
+   gfp_t gfp_flags = low_order_gfp_flags;
  
  		if (orders[i] > 4)

gfp_flags = high_order_gfp_flags;



Acked-by: Laura Abbott 
___
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

[Laura Abbott]

On 2/15/19 11:01 AM, 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.



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.


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/1472769644-11039-4-git-send-email-labb...@redhat.com/T/#u
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: android: ion: Use low_order_gfp_flags for smaller allocations

[Laura Abbott]

On 2/11/19 11:09 PM, Jing Xia wrote:

gfp_flags is always set high_order_gfp_flags even if allocations of
order 0 are made.But for smaller allocations, the system should be able
to reclaim some memory.

Signed-off-by: Jing Xia 
Reviewed-by: Yuming Han 
Reviewed-by: Zhaoyang Huang 
Reviewed-by: Orson Zhai 
---
  drivers/staging/android/ion/ion_system_heap.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/android/ion/ion_system_heap.c 
b/drivers/staging/android/ion/ion_system_heap.c
index 0383f75..20f2103 100644
--- a/drivers/staging/android/ion/ion_system_heap.c
+++ b/drivers/staging/android/ion/ion_system_heap.c
@@ -223,10 +223,10 @@ static void ion_system_heap_destroy_pools(struct 
ion_page_pool **pools)
  static int ion_system_heap_create_pools(struct ion_page_pool **pools)
  {
int i;
-   gfp_t gfp_flags = low_order_gfp_flags;
  
  	for (i = 0; i < NUM_ORDERS; i++) {

struct ion_page_pool *pool;
+   gfp_t gfp_flags = low_order_gfp_flags;
  
  		if (orders[i] > 4)

gfp_flags = high_order_gfp_flags;



This was already submitted in
https://lore.kernel.org/lkml/1549004386-38778-1-git-send-email-saberlily@hisilicon.com/T/#u
(I'm also very behind on Ion e-mail and need to catch up...)

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

Re: [PATCH 3/4] dma-buf: add support for mapping with dma mapping attributes

[Laura Abbott]

On 1/19/19 2:25 AM, Christoph Hellwig wrote:

On Fri, Jan 18, 2019 at 10:37:46AM -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.

Signed-off-by: Liam Mark 


And who is going to decide which ones to pass?  And who documents
which ones are safe?

I'd much rather have explicit, well documented dma-buf flags that
might get translated to the DMA API flags, which are not error checked,
not very well documented and way to easy to get wrong.



I'm not sure having flags in dma-buf really solves anything
given drivers can use the attributes directly with dma_map
anyway, which is what we're looking to do. The intention
is for the driver creating the dma_buf attachment to have
the knowledge of which flags to use.

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

Re: [PATCH 3/4] dma-buf: add support for mapping with dma mapping attributes

[Laura Abbott]

On 1/18/19 1:32 PM, Liam Mark wrote:

On Fri, 18 Jan 2019, Laura Abbott wrote:


On 1/18/19 10:37 AM, 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.

Signed-off-by: Liam Mark 
---
   include/linux/dma-buf.h | 3 +++
   1 file changed, 3 insertions(+)

diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
index 58725f890b5b..59bf33e09e2d 100644
--- a/include/linux/dma-buf.h
+++ b/include/linux/dma-buf.h
@@ -308,6 +308,8 @@ struct dma_buf {
* @dev: device attached to the buffer.
* @node: list of dma_buf_attachment.
* @priv: exporter specific attachment data.
+ * @dma_map_attrs: DMA mapping attributes to be used in
+ *dma_buf_map_attachment() and dma_buf_unmap_attachment().
*
* This structure holds the attachment information between the dma_buf
buffer
* and its user device(s). The list contains one attachment struct per
device
@@ -323,6 +325,7 @@ struct dma_buf_attachment {
struct device *dev;
struct list_head node;
void *priv;
+   unsigned long dma_map_attrs;
   };
 /**



Did you miss part of this patch? This only adds it to the structure but
doesn't
add it to any API. The same commment applies to the follow up patch,
I don't quite see how it's being used.



Were you asking for a cleaner DMA-buf API to set this field or were you
asking for a change to an upstream client to make use of this field?

I have clients set the dma_map_attrs field directly on their
dma_buf_attachment struct before calling dma_buf_map_attachment (if they
need this functionality).
Of course this is all being used in Android for out of tree drivers, but
I assume it is just as useful to everyone else who has cached ION buffers
which aren't always accessed by the CPU.

My understanding is that AOSP Android on Hikey 960 also is currently
suffering from too many CMOs due to dma_map_attachemnt always applying
CMOs, so this support should help them avoid it.



A I see how you intend this to be used now! I was missing
that clients would do attachment->dma_map_attrs = blah
and that was how it would be stored as opposed to passing
it in at the top level for dma_buf_map. I'll give this some
more thought but I think it could work if Sumit is okay
with the approach.

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

Re: [PATCH 3/4] dma-buf: add support for mapping with dma mapping attributes

[Laura Abbott]

On 1/18/19 10:37 AM, 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.

Signed-off-by: Liam Mark 
---
  include/linux/dma-buf.h | 3 +++
  1 file changed, 3 insertions(+)

diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
index 58725f890b5b..59bf33e09e2d 100644
--- a/include/linux/dma-buf.h
+++ b/include/linux/dma-buf.h
@@ -308,6 +308,8 @@ struct dma_buf {
   * @dev: device attached to the buffer.
   * @node: list of dma_buf_attachment.
   * @priv: exporter specific attachment data.
+ * @dma_map_attrs: DMA mapping attributes to be used in
+ *dma_buf_map_attachment() and dma_buf_unmap_attachment().
   *
   * This structure holds the attachment information between the dma_buf buffer
   * and its user device(s). The list contains one attachment struct per device
@@ -323,6 +325,7 @@ struct dma_buf_attachment {
struct device *dev;
struct list_head node;
void *priv;
+   unsigned long dma_map_attrs;
  };
  
  /**




Did you miss part of this patch? This only adds it to the structure but doesn't
add it to any API. The same commment applies to the follow up patch,
I don't quite see how it's being used.

Thanks,
Laura
___
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

[Laura Abbott]

On 1/18/19 12:43 PM, Andrew F. Davis wrote:

On 1/18/19 2:31 PM, Laura Abbott wrote:

On 1/17/19 8:13 AM, Andrew F. Davis wrote:

On 1/16/19 4:48 PM, Liam Mark wrote:

On Wed, 16 Jan 2019, Andrew F. Davis wrote:


On 1/15/19 1:05 PM, Laura Abbott wrote:

On 1/15/19 10:38 AM, 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).



I agree it's broken, hence my desire to remove it :)

The other problem is that uncached buffers are being used for
performance reason so anything that would involve getting
rid of the logical address would probably negate any performance
benefit.



I wouldn't go as far as to remove them just yet.. Liam seems pretty
adamant that they have valid uses. I'm just not sure performance is one
of them, maybe in the case of software locks between devices or
something where there needs to be a lot of back and forth interleaved
access on small amounts of data?



I wasn't aware that ARM considered this not supported, I thought it was
supported but they advised against it because of the potential
performance
impact.



Not sure what you mean by "this" being not supported, do you mean mixed
attribute mappings? If so, it will certainly cause problems, and the
problems will change from platform to platform, avoid at all costs is my
understanding of ARM's position.


This is after all supported in the DMA APIs and up until now devices
have
been successfully commercializing with this configurations, and I think
they will continue to commercialize with these configurations for
quite a
while.



Use of uncached memory mappings are almost always wrong in my experience
and are used to work around some bug or because the user doesn't want to
implement proper CMOs. Counter examples welcome.


It would be really unfortunate if support was removed as I think that
would drive clients away from using upstream ION.



I'm not petitioning to remove su

Re: [PATCH 1/4] staging: android: ion: Support cpu access during dma_buf_detach

[Laura Abbott]

On 1/18/19 10:37 AM, Liam Mark wrote:

Often userspace doesn't know when the kernel will be calling dma_buf_detach
on the buffer.
If userpace starts its CPU access at the same time as the sg list is being
freed it could end up accessing the sg list after it has been freed.

Thread AThread B
- DMA_BUF_IOCTL_SYNC IOCT
  - ion_dma_buf_begin_cpu_access
   - list_for_each_entry
- ion_dma_buf_detatch
 - free_duped_table
- dma_sync_sg_for_cpu

Fix this by getting the ion_buffer lock before freeing the sg table memory.

Fixes: 2a55e7b5e544 ("staging: android: ion: Call dma_map_sg for syncing and 
mapping")
Signed-off-by: Liam Mark 
---
  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 a0802de8c3a1..6f5afab7c1a1 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -248,10 +248,10 @@ static void ion_dma_buf_detatch(struct dma_buf *dmabuf,
struct ion_dma_buf_attachment *a = attachment->priv;
struct ion_buffer *buffer = dmabuf->priv;
  
-	free_duped_table(a->table);

mutex_lock(>lock);
list_del(>list);
mutex_unlock(>lock);
+   free_duped_table(a->table);
  
  	kfree(a);

  }



Acked-by: Laura Abbott 
___
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

[Laura Abbott]

On 1/17/19 8:13 AM, Andrew F. Davis wrote:

On 1/16/19 4:48 PM, Liam Mark wrote:

On Wed, 16 Jan 2019, Andrew F. Davis wrote:


On 1/15/19 1:05 PM, Laura Abbott wrote:

On 1/15/19 10:38 AM, 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).



I agree it's broken, hence my desire to remove it :)

The other problem is that uncached buffers are being used for
performance reason so anything that would involve getting
rid of the logical address would probably negate any performance
benefit.



I wouldn't go as far as to remove them just yet.. Liam seems pretty
adamant that they have valid uses. I'm just not sure performance is one
of them, maybe in the case of software locks between devices or
something where there needs to be a lot of back and forth interleaved
access on small amounts of data?



I wasn't aware that ARM considered this not supported, I thought it was
supported but they advised against it because of the potential performance
impact.



Not sure what you mean by "this" being not supported, do you mean mixed
attribute mappings? If so, it will certainly cause problems, and the
problems will change from platform to platform, avoid at all costs is my
understanding of ARM's position.


This is after all supported in the DMA APIs and up until now devices have
been successfully commercializing with this configurations, and I think
they will continue to commercialize with these configurations for quite a
while.



Use of uncached memory mappings are almost always wrong in my experience
and are used to work around some bug or because the user doesn't want to
implement proper CMOs. Counter examples welcome.


It would be really unfortunate if support was removed as I think that
would drive clients away from using upstream ION.



I'm not petitioning to remove support, but at very least lets reverse
the ION_FLAG_CACHED flag. Ion should hand out cached normal m

Re: [PATCH 00/14] Misc ION cleanups and adding unmapped heap

[Laura Abbott]

On 1/16/19 8:05 AM, Andrew F. Davis wrote:

On 1/15/19 12:58 PM, Laura Abbott wrote:

On 1/15/19 9:47 AM, Andrew F. Davis wrote:

On 1/14/19 8:39 PM, Laura Abbott wrote:

On 1/11/19 10:05 AM, Andrew F. Davis wrote:

Hello all,

This is a set of (hopefully) non-controversial cleanups for the ION
framework and current set of heaps. These were found as I start to
familiarize myself with the framework to help in whatever way I
can in getting all this up to the standards needed for de-staging.

I would like to get some ideas of what is left to work on to get ION
out of staging. Has there been some kind of agreement on what ION
should
eventually end up being? To me it looks like it is being whittled
away at
to it's most core functions. To me that is looking like being a DMA-BUF
user-space front end, simply advertising available memory backings in a
system and providing allocations as DMA-BUF handles. If this is the
case
then it looks close to being ready to me at least, but I would love to
hear any other opinions and concerns.



Yes, at this point the only functionality that people are really
depending on is the ability to allocate a dma_buf easily from userspace.


Back to this patchset, the last patch may be a bit different than the
others, it adds an unmapped heaps type and creation helper. I wanted to
get this in to show off another heap type and maybe some issues we may
have with the current ION framework. The unmapped heap is used when the
backing memory should not (or cannot) be touched. Currently this kind
of heap is used for firewalled secure memory that can be allocated like
normal heap memory but only used by secure devices (OP-TEE, crypto HW,
etc). It is basically just copied from the "carveout" heap type with
the
only difference being it is not mappable to userspace and we do not
clear
the memory (as we should not map it either). So should this really be a
new heap type? Or maybe advertised as a carveout heap but with an
additional allocation flag? Perhaps we do away with "types" altogether
and just have flags, coherent/non-coherent, mapped/unmapped, etc.

Maybe more thinking will be needed afterall..



So the cleanup looks okay (I need to finish reviewing) but I'm not a
fan of adding another heaptype without solving the problem of adding
some sort of devicetree binding or other method of allocating and
placing Ion heaps. That plus uncached buffers are one of the big
open problems that need to be solved for destaging Ion. See
https://lore.kernel.org/lkml/20181120164636.jcw7li2uaa3cmwc3@DESKTOP-E1NTVVP.localdomain/


for some background on that problem.



I'm under the impression that adding heaps like carveouts/chunk will be
rather system specific and so do not lend themselves well to a universal
DT style exporter. For instance a carveout memory space can be reported
by a device at runtime, then the driver managing that device should go
and use the carveout heap helpers to export that heap. If this is the
case then I'm not sure it is a problem for the ION core framework to
solve, but rather the users of it to figure out how best to create the
various heaps. All Ion needs to do is allow exporting and advertising
them IMHO.



I think it is a problem for the Ion core framework to take care of.
Ion is useless if you don't actually have the heaps. Nobody has
actually gotten a full Ion solution end-to-end with a carveout heap
working in mainline because any proposals have been rejected. I think
we need at least one example in mainline of how creating a carveout
heap would work.


In our evil vendor trees we have several examples. The issue being that
Ion is still staging and attempts for generic DT heap definitions
haven't seemed to go so well. So for now we just keep it specific to our
platforms until upstream makes a direction decision.



Yeah, it's been a bit of a chicken and egg in that this has been
blocking Ion getting out of staging but we don't actually have
in-tree users because it's still in staging.




Thanks for the background thread link, I've been looking for some info
on current status of all this and "ion" is a bit hard to search the
lists for. The core reason for posting this cleanup series is to throw
my hat into the ring of all this Ion work and start getting familiar
with the pending issues. The last two patches are not all that important
to get in right now.

In that thread you linked above, it seems we may have arrived at a
similar problem for different reasons. I think the root issue is the Ion
core makes too many assumptions about the heap memory. My proposal would
be to allow the heap exporters more control over the DMA-BUF ops, maybe
even going as far as letting them provide their own complete struct
dma_buf_ops.

Let me give an example where I think this is going to be useful. We have
the classic constraint solving problem on our SoCs. Our SoCs are full of
various coherent and non-coherent devices, some require contiguous
memory allocations,

Re: [PATCH 12/14] staging: android: ion: Declare helpers for carveout and chunk heaps

[Laura Abbott]

On 1/18/19 1:59 AM, Greg Kroah-Hartman wrote:

On Fri, Jan 11, 2019 at 12:05:21PM -0600, Andrew F. Davis wrote:

When enabled the helpers functions for creating carveout and chunk heaps
should have declarations in the ION header.


Why?  No one calls these from what I can tell.

Which makes me believe we should just delete the
drivers/staging/android/ion/ion_carveout_heap.c and
drivers/staging/android/ion/ion_chunk_heap.c files as there are no
in-tree users?

Any objection to me doing that?

thanks,

greg k-h



I'd rather not delete it quite yet. Part of this entire thread is a
discussion on how to let those heaps and associated function actually
be called in some way in tree. I expect them to either get called
in tree or be replaced.

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

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

[Laura Abbott]

On 1/16/19 9:12 AM, Andrew F. Davis wrote:

On 1/16/19 9:28 AM, Brian Starkey wrote:

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.



Right now the names are only for debug. I accidentally left the name
null once and got a kernel crash. This is the only spot where it is
needed so I fixed it up. The other option is to make the name mandatory
and properly error out, I don't want to do that right now until the
below discussion is had to see if names really do matter or not.



Yes, the heap names are part of the query API and are the expected
way to identify individual heaps for the API at the moment so having
a null heap name is incorrect. The heap name seemed like the best way
to identify heaps to userspace but if you have an alternative proposal
I'd be interested.

Thanks,
Laura






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

Re: [PATCH 14/14] staging: android: ion: Add UNMAPPED heap type and helper

[Laura Abbott]

On 1/15/19 10:43 AM, Laura Abbott wrote:

On 1/15/19 7:58 AM, Andrew F. Davis wrote:

On 1/14/19 8:32 PM, Laura Abbott wrote:

On 1/11/19 10:05 AM, Andrew F. Davis wrote:

The "unmapped" heap is very similar to the carveout heap except
the backing memory is presumed to be unmappable by the host, in
my specific case due to firewalls. This memory can still be
allocated from and used by devices that do have access to the
backing memory.

Based originally on the secure/unmapped heap from Linaro for
the OP-TEE SDP implementation, this was re-written to match
the carveout heap helper code.

Suggested-by: Etienne Carriere 
Signed-off-by: Andrew F. Davis 
---
   drivers/staging/android/ion/Kconfig   |  10 ++
   drivers/staging/android/ion/Makefile  |   1 +
   drivers/staging/android/ion/ion.h |  16 +++
   .../staging/android/ion/ion_unmapped_heap.c   | 123 ++
   drivers/staging/android/uapi/ion.h    |   3 +
   5 files changed, 153 insertions(+)
   create mode 100644 drivers/staging/android/ion/ion_unmapped_heap.c

diff --git a/drivers/staging/android/ion/Kconfig
b/drivers/staging/android/ion/Kconfig
index 0fdda6f62953..a117b8b91b14 100644
--- a/drivers/staging/android/ion/Kconfig
+++ b/drivers/staging/android/ion/Kconfig
@@ -42,3 +42,13 @@ config ION_CMA_HEAP
 Choose this option to enable CMA heaps with Ion. This heap is
backed
 by the Contiguous Memory Allocator (CMA). If your system has
these
 regions, you should say Y here.
+
+config ION_UNMAPPED_HEAP
+    bool "ION unmapped heap support"
+    depends on ION
+    help
+  Choose this option to enable UNMAPPED heaps with Ion. This heap is
+  backed in specific memory pools, carveout from the Linux memory.
+  Unlike carveout heaps these are assumed to be not mappable by
+  kernel or user-space.
+  Unless you know your system has these regions, you should say N
here.
diff --git a/drivers/staging/android/ion/Makefile
b/drivers/staging/android/ion/Makefile
index 17f3a7569e3d..c71a1f3de581 100644
--- a/drivers/staging/android/ion/Makefile
+++ b/drivers/staging/android/ion/Makefile
@@ -4,3 +4,4 @@ obj-$(CONFIG_ION_SYSTEM_HEAP) += ion_system_heap.o
ion_page_pool.o
   obj-$(CONFIG_ION_CARVEOUT_HEAP) += ion_carveout_heap.o
   obj-$(CONFIG_ION_CHUNK_HEAP) += ion_chunk_heap.o
   obj-$(CONFIG_ION_CMA_HEAP) += ion_cma_heap.o
+obj-$(CONFIG_ION_UNMAPPED_HEAP) += ion_unmapped_heap.o
diff --git a/drivers/staging/android/ion/ion.h
b/drivers/staging/android/ion/ion.h
index 97b2876b165a..ce74332018ba 100644
--- a/drivers/staging/android/ion/ion.h
+++ b/drivers/staging/android/ion/ion.h
@@ -341,4 +341,20 @@ static inline struct ion_heap
*ion_chunk_heap_create(phys_addr_t base, size_t si
   }
   #endif
   +#ifdef CONFIG_ION_UNMAPPED_HEAP
+/**
+ * ion_unmapped_heap_create
+ * @base:    base address of carveout memory
+ * @size:    size of carveout memory region
+ *
+ * Creates an unmapped ion_heap using the passed in data
+ */
+struct ion_heap *ion_unmapped_heap_create(phys_addr_t base, size_t
size);
+#else
+struct ion_heap *ion_unmapped_heap_create(phys_addr_t base, size_t size)
+{
+    return ERR_PTR(-ENODEV);
+}
+#endif
+
   #endif /* _ION_H */
diff --git a/drivers/staging/android/ion/ion_unmapped_heap.c
b/drivers/staging/android/ion/ion_unmapped_heap.c
new file mode 100644
index ..7602b659c2ec
--- /dev/null
+++ b/drivers/staging/android/ion/ion_unmapped_heap.c
@@ -0,0 +1,123 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * ION Memory Allocator unmapped heap helper
+ *
+ * Copyright (C) 2015-2016 Texas Instruments Incorporated -
http://www.ti.com/
+ *    Andrew F. Davis 
+ *
+ * ION "unmapped" heaps are physical memory heaps not by default
mapped into
+ * a virtual address space. The buffer owner can explicitly request
kernel
+ * space mappings but the underlying memory may still not be
accessible for
+ * various reasons, such as firewalls.
+ */
+
+#include 
+#include 
+#include 
+#include 
+
+#include "ion.h"
+
+#define ION_UNMAPPED_ALLOCATE_FAIL -1
+
+struct ion_unmapped_heap {
+    struct ion_heap heap;
+    struct gen_pool *pool;
+};
+
+static phys_addr_t ion_unmapped_allocate(struct ion_heap *heap,
+ unsigned long size)
+{
+    struct ion_unmapped_heap *unmapped_heap =
+    container_of(heap, struct ion_unmapped_heap, heap);
+    unsigned long offset;
+
+    offset = gen_pool_alloc(unmapped_heap->pool, size);
+    if (!offset)
+    return ION_UNMAPPED_ALLOCATE_FAIL;
+
+    return offset;
+}
+
+static void ion_unmapped_free(struct ion_heap *heap, phys_addr_t addr,
+  unsigned long size)
+{
+    struct ion_unmapped_heap *unmapped_heap =
+    container_of(heap, struct ion_unmapped_heap, heap);
+
+    gen_pool_free(unmapped_heap->pool, addr, size);
+}
+
+static int ion_unmapped_heap_allocate(struct ion_heap *heap,
+  struct ion_buffer *buf

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

[Laura Abbott]

On 1/15/19 10:38 AM, 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).



I agree it's broken, hence my desire to remove it :)

The other problem is that uncached buffers are being used for
performance reason so anything that would involve getting
rid of the logical address would probably negate any performance
benefit.


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


dma_buf_detach  (device cannot stay attached because it is being sent down
the pipeline and Camera doesn't know the end of the use case)



This seems like a broken use-case, I understand the desire to keep
everything as modular as possible and separate the steps, but at this
point no one owns this buffers backing memory, not the CPU or any
device. I would go as far as to say DMA-BUF should be free now to
de-allocate the backing storage if it wants, that way it could get ready
for the next attachment, which may change the required backing memory
completely.

All devices should attach before the first mapping, and only let go
after the task is complete, otherwise this buffers data needs copied off
to a different location or the CPU needs to take ownership in-between.



Maybe it's broken but it's the status quo and we spent a good

Re: [PATCH 00/14] Misc ION cleanups and adding unmapped heap

[Laura Abbott]

On 1/15/19 9:47 AM, Andrew F. Davis wrote:

On 1/14/19 8:39 PM, Laura Abbott wrote:

On 1/11/19 10:05 AM, Andrew F. Davis wrote:

Hello all,

This is a set of (hopefully) non-controversial cleanups for the ION
framework and current set of heaps. These were found as I start to
familiarize myself with the framework to help in whatever way I
can in getting all this up to the standards needed for de-staging.

I would like to get some ideas of what is left to work on to get ION
out of staging. Has there been some kind of agreement on what ION should
eventually end up being? To me it looks like it is being whittled away at
to it's most core functions. To me that is looking like being a DMA-BUF
user-space front end, simply advertising available memory backings in a
system and providing allocations as DMA-BUF handles. If this is the case
then it looks close to being ready to me at least, but I would love to
hear any other opinions and concerns.



Yes, at this point the only functionality that people are really
depending on is the ability to allocate a dma_buf easily from userspace.


Back to this patchset, the last patch may be a bit different than the
others, it adds an unmapped heaps type and creation helper. I wanted to
get this in to show off another heap type and maybe some issues we may
have with the current ION framework. The unmapped heap is used when the
backing memory should not (or cannot) be touched. Currently this kind
of heap is used for firewalled secure memory that can be allocated like
normal heap memory but only used by secure devices (OP-TEE, crypto HW,
etc). It is basically just copied from the "carveout" heap type with the
only difference being it is not mappable to userspace and we do not clear
the memory (as we should not map it either). So should this really be a
new heap type? Or maybe advertised as a carveout heap but with an
additional allocation flag? Perhaps we do away with "types" altogether
and just have flags, coherent/non-coherent, mapped/unmapped, etc.

Maybe more thinking will be needed afterall..



So the cleanup looks okay (I need to finish reviewing) but I'm not a
fan of adding another heaptype without solving the problem of adding
some sort of devicetree binding or other method of allocating and
placing Ion heaps. That plus uncached buffers are one of the big
open problems that need to be solved for destaging Ion. See
https://lore.kernel.org/lkml/20181120164636.jcw7li2uaa3cmwc3@DESKTOP-E1NTVVP.localdomain/

for some background on that problem.



I'm under the impression that adding heaps like carveouts/chunk will be
rather system specific and so do not lend themselves well to a universal
DT style exporter. For instance a carveout memory space can be reported
by a device at runtime, then the driver managing that device should go
and use the carveout heap helpers to export that heap. If this is the
case then I'm not sure it is a problem for the ION core framework to
solve, but rather the users of it to figure out how best to create the
various heaps. All Ion needs to do is allow exporting and advertising
them IMHO.



I think it is a problem for the Ion core framework to take care of.
Ion is useless if you don't actually have the heaps. Nobody has
actually gotten a full Ion solution end-to-end with a carveout heap
working in mainline because any proposals have been rejected. I think
we need at least one example in mainline of how creating a carveout
heap would work.


Thanks for the background thread link, I've been looking for some info
on current status of all this and "ion" is a bit hard to search the
lists for. The core reason for posting this cleanup series is to throw
my hat into the ring of all this Ion work and start getting familiar
with the pending issues. The last two patches are not all that important
to get in right now.

In that thread you linked above, it seems we may have arrived at a
similar problem for different reasons. I think the root issue is the Ion
core makes too many assumptions about the heap memory. My proposal would
be to allow the heap exporters more control over the DMA-BUF ops, maybe
even going as far as letting them provide their own complete struct
dma_buf_ops.

Let me give an example where I think this is going to be useful. We have
the classic constraint solving problem on our SoCs. Our SoCs are full of
various coherent and non-coherent devices, some require contiguous
memory allocations, others have in-line IOMMUs so can operate on
non-contiguous, etc..

DMA-BUF has a solution designed in for this we can use, namely
allocation at map time after all the attachments have come in. The
checking of each attached device to find the right backing memory is
something the DMA-BUF exporter has to do, and so this SoC specific logic
would have to be added to each exporting framework (DRM, V4L2, etc),
unless we have one unified system exporter everyone uses, Ion.



That's how dmabuf is supposed to work in theory but in 

Re: [PATCH 14/14] staging: android: ion: Add UNMAPPED heap type and helper

[Laura Abbott]

On 1/15/19 7:58 AM, Andrew F. Davis wrote:

On 1/14/19 8:32 PM, Laura Abbott wrote:

On 1/11/19 10:05 AM, Andrew F. Davis wrote:

The "unmapped" heap is very similar to the carveout heap except
the backing memory is presumed to be unmappable by the host, in
my specific case due to firewalls. This memory can still be
allocated from and used by devices that do have access to the
backing memory.

Based originally on the secure/unmapped heap from Linaro for
the OP-TEE SDP implementation, this was re-written to match
the carveout heap helper code.

Suggested-by: Etienne Carriere 
Signed-off-by: Andrew F. Davis 
---
   drivers/staging/android/ion/Kconfig   |  10 ++
   drivers/staging/android/ion/Makefile  |   1 +
   drivers/staging/android/ion/ion.h |  16 +++
   .../staging/android/ion/ion_unmapped_heap.c   | 123 ++
   drivers/staging/android/uapi/ion.h    |   3 +
   5 files changed, 153 insertions(+)
   create mode 100644 drivers/staging/android/ion/ion_unmapped_heap.c

diff --git a/drivers/staging/android/ion/Kconfig
b/drivers/staging/android/ion/Kconfig
index 0fdda6f62953..a117b8b91b14 100644
--- a/drivers/staging/android/ion/Kconfig
+++ b/drivers/staging/android/ion/Kconfig
@@ -42,3 +42,13 @@ config ION_CMA_HEAP
     Choose this option to enable CMA heaps with Ion. This heap is
backed
     by the Contiguous Memory Allocator (CMA). If your system has
these
     regions, you should say Y here.
+
+config ION_UNMAPPED_HEAP
+    bool "ION unmapped heap support"
+    depends on ION
+    help
+  Choose this option to enable UNMAPPED heaps with Ion. This heap is
+  backed in specific memory pools, carveout from the Linux memory.
+  Unlike carveout heaps these are assumed to be not mappable by
+  kernel or user-space.
+  Unless you know your system has these regions, you should say N
here.
diff --git a/drivers/staging/android/ion/Makefile
b/drivers/staging/android/ion/Makefile
index 17f3a7569e3d..c71a1f3de581 100644
--- a/drivers/staging/android/ion/Makefile
+++ b/drivers/staging/android/ion/Makefile
@@ -4,3 +4,4 @@ obj-$(CONFIG_ION_SYSTEM_HEAP) += ion_system_heap.o
ion_page_pool.o
   obj-$(CONFIG_ION_CARVEOUT_HEAP) += ion_carveout_heap.o
   obj-$(CONFIG_ION_CHUNK_HEAP) += ion_chunk_heap.o
   obj-$(CONFIG_ION_CMA_HEAP) += ion_cma_heap.o
+obj-$(CONFIG_ION_UNMAPPED_HEAP) += ion_unmapped_heap.o
diff --git a/drivers/staging/android/ion/ion.h
b/drivers/staging/android/ion/ion.h
index 97b2876b165a..ce74332018ba 100644
--- a/drivers/staging/android/ion/ion.h
+++ b/drivers/staging/android/ion/ion.h
@@ -341,4 +341,20 @@ static inline struct ion_heap
*ion_chunk_heap_create(phys_addr_t base, size_t si
   }
   #endif
   +#ifdef CONFIG_ION_UNMAPPED_HEAP
+/**
+ * ion_unmapped_heap_create
+ * @base:    base address of carveout memory
+ * @size:    size of carveout memory region
+ *
+ * Creates an unmapped ion_heap using the passed in data
+ */
+struct ion_heap *ion_unmapped_heap_create(phys_addr_t base, size_t
size);
+#else
+struct ion_heap *ion_unmapped_heap_create(phys_addr_t base, size_t size)
+{
+    return ERR_PTR(-ENODEV);
+}
+#endif
+
   #endif /* _ION_H */
diff --git a/drivers/staging/android/ion/ion_unmapped_heap.c
b/drivers/staging/android/ion/ion_unmapped_heap.c
new file mode 100644
index ..7602b659c2ec
--- /dev/null
+++ b/drivers/staging/android/ion/ion_unmapped_heap.c
@@ -0,0 +1,123 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * ION Memory Allocator unmapped heap helper
+ *
+ * Copyright (C) 2015-2016 Texas Instruments Incorporated -
http://www.ti.com/
+ *    Andrew F. Davis 
+ *
+ * ION "unmapped" heaps are physical memory heaps not by default
mapped into
+ * a virtual address space. The buffer owner can explicitly request
kernel
+ * space mappings but the underlying memory may still not be
accessible for
+ * various reasons, such as firewalls.
+ */
+
+#include 
+#include 
+#include 
+#include 
+
+#include "ion.h"
+
+#define ION_UNMAPPED_ALLOCATE_FAIL -1
+
+struct ion_unmapped_heap {
+    struct ion_heap heap;
+    struct gen_pool *pool;
+};
+
+static phys_addr_t ion_unmapped_allocate(struct ion_heap *heap,
+ unsigned long size)
+{
+    struct ion_unmapped_heap *unmapped_heap =
+    container_of(heap, struct ion_unmapped_heap, heap);
+    unsigned long offset;
+
+    offset = gen_pool_alloc(unmapped_heap->pool, size);
+    if (!offset)
+    return ION_UNMAPPED_ALLOCATE_FAIL;
+
+    return offset;
+}
+
+static void ion_unmapped_free(struct ion_heap *heap, phys_addr_t addr,
+  unsigned long size)
+{
+    struct ion_unmapped_heap *unmapped_heap =
+    container_of(heap, struct ion_unmapped_heap, heap);
+
+    gen_pool_free(unmapped_heap->pool, addr, size);
+}
+
+static int ion_unmapped_heap_allocate(struct ion_heap *heap,
+  struct ion_buffer *buffer,
+  unsigned long si

Re: [PATCH 00/14] Misc ION cleanups and adding unmapped heap

[Laura Abbott]

On 1/11/19 10:05 AM, Andrew F. Davis wrote:

Hello all,

This is a set of (hopefully) non-controversial cleanups for the ION
framework and current set of heaps. These were found as I start to
familiarize myself with the framework to help in whatever way I
can in getting all this up to the standards needed for de-staging.

I would like to get some ideas of what is left to work on to get ION
out of staging. Has there been some kind of agreement on what ION should
eventually end up being? To me it looks like it is being whittled away at
to it's most core functions. To me that is looking like being a DMA-BUF
user-space front end, simply advertising available memory backings in a
system and providing allocations as DMA-BUF handles. If this is the case
then it looks close to being ready to me at least, but I would love to
hear any other opinions and concerns.



Yes, at this point the only functionality that people are really
depending on is the ability to allocate a dma_buf easily from userspace.


Back to this patchset, the last patch may be a bit different than the
others, it adds an unmapped heaps type and creation helper. I wanted to
get this in to show off another heap type and maybe some issues we may
have with the current ION framework. The unmapped heap is used when the
backing memory should not (or cannot) be touched. Currently this kind
of heap is used for firewalled secure memory that can be allocated like
normal heap memory but only used by secure devices (OP-TEE, crypto HW,
etc). It is basically just copied from the "carveout" heap type with the
only difference being it is not mappable to userspace and we do not clear
the memory (as we should not map it either). So should this really be a
new heap type? Or maybe advertised as a carveout heap but with an
additional allocation flag? Perhaps we do away with "types" altogether
and just have flags, coherent/non-coherent, mapped/unmapped, etc.

Maybe more thinking will be needed afterall..



So the cleanup looks okay (I need to finish reviewing) but I'm not a
fan of adding another heaptype without solving the problem of adding
some sort of devicetree binding or other method of allocating and
placing Ion heaps. That plus uncached buffers are one of the big
open problems that need to be solved for destaging Ion. See
https://lore.kernel.org/lkml/20181120164636.jcw7li2uaa3cmwc3@DESKTOP-E1NTVVP.localdomain/
for some background on that problem.

Thanks,
Laura


Thanks,
Andrew

Andrew F. Davis (14):
   staging: android: ion: Add proper header information
   staging: android: ion: Remove empty ion_ioctl_dir() function
   staging: android: ion: Merge ion-ioctl.c into ion.c
   staging: android: ion: Remove leftover comment
   staging: android: ion: Remove struct ion_platform_heap
   staging: android: ion: Fixup some white-space issues
   staging: android: ion: Sync comment docs with struct ion_buffer
   staging: android: ion: Remove base from ion_carveout_heap
   staging: android: ion: Remove base from ion_chunk_heap
   staging: android: ion: Remove unused headers
   staging: android: ion: Allow heap name to be null
   staging: android: ion: Declare helpers for carveout and chunk heaps
   staging: android: ion: Do not sync CPU cache on map/unmap
   staging: android: ion: Add UNMAPPED heap type and helper

  drivers/staging/android/ion/Kconfig   |  10 ++
  drivers/staging/android/ion/Makefile  |   3 +-
  drivers/staging/android/ion/ion-ioctl.c   |  98 --
  drivers/staging/android/ion/ion.c |  93 +++--
  drivers/staging/android/ion/ion.h |  87 -
  .../staging/android/ion/ion_carveout_heap.c   |  19 +--
  drivers/staging/android/ion/ion_chunk_heap.c  |  25 ++--
  drivers/staging/android/ion/ion_cma_heap.c|   6 +-
  drivers/staging/android/ion/ion_heap.c|   8 +-
  drivers/staging/android/ion/ion_page_pool.c   |   2 +-
  drivers/staging/android/ion/ion_system_heap.c |   8 +-
  .../staging/android/ion/ion_unmapped_heap.c   | 123 ++
  drivers/staging/android/uapi/ion.h|   3 +
  13 files changed, 307 insertions(+), 178 deletions(-)
  delete mode 100644 drivers/staging/android/ion/ion-ioctl.c
  create mode 100644 drivers/staging/android/ion/ion_unmapped_heap.c



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

Re: [PATCH 14/14] staging: android: ion: Add UNMAPPED heap type and helper

[Laura Abbott]

On 1/11/19 10:05 AM, Andrew F. Davis wrote:

The "unmapped" heap is very similar to the carveout heap except
the backing memory is presumed to be unmappable by the host, in
my specific case due to firewalls. This memory can still be
allocated from and used by devices that do have access to the
backing memory.

Based originally on the secure/unmapped heap from Linaro for
the OP-TEE SDP implementation, this was re-written to match
the carveout heap helper code.

Suggested-by: Etienne Carriere 
Signed-off-by: Andrew F. Davis 
---
  drivers/staging/android/ion/Kconfig   |  10 ++
  drivers/staging/android/ion/Makefile  |   1 +
  drivers/staging/android/ion/ion.h |  16 +++
  .../staging/android/ion/ion_unmapped_heap.c   | 123 ++
  drivers/staging/android/uapi/ion.h|   3 +
  5 files changed, 153 insertions(+)
  create mode 100644 drivers/staging/android/ion/ion_unmapped_heap.c

diff --git a/drivers/staging/android/ion/Kconfig 
b/drivers/staging/android/ion/Kconfig
index 0fdda6f62953..a117b8b91b14 100644
--- a/drivers/staging/android/ion/Kconfig
+++ b/drivers/staging/android/ion/Kconfig
@@ -42,3 +42,13 @@ config ION_CMA_HEAP
  Choose this option to enable CMA heaps with Ion. This heap is backed
  by the Contiguous Memory Allocator (CMA). If your system has these
  regions, you should say Y here.
+
+config ION_UNMAPPED_HEAP
+   bool "ION unmapped heap support"
+   depends on ION
+   help
+ Choose this option to enable UNMAPPED heaps with Ion. This heap is
+ backed in specific memory pools, carveout from the Linux memory.
+ Unlike carveout heaps these are assumed to be not mappable by
+ kernel or user-space.
+ Unless you know your system has these regions, you should say N here.
diff --git a/drivers/staging/android/ion/Makefile 
b/drivers/staging/android/ion/Makefile
index 17f3a7569e3d..c71a1f3de581 100644
--- a/drivers/staging/android/ion/Makefile
+++ b/drivers/staging/android/ion/Makefile
@@ -4,3 +4,4 @@ obj-$(CONFIG_ION_SYSTEM_HEAP) += ion_system_heap.o 
ion_page_pool.o
  obj-$(CONFIG_ION_CARVEOUT_HEAP) += ion_carveout_heap.o
  obj-$(CONFIG_ION_CHUNK_HEAP) += ion_chunk_heap.o
  obj-$(CONFIG_ION_CMA_HEAP) += ion_cma_heap.o
+obj-$(CONFIG_ION_UNMAPPED_HEAP) += ion_unmapped_heap.o
diff --git a/drivers/staging/android/ion/ion.h 
b/drivers/staging/android/ion/ion.h
index 97b2876b165a..ce74332018ba 100644
--- a/drivers/staging/android/ion/ion.h
+++ b/drivers/staging/android/ion/ion.h
@@ -341,4 +341,20 @@ static inline struct ion_heap 
*ion_chunk_heap_create(phys_addr_t base, size_t si
  }
  #endif
  
+#ifdef CONFIG_ION_UNMAPPED_HEAP

+/**
+ * ion_unmapped_heap_create
+ * @base:  base address of carveout memory
+ * @size:  size of carveout memory region
+ *
+ * Creates an unmapped ion_heap using the passed in data
+ */
+struct ion_heap *ion_unmapped_heap_create(phys_addr_t base, size_t size);
+#else
+struct ion_heap *ion_unmapped_heap_create(phys_addr_t base, size_t size)
+{
+   return ERR_PTR(-ENODEV);
+}
+#endif
+
  #endif /* _ION_H */
diff --git a/drivers/staging/android/ion/ion_unmapped_heap.c 
b/drivers/staging/android/ion/ion_unmapped_heap.c
new file mode 100644
index ..7602b659c2ec
--- /dev/null
+++ b/drivers/staging/android/ion/ion_unmapped_heap.c
@@ -0,0 +1,123 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * ION Memory Allocator unmapped heap helper
+ *
+ * Copyright (C) 2015-2016 Texas Instruments Incorporated - http://www.ti.com/
+ * Andrew F. Davis 
+ *
+ * ION "unmapped" heaps are physical memory heaps not by default mapped into
+ * a virtual address space. The buffer owner can explicitly request kernel
+ * space mappings but the underlying memory may still not be accessible for
+ * various reasons, such as firewalls.
+ */
+
+#include 
+#include 
+#include 
+#include 
+
+#include "ion.h"
+
+#define ION_UNMAPPED_ALLOCATE_FAIL -1
+
+struct ion_unmapped_heap {
+   struct ion_heap heap;
+   struct gen_pool *pool;
+};
+
+static phys_addr_t ion_unmapped_allocate(struct ion_heap *heap,
+unsigned long size)
+{
+   struct ion_unmapped_heap *unmapped_heap =
+   container_of(heap, struct ion_unmapped_heap, heap);
+   unsigned long offset;
+
+   offset = gen_pool_alloc(unmapped_heap->pool, size);
+   if (!offset)
+   return ION_UNMAPPED_ALLOCATE_FAIL;
+
+   return offset;
+}
+
+static void ion_unmapped_free(struct ion_heap *heap, phys_addr_t addr,
+ unsigned long size)
+{
+   struct ion_unmapped_heap *unmapped_heap =
+   container_of(heap, struct ion_unmapped_heap, heap);
+
+   gen_pool_free(unmapped_heap->pool, addr, size);
+}
+
+static int ion_unmapped_heap_allocate(struct ion_heap *heap,
+ struct ion_buffer *buffer,
+ unsigned long 

Re: [PATCH] staging: android: ion: Add chunk heaps instantiation

[Laura Abbott]

On 1/2/19 10:28 PM, Alexey Skidanov wrote:



On 1/3/19 12:37 AM, Laura Abbott wrote:

On 12/20/18 1:29 PM, Alexey Skidanov wrote:



On 12/20/18 10:36 PM, Laura Abbott wrote:

On 12/16/18 2:46 AM, Alexey Skidanov wrote:

Chunk heap instantiation should be supported for device tree platforms
and
non device tree platforms. For device tree platforms, it's a platform
specific code responsibility to retrieve the heap configuration data
and to call the appropriate API for heap creation. For non device tree
platforms, there is no defined way to create the heaps.

This patch provides the way of chunk heaps creation using
"ion_chunk_heap=name:size@start" kernel boot parameter.



I've been thinking about this and I think it works but
I'm still kind of torn about not having devicetree bindings.
It doesn't _preclude_ devicetree bindings but I'd hate to
merge this and then end up with something incompatible.
I do want to support non-Android use cases too.

Sorry, what do you mean by non-Android cases?


Any user of Ion that isn't tied to Android. This includes other
userspace frameworks as well as non-devicetree targets.


Sorry, don't follow you ... I tested the patch on Ubuntu machine - so
the non-Android and non-devicetree case is obviously supported :)


This patch _doesn't_ provide the required support for devicetree
bindings and Android use case. I'm trying to balance wanting
to support this use case with knowing that the Android use
case is still unsolved.

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

Re: [PATCH] staging: android: ion: add buffer flag update ioctl

[Laura Abbott]

On 1/3/19 5:42 PM, Zengtao (B) wrote:

-Original Message-
From: Laura Abbott [mailto:labb...@redhat.com]
Sent: Thursday, January 03, 2019 6:31 AM
To: Zengtao (B) ; sumit.sem...@linaro.org
Cc: Greg Kroah-Hartman ; Arve Hjønnevåg
; Todd Kjos ; Martijn Coenen
; Joel Fernandes ;
de...@driverdev.osuosl.org; dri-de...@lists.freedesktop.org;
linaro-mm-...@lists.linaro.org; linux-ker...@vger.kernel.org
Subject: Re: [PATCH] staging: android: ion: add buffer flag update ioctl

On 12/23/18 6:47 PM, Zengtao (B) wrote:

Hi laura:


-Original Message-
From: Laura Abbott [mailto:labb...@redhat.com]
Sent: Friday, December 21, 2018 4:50 AM
To: Zengtao (B) ;

sumit.sem...@linaro.org

Cc: Greg Kroah-Hartman ; Arve

Hjønnevåg

; Todd Kjos ; Martijn

Coenen

; Joel Fernandes ;
de...@driverdev.osuosl.org; dri-de...@lists.freedesktop.org;
linaro-mm-...@lists.linaro.org; linux-ker...@vger.kernel.org
Subject: Re: [PATCH] staging: android: ion: add buffer flag update
ioctl

On 12/19/18 5:39 PM, Zengtao (B) wrote:

Hi laura:


-Original Message-
From: Laura Abbott [mailto:labb...@redhat.com]
Sent: Thursday, December 20, 2018 2:10 AM
To: Zengtao (B) ;

sumit.sem...@linaro.org

Cc: Greg Kroah-Hartman ; Arve

Hjønnevåg

; Todd Kjos ; Martijn

Coenen

; Joel Fernandes ;
de...@driverdev.osuosl.org; dri-de...@lists.freedesktop.org;
linaro-mm-...@lists.linaro.org; linux-ker...@vger.kernel.org
Subject: Re: [PATCH] staging: android: ion: add buffer flag update
ioctl

On 12/19/18 9:19 AM, Zeng Tao wrote:

In some usecases, the buffer cached attribute is not determined at
allocation time, it's determined just before the real cpu mapping.
And from the memory view of point, a buffer should not have the

cached

attribute util is really mapped by the cpu. So in this patch, we
introduced the new ioctl command to target the requirement.



This is racy and error prone. Can you explain more what problem

you

are trying to solve?


My use case is like this:
1.  There are two process A and B, A takes case of ion buffer
allocation,

and

pass the buffer fd to B, then B maps and uses it.
2.  Process B need to map the buffer with different cached attribute
for different use case, for example, if the buffer is used for pure
software algorithm, then we need to map it as cached, otherwise
non-cached, and B needs to deal with both cases.
And unfortunately the mmap syscall takes no cached flags and we
can't decide the cache attribute when we are doing the mmap, so I
introduce new the ioctl even though I think the solution is not as

good.





Thanks for the explanation, this was about the use case I expected.
I'm pretty sure I had this exact problem once upon a time and we
didn't come up with a solution. I'd still like to get rid of uncached
buffers in general and just use cached buffers (see
http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/201
8-N
ovember/128842.html)
What's your usecase for uncached buffers?


Some buffers are only used by HW, in this case, we tend to use

uncached buffers.

But sometimes the SW need to read few buffer contents for debug
purpose and no synchronization is needed.



If this is primarily for debug, that's not really a compelling reason to
support uncached buffers. It's incredibly difficult to do uncached buffers
correctly I'd rather spend effort on making the cached use cases work
better.


No, not only for debug, I meant if the buffers are uncached, the SW will only 
mmap
them for debug or even don't need to mmap them.
So for the two kinds of buffers:
1.  uncached: only the HW need to access it, the SW don't need to mmap it or 
just for debug.
2.  cached: both the HW and the SW need to access it.
The ION is designed as a generalized memory manager, I think we should cover as 
many
requirements as we can in the ION design, so I 'd rather that we keep the 
uncached support.
And I don’t quite understand the difficulty you mentioned to support the 
uncached buffers, would
you explain a little more about it.



We end up with aliasing problems. Each kernel page still has a cached
mapping so it's very difficult to keep the two mappings in sync.
https://lore.kernel.org/lkml/20181120164636.jcw7li2uaa3cmwc3@DESKTOP-E1NTVVP.localdomain/
This thread does a better job of explaining the problem and the
objections to uncached buffers.

And if the software never touches the buffer, why does it
matter if the buffer is uncached?

Laura


Thanks
Zengtao








Signed-off-by: Zeng Tao 
---
 drivers/staging/android/ion/ion-ioctl.c |  4 
 drivers/staging/android/ion/ion.c   | 17

+

 drivers/staging/android/ion/ion.h   |  1 +
 drivers/staging/android/uapi/ion.h  | 22

++

 4 files changed, 44 insertions(+)

diff --git a/drivers/staging/android/ion/ion-ioctl.c
b/drivers/staging/android/ion/ion-ioctl.c
index a8d3cc4..60bb702 100644
--- a/drivers/staging/android/ion/ion-ioctl.c
+++ b/drivers/staging/android/ion/ion-ioctl.c

Re: [PATCH] staging: android: ion: Add chunk heaps instantiation

[Laura Abbott]

On 12/20/18 1:29 PM, Alexey Skidanov wrote:



On 12/20/18 10:36 PM, Laura Abbott wrote:

On 12/16/18 2:46 AM, Alexey Skidanov wrote:

Chunk heap instantiation should be supported for device tree platforms
and
non device tree platforms. For device tree platforms, it's a platform
specific code responsibility to retrieve the heap configuration data
and to call the appropriate API for heap creation. For non device tree
platforms, there is no defined way to create the heaps.

This patch provides the way of chunk heaps creation using
"ion_chunk_heap=name:size@start" kernel boot parameter.



I've been thinking about this and I think it works but
I'm still kind of torn about not having devicetree bindings.
It doesn't _preclude_ devicetree bindings but I'd hate to
merge this and then end up with something incompatible.
I do want to support non-Android use cases too.

Sorry, what do you mean by non-Android cases?


Any user of Ion that isn't tied to Android. This includes other
userspace frameworks as well as non-devicetree targets.



I'm also curious about the value of this heap with just PAGE_SIZE.
The original purpose of the chunk heap was a carveout where
you could easily get allocations large than PAGE_SIZE to
reduce TLB pressure. Do you have another use case for the
chunk heap?

It need to be fixed ... Obviously ... The minimum allocation size should
be parametrized


Sumit, do you have any thoughts?

Thanks,
Laura


Link:
http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2018-November/128495.html

Signed-off-by: Alexey Skidanov 
---
   drivers/staging/android/ion/ion_chunk_heap.c | 96
+---
   include/linux/ion.h  | 18 ++
   2 files changed, 105 insertions(+), 9 deletions(-)
   create mode 100644 include/linux/ion.h

diff --git a/drivers/staging/android/ion/ion_chunk_heap.c
b/drivers/staging/android/ion/ion_chunk_heap.c
index 102c093..1a8e3ad 100644
--- a/drivers/staging/android/ion/ion_chunk_heap.c
+++ b/drivers/staging/android/ion/ion_chunk_heap.c
@@ -21,8 +21,12 @@
   #include 
   #include 
   #include 
+#include 
   #include "ion.h"
   +static struct ion_chunk_heap_cfg
chunk_heap_cfg[MAX_NUM_OF_CHUNK_HEAPS];
+static unsigned int num_of_req_chunk_heaps;
+
   struct ion_chunk_heap {
   struct ion_heap heap;
   struct gen_pool *pool;
@@ -117,15 +121,15 @@ static struct ion_heap_ops chunk_heap_ops = {
   .unmap_kernel = ion_heap_unmap_kernel,
   };
   -struct ion_heap *ion_chunk_heap_create(struct ion_platform_heap
*heap_data)
+static struct ion_heap *ion_chunk_heap_create(struct
ion_chunk_heap_cfg *heap_cfg)
   {
   struct ion_chunk_heap *chunk_heap;
   int ret;
   struct page *page;
   size_t size;
   -    page = pfn_to_page(PFN_DOWN(heap_data->base));
-    size = heap_data->size;
+    page = pfn_to_page(PFN_DOWN(heap_cfg->base));
+    size = heap_cfg->size;
     ret = ion_heap_pages_zero(page, size,
pgprot_writecombine(PAGE_KERNEL));
   if (ret)
@@ -135,23 +139,27 @@ struct ion_heap *ion_chunk_heap_create(struct
ion_platform_heap *heap_data)
   if (!chunk_heap)
   return ERR_PTR(-ENOMEM);
   -    chunk_heap->chunk_size = (unsigned long)heap_data->priv;
+    chunk_heap->chunk_size = heap_cfg->chunk_size;
   chunk_heap->pool =
gen_pool_create(get_order(chunk_heap->chunk_size) +
  PAGE_SHIFT, -1);
   if (!chunk_heap->pool) {
   ret = -ENOMEM;
   goto error_gen_pool_create;
   }
-    chunk_heap->base = heap_data->base;
-    chunk_heap->size = heap_data->size;
+    chunk_heap->base = heap_cfg->base;
+    chunk_heap->size = heap_cfg->size;
+    chunk_heap->heap.name = heap_cfg->heap_name;
   chunk_heap->allocated = 0;
   -    gen_pool_add(chunk_heap->pool, chunk_heap->base,
heap_data->size, -1);
+    gen_pool_add(chunk_heap->pool, chunk_heap->base, heap_cfg->size,
-1);
   chunk_heap->heap.ops = _heap_ops;
   chunk_heap->heap.type = ION_HEAP_TYPE_CHUNK;
   chunk_heap->heap.flags = ION_HEAP_FLAG_DEFER_FREE;
-    pr_debug("%s: base %pa size %zu\n", __func__,
- _heap->base, heap_data->size);
+
+    pr_info("%s: name %s base %pa size %zu\n", __func__,
+    heap_cfg->heap_name,
+    _cfg->base,
+    heap_cfg->size);
     return _heap->heap;
   @@ -160,3 +168,73 @@ struct ion_heap *ion_chunk_heap_create(struct
ion_platform_heap *heap_data)
   return ERR_PTR(ret);
   }
   +static int __init setup_heap(char *param)
+{
+    char *at_sign, *coma, *colon;
+    size_t size_to_copy;
+    struct ion_chunk_heap_cfg *cfg;
+
+    do {
+    cfg = _heap_cfg[num_of_req_chunk_heaps];
+
+    /* heap name */
+    colon = strchr(param, ':');
+    if (!colon)
+    return -EINVAL;
+
+    size_to_copy = min_t(size_t, MAX_CHUNK_HEAP_NAME_

Re: [PATCH] staging: android: ion: move map_kernel to ion_dma_buf_kmap

[Laura Abbott]

On 12/24/18 12:19 AM, Qing Xia wrote:

Now, as Google's user guide, if userspace need clean ION buffer's
cache, they should call ioctl(fd, DMA_BUF_IOCTL_SYNC, sync). Then
we found that ion_dma_buf_begin_cpu_access/ion_dma_buf_end_cpu_access
will do ION buffer's map_kernel, it's not necessary. And if usersapce
only need clean cache, they will call ion_dma_buf_end_cpu_access by
dmabuf's ioctl, ION buffer's kmap_cnt will be wrong value -1, then
driver could not get right kernel vaddr by dma_buf_kmap.



The problem is this subtly violates how dma_buf is supposed
to work. All setup is supposed to happen in begin_cpu_access.
I agree calling map kernel each time isn't great but I think
this needs to be worked out with dma_buf.

Thanks,
Laura


Signed-off-by: Qing Xia 
---
  drivers/staging/android/ion/ion.c | 46 ++-
  1 file changed, 21 insertions(+), 25 deletions(-)

diff --git a/drivers/staging/android/ion/ion.c 
b/drivers/staging/android/ion/ion.c
index 9907332..f7e2812 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -303,45 +303,47 @@ static void ion_dma_buf_release(struct dma_buf *dmabuf)
  static void *ion_dma_buf_kmap(struct dma_buf *dmabuf, unsigned long offset)
  {
struct ion_buffer *buffer = dmabuf->priv;
+   void *vaddr;
  
-	return buffer->vaddr + offset * PAGE_SIZE;

+   if (buffer->heap->ops->map_kernel) {
+   mutex_lock(>lock);
+   vaddr = ion_buffer_kmap_get(buffer);
+   mutex_unlock(>lock);
+   if (IS_ERR(vaddr))
+   return vaddr;
+
+   return vaddr + offset * PAGE_SIZE;
+   }
+
+   return NULL;
  }
  
  static void ion_dma_buf_kunmap(struct dma_buf *dmabuf, unsigned long offset,

   void *ptr)
  {
+   struct ion_buffer *buffer = dmabuf->priv;
+
+   if (buffer->heap->ops->map_kernel) {
+   mutex_lock(>lock);
+   ion_buffer_kmap_put(buffer);
+   mutex_unlock(>lock);
+   }
  }
  
  static int ion_dma_buf_begin_cpu_access(struct dma_buf *dmabuf,

enum dma_data_direction direction)
  {
struct ion_buffer *buffer = dmabuf->priv;
-   void *vaddr;
struct ion_dma_buf_attachment *a;
-   int ret = 0;
-
-   /*
-* TODO: Move this elsewhere because we don't always need a vaddr
-*/
-   if (buffer->heap->ops->map_kernel) {
-   mutex_lock(>lock);
-   vaddr = ion_buffer_kmap_get(buffer);
-   if (IS_ERR(vaddr)) {
-   ret = PTR_ERR(vaddr);
-   goto unlock;
-   }
-   mutex_unlock(>lock);
-   }
  
  	mutex_lock(>lock);

list_for_each_entry(a, >attachments, list) {
dma_sync_sg_for_cpu(a->dev, a->table->sgl, a->table->nents,
direction);
}
-
-unlock:
mutex_unlock(>lock);
-   return ret;
+
+   return 0;
  }
  
  static int ion_dma_buf_end_cpu_access(struct dma_buf *dmabuf,

@@ -350,12 +352,6 @@ static int ion_dma_buf_end_cpu_access(struct dma_buf 
*dmabuf,
struct ion_buffer *buffer = dmabuf->priv;
struct ion_dma_buf_attachment *a;
  
-	if (buffer->heap->ops->map_kernel) {

-   mutex_lock(>lock);
-   ion_buffer_kmap_put(buffer);
-   mutex_unlock(>lock);
-   }
-
mutex_lock(>lock);
list_for_each_entry(a, >attachments, list) {
dma_sync_sg_for_device(a->dev, a->table->sgl, a->table->nents,



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

Re: [PATCH] staging: android: ion: add buffer flag update ioctl

[Laura Abbott]

On 12/23/18 6:47 PM, Zengtao (B) wrote:

Hi laura:


-Original Message-
From: Laura Abbott [mailto:labb...@redhat.com]
Sent: Friday, December 21, 2018 4:50 AM
To: Zengtao (B) ; sumit.sem...@linaro.org
Cc: Greg Kroah-Hartman ; Arve Hjønnevåg
; Todd Kjos ; Martijn Coenen
; Joel Fernandes ;
de...@driverdev.osuosl.org; dri-de...@lists.freedesktop.org;
linaro-mm-...@lists.linaro.org; linux-ker...@vger.kernel.org
Subject: Re: [PATCH] staging: android: ion: add buffer flag update ioctl

On 12/19/18 5:39 PM, Zengtao (B) wrote:

Hi laura:


-Original Message-
From: Laura Abbott [mailto:labb...@redhat.com]
Sent: Thursday, December 20, 2018 2:10 AM
To: Zengtao (B) ;

sumit.sem...@linaro.org

Cc: Greg Kroah-Hartman ; Arve

Hjønnevåg

; Todd Kjos ; Martijn

Coenen

; Joel Fernandes ;
de...@driverdev.osuosl.org; dri-de...@lists.freedesktop.org;
linaro-mm-...@lists.linaro.org; linux-ker...@vger.kernel.org
Subject: Re: [PATCH] staging: android: ion: add buffer flag update
ioctl

On 12/19/18 9:19 AM, Zeng Tao wrote:

In some usecases, the buffer cached attribute is not determined at
allocation time, it's determined just before the real cpu mapping.
And from the memory view of point, a buffer should not have the

cached

attribute util is really mapped by the cpu. So in this patch, we
introduced the new ioctl command to target the requirement.



This is racy and error prone. Can you explain more what problem you
are trying to solve?


My use case is like this:
1.  There are two process A and B, A takes case of ion buffer allocation,

and

   pass the buffer fd to B, then B maps and uses it.
2.  Process B need to map the buffer with different cached attribute
for different use case, for example, if the buffer is used for pure
software algorithm, then we need to map it as cached, otherwise
non-cached, and B needs to deal with both cases.
And unfortunately the mmap syscall takes no cached flags and we can't
decide the cache attribute when we are doing the mmap, so I introduce
new the ioctl even though I think the solution is not as good.




Thanks for the explanation, this was about the use case I expected.
I'm pretty sure I had this exact problem once upon a time and we didn't
come up with a solution. I'd still like to get rid of uncached buffers in
general and just use cached buffers (see
http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2018-N
ovember/128842.html)
What's your usecase for uncached buffers?


Some buffers are only used by HW, in this case, we tend to use uncached buffers.
But sometimes the SW need to read few buffer contents for debug purpose and
no synchronization is needed.



If this is primarily for debug, that's not really a compelling reason to
support uncached buffers. It's incredibly difficult to do uncached
buffers correctly I'd rather spend effort on making the cached
use cases work better.

Thanks,
Laura






Signed-off-by: Zeng Tao 
---
drivers/staging/android/ion/ion-ioctl.c |  4 
drivers/staging/android/ion/ion.c   | 17

+

drivers/staging/android/ion/ion.h   |  1 +
drivers/staging/android/uapi/ion.h  | 22

++

4 files changed, 44 insertions(+)

diff --git a/drivers/staging/android/ion/ion-ioctl.c
b/drivers/staging/android/ion/ion-ioctl.c
index a8d3cc4..60bb702 100644
--- a/drivers/staging/android/ion/ion-ioctl.c
+++ b/drivers/staging/android/ion/ion-ioctl.c
@@ -12,6 +12,7 @@

union ion_ioctl_arg {
struct ion_allocation_data allocation;
+   struct ion_buffer_flag_data update;
struct ion_heap_query query;
};

@@ -83,6 +84,9 @@ long ion_ioctl(struct file *filp, unsigned int
cmd, unsigned long arg)

break;
}
+   case ION_IOC_BUFFER_UPDATE:
+   ret = ion_buffer_update(data.update.fd, data.update.flags);
+   break;
case ION_IOC_HEAP_QUERY:
ret = ion_query_heaps();
break;
diff --git a/drivers/staging/android/ion/ion.c
b/drivers/staging/android/ion/ion.c
index 9907332..f1404dc 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -436,6 +436,23 @@ int ion_alloc(size_t len, unsigned int

heap_id_mask, unsigned int flags)

return fd;
}

+int ion_buffer_update(unsigned int fd, unsigned int flags) {
+   struct dma_buf *dmabuf;
+   struct ion_buffer *buffer;
+
+   dmabuf = dma_buf_get(fd);
+
+   if (!dmabuf)
+   return -EINVAL;
+
+   buffer = dmabuf->priv;
+   buffer->flags = flags;
+   dma_buf_put(dmabuf);
+
+   return 0;
+}
+
int ion_query_heaps(struct ion_heap_query *query)
{
struct ion_device *dev = internal_dev; diff --git
a/drivers/staging/android/ion/ion.h
b/drivers/staging/android/ion/ion.h
index c006fc1..99bf9ab 100644
--- a/drivers/staging/android/ion/ion.h
+++ b/drivers/staging/android/ion/ion.h
@@ -199,6 +199,7 @@ int ion_heap_pages_zero(struct page *page,

Re: [PATCH] staging: android: ion: add buffer flag update ioctl

[Laura Abbott]

On 12/19/18 5:39 PM, Zengtao (B) wrote:

Hi laura:


-Original Message-
From: Laura Abbott [mailto:labb...@redhat.com]
Sent: Thursday, December 20, 2018 2:10 AM
To: Zengtao (B) ; sumit.sem...@linaro.org
Cc: Greg Kroah-Hartman ; Arve Hjønnevåg
; Todd Kjos ; Martijn Coenen
; Joel Fernandes ;
de...@driverdev.osuosl.org; dri-de...@lists.freedesktop.org;
linaro-mm-...@lists.linaro.org; linux-ker...@vger.kernel.org
Subject: Re: [PATCH] staging: android: ion: add buffer flag update ioctl

On 12/19/18 9:19 AM, Zeng Tao wrote:

In some usecases, the buffer cached attribute is not determined at
allocation time, it's determined just before the real cpu mapping.
And from the memory view of point, a buffer should not have the

cached

attribute util is really mapped by the cpu. So in this patch, we
introduced the new ioctl command to target the requirement.



This is racy and error prone. Can you explain more what problem you are
trying to solve?


My use case is like this:
1.  There are two process A and B, A takes case of ion buffer allocation, and
  pass the buffer fd to B, then B maps and uses it.
2.  Process B need to map the buffer with different cached attribute for
different use case, for example, if the buffer is used for pure software 
algorithm,
then we need to map it as cached, otherwise non-cached, and B needs to deal
with both cases.
And unfortunately the mmap syscall takes no cached flags and we can't decide
the cache attribute when we are doing the mmap, so I introduce new the ioctl
even though I think the solution is not as good.




Thanks for the explanation, this was about the use case I expected.
I'm pretty sure I had this exact problem once upon a time and we
didn't come up with a solution. I'd still like to get rid of
uncached buffers in general and just use cached buffers
(see 
http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2018-November/128842.html)
What's your usecase for uncached buffers?




Signed-off-by: Zeng Tao 
---
   drivers/staging/android/ion/ion-ioctl.c |  4 
   drivers/staging/android/ion/ion.c   | 17 +
   drivers/staging/android/ion/ion.h   |  1 +
   drivers/staging/android/uapi/ion.h  | 22

++

   4 files changed, 44 insertions(+)

diff --git a/drivers/staging/android/ion/ion-ioctl.c
b/drivers/staging/android/ion/ion-ioctl.c
index a8d3cc4..60bb702 100644
--- a/drivers/staging/android/ion/ion-ioctl.c
+++ b/drivers/staging/android/ion/ion-ioctl.c
@@ -12,6 +12,7 @@

   union ion_ioctl_arg {
struct ion_allocation_data allocation;
+   struct ion_buffer_flag_data update;
struct ion_heap_query query;
   };

@@ -83,6 +84,9 @@ long ion_ioctl(struct file *filp, unsigned int cmd,
unsigned long arg)

break;
}
+   case ION_IOC_BUFFER_UPDATE:
+   ret = ion_buffer_update(data.update.fd, data.update.flags);
+   break;
case ION_IOC_HEAP_QUERY:
ret = ion_query_heaps();
break;
diff --git a/drivers/staging/android/ion/ion.c
b/drivers/staging/android/ion/ion.c
index 9907332..f1404dc 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -436,6 +436,23 @@ int ion_alloc(size_t len, unsigned int

heap_id_mask, unsigned int flags)

return fd;
   }

+int ion_buffer_update(unsigned int fd, unsigned int flags) {
+   struct dma_buf *dmabuf;
+   struct ion_buffer *buffer;
+
+   dmabuf = dma_buf_get(fd);
+
+   if (!dmabuf)
+   return -EINVAL;
+
+   buffer = dmabuf->priv;
+   buffer->flags = flags;
+   dma_buf_put(dmabuf);
+
+   return 0;
+}
+
   int ion_query_heaps(struct ion_heap_query *query)
   {
struct ion_device *dev = internal_dev; diff --git
a/drivers/staging/android/ion/ion.h
b/drivers/staging/android/ion/ion.h
index c006fc1..99bf9ab 100644
--- a/drivers/staging/android/ion/ion.h
+++ b/drivers/staging/android/ion/ion.h
@@ -199,6 +199,7 @@ int ion_heap_pages_zero(struct page *page,

size_t size, pgprot_t pgprot);

   int ion_alloc(size_t len,
  unsigned int heap_id_mask,
  unsigned int flags);
+int ion_buffer_update(unsigned int fd, unsigned int flags);

   /**
* ion_heap_init_shrinker
diff --git a/drivers/staging/android/uapi/ion.h
b/drivers/staging/android/uapi/ion.h
index 5d70098..99753fc 100644
--- a/drivers/staging/android/uapi/ion.h
+++ b/drivers/staging/android/uapi/ion.h
@@ -74,6 +74,20 @@ struct ion_allocation_data {
__u32 unused;
   };

+/**
+ * struct ion_buffer_flag_data - metadata passed from userspace for
+update
+ * buffer flags
+ * @fd:file descriptor of the buffer
+ * @flags: flags passed to the buffer
+ *
+ * Provided by userspace as an argument to the ioctl  */
+
+struct ion_buffer_flag_data {
+   __u32 fd;
+   __u32 flags;
+}
+
   #define MAX_HEAP_NAME32

   /**
@@ -116,6 +130,14 @@ 

Re: [PATCH] staging: android: ion: Add chunk heaps instantiation

[Laura Abbott]

On 12/16/18 2:46 AM, Alexey Skidanov wrote:

Chunk heap instantiation should be supported for device tree platforms and
non device tree platforms. For device tree platforms, it's a platform
specific code responsibility to retrieve the heap configuration data
and to call the appropriate API for heap creation. For non device tree
platforms, there is no defined way to create the heaps.

This patch provides the way of chunk heaps creation using
"ion_chunk_heap=name:size@start" kernel boot parameter.



I've been thinking about this and I think it works but
I'm still kind of torn about not having devicetree bindings.
It doesn't _preclude_ devicetree bindings but I'd hate to
merge this and then end up with something incompatible.
I do want to support non-Android use cases too.

I'm also curious about the value of this heap with just PAGE_SIZE.
The original purpose of the chunk heap was a carveout where
you could easily get allocations large than PAGE_SIZE to
reduce TLB pressure. Do you have another use case for the
chunk heap?

Sumit, do you have any thoughts?

Thanks,
Laura


Link: 
http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2018-November/128495.html
Signed-off-by: Alexey Skidanov 
---
  drivers/staging/android/ion/ion_chunk_heap.c | 96 +---
  include/linux/ion.h  | 18 ++
  2 files changed, 105 insertions(+), 9 deletions(-)
  create mode 100644 include/linux/ion.h

diff --git a/drivers/staging/android/ion/ion_chunk_heap.c 
b/drivers/staging/android/ion/ion_chunk_heap.c
index 102c093..1a8e3ad 100644
--- a/drivers/staging/android/ion/ion_chunk_heap.c
+++ b/drivers/staging/android/ion/ion_chunk_heap.c
@@ -21,8 +21,12 @@
  #include 
  #include 
  #include 
+#include 
  #include "ion.h"
  
+static struct ion_chunk_heap_cfg chunk_heap_cfg[MAX_NUM_OF_CHUNK_HEAPS];

+static unsigned int num_of_req_chunk_heaps;
+
  struct ion_chunk_heap {
struct ion_heap heap;
struct gen_pool *pool;
@@ -117,15 +121,15 @@ static struct ion_heap_ops chunk_heap_ops = {
.unmap_kernel = ion_heap_unmap_kernel,
  };
  
-struct ion_heap *ion_chunk_heap_create(struct ion_platform_heap *heap_data)

+static struct ion_heap *ion_chunk_heap_create(struct ion_chunk_heap_cfg 
*heap_cfg)
  {
struct ion_chunk_heap *chunk_heap;
int ret;
struct page *page;
size_t size;
  
-	page = pfn_to_page(PFN_DOWN(heap_data->base));

-   size = heap_data->size;
+   page = pfn_to_page(PFN_DOWN(heap_cfg->base));
+   size = heap_cfg->size;
  
  	ret = ion_heap_pages_zero(page, size, pgprot_writecombine(PAGE_KERNEL));

if (ret)
@@ -135,23 +139,27 @@ struct ion_heap *ion_chunk_heap_create(struct 
ion_platform_heap *heap_data)
if (!chunk_heap)
return ERR_PTR(-ENOMEM);
  
-	chunk_heap->chunk_size = (unsigned long)heap_data->priv;

+   chunk_heap->chunk_size = heap_cfg->chunk_size;
chunk_heap->pool = gen_pool_create(get_order(chunk_heap->chunk_size) +
   PAGE_SHIFT, -1);
if (!chunk_heap->pool) {
ret = -ENOMEM;
goto error_gen_pool_create;
}
-   chunk_heap->base = heap_data->base;
-   chunk_heap->size = heap_data->size;
+   chunk_heap->base = heap_cfg->base;
+   chunk_heap->size = heap_cfg->size;
+   chunk_heap->heap.name = heap_cfg->heap_name;
chunk_heap->allocated = 0;
  
-	gen_pool_add(chunk_heap->pool, chunk_heap->base, heap_data->size, -1);

+   gen_pool_add(chunk_heap->pool, chunk_heap->base, heap_cfg->size, -1);
chunk_heap->heap.ops = _heap_ops;
chunk_heap->heap.type = ION_HEAP_TYPE_CHUNK;
chunk_heap->heap.flags = ION_HEAP_FLAG_DEFER_FREE;
-   pr_debug("%s: base %pa size %zu\n", __func__,
-_heap->base, heap_data->size);
+
+   pr_info("%s: name %s base %pa size %zu\n", __func__,
+   heap_cfg->heap_name,
+   _cfg->base,
+   heap_cfg->size);
  
  	return _heap->heap;
  
@@ -160,3 +168,73 @@ struct ion_heap *ion_chunk_heap_create(struct ion_platform_heap *heap_data)

return ERR_PTR(ret);
  }
  
+static int __init setup_heap(char *param)

+{
+   char *at_sign, *coma, *colon;
+   size_t size_to_copy;
+   struct ion_chunk_heap_cfg *cfg;
+
+   do {
+   cfg = _heap_cfg[num_of_req_chunk_heaps];
+
+   /* heap name */
+   colon = strchr(param, ':');
+   if (!colon)
+   return -EINVAL;
+
+   size_to_copy = min_t(size_t, MAX_CHUNK_HEAP_NAME_SIZE - 1,
+(colon - param));
+   strncpy(cfg->heap_name,  param, size_to_copy);
+   cfg->heap_name[size_to_copy] = '\0';
+
+   /* heap size */
+   cfg->size = memparse((colon + 1), _sign);
+   if ((colon + 1) == at_sign)
+   return -EINVAL;
+

Re: [PATCH] staging: android: ion: add buffer flag update ioctl

[Laura Abbott]

On 12/19/18 9:19 AM, Zeng Tao wrote:

In some usecases, the buffer cached attribute is not determined at
allocation time, it's determined just before the real cpu mapping.
And from the memory view of point, a buffer should not have the cached
attribute util is really mapped by the cpu. So in this patch, we
introduced the new ioctl command to target the requirement.



This is racy and error prone. Can you explain more what
problem you are trying to solve?


Signed-off-by: Zeng Tao 
---
  drivers/staging/android/ion/ion-ioctl.c |  4 
  drivers/staging/android/ion/ion.c   | 17 +
  drivers/staging/android/ion/ion.h   |  1 +
  drivers/staging/android/uapi/ion.h  | 22 ++
  4 files changed, 44 insertions(+)

diff --git a/drivers/staging/android/ion/ion-ioctl.c 
b/drivers/staging/android/ion/ion-ioctl.c
index a8d3cc4..60bb702 100644
--- a/drivers/staging/android/ion/ion-ioctl.c
+++ b/drivers/staging/android/ion/ion-ioctl.c
@@ -12,6 +12,7 @@
  
  union ion_ioctl_arg {

struct ion_allocation_data allocation;
+   struct ion_buffer_flag_data update;
struct ion_heap_query query;
  };
  
@@ -83,6 +84,9 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
  
  		break;

}
+   case ION_IOC_BUFFER_UPDATE:
+   ret = ion_buffer_update(data.update.fd, data.update.flags);
+   break;
case ION_IOC_HEAP_QUERY:
ret = ion_query_heaps();
break;
diff --git a/drivers/staging/android/ion/ion.c 
b/drivers/staging/android/ion/ion.c
index 9907332..f1404dc 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -436,6 +436,23 @@ int ion_alloc(size_t len, unsigned int heap_id_mask, 
unsigned int flags)
return fd;
  }
  
+int ion_buffer_update(unsigned int fd, unsigned int flags)

+{
+   struct dma_buf *dmabuf;
+   struct ion_buffer *buffer;
+
+   dmabuf = dma_buf_get(fd);
+
+   if (!dmabuf)
+   return -EINVAL;
+
+   buffer = dmabuf->priv;
+   buffer->flags = flags;
+   dma_buf_put(dmabuf);
+
+   return 0;
+}
+
  int ion_query_heaps(struct ion_heap_query *query)
  {
struct ion_device *dev = internal_dev;
diff --git a/drivers/staging/android/ion/ion.h 
b/drivers/staging/android/ion/ion.h
index c006fc1..99bf9ab 100644
--- a/drivers/staging/android/ion/ion.h
+++ b/drivers/staging/android/ion/ion.h
@@ -199,6 +199,7 @@ int ion_heap_pages_zero(struct page *page, size_t size, 
pgprot_t pgprot);
  int ion_alloc(size_t len,
  unsigned int heap_id_mask,
  unsigned int flags);
+int ion_buffer_update(unsigned int fd, unsigned int flags);
  
  /**

   * ion_heap_init_shrinker
diff --git a/drivers/staging/android/uapi/ion.h 
b/drivers/staging/android/uapi/ion.h
index 5d70098..99753fc 100644
--- a/drivers/staging/android/uapi/ion.h
+++ b/drivers/staging/android/uapi/ion.h
@@ -74,6 +74,20 @@ struct ion_allocation_data {
__u32 unused;
  };
  
+/**

+ * struct ion_buffer_flag_data - metadata passed from userspace for update
+ * buffer flags
+ * @fd:file descriptor of the buffer
+ * @flags: flags passed to the buffer
+ *
+ * Provided by userspace as an argument to the ioctl
+ */
+
+struct ion_buffer_flag_data {
+   __u32 fd;
+   __u32 flags;
+}
+
  #define MAX_HEAP_NAME 32
  
  /**

@@ -116,6 +130,14 @@ struct ion_heap_query {
  struct ion_allocation_data)
  
  /**

+ * DOC: ION_IOC_BUFFER_UPDATE - update the specified ion buffer flags
+ *
+ * Takes an ion_buffer_flag_data structure and returns the result of the
+ * buffer flag update operation.
+ */
+#define ION_IOC_BUFFER_UPDATE  _IOWR(ION_IOC_MAGIC, 1, \
+ struct ion_buffer_flag_data)
+/**
   * DOC: ION_IOC_HEAP_QUERY - information about available heaps
   *
   * Takes an ion_heap_query structure and populates information about



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

Re: [PATCH] staging: android: ion: Remove unused header files

[Laura Abbott]

On 11/30/18 6:43 AM, Yangtao Li wrote:

seq_file.h does not need to be included,so remove it.



Acked-by: Laura Abbott 


Signed-off-by: Yangtao Li 
---
  drivers/staging/android/ion/ion.c | 1 -
  drivers/staging/android/ion/ion_system_heap.c | 1 -
  2 files changed, 2 deletions(-)

diff --git a/drivers/staging/android/ion/ion.c 
b/drivers/staging/android/ion/ion.c
index 99073325b0c0..0d61e9cd0887 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -23,7 +23,6 @@
  #include 
  #include 
  #include 
-#include 
  #include 
  #include 
  #include 
diff --git a/drivers/staging/android/ion/ion_system_heap.c 
b/drivers/staging/android/ion/ion_system_heap.c
index 548bb02c0ca6..9ce2c0d7ac17 100644
--- a/drivers/staging/android/ion/ion_system_heap.c
+++ b/drivers/staging/android/ion/ion_system_heap.c
@@ -11,7 +11,6 @@
  #include 
  #include 
  #include 
-#include 
  #include 
  #include 
  #include "ion.h"



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

Re: [PATCH] staging: android: ion: Add chunk heap initialization

[Laura Abbott]

On 11/27/18 12:07 PM, Alexey Skidanov wrote:



On 11/27/18 9:20 PM, Laura Abbott wrote:

On 11/26/18 10:43 AM, Alexey Skidanov wrote:



On 11/26/18 6:39 PM, Laura Abbott wrote:

On 11/25/18 2:02 PM, Alexey Skidanov wrote:



On 11/25/18 11:40 PM, Laura Abbott wrote:

On 11/25/18 1:22 PM, Alexey Skidanov wrote:



On 11/25/18 10:51 PM, Laura Abbott wrote:

On 11/11/18 11:29 AM, Alexey Skidanov wrote:

Create chunk heap of specified size and base address by adding
"ion_chunk_heap=size@start" kernel boot parameter.

Signed-off-by: Alexey Skidanov 
---
  drivers/staging/android/ion/ion_chunk_heap.c | 40

  1 file changed, 40 insertions(+)

diff --git a/drivers/staging/android/ion/ion_chunk_heap.c
b/drivers/staging/android/ion/ion_chunk_heap.c
index 159d72f..67573aa4 100644
--- a/drivers/staging/android/ion/ion_chunk_heap.c
+++ b/drivers/staging/android/ion/ion_chunk_heap.c
@@ -135,6 +135,7 @@ struct ion_heap *ion_chunk_heap_create(struct
ion_platform_heap *heap_data)
  }
  chunk_heap->base = heap_data->base;
  chunk_heap->size = heap_data->size;
+    chunk_heap->heap.name = heap_data->name;
  chunk_heap->allocated = 0;
    gen_pool_add(chunk_heap->pool, chunk_heap->base,
heap_data->size, -1);
@@ -151,3 +152,42 @@ struct ion_heap *ion_chunk_heap_create(struct
ion_platform_heap *heap_data)
  return ERR_PTR(ret);
  }
  +static u64 base;
+static u64 size;
+
+static int __init setup_heap(char *param)
+{
+    char *p, *pp;
+
+    size = memparse(param, );
+    if (param == p)
+    return -EINVAL;
+
+    if (*p == '@')
+    base = memparse(p + 1, );
+    else
+    return -EINVAL;
+
+    if (p == pp)
+    return -EINVAL;
+
+    return 0;
+}
+
+__setup("ion_chunk_heap=", setup_heap);
+
+static int ion_add_chunk_heap(void)
+{
+    struct ion_heap *heap;
+    struct ion_platform_heap plat_heap = {.base = base,
+  .size = size,
+  .name = "chunk_heap",
+  .priv = (void *)PAGE_SIZE};
+    heap = ion_chunk_heap_create(_heap);
+    if (heap)
+    ion_device_add_heap(heap);
+
+    return 0;
+}
+device_initcall(ion_add_chunk_heap);
+



This solves a problem but not enough of the problem.

We need to be able to support more than one chunk/carveout
heap.

This is easy to support.
This also assumes that the memory has already been

reserved/placed and that you know the base and size to
pass on the command line. Part of the issue with the carveout
heaps is that we need a way to tell the kernel to reserve
the memory early enough and then get that information to
Ion. Hard coding memory locations tends to be buggy from
my past experience with Ion.

memmap= kernel option marks the memory region(s) as reserved (Zone
Allocator doesn't use this memory region(s)). So the heap(s) may
manage
this memory region(s).


memmap= is x86 only. I really don't like using the command line for
specifying the base/size as it seems likely to conflict with platforms
that rely on devicetree for reserving memory regions.

Thanks,
Laura


I see ... So probably the better way is the one similar to this
https://elixir.bootlin.com/linux/latest/source/kernel/dma/contiguous.c#L245


?



Correct. For platforms that need devicetree, we need a way to specify
that a region should become an Ion heap. I went through a similar
exercise with CMA heaps before I kind of gave up on figuring out a
binding and just had Ion enumerate all CMA heaps. We do still need
a solution to work with non-DT platforms as well so whatever we
come up with needs to plausibly work for both cases. Your solution
would cover the non-DT case but I'd really like to make sure we
at least have a path forward for the devicetree case as well.


I would say that we have the following steps to consider:

1. Memory reservation. The suggested solution doesn't care how it's done.

2. Per-heap information passing to the Kernel. It's different for DT and
non-DT cases.

3. Heap objects instantiation. The DT and non-DT cases have different
ways/formats to pass this per-heap information. But once the parsing is
done, the rest of the code is common.

I think it clearly defines the steps covering both cases. What do you
think?



Yes, that sounds about right.



So, in this patch step #2 - is setup_heap() and step #3 - is
ion_add_chunk_heap(). The only missing part is to support several heap
instances creation, correct?



Mostly yes. I'd like to see struct ion_platform_heap go away since
it really isn't used for anything else but we need another
way to get the reserved memory information into Ion.

Thanks,
Laura


Thanks,
Alexey



Thanks,
Alexey


Thanks,
Laura


Thanks,
Alexey



If you'd like to see about coming up with a complete solution,
feel free to resubmit but I'm still strongly considering
removing these heaps.


I will add the multiple heaps s

Re: [PATCH] staging: android: ion: Add chunk heap initialization

[Laura Abbott]

On 11/26/18 10:43 AM, Alexey Skidanov wrote:



On 11/26/18 6:39 PM, Laura Abbott wrote:

On 11/25/18 2:02 PM, Alexey Skidanov wrote:



On 11/25/18 11:40 PM, Laura Abbott wrote:

On 11/25/18 1:22 PM, Alexey Skidanov wrote:



On 11/25/18 10:51 PM, Laura Abbott wrote:

On 11/11/18 11:29 AM, Alexey Skidanov wrote:

Create chunk heap of specified size and base address by adding
"ion_chunk_heap=size@start" kernel boot parameter.

Signed-off-by: Alexey Skidanov 
---
     drivers/staging/android/ion/ion_chunk_heap.c | 40

     1 file changed, 40 insertions(+)

diff --git a/drivers/staging/android/ion/ion_chunk_heap.c
b/drivers/staging/android/ion/ion_chunk_heap.c
index 159d72f..67573aa4 100644
--- a/drivers/staging/android/ion/ion_chunk_heap.c
+++ b/drivers/staging/android/ion/ion_chunk_heap.c
@@ -135,6 +135,7 @@ struct ion_heap *ion_chunk_heap_create(struct
ion_platform_heap *heap_data)
     }
     chunk_heap->base = heap_data->base;
     chunk_heap->size = heap_data->size;
+    chunk_heap->heap.name = heap_data->name;
     chunk_heap->allocated = 0;
       gen_pool_add(chunk_heap->pool, chunk_heap->base,
heap_data->size, -1);
@@ -151,3 +152,42 @@ struct ion_heap *ion_chunk_heap_create(struct
ion_platform_heap *heap_data)
     return ERR_PTR(ret);
     }
     +static u64 base;
+static u64 size;
+
+static int __init setup_heap(char *param)
+{
+    char *p, *pp;
+
+    size = memparse(param, );
+    if (param == p)
+    return -EINVAL;
+
+    if (*p == '@')
+    base = memparse(p + 1, );
+    else
+    return -EINVAL;
+
+    if (p == pp)
+    return -EINVAL;
+
+    return 0;
+}
+
+__setup("ion_chunk_heap=", setup_heap);
+
+static int ion_add_chunk_heap(void)
+{
+    struct ion_heap *heap;
+    struct ion_platform_heap plat_heap = {.base = base,
+  .size = size,
+  .name = "chunk_heap",
+  .priv = (void *)PAGE_SIZE};
+    heap = ion_chunk_heap_create(_heap);
+    if (heap)
+    ion_device_add_heap(heap);
+
+    return 0;
+}
+device_initcall(ion_add_chunk_heap);
+



This solves a problem but not enough of the problem.

We need to be able to support more than one chunk/carveout
heap.

This is easy to support.
This also assumes that the memory has already been

reserved/placed and that you know the base and size to
pass on the command line. Part of the issue with the carveout
heaps is that we need a way to tell the kernel to reserve
the memory early enough and then get that information to
Ion. Hard coding memory locations tends to be buggy from
my past experience with Ion.

memmap= kernel option marks the memory region(s) as reserved (Zone
Allocator doesn't use this memory region(s)). So the heap(s) may manage
this memory region(s).


memmap= is x86 only. I really don't like using the command line for
specifying the base/size as it seems likely to conflict with platforms
that rely on devicetree for reserving memory regions.

Thanks,
Laura


I see ... So probably the better way is the one similar to this
https://elixir.bootlin.com/linux/latest/source/kernel/dma/contiguous.c#L245

?



Correct. For platforms that need devicetree, we need a way to specify
that a region should become an Ion heap. I went through a similar
exercise with CMA heaps before I kind of gave up on figuring out a
binding and just had Ion enumerate all CMA heaps. We do still need
a solution to work with non-DT platforms as well so whatever we
come up with needs to plausibly work for both cases. Your solution
would cover the non-DT case but I'd really like to make sure we
at least have a path forward for the devicetree case as well.


I would say that we have the following steps to consider:

1. Memory reservation. The suggested solution doesn't care how it's done.

2. Per-heap information passing to the Kernel. It's different for DT and
non-DT cases.

3. Heap objects instantiation. The DT and non-DT cases have different
ways/formats to pass this per-heap information. But once the parsing is
done, the rest of the code is common.

I think it clearly defines the steps covering both cases. What do you think?



Yes, that sounds about right.


Thanks,
Alexey


Thanks,
Laura


Thanks,
Alexey



If you'd like to see about coming up with a complete solution,
feel free to resubmit but I'm still strongly considering
removing these heaps.


I will add the multiple heaps support and resubmit the patch

Thanks,
Laura

Thanks,
Alexey







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

Re: [PATCH] staging: android: ion: Add chunk heap initialization

[Laura Abbott]

On 11/25/18 2:02 PM, Alexey Skidanov wrote:



On 11/25/18 11:40 PM, Laura Abbott wrote:

On 11/25/18 1:22 PM, Alexey Skidanov wrote:



On 11/25/18 10:51 PM, Laura Abbott wrote:

On 11/11/18 11:29 AM, Alexey Skidanov wrote:

Create chunk heap of specified size and base address by adding
"ion_chunk_heap=size@start" kernel boot parameter.

Signed-off-by: Alexey Skidanov 
---
    drivers/staging/android/ion/ion_chunk_heap.c | 40

    1 file changed, 40 insertions(+)

diff --git a/drivers/staging/android/ion/ion_chunk_heap.c
b/drivers/staging/android/ion/ion_chunk_heap.c
index 159d72f..67573aa4 100644
--- a/drivers/staging/android/ion/ion_chunk_heap.c
+++ b/drivers/staging/android/ion/ion_chunk_heap.c
@@ -135,6 +135,7 @@ struct ion_heap *ion_chunk_heap_create(struct
ion_platform_heap *heap_data)
    }
    chunk_heap->base = heap_data->base;
    chunk_heap->size = heap_data->size;
+    chunk_heap->heap.name = heap_data->name;
    chunk_heap->allocated = 0;
      gen_pool_add(chunk_heap->pool, chunk_heap->base,
heap_data->size, -1);
@@ -151,3 +152,42 @@ struct ion_heap *ion_chunk_heap_create(struct
ion_platform_heap *heap_data)
    return ERR_PTR(ret);
    }
    +static u64 base;
+static u64 size;
+
+static int __init setup_heap(char *param)
+{
+    char *p, *pp;
+
+    size = memparse(param, );
+    if (param == p)
+    return -EINVAL;
+
+    if (*p == '@')
+    base = memparse(p + 1, );
+    else
+    return -EINVAL;
+
+    if (p == pp)
+    return -EINVAL;
+
+    return 0;
+}
+
+__setup("ion_chunk_heap=", setup_heap);
+
+static int ion_add_chunk_heap(void)
+{
+    struct ion_heap *heap;
+    struct ion_platform_heap plat_heap = {.base = base,
+  .size = size,
+  .name = "chunk_heap",
+  .priv = (void *)PAGE_SIZE};
+    heap = ion_chunk_heap_create(_heap);
+    if (heap)
+    ion_device_add_heap(heap);
+
+    return 0;
+}
+device_initcall(ion_add_chunk_heap);
+



This solves a problem but not enough of the problem.

We need to be able to support more than one chunk/carveout
heap.

This is easy to support.
This also assumes that the memory has already been

reserved/placed and that you know the base and size to
pass on the command line. Part of the issue with the carveout
heaps is that we need a way to tell the kernel to reserve
the memory early enough and then get that information to
Ion. Hard coding memory locations tends to be buggy from
my past experience with Ion.

memmap= kernel option marks the memory region(s) as reserved (Zone
Allocator doesn't use this memory region(s)). So the heap(s) may manage
this memory region(s).


memmap= is x86 only. I really don't like using the command line for
specifying the base/size as it seems likely to conflict with platforms
that rely on devicetree for reserving memory regions.

Thanks,
Laura


I see ... So probably the better way is the one similar to this
https://elixir.bootlin.com/linux/latest/source/kernel/dma/contiguous.c#L245
?



Correct. For platforms that need devicetree, we need a way to specify
that a region should become an Ion heap. I went through a similar
exercise with CMA heaps before I kind of gave up on figuring out a
binding and just had Ion enumerate all CMA heaps. We do still need
a solution to work with non-DT platforms as well so whatever we
come up with needs to plausibly work for both cases. Your solution
would cover the non-DT case but I'd really like to make sure we
at least have a path forward for the devicetree case as well.

Thanks,
Laura


Thanks,
Alexey



If you'd like to see about coming up with a complete solution,
feel free to resubmit but I'm still strongly considering
removing these heaps.


I will add the multiple heaps support and resubmit the patch

Thanks,
Laura

Thanks,
Alexey





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

Re: [PATCH] staging: android: ion: Add chunk heap initialization

[Laura Abbott]

On 11/25/18 1:22 PM, Alexey Skidanov wrote:



On 11/25/18 10:51 PM, Laura Abbott wrote:

On 11/11/18 11:29 AM, Alexey Skidanov wrote:

Create chunk heap of specified size and base address by adding
"ion_chunk_heap=size@start" kernel boot parameter.

Signed-off-by: Alexey Skidanov 
---
   drivers/staging/android/ion/ion_chunk_heap.c | 40

   1 file changed, 40 insertions(+)

diff --git a/drivers/staging/android/ion/ion_chunk_heap.c
b/drivers/staging/android/ion/ion_chunk_heap.c
index 159d72f..67573aa4 100644
--- a/drivers/staging/android/ion/ion_chunk_heap.c
+++ b/drivers/staging/android/ion/ion_chunk_heap.c
@@ -135,6 +135,7 @@ struct ion_heap *ion_chunk_heap_create(struct
ion_platform_heap *heap_data)
   }
   chunk_heap->base = heap_data->base;
   chunk_heap->size = heap_data->size;
+    chunk_heap->heap.name = heap_data->name;
   chunk_heap->allocated = 0;
     gen_pool_add(chunk_heap->pool, chunk_heap->base,
heap_data->size, -1);
@@ -151,3 +152,42 @@ struct ion_heap *ion_chunk_heap_create(struct
ion_platform_heap *heap_data)
   return ERR_PTR(ret);
   }
   +static u64 base;
+static u64 size;
+
+static int __init setup_heap(char *param)
+{
+    char *p, *pp;
+
+    size = memparse(param, );
+    if (param == p)
+    return -EINVAL;
+
+    if (*p == '@')
+    base = memparse(p + 1, );
+    else
+    return -EINVAL;
+
+    if (p == pp)
+    return -EINVAL;
+
+    return 0;
+}
+
+__setup("ion_chunk_heap=", setup_heap);
+
+static int ion_add_chunk_heap(void)
+{
+    struct ion_heap *heap;
+    struct ion_platform_heap plat_heap = {.base = base,
+  .size = size,
+  .name = "chunk_heap",
+  .priv = (void *)PAGE_SIZE};
+    heap = ion_chunk_heap_create(_heap);
+    if (heap)
+    ion_device_add_heap(heap);
+
+    return 0;
+}
+device_initcall(ion_add_chunk_heap);
+



This solves a problem but not enough of the problem.

We need to be able to support more than one chunk/carveout
heap.

This is easy to support.
This also assumes that the memory has already been

reserved/placed and that you know the base and size to
pass on the command line. Part of the issue with the carveout
heaps is that we need a way to tell the kernel to reserve
the memory early enough and then get that information to
Ion. Hard coding memory locations tends to be buggy from
my past experience with Ion.

memmap= kernel option marks the memory region(s) as reserved (Zone
Allocator doesn't use this memory region(s)). So the heap(s) may manage
this memory region(s).


memmap= is x86 only. I really don't like using the command line for
specifying the base/size as it seems likely to conflict with platforms
that rely on devicetree for reserving memory regions.

Thanks,
Laura



If you'd like to see about coming up with a complete solution,
feel free to resubmit but I'm still strongly considering
removing these heaps.


I will add the multiple heaps support and resubmit the patch

Thanks,
Laura

Thanks,
Alexey



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

Re: [PATCH] staging: android: ion: Add chunk heap initialization

[Laura Abbott]

On 11/11/18 11:29 AM, Alexey Skidanov wrote:

Create chunk heap of specified size and base address by adding
"ion_chunk_heap=size@start" kernel boot parameter.

Signed-off-by: Alexey Skidanov 
---
  drivers/staging/android/ion/ion_chunk_heap.c | 40 
  1 file changed, 40 insertions(+)

diff --git a/drivers/staging/android/ion/ion_chunk_heap.c 
b/drivers/staging/android/ion/ion_chunk_heap.c
index 159d72f..67573aa4 100644
--- a/drivers/staging/android/ion/ion_chunk_heap.c
+++ b/drivers/staging/android/ion/ion_chunk_heap.c
@@ -135,6 +135,7 @@ struct ion_heap *ion_chunk_heap_create(struct 
ion_platform_heap *heap_data)
}
chunk_heap->base = heap_data->base;
chunk_heap->size = heap_data->size;
+   chunk_heap->heap.name = heap_data->name;
chunk_heap->allocated = 0;
  
  	gen_pool_add(chunk_heap->pool, chunk_heap->base, heap_data->size, -1);

@@ -151,3 +152,42 @@ struct ion_heap *ion_chunk_heap_create(struct 
ion_platform_heap *heap_data)
return ERR_PTR(ret);
  }
  
+static u64 base;

+static u64 size;
+
+static int __init setup_heap(char *param)
+{
+   char *p, *pp;
+
+   size = memparse(param, );
+   if (param == p)
+   return -EINVAL;
+
+   if (*p == '@')
+   base = memparse(p + 1, );
+   else
+   return -EINVAL;
+
+   if (p == pp)
+   return -EINVAL;
+
+   return 0;
+}
+
+__setup("ion_chunk_heap=", setup_heap);
+
+static int ion_add_chunk_heap(void)
+{
+   struct ion_heap *heap;
+   struct ion_platform_heap plat_heap = {.base = base,
+ .size = size,
+ .name = "chunk_heap",
+ .priv = (void *)PAGE_SIZE};
+   heap = ion_chunk_heap_create(_heap);
+   if (heap)
+   ion_device_add_heap(heap);
+
+   return 0;
+}
+device_initcall(ion_add_chunk_heap);
+



This solves a problem but not enough of the problem.

We need to be able to support more than one chunk/carveout
heap. This also assumes that the memory has already been
reserved/placed and that you know the base and size to
pass on the command line. Part of the issue with the carveout
heaps is that we need a way to tell the kernel to reserve
the memory early enough and then get that information to
Ion. Hard coding memory locations tends to be buggy from
my past experience with Ion.

If you'd like to see about coming up with a complete solution,
feel free to resubmit but I'm still strongly considering
removing these heaps.

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

Re: [PATCH] Add carve out heap name initialization

[Laura Abbott]

On 11/8/18 11:34 AM, Alexey Skidanov wrote:



On 11/8/18 9:15 PM, Laura Abbott wrote:

On 10/22/18 2:15 PM, Alexey Skidanov wrote:

Heap name is mundatory.



I'm wary of this and the other change because it misses the
broader problem of dealing with the carveout heaps.
I still want to remove the carveout and chunk heap. I get
that it's being used for out of tree work but at this point
the focus needs to be on moving Ion out of staging and
if we can't get an end-to-end solution for carveout/chunk
heaps to be allocated in tree this needs to be removed.

Thanks,
Laura

There are several options I would suggest:
1. The heaps may be initialized by parsing some kernel parameter,
defining the contiguous chunks
2. Some functions may be exported by ION to initialize the heaps
3. CONFIG_XXX options (just like it's in CMA)



We've had lots of suggestions but nobody has actually stepped
up to submit patches to make this work. If you'd like to submit
patches that would be great.

Thanks,
Laura


Thanks,
Alexey




Signed-off-by: Alexey Skidanov 
---
   drivers/staging/android/ion/ion_carveout_heap.c | 1 +
   1 file changed, 1 insertion(+)

diff --git a/drivers/staging/android/ion/ion_carveout_heap.c
b/drivers/staging/android/ion/ion_carveout_heap.c
index e129237..e89b464 100644
--- a/drivers/staging/android/ion/ion_carveout_heap.c
+++ b/drivers/staging/android/ion/ion_carveout_heap.c
@@ -131,6 +131,7 @@ struct ion_heap *ion_carveout_heap_create(struct
ion_platform_heap *heap_data)
   gen_pool_add(carveout_heap->pool, carveout_heap->base,
heap_data->size,
    -1);
   carveout_heap->heap.ops = _heap_ops;
+    carveout_heap->heap.name = heap_data->name;
   carveout_heap->heap.type = ION_HEAP_TYPE_CARVEOUT;
   carveout_heap->heap.flags = ION_HEAP_FLAG_DEFER_FREE;
  




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

Re: [PATCH] Add carve out heap name initialization

[Laura Abbott]

On 10/22/18 2:15 PM, Alexey Skidanov wrote:

Heap name is mundatory.



I'm wary of this and the other change because it misses the
broader problem of dealing with the carveout heaps.
I still want to remove the carveout and chunk heap. I get
that it's being used for out of tree work but at this point
the focus needs to be on moving Ion out of staging and
if we can't get an end-to-end solution for carveout/chunk
heaps to be allocated in tree this needs to be removed.

Thanks,
Laura


Signed-off-by: Alexey Skidanov 
---
  drivers/staging/android/ion/ion_carveout_heap.c | 1 +
  1 file changed, 1 insertion(+)

diff --git a/drivers/staging/android/ion/ion_carveout_heap.c 
b/drivers/staging/android/ion/ion_carveout_heap.c
index e129237..e89b464 100644
--- a/drivers/staging/android/ion/ion_carveout_heap.c
+++ b/drivers/staging/android/ion/ion_carveout_heap.c
@@ -131,6 +131,7 @@ struct ion_heap *ion_carveout_heap_create(struct 
ion_platform_heap *heap_data)
gen_pool_add(carveout_heap->pool, carveout_heap->base, heap_data->size,
 -1);
carveout_heap->heap.ops = _heap_ops;
+   carveout_heap->heap.name = heap_data->name;
carveout_heap->heap.type = ION_HEAP_TYPE_CARVEOUT;
carveout_heap->heap.flags = ION_HEAP_FLAG_DEFER_FREE;
  



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

Re: [PATCH v5] staging: android: ion: Add per-heap counters

[Laura Abbott]

On 10/9/18 12:08 PM, Alexey Skidanov wrote:

Heap statistics have been removed and currently even basics statistics
are missing.

This patch creates per heap debugfs directory /sys/kernel/debug/
and adds the following counters:
- the number of allocated buffers;
- the number of allocated bytes;
- the number of allocated bytes watermark.



Acked-by: Laura Abbott 


Signed-off-by: Alexey Skidanov 
---

v3:
Removed debugfs_create_dir() return value checking
v4:
Added spinlock to protect heap statistics
v5:
Rebased on staging-next

  drivers/staging/android/ion/ion.c | 50 ---
  drivers/staging/android/ion/ion.h |  9 +++
  2 files changed, 51 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/android/ion/ion.c 
b/drivers/staging/android/ion/ion.c
index 9907332..6fd8979 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -95,6 +95,13 @@ static struct ion_buffer *ion_buffer_create(struct ion_heap 
*heap,
goto err1;
}
  
+	spin_lock(>stat_lock);

+   heap->num_of_buffers++;
+   heap->num_of_alloc_bytes += len;
+   if (heap->num_of_alloc_bytes > heap->alloc_bytes_wm)
+   heap->alloc_bytes_wm = heap->num_of_alloc_bytes;
+   spin_unlock(>stat_lock);
+
INIT_LIST_HEAD(>attachments);
mutex_init(>lock);
mutex_lock(>buffer_lock);
@@ -117,6 +124,11 @@ void ion_buffer_destroy(struct ion_buffer *buffer)
buffer->heap->ops->unmap_kernel(buffer->heap, buffer);
}
buffer->heap->ops->free(buffer);
+   spin_lock(>heap->stat_lock);
+   buffer->heap->num_of_buffers--;
+   buffer->heap->num_of_alloc_bytes -= buffer->size;
+   spin_unlock(>heap->stat_lock);
+
kfree(buffer);
  }
  
@@ -528,12 +540,15 @@ void ion_device_add_heap(struct ion_heap *heap)

  {
struct ion_device *dev = internal_dev;
int ret;
+   struct dentry *heap_root;
+   char debug_name[64];
  
  	if (!heap->ops->allocate || !heap->ops->free)

pr_err("%s: can not add heap with invalid ops struct.\n",
   __func__);
  
  	spin_lock_init(>free_lock);

+   spin_lock_init(>stat_lock);
heap->free_list_size = 0;
  
  	if (heap->flags & ION_HEAP_FLAG_DEFER_FREE)

@@ -546,6 +561,33 @@ void ion_device_add_heap(struct ion_heap *heap)
}
  
  	heap->dev = dev;

+   heap->num_of_buffers = 0;
+   heap->num_of_alloc_bytes = 0;
+   heap->alloc_bytes_wm = 0;
+
+   heap_root = debugfs_create_dir(heap->name, dev->debug_root);
+   debugfs_create_u64("num_of_buffers",
+  0444, heap_root,
+  >num_of_buffers);
+   debugfs_create_u64("num_of_alloc_bytes",
+  0444,
+  heap_root,
+  >num_of_alloc_bytes);
+   debugfs_create_u64("alloc_bytes_wm",
+  0444,
+  heap_root,
+  >alloc_bytes_wm);
+
+   if (heap->shrinker.count_objects &&
+   heap->shrinker.scan_objects) {
+   snprintf(debug_name, 64, "%s_shrink", heap->name);
+   debugfs_create_file(debug_name,
+   0644,
+   heap_root,
+   heap,
+   _shrink_fops);
+   }
+
down_write(>lock);
heap->id = heap_id++;
/*
@@ -555,14 +597,6 @@ void ion_device_add_heap(struct ion_heap *heap)
plist_node_init(>node, -heap->id);
plist_add(>node, >heaps);
  
-	if (heap->shrinker.count_objects && heap->shrinker.scan_objects) {

-   char debug_name[64];
-
-   snprintf(debug_name, 64, "%s_shrink", heap->name);
-   debugfs_create_file(debug_name, 0644, dev->debug_root,
-   heap, _shrink_fops);
-   }
-
dev->heap_cnt++;
up_write(>lock);
  }
diff --git a/drivers/staging/android/ion/ion.h 
b/drivers/staging/android/ion/ion.h
index c006fc1..47b594c 100644
--- a/drivers/staging/android/ion/ion.h
+++ b/drivers/staging/android/ion/ion.h
@@ -157,6 +157,9 @@ struct ion_heap_ops {
   * @lock: protects the free list
   * @waitqueue:queue to wait on from deferred free thread
   * @task: task struct of deferred free thread
+ * @num_of_buffers the number of currently allocated buffers
+ * @num_of_alloc_bytes the number of allocated bytes
+ * @alloc_bytes_wm the number of allocated bytes watermark
   *
   * Represents a pool of memory from which buffers can

Re: [PATCH] staging: android: ion: Fixed uninitialized heap name access

[Laura Abbott]

On 10/22/2018 07:02 AM, Alexey Skidanov wrote:

The heap name might be uninitialized and access might crash the
kernel.



The heap name should never be null so this seems like this is being
fixed in the wrong place. Can you explain more how you are hitting
this issue?

Thanks,
Laura


Signed-off-by: Alexey Skidanov 
---
  drivers/staging/android/ion/ion.c | 7 +--
  1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/android/ion/ion.c 
b/drivers/staging/android/ion/ion.c
index 9907332..55bca92d 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -459,8 +459,11 @@ 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);
-   hdata.name[sizeof(hdata.name) - 1] = '\0';
+   if (heap->name) {
+   strncpy(hdata.name, heap->name, MAX_HEAP_NAME);
+   hdata.name[sizeof(hdata.name) - 1] = '\0';
+   }
+
hdata.type = heap->type;
hdata.heap_id = heap->id;
  



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

Re: Direct io failed with ion

[Laura Abbott]

On 10/11/2018 03:44 AM, Rock Lee wrote:

On Thu, Oct 11, 2018 at 4:12 PM Dan Carpenter  wrote:


On Thu, Oct 11, 2018 at 11:24:49AM +0800, Rock Lee wrote:

Hi
 I tested direct io with a ion allocated buffer, got a -EFAULT
error. It turned out that ion_mmap() set VM_IO & VM_PFNMAP, but
check_vma_flags() (do_direct_IO() calls it) doesn't allow that VMA has
these flags set. Could you give me any hit that could solve this
issue?



You must be using an old kernel because ion_mmap() was changed in April.


Yes, I am using linux-4.4 which is a little old. Even in linux-4.18,
ion_mmap() still callls remap_pfn_range() if the heap is
carvout/system/cma/chunk. But remap_pfn_range() set VM_IO & IO_PFNMAP
as well.



Yes, I don't think there's a way around this without moving away
from remap_pfn_range. I thought there was a reason why we
needed to use remap_pfn_range but it's escaping me. If you wanted
to do the work to move away from remap_pfn_range, that would
be appreciated.

Thanks,
Laura


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

Re: staging: android: ion: aligned allocation support

[Laura Abbott]

On 10/03/2018 01:03 PM, Alexey Skidanov wrote:



On 10/03/2018 09:07 PM, Laura Abbott wrote:

On 10/02/2018 07:27 AM, Alexey Skidanov wrote:

Hi,

Sometimes HW requires memory buffer to be aligned in order to be used
properly.  Of course, we may overcome the lack of aligned allocation
support, but we may easily add it because CMA and gen_pool (used by
several heaps) already support it.

Does someone have an objection to add it?

Thanks,
Alexey



The alignment option was removed from the allocation API before
because the most common heap (system heap) didn't support it
and it was causing more confusion. We've already mangled the
ABI once so I really don't want to break it again. I'm not
opposed to adding alignment support for the CMA via the allocation
flags.

Currently, the flags member is used to define the way the buffer will be
mapped - cached or uncached. So,if I understand you correct, we need to
add ION_FLAG_ALIGNED flag and to share 32 bit field between flags and
flags specific data (alignment value) ?



Yes, that's what I was thinking.


I'm probably going to remove the carveout and chunk heap because

nobody has stepped up to figure out how to tie allocation of those
to device tree or another method.

Thanks,
Laura


Thanks,
Alexey



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

Re: staging: android: ion: aligned allocation support

[Laura Abbott]

On 10/02/2018 07:27 AM, Alexey Skidanov wrote:

Hi,

Sometimes HW requires memory buffer to be aligned in order to be used
properly.  Of course, we may overcome the lack of aligned allocation
support, but we may easily add it because CMA and gen_pool (used by
several heaps) already support it.

Does someone have an objection to add it?

Thanks,
Alexey



The alignment option was removed from the allocation API before
because the most common heap (system heap) didn't support it
and it was causing more confusion. We've already mangled the
ABI once so I really don't want to break it again. I'm not
opposed to adding alignment support for the CMA via the allocation
flags. I'm probably going to remove the carveout and chunk heap because
nobody has stepped up to figure out how to tie allocation of those
to device tree or another method.

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

Re: [PATCH v4] staging: android: ion: Add per-heap counters

[Laura Abbott]

On 09/30/2018 08:24 AM, Alexey Skidanov wrote:

Heap statistics have been removed and currently even basics statistics
are missing.

This patch creates per heap debugfs directory /sys/kernel/debug/
and adds the following counters:
- the number of allocated buffers;
- the number of allocated bytes;
- the number of allocated bytes watermark.



If none of the other Android people have strong opinions

Acked-by: Laura Abbott 


Signed-off-by: Alexey Skidanov 
---

v3:
Removed debugfs_create_dir() return value checking
v4:
Added spinlock to protect heap statistics

  drivers/staging/android/ion/ion.c | 50 ---
  drivers/staging/android/ion/ion.h | 10 +++-
  2 files changed, 51 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/android/ion/ion.c 
b/drivers/staging/android/ion/ion.c
index 9907332..6fd8979 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -95,6 +95,13 @@ static struct ion_buffer *ion_buffer_create(struct ion_heap 
*heap,
goto err1;
}
  
+	spin_lock(>stat_lock);

+   heap->num_of_buffers++;
+   heap->num_of_alloc_bytes += len;
+   if (heap->num_of_alloc_bytes > heap->alloc_bytes_wm)
+   heap->alloc_bytes_wm = heap->num_of_alloc_bytes;
+   spin_unlock(>stat_lock);
+
INIT_LIST_HEAD(>attachments);
mutex_init(>lock);
mutex_lock(>buffer_lock);
@@ -117,6 +124,11 @@ void ion_buffer_destroy(struct ion_buffer *buffer)
buffer->heap->ops->unmap_kernel(buffer->heap, buffer);
}
buffer->heap->ops->free(buffer);
+   spin_lock(>heap->stat_lock);
+   buffer->heap->num_of_buffers--;
+   buffer->heap->num_of_alloc_bytes -= buffer->size;
+   spin_unlock(>heap->stat_lock);
+
kfree(buffer);
  }
  
@@ -528,12 +540,15 @@ void ion_device_add_heap(struct ion_heap *heap)

  {
struct ion_device *dev = internal_dev;
int ret;
+   struct dentry *heap_root;
+   char debug_name[64];
  
  	if (!heap->ops->allocate || !heap->ops->free)

pr_err("%s: can not add heap with invalid ops struct.\n",
   __func__);
  
  	spin_lock_init(>free_lock);

+   spin_lock_init(>stat_lock);
heap->free_list_size = 0;
  
  	if (heap->flags & ION_HEAP_FLAG_DEFER_FREE)

@@ -546,6 +561,33 @@ void ion_device_add_heap(struct ion_heap *heap)
}
  
  	heap->dev = dev;

+   heap->num_of_buffers = 0;
+   heap->num_of_alloc_bytes = 0;
+   heap->alloc_bytes_wm = 0;
+
+   heap_root = debugfs_create_dir(heap->name, dev->debug_root);
+   debugfs_create_u64("num_of_buffers",
+  0444, heap_root,
+  >num_of_buffers);
+   debugfs_create_u64("num_of_alloc_bytes",
+  0444,
+  heap_root,
+  >num_of_alloc_bytes);
+   debugfs_create_u64("alloc_bytes_wm",
+  0444,
+  heap_root,
+  >alloc_bytes_wm);
+
+   if (heap->shrinker.count_objects &&
+   heap->shrinker.scan_objects) {
+   snprintf(debug_name, 64, "%s_shrink", heap->name);
+   debugfs_create_file(debug_name,
+   0644,
+   heap_root,
+   heap,
+   _shrink_fops);
+   }
+
down_write(>lock);
heap->id = heap_id++;
/*
@@ -555,14 +597,6 @@ void ion_device_add_heap(struct ion_heap *heap)
plist_node_init(>node, -heap->id);
plist_add(>node, >heaps);
  
-	if (heap->shrinker.count_objects && heap->shrinker.scan_objects) {

-   char debug_name[64];
-
-   snprintf(debug_name, 64, "%s_shrink", heap->name);
-   debugfs_create_file(debug_name, 0644, dev->debug_root,
-   heap, _shrink_fops);
-   }
-
dev->heap_cnt++;
up_write(>lock);
  }
diff --git a/drivers/staging/android/ion/ion.h 
b/drivers/staging/android/ion/ion.h
index 16cbd38..f487127 100644
--- a/drivers/staging/android/ion/ion.h
+++ b/drivers/staging/android/ion/ion.h
@@ -159,6 +159,9 @@ struct ion_heap_ops {
   * @task: task struct of deferred free thread
   * @debug_show:   called when heap debug file is read to add any
   *heap specific debug info to output
+ * @num_of_buffers the number of currently allocated buffers
+ * @num_of_alloc_bytes the number of allocated bytes
+ * @alloc_bytes_wm the number of allocated bytes watermark
   *
   * Repr

Re: [PATCH v3] staging: android: ion: Add per-heap counters

[Laura Abbott]

On 09/11/2018 04:29 AM, Alexey Skidanov wrote:

Heap statistics have been removed and currently even basics statistics
are missing.

This patch creates per heap debugfs directory /sys/kernel/debug/
and adds the following counters:
- the number of allocated buffers;
- the number of allocated bytes;
- the number of allocated bytes watermark.

Signed-off-by: Alexey Skidanov 
---

  v3:
Removed debugfs_create_dir() return value checking

  drivers/staging/android/ion/ion.c | 46 ---
  drivers/staging/android/ion/ion.h |  6 ++---
  2 files changed, 41 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/android/ion/ion.c 
b/drivers/staging/android/ion/ion.c
index 9907332..ba4c6e6 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -95,6 +95,11 @@ static struct ion_buffer *ion_buffer_create(struct ion_heap 
*heap,
goto err1;
}
  
+	heap->num_of_buffers++;

+   heap->num_of_alloc_bytes += len;
+   if (heap->num_of_alloc_bytes > heap->alloc_bytes_wm)
+   heap->alloc_bytes_wm = heap->num_of_alloc_bytes;
+
INIT_LIST_HEAD(>attachments);
mutex_init(>lock);
mutex_lock(>buffer_lock);
@@ -117,6 +122,9 @@ void ion_buffer_destroy(struct ion_buffer *buffer)
buffer->heap->ops->unmap_kernel(buffer->heap, buffer);
}
buffer->heap->ops->free(buffer);
+   buffer->heap->num_of_buffers--;
+   buffer->heap->num_of_alloc_bytes -= buffer->size;
+
kfree(buffer);
  }
  
@@ -528,6 +536,8 @@ void ion_device_add_heap(struct ion_heap *heap)

  {
struct ion_device *dev = internal_dev;
int ret;
+   struct dentry *heap_root;
+   char debug_name[64];
  
  	if (!heap->ops->allocate || !heap->ops->free)

pr_err("%s: can not add heap with invalid ops struct.\n",
@@ -546,6 +556,34 @@ void ion_device_add_heap(struct ion_heap *heap)
}
  
  	heap->dev = dev;

+   heap->num_of_buffers = 0;
+   heap->num_of_alloc_bytes = 0;
+   heap->alloc_bytes_wm = 0;
+
+   /* Create heap root directory */
+   heap_root = debugfs_create_dir(heap->name, dev->debug_root);
+   debugfs_create_u64("num_of_buffers",
+  0444, heap_root,
+  >num_of_buffers);
+   debugfs_create_u64("num_of_alloc_bytes",
+  0444,
+  heap_root,
+  >num_of_alloc_bytes);
+   debugfs_create_u64("alloc_bytes_wm",
+  0444,
+  heap_root,
+  >alloc_bytes_wm);
+
+   if (heap->shrinker.count_objects &&
+   heap->shrinker.scan_objects) {
+   snprintf(debug_name, 64, "%s_shrink", heap->name);
+   debugfs_create_file(debug_name,
+   0644,
+   heap_root,
+   heap,
+   _shrink_fops);
+   }
+
down_write(>lock);
heap->id = heap_id++;
/*
@@ -555,14 +593,6 @@ void ion_device_add_heap(struct ion_heap *heap)
plist_node_init(>node, -heap->id);
plist_add(>node, >heaps);
  
-	if (heap->shrinker.count_objects && heap->shrinker.scan_objects) {

-   char debug_name[64];
-
-   snprintf(debug_name, 64, "%s_shrink", heap->name);
-   debugfs_create_file(debug_name, 0644, dev->debug_root,
-   heap, _shrink_fops);
-   }
-
dev->heap_cnt++;
up_write(>lock);
  }
diff --git a/drivers/staging/android/ion/ion.h 
b/drivers/staging/android/ion/ion.h
index 16cbd38..bea84b6 100644
--- a/drivers/staging/android/ion/ion.h
+++ b/drivers/staging/android/ion/ion.h
@@ -179,9 +179,9 @@ struct ion_heap {
spinlock_t free_lock;
wait_queue_head_t waitqueue;
struct task_struct *task;
-
-   int (*debug_show)(struct ion_heap *heap, struct seq_file *s,
- void *unused);
+   u64 num_of_buffers;
+   u64 num_of_alloc_bytes;
+   u64 alloc_bytes_wm;


What lock is protecting these?


  };
  
  /**




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

Re: [PATCH v3] staging: android: ion: Add per-heap counters

[Laura Abbott]

On 09/18/2018 10:50 AM, Greg KH wrote:

On Tue, Sep 11, 2018 at 02:29:19PM +0300, Alexey Skidanov wrote:

Heap statistics have been removed and currently even basics statistics
are missing.

This patch creates per heap debugfs directory /sys/kernel/debug/
and adds the following counters:
- the number of allocated buffers;
- the number of allocated bytes;
- the number of allocated bytes watermark.

Signed-off-by: Alexey Skidanov 
---


It would be great to get an ack from some of the ion developers...

{hint}



I am a bit behind on reviewing. It generally looks okay but I wanted
to look a bit more before giving an Ack. Give me to the end of the
week to take a look.

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

Re: [PATCH] staging: android: ion: Add per-heap counters

[Laura Abbott]

On 09/10/2018 03:00 AM, Alexey Skidanov wrote:



On 09/10/2018 12:36 PM, Dan Carpenter wrote:

On Sun, Sep 09, 2018 at 01:44:31AM +0300, Alexey Skidanov wrote:

The heap statistics have been removed and currently even basics statistics
are missing.


Remind me why did we remove them?  What was the git hash?

15c6098cfec5 ("staging: android: ion: Remove ion_handle and ion_client")
According to Laura, "The goal was to rip out the racy handle/buffer mess
and the associated debugfs there"


Yes, the debugfs was removed with that commit because the underlying
handle/client were being removed and the associated debugfs
tended to race with tear down. There was also a question about what
information should go in Ion vs. dma_buf since Ion is now basically
a wrapper around dma_buf.

I don't disagree that we want some kind of debugfs counters but
I'd like to see some description of what problems we can solve
with this that aren't also solved by the debugfs with dma_buf.

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

Re: [PATCH] android:ion: the order and count are in the wrong position

[Laura Abbott]

On 09/07/2018 08:20 AM, jun qian wrote:

The value in the wrong position will cause misunderstanding,
when the debug infomations display in the window.



I think the existing order is okay, it's just not separated
well. It's "$count pages of order $order". I also just acked a
patch to remove all this code because it's dead on mainline
anyway. For future work, we should look to make the debugfs
output clearer to avoid ambiguity.

Thanks,
Laura


Signed-off-by: jun qian 
---
  drivers/staging/android/ion/ion_system_heap.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/android/ion/ion_system_heap.c 
b/drivers/staging/android/ion/ion_system_heap.c
index 701eb9f3b0f1..54b8a7710958 100644
--- a/drivers/staging/android/ion/ion_system_heap.c
+++ b/drivers/staging/android/ion/ion_system_heap.c
@@ -225,10 +225,10 @@ static int ion_system_heap_debug_show(struct ion_heap 
*heap, struct seq_file *s,
pool = sys_heap->pools[i];
  
  		seq_printf(s, "%d order %u highmem pages %lu total\n",

-  pool->high_count, pool->order,
+  pool->order, pool->high_count,
   (PAGE_SIZE << pool->order) * pool->high_count);
seq_printf(s, "%d order %u lowmem pages %lu total\n",
-  pool->low_count, pool->order,
+  pool->order, pool->low_count,
   (PAGE_SIZE << pool->order) * pool->low_count);
}
  



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

Re: [PATCH v3] staging: android: ion: Clean unused debug_show memeber of the heap object

[Laura Abbott]

On 09/04/2018 10:03 PM, Alexey Skidanov wrote:

ION had supported heap debug info under /sys/kernel/debug/ion/.
This support have been removed but some leftovers (dead code) still exist.

This patch removes the existing dead code.



Acked-by: Laura Abbott 


Fixes: 15c6098cfec5 ("staging: android: ion: Remove ion_handle and ion_client")
Signed-off-by: Alexey Skidanov 
---
  drivers/staging/android/ion/ion.h |  5 -
  drivers/staging/android/ion/ion_system_heap.c | 24 
  2 files changed, 29 deletions(-)

diff --git a/drivers/staging/android/ion/ion.h 
b/drivers/staging/android/ion/ion.h
index 876197b..0afa9cd 100644
--- a/drivers/staging/android/ion/ion.h
+++ b/drivers/staging/android/ion/ion.h
@@ -158,8 +158,6 @@ struct ion_heap_ops {
   * @lock: protects the free list
   * @waitqueue:queue to wait on from deferred free thread
   * @task: task struct of deferred free thread
- * @debug_show:called when heap debug file is read to add any
- * heap specific debug info to output
   *
   * Represents a pool of memory from which buffers can be made.  In some
   * systems the only heap is regular system memory allocated via vmalloc.
@@ -180,9 +178,6 @@ struct ion_heap {
spinlock_t free_lock;
wait_queue_head_t waitqueue;
struct task_struct *task;
-
-   int (*debug_show)(struct ion_heap *heap, struct seq_file *s,
- void *unused);
  };
  
  /**

diff --git a/drivers/staging/android/ion/ion_system_heap.c 
b/drivers/staging/android/ion/ion_system_heap.c
index b5c3195..d0d0490 100644
--- a/drivers/staging/android/ion/ion_system_heap.c
+++ b/drivers/staging/android/ion/ion_system_heap.c
@@ -213,29 +213,6 @@ static struct ion_heap_ops system_heap_ops = {
.shrink = ion_system_heap_shrink,
  };
  
-static int ion_system_heap_debug_show(struct ion_heap *heap, struct seq_file *s,

- void *unused)
-{
-   struct ion_system_heap *sys_heap = container_of(heap,
-   struct ion_system_heap,
-   heap);
-   int i;
-   struct ion_page_pool *pool;
-
-   for (i = 0; i < NUM_ORDERS; i++) {
-   pool = sys_heap->pools[i];
-
-   seq_printf(s, "%d order %u highmem pages %lu total\n",
-  pool->high_count, pool->order,
-  (PAGE_SIZE << pool->order) * pool->high_count);
-   seq_printf(s, "%d order %u lowmem pages %lu total\n",
-  pool->low_count, pool->order,
-  (PAGE_SIZE << pool->order) * pool->low_count);
-   }
-
-   return 0;
-}
-
  static void ion_system_heap_destroy_pools(struct ion_page_pool **pools)
  {
int i;
@@ -282,7 +259,6 @@ static struct ion_heap *__ion_system_heap_create(void)
if (ion_system_heap_create_pools(heap->pools))
goto free_heap;
  
-	heap->heap.debug_show = ion_system_heap_debug_show;

return >heap;
  
  free_heap:




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

Re: [PATCH v2] staging: android: ion: fix ION_IOC_{MAP,SHARE} use-after-free

[Laura Abbott]

On 09/04/2018 09:33 AM, Greg Hackmann wrote:

The ION_IOC_{MAP,SHARE} ioctls drop and reacquire client->lock several
times while operating on one of the client's ion_handles.  This creates
windows where userspace can call ION_IOC_FREE on the same client with
the same handle, and effectively make the kernel drop its own reference.
For example:

- thread A: ION_IOC_ALLOC creates an ion_handle with refcount 1
- thread A: starts ION_IOC_MAP and increments the refcount to 2
- thread B: ION_IOC_FREE decrements the refcount to 1
- thread B: ION_IOC_FREE decrements the refcount to 0 and frees the
 handle
- thread A: continues ION_IOC_MAP with a dangling ion_handle * to
 freed memory

Fix this by holding client->lock for the duration of
ION_IOC_{MAP,SHARE}, preventing the concurrent ION_IOC_FREE.  Also
remove ion_handle_get_by_id(), since there's literally no way to use it
safely.

This patch is applied on top of 4.4.y, and applies to older kernels
too.  4.9.y was fixed separately.  Kernels 4.12 and later are
unaffected, since all the underlying ion_handle infrastructure has been
ripped out.



Acked-by: Laura Abbott 


Cc: sta...@vger.kernel.org # v4.4-
Signed-off-by: Greg Hackmann 
---
v2: remove Change-Id line from commit message

  drivers/staging/android/ion/ion.c | 60 +++
  1 file changed, 37 insertions(+), 23 deletions(-)

diff --git a/drivers/staging/android/ion/ion.c 
b/drivers/staging/android/ion/ion.c
index 47cb163da9a0..4adb1138af09 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -449,18 +449,6 @@ static struct ion_handle 
*ion_handle_get_by_id_nolock(struct ion_client *client,
return ERR_PTR(-EINVAL);
  }
  
-struct ion_handle *ion_handle_get_by_id(struct ion_client *client,

-   int id)
-{
-   struct ion_handle *handle;
-
-   mutex_lock(>lock);
-   handle = ion_handle_get_by_id_nolock(client, id);
-   mutex_unlock(>lock);
-
-   return handle;
-}
-
  static bool ion_handle_validate(struct ion_client *client,
struct ion_handle *handle)
  {
@@ -1138,24 +1126,28 @@ static struct dma_buf_ops dma_buf_ops = {
.kunmap = ion_dma_buf_kunmap,
  };
  
-struct dma_buf *ion_share_dma_buf(struct ion_client *client,

-   struct ion_handle *handle)
+static struct dma_buf *__ion_share_dma_buf(struct ion_client *client,
+  struct ion_handle *handle,
+  bool lock_client)
  {
DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
struct ion_buffer *buffer;
struct dma_buf *dmabuf;
bool valid_handle;
  
-	mutex_lock(>lock);

+   if (lock_client)
+   mutex_lock(>lock);
valid_handle = ion_handle_validate(client, handle);
if (!valid_handle) {
WARN(1, "%s: invalid handle passed to share.\n", __func__);
-   mutex_unlock(>lock);
+   if (lock_client)
+   mutex_unlock(>lock);
return ERR_PTR(-EINVAL);
}
buffer = handle->buffer;
ion_buffer_get(buffer);
-   mutex_unlock(>lock);
+   if (lock_client)
+   mutex_unlock(>lock);
  
  	exp_info.ops = _buf_ops;

exp_info.size = buffer->size;
@@ -1170,14 +1162,21 @@ struct dma_buf *ion_share_dma_buf(struct ion_client 
*client,
  
  	return dmabuf;

  }
+
+struct dma_buf *ion_share_dma_buf(struct ion_client *client,
+ struct ion_handle *handle)
+{
+   return __ion_share_dma_buf(client, handle, true);
+}
  EXPORT_SYMBOL(ion_share_dma_buf);
  
-int ion_share_dma_buf_fd(struct ion_client *client, struct ion_handle *handle)

+static int __ion_share_dma_buf_fd(struct ion_client *client,
+ struct ion_handle *handle, bool lock_client)
  {
struct dma_buf *dmabuf;
int fd;
  
-	dmabuf = ion_share_dma_buf(client, handle);

+   dmabuf = __ion_share_dma_buf(client, handle, lock_client);
if (IS_ERR(dmabuf))
return PTR_ERR(dmabuf);
  
@@ -1187,8 +1186,19 @@ int ion_share_dma_buf_fd(struct ion_client *client, struct ion_handle *handle)
  
  	return fd;

  }
+
+int ion_share_dma_buf_fd(struct ion_client *client, struct ion_handle *handle)
+{
+   return __ion_share_dma_buf_fd(client, handle, true);
+}
  EXPORT_SYMBOL(ion_share_dma_buf_fd);
  
+static int ion_share_dma_buf_fd_nolock(struct ion_client *client,

+  struct ion_handle *handle)
+{
+   return __ion_share_dma_buf_fd(client, handle, false);
+}
+
  struct ion_handle *ion_import_dma_buf(struct ion_client *client, int fd)
  {
struct dma_buf *dmabuf;
@@ -1335,11 +1345,15 @@ static long ion_ioctl(struct file *filp, unsigned int 
cmd, unsigned long ar

Re: [PATCH v2] staging: android: ion: Clean unused debug_show memeber of the heap object

[Laura Abbott]

On 08/26/2018 01:08 PM, Alexey Skidanov wrote:

ION had supported heap debug info under /sys/kernel/debug/ion/.
This support have been removed but some leftovers (dead code) still exist.

This patch removes the existing dead code.



This was actually an unintended side effect. The goal was to rip out
the racy handle/buffer mess and the associated debugfs there. I do
think the extra heap_show for system heap is useful but I'd rather
see it reimplemented with a full debugfs vs. trying to just tack it
on.

I'd like to see the exact commit where this was orphaned (15c6098cfec5
("staging: android: ion: Remove ion_handle and ion_client")) specified
somewhere in the commit text so if you add that, you can add

Acked-by: Laura Abbott 


Signed-off-by: Alexey Skidanov 
---
  drivers/staging/android/ion/ion.h |  5 -
  drivers/staging/android/ion/ion_system_heap.c | 24 
  2 files changed, 29 deletions(-)

diff --git a/drivers/staging/android/ion/ion.h 
b/drivers/staging/android/ion/ion.h
index 876197b..0afa9cd 100644
--- a/drivers/staging/android/ion/ion.h
+++ b/drivers/staging/android/ion/ion.h
@@ -158,8 +158,6 @@ struct ion_heap_ops {
   * @lock: protects the free list
   * @waitqueue:queue to wait on from deferred free thread
   * @task: task struct of deferred free thread
- * @debug_show:called when heap debug file is read to add any
- * heap specific debug info to output
   *
   * Represents a pool of memory from which buffers can be made.  In some
   * systems the only heap is regular system memory allocated via vmalloc.
@@ -180,9 +178,6 @@ struct ion_heap {
spinlock_t free_lock;
wait_queue_head_t waitqueue;
struct task_struct *task;
-
-   int (*debug_show)(struct ion_heap *heap, struct seq_file *s,
- void *unused);
  };
  
  /**

diff --git a/drivers/staging/android/ion/ion_system_heap.c 
b/drivers/staging/android/ion/ion_system_heap.c
index b5c3195..d0d0490 100644
--- a/drivers/staging/android/ion/ion_system_heap.c
+++ b/drivers/staging/android/ion/ion_system_heap.c
@@ -213,29 +213,6 @@ static struct ion_heap_ops system_heap_ops = {
.shrink = ion_system_heap_shrink,
  };
  
-static int ion_system_heap_debug_show(struct ion_heap *heap, struct seq_file *s,

- void *unused)
-{
-   struct ion_system_heap *sys_heap = container_of(heap,
-   struct ion_system_heap,
-   heap);
-   int i;
-   struct ion_page_pool *pool;
-
-   for (i = 0; i < NUM_ORDERS; i++) {
-   pool = sys_heap->pools[i];
-
-   seq_printf(s, "%d order %u highmem pages %lu total\n",
-  pool->high_count, pool->order,
-  (PAGE_SIZE << pool->order) * pool->high_count);
-   seq_printf(s, "%d order %u lowmem pages %lu total\n",
-  pool->low_count, pool->order,
-  (PAGE_SIZE << pool->order) * pool->low_count);
-   }
-
-   return 0;
-}
-
  static void ion_system_heap_destroy_pools(struct ion_page_pool **pools)
  {
int i;
@@ -282,7 +259,6 @@ static struct ion_heap *__ion_system_heap_create(void)
if (ion_system_heap_create_pools(heap->pools))
goto free_heap;
  
-	heap->heap.debug_show = ion_system_heap_debug_show;

return >heap;
  
  free_heap:




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

[PATCH] staging: android: ion: Return an ERR_PTR in ion_map_kernel

[Laura Abbott]

The expected return value from ion_map_kernel is an ERR_PTR. The error
path for a vmalloc failure currently just returns NULL, triggering
a warning in ion_buffer_kmap_get. Encode the vmalloc failure as an ERR_PTR.

Reported-by: syzbot+55b1d9f811650de94...@syzkaller.appspotmail.com
Signed-off-by: Laura Abbott 
---
 drivers/staging/android/ion/ion_heap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/android/ion/ion_heap.c 
b/drivers/staging/android/ion/ion_heap.c
index 772dad65396e..f32c12439eee 100644
--- a/drivers/staging/android/ion/ion_heap.c
+++ b/drivers/staging/android/ion/ion_heap.c
@@ -29,7 +29,7 @@ void *ion_heap_map_kernel(struct ion_heap *heap,
struct page **tmp = pages;
 
if (!pages)
-   return NULL;
+   return ERR_PTR(-ENOMEM);
 
if (buffer->flags & ION_FLAG_CACHED)
pgprot = PAGE_KERNEL;
-- 
2.17.0

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

[PATCH] staging: android: ion: Switch to pr_warn_once in ion_buffer_destroy

[Laura Abbott]

Syzbot reported yet another warning with Ion:

WARNING: CPU: 0 PID: 1467 at drivers/staging/android/ion/ion.c:122
ion_buffer_destroy+0xd4/0x190 drivers/staging/android/ion/ion.c:122
Kernel panic - not syncing: panic_on_warn set ...

This is catching that a buffer was freed with an existing kernel mapping
still present. This can be easily be triggered from userspace by calling
DMA_BUF_SYNC_START without calling DMA_BUF_SYNC_END. Switch to a single
pr_warn_once to indicate the error without being disruptive.

Reported-by: syzbot+cd8bcd40cb049efa2...@syzkaller.appspotmail.com
Reported-by: syzbot <syzkal...@googlegroups.com>
Signed-off-by: Laura Abbott <labb...@redhat.com>
---
 drivers/staging/android/ion/ion.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/android/ion/ion.c 
b/drivers/staging/android/ion/ion.c
index e74db7902549..a68329411b29 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -114,8 +114,11 @@ static struct ion_buffer *ion_buffer_create(struct 
ion_heap *heap,
 
 void ion_buffer_destroy(struct ion_buffer *buffer)
 {
-   if (WARN_ON(buffer->kmap_cnt > 0))
+   if (buffer->kmap_cnt > 0) {
+   pr_warn_once("%s: buffer still mapped in the kernel\n",
+__func__);
buffer->heap->ops->unmap_kernel(buffer->heap, buffer);
+   }
buffer->heap->ops->free(buffer);
kfree(buffer);
 }
-- 
2.17.0

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

Re: WARNING in ion_buffer_destroy

[Laura Abbott]

On 05/09/2018 11:59 PM, Dmitry Vyukov wrote:

On Wed, Jan 10, 2018 at 7:14 PM, Laura Abbott <labb...@redhat.com> wrote:

On 01/09/2018 02:58 PM, syzbot wrote:


Hello,

syzkaller hit the following crash on
06d41862286aa7bc634a1dd9e6e7e96f925ef30a
git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/master
compiler: gcc (GCC) 7.1.1 20170620
.config is attached
Raw console output is attached.
C reproducer is attached
syzkaller reproducer is attached. See https://goo.gl/kgGztJ
for information about syzkaller reproducers


IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+cd8bcd40cb049efa2...@syzkaller.appspotmail.com
It will help syzbot understand when the bug is fixed. See footer for
details.
If you forward the report, please keep this part and the footer.

audit: type=1400 audit(1515538424.230:7): avc:  denied  { map } for
pid=3499 comm="syzkaller239906" path="/root/syzkaller239906633" dev="sda1"
ino=16481 scontext=unconfined_u:system_r:insmod_t:s0-s0:c0.c1023
tcontext=unconfined_u:object_r:user_home_t:s0 tclass=file permissive=1
WARNING: CPU: 0 PID: 1467 at drivers/staging/android/ion/ion.c:122
ion_buffer_destroy+0xd4/0x190 drivers/staging/android/ion/ion.c:122
Kernel panic - not syncing: panic_on_warn set ...

CPU: 0 PID: 1467 Comm: ion_system_heap Not tainted
4.15.0-rc7-next-20180109+ #92
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Call Trace:
   __dump_stack lib/dump_stack.c:17 [inline]
   dump_stack+0x194/0x257 lib/dump_stack.c:53
   panic+0x1e4/0x41c kernel/panic.c:183
   __warn+0x1dc/0x200 kernel/panic.c:547
   report_bug+0x211/0x2d0 lib/bug.c:184
   fixup_bug.part.11+0x37/0x80 arch/x86/kernel/traps.c:178
   fixup_bug arch/x86/kernel/traps.c:247 [inline]
   do_error_trap+0x2d7/0x3e0 arch/x86/kernel/traps.c:296
   do_invalid_op+0x1b/0x20 arch/x86/kernel/traps.c:315
   invalid_op+0x22/0x40 arch/x86/entry/entry_64.S:1079
RIP: 0010:ion_buffer_destroy+0xd4/0x190
drivers/staging/android/ion/ion.c:122
RSP: 0018:8801d3a9fd28 EFLAGS: 00010293
RAX: 8801d39ee700 RBX: 8801c00e57c0 RCX: 8415d2a4
RDX:  RSI: 0001 RDI: 8801d5ada5b8
RBP: 8801d3a9fd50 R08:  R09: 11003a753f8a
R10: 8801d3a9fc18 R11:  R12: 86e4c980
R13: 8801d5ada580 R14: 8801c00e57e0 R15: 0001
   ion_heap_deferred_free+0x290/0x650
drivers/staging/android/ion/ion_heap.c:236
   kthread+0x33c/0x400 kernel/kthread.c:238
   ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:524
Dumping ftrace buffer:
 (ftrace buffer empty)
Kernel Offset: disabled
Rebooting in 86400 seconds..



This is catching that a buffer was freed with an existing kernel
map still present. The problem is this can easily be triggered from
userspace by calling DMA_BUF_SYNC_START without calling
DMA_BUF_SYNC_END. It's clearly not appropriate for userspace to
be able to trigger a warning so I'll see about switching this to
a pr_warn_once.


Hi Laura,

Any updates on this?



I thought I had sent a fix for this but I guess not. I'll see about
getting one out.

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

Re: A few questions about warnings in the ion driver

[Laura Abbott]

On 05/07/2018 07:51 AM, Nathan Chancellor wrote:

On Mon, May 07, 2018 at 06:46:23AM -0700, Laura Abbott wrote:

On 05/06/2018 06:43 PM, Nathan Chancellor wrote:

Hi everyone,

I compiled the ion driver with W=1 where I encountered the two following
warnings and I had a couple of questions about solving them.


1.

drivers/staging/android/ion/ion.c: In function ‘ion_dma_buf_begin_cpu_access’:
drivers/staging/android/ion/ion.c:316:8: warning: variable ‘vaddr’ set but not 
used [-Wunused-but-set-variable]

which concerns the following function:

static int ion_dma_buf_begin_cpu_access(struct dma_buf *dmabuf,
  enum dma_data_direction direction)
{
  struct ion_buffer *buffer = dmabuf->priv;
  void *vaddr;
  struct ion_dma_buf_attachment *a;

  /*
   * TODO: Move this elsewhere because we don't always need a vaddr
   */
  if (buffer->heap->ops->map_kernel) {
  mutex_lock(>lock);
  vaddr = ion_buffer_kmap_get(buffer);
  mutex_unlock(>lock);
  }

  mutex_lock(>lock);
  list_for_each_entry(a, >attachments, list) {
  dma_sync_sg_for_cpu(a->dev, a->table->sgl, a->table->nents,
  direction);
  }
  mutex_unlock(>lock);

  return 0;
}

Can vaddr be removed and just have ion_buffer_kmap_get remain, since it is 
never used again in the function?



I think a better solution is to check the return value of vaddr
and error out if it fails.



Something like this? I was unsure if -ENOMEM or -EINVAL was a better
error return code in this context.

diff --git a/drivers/staging/android/ion/ion.c 
b/drivers/staging/android/ion/ion.c
index d10b60fe4a29..d1c149bb15c3 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -315,6 +315,7 @@ static int ion_dma_buf_begin_cpu_access(struct dma_buf 
*dmabuf,
 struct ion_buffer *buffer = dmabuf->priv;
 void *vaddr;
 struct ion_dma_buf_attachment *a;
+   int ret = 0;

 /*
  * TODO: Move this elsewhere because we don't always need a vaddr
@@ -322,6 +323,10 @@ static int ion_dma_buf_begin_cpu_access(struct dma_buf 
*dmabuf,
 if (buffer->heap->ops->map_kernel) {
 mutex_lock(>lock);
 vaddr = ion_buffer_kmap_get(buffer);
+   if (IS_ERR(vaddr)) {
+   ret = -ENOMEM;


A better practice is to just return the error that's returned

ret = PTR_ERR(vaddr);

Other than that looks okay for submission as a patch.

Thanks,
Laura


+   goto unlock;
+   }
 mutex_unlock(>lock);
 }

@@ -330,9 +335,10 @@ static int ion_dma_buf_begin_cpu_access(struct dma_buf 
*dmabuf,
 dma_sync_sg_for_cpu(a->dev, a->table->sgl, a->table->nents,
 direction);
 }
-   mutex_unlock(>lock);

-   return 0;
+unlock:
+   mutex_unlock(>lock);
+   return ret;
  }

  static int ion_dma_buf_end_cpu_access(struct dma_buf *dmabuf,



2.

drivers/staging/android/ion/ion_carveout_heap.c:106:18: warning: no previous 
prototype for ‘ion_carveout_heap_create’ [-Wmissing-prototypes]
drivers/staging/android/ion/ion_chunk_heap.c:111:18: warning: no previous 
prototype for ‘ion_chunk_heap_create’ [-Wmissing-prototypes]

It appears neither of these functions are used since commit 2f87f50b2340
("staging: android: ion: Rework heap registration/enumeration"). Ultimately,
removing the whole file fixes this warning; is there still a need to rework
them or can they be removed?



I'd still like to delete it. I haven't seen anyone come out to re-work it.
That said, I think my preference is still to keep it for now until
we do another round of updates.



Understood, I figured it probably isn't wise to remove stuff in the
middle of a release cycle but hey, never know. I will leave it alone
for now.

Thanks!
Nathan


Thanks for looking at these.

Laura



If any part of this email was formatted incorrectly or could be done better,
please let me know!

Thank you,
Nathan Chancellor





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

Re: [PATCH 1/2] staging: android: ion: Fix license identifier comment format

[Laura Abbott]

On 05/07/2018 07:40 AM, Nathan Chancellor wrote:

On Mon, May 07, 2018 at 06:37:52AM -0700, Laura Abbott wrote:

On 05/06/2018 06:18 PM, Nathan Chancellor wrote:

checkpatch.pl complains these are invalid because the rules in
Documentation/process/license-rules.rst state that C headers should
have "/* */" style comments.



The SPDX markings are special, but I don't see anything from a
quick read of the SPDX specification that says they have to use //.
I think this is going to generate a lot of possible noise so it
might be worth adjusting checkpatch.

Thanks,
Laura


Under section 2 of "License identifier syntax" in the license rules
file, it shows the preferred style for each type of file. Apparently
there was some build breakage with // in header files so I assume they
want /* */ for uniformity sake.

Thanks!
Nathan



Ah thanks for pointing me to that. I parsed your commit text completely
wrong. My biggest concern is being consistent and not breaking anything
so assuming this patch aligns with that:

Acked-by: Laura Abbott <labb...@redhat.com>




Signed-off-by: Nathan Chancellor <natechancel...@gmail.com>
---
   drivers/staging/android/ion/ion.h  | 2 +-
   drivers/staging/android/uapi/ion.h | 2 +-
   2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/android/ion/ion.h 
b/drivers/staging/android/ion/ion.h
index ea0897812780..16cbd38a7160 100644
--- a/drivers/staging/android/ion/ion.h
+++ b/drivers/staging/android/ion/ion.h
@@ -1,4 +1,4 @@
-// SPDX-License-Identifier: GPL-2.0
+/* SPDX-License-Identifier: GPL-2.0 */
   /*
* drivers/staging/android/ion/ion.h
*
diff --git a/drivers/staging/android/uapi/ion.h 
b/drivers/staging/android/uapi/ion.h
index 825d3e95ccd3..5d7009884c13 100644
--- a/drivers/staging/android/uapi/ion.h
+++ b/drivers/staging/android/uapi/ion.h
@@ -1,4 +1,4 @@
-// SPDX-License-Identifier: GPL-2.0
+/* SPDX-License-Identifier: GPL-2.0 */
   /*
* drivers/staging/android/uapi/ion.h
*





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

Re: A few questions about warnings in the ion driver

[Laura Abbott]

On 05/06/2018 06:43 PM, Nathan Chancellor wrote:

Hi everyone,

I compiled the ion driver with W=1 where I encountered the two following
warnings and I had a couple of questions about solving them.


1.

drivers/staging/android/ion/ion.c: In function ‘ion_dma_buf_begin_cpu_access’:
drivers/staging/android/ion/ion.c:316:8: warning: variable ‘vaddr’ set but not 
used [-Wunused-but-set-variable]

which concerns the following function:

static int ion_dma_buf_begin_cpu_access(struct dma_buf *dmabuf,
 enum dma_data_direction direction)
{
 struct ion_buffer *buffer = dmabuf->priv;
 void *vaddr;
 struct ion_dma_buf_attachment *a;

 /*
  * TODO: Move this elsewhere because we don't always need a vaddr
  */
 if (buffer->heap->ops->map_kernel) {
 mutex_lock(>lock);
 vaddr = ion_buffer_kmap_get(buffer);
 mutex_unlock(>lock);
 }

 mutex_lock(>lock);
 list_for_each_entry(a, >attachments, list) {
 dma_sync_sg_for_cpu(a->dev, a->table->sgl, a->table->nents,
 direction);
 }
 mutex_unlock(>lock);

 return 0;
}

Can vaddr be removed and just have ion_buffer_kmap_get remain, since it is 
never used again in the function?



I think a better solution is to check the return value of vaddr
and error out if it fails.



2.

drivers/staging/android/ion/ion_carveout_heap.c:106:18: warning: no previous 
prototype for ‘ion_carveout_heap_create’ [-Wmissing-prototypes]
drivers/staging/android/ion/ion_chunk_heap.c:111:18: warning: no previous 
prototype for ‘ion_chunk_heap_create’ [-Wmissing-prototypes]

It appears neither of these functions are used since commit 2f87f50b2340
("staging: android: ion: Rework heap registration/enumeration"). Ultimately,
removing the whole file fixes this warning; is there still a need to rework
them or can they be removed?



I'd still like to delete it. I haven't seen anyone come out to re-work it.
That said, I think my preference is still to keep it for now until
we do another round of updates.

Thanks for looking at these.

Laura



If any part of this email was formatted incorrectly or could be done better,
please let me know!

Thank you,
Nathan Chancellor



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

Re: [PATCH 2/2] staging: android: ion: Remove unnecessary blank line

[Laura Abbott]

On 05/06/2018 06:18 PM, Nathan Chancellor wrote:

Fixes a checkpatch.pl warning.

Signed-off-by: Nathan Chancellor <natechancel...@gmail.com>
---
  drivers/staging/android/ion/ion.c | 1 -
  1 file changed, 1 deletion(-)

diff --git a/drivers/staging/android/ion/ion.c 
b/drivers/staging/android/ion/ion.c
index 269a431646be..d10b60fe4a29 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -95,7 +95,6 @@ static struct ion_buffer *ion_buffer_create(struct ion_heap 
*heap,
goto err1;
}
  
-

INIT_LIST_HEAD(>attachments);
mutex_init(>lock);
mutex_lock(>buffer_lock);



Acked-by: Laura Abbott <labb...@redhat.com>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/2] staging: android: ion: Fix license identifier comment format

[Laura Abbott]

On 05/06/2018 06:18 PM, Nathan Chancellor wrote:

checkpatch.pl complains these are invalid because the rules in
Documentation/process/license-rules.rst state that C headers should
have "/* */" style comments.



The SPDX markings are special, but I don't see anything from a
quick read of the SPDX specification that says they have to use //.
I think this is going to generate a lot of possible noise so it
might be worth adjusting checkpatch.

Thanks,
Laura


Signed-off-by: Nathan Chancellor 
---
  drivers/staging/android/ion/ion.h  | 2 +-
  drivers/staging/android/uapi/ion.h | 2 +-
  2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/android/ion/ion.h 
b/drivers/staging/android/ion/ion.h
index ea0897812780..16cbd38a7160 100644
--- a/drivers/staging/android/ion/ion.h
+++ b/drivers/staging/android/ion/ion.h
@@ -1,4 +1,4 @@
-// SPDX-License-Identifier: GPL-2.0
+/* SPDX-License-Identifier: GPL-2.0 */
  /*
   * drivers/staging/android/ion/ion.h
   *
diff --git a/drivers/staging/android/uapi/ion.h 
b/drivers/staging/android/uapi/ion.h
index 825d3e95ccd3..5d7009884c13 100644
--- a/drivers/staging/android/uapi/ion.h
+++ b/drivers/staging/android/uapi/ion.h
@@ -1,4 +1,4 @@
-// SPDX-License-Identifier: GPL-2.0
+/* SPDX-License-Identifier: GPL-2.0 */
  /*
   * drivers/staging/android/uapi/ion.h
   *



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

Re: [PATCH] ion: Consider ion pool pages as indirectly reclaimable

[Laura Abbott]

On 04/27/2018 02:29 AM, vji...@codeaurora.org wrote:

On 2018-04-27 10:40, vji...@codeaurora.org wrote:

On 2018-04-25 21:17, Laura Abbott wrote:

On 04/24/2018 08:43 PM, vji...@codeaurora.org wrote:

From: Vijayanand Jitta <vji...@codeaurora.org>

An issue is observed where mallocs are failing due to overcommit failure.
The failure happens when there is high ION page pool since ION page
pool is not considered reclaimable by the overcommit calculation code.
This change considers ion pool pages as indirectly reclaimable and thus
accounted as available memory in the overcommit calculation.

Signed-off-by: Vijayanand Jitta <vji...@codeaurora.org>
---
  drivers/staging/android/ion/ion_page_pool.c | 5 +
  1 file changed, 5 insertions(+)

diff --git a/drivers/staging/android/ion/ion_page_pool.c 
b/drivers/staging/android/ion/ion_page_pool.c
index db8f614..9bc56eb 100644
--- a/drivers/staging/android/ion/ion_page_pool.c
+++ b/drivers/staging/android/ion/ion_page_pool.c
@@ -32,6 +32,9 @@ static void ion_page_pool_add(struct ion_page_pool *pool, 
struct page *page)
  list_add_tail(>lru, >low_items);
  pool->low_count++;
  }
+
+    mod_node_page_state(page_pgdat(page), NR_INDIRECTLY_RECLAIMABLE_BYTES,
+    (1 << (PAGE_SHIFT + pool->order)));
  mutex_unlock(>mutex);
  }
  @@ -50,6 +53,8 @@ static struct page *ion_page_pool_remove(struct 
ion_page_pool *pool, bool high)
  }
    list_del(>lru);
+    mod_node_page_state(page_pgdat(page), NR_INDIRECTLY_RECLAIMABLE_BYTES,
+    -(1 << (PAGE_SHIFT + pool->order)));
  return page;
  }



I'm sure this fixes the problem but I don't think we want to
start throwing page adjustments into Ion. Why isn't this
memory already considered reclaimable by existing calculations?

Thanks,
Laura


You can refer to discussion here https://lkml.org/lkml/2018/3/5/361 introducing
NR_INDIRECTLY_RECLAIMABLE_BYTES for the memory which is not currently considered
as reclaimable

Thanks,
Vijay


There was also discussion specific to ion in that thread you can find it here
https://lkml.org/lkml/2018/4/25/642

Thanks,
Vijay


Thanks for pointing that thread out. I'm still a little wary since
Ion is in staging but if the rest of mm are okay with it

Acked-by: Laura Abbott <labb...@redhat.com>

Thanks,
Laura

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

Re: [PATCH] ion: Consider ion pool pages as indirectly reclaimable

[Laura Abbott]

On 04/24/2018 08:43 PM, vji...@codeaurora.org wrote:

From: Vijayanand Jitta 

An issue is observed where mallocs are failing due to overcommit failure.
The failure happens when there is high ION page pool since ION page
pool is not considered reclaimable by the overcommit calculation code.
This change considers ion pool pages as indirectly reclaimable and thus
accounted as available memory in the overcommit calculation.

Signed-off-by: Vijayanand Jitta 
---
  drivers/staging/android/ion/ion_page_pool.c | 5 +
  1 file changed, 5 insertions(+)

diff --git a/drivers/staging/android/ion/ion_page_pool.c 
b/drivers/staging/android/ion/ion_page_pool.c
index db8f614..9bc56eb 100644
--- a/drivers/staging/android/ion/ion_page_pool.c
+++ b/drivers/staging/android/ion/ion_page_pool.c
@@ -32,6 +32,9 @@ static void ion_page_pool_add(struct ion_page_pool *pool, 
struct page *page)
list_add_tail(>lru, >low_items);
pool->low_count++;
}
+
+   mod_node_page_state(page_pgdat(page), NR_INDIRECTLY_RECLAIMABLE_BYTES,
+   (1 << (PAGE_SHIFT + pool->order)));
mutex_unlock(>mutex);
  }
  
@@ -50,6 +53,8 @@ static struct page *ion_page_pool_remove(struct ion_page_pool *pool, bool high)

}
  
  	list_del(>lru);

+   mod_node_page_state(page_pgdat(page), NR_INDIRECTLY_RECLAIMABLE_BYTES,
+   -(1 << (PAGE_SHIFT + pool->order)));
return page;
  }
  



I'm sure this fixes the problem but I don't think we want to
start throwing page adjustments into Ion. Why isn't this
memory already considered reclaimable by existing calculations?

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

Re: [PATCH] MAINTAINERS: add dri-devel for Android ION

[Laura Abbott]

On 04/04/2018 03:30 AM, Daniel Vetter wrote:

Most of the other cross-driver gfx infrastructure (dma_buf, dma_fence)
also gets cross posted to all the relevant gfx/memory lists. Doing the
same for ION means people won't miss relevant patches.



No problem from me, the rate of checkpatch fixups should be
small these days.

Acked-by: Laura Abbott <labb...@redhat.com>


Cc: Laura Abbott <labb...@redhat.com>
Cc: Sumit Semwal <sumit.sem...@linaro.org>
Cc: de...@driverdev.osuosl.org
Cc: dri-de...@lists.freedesktop.org
Cc: linaro-mm-...@lists.linaro.org (moderated for non-subscribers)
Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org>
Signed-off-by: Daniel Vetter <daniel.vet...@intel.com>
---
  MAINTAINERS | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 555db72d4eb7..d43cdfca3eb5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -902,6 +902,8 @@ ANDROID ION DRIVER
  M:Laura Abbott <labb...@redhat.com>
  M:Sumit Semwal <sumit.sem...@linaro.org>
  L:de...@driverdev.osuosl.org
+L: dri-de...@lists.freedesktop.org
+L: linaro-mm-...@lists.linaro.org (moderated for non-subscribers)
  S:Supported
  F:drivers/staging/android/ion
  F:drivers/staging/android/uapi/ion.h



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

Re: [PATCH] staging: android: ion: Update wording in drivers/staging/android/ion/Kconfig

[Laura Abbott]

On 03/15/2018 11:13 AM, Phillip Potter wrote:

Changes the usage of the word 'Chose' to 'Choose' in the ION
Memory Manager Kconfig.

Signed-off-by: Phillip Potter <p...@philpotter.co.uk>
---
--- a/drivers/staging/android/ion/Kconfig
+++ b/drivers/staging/android/ion/Kconfig
@@ -4,7 +4,7 @@ menuconfig ION
select GENERIC_ALLOCATOR
select DMA_SHARED_BUFFER
---help---
- Chose this option to enable the ION Memory Manager,
+ Choose this option to enable the ION Memory Manager,
  used by Android to efficiently allocate buffers
  from userspace that can be shared between drivers.
  If you're not using Android its probably safe to



The formatting here looks a little funny, did you manually edit the
patch to take out the diff-stat and index? The patch seem to apply
okay, so I'm just curious. Either way,

Acked-by: Laura Abbott <labb...@redhat.com>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Laura Abbott]

On 03/08/2018 04:45 PM, Liam Mark wrote:

On Wed, 7 Mar 2018, Laura Abbott wrote:


On 02/28/2018 09:18 PM, Liam Mark wrote:

The issue:

Currently in ION if you allocate uncached memory it is possible that there
are still dirty lines in the cache.  And often these dirty lines in the
cache are the zeros which were meant to clear out any sensitive kernel
data.

What this means is that if you allocate uncached memory from ION, and then
subsequently write to that buffer (using the uncached mapping you are
provided by ION) then the data you have written could be corrupted at some
point in the future if a dirty line is evicted from the cache.

Also this means there is a potential security issue.  If an un-privileged
userspace user allocated uncached memory (for example from the system heap)
and then if they were to read from that buffer (through the un-cached
mapping they are provided by ION), and if some of the zeros which were
written to that memory are still in the cache then this un-privileged
userspace user could read potentially sensitive kernel data.


For the use case you are describing we don't actually need the
memory to be non-cached until it comes time to do the dma mapping.
Here's a proposal to shoot holes in:

- Before any dma_buf attach happens, all mmap mappings are cached
- At the time attach happens, we shoot down any existing userspace
mappings, do the dma_map with appropriate flags to clean the pages
and then allow remapping to userspace as uncached. Really this
looks like a variation on the old Ion faulting code which I removed
except it's for uncached buffers instead of cached buffers.



Thanks Laura, I will take a look to see if I can think of any concerns.

Initial thoughts.
- What about any kernel mappings (kmap/vmap) the client has made?



We could either synchronize with dma_buf_{begin,end}_cpu_access
or just disallow the mapping to happen if there's an outstanding
kmap or vmap. Is this an actual problem or only theoretical?


- I guess it would be tempting to only do this behavior for memory that
came from buddy (as opposed to the pool since it should be clean), but we
would need to be careful that no pages sneak into the pool without being
cleaned (example: client allocs then frees without ever call
dma_buf_attach).



You're welcome to try that optimization but I think we should
focus on the basics first. Honestly it might make sense just to
have a single pool at this point since the cost of syncing
is not happening on the allocation path.


Potential problems:
- I'm not 100% about the behavior here if the attaching device
is already dma_coherent. I also consider uncached mappings
enough of a device specific optimization that you shouldn't
do them unless you know it's needed.


I don't believe we want to allow uncached memory to be dma mapped by an
io-coherent device and this is something I would like to eventually block.

Since there is always a kernel cached mapping for ION uncached memory then
speculative access could still be putting lines in the cache, so when an
io-coherent device tries to read this uncached memory its snoop into the
cache could find one of these 'stale' clean cache lines and end up using
stale data.
Agree?



Sounds right.


- The locking/sequencing with userspace could be tricky
since userspace may not like us ripping mappings out from
underneath if it's trying to access.


Perhaps delay this work to the dma_map_attachment call since when the data
is dma mapped the CPU shouldn't be accessing it?

Or worst case perhaps fail all map attempts to uncached memory until the
memory has been dma mapped (and cleaned) at least once?



My concern was mostly concurrent userspace access on a buffer
that's being dma_mapped but that sounds racy to begin with.

I suggested disallowing mmap until dma_mapping before and I thought
that was not possible?

Thanks,
Laura


Thanks,
Liam

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project



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

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

[Laura Abbott]

On 02/28/2018 09:18 PM, Liam Mark wrote:

The issue:

Currently in ION if you allocate uncached memory it is possible that there
are still dirty lines in the cache.  And often these dirty lines in the
cache are the zeros which were meant to clear out any sensitive kernel
data.

What this means is that if you allocate uncached memory from ION, and then
subsequently write to that buffer (using the uncached mapping you are
provided by ION) then the data you have written could be corrupted at some
point in the future if a dirty line is evicted from the cache.

Also this means there is a potential security issue.  If an un-privileged
userspace user allocated uncached memory (for example from the system heap)
and then if they were to read from that buffer (through the un-cached
mapping they are provided by ION), and if some of the zeros which were
written to that memory are still in the cache then this un-privileged
userspace user could read potentially sensitive kernel data.


For the use case you are describing we don't actually need the
memory to be non-cached until it comes time to do the dma mapping.
Here's a proposal to shoot holes in:

- Before any dma_buf attach happens, all mmap mappings are cached
- At the time attach happens, we shoot down any existing userspace
mappings, do the dma_map with appropriate flags to clean the pages
and then allow remapping to userspace as uncached. Really this
looks like a variation on the old Ion faulting code which I removed
except it's for uncached buffers instead of cached buffers.

Potential problems:
- I'm not 100% about the behavior here if the attaching device
is already dma_coherent. I also consider uncached mappings
enough of a device specific optimization that you shouldn't
do them unless you know it's needed.
- The locking/sequencing with userspace could be tricky
since userspace may not like us ripping mappings out from
underneath if it's trying to access.

Thoughts?

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

Re: [PATCH 2/2] selftests: ion: Add simple test with the vgem driver

[Laura Abbott]

On 02/26/2018 09:07 AM, Shuah Khan wrote:

On 02/19/2018 11:33 AM, Daniel Vetter wrote:

On Mon, Feb 19, 2018 at 10:18:21AM -0800, Laura Abbott wrote:

On 02/19/2018 07:31 AM, Daniel Vetter wrote:

On Thu, Feb 15, 2018 at 05:24:45PM -0800, Laura Abbott wrote:

Ion is designed to be a framework used by other clients who perform
operations on the buffer. Use the DRM vgem client as a simple consumer.
In conjunction with the dma-buf sync ioctls, this tests the full attach/map
path for the system heap.

Signed-off-by: Laura Abbott <labb...@redhat.com>
---
   tools/testing/selftests/android/ion/Makefile  |   3 +-
   tools/testing/selftests/android/ion/config|   1 +
   tools/testing/selftests/android/ion/ionmap_test.c | 136 
++
   3 files changed, 139 insertions(+), 1 deletion(-)
   create mode 100644 tools/testing/selftests/android/ion/ionmap_test.c

diff --git a/tools/testing/selftests/android/ion/Makefile 
b/tools/testing/selftests/android/ion/Makefile
index 96e0c448b39d..d23b6d537d8b 100644
--- a/tools/testing/selftests/android/ion/Makefile
+++ b/tools/testing/selftests/android/ion/Makefile
@@ -2,7 +2,7 @@
   INCLUDEDIR := -I. -I../../../../../drivers/staging/android/uapi/
   CFLAGS := $(CFLAGS) $(INCLUDEDIR) -Wall -O2 -g
-TEST_GEN_FILES := ionapp_export ionapp_import
+TEST_GEN_FILES := ionapp_export ionapp_import ionmap_test
   all: $(TEST_GEN_FILES)
@@ -14,3 +14,4 @@ include ../../lib.mk
   $(OUTPUT)/ionapp_export: ionapp_export.c ipcsocket.c ionutils.c
   $(OUTPUT)/ionapp_import: ionapp_import.c ipcsocket.c ionutils.c
+$(OUTPUT)/ionmap_test: ionmap_test.c ionutils.c
diff --git a/tools/testing/selftests/android/ion/config 
b/tools/testing/selftests/android/ion/config
index 19db6ca9aa2b..b4ad748a9dd9 100644
--- a/tools/testing/selftests/android/ion/config
+++ b/tools/testing/selftests/android/ion/config
@@ -2,3 +2,4 @@ CONFIG_ANDROID=y
   CONFIG_STAGING=y
   CONFIG_ION=y
   CONFIG_ION_SYSTEM_HEAP=y
+CONFIG_DRM_VGEM=y
diff --git a/tools/testing/selftests/android/ion/ionmap_test.c 
b/tools/testing/selftests/android/ion/ionmap_test.c
new file mode 100644
index ..dab36b06b37d
--- /dev/null
+++ b/tools/testing/selftests/android/ion/ionmap_test.c
@@ -0,0 +1,136 @@
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+
+#include 
+
+#include 
+
+#include "ion.h"
+#include "ionutils.h"
+
+int check_vgem(int fd)
+{
+   drm_version_t version = { 0 };
+   char name[5];
+   int ret;
+
+   version.name_len = 4;
+   version.name = name;
+
+   ret = ioctl(fd, DRM_IOCTL_VERSION, );
+   if (ret)
+   return 1;
+
+   return strcmp(name, "vgem");
+}
+
+int open_vgem(void)
+{
+   int i, fd;
+   const char *drmstr = "/dev/dri/card";
+
+   fd = -1;
+   for (i = 0; i < 16; i++) {
+   char name[80];
+
+   sprintf(name, "%s%u", drmstr, i);
+
+   fd = open(name, O_RDWR);
+   if (fd < 0)
+   continue;
+
+   if (check_vgem(fd)) {
+   close(fd);
+   continue;
+   } else {
+   break;
+   }
+
+   }
+   return fd;
+}
+
+int import_vgem_fd(int vgem_fd, int dma_buf_fd, uint32_t *handle)
+{
+   struct drm_prime_handle import_handle = { 0 };
+   int ret;
+
+   import_handle.fd = dma_buf_fd;
+   import_handle.flags = 0;
+   import_handle.handle = 0;
+
+   ret = ioctl(vgem_fd, DRM_IOCTL_PRIME_FD_TO_HANDLE, _handle);
+   if (ret == 0)
+   *handle = import_handle.handle;
+   return ret;
+}
+
+void close_handle(int vgem_fd, uint32_t handle)
+{
+   struct drm_gem_close close = { 0 };
+
+   close.handle = handle;
+   ioctl(vgem_fd, DRM_IOCTL_GEM_CLOSE, );
+}
+
+int main()
+{
+   int ret, vgem_fd;
+   struct ion_buffer_info info;
+   uint32_t handle = 0;
+   struct dma_buf_sync sync = { 0 };
+
+   info.heap_type = ION_HEAP_TYPE_SYSTEM;
+   info.heap_size = 4096;
+   info.flag_type = ION_FLAG_CACHED;
+
+   ret = ion_export_buffer_fd();
+   if (ret < 0) {
+   printf("ion buffer alloc failed\n");
+   return -1;
+   }
+
+   vgem_fd = open_vgem();
+   if (vgem_fd < 0) {
+   ret = vgem_fd;
+   printf("Failed to open vgem\n");
+   goto out_ion;
+   }
+
+   ret = import_vgem_fd(vgem_fd, info.buffd, );
+
+   if (ret < 0) {
+   printf("Failed to import buffer\n");
+   goto out_vgem;
+   }
+
+   sync.flags = DMA_BUF_SYNC_START | DMA_BUF_SYNC_RW;
+   ret = ioctl(info.buffd, DMA_BUF_IOCTL_SYNC, );
+   if (ret)
+   printf("sync start failed %d\n", errno);
+
+   memset(info.buffer, 0xff, 4096)

[PATCHv2 1/2] selftests: ion: Remove some prints

[Laura Abbott]

There's no need to print messages each time we alloc and free. Remove them.

Signed-off-by: Laura Abbott <labb...@redhat.com>
---
v2: No changes
---
 tools/testing/selftests/android/ion/ionutils.c | 6 --
 1 file changed, 6 deletions(-)

diff --git a/tools/testing/selftests/android/ion/ionutils.c 
b/tools/testing/selftests/android/ion/ionutils.c
index ce69c14f51fa..7d1d37c4ef6a 100644
--- a/tools/testing/selftests/android/ion/ionutils.c
+++ b/tools/testing/selftests/android/ion/ionutils.c
@@ -80,11 +80,6 @@ int ion_export_buffer_fd(struct ion_buffer_info *ion_info)
heap_id = MAX_HEAP_COUNT + 1;
for (i = 0; i < query.cnt; i++) {
if (heap_data[i].type == ion_info->heap_type) {
-   printf("--\n");
-   printf("heap type: %d\n", heap_data[i].type);
-   printf("  heap id: %d\n", heap_data[i].heap_id);
-   printf("heap name: %s\n", heap_data[i].name);
-   printf("--\n");
heap_id = heap_data[i].heap_id;
break;
}
@@ -204,7 +199,6 @@ void ion_close_buffer_fd(struct ion_buffer_info *ion_info)
/* Finally, close the client fd */
if (ion_info->ionfd > 0)
close(ion_info->ionfd);
-   printf("<%s>: buffer release successfully\n", __func__);
}
 }
 
-- 
2.14.3

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

[PATCHv2 2/2] selftests: ion: Add simple test with the vgem driver

[Laura Abbott]

Ion is designed to be a framework used by other clients who perform
operations on the buffer. Use the DRM vgem client as a simple consumer.
In conjunction with the dma-buf sync ioctls, this tests the full attach/map
path for the system heap.

Reviewed-by: Daniel Vetter <daniel.vet...@ffwll.ch>
Signed-off-by: Laura Abbott <labb...@redhat.com>
---
v2: Introduce drmIoctl wrapper per suggestion of Daniel Vetter to better
match how DRM ioctls actually work.
---
 tools/testing/selftests/android/ion/Makefile  |   3 +-
 tools/testing/selftests/android/ion/config|   1 +
 tools/testing/selftests/android/ion/ionmap_test.c | 149 ++
 3 files changed, 152 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/android/ion/ionmap_test.c

diff --git a/tools/testing/selftests/android/ion/Makefile 
b/tools/testing/selftests/android/ion/Makefile
index 96e0c448b39d..d23b6d537d8b 100644
--- a/tools/testing/selftests/android/ion/Makefile
+++ b/tools/testing/selftests/android/ion/Makefile
@@ -2,7 +2,7 @@
 INCLUDEDIR := -I. -I../../../../../drivers/staging/android/uapi/
 CFLAGS := $(CFLAGS) $(INCLUDEDIR) -Wall -O2 -g
 
-TEST_GEN_FILES := ionapp_export ionapp_import
+TEST_GEN_FILES := ionapp_export ionapp_import ionmap_test
 
 all: $(TEST_GEN_FILES)
 
@@ -14,3 +14,4 @@ include ../../lib.mk
 
 $(OUTPUT)/ionapp_export: ionapp_export.c ipcsocket.c ionutils.c
 $(OUTPUT)/ionapp_import: ionapp_import.c ipcsocket.c ionutils.c
+$(OUTPUT)/ionmap_test: ionmap_test.c ionutils.c
diff --git a/tools/testing/selftests/android/ion/config 
b/tools/testing/selftests/android/ion/config
index 19db6ca9aa2b..b4ad748a9dd9 100644
--- a/tools/testing/selftests/android/ion/config
+++ b/tools/testing/selftests/android/ion/config
@@ -2,3 +2,4 @@ CONFIG_ANDROID=y
 CONFIG_STAGING=y
 CONFIG_ION=y
 CONFIG_ION_SYSTEM_HEAP=y
+CONFIG_DRM_VGEM=y
diff --git a/tools/testing/selftests/android/ion/ionmap_test.c 
b/tools/testing/selftests/android/ion/ionmap_test.c
new file mode 100644
index ..58ecdc3511d2
--- /dev/null
+++ b/tools/testing/selftests/android/ion/ionmap_test.c
@@ -0,0 +1,149 @@
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+
+#include 
+
+#include 
+
+#include "ion.h"
+#include "ionutils.h"
+
+/* Local copy of drmIoctl to match the expected behavior */
+static int drmIoctl(int fd, unsigned long request, void *arg)
+{
+   int ret;
+
+   do {
+   ret = ioctl(fd, request, arg);
+   } while (ret == -1 && (errno == EINTR || errno == EAGAIN));
+}
+
+int check_vgem(int fd)
+{
+   drm_version_t version = { 0 };
+   char name[5];
+   int ret;
+
+   version.name_len = 4;
+   version.name = name;
+
+   ret = drmIoctl(fd, DRM_IOCTL_VERSION, );
+   if (ret)
+   return 1;
+
+   return strcmp(name, "vgem");
+}
+
+int open_vgem(void)
+{
+   int i, fd;
+   const char *drmstr = "/dev/dri/card";
+
+   fd = -1;
+   for (i = 0; i < 16; i++) {
+   char name[80];
+
+   sprintf(name, "%s%u", drmstr, i);
+
+   fd = open(name, O_RDWR);
+   if (fd < 0)
+   continue;
+
+   if (check_vgem(fd)) {
+   close(fd);
+   continue;
+   } else {
+   break;
+   }
+
+   }
+   return fd;
+}
+
+int import_vgem_fd(int vgem_fd, int dma_buf_fd, uint32_t *handle)
+{
+   struct drm_prime_handle import_handle = { 0 };
+   int ret;
+
+   import_handle.fd = dma_buf_fd;
+   import_handle.flags = 0;
+   import_handle.handle = 0;
+
+   ret = drmIoctl(vgem_fd, DRM_IOCTL_PRIME_FD_TO_HANDLE, _handle);
+   if (ret == 0)
+   *handle = import_handle.handle;
+   return ret;
+}
+
+void close_handle(int vgem_fd, uint32_t handle)
+{
+   struct drm_gem_close close = { 0 };
+
+   close.handle = handle;
+   drmIoctl(vgem_fd, DRM_IOCTL_GEM_CLOSE, );
+}
+
+int main()
+{
+   int ret, vgem_fd;
+   struct ion_buffer_info info;
+   uint32_t handle = 0;
+   struct dma_buf_sync sync = { 0 };
+
+   info.heap_type = ION_HEAP_TYPE_SYSTEM;
+   info.heap_size = 4096;
+   info.flag_type = ION_FLAG_CACHED;
+
+   ret = ion_export_buffer_fd();
+   if (ret < 0) {
+   printf("ion buffer alloc failed\n");
+   return -1;
+   }
+
+   vgem_fd = open_vgem();
+   if (vgem_fd < 0) {
+   ret = vgem_fd;
+   printf("Failed to open vgem\n");
+   goto out_ion;
+   }
+
+   ret = import_vgem_fd(vgem_fd, info.buffd, );
+
+   if (ret < 0) {
+   printf("Failed to import buffer\n");
+   goto out_vgem;
+   }
+
+   /*
+* While not strictly drm, the drmIoctl proper

[PATCHv2 0/2] Ion unit test with VGEM

[Laura Abbott]

Hi,

This is v2 of the series to add a unit test to Ion with VGEM. From v1:

Ion hasn't had much in the way of unit tests and fixing that is
something that needs to happen before it can move out of staging. The
difficult part of testing parts of Ion is that it relies on having a
kernel driver to actually make some of the dma_buf calls. The vgem
DRM driver exists mostly for testing and works great for this case.

Changes since v1:
- Dropped RFC
- Feedback/review from Daniel Vetter about DRM ioctls

Thanks,
Laura

Laura Abbott (2):
  selftests: ion: Remove some prints
  selftests: ion: Add simple test with the vgem driver

 tools/testing/selftests/android/ion/Makefile  |   3 +-
 tools/testing/selftests/android/ion/config|   1 +
 tools/testing/selftests/android/ion/ionmap_test.c | 149 ++
 tools/testing/selftests/android/ion/ionutils.c|   6 -
 4 files changed, 152 insertions(+), 7 deletions(-)
 create mode 100644 tools/testing/selftests/android/ion/ionmap_test.c

-- 
2.14.3

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

Re: [PATCH 2/2] selftests: ion: Add simple test with the vgem driver

[Laura Abbott]

On 02/19/2018 07:31 AM, Daniel Vetter wrote:

On Thu, Feb 15, 2018 at 05:24:45PM -0800, Laura Abbott wrote:

Ion is designed to be a framework used by other clients who perform
operations on the buffer. Use the DRM vgem client as a simple consumer.
In conjunction with the dma-buf sync ioctls, this tests the full attach/map
path for the system heap.

Signed-off-by: Laura Abbott <labb...@redhat.com>
---
  tools/testing/selftests/android/ion/Makefile  |   3 +-
  tools/testing/selftests/android/ion/config|   1 +
  tools/testing/selftests/android/ion/ionmap_test.c | 136 ++
  3 files changed, 139 insertions(+), 1 deletion(-)
  create mode 100644 tools/testing/selftests/android/ion/ionmap_test.c

diff --git a/tools/testing/selftests/android/ion/Makefile 
b/tools/testing/selftests/android/ion/Makefile
index 96e0c448b39d..d23b6d537d8b 100644
--- a/tools/testing/selftests/android/ion/Makefile
+++ b/tools/testing/selftests/android/ion/Makefile
@@ -2,7 +2,7 @@
  INCLUDEDIR := -I. -I../../../../../drivers/staging/android/uapi/
  CFLAGS := $(CFLAGS) $(INCLUDEDIR) -Wall -O2 -g
  
-TEST_GEN_FILES := ionapp_export ionapp_import

+TEST_GEN_FILES := ionapp_export ionapp_import ionmap_test
  
  all: $(TEST_GEN_FILES)
  
@@ -14,3 +14,4 @@ include ../../lib.mk
  
  $(OUTPUT)/ionapp_export: ionapp_export.c ipcsocket.c ionutils.c

  $(OUTPUT)/ionapp_import: ionapp_import.c ipcsocket.c ionutils.c
+$(OUTPUT)/ionmap_test: ionmap_test.c ionutils.c
diff --git a/tools/testing/selftests/android/ion/config 
b/tools/testing/selftests/android/ion/config
index 19db6ca9aa2b..b4ad748a9dd9 100644
--- a/tools/testing/selftests/android/ion/config
+++ b/tools/testing/selftests/android/ion/config
@@ -2,3 +2,4 @@ CONFIG_ANDROID=y
  CONFIG_STAGING=y
  CONFIG_ION=y
  CONFIG_ION_SYSTEM_HEAP=y
+CONFIG_DRM_VGEM=y
diff --git a/tools/testing/selftests/android/ion/ionmap_test.c 
b/tools/testing/selftests/android/ion/ionmap_test.c
new file mode 100644
index ..dab36b06b37d
--- /dev/null
+++ b/tools/testing/selftests/android/ion/ionmap_test.c
@@ -0,0 +1,136 @@
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+
+#include 
+
+#include 
+
+#include "ion.h"
+#include "ionutils.h"
+
+int check_vgem(int fd)
+{
+   drm_version_t version = { 0 };
+   char name[5];
+   int ret;
+
+   version.name_len = 4;
+   version.name = name;
+
+   ret = ioctl(fd, DRM_IOCTL_VERSION, );
+   if (ret)
+   return 1;
+
+   return strcmp(name, "vgem");
+}
+
+int open_vgem(void)
+{
+   int i, fd;
+   const char *drmstr = "/dev/dri/card";
+
+   fd = -1;
+   for (i = 0; i < 16; i++) {
+   char name[80];
+
+   sprintf(name, "%s%u", drmstr, i);
+
+   fd = open(name, O_RDWR);
+   if (fd < 0)
+   continue;
+
+   if (check_vgem(fd)) {
+   close(fd);
+   continue;
+   } else {
+   break;
+   }
+
+   }
+   return fd;
+}
+
+int import_vgem_fd(int vgem_fd, int dma_buf_fd, uint32_t *handle)
+{
+   struct drm_prime_handle import_handle = { 0 };
+   int ret;
+
+   import_handle.fd = dma_buf_fd;
+   import_handle.flags = 0;
+   import_handle.handle = 0;
+
+   ret = ioctl(vgem_fd, DRM_IOCTL_PRIME_FD_TO_HANDLE, _handle);
+   if (ret == 0)
+   *handle = import_handle.handle;
+   return ret;
+}
+
+void close_handle(int vgem_fd, uint32_t handle)
+{
+   struct drm_gem_close close = { 0 };
+
+   close.handle = handle;
+   ioctl(vgem_fd, DRM_IOCTL_GEM_CLOSE, );
+}
+
+int main()
+{
+   int ret, vgem_fd;
+   struct ion_buffer_info info;
+   uint32_t handle = 0;
+   struct dma_buf_sync sync = { 0 };
+
+   info.heap_type = ION_HEAP_TYPE_SYSTEM;
+   info.heap_size = 4096;
+   info.flag_type = ION_FLAG_CACHED;
+
+   ret = ion_export_buffer_fd();
+   if (ret < 0) {
+   printf("ion buffer alloc failed\n");
+   return -1;
+   }
+
+   vgem_fd = open_vgem();
+   if (vgem_fd < 0) {
+   ret = vgem_fd;
+   printf("Failed to open vgem\n");
+   goto out_ion;
+   }
+
+   ret = import_vgem_fd(vgem_fd, info.buffd, );
+
+   if (ret < 0) {
+   printf("Failed to import buffer\n");
+   goto out_vgem;
+   }
+
+   sync.flags = DMA_BUF_SYNC_START | DMA_BUF_SYNC_RW;
+   ret = ioctl(info.buffd, DMA_BUF_IOCTL_SYNC, );
+   if (ret)
+   printf("sync start failed %d\n", errno);
+
+   memset(info.buffer, 0xff, 4096);
+
+   sync.flags = DMA_BUF_SYNC_END | DMA_BUF_SYNC_RW;
+   ret = ioctl(info.buffd, DMA_BUF_IOCTL_SYNC, );
+   if (ret)
+   printf("sync en

Re: [PATCH] staging: android: ion: Change dma_buf_kmap()/dma_buf_kmap_atomic() implementation

[Laura Abbott]

On 02/16/2018 04:17 AM, Alexey Skidanov wrote:



On 02/16/2018 01:48 AM, Laura Abbott wrote:

On 02/12/2018 02:33 PM, Alexey Skidanov wrote:

Current ion kernel mapping implementation uses vmap() to map previously
allocated buffers into kernel virtual address space.

On 32-bit platforms, vmap() might fail on large enough buffers due to the
limited available vmalloc space. dma_buf_kmap() should guarantee that
only one page is mapped at a time.

To fix this, kmap()/kmap_atomic() is used to implement the appropriate
interfaces.

Signed-off-by: Alexey Skidanov <alexey.skida...@intel.com>
---
   drivers/staging/android/ion/ion.c | 97
+++
   drivers/staging/android/ion/ion.h |  1 -
   2 files changed, 48 insertions(+), 50 deletions(-)

diff --git a/drivers/staging/android/ion/ion.c
b/drivers/staging/android/ion/ion.c
index 57e0d80..75830e3 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -27,6 +27,7 @@
   #include 
   #include 
   #include 
+#include 
     #include "ion.h"
   @@ -119,8 +120,6 @@ static struct ion_buffer
*ion_buffer_create(struct ion_heap *heap,
     void ion_buffer_destroy(struct ion_buffer *buffer)
   {
-    if (WARN_ON(buffer->kmap_cnt > 0))
-    buffer->heap->ops->unmap_kernel(buffer->heap, buffer);
   buffer->heap->ops->free(buffer);
   kfree(buffer);
   }
@@ -140,34 +139,6 @@ static void _ion_buffer_destroy(struct ion_buffer
*buffer)
   ion_buffer_destroy(buffer);
   }
   -static void *ion_buffer_kmap_get(struct ion_buffer *buffer)
-{
-    void *vaddr;
-
-    if (buffer->kmap_cnt) {
-    buffer->kmap_cnt++;
-    return buffer->vaddr;
-    }
-    vaddr = buffer->heap->ops->map_kernel(buffer->heap, buffer);
-    if (WARN_ONCE(!vaddr,
-  "heap->ops->map_kernel should return ERR_PTR on error"))
-    return ERR_PTR(-EINVAL);
-    if (IS_ERR(vaddr))
-    return vaddr;
-    buffer->vaddr = vaddr;
-    buffer->kmap_cnt++;
-    return vaddr;
-}
-
-static void ion_buffer_kmap_put(struct ion_buffer *buffer)
-{
-    buffer->kmap_cnt--;
-    if (!buffer->kmap_cnt) {
-    buffer->heap->ops->unmap_kernel(buffer->heap, buffer);
-    buffer->vaddr = NULL;
-    }
-}
-
   static struct sg_table *dup_sg_table(struct sg_table *table)
   {
   struct sg_table *new_table;
@@ -305,34 +276,68 @@ static void ion_dma_buf_release(struct dma_buf
*dmabuf)
   _ion_buffer_destroy(buffer);
   }
   +static inline int sg_page_count(struct scatterlist *sg)
+{
+    return PAGE_ALIGN(sg->offset + sg->length) >> PAGE_SHIFT;
+}
+
+static struct page *ion_dma_buf_get_page(struct sg_table *sg_table,
+ unsigned long offset)
+{
+    struct page *page;
+    struct scatterlist *sg;
+    int i;
+    unsigned int nr_pages;
+
+    nr_pages = 0;
+    page = NULL;
+    /* Find the page with specified offset */
+    for_each_sg(sg_table->sgl, sg, sg_table->nents, i) {
+    if (nr_pages + sg_page_count(sg) > offset) {
+    page = nth_page(sg_page(sg), offset - nr_pages);
+    break;
+    }
+
+    nr_pages += sg_page_count(sg);
+    }
+
+    return page;
+}
   static void *ion_dma_buf_kmap(struct dma_buf *dmabuf, unsigned long
offset)
   {
   struct ion_buffer *buffer = dmabuf->priv;
   -    return buffer->vaddr + offset * PAGE_SIZE;
+    return kmap(ion_dma_buf_get_page(buffer->sg_table, offset));
   }


This unfortunately doesn't work for uncached buffers. We need to create
an uncached alias otherwise we break what little coherency that guarantees.
I'm still convinced that we can't actually do the uncached buffers
correctly and they should be removed...

Thanks,
Laura


I assume that your concern is possible cache coherency issue as result
of the memory access with the different cache attributes. Especially, if
these accesses are from different context.

But dma-buf requires that the cpu access should be started with
dma_buf_start_cpu_access() and ended with dma_buf_end_cpu_access() (and
with appropriate ioctl calls from the user space) to ensure that there
is no IO coherency issues. That is, the appropriate cache lines are
invalidated before the access and are flushed after the access (in case
of read/write access).

So, it seems, that in the end of each CPU access, the most update copy
of the buffer is in the RAM.

Probably, I'm wrong but I don't see the issue.



Part of the issue is that uncached buffers are often used so we don't
need to do any cache operations. We do the sync currently but there's
interest in removing it for uncached buffers.

I also haven't looked at aliasing rules on x86 closely but at least
on arm, coherency with different cache attributes is difficult to
get right and has a big "not recommended" warning in the description.
You may be right that the above sequence would work out 

[RFC PATCH 0/2] Ion unit test with VGEM

[Laura Abbott]

Hi,

Ion hasn't had much in the way of unit tests and fixing that is
something that needs to happen before it can move out of staging. The
difficult part of testing parts of Ion is that it relies on having a
kernel driver to actually make some of the dma_buf calls. The vgem
DRM driver exists mostly for testing and works great for this case. I
went back and forth about trying to put this in the existing graphics
test suite but I think having something in the self-tests directory is
easier.

I'm mostly interested in feedback about the use of the DRM APIs but I
appreciate any and all comments.

Thanks,
Laura

Laura Abbott (2):
  selftests: ion: Remove some prints
  selftests: ion: Add simple test with the vgem driver

 tools/testing/selftests/android/ion/Makefile  |   3 +-
 tools/testing/selftests/android/ion/config|   1 +
 tools/testing/selftests/android/ion/ionmap_test.c | 136 ++
 tools/testing/selftests/android/ion/ionutils.c|   6 -
 4 files changed, 139 insertions(+), 7 deletions(-)
 create mode 100644 tools/testing/selftests/android/ion/ionmap_test.c

-- 
2.14.3

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

[PATCH 2/2] selftests: ion: Add simple test with the vgem driver

[Laura Abbott]

Ion is designed to be a framework used by other clients who perform
operations on the buffer. Use the DRM vgem client as a simple consumer.
In conjunction with the dma-buf sync ioctls, this tests the full attach/map
path for the system heap.

Signed-off-by: Laura Abbott <labb...@redhat.com>
---
 tools/testing/selftests/android/ion/Makefile  |   3 +-
 tools/testing/selftests/android/ion/config|   1 +
 tools/testing/selftests/android/ion/ionmap_test.c | 136 ++
 3 files changed, 139 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/android/ion/ionmap_test.c

diff --git a/tools/testing/selftests/android/ion/Makefile 
b/tools/testing/selftests/android/ion/Makefile
index 96e0c448b39d..d23b6d537d8b 100644
--- a/tools/testing/selftests/android/ion/Makefile
+++ b/tools/testing/selftests/android/ion/Makefile
@@ -2,7 +2,7 @@
 INCLUDEDIR := -I. -I../../../../../drivers/staging/android/uapi/
 CFLAGS := $(CFLAGS) $(INCLUDEDIR) -Wall -O2 -g
 
-TEST_GEN_FILES := ionapp_export ionapp_import
+TEST_GEN_FILES := ionapp_export ionapp_import ionmap_test
 
 all: $(TEST_GEN_FILES)
 
@@ -14,3 +14,4 @@ include ../../lib.mk
 
 $(OUTPUT)/ionapp_export: ionapp_export.c ipcsocket.c ionutils.c
 $(OUTPUT)/ionapp_import: ionapp_import.c ipcsocket.c ionutils.c
+$(OUTPUT)/ionmap_test: ionmap_test.c ionutils.c
diff --git a/tools/testing/selftests/android/ion/config 
b/tools/testing/selftests/android/ion/config
index 19db6ca9aa2b..b4ad748a9dd9 100644
--- a/tools/testing/selftests/android/ion/config
+++ b/tools/testing/selftests/android/ion/config
@@ -2,3 +2,4 @@ CONFIG_ANDROID=y
 CONFIG_STAGING=y
 CONFIG_ION=y
 CONFIG_ION_SYSTEM_HEAP=y
+CONFIG_DRM_VGEM=y
diff --git a/tools/testing/selftests/android/ion/ionmap_test.c 
b/tools/testing/selftests/android/ion/ionmap_test.c
new file mode 100644
index ..dab36b06b37d
--- /dev/null
+++ b/tools/testing/selftests/android/ion/ionmap_test.c
@@ -0,0 +1,136 @@
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+
+#include 
+
+#include 
+
+#include "ion.h"
+#include "ionutils.h"
+
+int check_vgem(int fd)
+{
+   drm_version_t version = { 0 };
+   char name[5];
+   int ret;
+
+   version.name_len = 4;
+   version.name = name;
+
+   ret = ioctl(fd, DRM_IOCTL_VERSION, );
+   if (ret)
+   return 1;
+
+   return strcmp(name, "vgem");
+}
+
+int open_vgem(void)
+{
+   int i, fd;
+   const char *drmstr = "/dev/dri/card";
+
+   fd = -1;
+   for (i = 0; i < 16; i++) {
+   char name[80];
+
+   sprintf(name, "%s%u", drmstr, i);
+
+   fd = open(name, O_RDWR);
+   if (fd < 0)
+   continue;
+
+   if (check_vgem(fd)) {
+   close(fd);
+   continue;
+   } else {
+   break;
+   }
+
+   }
+   return fd;
+}
+
+int import_vgem_fd(int vgem_fd, int dma_buf_fd, uint32_t *handle)
+{
+   struct drm_prime_handle import_handle = { 0 };
+   int ret;
+
+   import_handle.fd = dma_buf_fd;
+   import_handle.flags = 0;
+   import_handle.handle = 0;
+
+   ret = ioctl(vgem_fd, DRM_IOCTL_PRIME_FD_TO_HANDLE, _handle);
+   if (ret == 0)
+   *handle = import_handle.handle;
+   return ret;
+}
+
+void close_handle(int vgem_fd, uint32_t handle)
+{
+   struct drm_gem_close close = { 0 };
+
+   close.handle = handle;
+   ioctl(vgem_fd, DRM_IOCTL_GEM_CLOSE, );
+}
+
+int main()
+{
+   int ret, vgem_fd;
+   struct ion_buffer_info info;
+   uint32_t handle = 0;
+   struct dma_buf_sync sync = { 0 };
+
+   info.heap_type = ION_HEAP_TYPE_SYSTEM;
+   info.heap_size = 4096;
+   info.flag_type = ION_FLAG_CACHED;
+
+   ret = ion_export_buffer_fd();
+   if (ret < 0) {
+   printf("ion buffer alloc failed\n");
+   return -1;
+   }
+
+   vgem_fd = open_vgem();
+   if (vgem_fd < 0) {
+   ret = vgem_fd;
+   printf("Failed to open vgem\n");
+   goto out_ion;
+   }
+
+   ret = import_vgem_fd(vgem_fd, info.buffd, );
+
+   if (ret < 0) {
+   printf("Failed to import buffer\n");
+   goto out_vgem;
+   }
+
+   sync.flags = DMA_BUF_SYNC_START | DMA_BUF_SYNC_RW;
+   ret = ioctl(info.buffd, DMA_BUF_IOCTL_SYNC, );
+   if (ret)
+   printf("sync start failed %d\n", errno);
+
+   memset(info.buffer, 0xff, 4096);
+
+   sync.flags = DMA_BUF_SYNC_END | DMA_BUF_SYNC_RW;
+   ret = ioctl(info.buffd, DMA_BUF_IOCTL_SYNC, );
+   if (ret)
+   printf("sync end failed %d\n", errno);
+
+   close_handle(vgem_fd, handle);
+   ret = 0;
+
+out_vgem:
+   

[PATCH 1/2] selftests: ion: Remove some prints

[Laura Abbott]

There's no need to print messages each time we alloc and free. Remove them.

Signed-off-by: Laura Abbott <labb...@redhat.com>
---
 tools/testing/selftests/android/ion/ionutils.c | 6 --
 1 file changed, 6 deletions(-)

diff --git a/tools/testing/selftests/android/ion/ionutils.c 
b/tools/testing/selftests/android/ion/ionutils.c
index ce69c14f51fa..7d1d37c4ef6a 100644
--- a/tools/testing/selftests/android/ion/ionutils.c
+++ b/tools/testing/selftests/android/ion/ionutils.c
@@ -80,11 +80,6 @@ int ion_export_buffer_fd(struct ion_buffer_info *ion_info)
heap_id = MAX_HEAP_COUNT + 1;
for (i = 0; i < query.cnt; i++) {
if (heap_data[i].type == ion_info->heap_type) {
-   printf("--\n");
-   printf("heap type: %d\n", heap_data[i].type);
-   printf("  heap id: %d\n", heap_data[i].heap_id);
-   printf("heap name: %s\n", heap_data[i].name);
-   printf("--\n");
heap_id = heap_data[i].heap_id;
break;
}
@@ -204,7 +199,6 @@ void ion_close_buffer_fd(struct ion_buffer_info *ion_info)
/* Finally, close the client fd */
if (ion_info->ionfd > 0)
close(ion_info->ionfd);
-   printf("<%s>: buffer release successfully\n", __func__);
}
 }
 
-- 
2.14.3

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

Re: [PATCH] staging: android: ion: Change dma_buf_kmap()/dma_buf_kmap_atomic() implementation

[Laura Abbott]

On 02/12/2018 02:33 PM, Alexey Skidanov wrote:

Current ion kernel mapping implementation uses vmap() to map previously
allocated buffers into kernel virtual address space.

On 32-bit platforms, vmap() might fail on large enough buffers due to the
limited available vmalloc space. dma_buf_kmap() should guarantee that
only one page is mapped at a time.

To fix this, kmap()/kmap_atomic() is used to implement the appropriate
interfaces.

Signed-off-by: Alexey Skidanov 
---
  drivers/staging/android/ion/ion.c | 97 +++
  drivers/staging/android/ion/ion.h |  1 -
  2 files changed, 48 insertions(+), 50 deletions(-)

diff --git a/drivers/staging/android/ion/ion.c 
b/drivers/staging/android/ion/ion.c
index 57e0d80..75830e3 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -27,6 +27,7 @@
  #include 
  #include 
  #include 
+#include 
  
  #include "ion.h"
  
@@ -119,8 +120,6 @@ static struct ion_buffer *ion_buffer_create(struct ion_heap *heap,
  
  void ion_buffer_destroy(struct ion_buffer *buffer)

  {
-   if (WARN_ON(buffer->kmap_cnt > 0))
-   buffer->heap->ops->unmap_kernel(buffer->heap, buffer);
buffer->heap->ops->free(buffer);
kfree(buffer);
  }
@@ -140,34 +139,6 @@ static void _ion_buffer_destroy(struct ion_buffer *buffer)
ion_buffer_destroy(buffer);
  }
  
-static void *ion_buffer_kmap_get(struct ion_buffer *buffer)

-{
-   void *vaddr;
-
-   if (buffer->kmap_cnt) {
-   buffer->kmap_cnt++;
-   return buffer->vaddr;
-   }
-   vaddr = buffer->heap->ops->map_kernel(buffer->heap, buffer);
-   if (WARN_ONCE(!vaddr,
- "heap->ops->map_kernel should return ERR_PTR on error"))
-   return ERR_PTR(-EINVAL);
-   if (IS_ERR(vaddr))
-   return vaddr;
-   buffer->vaddr = vaddr;
-   buffer->kmap_cnt++;
-   return vaddr;
-}
-
-static void ion_buffer_kmap_put(struct ion_buffer *buffer)
-{
-   buffer->kmap_cnt--;
-   if (!buffer->kmap_cnt) {
-   buffer->heap->ops->unmap_kernel(buffer->heap, buffer);
-   buffer->vaddr = NULL;
-   }
-}
-
  static struct sg_table *dup_sg_table(struct sg_table *table)
  {
struct sg_table *new_table;
@@ -305,34 +276,68 @@ static void ion_dma_buf_release(struct dma_buf *dmabuf)
_ion_buffer_destroy(buffer);
  }
  
+static inline int sg_page_count(struct scatterlist *sg)

+{
+   return PAGE_ALIGN(sg->offset + sg->length) >> PAGE_SHIFT;
+}
+
+static struct page *ion_dma_buf_get_page(struct sg_table *sg_table,
+unsigned long offset)
+{
+   struct page *page;
+   struct scatterlist *sg;
+   int i;
+   unsigned int nr_pages;
+
+   nr_pages = 0;
+   page = NULL;
+   /* Find the page with specified offset */
+   for_each_sg(sg_table->sgl, sg, sg_table->nents, i) {
+   if (nr_pages + sg_page_count(sg) > offset) {
+   page = nth_page(sg_page(sg), offset - nr_pages);
+   break;
+   }
+
+   nr_pages += sg_page_count(sg);
+   }
+
+   return page;
+}
  static void *ion_dma_buf_kmap(struct dma_buf *dmabuf, unsigned long offset)
  {
struct ion_buffer *buffer = dmabuf->priv;
  
-	return buffer->vaddr + offset * PAGE_SIZE;

+   return kmap(ion_dma_buf_get_page(buffer->sg_table, offset));
  }


This unfortunately doesn't work for uncached buffers. We need to create
an uncached alias otherwise we break what little coherency that guarantees.
I'm still convinced that we can't actually do the uncached buffers
correctly and they should be removed...

Thanks,
Laura

  
  static void ion_dma_buf_kunmap(struct dma_buf *dmabuf, unsigned long offset,

   void *ptr)
  {
+   kunmap(ptr);
+}
+
+static void *ion_dma_buf_kmap_atomic(struct dma_buf *dmabuf,
+unsigned long offset)
+{
+   struct ion_buffer *buffer = dmabuf->priv;
+
+   return kmap_atomic(ion_dma_buf_get_page(buffer->sg_table,
+   offset));
+}
+
+static void ion_dma_buf_kunmap_atomic(struct dma_buf *dmabuf,
+ unsigned long offset,
+ void *ptr)
+{
+   kunmap_atomic(ptr);
  }
  
  static int ion_dma_buf_begin_cpu_access(struct dma_buf *dmabuf,

enum dma_data_direction direction)
  {
struct ion_buffer *buffer = dmabuf->priv;
-   void *vaddr;
struct ion_dma_buf_attachment *a;
  
-	/*

-* TODO: Move this elsewhere because we don't always need a vaddr
-*/
-   if (buffer->heap->ops->map_kernel) {
-   mutex_lock(>lock);
-   vaddr = ion_buffer_kmap_get(buffer);
-   mutex_unlock(>lock);
-   }
-
  

Re: [PATCH] staging: android: ion: Initialize dma_address of new sg list

[Laura Abbott]

On 02/12/2018 01:25 PM, Liam Mark wrote:


On Mon, 12 Feb 2018, Dan Carpenter wrote:


On Fri, Feb 09, 2018 at 10:16:56PM -0800, Liam Mark wrote:

Fix the dup_sg_table function to initialize the dma_address of the new
sg list entries instead of the source dma_address entries.

Fixes: 17fd283f3870 ("staging: android: ion: Duplicate sg_table")
Signed-off-by: Liam Mark 


How did you find this bug?  What are the user visible effects of this
bug?  I'm probably going to ask you to send a v2 patch with a new
changelog depending on the answers to these questions.


I noticed the bug when reading through the code, I haven’t seen any
visible effects of this bug.

 From looking at the code I don’t see any obvious ways that this bug would
introduce any issues with the current code base.

Liam



Given the way we duplicate the sg_list this should be harmless but you
are right it's cleaner if we initialize the new list. You can add my
Ack when you update with a new change log clarifying this.

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

Re: [PATCH] staging: android: ion: Add requested allocation alignment

[Laura Abbott]

On 02/12/2018 12:22 PM, Alexey Skidanov wrote:



On 02/12/2018 09:52 PM, Laura Abbott wrote:

On 02/12/2018 11:11 AM, Alexey Skidanov wrote:


On 02/12/2018 08:42 PM, Laura Abbott wrote:

On 02/10/2018 02:17 AM, Alexey Skidanov wrote:

Current ion defined allocation ioctl doesn't allow to specify the
requested
allocation alignment. CMA heap allocates buffers aligned on buffer size
page order.

Sometimes, the alignment requirement is less restrictive. In such
cases,
providing specific alignment may reduce the external memory
fragmentation
and in some cases it may avoid the allocation request failure.


I really do not want to bring this back as part of the regular
ABI.

Yes, I know it was removed in 4.12.
Having an alignment parameter that gets used for exactly

one heap only leads to confusion (which is why it was removed
from the ABI in the first place).

You are correct regarding the CMA heap. But, probably it may be used by
custom heap as well.


I can think of a lot of instances where it could be used but
ultimately there needs to be an actual in kernel user who wants
it.


The alignment came from the behavior of the DMA APIs. Do you
actually need to specify any alignment from userspace or do
you only need page size?

Yes. If CMA gives it for free, I would suggest to let the ion user to
decide


I'm really not convinced changing the ABI yet again just to let
the user decide is actually worth it. If we can manage it, I'd
much rather see a proposal that doesn't change the ABI.

I didn't actually change the ABI - I just use the "unused" member:
struct ion_allocation_data {
@@ -80,7 +79,7 @@ struct ion_allocation_data {
 __u32 heap_id_mask;
 __u32 flags;
 __u32 fd;
-   __u32 unused;
+   __u32 align;
  };



Something that was previously unused is now being used. That's a change
in how the structure is interpreted which is an ABI change, especially
since we haven't been checking that the __unused field is set
to 0.


As an alternative, I may add __u64 heap_specific_param - but this will
change the ABI. But, probably it makes the ABI more generic?


Why does the ABI need to be more generic? If we change the ABI
we're stuck with it and I'd like to approach it as the very last
resort.

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

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

[Laura Abbott]

On 02/09/2018 10:21 PM, Liam Mark wrote:

The ION begin_cpu_access and end_cpu_access functions use the
dma_sync_sg_for_cpu and dma_sync_sg_for_device APIs to perform cache
maintenance.

Currently it is possible to apply cache maintenance, via the
begin_cpu_access and end_cpu_access APIs, to ION buffers which are not
dma mapped.

The dma sync sg APIs should not be called on sg lists which have not been
dma mapped as this can result in cache maintenance being applied to the
wrong address. If an sg list has not been dma mapped then its dma_address
field has not been populated, some dma ops such as the swiotlb_dma_ops ops
use the dma_address field to calculate the address onto which to apply
cache maintenance.

Fix the ION begin_cpu_access and end_cpu_access functions to only apply
cache maintenance to buffers which have been dma mapped.


I think this looks okay. I was initially concerned about concurrency and
setting the dma_mapped flag but I think that should be handled by the
caller synchronizing map/unmap/cpu_access calls (we might need to re-evaluate
in the future)

I would like to hold on queuing this for just a little bit until I
finish working on the Ion unit test (doing this in the complete opposite
order of course). I'm assuming this passed your internal tests Liam?

Thanks,
Laura


Fixes: 2a55e7b5e544 ("staging: android: ion: Call dma_map_sg for syncing and 
mapping")
Signed-off-by: Liam Mark 
---
  drivers/staging/android/ion/ion.c | 16 
  1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/android/ion/ion.c 
b/drivers/staging/android/ion/ion.c
index f480885e346b..e5df5272823d 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -214,6 +214,7 @@ struct ion_dma_buf_attachment {
  	struct device *dev;caller 
  	struct sg_table *table;

struct list_head list;
+   bool dma_mapped;
  };
  
  static int ion_dma_buf_attach(struct dma_buf *dmabuf, struct device *dev,

@@ -235,6 +236,7 @@ static int ion_dma_buf_attach(struct dma_buf *dmabuf, 
struct device *dev,
  
  	a->table = table;

a->dev = dev;
+   a->dma_mapped = false;
INIT_LIST_HEAD(>list);
  
  	attachment->priv = a;

@@ -272,6 +274,7 @@ static struct sg_table *ion_map_dma_buf(struct 
dma_buf_attachment *attachment,
direction))
return ERR_PTR(-ENOMEM);
  
+	a->dma_mapped = true;

return table;
  }
  
@@ -279,7 +282,10 @@ static void ion_unmap_dma_buf(struct dma_buf_attachment *attachment,

  struct sg_table *table,
  enum dma_data_direction direction)
  {
+   struct ion_dma_buf_attachment *a = attachment->priv;
+
dma_unmap_sg(attachment->dev, table->sgl, table->nents, direction);
+   a->dma_mapped = false;
  }
  
  static int ion_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma)

@@ -345,8 +351,9 @@ static int ion_dma_buf_begin_cpu_access(struct dma_buf 
*dmabuf,
  
  	mutex_lock(>lock);

list_for_each_entry(a, >attachments, list) {
-   dma_sync_sg_for_cpu(a->dev, a->table->sgl, a->table->nents,
-   direction);
+   if (a->dma_mapped)
+   dma_sync_sg_for_cpu(a->dev, a->table->sgl,
+   a->table->nents, direction);
}
mutex_unlock(>lock);
  
@@ -367,8 +374,9 @@ static int ion_dma_buf_end_cpu_access(struct dma_buf *dmabuf,
  
  	mutex_lock(>lock);

list_for_each_entry(a, >attachments, list) {
-   dma_sync_sg_for_device(a->dev, a->table->sgl, a->table->nents,
-  direction);
+   if (a->dma_mapped)
+   dma_sync_sg_for_device(a->dev, a->table->sgl,
+  a->table->nents, direction);
}
mutex_unlock(>lock);
  



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

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

[Laura Abbott]

On 02/09/2018 10:21 PM, Liam Mark wrote:

The ION begin_cpu_access and end_cpu_access functions use the
dma_sync_sg_for_cpu and dma_sync_sg_for_device APIs to perform cache
maintenance.

Currently it is possible to apply cache maintenance, via the
begin_cpu_access and end_cpu_access APIs, to ION buffers which are not
dma mapped.

The dma sync sg APIs should not be called on sg lists which have not been
dma mapped as this can result in cache maintenance being applied to the
wrong address. If an sg list has not been dma mapped then its dma_address
field has not been populated, some dma ops such as the swiotlb_dma_ops ops
use the dma_address field to calculate the address onto which to apply
cache maintenance.

Fix the ION begin_cpu_access and end_cpu_access functions to only apply
cache maintenance to buffers which have been dma mapped.


I think this looks okay. I was initially concerned about concurrency and
setting the dma_mapped flag but I think that should be handled by the
caller synchronizing map/unmap/cpu_access calls (we might need to re-evaluate
in the future)

I would like to hold on queuing this for just a little bit until I
finish working on the Ion unit test (doing this in the complete opposite
order of course). I'm assuming this passed your internal tests Liam?

Thanks,
Laura


Fixes: 2a55e7b5e544 ("staging: android: ion: Call dma_map_sg for syncing and 
mapping")
Signed-off-by: Liam Mark 
---
  drivers/staging/android/ion/ion.c | 16 
  1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/android/ion/ion.c 
b/drivers/staging/android/ion/ion.c
index f480885e346b..e5df5272823d 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -214,6 +214,7 @@ struct ion_dma_buf_attachment {
  	struct device *dev;caller 
  	struct sg_table *table;

struct list_head list;
+   bool dma_mapped;
  };
  
  static int ion_dma_buf_attach(struct dma_buf *dmabuf, struct device *dev,

@@ -235,6 +236,7 @@ static int ion_dma_buf_attach(struct dma_buf *dmabuf, 
struct device *dev,
  
  	a->table = table;

a->dev = dev;
+   a->dma_mapped = false;
INIT_LIST_HEAD(>list);
  
  	attachment->priv = a;

@@ -272,6 +274,7 @@ static struct sg_table *ion_map_dma_buf(struct 
dma_buf_attachment *attachment,
direction))
return ERR_PTR(-ENOMEM);
  
+	a->dma_mapped = true;

return table;
  }
  
@@ -279,7 +282,10 @@ static void ion_unmap_dma_buf(struct dma_buf_attachment *attachment,

  struct sg_table *table,
  enum dma_data_direction direction)
  {
+   struct ion_dma_buf_attachment *a = attachment->priv;
+
dma_unmap_sg(attachment->dev, table->sgl, table->nents, direction);
+   a->dma_mapped = false;
  }
  
  static int ion_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma)

@@ -345,8 +351,9 @@ static int ion_dma_buf_begin_cpu_access(struct dma_buf 
*dmabuf,
  
  	mutex_lock(>lock);

list_for_each_entry(a, >attachments, list) {
-   dma_sync_sg_for_cpu(a->dev, a->table->sgl, a->table->nents,
-   direction);
+   if (a->dma_mapped)
+   dma_sync_sg_for_cpu(a->dev, a->table->sgl,
+   a->table->nents, direction);
}
mutex_unlock(>lock);
  
@@ -367,8 +374,9 @@ static int ion_dma_buf_end_cpu_access(struct dma_buf *dmabuf,
  
  	mutex_lock(>lock);

list_for_each_entry(a, >attachments, list) {
-   dma_sync_sg_for_device(a->dev, a->table->sgl, a->table->nents,
-  direction);
+   if (a->dma_mapped)
+   dma_sync_sg_for_device(a->dev, a->table->sgl,
+  a->table->nents, direction);
}
mutex_unlock(>lock);
  



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

Re: ion kernel mapping implementation

[Laura Abbott]

On 02/12/2018 11:21 AM, Alexey Skidanov wrote:



On 02/12/2018 08:30 PM, Laura Abbott wrote:

On 02/10/2018 01:43 AM, Alexey Skidanov wrote:

Hi,

Current ion kernel mapping implementation uses vmap() to map previously
allocated buffers into kernel virtual address space. On 32 bit
platforms, vmap() might fail on large enough buffers due to the limited
available vmalloc space.

dma_buf_kmap() should guarantee that only one page is mapped at a time.
So, probably it's better to implement dma_buf_kmap() by kmap() and not
by vmap()?

Thanks,
Alexey



The short answer is yes.

The long answer is that the conversion to the dma_buf APIs kept the
existing Ion behavior which mapped the entire buffer. We got away
with this because the in kernel mapping APIs are used very infrequently
and with buffers that never triggered an exhaustion of vmalloc
space.

If we actually start seeing problems with this we can fix it up
but I don't consider this a high priority item.

I have the patch fixing this potential bug. I would like to submit it
for review, if you are ok with it. Please, just let me know.


Yes, please submit from review and we can go from there.



Thanks,
Laura


Thanks,
Alexey



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

Re: [PATCH] staging: android: ion: Add requested allocation alignment

[Laura Abbott]

On 02/12/2018 11:11 AM, Alexey Skidanov wrote:


On 02/12/2018 08:42 PM, Laura Abbott wrote:

On 02/10/2018 02:17 AM, Alexey Skidanov wrote:

Current ion defined allocation ioctl doesn't allow to specify the
requested
allocation alignment. CMA heap allocates buffers aligned on buffer size
page order.

Sometimes, the alignment requirement is less restrictive. In such cases,
providing specific alignment may reduce the external memory fragmentation
and in some cases it may avoid the allocation request failure.


I really do not want to bring this back as part of the regular
ABI.

Yes, I know it was removed in 4.12.
Having an alignment parameter that gets used for exactly

one heap only leads to confusion (which is why it was removed
from the ABI in the first place).

You are correct regarding the CMA heap. But, probably it may be used by
custom heap as well.


I can think of a lot of instances where it could be used but
ultimately there needs to be an actual in kernel user who wants
it.


The alignment came from the behavior of the DMA APIs. Do you
actually need to specify any alignment from userspace or do
you only need page size?

Yes. If CMA gives it for free, I would suggest to let the ion user to decide


I'm really not convinced changing the ABI yet again just to let
the user decide is actually worth it. If we can manage it, I'd
much rather see a proposal that doesn't change the ABI.


Thanks,
Laura


Thanks,
Alexey



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

Re: [PATCH] staging: android: ion: Add requested allocation alignment

[Laura Abbott]

On 02/10/2018 02:17 AM, Alexey Skidanov wrote:

Current ion defined allocation ioctl doesn't allow to specify the requested
allocation alignment. CMA heap allocates buffers aligned on buffer size
page order.

Sometimes, the alignment requirement is less restrictive. In such cases,
providing specific alignment may reduce the external memory fragmentation
and in some cases it may avoid the allocation request failure.



I really do not want to bring this back as part of the regular
ABI. Having an alignment parameter that gets used for exactly
one heap only leads to confusion (which is why it was removed
from the ABI in the first place).

The alignment came from the behavior of the DMA APIs. Do you
actually need to specify any alignment from userspace or do
you only need page size?

Thanks,
Laura


To fix this, we add an alignment parameter to the allocation ioctl.

Signed-off-by: Alexey Skidanov 
---
  drivers/staging/android/ion/ion-ioctl.c |  3 ++-
  drivers/staging/android/ion/ion.c   | 14 +-
  drivers/staging/android/ion/ion.h   |  9 ++---
  drivers/staging/android/ion/ion_carveout_heap.c |  3 ++-
  drivers/staging/android/ion/ion_chunk_heap.c|  3 ++-
  drivers/staging/android/ion/ion_cma_heap.c  | 18 --
  drivers/staging/android/ion/ion_system_heap.c   |  6 --
  drivers/staging/android/uapi/ion.h  |  7 +++
  8 files changed, 40 insertions(+), 23 deletions(-)

diff --git a/drivers/staging/android/ion/ion-ioctl.c 
b/drivers/staging/android/ion/ion-ioctl.c
index c789893..9fe022b 100644
--- a/drivers/staging/android/ion/ion-ioctl.c
+++ b/drivers/staging/android/ion/ion-ioctl.c
@@ -83,7 +83,8 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned 
long arg)
  
  		fd = ion_alloc(data.allocation.len,

   data.allocation.heap_id_mask,
-  data.allocation.flags);
+  data.allocation.flags,
+  data.allocation.align);
if (fd < 0)
return fd;
  
diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c

index f480885..35ddc7a 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -78,7 +78,8 @@ static void ion_buffer_add(struct ion_device *dev,
  static struct ion_buffer *ion_buffer_create(struct ion_heap *heap,
struct ion_device *dev,
unsigned long len,
-   unsigned long flags)
+   unsigned long flags,
+   unsigned int align)
  {
struct ion_buffer *buffer;
int ret;
@@ -90,14 +91,14 @@ static struct ion_buffer *ion_buffer_create(struct ion_heap 
*heap,
buffer->heap = heap;
buffer->flags = flags;
  
-	ret = heap->ops->allocate(heap, buffer, len, flags);

+   ret = heap->ops->allocate(heap, buffer, len, flags, align);
  
  	if (ret) {

if (!(heap->flags & ION_HEAP_FLAG_DEFER_FREE))
goto err2;
  
  		ion_heap_freelist_drain(heap, 0);

-   ret = heap->ops->allocate(heap, buffer, len, flags);
+   ret = heap->ops->allocate(heap, buffer, len, flags, align);
if (ret)
goto err2;
}
@@ -390,7 +391,10 @@ static const struct dma_buf_ops dma_buf_ops = {
.unmap = ion_dma_buf_kunmap,
  };
  
-int ion_alloc(size_t len, unsigned int heap_id_mask, unsigned int flags)

+int ion_alloc(size_t len,
+ unsigned int heap_id_mask,
+ unsigned int flags,
+ unsigned int align)
  {
struct ion_device *dev = internal_dev;
struct ion_buffer *buffer = NULL;
@@ -417,7 +421,7 @@ int ion_alloc(size_t len, unsigned int heap_id_mask, 
unsigned int flags)
/* 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);
+   buffer = ion_buffer_create(heap, dev, len, flags, align);
if (!IS_ERR(buffer))
break;
}
diff --git a/drivers/staging/android/ion/ion.h 
b/drivers/staging/android/ion/ion.h
index f5f9cd6..0c161d2 100644
--- a/drivers/staging/android/ion/ion.h
+++ b/drivers/staging/android/ion/ion.h
@@ -123,8 +123,10 @@ struct ion_device {
   */
  struct ion_heap_ops {
int (*allocate)(struct ion_heap *heap,
-   struct ion_buffer *buffer, unsigned long len,
-   unsigned long flags);
+   struct ion_buffer *buffer,
+   unsigned long len,
+   unsigned long flags,
+   unsigned int 

Re: ion kernel mapping implementation

[Laura Abbott]

On 02/10/2018 01:43 AM, Alexey Skidanov wrote:

Hi,

Current ion kernel mapping implementation uses vmap() to map previously
allocated buffers into kernel virtual address space. On 32 bit
platforms, vmap() might fail on large enough buffers due to the limited
available vmalloc space.

dma_buf_kmap() should guarantee that only one page is mapped at a time.
So, probably it's better to implement dma_buf_kmap() by kmap() and not
by vmap()?

Thanks,
Alexey



The short answer is yes.

The long answer is that the conversion to the dma_buf APIs kept the
existing Ion behavior which mapped the entire buffer. We got away
with this because the in kernel mapping APIs are used very infrequently
and with buffers that never triggered an exhaustion of vmalloc
space.

If we actually start seeing problems with this we can fix it up
but I don't consider this a high priority item.

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

Re: staging: ion: ION allocation fall back order depends on heap linkage order

[Laura Abbott]

On 02/07/2018 07:10 AM, Alexey Skidanov wrote:



On 02/07/2018 04:58 PM, Laura Abbott wrote:

On 02/06/2018 11:05 PM, Alexey Skidanov wrote:




Yup, you've hit upon a key problem. Having fallbacks be stable
was always a problem and the recommendation these days is to
not rely on them. You can specify a heap at a time and fallback
manually if you want that behavior.

If you have a proposal to make fallbacks work reliably without
overly complicating the ABI I'm happy to review it.

Thanks,
Laura


I think it's possible to "automate" the "manual fallback" behavior. But
the real issues is using heap id to specify the particular heap object.

Current API (allocation IOCTL) requires to specify the particular heap
object by using heap id. From the other hand, the user space doesn't
control the heaps creation order and heap id assignment. So it may be
tricky, especially when more than one object of the same heap type is
created automatically.

Thanks,
Alexey




The query ioctl is designed to get the heap ID information without
needing to rely on the linking order or anything else defined in
the kernel.

Thanks,
Laura


That is true. But if we have 2 *automatically created* heaps of the same
type, how userspace can distinguish between them?

Thanks,
Alexey



The query ioctl also gives the name which should be different
for each heap. It's not ideal but the name/heap type are the best
way to differentiate between heaps without resorting to hard
coding.

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

Re: staging: ion: ION allocation fall back order depends on heap linkage order

[Laura Abbott]

On 02/06/2018 11:05 PM, Alexey Skidanov wrote:




Yup, you've hit upon a key problem. Having fallbacks be stable
was always a problem and the recommendation these days is to
not rely on them. You can specify a heap at a time and fallback
manually if you want that behavior.

If you have a proposal to make fallbacks work reliably without
overly complicating the ABI I'm happy to review it.

Thanks,
Laura


I think it's possible to "automate" the "manual fallback" behavior. But
the real issues is using heap id to specify the particular heap object.

Current API (allocation IOCTL) requires to specify the particular heap
object by using heap id. From the other hand, the user space doesn't
control the heaps creation order and heap id assignment. So it may be
tricky, especially when more than one object of the same heap type is
created automatically.

Thanks,
Alexey




The query ioctl is designed to get the heap ID information without
needing to rely on the linking order or anything else defined in
the kernel.

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

Re: [PATCH v3] staging: android: ion: Add implementation of dma_buf_vmap and dma_buf_vunmap

[Laura Abbott]

On 01/31/2018 10:10 PM, Alexey Skidanov wrote:


On 01/31/2018 03:00 PM, Greg KH wrote:

On Wed, Jan 31, 2018 at 02:03:42PM +0200, Alexey Skidanov wrote:

Any driver may access shared buffers, created by ion, using dma_buf_vmap and
dma_buf_vunmap dma-buf API that maps/unmaps previosuly allocated buffers into
the kernel virtual address space. The implementation of these API is missing in
the current ion implementation.

Signed-off-by: Alexey Skidanov 
---


No review from any other Intel developers? :(

Will add.


Anyway, what in-tree driver needs access to these functions?

I'm not sure that there are the in-tree drivers using these functions and ion as> 
buffer exporter because they are not implemented in ion :) But there are some 
in-tre> drivers using these APIs (gpu drivers) with other buffer exporters.


It's still not clear why you need to implement these APIs.
Are you planning to use Ion with GPU drivers? I'm especially
interested in this if you have a non-Android use case.

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

Re: staging: ion: ION allocation fall back order depends on heap linkage order

[Laura Abbott]

On 01/28/2018 08:24 AM, Alexey Skidanov wrote:

Hi,

According to my understanding, the allocation fall back order
completely depends on heap->id that is assigned during the heap
creation:
plist_for_each_entry(heap, >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))
 break;
 }

On creation, each heap is added to the priority list according to the
priority assigned:

...
static int heap_id;
...
void ion_device_add_heap(struct ion_heap *heap)
{
 ...
 heap->id = heap_id++;
 ...
}


The order of creation is the order of linkage defined in the Makefile.
Thus, by default, we have:

heap id 2, type ION_HEAP_TYPE_DMA
heap id 1, type ION_HEAP_TYPE_SYSTEM
heap id 0, type ION_HEAP_TYPE_SYSTEM_CONTIG

Changing the linkage order:
diff --git a/drivers/staging/android/ion/Makefile
b/drivers/staging/android/ion/Makefile
index bb30bf8..e05052c 100644
--- a/drivers/staging/android/ion/Makefile
+++ b/drivers/staging/android/ion/Makefile
@@ -1,6 +1,6 @@
  # SPDX-License-Identifier: GPL-2.0
  obj-$(CONFIG_ION) +=   ion.o ion-ioctl.o ion_heap.o
-obj-$(CONFIG_ION_SYSTEM_HEAP) += ion_system_heap.o ion_page_pool.o
  obj-$(CONFIG_ION_CARVEOUT_HEAP) += ion_carveout_heap.o
  obj-$(CONFIG_ION_CHUNK_HEAP) += ion_chunk_heap.o
  obj-$(CONFIG_ION_CMA_HEAP) += ion_cma_heap.o
+obj-$(CONFIG_ION_SYSTEM_HEAP) += ion_system_heap.o ion_page_pool.o

I get the following order:

heap id 2, type ION_HEAP_TYPE_SYSTEM
heap id 1, type ION_HEAP_TYPE_SYSTEM_CONTIG
heap id 0, type ION_HEAP_TYPE_DMA

So, if the user specifies more than 1 heap in the heap_id_mask during
allocation, the allocation fall back order completely depends on the
order of linkage. Probably, it's better to let the user to define the
fall back order (and NOT to be dependent on the linkage order at all)
?



Yup, you've hit upon a key problem. Having fallbacks be stable
was always a problem and the recommendation these days is to
not rely on them. You can specify a heap at a time and fallback
manually if you want that behavior.

If you have a proposal to make fallbacks work reliably without
overly complicating the ABI I'm happy to review it.

Thanks,
Laura

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

Re: [PATCH 1/2] staging: android: ion: Remove dead code in ion_page_pool_free

[Laura Abbott]

On 02/06/2018 03:10 PM, Laura Abbott wrote:

On 02/04/2018 07:26 PM, Yisheng Xie wrote:

ion_page_pool_add will always return 0, however ion_page_pool_free will
call ion_page_pool_free_pages when ion_page_pool_add's return value is
not 0, so it is a dead code which can be removed.



Can you clean up ion_page_pool_add to be a void return as well?
No sense in having it just always return 0.



Nevermind, just saw the follow up patch. Both of them:

Acked-by: Laura Abbott <labb...@redhat.com>


Signed-off-by: Yisheng Xie <xieyishe...@huawei.com>
---
  drivers/staging/android/ion/ion_page_pool.c | 6 +-
  1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/staging/android/ion/ion_page_pool.c 
b/drivers/staging/android/ion/ion_page_pool.c
index 4452e28..150626f 100644
--- a/drivers/staging/android/ion/ion_page_pool.c
+++ b/drivers/staging/android/ion/ion_page_pool.c
@@ -79,13 +79,9 @@ struct page *ion_page_pool_alloc(struct ion_page_pool *pool)
  void ion_page_pool_free(struct ion_page_pool *pool, struct page *page)
  {
-    int ret;
-
  BUG_ON(pool->order != compound_order(page));
-    ret = ion_page_pool_add(pool, page);
-    if (ret)
-    ion_page_pool_free_pages(pool, page);
+    ion_page_pool_add(pool, page);
  }
  static int ion_page_pool_total(struct ion_page_pool *pool, bool high)





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

Re: [PATCH 1/2] staging: android: ion: Remove dead code in ion_page_pool_free

[Laura Abbott]

On 02/04/2018 07:26 PM, Yisheng Xie wrote:

ion_page_pool_add will always return 0, however ion_page_pool_free will
call ion_page_pool_free_pages when ion_page_pool_add's return value is
not 0, so it is a dead code which can be removed.



Can you clean up ion_page_pool_add to be a void return as well?
No sense in having it just always return 0.


Signed-off-by: Yisheng Xie 
---
  drivers/staging/android/ion/ion_page_pool.c | 6 +-
  1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/staging/android/ion/ion_page_pool.c 
b/drivers/staging/android/ion/ion_page_pool.c
index 4452e28..150626f 100644
--- a/drivers/staging/android/ion/ion_page_pool.c
+++ b/drivers/staging/android/ion/ion_page_pool.c
@@ -79,13 +79,9 @@ struct page *ion_page_pool_alloc(struct ion_page_pool *pool)
  
  void ion_page_pool_free(struct ion_page_pool *pool, struct page *page)

  {
-   int ret;
-
BUG_ON(pool->order != compound_order(page));
  
-	ret = ion_page_pool_add(pool, page);

-   if (ret)
-   ion_page_pool_free_pages(pool, page);
+   ion_page_pool_add(pool, page);
  }
  
  static int ion_page_pool_total(struct ion_page_pool *pool, bool high)




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

Re: [PATCH 2/2] staging: android: ion: Remove lable debugfs_done

[Laura Abbott]

On 02/01/2018 04:34 AM, Yisheng Xie wrote:

When failed to create debug_root, we will go on initail other part of
ion, so we can just info this message to user and do not need a lable
to jump.



Acked-by: Laura Abbott <labb...@redhat.com>


Signed-off-by: Yisheng Xie <xieyishe...@huawei.com>
---
  drivers/staging/android/ion/ion.c | 5 +
  1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/staging/android/ion/ion.c 
b/drivers/staging/android/ion/ion.c
index 3e41644..70a70f5 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -601,12 +601,9 @@ static int ion_device_create(void)
}
  
  	idev->debug_root = debugfs_create_dir("ion", NULL);

-   if (!idev->debug_root) {
+   if (!idev->debug_root)
pr_err("ion: failed to create debugfs root directory.\n");
-   goto debugfs_done;
-   }
  
-debugfs_done:

idev->buffers = RB_ROOT;
mutex_init(>buffer_lock);
init_rwsem(>lock);



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

Re: [PATCH 1/2] staging: android: ion: Avoid NULL point in error path

[Laura Abbott]

On 02/01/2018 04:34 AM, Yisheng Xie wrote:

If we failed to create debugfs for ion at ion_device_create, the
debug_root of ion_device will be NULL, and then when try to create debug
file for shrinker of heap it will be create on the top of debugfs. If we
also failed to create this the debug file, it call dentry_path to found
the path of debug_root, then a NULL point will occur.

Fix this by avoiding call dentry_path, but show the debug name only when
failed to create debug file for shrinker.



Acked-by: Laura Abbott <labb...@redhat.com>


Signed-off-by: Yisheng Xie <xieyishe...@huawei.com>
---
  drivers/staging/android/ion/ion.c | 10 +++---
  1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/android/ion/ion.c 
b/drivers/staging/android/ion/ion.c
index f480885..3e41644 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -570,13 +570,9 @@ void ion_device_add_heap(struct ion_heap *heap)
debug_file = debugfs_create_file(
debug_name, 0644, dev->debug_root, heap,
_shrink_fops);
-   if (!debug_file) {
-   char buf[256], *path;
-
-   path = dentry_path(dev->debug_root, buf, 256);
-   pr_err("Failed to create heap shrinker debugfs at 
%s/%s\n",
-  path, debug_name);
-   }
+   if (!debug_file)
+   pr_err("Failed to create ion heap shrinker debugfs at 
%s\n",
+  debug_name);
}
  
  	dev->heap_cnt++;




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