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-devel@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.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-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 

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RESEND][PATCH v8 0/5] DMA-BUF Heaps (destaging ION)

[Laura Abbott]

On 9/6/19 2:47 PM, John Stultz wrote:

Here is yet another pass at the dma-buf heaps patchset Andrew
and I have been working on which tries to destage a fair chunk
of ION functionality.

The patchset implements per-heap devices which can be opened
directly and then an ioctl is used to allocate a dmabuf from the
heap.

The interface is similar, but much simpler then IONs, only
providing an ALLOC ioctl.

Also, I've provided relatively simple system and cma heaps.

I've booted and tested these patches with AOSP on the HiKey960
using the kernel tree here:
   
https://git.linaro.org/people/john.stultz/android-dev.git/log/?h=dev/dma-buf-heap

And the userspace changes here:
   https://android-review.googlesource.com/c/device/linaro/hikey/+/909436

Compared to ION, this patchset is missing the system-contig,
carveout and chunk heaps, as I don't have a device that uses
those, so I'm unable to do much useful validation there.
Additionally we have no upstream users of chunk or carveout,
and the system-contig has been deprecated in the common/andoid-*
kernels, so this should be ok.

I've also removed the stats accounting, since any such accounting
should be implemented by dma-buf core or the heaps themselves.

Most of the changes in this revision are adddressing the more
concrete feedback from Christoph (many thanks!). Though I'm not
sure if some of the less specific feedback was completely resolved
in discussion last time around. Please let me know!

New in v8:
* Make struct dma_heap_ops consts (Suggested by Christoph)
* Add flush_kernel_vmap_range/invalidate_kernel_vmap_range calls
   (suggested by Christoph)
* Condense dma_heap_buffer and heap_helper_buffer (suggested by
   Christoph)
* Get rid of needless struct system_heap (suggested by Christoph)
* Fix indentation by using shorter argument names (suggested by
   Christoph)
* Remove unused private_flags value
* Add forgotten include file to fix build issue on x86
* Checkpatch whitespace fixups

Thoughts and feedback would be greatly appreciated!

thanks
-john

Cc: Laura Abbott 
Cc: Benjamin Gaignard 
Cc: Sumit Semwal 
Cc: Liam Mark 
Cc: Pratik Patel 
Cc: Brian Starkey 
Cc: Vincent Donnefort 
Cc: Sudipto Paul 
Cc: Andrew F. Davis 
Cc: Christoph Hellwig 
Cc: Chenbo Feng 
Cc: Alistair Strachan 
Cc: Hridya Valsaraju 
Cc: dri-devel@lists.freedesktop.org


Andrew F. Davis (1):
   dma-buf: Add dma-buf heaps framework

John Stultz (4):
   dma-buf: heaps: Add heap helpers
   dma-buf: heaps: Add system heap to dmabuf heaps
   dma-buf: heaps: Add CMA heap to dmabuf heaps
   kselftests: Add dma-heap test

  MAINTAINERS   |  18 ++
  drivers/dma-buf/Kconfig   |  11 +
  drivers/dma-buf/Makefile  |   2 +
  drivers/dma-buf/dma-heap.c| 250 
  drivers/dma-buf/heaps/Kconfig |  14 +
  drivers/dma-buf/heaps/Makefile|   4 +
  drivers/dma-buf/heaps/cma_heap.c  | 164 +++
  drivers/dma-buf/heaps/heap-helpers.c  | 269 ++
  drivers/dma-buf/heaps/heap-helpers.h  |  55 
  drivers/dma-buf/heaps/system_heap.c   | 122 
  include/linux/dma-heap.h  |  59 
  include/uapi/linux/dma-heap.h |  55 
  tools/testing/selftests/dmabuf-heaps/Makefile |   9 +
  .../selftests/dmabuf-heaps/dmabuf-heap.c  | 230 +++
  14 files changed, 1262 insertions(+)
  create mode 100644 drivers/dma-buf/dma-heap.c
  create mode 100644 drivers/dma-buf/heaps/Kconfig
  create mode 100644 drivers/dma-buf/heaps/Makefile
  create mode 100644 drivers/dma-buf/heaps/cma_heap.c
  create mode 100644 drivers/dma-buf/heaps/heap-helpers.c
  create mode 100644 drivers/dma-buf/heaps/heap-helpers.h
  create mode 100644 drivers/dma-buf/heaps/system_heap.c
  create mode 100644 include/linux/dma-heap.h
  create mode 100644 include/uapi/linux/dma-heap.h
  create mode 100644 tools/testing/selftests/dmabuf-heaps/Makefile
  create mode 100644 tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c



I've seen a couple of details that need to be fixed and can be
fixed fairly easily but as far as the overall design goes it looks
good. Once those are fixed up, you can add

Acked-by: Laura Abbott 

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);



___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-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



___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v6 4/5] dma-buf: heaps: Add CMA heap to dmabuf heaps

[Laura Abbott]

On 7/24/19 2:59 AM, Christoph Hellwig wrote:

On Mon, Jul 22, 2019 at 10:04:06PM -0700, John Stultz wrote:

Apologies, I'm not sure I'm understanding your suggestion here.
dma_alloc_contiguous() does have some interesting optimizations
(avoiding allocating single page from cma), though its focus on
default area vs specific device area doesn't quite match up the usage
model for dma heaps.  Instead of allocating memory for a single
device, we want to be able to allow userland, for a given usage mode,
to be able to allocate a dmabuf from a specific heap of memory which
will satisfy the usage mode for that buffer pipeline (across multiple
devices).

Now, indeed, the system and cma heaps in this patchset are a bit
simple/trivial (though useful with my devices that require contiguous
buffers for the display driver), but more complex ION heaps have
traditionally stayed out of tree in vendor code, in many cases making
incompatible tweaks to the ION core dmabuf exporter logic.


So what would the more complicated heaps be?


That's why
dmabuf heaps is trying to destage ION in a way that allows heaps to
implement their exporter logic themselves, so we can start pulling
those more complicated heaps out of their vendor hidey-holes and get
some proper upstream review.

But I suspect I just am confused as to what your suggesting. Maybe
could you expand a bit? Apologies for being a bit dense.


My suggestion is to merge the system and CMA heaps.  CMA (at least
the system-wide CMA area) is really just an optimization to get
large contigous regions more easily.  We should make use of it as
transparent as possible, just like we do in the DMA code.



It's not just an optimization for Ion though. Ion was designed to
let the callers choose between system and multiple CMA heaps. On other
systems there may be multiple CMA regions dedicated to a specific
purpose or placed at a specific address. The callers need to
be able to choose exactly whether they want a particular CMA region
or discontiguous regions.

Thanks,
Laura
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-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 

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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v6 0/5] DMA-BUF Heaps (destaging ION)

[Laura Abbott]

On 6/24/19 3:49 PM, John Stultz wrote:

Here is another pass at the dma-buf heaps patchset Andrew and I
have been working on which tries to destage a fair chunk of ION
functionality.



I've gotten bogged down with both work and personal tasks
so I haven't had a chance to look too closely but, once again,
I'm happy to see this continue to move forward.


The patchset implements per-heap devices which can be opened
directly and then an ioctl is used to allocate a dmabuf from the
heap.

The interface is similar, but much simpler then IONs, only
providing an ALLOC ioctl.

Also, I've provided relatively simple system and cma heaps.

I've booted and tested these patches with AOSP on the HiKey960
using the kernel tree here:
   
https://git.linaro.org/people/john.stultz/android-dev.git/log/?h=dev/dma-buf-heap

And the userspace changes here:
   https://android-review.googlesource.com/c/device/linaro/hikey/+/909436

Compared to ION, this patchset is missing the system-contig,
carveout and chunk heaps, as I don't have a device that uses
those, so I'm unable to do much useful validation there.
Additionally we have no upstream users of chunk or carveout,
and the system-contig has been deprecated in the common/andoid-*
kernels, so this should be ok.

I've also removed the stats accounting for now, since any such
accounting should be implemented by dma-buf core or the heaps
themselves.


New in v6:
* Number of cleanups and error path fixes suggested by Brian Starkey,
   many thanks for his close review and suggestions!


Outstanding concerns:
* Need to better understand various secure heap implementations.
   Some concern that heap private flags will be needed, but its
   also possible that dma-buf heaps can't solve everyone's needs,
   in which case, a vendor's secure buffer driver can implement
   their own dma-buf exporter. So I'm not too worried here.



syzbot found a DoS with Ion which I ACKed a fix for.
https://lore.kernel.org/lkml/03763360-a7de-de87-eb90-ba7838143...@i-love.sakura.ne.jp/
This series doesn't have the page pooling so that particular bug may
not be applicable but given this is not the first time I've
seen Ion used as a DoS mechanism, it would be good to think about
putting in some basic checks.

Thanks,
Laura


Thoughts and feedback would be greatly appreciated!

thanks
-john

Cc: Laura Abbott 
Cc: Benjamin Gaignard 
Cc: Sumit Semwal 
Cc: Liam Mark 
Cc: Pratik Patel 
Cc: Brian Starkey 
Cc: Vincent Donnefort 
Cc: Sudipto Paul 
Cc: Andrew F. Davis 
Cc: Christoph Hellwig 
Cc: Chenbo Feng 
Cc: Alistair Strachan 
Cc: dri-devel@lists.freedesktop.org

Andrew F. Davis (1):
   dma-buf: Add dma-buf heaps framework

John Stultz (4):
   dma-buf: heaps: Add heap helpers
   dma-buf: heaps: Add system heap to dmabuf heaps
   dma-buf: heaps: Add CMA heap to dmabuf heaps
   kselftests: Add dma-heap test

  MAINTAINERS   |  18 ++
  drivers/dma-buf/Kconfig   |  10 +
  drivers/dma-buf/Makefile  |   2 +
  drivers/dma-buf/dma-heap.c| 249 +
  drivers/dma-buf/heaps/Kconfig |  14 +
  drivers/dma-buf/heaps/Makefile|   4 +
  drivers/dma-buf/heaps/cma_heap.c  | 169 +++
  drivers/dma-buf/heaps/heap-helpers.c  | 262 ++
  drivers/dma-buf/heaps/heap-helpers.h  |  54 
  drivers/dma-buf/heaps/system_heap.c   | 121 
  include/linux/dma-heap.h  |  59 
  include/uapi/linux/dma-heap.h |  55 
  tools/testing/selftests/dmabuf-heaps/Makefile |   9 +
  .../selftests/dmabuf-heaps/dmabuf-heap.c  | 234 
  14 files changed, 1260 insertions(+)
  create mode 100644 drivers/dma-buf/dma-heap.c
  create mode 100644 drivers/dma-buf/heaps/Kconfig
  create mode 100644 drivers/dma-buf/heaps/Makefile
  create mode 100644 drivers/dma-buf/heaps/cma_heap.c
  create mode 100644 drivers/dma-buf/heaps/heap-helpers.c
  create mode 100644 drivers/dma-buf/heaps/heap-helpers.h
  create mode 100644 drivers/dma-buf/heaps/system_heap.c
  create mode 100644 include/linux/dma-heap.h
  create mode 100644 include/uapi/linux/dma-heap.h
  create mode 100644 tools/testing/selftests/dmabuf-heaps/Makefile
  create mode 100644 tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c



___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-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-devel@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;
  



___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-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

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC][PATCH 1/5 v2] dma-buf: Add dma-buf heaps framework

[Laura Abbott]

On 3/15/19 2:29 PM, John Stultz wrote:

On Fri, Mar 15, 2019 at 1:18 PM Laura Abbott  wrote:


On 3/5/19 12:54 PM, John Stultz wrote:

+DMA-BUF HEAPS FRAMEWORK
+M:   Laura Abbott
+R:   Liam Mark
+R:   Brian Starkey
+R:   "Andrew F. Davis"
+R:   John Stultz
+S:   Maintained
+L:   linux-me...@vger.kernel.org
+L:   dri-devel@lists.freedesktop.org
+L:   linaro-mm-...@lists.linaro.org  (moderated for non-subscribers)
+F:   include/uapi/linux/dma-heap.h
+F:   include/linux/dma-heap.h
+F:   drivers/dma-buf/dma-heap.c
+F:   drivers/dma-buf/heaps/*
+T:   git git://anongit.freedesktop.org/drm/drm-misc


So I talked about this with Sumit privately but I think
it might make sense to have me step down as maintainer when
this goes out of staging. I mostly worked on Ion at my
previous position and anything I do now is mostly a side
project. I still want to see it succeed which is why I
took on the maintainer role but I don't want to become blocking
for people who have a stronger vision about where this needs
to go (see also, I'm not working with this on a daily basis).

If you just want someone to help review or take patches
to be pulled, I'm happy to do so but I'd hate to become
the bottleneck on getting things done for people who
are attempting to do real work.


I worry this will make everyone to touch the side of their nose and
yell "NOT IT!" :)

First of all, thank you so much for your efforts maintaining ION along
with your attempts to drag out requirements from interested parties
and the numerous attempts to get collaborative discussion going at
countless conferences! Your persistence and continual nudging in the
face of apathetic private users of the code probably cannot be
appreciated enough!

Your past practical experience with ION and active work with the
upstream community made you a stand out pick for this, but I
understand not wanting to be eternally stuck with a maintainership if
your not active in the area.  I'm happy to volunteer as a neutral
party, but I worry my limited experience with some of the more
complicated usage would make my opinions less informed then they
probably need to be.  Further, as a neutral party, Sumit would
probably be a better pick since he's already maintaining the dmabuf
core.



Honestly if you're doing the work to re-write everything, I
think you're more than qualified to be the maintainer. I
would also support Sumit as well.


So I'd nominate Andrew, Liam or Benjamin (or all three?) as they all
have more practical experience enabling past ION heaps on real devices
and have demonstrated active interest in working in the community.



I do think this would benefit both from multiple maintainers and
from maintainers who are actively using the framework. Like I
said, I can still be a maintainer but I think having some comaintainers
would be very helpful (and I'd support any of the names you've
suggested)


So, in other words... NOT IT! :)


I think you have to shout "Noes goes" first. :)


-john



Thanks,
Laura

P.S. For the benefit of anyone who's confused,
https://en.wikipedia.org/wiki/Nose_goes
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC][PATCH 5/5 v2] kselftests: Add dma-heap test

[Laura Abbott]

On 3/15/19 1:13 PM, John Stultz wrote:

On Fri, Mar 15, 2019 at 1:07 PM Laura Abbott  wrote:


On 3/6/19 9:01 AM, John Stultz wrote:

On Wed, Mar 6, 2019 at 8:14 AM Benjamin Gaignard
 wrote:

Le mar. 5 mars 2019 à 21:54, John Stultz  a écrit :

+
+   printf("Allocating 1 MEG\n");
+   ret = dmabuf_heap_alloc(heap_fd, ONE_MEG, 0, _fd);
+   if (ret)
+   goto out;
+
+   /* DO SOMETHING WITH THE DMABUF HERE? */


You can do a call to mmap and write a pattern in the buffer.


Yea. I can also do some invalid allocations to make sure things fail properly.

But I was talking a bit w/ Sumit about the lack of any general dmabuf
tests, and am curious if we need to have a importer device driver that
can validate its a real dmabuf and exercise more of the dmabuf ops.

thanks
-john



There's the vgem driver in drm. I did some work to clean that
up so it could take an import af33a9190d02 ("drm/vgem: Enable dmabuf import
interfaces") . I mostly used it for private tests and never ended
up upstreaming any of the tests.


Thanks for the poitner, I'll check that out as well!  Also, if you
still have them around, I'd be interested in checking out the tests to
try to get something integrated into kselftest.

Talking with Brian yesterday, there was some thought that we should
try to put together some sort of example dmabuf pipeline that isn't
hardware dependent and can be used to demonstrate the usage model as
well as validate the frameworks and maybe even benchmark some of the
ideas floating around right now.  So suggestions here would be
welcome!



So the existing ion selftest (tools/testing/selftests/android/ion)
does make use of the import to do some very simple tests.
I can't seem to find the more complex tests I had though
they may have been lost during my last machine move :(

I do think building off of vgem would be a good first step
for a testing pipeline, although I worry we wouldn't be
able to measure caching effects without a real device since
setting up coherency testing otherwise seems error prone.

Thanks,
Laura


thanks
-john



___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC][PATCH 0/5 v2] DMA-BUF Heaps (destaging ION)

[Laura Abbott]

On 3/5/19 12:54 PM, John Stultz wrote:

Here is a initial RFC of the dma-buf heaps patchset Andrew and I
have been working on which tries to destage a fair chunk of ION
functionality.

The patchset implements per-heap devices which can be opened
directly and then an ioctl is used to allocate a dmabuf from the
heap.

The interface is similar, but much simpler then IONs, only
providing an ALLOC ioctl.

Also, I've provided simple system and cma heaps. The system
heap in particular is missing the page-pool optimizations ION
had, but works well enough to validate the interface.

I've booted and tested these patches with AOSP on the HiKey960
using the kernel tree here:
   
https://git.linaro.org/people/john.stultz/android-dev.git/log/?h=dev/dma-buf-heap

And the userspace changes here:
   https://android-review.googlesource.com/c/device/linaro/hikey/+/909436


Compared to ION, this patchset is missing the system-contig,
carveout and chunk heaps, as I don't have a device that uses
those, so I'm unable to do much useful validation there.
Additionally we have no upstream users of chunk or carveout,
and the system-contig has been deprecated in the common/andoid-*
kernels, so this should be ok.

I've also removed the stats accounting for now, since it should
be implemented by the heaps themselves.

Eventual TODOS:
* Reimplement page-pool for system heap (working on this)
* Add stats accounting to system/cma heaps
* Make the kselftest actually useful
* Add other heaps folks see as useful (would love to get
   some help from actual carveout/chunk users)!

That said, the main user-interface is shaping up and I wanted
to get some input on the device model (particularly from GreKH)
and any other API/ABI specific input.

thanks
-john

Cc: Laura Abbott 
Cc: Benjamin Gaignard 
Cc: Greg KH 
Cc: Sumit Semwal 
Cc: Liam Mark 
Cc: Brian Starkey 
Cc: Andrew F. Davis 
Cc: Chenbo Feng 
Cc: Alistair Strachan 
Cc: dri-devel@lists.freedesktop.org

Andrew F. Davis (1):
   dma-buf: Add dma-buf heaps framework

John Stultz (4):
   dma-buf: heaps: Add heap helpers
   dma-buf: heaps: Add system heap to dmabuf heaps
   dma-buf: heaps: Add CMA heap to dmabuf heapss
   kselftests: Add dma-heap test

  MAINTAINERS|  16 +
  drivers/dma-buf/Kconfig|  10 +
  drivers/dma-buf/Makefile   |   2 +
  drivers/dma-buf/dma-heap.c | 191 
  drivers/dma-buf/heaps/Kconfig  |  14 +
  drivers/dma-buf/heaps/Makefile |   4 +
  drivers/dma-buf/heaps/cma_heap.c   | 164 ++
  drivers/dma-buf/heaps/heap-helpers.c   | 335 +
  drivers/dma-buf/heaps/heap-helpers.h   |  48 +++
  drivers/dma-buf/heaps/system_heap.c| 132 
  include/linux/dma-heap.h   |  65 
  include/uapi/linux/dma-heap.h  |  52 
  tools/testing/selftests/dmabuf-heaps/Makefile  |  11 +
  tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c |  96 ++
  14 files changed, 1140 insertions(+)
  create mode 100644 drivers/dma-buf/dma-heap.c
  create mode 100644 drivers/dma-buf/heaps/Kconfig
  create mode 100644 drivers/dma-buf/heaps/Makefile
  create mode 100644 drivers/dma-buf/heaps/cma_heap.c
  create mode 100644 drivers/dma-buf/heaps/heap-helpers.c
  create mode 100644 drivers/dma-buf/heaps/heap-helpers.h
  create mode 100644 drivers/dma-buf/heaps/system_heap.c
  create mode 100644 include/linux/dma-heap.h
  create mode 100644 include/uapi/linux/dma-heap.h
  create mode 100644 tools/testing/selftests/dmabuf-heaps/Makefile
  create mode 100644 tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c



This is looking really great. Thanks for doing the work to
push this forward. It seems like we're in general agreement
about much of this. Which of the issues that have come up
do you think are a "hard no" to keeping this from going in?

Thanks,
Laura
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC][PATCH 1/5 v2] dma-buf: Add dma-buf heaps framework

[Laura Abbott]

On 3/5/19 12:54 PM, John Stultz wrote:

+DMA-BUF HEAPS FRAMEWORK
+M: Laura Abbott
+R: Liam Mark
+R: Brian Starkey
+R: "Andrew F. Davis"
+R: John Stultz
+S: Maintained
+L: linux-me...@vger.kernel.org
+L: dri-devel@lists.freedesktop.org
+L: linaro-mm-...@lists.linaro.org  (moderated for non-subscribers)
+F: include/uapi/linux/dma-heap.h
+F: include/linux/dma-heap.h
+F: drivers/dma-buf/dma-heap.c
+F: drivers/dma-buf/heaps/*
+T: git git://anongit.freedesktop.org/drm/drm-misc


So I talked about this with Sumit privately but I think
it might make sense to have me step down as maintainer when
this goes out of staging. I mostly worked on Ion at my
previous position and anything I do now is mostly a side
project. I still want to see it succeed which is why I
took on the maintainer role but I don't want to become blocking
for people who have a stronger vision about where this needs
to go (see also, I'm not working with this on a daily basis).

If you just want someone to help review or take patches
to be pulled, I'm happy to do so but I'd hate to become
the bottleneck on getting things done for people who
are attempting to do real work.

Thanks,
Laura
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC][PATCH 5/5 v2] kselftests: Add dma-heap test

[Laura Abbott]

On 3/6/19 9:01 AM, John Stultz wrote:

On Wed, Mar 6, 2019 at 8:14 AM Benjamin Gaignard
 wrote:

Le mar. 5 mars 2019 à 21:54, John Stultz  a écrit :

+
+   printf("Allocating 1 MEG\n");
+   ret = dmabuf_heap_alloc(heap_fd, ONE_MEG, 0, _fd);
+   if (ret)
+   goto out;
+
+   /* DO SOMETHING WITH THE DMABUF HERE? */


You can do a call to mmap and write a pattern in the buffer.


Yea. I can also do some invalid allocations to make sure things fail properly.

But I was talking a bit w/ Sumit about the lack of any general dmabuf
tests, and am curious if we need to have a importer device driver that
can validate its a real dmabuf and exercise more of the dmabuf ops.

thanks
-john



There's the vgem driver in drm. I did some work to clean that
up so it could take an import af33a9190d02 ("drm/vgem: Enable dmabuf import
interfaces") . I mostly used it for private tests and never ended
up upstreaming any of the tests.

Thanks,
Laura
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/vgem: fix use-after-free when drm_gem_handle_create() fails

[Laura Abbott]

On 2/26/19 1:44 PM, Eric Biggers wrote:

From: Eric Biggers 

If drm_gem_handle_create() fails in vgem_gem_create(), then the
drm_vgem_gem_object is freed twice: once when the reference is dropped
by drm_gem_object_put_unlocked(), and again by __vgem_gem_destroy().

This was hit by syzkaller using fault injection.

Fix it by skipping the second free.

Reported-by: syzbot+e73f2fb5ed5a5df36...@syzkaller.appspotmail.com
Fixes: af33a9190d02 ("drm/vgem: Enable dmabuf import interfaces")
Reviewed-by: Chris Wilson 
Cc: Laura Abbott 
Cc: Daniel Vetter 
Cc: sta...@vger.kernel.org
Signed-off-by: Eric Biggers 
---
  drivers/gpu/drm/vgem/vgem_drv.c | 6 +-
  1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c
index 5930facd6d2d8..11a8f99ba18c5 100644
--- a/drivers/gpu/drm/vgem/vgem_drv.c
+++ b/drivers/gpu/drm/vgem/vgem_drv.c
@@ -191,13 +191,9 @@ static struct drm_gem_object *vgem_gem_create(struct 
drm_device *dev,
ret = drm_gem_handle_create(file, >base, handle);
drm_gem_object_put_unlocked(>base);
if (ret)
-   goto err;
+   return ERR_PTR(ret);
  
  	return >base;

-
-err:
-   __vgem_gem_destroy(obj);
-   return ERR_PTR(ret);
  }
  
  static int vgem_gem_dumb_create(struct drm_file *file, struct drm_device *dev,





Acked-by: Laura Abbott 
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [EARLY RFC][PATCH] dma-buf: Add dma-buf heaps framework

[Laura Abbott]

On 2/25/19 6:36 AM, Andrew F. Davis wrote:

This framework allows a unified userspace interface for dma-buf
exporters, allowing userland to allocate specific types of memory
for use in dma-buf sharing.

Each heap is given its own device node, which a user can allocate
a dma-buf fd from using the DMA_HEAP_IOC_ALLOC.

Signed-off-by: Andrew F. Davis 
---

Hello all,

I had a little less time over the weekend than I thought I would to
clean this up more and finish the first set of user heaps, but wanted
to get this out anyway.

ION in its current form assumes a lot about the memory it exports and
these assumptions lead to restrictions on the full range of operations
dma-buf's can produce. Due to this if we are to add this as an extension
of the core dma-buf then it should only be the user-space advertisement
and allocation front-end. All dma-buf exporting and operation need to be
placed in the control of the exporting heap. The core becomes rather
small, just a few hundred lines you see here. This is not to say we
should not provide helpers to make the heap exporters more simple, but
they should only be helpers and not enforced by the core framework.

Basic use model here is an exporter (dedicated heap memory driver, CMA,
System, etc.) registers with the framework by providing a struct
dma_heap_info which gives us the needed info to export this heap to
userspace as a device node. Next a user will request an allocation,
the IOCTL is parsed and the request made to a heap provided alloc() op.
The heap should return the filled out struct dma_heap_buffer, including
exporting the buffer as a dma-buf. This dma-buf we then return to the
user as a fd. Buffer free is still a bit open as we need to update some
stats and free some memory, but the release operation is controlled by
the heap exporter, so some hook will have to be created.

It all needs a bit more work, and I'm sure I'll find missing parts when
I add some more heaps, but hopefully this framework is simple enough that
it does not impede the implementation of all functionality once provided
by ION (shrinker, delayed free), nor any new functionality needed for
future heap exporting devices.



I like this concept and I'm happy to see it go forward. Some high level
comments


Thanks,
Andrew

  drivers/dma-buf/Kconfig   |  12 ++
  drivers/dma-buf/Makefile  |   1 +
  drivers/dma-buf/dma-heap.c| 268 ++
  include/linux/dma-heap.h  |  57 
  include/uapi/linux/dma-heap.h |  54 +++
  5 files changed, 392 insertions(+)
  create mode 100644 drivers/dma-buf/dma-heap.c
  create mode 100644 include/linux/dma-heap.h
  create mode 100644 include/uapi/linux/dma-heap.h

diff --git a/drivers/dma-buf/Kconfig b/drivers/dma-buf/Kconfig
index 2e5a0faa2cb1..30b0d7c83945 100644
--- a/drivers/dma-buf/Kconfig
+++ b/drivers/dma-buf/Kconfig
@@ -39,4 +39,16 @@ config UDMABUF
  A driver to let userspace turn memfd regions into dma-bufs.
  Qemu can use this to create host dmabufs for guest framebuffers.
  
+menuconfig DMABUF_HEAPS

+   bool "DMA-BUF Userland Memory Heaps"
+   depends on HAS_DMA && MMU
+   select GENERIC_ALLOCATOR
+   select DMA_SHARED_BUFFER
+   help
+ Choose this option to enable the DMA-BUF userland memory heaps,
+ this allows userspace to allocate dma-bufs that can be shared between
+ drivers.
+
+source "drivers/dma-buf/heaps/Kconfig"
+
  endmenu
diff --git a/drivers/dma-buf/Makefile b/drivers/dma-buf/Makefile
index 0913a6ccab5a..b0332f143413 100644
--- a/drivers/dma-buf/Makefile
+++ b/drivers/dma-buf/Makefile
@@ -1,4 +1,5 @@
  obj-y := dma-buf.o dma-fence.o dma-fence-array.o reservation.o seqno-fence.o
+obj-$(CONFIG_DMABUF_HEAPS) += dma-heap.o
  obj-$(CONFIG_SYNC_FILE)   += sync_file.o
  obj-$(CONFIG_SW_SYNC) += sw_sync.o sync_debug.o
  obj-$(CONFIG_UDMABUF) += udmabuf.o
diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c
new file mode 100644
index ..72ed225fa892
--- /dev/null
+++ b/drivers/dma-buf/dma-heap.c
@@ -0,0 +1,268 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Framework for userspace DMA-BUF allocations
+ *
+ * Copyright (C) 2011 Google, Inc.
+ * Copyright (C) 2019 Linaro Ltd.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+
+#define DEVNAME "dma_heap"
+
+#define NUM_HEAP_MINORS 128
+static DEFINE_IDR(dma_heap_idr);
+static DEFINE_MUTEX(minor_lock); /* Protect idr accesses */
+
+dev_t dma_heap_devt;
+struct class *dma_heap_class;


Can we make sure this gets reviewed by Greg sooner rather than
later when we drop the RFC? I think the use of this here
is fine with the device model but I'd rather not find out at
the end.


+struct list_head dma_heap_list;
+struct dentry *dma_heap_debug_root;
+
+/**
+ * struct dma_heap - represents a dmabuf heap in the system
+ * @name:  used for 

Re: [EARLY RFC][PATCH 0/4] dmabuf pools infrastructure (destaging ION)

[Laura Abbott]

On 2/22/19 9:24 AM, John Stultz wrote:

On Fri, Feb 22, 2019 at 8:55 AM Andrew F. Davis  wrote:

On 2/21/19 1:40 AM, John Stultz wrote:

Here is a very early peek at my dmabuf pools patchset, which
tries to destage a fair chunk of ION functionality.

This build and boots, but I've not gotten to testing the actual
pool devices yet (need to write some kselftests)! I just wanted
some early feedback on the overall direction.

The patchset implements per-pool devices (extending my ion
per-heap devices patchset from last week), which can be opened
directly and then an ioctl is used to allocate a dmabuf from the
pool.

The interface is similar, but simpler then IONs, only providing
an ALLOC ioctl.

Also, I've only destaged the system/system-contig and cma pools,
since the ION carveout and chunk heaps depended on out of tree
board files to initialize those heaps. I'll leave that to folks
who are actually using those heaps.

Let me know what you think!



+1

Was this source not pulled from -next, I have some fixes in next that I
don't see in this code, so I won't review the code itself just yet (it
is and early RFC after all). For the concept itself I have a couple
small suggestions:


Oh, no, I've missed those. I was working off -rc7. I'll try to
re-integrate them in.


I'm not sure I like the name. "Pool" in the context of DMA-BUF feels
like it means something else, like some new feature of DMA-BUFs
exporters/importers can use for making buffer pools. How about just keep
the "heap" terminology to prevent too much re-wording. Maybe just call
this dma-buf/heaps/ ?


The name changing was mostly as Laura noted that the term heap has
caused confusion historically. I'm not really particular, and I do
worry about the naming overlap between dmabuf-pools and the pagepool
code was problematic. Due to that overlap, renaming things back will
be a small chore, but I've got only myself to blame there :)




Yeah I'm not set on changing the names. If everyone else finds
heap to be descriptive enough, we can keep it.

Thanks,
Laura
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [EARLY RFC][PATCH 0/4] ION per heap devices

[Laura Abbott]

On 2/19/19 1:30 PM, Andrew F. Davis wrote:

On 2/19/19 3:25 PM, Laura Abbott wrote:

On 2/15/19 12:24 PM, John Stultz wrote:

This is a *very early RFC* (it builds, that's all I'll say :)
but I wanted to share it to get some initial feedback before I
go down the rabit hole of trying to adapt the Android userland
code to get this fully validated.

This patchset tries to implement the per-heap devices for ION.
The main benefit is that it avoids multiplexing heap operations
through the /dev/ion interface, and allows for each heap to have
its own permissions/sepolicy rules.

Feedback would be greatly appreciated!
thanks
-john

Cc: Laura Abbott 
Cc: Sumit Semwal 
Cc: Liam Mark 
Cc: Brian Starkey 
Cc: Andrew F. Davis 
Cc: Alistair Strachan 
Cc: dri-devel@lists.freedesktop.org

John Stultz (4):
    ion: Add ION_VERSION ioctl
    ion: Initial hack to create per heap devices
    ion: Add HEAP_INFO ioctl to be able to fetch heap type
    ion: Make "legacy" /dev/ion support optional

   drivers/staging/android/ion/Kconfig |  7 +++
   drivers/staging/android/ion/ion-ioctl.c | 80
+
   drivers/staging/android/ion/ion.c   | 51 -
   drivers/staging/android/ion/ion.h   |  6 +++
   drivers/staging/android/uapi/ion.h  | 57 +++
   5 files changed, 191 insertions(+), 10 deletions(-)



So it occurs to me if this is going to be a new ABI
all together maybe we should just declare a new allocation ioctl
to be used with it. We can keep the old ioctls around
for legacy use cases and maybe eventually delete them
and just use the new allocation ioctl with the new
split heaps.



Why keep the old ones, this is staging, there are no legacy users (that
matter to kernel).. Slowing progress for the sake of backwards compat
with staging just slows the de-staging down.



I think we just fundamentally disagree here. I don't see keeping
legacy users as slowing anything down. We're still getting
the new ABI that we actually like and we get the chance to easily
go back and test. Having a non broken ABI makes it much
easier to do testing and validation and comparison. We can remove
the last ABI before we move it out of staging.

Thanks,
Laura


Andrew


Thanks,
Laura


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [EARLY RFC][PATCH 3/4] ion: Add HEAP_INFO ioctl to be able to fetch heap type

[Laura Abbott]

On 2/19/19 1:39 PM, Andrew F. Davis wrote:

On 2/19/19 3:13 PM, Laura Abbott wrote:

On 2/15/19 12:24 PM, John Stultz wrote:

The per-device heaps don't support HEAP_QUERY ioctl, since
the name is provided in the devnode path and the heapid isn't
useful with the new interface (one uses the fd of heapdevice).

But, one missing bit of functionality is a way to find the
heap type. So provide a HEAP_INFO ioctl which exposes the
heap type out so there is the potential for some sort of
dynamic heap matching/discovery.

Most likely this IOCTL will be useful when extended to allow
some sort of opaque constraint bitfield to be shared so userland
can match heaps with devices in a fully dynamic way.



We've been waiting on the constraint solving for a while and
it's never really happened :(



Most likely there will never be a one-size-fits-all solution here. So
allowing for an extensible ABI that permits new information to be
exported as needed will be important.


It certainly works but I'm concerned about adding this and
then finding (yet again) that it doesn't work. We're
getting the heap name now but do we lose anything if we
don't expose it as part of the ABI?



We can always add more ioctls, we cant go back and remove the old ones
if we make them too clunky and have to remove something they expose. A
simple starting ABI seems to make the most sense here. Even heap type
doesn't look like a good thing to expose, it is just as static and
one-off as heap name, I don't see it having all that much use :/



That's my point though, why are we adding this ioctl now if we
don't have a good idea of its use case or why we want the heap
type exposed? If we come up with a good use later we can
add the ioctl then with better requirements.


Andrew


Cc: Laura Abbott 
Cc: Sumit Semwal 
Cc: Liam Mark 
Cc: Brian Starkey 
Cc: Andrew F. Davis 
Cc: Alistair Strachan 
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: John Stultz 
---
   drivers/staging/android/ion/ion-ioctl.c | 12 
   drivers/staging/android/uapi/ion.h  | 22 ++
   2 files changed, 34 insertions(+)

diff --git a/drivers/staging/android/ion/ion-ioctl.c
b/drivers/staging/android/ion/ion-ioctl.c
index ea8d263..6db5969 100644
--- a/drivers/staging/android/ion/ion-ioctl.c
+++ b/drivers/staging/android/ion/ion-ioctl.c
@@ -14,6 +14,7 @@ union ion_ioctl_arg {
   struct ion_allocation_data allocation;
   struct ion_heap_allocation_data heap_allocation;
   struct ion_heap_query query;
+    struct ion_heap_info heap_info;
   u32 version;
   };
   @@ -149,6 +150,17 @@ long ion_heap_ioctl(struct file *filp, unsigned
int cmd, unsigned long arg)
     break;
   }
+    case ION_IOC_HEAP_INFO:
+    {
+    struct miscdevice *miscdev = filp->private_data;
+    struct ion_heap *heap;
+
+    heap = container_of(miscdev, struct ion_heap, heap_dev);
+
+    data.heap_info.type = heap->type;
+
+    break;
+    }
   case ION_IOC_VERSION:
   data.version = ION_VERSION;
   break;
diff --git a/drivers/staging/android/uapi/ion.h
b/drivers/staging/android/uapi/ion.h
index 20db09f..1b3ca1e 100644
--- a/drivers/staging/android/uapi/ion.h
+++ b/drivers/staging/android/uapi/ion.h
@@ -111,6 +111,19 @@ struct ion_heap_data {
   };
     /**
+ * struct ion_heap_info - Info about the heap
+ *
+ */
+struct ion_heap_info {
+    __u32 type;
+    __u32 reserved0;
+    __u32 reserved1;
+    __u32 reserved2;
+    __u32 reserved3;
+    __u32 reserved4;
+};
+
+/**
    * struct ion_heap_query - collection of data about all heaps
    * @cnt - total number of heaps to be copied
    * @heaps - buffer to copy heap data
@@ -159,4 +172,13 @@ struct ion_heap_query {
   #define ION_IOC_HEAP_ALLOC    _IOWR(ION_IOC_MAGIC, 10, \
     struct ion_heap_allocation_data)
   +/**
+ * DOC: ION_IOC_HEAP_INFO - allocate memory from heap
+ *
+ * Takes an ion_heap_query structure and populates information about
+ * available Ion heaps.
+ */
+#define ION_IOC_HEAP_INFO    _IOWR(ION_IOC_MAGIC, 11, \
+  struct ion_heap_allocation_data)
+
   #endif /* _UAPI_LINUX_ION_H */





___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [EARLY RFC][PATCH 0/4] ION per heap devices

[Laura Abbott]

On 2/15/19 12:24 PM, John Stultz wrote:

This is a *very early RFC* (it builds, that's all I'll say :)
but I wanted to share it to get some initial feedback before I
go down the rabit hole of trying to adapt the Android userland
code to get this fully validated.

This patchset tries to implement the per-heap devices for ION.
The main benefit is that it avoids multiplexing heap operations
through the /dev/ion interface, and allows for each heap to have
its own permissions/sepolicy rules.

Feedback would be greatly appreciated!
thanks
-john

Cc: Laura Abbott 
Cc: Sumit Semwal 
Cc: Liam Mark 
Cc: Brian Starkey 
Cc: Andrew F. Davis 
Cc: Alistair Strachan 
Cc: dri-devel@lists.freedesktop.org

John Stultz (4):
   ion: Add ION_VERSION ioctl
   ion: Initial hack to create per heap devices
   ion: Add HEAP_INFO ioctl to be able to fetch heap type
   ion: Make "legacy" /dev/ion support optional

  drivers/staging/android/ion/Kconfig |  7 +++
  drivers/staging/android/ion/ion-ioctl.c | 80 +
  drivers/staging/android/ion/ion.c   | 51 -
  drivers/staging/android/ion/ion.h   |  6 +++
  drivers/staging/android/uapi/ion.h  | 57 +++
  5 files changed, 191 insertions(+), 10 deletions(-)



So it occurs to me if this is going to be a new ABI
all together maybe we should just declare a new allocation ioctl
to be used with it. We can keep the old ioctls around
for legacy use cases and maybe eventually delete them
and just use the new allocation ioctl with the new
split heaps.

Thanks,
Laura
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-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 
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [EARLY RFC][PATCH 2/4] ion: Initial hack to create per heap devices

[Laura Abbott]

On 2/15/19 12:24 PM, John Stultz wrote:

One of the issues w/ the /dev/ion interface is that we have to
provide the complexity of a heap query interface and we end up
multiplexing all the heap access through that one interface via
a bit mask (which currently limits the heaps to 32).

There has been a long running todo to provide per-heap devices
which would make the heap discovery/query interface "ls", and
would allow for different heaps to have different permisisons
and sepolicy rules.

TODOs:
* Android doesn't use udev so "ion_heaps/%s" names don't
   automatically create a /dev/ subdir. I need to rework
   from miscdev to creating a proper device class and add
   a "subsystem" entry for the DeviceHandler to match with

* Each CMA region is exposed via a separate heap, not sure
   if this is desired or not, and we may need to improve the
   naming.



Every CMA region getting exposed was a side effect of doing
the eneumeration without tying it to devicetree or other firmware.
I'm not opposed to limiting the heaps exposed if we can find
a good way to do so that's still compliant with devicetree/whatever.

Thanks,
Laura


Cc: Laura Abbott 
Cc: Sumit Semwal 
Cc: Liam Mark 
Cc: Brian Starkey 
Cc: Andrew F. Davis 
Cc: Alistair Strachan 
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: John Stultz 
---
  drivers/staging/android/ion/ion-ioctl.c | 62 +
  drivers/staging/android/ion/ion.c   | 18 ++
  drivers/staging/android/ion/ion.h   |  2 ++
  drivers/staging/android/uapi/ion.h  | 28 +++
  4 files changed, 110 insertions(+)

diff --git a/drivers/staging/android/ion/ion-ioctl.c 
b/drivers/staging/android/ion/ion-ioctl.c
index 458a9f2..ea8d263 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_heap_allocation_data heap_allocation;
struct ion_heap_query query;
u32 version;
  };
@@ -100,3 +101,64 @@ long ion_ioctl(struct file *filp, unsigned int cmd, 
unsigned long arg)
}
return ret;
  }
+
+long ion_heap_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
+{
+   int ret = 0;
+   unsigned int dir;
+   union ion_ioctl_arg data;
+
+   dir = ion_ioctl_dir(cmd);
+
+   if (_IOC_SIZE(cmd) > sizeof(data))
+   return -EINVAL;
+
+   /*
+* The copy_from_user is unconditional here for both read and write
+* to do the validate. If there is no write for the ioctl, the
+* buffer is cleared
+*/
+   if (copy_from_user(, (void __user *)arg, _IOC_SIZE(cmd)))
+   return -EFAULT;
+
+   ret = validate_ioctl_arg(cmd, );
+   if (ret) {
+   pr_warn_once("%s: ioctl validate failed\n", __func__);
+   return ret;
+   }
+
+   if (!(dir & _IOC_WRITE))
+   memset(, 0, sizeof(data));
+
+   switch (cmd) {
+   case ION_IOC_HEAP_ALLOC:
+   {
+   struct miscdevice *miscdev = filp->private_data;
+   struct ion_heap *heap;
+   int fd;
+
+   heap = container_of(miscdev, struct ion_heap, heap_dev);
+
+   fd = ion_alloc(data.heap_allocation.len,
+  (1 << heap->id),
+  data.heap_allocation.flags);
+   if (fd < 0)
+   return fd;
+
+   data.heap_allocation.fd = fd;
+
+   break;
+   }
+   case ION_IOC_VERSION:
+   data.version = ION_VERSION;
+   break;
+   default:
+   return -ENOTTY;
+   }
+
+   if (dir & _IOC_READ) {
+   if (copy_to_user((void __user *)arg, , _IOC_SIZE(cmd)))
+   return -EFAULT;
+   }
+   return ret;
+}
diff --git a/drivers/staging/android/ion/ion.c 
b/drivers/staging/android/ion/ion.c
index 6f5afab..1f7c893 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -492,6 +492,14 @@ int ion_query_heaps(struct ion_heap_query *query)
return ret;
  }
  
+static const struct file_operations ion_heap_fops = {

+   .owner  = THIS_MODULE,
+   .unlocked_ioctl = ion_heap_ioctl,
+#ifdef CONFIG_COMPAT
+   .compat_ioctl   = ion_heap_ioctl,
+#endif
+};
+
  static const struct file_operations ion_fops = {
.owner  = THIS_MODULE,
.unlocked_ioctl = ion_ioctl,
@@ -540,12 +548,22 @@ void ion_device_add_heap(struct ion_heap *heap)
struct ion_device *dev = internal_dev;
int ret;
struct dentry *heap_root;
+   char *heap_name;
char debug_name[64];
  
  	if (!heap->ops->allocate || !heap->ops->free)

pr_err("%s: can not add heap with invalid ops struct.\n",
   

Re: [EARLY RFC][PATCH 3/4] ion: Add HEAP_INFO ioctl to be able to fetch heap type

[Laura Abbott]

On 2/15/19 12:24 PM, John Stultz wrote:

The per-device heaps don't support HEAP_QUERY ioctl, since
the name is provided in the devnode path and the heapid isn't
useful with the new interface (one uses the fd of heapdevice).

But, one missing bit of functionality is a way to find the
heap type. So provide a HEAP_INFO ioctl which exposes the
heap type out so there is the potential for some sort of
dynamic heap matching/discovery.

Most likely this IOCTL will be useful when extended to allow
some sort of opaque constraint bitfield to be shared so userland
can match heaps with devices in a fully dynamic way.



We've been waiting on the constraint solving for a while and
it's never really happened :(

It certainly works but I'm concerned about adding this and
then finding (yet again) that it doesn't work. We're
getting the heap name now but do we lose anything if we
don't expose it as part of the ABI?


Cc: Laura Abbott 
Cc: Sumit Semwal 
Cc: Liam Mark 
Cc: Brian Starkey 
Cc: Andrew F. Davis 
Cc: Alistair Strachan 
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: John Stultz 
---
  drivers/staging/android/ion/ion-ioctl.c | 12 
  drivers/staging/android/uapi/ion.h  | 22 ++
  2 files changed, 34 insertions(+)

diff --git a/drivers/staging/android/ion/ion-ioctl.c 
b/drivers/staging/android/ion/ion-ioctl.c
index ea8d263..6db5969 100644
--- a/drivers/staging/android/ion/ion-ioctl.c
+++ b/drivers/staging/android/ion/ion-ioctl.c
@@ -14,6 +14,7 @@ union ion_ioctl_arg {
struct ion_allocation_data allocation;
struct ion_heap_allocation_data heap_allocation;
struct ion_heap_query query;
+   struct ion_heap_info heap_info;
u32 version;
  };
  
@@ -149,6 +150,17 @@ long ion_heap_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
  
  		break;

}
+   case ION_IOC_HEAP_INFO:
+   {
+   struct miscdevice *miscdev = filp->private_data;
+   struct ion_heap *heap;
+
+   heap = container_of(miscdev, struct ion_heap, heap_dev);
+
+   data.heap_info.type = heap->type;
+
+   break;
+   }
case ION_IOC_VERSION:
data.version = ION_VERSION;
break;
diff --git a/drivers/staging/android/uapi/ion.h 
b/drivers/staging/android/uapi/ion.h
index 20db09f..1b3ca1e 100644
--- a/drivers/staging/android/uapi/ion.h
+++ b/drivers/staging/android/uapi/ion.h
@@ -111,6 +111,19 @@ struct ion_heap_data {
  };
  
  /**

+ * struct ion_heap_info - Info about the heap
+ *
+ */
+struct ion_heap_info {
+   __u32 type;
+   __u32 reserved0;
+   __u32 reserved1;
+   __u32 reserved2;
+   __u32 reserved3;
+   __u32 reserved4;
+};
+
+/**
   * struct ion_heap_query - collection of data about all heaps
   * @cnt - total number of heaps to be copied
   * @heaps - buffer to copy heap data
@@ -159,4 +172,13 @@ struct ion_heap_query {
  #define ION_IOC_HEAP_ALLOC_IOWR(ION_IOC_MAGIC, 10, \
  struct ion_heap_allocation_data)
  
+/**

+ * DOC: ION_IOC_HEAP_INFO - allocate memory from heap
+ *
+ * Takes an ion_heap_query structure and populates information about
+ * available Ion heaps.
+ */
+#define ION_IOC_HEAP_INFO  _IOWR(ION_IOC_MAGIC, 11, \
+ struct ion_heap_allocation_data)
+
  #endif /* _UAPI_LINUX_ION_H */



___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [EARLY RFC][PATCH 0/4] ION per heap devices

[Laura Abbott]

On 2/19/19 9:21 AM, John Stultz wrote:

On Mon, Feb 18, 2019 at 3:51 AM Brian Starkey  wrote:

On Fri, Feb 15, 2019 at 12:24:08PM -0800, John Stultz wrote:

This is a *very early RFC* (it builds, that's all I'll say :)
but I wanted to share it to get some initial feedback before I
go down the rabit hole of trying to adapt the Android userland
code to get this fully validated.

This patchset tries to implement the per-heap devices for ION.
The main benefit is that it avoids multiplexing heap operations
through the /dev/ion interface, and allows for each heap to have
its own permissions/sepolicy rules.


In general, I've always thought that having a device node per-heap is
a bit messy for userspace. Multiplexing through the single node
doesn't seem like an insurmountable problem, but the point about


Hrm. I guess for me having a custom enumeration interface over ioctl
seems less ideal compared to a directory list.


permissions/sepolicy is reasonably compelling if it's a real
requirement. What would be the reasons for that?


Its a bit second hand for me, so hopefully I don't have it wrong. I've
cc'ed some additional google folks and Benjamin for extra context, but
my sense of it is that you may have some less-trusted code that we're
fine with allocating system heap dma-bufs, but don't want to to be
giving access to more limited heaps like carveout or cma, or more
potentially security troubling heaps like system-contig.

thanks
-john



Yes, the discussion was always based on being able to set separate
security policy for each heap. It's not clear to me how strong a
requirement is these days or if there's other options to enforce
the same thing.

Thanks,
Laura
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [EARLY RFC][PATCH 1/4] ion: Add ION_VERSION ioctl

[Laura Abbott]

On 2/15/19 12:24 PM, John Stultz wrote:

With all the slight interface changes ion has had
through its time in staging, keeping userland working
properly has been a pain. Assuming more churn going
forward, provide a proper version interface.

Cc: Laura Abbott 
Cc: Sumit Semwal 
Cc: Liam Mark 
Cc: Brian Starkey 
Cc: Andrew F. Davis 
Cc: Alistair Strachan 
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: John Stultz 
---
  drivers/staging/android/ion/ion-ioctl.c | 4 
  drivers/staging/android/ion/ion.h   | 2 ++
  drivers/staging/android/uapi/ion.h  | 7 +++
  3 files changed, 13 insertions(+)

diff --git a/drivers/staging/android/ion/ion-ioctl.c 
b/drivers/staging/android/ion/ion-ioctl.c
index a8d3cc4..458a9f2 100644
--- a/drivers/staging/android/ion/ion-ioctl.c
+++ b/drivers/staging/android/ion/ion-ioctl.c
@@ -13,6 +13,7 @@
  union ion_ioctl_arg {
struct ion_allocation_data allocation;
struct ion_heap_query query;
+   u32 version;
  };
  
  static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)

@@ -86,6 +87,9 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned 
long arg)
case ION_IOC_HEAP_QUERY:
ret = ion_query_heaps();
break;
+   case ION_IOC_VERSION:
+   data.version = ION_VERSION;
+   break;
default:
return -ENOTTY;
}
diff --git a/drivers/staging/android/ion/ion.h 
b/drivers/staging/android/ion/ion.h
index 47b594c..439e682 100644
--- a/drivers/staging/android/ion/ion.h
+++ b/drivers/staging/android/ion/ion.h
@@ -21,6 +21,8 @@
  
  #include "../uapi/ion.h"
  
+#define ION_VERSION 3

+
  /**
   * struct ion_platform_heap - defines a heap in the given platform
   * @type: type of the heap from ion_heap_type enum
diff --git a/drivers/staging/android/uapi/ion.h 
b/drivers/staging/android/uapi/ion.h
index 5d70098..c480448 100644
--- a/drivers/staging/android/uapi/ion.h
+++ b/drivers/staging/android/uapi/ion.h
@@ -124,4 +124,11 @@ struct ion_heap_query {
  #define ION_IOC_HEAP_QUERY _IOWR(ION_IOC_MAGIC, 8, \
struct ion_heap_query)
  
+/**

+ * DOC: ION_IOC_VERSION - Get ION interface version
+ *
+ * Takes a u32 and returns the ION interface version
+ */
+#define ION_IOC_VERSION_IOR(ION_IOC_MAGIC, 9, u32)
+
  #endif /* _UAPI_LINUX_ION_H */



Like I said on the other thread, I was told no before
https://lore.kernel.org/lkml/1472769644-11039-4-git-send-email-labb...@redhat.com/T/#u
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Laura Abbott via dri-devel]

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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-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 
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-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






___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-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



___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-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 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-devel@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-devel@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-devel@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: 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,



___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-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-devel@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-devel@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-devel@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 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



___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-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"



___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] staging: ion: Rework ion_map_dma_buf() to minimize re-mapping

[Laura Abbott]

On 10/17/2018 07:53 PM, John Stultz wrote:

On Fri, Oct 12, 2018 at 10:51 AM, Laura Abbott  wrote:


I suspect most of the cost of the dma_map/dma_unmap is from the
cache flushing and not the actual mapping operations. If this
is the case, another option might be to figure out how to
incorporate dma_attrs so drivers can use DMA_ATTR_SKIP_CPU_SYNC
to decide when they actually want to sync.


So just to confirm on this point, I basically tested the following
change (whitespace corrupt, sorry):

diff --git a/drivers/staging/android/ion/ion.c
b/drivers/staging/android/ion/ion.c
index 24cb666..e76b0e2 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -273,8 +273,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))
 return ERR_PTR(-ENOMEM);

 return table;
@@ -284,7 +284,7 @@ static void ion_unmap_dma_buf(struct
dma_buf_attachment *attachment,
   struct sg_table *table,
   enum dma_data_direction direction)
  {
-   dma_unmap_sg(attachment->dev, table->sgl, table->nents, direction);
+   dma_unmap_sg_attrs(attachment->dev, table->sgl, table->nents,
direction, DMA_ATTR_SKIP_CPU_SYNC);
  }

  static int ion_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma)


And indeed, that performed similarly to the pre 4.12 ION code (and it
also had some of the same image caching error garbage we've seen w/
4.9 era kernels, which my earlier patch didn't have).

So yes, it seems having some way to conditionally skip cpu sync would
be good.  Though I'm not sure what sort of interface to using this you
might have in mind?



I hadn't quite gotten anything fully formed but I think solving this
will require changes at the dma-buf layer. One idea I had was allowing
dma_attrs to be set per attachment, the other was extending the
dma_buf_map_attachment to take an attrs argument. I'm at OSSEU this
week but I didn't want to forget about this. I'll see if I can turn
this into a more coherent proposal unless you get to it first.

Thanks,
Laura


thanks
-john



___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] staging: ion: Rework ion_map_dma_buf() to minimize re-mapping

[Laura Abbott]

On 10/10/2018 04:33 PM, John Stultz wrote:

Since 4.12, much later narrowed down to commit 2a55e7b5e544
("staging: android: ion: Call dma_map_sg for syncing and mapping"),
we have seen graphics performance issues on the HiKey960.

This was initially confounded by the fact that the out-of-tree
DRM driver was using HiSi custom ION heap which broke with the
4.12 ION abi changes, so there was lots of suspicion that the
performance problems were due to switching to a somewhat simple
cma based DRM driver for HiKey960. Additionally, as no
performance regression was seen w/ the original HiKey board
(which is SMP, not big.LITTLE as w/ HiKey960), there was some
thought that the out-of-tree EAS code wasn't quite optimized.

But after chasing a number of other leads, I found that
reverting the ION code to 4.11-era got the majority of the
graphics performance back (there may yet be further EAS tweaks
needed), which lead me to the dma_map_sg change.

In talking w/ Laura and Liam, it was suspected that the extra
cache operations were causing the trouble. Additionally, I found
that part of the reason we didn't see this w/ the original
HiKey board is that its (proprietary blob) GL code uses ion_mmap
and ion_map_dma_buf is called very rarely, where as with
HiKey960, the (also proprietary blob) GL code calls
ion_map_dma_buf much more frequently via the kernel driver.

Anyway, with the cause of the performance regression isolated,
I've tried to find a way to improve the performance of the
current code.

This approach, which I've mostly copied from the drm_prime
implementation is to try to track the direction we're mapping
the buffers so we can avoid calling dma_map/unmap_sg on every
ion_map_dma_buf/ion_unmap_dma_buf call, and instead try to do
the work in attach/detach paths.

I'm not 100% sure of the correctness here, so close review would
be good, but it gets the performance back to being similar to
reverting the ION code to the 4.11-era.

Feedback would be greatly appreciated!

Cc: Beata Michalska 
Cc: Matt Szczesiak 
Cc: Anders Pedersen 
Cc: John Reitan 
Cc: Liam Mark 
Cc: Laura Abbott 
Cc: Sumit Semwal 
Cc: Greg Kroah-Hartman 
Cc: Todd Kjos 
Cc: Martijn Coenen 
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: John Stultz 
---
  drivers/staging/android/ion/ion.c | 36 +++-
  1 file changed, 31 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/android/ion/ion.c 
b/drivers/staging/android/ion/ion.c
index 9907332..a4d7fca 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -199,6 +199,7 @@ struct ion_dma_buf_attachment {
struct device *dev;
struct sg_table *table;
struct list_head list;
+   enum dma_data_direction dir;
  };
  
  static int ion_dma_buf_attach(struct dma_buf *dmabuf,

@@ -220,6 +221,7 @@ static int ion_dma_buf_attach(struct dma_buf *dmabuf,
  
  	a->table = table;

a->dev = attachment->dev;
+   a->dir = DMA_NONE;
INIT_LIST_HEAD(>list);
  
  	attachment->priv = a;

@@ -236,6 +238,18 @@ static void ion_dma_buf_detatch(struct dma_buf *dmabuf,
  {
struct ion_dma_buf_attachment *a = attachment->priv;
struct ion_buffer *buffer = dmabuf->priv;
+   struct sg_table *table;
+
+   if (!a)
+   return;
+
+   table = a->table;
+   if (table) {
+   if (a->dir != DMA_NONE)
+   dma_unmap_sg(attachment->dev, table->sgl, table->nents,
+a->dir);
+   sg_free_table(table);
+   }
  
  	free_duped_table(a->table);

mutex_lock(>lock);
@@ -243,6 +257,7 @@ static void ion_dma_buf_detatch(struct dma_buf *dmabuf,
mutex_unlock(>lock);
  
  	kfree(a);

+   attachment->priv = NULL;
  }
  
  static struct sg_table *ion_map_dma_buf(struct dma_buf_attachment *attachment,

@@ -251,12 +266,24 @@ static struct sg_table *ion_map_dma_buf(struct 
dma_buf_attachment *attachment,
struct ion_dma_buf_attachment *a = attachment->priv;
struct sg_table *table;
  
-	table = a->table;

+   if (WARN_ON(direction == DMA_NONE || !a))
+   return ERR_PTR(-EINVAL);
  
-	if (!dma_map_sg(attachment->dev, table->sgl, table->nents,

-   direction))
-   return ERR_PTR(-ENOMEM);
+   if (a->dir == direction)
+   return a->table;
  
+	if (WARN_ON(a->dir != DMA_NONE))

+   return ERR_PTR(-EBUSY);
+
+   table = a->table;
+   if (!IS_ERR(table)) {
+   if (!dma_map_sg(attachment->dev, table->sgl, table->nents,
+   direction)) {
+   table = ERR_PTR(-ENOMEM);
+   } else {
+   a->dir = direction;
+   }
+   }
return table;
  }
  
@@ -264,7 +291,6 @@ static void ion_unm

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);
}
  



___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-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

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-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
*





___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-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>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-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
   *



___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCHv3] drm/amdkfd: Remove vla

[Laura Abbott]

There's an ongoing effort to remove VLAs[1] from the kernel to eventually
turn on -Wvla. Switch to a constant value that covers all hardware.

[1] https://lkml.org/lkml/2018/3/7/621

Reviewed-by: Felix Kuehling <felix.kuehl...@amd.com>
Acked-by: Christian König <christian.koe...@amd.com>
Signed-off-by: Laura Abbott <labb...@redhat.com>
---
v3: Introduced a #define for the max value, switched to pr_err_once to
avoid log flood, switched to sizeof(array) per private suggestion.
---
 drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c | 8 +---
 drivers/gpu/drm/amd/amdkfd/kfd_priv.h  | 2 ++
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
index 035c351f47c5..db6d9336b80d 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
@@ -139,10 +139,12 @@ static void interrupt_wq(struct work_struct *work)
 {
struct kfd_dev *dev = container_of(work, struct kfd_dev,
interrupt_work);
+   uint32_t ih_ring_entry[KFD_MAX_RING_ENTRY_SIZE];
 
-   uint32_t ih_ring_entry[DIV_ROUND_UP(
-   dev->device_info->ih_ring_entry_size,
-   sizeof(uint32_t))];
+   if (dev->device_info->ih_ring_entry_size > sizeof(ih_ring_entry)) {
+   dev_err_once(kfd_chardev(), "Ring entry too small\n");
+   return;
+   }
 
while (dequeue_ih_ring_entry(dev, ih_ring_entry))
dev->device_info->event_interrupt_class->interrupt_wq(dev,
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h 
b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
index 96a9cc0f02c9..a90db05dfe61 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
@@ -39,6 +39,8 @@
 
 #include "amd_shared.h"
 
+#define KFD_MAX_RING_ENTRY_SIZE8
+
 #define KFD_SYSFS_FILE_MODE 0444
 
 #define KFD_MMAP_DOORBELL_MASK 0x8ull
-- 
2.14.3

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCHv2] drm/i2c: tda998x: Remove VLA usage

[Laura Abbott]

There's an ongoing effort to remove VLAs[1] from the kernel to eventually
turn on -Wvla. The vla in reg_write_range is based on the length of data
passed. The one use of a non-constant size for this range is bounded by
the size buffer passed to hdmi_infoframe_pack which is a fixed size.
Switch to this upper bound.

[1] https://lkml.org/lkml/2018/3/7/621

Signed-off-by: Laura Abbott <labb...@redhat.com>
---
v2: Switch to make the buffer size more transparent and add a bounds
check.
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 13 +++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c 
b/drivers/gpu/drm/i2c/tda998x_drv.c
index 9e67a7b4e3a4..c8b6029b7839 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -466,13 +466,22 @@ reg_read_range(struct tda998x_priv *priv, u16 reg, char 
*buf, int cnt)
return ret;
 }
 
+#define MAX_WRITE_RANGE_BUF 32
+
 static void
 reg_write_range(struct tda998x_priv *priv, u16 reg, u8 *p, int cnt)
 {
struct i2c_client *client = priv->hdmi;
-   u8 buf[cnt+1];
+   /* This is the maximum size of the buffer passed in */
+   u8 buf[MAX_WRITE_RANGE_BUF + 1];
int ret;
 
+   if (cnt > MAX_WRITE_RANGE_BUF) {
+   dev_err(>dev, "Fixed write buffer too small (%d)\n",
+   MAX_WRITE_RANGE_BUF);
+   return;
+   }
+
buf[0] = REG2ADDR(reg);
memcpy([1], p, cnt);
 
@@ -679,7 +688,7 @@ static void
 tda998x_write_if(struct tda998x_priv *priv, u8 bit, u16 addr,
 union hdmi_infoframe *frame)
 {
-   u8 buf[32];
+   u8 buf[MAX_WRITE_RANGE_BUF];
ssize_t len;
 
len = hdmi_infoframe_pack(frame, buf, sizeof(buf));
-- 
2.14.3

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCHv2] drm/amdkfd: Remove vla

[Laura Abbott]

There's an ongoing effort to remove VLAs[1] from the kernel to eventually
turn on -Wvla. Switch to a constant value that covers all hardware.

[1] https://lkml.org/lkml/2018/3/7/621

Signed-off-by: Laura Abbott <labb...@redhat.com>
---
v2: Switch to a larger size to account for other hardware
---
 drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
index 035c351f47c5..c3a5a80e31ae 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
@@ -139,10 +139,12 @@ static void interrupt_wq(struct work_struct *work)
 {
struct kfd_dev *dev = container_of(work, struct kfd_dev,
interrupt_work);
+   uint32_t ih_ring_entry[8];
 
-   uint32_t ih_ring_entry[DIV_ROUND_UP(
-   dev->device_info->ih_ring_entry_size,
-   sizeof(uint32_t))];
+   if (dev->device_info->ih_ring_entry_size > (8 * sizeof(uint32_t))) {
+   dev_err(kfd_chardev(), "Ring entry too small\n");
+   return;
+   }
 
while (dequeue_ih_ring_entry(dev, ih_ring_entry))
dev->device_info->event_interrupt_class->interrupt_wq(dev,
-- 
2.14.3

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/i2c: tda998x: Remove VLA usage

[Laura Abbott]

On 04/09/2018 03:21 PM, Russell King - ARM Linux wrote:

On Mon, Apr 09, 2018 at 02:07:03PM -0700, Laura Abbott wrote:

There's an ongoing effort to remove VLAs[1] from the kernel to eventually
turn on -Wvla. The vla in reg_write_range is based on the length of data
passed. The one use of a non-constant size for this range is bounded by
the size buffer passed to hdmi_infoframe_pack which is a fixed size.
Switch to this upper bound.


Does this _really_ make it safer?  What if the code is modified to write
more than 32 bytes in the future?

Sorry, I don't think this is safer at all.



Yeah I wasn't 100% sure about this one. Elsewhere, we've added bounds
checks against the new static size buffer so we could do that here
to ensure we don't overrun the stack if we do need to write more
than 32 bytes in the future. Another option is to switch to
a kmalloc buffer. Are either of those options acceptable to you or
do you have a better idea of how to get rid of the VLA?

Thanks,
Laura
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/amdkfd: Remove vla

[Laura Abbott]

On 04/09/2018 11:38 PM, Christian König wrote:

Am 09.04.2018 um 23:06 schrieb Laura Abbott:

There's an ongoing effort to remove VLAs[1] from the kernel to eventually
turn on -Wvla. The single VLA usage in the amdkfd driver is actually
constant across all current platforms.


Actually that isn't correct.

Could be that we haven't upstreamed KFD support for them, but Vega10 have a 
different interrupt ring entry size and so would cause the error message here.


Switch to a constant size array
instead.


I would say to just make make the array bigger.

Regards,
Christian.



What array size would accommodate future chips?



[1] https://lkml.org/lkml/2018/3/7/621

Signed-off-by: Laura Abbott <labb...@redhat.com>
---
  drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c | 8 +---
  1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
index 035c351f47c5..c9863858f343 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
@@ -139,10 +139,12 @@ static void interrupt_wq(struct work_struct *work)
  {
  struct kfd_dev *dev = container_of(work, struct kfd_dev,
  interrupt_work);
+    uint32_t ih_ring_entry[4];
-    uint32_t ih_ring_entry[DIV_ROUND_UP(
-    dev->device_info->ih_ring_entry_size,
-    sizeof(uint32_t))];
+    if (dev->device_info->ih_ring_entry_size > (4 * sizeof(uint32_t))) {
+    dev_err(kfd_chardev(), "Ring entry too small\n");
+    return;
+    }
  while (dequeue_ih_ring_entry(dev, ih_ring_entry))
  dev->device_info->event_interrupt_class->interrupt_wq(dev,




___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/i2c: tda998x: Remove VLA usage

[Laura Abbott]

There's an ongoing effort to remove VLAs[1] from the kernel to eventually
turn on -Wvla. The vla in reg_write_range is based on the length of data
passed. The one use of a non-constant size for this range is bounded by
the size buffer passed to hdmi_infoframe_pack which is a fixed size.
Switch to this upper bound.

[1] https://lkml.org/lkml/2018/3/7/621

Signed-off-by: Laura Abbott <labb...@redhat.com>
---
This one really feels like it should be a #define but I wasn't sure
where the 32 came from. It looks like most other uses use one of the
#defines in include/linux/hdmi?
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c 
b/drivers/gpu/drm/i2c/tda998x_drv.c
index 9e67a7b4e3a4..29e2f49601c7 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -470,7 +470,8 @@ static void
 reg_write_range(struct tda998x_priv *priv, u16 reg, u8 *p, int cnt)
 {
struct i2c_client *client = priv->hdmi;
-   u8 buf[cnt+1];
+   /* This is the maximum size of the buffer passed in */
+   u8 buf[33];
int ret;
 
buf[0] = REG2ADDR(reg);
-- 
2.14.3

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/gma500: Remove VLA

[Laura Abbott]

There's an ongoing effort to remove VLAs[1] from the kernel to eventually
turn on -Wvla. Switch to a reasonable upper bound for the VLAs in
the gma500 driver.

[1] https://lkml.org/lkml/2018/3/7/621

Signed-off-by: Laura Abbott <labb...@redhat.com>
---
This was a little hard to figure out but I think 32 should be a
comfortable upper bound based on all the structures I saw. Of course I
can't test it.
---
 drivers/gpu/drm/gma500/psb_intel_sdvo.c | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/gma500/psb_intel_sdvo.c 
b/drivers/gpu/drm/gma500/psb_intel_sdvo.c
index 84507912be84..3d4fa9f6b94c 100644
--- a/drivers/gpu/drm/gma500/psb_intel_sdvo.c
+++ b/drivers/gpu/drm/gma500/psb_intel_sdvo.c
@@ -429,13 +429,20 @@ static const char *cmd_status_names[] = {
"Scaling not supported"
 };
 
+#define MAX_ARG_LEN 32
+
 static bool psb_intel_sdvo_write_cmd(struct psb_intel_sdvo *psb_intel_sdvo, u8 
cmd,
 const void *args, int args_len)
 {
-   u8 buf[args_len*2 + 2], status;
-   struct i2c_msg msgs[args_len + 3];
+   u8 buf[MAX_ARG_LEN*2 + 2], status;
+   struct i2c_msg msgs[MAX_ARG_LEN + 3];
int i, ret;
 
+   if (args_len > MAX_ARG_LEN) {
+   DRM_ERROR("Need to increase arg length\n");
+   return false;
+   }
+
psb_intel_sdvo_debug_write(psb_intel_sdvo, cmd, args, args_len);
 
for (i = 0; i < args_len; i++) {
-- 
2.14.3

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/amdkfd: Remove vla

[Laura Abbott]

There's an ongoing effort to remove VLAs[1] from the kernel to eventually
turn on -Wvla. The single VLA usage in the amdkfd driver is actually
constant across all current platforms. Switch to a constant size array
instead.

[1] https://lkml.org/lkml/2018/3/7/621

Signed-off-by: Laura Abbott <labb...@redhat.com>
---
 drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
index 035c351f47c5..c9863858f343 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
@@ -139,10 +139,12 @@ static void interrupt_wq(struct work_struct *work)
 {
struct kfd_dev *dev = container_of(work, struct kfd_dev,
interrupt_work);
+   uint32_t ih_ring_entry[4];
 
-   uint32_t ih_ring_entry[DIV_ROUND_UP(
-   dev->device_info->ih_ring_entry_size,
-   sizeof(uint32_t))];
+   if (dev->device_info->ih_ring_entry_size > (4 * sizeof(uint32_t))) {
+   dev_err(kfd_chardev(), "Ring entry too small\n");
+   return;
+   }
 
while (dequeue_ih_ring_entry(dev, ih_ring_entry))
dev->device_info->event_interrupt_class->interrupt_wq(dev,
-- 
2.14.3

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4] drm/i915/dp: Send DPCD ON for MST before phy_up

[Laura Abbott]

On 04/06/2018 11:52 AM, Lyude Paul wrote:

When doing a modeset where the sink is transitioning from D3 to D0 , it
would sometimes be possible for the initial power_up_phy() to start
timing out. This would only be observed in the last action before the
sink went into D3 mode was intel_dp_sink_dpms(DRM_MODE_DPMS_OFF). We
originally thought this might be an issue with us accidentally shutting
off the aux block when putting the sink into D3, but since the DP spec
mandates that sinks must wake up within 1ms while we have 100ms to
respond to an ESI irq, this didn't really add up. Turns out that the
problem is more subtle then that:

It turns out that the timeout is from us not enabling DPMS on the MST
hub before actually trying to initiate sideband communications. This
would cause the first sideband communication (power_up_phy()), to start
timing out because the sink wasn't ready to respond. Afterwards, we
would call intel_dp_sink_dpms(DRM_MODE_DPMS_ON) in
intel_ddi_pre_enable_dp(), which would actually result in waking up the
sink so that sideband requests would work again.

Since DPMS is what lets us actually bring the hub up into a state where
sideband communications become functional again, we just need to make
sure to enable DPMS on the display before attempting to perform sideband
communications.

Changes since v1:
- Remove comment above if (!intel_dp->is_mst) - vsryjala
- Move intel_dp_sink_dpms() for MST into intel_dp_post_disable_mst() to
   keep enable/disable paths symmetrical
- Improve commit message - dhnkrn
Changes since v2:
- Only send DPMS off when we're disabling the last sink, and only send
   DPMS on when we're enabling the first sink - dhnkrn
Changes since v3:
- Check against is_mst, not intel_dp->is_mst - dhnkrn/vsyrjala



For the booting docked with lid down case:

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


Signed-off-by: Lyude Paul <ly...@redhat.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandi...@intel.com>
Cc: Ville Syrjälä <ville.syrj...@linux.intel.com>
Cc: Laura Abbott <labb...@redhat.com>
Cc: sta...@vger.kernel.org
Fixes: ad260ab32a4d9 ("drm/i915/dp: Write to SET_POWER dpcd to enable MST hub.")
---
  drivers/gpu/drm/i915/intel_ddi.c| 8 ++--
  drivers/gpu/drm/i915/intel_dp_mst.c | 8 +++-
  2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index a6672a9abd85..92cb26b18a9b 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -2324,7 +2324,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder 
*encoder,
intel_prepare_dp_ddi_buffers(encoder, crtc_state);
  
  	intel_ddi_init_dp_buf_reg(encoder);

-   intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
+   if (!is_mst)
+   intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
intel_dp_start_link_train(intel_dp);
if (port != PORT_A || INTEL_GEN(dev_priv) >= 9)
intel_dp_stop_link_train(intel_dp);
@@ -2422,12 +2423,15 @@ static void intel_ddi_post_disable_dp(struct 
intel_encoder *encoder,
struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
struct intel_digital_port *dig_port = enc_to_dig_port(>base);
struct intel_dp *intel_dp = _port->dp;
+   bool is_mst = intel_crtc_has_type(old_crtc_state,
+ INTEL_OUTPUT_DP_MST);
  
  	/*

 * Power down sink before disabling the port, otherwise we end
 * up getting interrupts from the sink on detecting link loss.
 */
-   intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
+   if (!is_mst)
+   intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
  
  	intel_disable_ddi_buf(encoder);
  
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c

index c3de0918ee13..9e6956c08688 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -180,9 +180,11 @@ static void intel_mst_post_disable_dp(struct intel_encoder 
*encoder,
intel_dp->active_mst_links--;
  
  	intel_mst->connector = NULL;

-   if (intel_dp->active_mst_links == 0)
+   if (intel_dp->active_mst_links == 0) {
+   intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
intel_dig_port->base.post_disable(_dig_port->base,
  old_crtc_state, NULL);
+   }
  
  	DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links);

  }
@@ -223,7 +225,11 @@ static void intel_mst_pre_enable_dp(struct intel_encoder 
*encoder,
  
  	DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links);
  
+	if (intel_dp->active_mst_links == 0)

+   intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
+
drm_dp_send_power_updown_phy(_dp->mst_mgr, connector->port, true);

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-devel@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-devel@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



___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/i915: Keep AUX block running when disabling DPMS for MST

[Laura Abbott]

On 04/02/2018 02:26 PM, Lyude Paul wrote:

While enabling/disabling DPMS before link training with MST hubs is
perfectly valid; unfortunately disabling DPMS results in some devices
disabling their AUX CH block as well. For SST this isn't as much of a
problem, but for MST we need to be able to continue handling aux
transactions even when none of the sinks are turned on since it's
possible for us to have a single atomic commit which results in
disabling each downstream sink, followed by subsequently re-enabling
each sink.

If we don't do this, we'll end up stalling any pending ESI interrupts
from the sink for up to 1ms. Unfortunately, dropping ESIs during this
timespan makes it so that link fallback retraining for MST (which I will
be submitting to the ML shortly) fails due to the channel EQ failure
interrupts potentially getting dropped. Additionally, when performing a
modeset that brings the hub status's link status from bad -> good having
ESIs disabled for that long causes us to miss the hub's response to us
trying to start link training as well.

Since any sink with MST is going to support DisplayPort 1.2 anyway, save
us the hassle of trying to wait until the sink comes back up and just
never shut the aux block down.

Changes since v2:
  - Fix patch name, no functional changes

Signed-off-by: Lyude Paul <ly...@redhat.com>
Cc: Laura Abbott <labb...@redhat.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandi...@intel.com>
Cc: Ville Syrjälä <ville.syrj...@linux.intel.com>
Cc: sta...@vger.kernel.org
Fixes: ad260ab32a4d9 ("drm/i915/dp: Write to SET_POWER dpcd to enable MST hub.")


Still able to boot docked and lid closed so

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


---
  drivers/gpu/drm/i915/intel_dp.c | 6 --
  1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 62f82c4298ac..0479c377981b 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2589,11 +2589,13 @@ void intel_dp_sink_dpms(struct intel_dp *intel_dp, int 
mode)
return;
  
  	if (mode != DRM_MODE_DPMS_ON) {

+   unsigned char data = intel_dp->is_mst ?
+   DP_SET_POWER_D3_AUX_ON : DP_SET_POWER_D3;
+
if (downstream_hpd_needs_d0(intel_dp))
return;
  
-		ret = drm_dp_dpcd_writeb(_dp->aux, DP_SET_POWER,

-DP_SET_POWER_D3);
+   ret = drm_dp_dpcd_writeb(_dp->aux, DP_SET_POWER, data);
} else {
struct intel_lspcon *lspcon = dp_to_lspcon(intel_dp);
  



___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-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

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[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

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-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

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

[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:
+   

[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

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[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

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] staging: android: ion: Fix dma direction for dma_sync_sg_for_cpu/device

[Laura Abbott]

On 12/15/2017 12:59 PM, Sushmita Susheelendra wrote:

Use the direction argument passed into begin_cpu_access
and end_cpu_access when calling the dma_sync_sg_for_cpu/device.
The actual cache primitive called depends on the direction
passed in.



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


Signed-off-by: Sushmita Susheelendra <ssush...@codeaurora.org>
---
  drivers/staging/android/ion/ion.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/android/ion/ion.c 
b/drivers/staging/android/ion/ion.c
index a7d9b0e..f480885 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -346,7 +346,7 @@ 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,
-   DMA_BIDIRECTIONAL);
+   direction);
}
mutex_unlock(>lock);
  
@@ -368,7 +368,7 @@ 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,
-  DMA_BIDIRECTIONAL);
+  direction);
}
mutex_unlock(>lock);
  



___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v6 2/2] staging: ion: create one device entry per heap

[Laura Abbott]

On 12/02/2017 07:53 AM, Greg KH wrote:

This is one of the item in the TODO list before been able to unstage ION
which is my real need.

Why does it matter where in the tree this code is?  Don't go adding new
things to it that are not needed.  Who needs this?  What userspace code
wants this type of multiple ion devices?



Requirements came in from several places to split /dev/ion -> /dev/ion0
and /dev/ion1 so that security policy (i.e. selinux) could be used to
protect access to certain heaps. I wanted the ABI to be settled before
trying to move out of staging, hence the line in the TODO list about
doing the split.


thanks,

greg k-h


Thanks,
Laura
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Regression in TTM driver w/Linus' master

[Laura Abbott]

Hi,

Fedora QA testing reported a panic when booting up VMs
using qmeu vga drivers 
(https://paste.fedoraproject.org/paste/498yRWTCJv2LKIrmj4EliQ)

[   30.108507] [ cut here ]
[   30.108920] kernel BUG at ./include/linux/gfp.h:408!
[   30.109356] invalid opcode:  [#1] SMP
[   30.109700] Modules linked in: fuse nf_conntrack_netbios_ns 
nf_conntrack_broadcast xt_CT ip6t_rpfilter ip6t_REJECT nf_reject_ipv6 
xt_conntrack devlink ip_set nfnetlink ebtable_nat ebtable_broute bridge 
ip6table_nat nf_conntrack_ipv6 nf_defrag_ipv6 nf_nat_ipv6 ip6table_mangle 
ip6table_raw ip6table_security iptable_nat nf_conntrack_ipv4 nf_defrag_ipv4 
nf_nat_ipv4 nf_nat nf_conntrack libcrc32c iptable_mangle iptable_raw 
iptable_security ebtable_filter ebtables ip6table_filter ip6_tables 
snd_hda_codec_generic kvm_intel kvm snd_hda_intel snd_hda_codec irqbypass ppdev 
snd_hda_core snd_hwdep snd_seq snd_seq_device snd_pcm bochs_drm ttm joydev 
drm_kms_helper virtio_balloon snd_timer snd parport_pc drm soundcore parport 
i2c_piix4 nls_utf8 isofs squashfs zstd_decompress xxhash 8021q garp mrp stp llc 
virtio_net
[   30.115605]  virtio_console virtio_scsi crct10dif_pclmul crc32_pclmul 
crc32c_intel ghash_clmulni_intel serio_raw virtio_pci virtio_ring virtio 
ata_generic pata_acpi qemu_fw_cfg sunrpc scsi_transport_iscsi loop
[   30.117425] CPU: 0 PID: 1347 Comm: gnome-shell Not tainted 
4.15.0-0.rc0.git6.1.fc28.x86_64 #1
[   30.118141] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 
1.10.2-2.fc27 04/01/2014
[   30.118866] task: 923a77e03380 task.stack: a78182228000
[   30.119366] RIP: 0010:__alloc_pages_nodemask+0x35e/0x430
[   30.119810] RSP: :a7818222bba8 EFLAGS: 00010202
[   30.120250] RAX: 0001 RBX: 014382c6 RCX: 0006
[   30.120840] RDX:  RSI: 0009 RDI: 
[   30.121443] RBP: 923a760d6000 R08:  R09: 0006
[   30.122039] R10: 0040 R11: 0300 R12: 923a729273c0
[   30.122629] R13:  R14:  R15: 923a7483d400
[   30.123223] FS:  7fe48da7dac0() GS:923a7cc0() 
knlGS:
[   30.123896] CS:  0010 DS:  ES:  CR0: 80050033
[   30.124373] CR2: 7fe457b73000 CR3: 78313000 CR4: 06f0
[   30.124968] Call Trace:
[   30.125186]  ttm_pool_populate+0x19b/0x400 [ttm]
[   30.125578]  ttm_bo_vm_fault+0x325/0x570 [ttm]
[   30.125964]  __do_fault+0x19/0x11e
[   30.126255]  __handle_mm_fault+0xcd3/0x1260
[   30.126609]  handle_mm_fault+0x14c/0x310
[   30.126947]  __do_page_fault+0x28c/0x530
[   30.127282]  do_page_fault+0x32/0x270
[   30.127593]  async_page_fault+0x22/0x30
[   30.127922] RIP: 0033:0x7fe48aae39a8
[   30.128225] RSP: 002b:7ffc21c4d928 EFLAGS: 00010206
[   30.128664] RAX: 7fe457b73000 RBX: 55cd4c1041a0 RCX: 7fe457b73040
[   30.129259] RDX: 0030 RSI:  RDI: 7fe457b73000
[   30.129855] RBP: 0300 R08: 000c R09: 0001
[   30.130457] R10: 0001 R11: 0246 R12: 55cd4c1041a0
[   30.131054] R13: 55cd4bdfe990 R14: 55cd4c104110 R15: 0400
[   30.131648] Code: 11 01 00 0f 84 a9 00 00 00 65 ff 0d 6d cc dd 44 e9 0f ff ff ff 
40 80 cd 80 e9 99 fe ff ff 48 89 c7 e8 e7 f6 01 00 e9 b7 fe ff ff <0f> 0b 0f ff 
e9 40 fd ff ff 65 48 8b 04 25 80 d5 00 00 8b 40 4c
[   30.133245] RIP: __alloc_pages_nodemask+0x35e/0x430 RSP: a7818222bba8
[   30.133836] ---[ end trace d4f1deb60784f40a ]---

This is based off of Linus' master branch at 
c8a0739b185d11d6e2ca7ad9f5835841d1cfc765
Configs are at 
https://git.kernel.org/pub/scm/linux/kernel/git/jwboyer/fedora.git/commit/?h=rawhide=0be14662c54f49b4e640868b9d67df18d39edff0


Thanks,
Laura
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v6 2/2] staging: ion: create one device entry per heap

[Laura Abbott]

On 11/06/2017 07:59 AM, Benjamin Gaignard wrote:

Instead a getting only one common device "/dev/ion" for
all the heaps this patch allow to create one device
entry ("/dev/ionX") per heap.
Getting an entry per heap could allow to set security rules
per heap and global ones for all heaps.

Allocation requests will be only allowed if the mask_id
match with device minor.
Query request could be done on any of the devices.



With this patch, sysfs looks like:

$ ls /sys/devices/
breakpoint ion platform software system virtual

From an Ion perspective, you can have

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

Another Ack for the device model stuff would be good but I'll
assume deafening silence means nobody hates it.

Thanks,
Laura


Signed-off-by: Benjamin Gaignard <benjamin.gaign...@linaro.org>
---
  drivers/staging/android/TODO|  1 -
  drivers/staging/android/ion/Kconfig |  7 
  drivers/staging/android/ion/ion-ioctl.c | 18 --
  drivers/staging/android/ion/ion.c   | 62 +
  drivers/staging/android/ion/ion.h   | 15 ++--
  5 files changed, 91 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/android/TODO b/drivers/staging/android/TODO
index 687e0ea..8a11931 100644
--- a/drivers/staging/android/TODO
+++ b/drivers/staging/android/TODO
@@ -8,7 +8,6 @@ TODO:
  ion/
   - Add dt-bindings for remaining heaps (chunk and carveout heaps). This would
 involve putting appropriate bindings in a memory node for Ion to find.
- - Split /dev/ion up into multiple nodes (e.g. /dev/ion/heap0)
   - Better test framework (integration with VGEM was suggested)
  
  Please send patches to Greg Kroah-Hartman <g...@kroah.com> and Cc:

diff --git a/drivers/staging/android/ion/Kconfig 
b/drivers/staging/android/ion/Kconfig
index a517b2d..cb4666e 100644
--- a/drivers/staging/android/ion/Kconfig
+++ b/drivers/staging/android/ion/Kconfig
@@ -10,6 +10,13 @@ menuconfig ION
  If you're not using Android its probably safe to
  say N here.
  
+config ION_LEGACY_DEVICE_API

+   bool "Keep using Ion legacy misc device API"
+   depends on ION
+   help
+ Choose this option to keep using Ion legacy misc device API
+ i.e. /dev/ion
+
  config ION_SYSTEM_HEAP
bool "Ion system heap"
depends on ION
diff --git a/drivers/staging/android/ion/ion-ioctl.c 
b/drivers/staging/android/ion/ion-ioctl.c
index e26b786..bb5c77b 100644
--- a/drivers/staging/android/ion/ion-ioctl.c
+++ b/drivers/staging/android/ion/ion-ioctl.c
@@ -25,7 +25,8 @@ union ion_ioctl_arg {
struct ion_heap_query query;
  };
  
-static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)

+static int validate_ioctl_arg(struct file *filp,
+ unsigned int cmd, union ion_ioctl_arg *arg)
  {
switch (cmd) {
case ION_IOC_HEAP_QUERY:
@@ -34,6 +35,19 @@ static int validate_ioctl_arg(unsigned int cmd, union 
ion_ioctl_arg *arg)
arg->query.reserved2 )
return -EINVAL;
break;
+
+   case ION_IOC_ALLOC:
+   {
+   int mask = 1 << iminor(filp->f_inode);
+
+#ifdef CONFIG_ION_LEGACY_DEVICE_API
+   if (imajor(filp->f_inode) == MISC_MAJOR)
+   return 0;
+#endif
+   if (!(arg->allocation.heap_id_mask & mask))
+   return -EINVAL;
+   break;
+   }
default:
break;
}
@@ -69,7 +83,7 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned 
long arg)
if (copy_from_user(, (void __user *)arg, _IOC_SIZE(cmd)))
return -EFAULT;
  
-	ret = validate_ioctl_arg(cmd, );

+   ret = validate_ioctl_arg(filp, cmd, );
if (WARN_ON_ONCE(ret))
return ret;
  
diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c

index fda9756..2c2568b 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -40,6 +40,9 @@
  
  #include "ion.h"
  
+#define ION_DEV_MAX 32

+#define ION_NAME "ion"
+
  static struct ion_device *internal_dev;
  static int heap_id;
  
@@ -535,15 +538,38 @@ static int debug_shrink_get(void *data, u64 *val)

  DEFINE_SIMPLE_ATTRIBUTE(debug_shrink_fops, debug_shrink_get,
debug_shrink_set, "%llu\n");
  
-void ion_device_add_heap(struct ion_heap *heap)

+static struct device ion_bus = {
+   .init_name = ION_NAME,
+};
+
+static struct bus_type ion_bus_type = {
+   .name = ION_NAME,
+};
+
+int ion_device_add_heap(struct ion_heap *heap)
  {
struct dentry *debug_file;
struct ion_device *dev = internal_dev;
+   int ret = 0;
  
  	if (!heap->ops->allocate || !heap->ops->free)

pr_err("%s: can not add heap with invalid ops struct.\n&q

Re: [PATCH v6 1/2] staging: ion: reorder include

[Laura Abbott]

On 11/06/2017 07:59 AM, Benjamin Gaignard wrote:

Put include in alphabetic order



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


Signed-off-by: Benjamin Gaignard <benjamin.gaign...@linaro.org>
---
  drivers/staging/android/ion/ion.c | 14 +++---
  1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/android/ion/ion.c 
b/drivers/staging/android/ion/ion.c
index a7d9b0e..fda9756 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -15,28 +15,28 @@
   *
   */
  
+#include 

+#include 
  #include 
+#include 
  #include 
+#include 
  #include 
  #include 
  #include 
-#include 
+#include 
  #include 
  #include 
  #include 
  #include 
-#include 
  #include 
  #include 
  #include 
-#include 
+#include 
  #include 
+#include 
  #include 
  #include 
-#include 
-#include 
-#include 
-#include 
  
  #include "ion.h"
  



___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v6 2/2] staging: ion: create one device entry per heap

[Laura Abbott]

On 10/31/2017 12:11 PM, Mark Brown wrote:
> On Tue, Oct 31, 2017 at 12:03:35PM -0700, Laura Abbott wrote:
> 
>> I'm not a fan of the platform bus but I have mixed feelings about
>> creating a dedicated bus type. I guess if we really need a bus
>> type we can do it later?
> 
> There was a discussion a while ago in the context of I2C/SPI MFDs
> which concluded that if you need a bus and it's going to be effectively
> noop then you should just use the platform bus as anything else will
> consist almost entirely of cut'n'paste from the platform bus with some
> light sed usage and code duplication is bad.  It's not super lovely as
> it's not actually a memory mapped device but it's the best idea we've
> got.
> 

Thanks for the pointer.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v6 2/2] staging: ion: create one device entry per heap

[Laura Abbott]

On 10/23/2017 08:55 AM, Benjamin Gaignard wrote:
> Instead a getting only one common device "/dev/ion" for
> all the heaps this patch allow to create one device
> entry ("/dev/ionX") per heap.
> Getting an entry per heap could allow to set security rules
> per heap and global ones for all heaps.
> 
> Allocation requests will be only allowed if the mask_id
> match with device minor.
> Query request could be done on any of the devices.
> 

I'm wondering if we should always keep /dev/ion for the query
ioctl and just disallow allocation from /dev/ion. I guess
running the query ioctl on /dev/ion0 always wouldn't be too
bad? Anyone else have strong opinions?

> Ion devices are parentless so it is need to add platform_bus as
> parent and platform_bus_type as bus to be put in /sys/device/paltform.
> Those two parameters need platform_device.h to be included but 
> include files weren't in alphabetic order so I reorder them correctly.
> 

I'm not a fan of the platform bus but I have mixed feelings about
creating a dedicated bus type. I guess if we really need a bus
type we can do it later?


Thanks,
Laura

> Signed-off-by: Benjamin Gaignard 
> ---
>  drivers/staging/android/TODO|  1 -
>  drivers/staging/android/ion/Kconfig |  7 +
>  drivers/staging/android/ion/ion-ioctl.c | 18 +--
>  drivers/staging/android/ion/ion.c   | 53 
> ++---
>  drivers/staging/android/ion/ion.h   | 15 --
>  5 files changed, 79 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/staging/android/TODO b/drivers/staging/android/TODO
> index 5f14247..d770ffa 100644
> --- a/drivers/staging/android/TODO
> +++ b/drivers/staging/android/TODO
> @@ -9,7 +9,6 @@ TODO:
>  ion/
>   - Add dt-bindings for remaining heaps (chunk and carveout heaps). This would
> involve putting appropriate bindings in a memory node for Ion to find.
> - - Split /dev/ion up into multiple nodes (e.g. /dev/ion/heap0)
>   - Better test framework (integration with VGEM was suggested)
>  
>  Please send patches to Greg Kroah-Hartman  and Cc:
> diff --git a/drivers/staging/android/ion/Kconfig 
> b/drivers/staging/android/ion/Kconfig
> index a517b2d..cb4666e 100644
> --- a/drivers/staging/android/ion/Kconfig
> +++ b/drivers/staging/android/ion/Kconfig
> @@ -10,6 +10,13 @@ menuconfig ION
> If you're not using Android its probably safe to
> say N here.
>  
> +config ION_LEGACY_DEVICE_API
> + bool "Keep using Ion legacy misc device API"
> + depends on ION
> + help
> +   Choose this option to keep using Ion legacy misc device API
> +   i.e. /dev/ion
> +
>  config ION_SYSTEM_HEAP
>   bool "Ion system heap"
>   depends on ION
> diff --git a/drivers/staging/android/ion/ion-ioctl.c 
> b/drivers/staging/android/ion/ion-ioctl.c
> index e26b786..bb5c77b 100644
> --- a/drivers/staging/android/ion/ion-ioctl.c
> +++ b/drivers/staging/android/ion/ion-ioctl.c
> @@ -25,7 +25,8 @@ union ion_ioctl_arg {
>   struct ion_heap_query query;
>  };
>  
> -static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
> +static int validate_ioctl_arg(struct file *filp,
> +   unsigned int cmd, union ion_ioctl_arg *arg)
>  {
>   switch (cmd) {
>   case ION_IOC_HEAP_QUERY:
> @@ -34,6 +35,19 @@ static int validate_ioctl_arg(unsigned int cmd, union 
> ion_ioctl_arg *arg)
>   arg->query.reserved2 )
>   return -EINVAL;
>   break;
> +
> + case ION_IOC_ALLOC:
> + {
> + int mask = 1 << iminor(filp->f_inode);
> +
> +#ifdef CONFIG_ION_LEGACY_DEVICE_API
> + if (imajor(filp->f_inode) == MISC_MAJOR)
> + return 0;
> +#endif
> + if (!(arg->allocation.heap_id_mask & mask))
> + return -EINVAL;
> + break;
> + }
>   default:
>   break;
>   }
> @@ -69,7 +83,7 @@ long ion_ioctl(struct file *filp, unsigned int cmd, 
> unsigned long arg)
>   if (copy_from_user(, (void __user *)arg, _IOC_SIZE(cmd)))
>   return -EFAULT;
>  
> - ret = validate_ioctl_arg(cmd, );
> + ret = validate_ioctl_arg(filp, cmd, );
>   if (WARN_ON_ONCE(ret))
>   return ret;
>  
> diff --git a/drivers/staging/android/ion/ion.c 
> b/drivers/staging/android/ion/ion.c
> index 93e2c90..dd66f55 100644
> --- a/drivers/staging/android/ion/ion.c
> +++ b/drivers/staging/android/ion/ion.c
> @@ -15,31 +15,35 @@
>   *
>   */
>  
> +#include 
> +#include 
>  #include 
> +#include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> -#include 
> +#include 
>  #include 
>  #include 
>  #include 
>  #include 
> -#include 
>  #include 
>  #include 
> +#include 
>  #include 
> -#include 
> +#include 
>  #include 
> +#include 
>  #include 
>  #include 
> -#include 
> -#include 
> -#include 
> -#include 
>  
>  #include "ion.h"
>  
> +#define 

Re: [PATCH v5 2/2] staging: ion: create one device entry per heap

[Laura Abbott]

On 09/27/2017 06:20 AM, Benjamin Gaignard wrote:
> diff --git a/drivers/staging/android/ion/ion.c 
> b/drivers/staging/android/ion/ion.c
> index 93e2c90..092b24c 100644
> --- a/drivers/staging/android/ion/ion.c
> +++ b/drivers/staging/android/ion/ion.c
> @@ -40,6 +40,8 @@
>  
>  #include "ion.h"
>  
> +#define ION_DEV_MAX 32
> +
>  static struct ion_device *internal_dev;
>  static int heap_id;
>  
> @@ -537,15 +539,28 @@ static int debug_shrink_get(void *data, u64 *val)
>  DEFINE_SIMPLE_ATTRIBUTE(debug_shrink_fops, debug_shrink_get,
>   debug_shrink_set, "%llu\n");
>  
> -void ion_device_add_heap(struct ion_heap *heap)
> +int ion_device_add_heap(struct ion_heap *heap)
>  {
>   struct dentry *debug_file;
>   struct ion_device *dev = internal_dev;
> + int ret = 0;
>  
>   if (!heap->ops->allocate || !heap->ops->free)
>   pr_err("%s: can not add heap with invalid ops struct.\n",
>  __func__);
>  
> + if (heap_id >= ION_DEV_MAX)
> + return -EBUSY;
> +
> + heap->ddev.devt = MKDEV(MAJOR(dev->devt), heap_id);
> + dev_set_name(>ddev, "ion%d", heap_id);
> + device_initialize(>ddev);
> + cdev_init(>chrdev, _fops);
> + heap->chrdev.owner = THIS_MODULE;
> + ret = cdev_device_add(>chrdev, >ddev);
> + if (ret < 0)
> + return ret;
> +
>   spin_lock_init(>free_lock);
>   heap->free_list_size = 0;
>  
> @@ -583,6 +598,8 @@ void ion_device_add_heap(struct ion_heap *heap)
>  
>   dev->heap_cnt++;
>   up_write(>lock);
> +
> + return ret;
>  }
>  EXPORT_SYMBOL(ion_device_add_heap);
>  
> @@ -595,6 +612,7 @@ static int ion_device_create(void)
>   if (!idev)
>   return -ENOMEM;
>  
> +#ifdef CONFIG_ION_LEGACY_DEVICE_API
>   idev->dev.minor = MISC_DYNAMIC_MINOR;
>   idev->dev.name = "ion";
>   idev->dev.fops = _fops;
> @@ -605,6 +623,17 @@ static int ion_device_create(void)
>   kfree(idev);
>   return ret;
>   }
> +#endif
> +
> + ret = alloc_chrdev_region(>devt, 0, ION_DEV_MAX, "ion");
> + if (ret) {
> + pr_err("ion: unable to allocate device\n");
> +#ifdef CONFIG_ION_LEGACY_DEVICE_API
> + misc_deregister(>dev);
> +#endif
> + kfree(idev);
> + return ret;
> + }
>  
>   idev->debug_root = debugfs_create_dir("ion", NULL);
>   if (!idev->debug_root) {

I'm not 100% sure about the device hierarchy here. We're
ending up with devices at the root of /sys/devices

/sys/devices # ls
breakpoint ion0 ion1 ion2 platform software system virtual 

and the Android init system doesn't pick this up. I'll
admit to being out of my area here but I don't think
this looks quite right.

Thanks,
Laura



___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 2/2] staging: ion: create one device entry per heap

[Laura Abbott]

On 10/10/2017 02:11 AM, Mark Brown wrote:
> On Mon, Oct 09, 2017 at 05:10:37PM -0700, Laura Abbott wrote:
>> On 10/09/2017 03:08 PM, Mark Brown wrote:
>>> On Mon, Oct 09, 2017 at 02:25:47PM -0700, Laura Abbott wrote:
> 
>>>> Anyway, to move this forward I think we need to see a proof of concept
>>>> of using selinux to protect access to specific heaps.
> 
>>> Aren't Unix permissions enough with separate files or am I
>>> misunderstanding what you're looking to see a proof of concept for?
> 
>> The goal is to be able to restrict heap access to certain services
>> and selinux groups on Android so straight unix permissions aren't
>> sufficient.
> 
> Oh, there's Android users for this?  The users I was aware of were
> non-Android.  Though even so I'd have thought that given that SELinux is
> a superset of Unix file permissions it ought to be sufficient to be able
> to use them.  I'd been thinking people were suggesting SELinux as a
> replacement for file permissions, using the single file and the greater
> capabilities of SELinux.
> 
Unix file permissions are necessary but not sufficient, they
can be used separately. Mostly what I want to see before
merging this is an example that splitting the Ion heaps provides
more protection than just keeping /dev/ion.

Thanks,
Laura
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 2/2] staging: ion: create one device entry per heap

[Laura Abbott]

On 10/09/2017 03:08 PM, Mark Brown wrote:
> On Mon, Oct 09, 2017 at 02:25:47PM -0700, Laura Abbott wrote:
> 
>> Anyway, to move this forward I think we need to see a proof of concept
>> of using selinux to protect access to specific heaps.
> 
> Aren't Unix permissions enough with separate files or am I
> misunderstanding what you're looking to see a proof of concept for?
> 

The goal is to be able to restrict heap access to certain services
and selinux groups on Android so straight unix permissions aren't
sufficient.

Thanks,
Laura
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 2/2] staging: ion: create one device entry per heap

[Laura Abbott]

On 10/05/2017 06:06 AM, Benjamin Gaignard wrote:
> 2017-10-04 12:17 GMT+02:00 Mark Brown :
>> On Tue, Oct 03, 2017 at 04:08:30PM -0700, Sandeep Patil wrote:
>>
>>> It is entirely possible and easy in android/ueventd to create those nodes
>>> under "/dev/ion/".  (assuming the heap 'subsystem' for these new devices 
>>> will
>>> point to 'ion').
> 
> I think it is the same problem than for webcam under v4l framework.
> Each time you plug a webcam you got a v4l node but android/uevent rules
> the plug order doesn't have impact.
> The same think will happen for ion nodes it may be even easier because
> the heap will always being created in the smae order for a given product
> configuration.
> 

Relying on the heap being created in the same order seems troublesome.
If for some reason it changes in the kernel we might break something
in userspace.

Anyway, to move this forward I think we need to see a proof of concept
of using selinux to protect access to specific heaps.

Thanks,
Laura

>>
>> The reason I didn't say /dev/ion/foo initially is that if people want to
>> keep the existing /dev/ion around for compatibility reasons then the
>> /dev/ion name isn't available which might cause issues.  Otherwise just
>> dumping everything under a directory (perhaps with a different name) was
>> my first thought as well.
>>
>>> (Also FWIW, the SELinux permissions are also possible with the current ion
>>> implementation by adding rules to disallow specific ioctls instead of adding
>>> permissions to access device node as this change would do)
>>
>> AIUI the request is to limit access to specific heaps, and obviously not
>> everyone wants to deal with SELinux at all.

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 1/2] staging: ion: simplify ioctl args checking function

[Laura Abbott]

On 10/09/2017 02:21 AM, Benjamin Gaignard wrote:
> 2017-09-27 15:20 GMT+02:00 Benjamin Gaignard <benjamin.gaign...@linaro.org>:
>> Make arguments checking more easy to read.
>>
> 
> Hi Laura,
> 
> Even if we don't have found a solution for the second patch I believe
> this one could be useful.
> May I ask you your point of view on those few lines ?
> 
> Benjamin
> 

Yes, this looks better.

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

>> Signed-off-by: Benjamin Gaignard <benjamin.gaign...@linaro.org>
>> ---
>>  drivers/staging/android/ion/ion-ioctl.c | 11 +--
>>  1 file changed, 5 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/staging/android/ion/ion-ioctl.c 
>> b/drivers/staging/android/ion/ion-ioctl.c
>> index d9f8b14..e26b786 100644
>> --- a/drivers/staging/android/ion/ion-ioctl.c
>> +++ b/drivers/staging/android/ion/ion-ioctl.c
>> @@ -27,19 +27,18 @@ union ion_ioctl_arg {
>>
>>  static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
>>  {
>> -   int ret = 0;
>> -
>> switch (cmd) {
>> case ION_IOC_HEAP_QUERY:
>> -   ret = arg->query.reserved0 != 0;
>> -   ret |= arg->query.reserved1 != 0;
>> -   ret |= arg->query.reserved2 != 0;
>> +   if (arg->query.reserved0 ||
>> +   arg->query.reserved1 ||
>> +   arg->query.reserved2)
>> +   return -EINVAL;
>> break;
>> default:
>> break;
>> }
>>
>> -   return ret ? -EINVAL : 0;
>> +   return 0;
>>  }
>>
>>  /* fix up the cases where the ioctl direction bits are incorrect */
>> --
>> 2.7.4
>>
> 
> 
> 

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] cma: Take __GFP_NOWARN into account in cma_alloc()

[Laura Abbott]

On 10/04/2017 05:54 AM, Boris Brezillon wrote:
> cma_alloc() unconditionally prints an INFO message when the CMA
> allocation fails. Make this message conditional on the non-presence of
> __GFP_NOWARN in gfp_mask.
> 
> Signed-off-by: Boris Brezillon <boris.brezil...@free-electrons.com>

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

> ---
> Hello,
> 
> This patch aims at removing INFO messages that are displayed when the
> VC4 driver tries to allocate buffer objects. From the driver perspective
> an allocation failure is acceptable, and the driver can possibly do
> something to make following allocation succeed (like flushing the VC4
> internal cache).
> 
> Also, I don't understand why this message is only an INFO message, and
> not a WARN (pr_warn()). Please let me know if you have good reasons to
> keep it as an unconditional pr_info().
> 
> Thanks,
> 
> Boris
> ---
>  mm/cma.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/cma.c b/mm/cma.c
> index c0da318c020e..022e52bd8370 100644
> --- a/mm/cma.c
> +++ b/mm/cma.c
> @@ -460,7 +460,7 @@ struct page *cma_alloc(struct cma *cma, size_t count, 
> unsigned int align,
>  
>   trace_cma_alloc(pfn, page, count, align);
>  
> - if (ret) {
> + if (ret && !(gfp_mask & __GFP_NOWARN)) {
>   pr_info("%s: alloc failed, req-size: %zu pages, ret: %d\n",
>   __func__, count, ret);
>   cma_debug_show_areas(cma);
> 

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 2/2] staging: ion: create one device entry per heap

[Laura Abbott]

On 10/03/2017 04:08 PM, Sandeep Patil wrote:
> On Tue, Oct 03, 2017 at 02:42:32PM -0700, Laura Abbott wrote:
>> On 10/03/2017 09:48 AM, Mark Brown wrote:
>>> On Mon, Oct 02, 2017 at 11:07:48AM -0700, Laura Abbott wrote:
>>>
>>>> Thinking about this a bit more, I'm not 100% sure if this
>>>> will allow the security rules we want. Heap ids are assigned
>>>> dynamically and therefore so will the /dev/ionX designation.
>>>> From my understanding, security rules like selinux need to
>>>> be fully specified at boot time so I'm not sure how you would
>>>> be able to write rules to differentiate between /dev/ionX and
>>>> /dev/ionY without knowing the values at boottime.
>>>
>>> Isn't this something that should be managable via udev rules that ensure
>>> stable names in the same way as for things like disks or ethernet
>>> controllers (even if it just ends up doing something like /dev/ion-gpu
>>> or whatever)?  If we're not giving it enough information to assign stable
>>> names where needed we probably need to fix that anyway.
>>>
>>
>> Android doesn't use a standard udev so we'd need something that
>> would work there. My understanding was that android needs everything
>> specified at boot unless that's changed.
>>
>> There would be enough information to assign stable names
>> (e.g. /dev/ion-system) if we used the query ioctl to find out
>> what's actually available. Is just the ioctl useful though?
> 
> Wouldn't this new ioctl() to query the heap name also result in special case
> handling of all ion devices in user space?
> 

I'm not quite sure what you are referring to. If you mean we have
to match on the heap name then yes that's going to happen but we
can't just zero knowledge which heap to allocate from and matching
on heap names seemed like the easiest way to make that happen.

> If the devices are named with their corresponding heap names like ion-system, 
> ion-cma etc.
> It is entirely possible and easy in android/ueventd to create those nodes
> under "/dev/ion/".  (assuming the heap 'subsystem' for these new devices will
> point to 'ion').
> 
> Something like the following should work if added in ueventd.rc
> 
>   subsystem ion
> devname uevent_devname
> dirname /dev/ion
> 
> Also, makes SElinux labelling easier.
> 

That's useful to know, thanks.

> (Also FWIW, the SELinux permissions are also possible with the current ion
> implementation by adding rules to disallow specific ioctls instead of adding
> permissions to access device node as this change would do)
>

Can selinux disallow access within the ioctls though? The access control
wanted is at a heap granularity and disallowing certain ioctls won't fix
that.
 
> 
> - ssp
> 

Thanks,
Laura
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 2/2] staging: ion: create one device entry per heap

[Laura Abbott]

On 10/03/2017 09:48 AM, Mark Brown wrote:
> On Mon, Oct 02, 2017 at 11:07:48AM -0700, Laura Abbott wrote:
> 
>> Thinking about this a bit more, I'm not 100% sure if this
>> will allow the security rules we want. Heap ids are assigned
>> dynamically and therefore so will the /dev/ionX designation.
>> From my understanding, security rules like selinux need to
>> be fully specified at boot time so I'm not sure how you would
>> be able to write rules to differentiate between /dev/ionX and
>> /dev/ionY without knowing the values at boottime.
> 
> Isn't this something that should be managable via udev rules that ensure
> stable names in the same way as for things like disks or ethernet
> controllers (even if it just ends up doing something like /dev/ion-gpu
> or whatever)?  If we're not giving it enough information to assign stable
> names where needed we probably need to fix that anyway.
> 

Android doesn't use a standard udev so we'd need something that
would work there. My understanding was that android needs everything
specified at boot unless that's changed.

There would be enough information to assign stable names
(e.g. /dev/ion-system) if we used the query ioctl to find out
what's actually available. Is just the ioctl useful though?

Thanks,
Laura
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 2/2] staging: ion: create one device entry per heap

[Laura Abbott]

On 09/27/2017 06:20 AM, Benjamin Gaignard wrote:
> Instead a getting only one common device "/dev/ion" for
> all the heaps this patch allow to create one device
> entry ("/dev/ionX") per heap.
> Getting an entry per heap could allow to set security rules
> per heap and global ones for all heaps.
> 
> Allocation requests will be only allowed if the mask_id
> match with device minor.
> Query request could be done on any of the devices.
>
Thinking about this a bit more, I'm not 100% sure if this
will allow the security rules we want. Heap ids are assigned
dynamically and therefore so will the /dev/ionX designation.
From my understanding, security rules like selinux need to
be fully specified at boot time so I'm not sure how you would
be able to write rules to differentiate between /dev/ionX and
/dev/ionY without knowing the values at boottime.

So I think we need a different way to match the heap ids to
get the security we want unless my understanding of security
policies is wrong and we can dynamically specify permissions.

Thanks,
Laura
> Signed-off-by: Benjamin Gaignard 
> ---
> version 5:
> - create a configuration flag to keep legacy Ion misc device
> 
> version 4:
> - add a configuration flag to switch between legacy Ion misc device
>   and one device per heap version.
> 
> version 3:
> - change ion_device_add_heap prototype to return a possible error.
> 
> version 2:
> - simplify ioctl check like propose by Dan
> - make sure that we don't register more than ION_DEV_MAX heaps.
> 
>  drivers/staging/android/TODO|  1 -
>  drivers/staging/android/ion/Kconfig |  7 +++
>  drivers/staging/android/ion/ion-ioctl.c | 18 --
>  drivers/staging/android/ion/ion.c   | 31 ++-
>  drivers/staging/android/ion/ion.h   | 15 +--
>  5 files changed, 66 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/android/TODO b/drivers/staging/android/TODO
> index 5f14247..d770ffa 100644
> --- a/drivers/staging/android/TODO
> +++ b/drivers/staging/android/TODO
> @@ -9,7 +9,6 @@ TODO:
>  ion/
>   - Add dt-bindings for remaining heaps (chunk and carveout heaps). This would
> involve putting appropriate bindings in a memory node for Ion to find.
> - - Split /dev/ion up into multiple nodes (e.g. /dev/ion/heap0)
>   - Better test framework (integration with VGEM was suggested)
>  
>  Please send patches to Greg Kroah-Hartman  and Cc:
> diff --git a/drivers/staging/android/ion/Kconfig 
> b/drivers/staging/android/ion/Kconfig
> index a517b2d..cb4666e 100644
> --- a/drivers/staging/android/ion/Kconfig
> +++ b/drivers/staging/android/ion/Kconfig
> @@ -10,6 +10,13 @@ menuconfig ION
> If you're not using Android its probably safe to
> say N here.
>  
> +config ION_LEGACY_DEVICE_API
> + bool "Keep using Ion legacy misc device API"
> + depends on ION
> + help
> +   Choose this option to keep using Ion legacy misc device API
> +   i.e. /dev/ion
> +
>  config ION_SYSTEM_HEAP
>   bool "Ion system heap"
>   depends on ION
> diff --git a/drivers/staging/android/ion/ion-ioctl.c 
> b/drivers/staging/android/ion/ion-ioctl.c
> index e26b786..bb5c77b 100644
> --- a/drivers/staging/android/ion/ion-ioctl.c
> +++ b/drivers/staging/android/ion/ion-ioctl.c
> @@ -25,7 +25,8 @@ union ion_ioctl_arg {
>   struct ion_heap_query query;
>  };
>  
> -static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
> +static int validate_ioctl_arg(struct file *filp,
> +   unsigned int cmd, union ion_ioctl_arg *arg)
>  {
>   switch (cmd) {
>   case ION_IOC_HEAP_QUERY:
> @@ -34,6 +35,19 @@ static int validate_ioctl_arg(unsigned int cmd, union 
> ion_ioctl_arg *arg)
>   arg->query.reserved2 )
>   return -EINVAL;
>   break;
> +
> + case ION_IOC_ALLOC:
> + {
> + int mask = 1 << iminor(filp->f_inode);
> +
> +#ifdef CONFIG_ION_LEGACY_DEVICE_API
> + if (imajor(filp->f_inode) == MISC_MAJOR)
> + return 0;
> +#endif
> + if (!(arg->allocation.heap_id_mask & mask))
> + return -EINVAL;
> + break;
> + }
>   default:
>   break;
>   }
> @@ -69,7 +83,7 @@ long ion_ioctl(struct file *filp, unsigned int cmd, 
> unsigned long arg)
>   if (copy_from_user(, (void __user *)arg, _IOC_SIZE(cmd)))
>   return -EFAULT;
>  
> - ret = validate_ioctl_arg(cmd, );
> + ret = validate_ioctl_arg(filp, cmd, );
>   if (WARN_ON_ONCE(ret))
>   return ret;
>  
> diff --git a/drivers/staging/android/ion/ion.c 
> b/drivers/staging/android/ion/ion.c
> index 93e2c90..092b24c 100644
> --- a/drivers/staging/android/ion/ion.c
> +++ b/drivers/staging/android/ion/ion.c
> @@ -40,6 +40,8 @@
>  
>  #include "ion.h"
>  
> +#define ION_DEV_MAX 32
> +
>  static struct ion_device 

Re: [PATCH v4 2/2] staging: ion: create one device entry per heap

[Laura Abbott]

On 09/26/2017 09:17 AM, Mark Brown wrote:

On Tue, Sep 26, 2017 at 02:07:05PM +0200, Benjamin Gaignard wrote:


version 4:
- add a configuration flag to switch between legacy Ion misc device
   and one device per heap version.


Should this be a switch or should it just be enabling and disabling the
legacy device with the per heap ones always availalbe?  I can't see that
the new devices would do any harm or have trouble interacting with the
per heap ones.  Being able to have both enabled would make things easier
for userspaces that are moving to the device per heap interface.



Agreed. We should be enabling the new interface unconditionally. The
old /dev/ion interface should coexist to allow for backwards
compatibility but keep it under a Kconfig to allow it to be turned off
for security or other reasons.

Thanks,
Laura
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 2/2] staging: ion: create one device entry per heap

[Laura Abbott]

On 09/25/2017 11:56 PM, Greg KH wrote:

On Tue, Sep 26, 2017 at 07:09:14AM +0200, Daniel Vetter wrote:

On Mon, Sep 25, 2017 at 11:26:27AM -0700, Laura Abbott wrote:

On 09/20/2017 01:45 AM, Benjamin Gaignard wrote:

Instead a getting one common device "/dev/ion" for
all the heaps this patch allow to create one device
entry ("/dev/ionX") per heap.
Getting an entry per heap could allow to set security rules
per heap and global ones for all heaps.

Allocation requests will be only allowed if the mask_id
match with device minor.
Query request could be done on any of the devices.
Deivce node major will also change and that may impact init scripts.



We should start Cc linux-api for future changes since we're going
to have more than just /dev/ion.

Thinking about this some more, I think we need to allow backwards
compatibility. It's just not feasible to keep throwing workarounds
into userspace if we can avoid it. I'd propose keeping the old /dev/ion
misc interface available under a Kconfig and then creating the new
split heaps in parallel.

On a somewhat related note, there was some interest in possibly
having a sysfs interface for Ion long term. I don't think this
needs to happen right now but I'd like whatever we do to not
make adding that harder.


Not sure sysfs is a good idea for allocating buffers. The backlight
interface is in sysfs, which defacto makes it a root-only interface.
Distros really hate that, because it makes unpriviledged X/wayland really
hard to pull of. Passing a device file otoh from logind to the compositor
is dead simple (and how X et al get at e.g. the drm/input devices
already).


sysfs is not a good idea for allocating buffers.  We already have some
out-of-tree drm drivers doing horrid things in sysfs in ways that
totally abuse that api, and vendors have to do crazy things with selinux
rules to try to lock it down because of that.  A device node is fine, we
are used to that for graphics stuff :)

thanks,

greg k-h



I wasn't thinking of sysfs for allocation, this was for exposure of
other Ion information that might make more sense than debugfs. Like
I said, this was mostly forward thinking to make sure we aren't
stuck later.

Thanks,
Laura
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 2/2] staging: ion: create one device entry per heap

[Laura Abbott]

On 09/20/2017 01:45 AM, Benjamin Gaignard wrote:

Instead a getting one common device "/dev/ion" for
all the heaps this patch allow to create one device
entry ("/dev/ionX") per heap.
Getting an entry per heap could allow to set security rules
per heap and global ones for all heaps.

Allocation requests will be only allowed if the mask_id
match with device minor.
Query request could be done on any of the devices.
Deivce node major will also change and that may impact init scripts.



We should start Cc linux-api for future changes since we're going
to have more than just /dev/ion.

Thinking about this some more, I think we need to allow backwards
compatibility. It's just not feasible to keep throwing workarounds
into userspace if we can avoid it. I'd propose keeping the old /dev/ion
misc interface available under a Kconfig and then creating the new
split heaps in parallel.

On a somewhat related note, there was some interest in possibly
having a sysfs interface for Ion long term. I don't think this
needs to happen right now but I'd like whatever we do to not
make adding that harder.

Thanks,
Laura
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/3] staging: android: ion: Minor clean ups and fixes

[Laura Abbott]

On 05/17/2017 01:15 AM, Archit Taneja wrote:
> The recent ION clean ups introduced some leftover code that can be
> removed, and a bug that comes up if the call to dma_buf_map_attachment()
> from an importer fails. Fix these.
> 
> Archit Taneja (3):
>   staging: android: ion: Remove unused members from ion_buffer
>   staging: android: ion: Remove ION_FLAG_CACHED_NEEDS_SYNC
>   staging: android: ion: Avoid calling free_duped_table() twice
> 
>  drivers/staging/android/ion/ion.c  | 14 +++---
>  drivers/staging/android/ion/ion.h  | 14 --
>  drivers/staging/android/uapi/ion.h |  6 --
>  3 files changed, 3 insertions(+), 31 deletions(-)
> 

Acked-by: Laura Abbott <labb...@redhat.com>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCHv2] drm/prime: Forward declare struct device

[Laura Abbott]

We need a declaration of struct device to avoid warnings:

In file included from include/drm/drm_file.h:38:0,
 from drivers/gpu/drm/drm_file.c:38:
include/drm/drm_prime.h:71:14: warning: 'struct device' declared inside
  parameter list will not be visible outside of this definition or
  declaration
  struct device *attach_dev);
 ^~

Forward declare it.

Signed-off-by: Laura Abbott <labb...@redhat.com>
---
v2: Switch to foward declaration instead of including a header.
---
 include/drm/drm_prime.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/drm/drm_prime.h b/include/drm/drm_prime.h
index 46fd1fb..59ccab4 100644
--- a/include/drm/drm_prime.h
+++ b/include/drm/drm_prime.h
@@ -50,6 +50,8 @@ struct drm_prime_file_private {
struct rb_root handles;
 };
 
+struct device;
+
 struct dma_buf_export_info;
 struct dma_buf;
 
-- 
2.7.4

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel