Re: [PATCH] staging: android: ion: add buffer flag update ioctl
From: Laura Abbott
Date: Wed Jan 02 2019 - 17:31:13 EST
On 12/23/18 6:47 PM, Zengtao (B) wrote:
Hi laura:
-----Original Message-----
From: Laura Abbott [mailto:labbott@xxxxxxxxxx]
Sent: Friday, December 21, 2018 4:50 AM
To: Zengtao (B) <prime.zeng@xxxxxxxxxxxxx>; sumit.semwal@xxxxxxxxxx
Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>; Arve HjÃnnevÃg
<arve@xxxxxxxxxxx>; Todd Kjos <tkjos@xxxxxxxxxxx>; Martijn Coenen
<maco@xxxxxxxxxxx>; Joel Fernandes <joel@xxxxxxxxxxxxxxxxx>;
devel@xxxxxxxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx;
linaro-mm-sig@xxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
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:labbott@xxxxxxxxxx]
Sent: Thursday, December 20, 2018 2:10 AM
To: Zengtao (B) <prime.zeng@xxxxxxxxxxxxx>;
sumit.semwal@xxxxxxxxxx
Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>; Arve
HjÃnnevÃg
<arve@xxxxxxxxxxx>; Todd Kjos <tkjos@xxxxxxxxxxx>; Martijn
Coenen
<maco@xxxxxxxxxxx>; Joel Fernandes <joel@xxxxxxxxxxxxxxxxx>;
devel@xxxxxxxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx;
linaro-mm-sig@xxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
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 <prime.zeng@xxxxxxxxxxxxx>
---
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(&data.query);
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