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.
Signed-off-by: Zeng Tao <[email protected]>
---
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(&dev->lock);
plist_for_each_entry(heap, &dev->heaps, node) {
@@ -421,10 +421,10 @@ static int ion_alloc(size_t len, unsigned int heap_id_mask, unsigned int flags)
up_read(&dev->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 = &dma_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(&exp_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;
--
2.7.4
If we're going to export ion_alloc() for other drivers to use then let's
make an ion_free() helper function as well.
void ion_free(struct dma_buf *dmabuf)
{
dma_buf_put(dmabuf);
}
regards,
dan carpenter
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 <[email protected]>
> ---
> 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(&dev->lock);
> plist_for_each_entry(heap, &dev->heaps, node) {
> @@ -421,10 +421,10 @@ static int ion_alloc(size_t len, unsigned int heap_id_mask, unsigned int flags)
> up_read(&dev->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 = &dma_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(&exp_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;
>
>
On Sat, Mar 30, 2019 at 02:40:16AM +0800, 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.
>
> Signed-off-by: Zeng Tao <[email protected]>
> ---
> 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(&dev->lock);
> plist_for_each_entry(heap, &dev->heaps, node) {
> @@ -421,10 +421,10 @@ static int ion_alloc(size_t len, unsigned int heap_id_mask, unsigned int flags)
> up_read(&dev->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 = &dma_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(&exp_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);
If you are going to do this (and personally I'm with Laura in that I
don't think you need it) this should be EXPORT_SYMBOL_GPL() please.
thanks,
greg k-h
On Sat, Mar 30, 2019 at 02:40:16AM +0800, 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.
As this really is just DMA, what is wrong with the existing dma
framework that makes it hard to use? You have seen all of the changes
recently to it, right?
thanks,
greg k-h
Hi laura:
>-----Original Message-----
>From: Laura Abbott [mailto:[email protected]]
>Sent: Friday, March 29, 2019 9:27 PM
>To: Zengtao (B) <[email protected]>; [email protected]
>Cc: Greg Kroah-Hartman <[email protected]>; Arve Hjønnevåg
><[email protected]>; Todd Kjos <[email protected]>; Martijn Coenen
><[email protected]>; Joel Fernandes <[email protected]>;
>Christian Brauner <[email protected]>; [email protected];
>[email protected]; [email protected];
>[email protected]
>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)
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.
>
>> Signed-off-by: Zeng Tao <[email protected]>
>> ---
>> 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(&dev->lock);
>> plist_for_each_entry(heap, &dev->heaps, node) { @@ -421,10
>+421,10
>> @@ static int ion_alloc(size_t len, unsigned int heap_id_mask,
>unsigned int flags)
>> up_read(&dev->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 = &dma_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(&exp_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;
>>
>>
>-----Original Message-----
>From: Greg Kroah-Hartman [mailto:[email protected]]
>Sent: Saturday, March 30, 2019 12:02 AM
>To: Zengtao (B) <[email protected]>
>Cc: [email protected]; [email protected];
>[email protected]; Todd Kjos <[email protected]>;
>[email protected]; [email protected];
>[email protected]; Arve Hjønnevåg <[email protected]>;
>Joel Fernandes <[email protected]>; Martijn Coenen
><[email protected]>; Christian Brauner <[email protected]>
>Subject: Re: [PATCH] staging: android: ion: refactory ion_alloc for kernel
>driver use
>
>On Sat, Mar 30, 2019 at 02:40:16AM +0800, 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.
>>
>> Signed-off-by: Zeng Tao <[email protected]>
>> ---
>> 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(&dev->lock);
>> plist_for_each_entry(heap, &dev->heaps, node) { @@ -421,10
>+421,10
>> @@ static int ion_alloc(size_t len, unsigned int heap_id_mask,
>unsigned int flags)
>> up_read(&dev->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 = &dma_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(&exp_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);
>
>If you are going to do this (and personally I'm with Laura in that I don't
>think you need it) this should be EXPORT_SYMBOL_GPL() please.
>
Agree, thanks
Regards
Zengtao
>thanks,
>
>greg k-h
>-----Original Message-----
>From: Greg Kroah-Hartman [mailto:[email protected]]
>Sent: Saturday, March 30, 2019 12:04 AM
>To: Zengtao (B) <[email protected]>
>Cc: [email protected]; [email protected];
>[email protected]; Todd Kjos <[email protected]>;
>[email protected]; [email protected];
>[email protected]; Arve Hjønnevåg <[email protected]>;
>Joel Fernandes <[email protected]>; Martijn Coenen
><[email protected]>; Christian Brauner <[email protected]>
>Subject: Re: [PATCH] staging: android: ion: refactory ion_alloc for kernel
>driver use
>
>On Sat, Mar 30, 2019 at 02:40:16AM +0800, 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.
>
>As this really is just DMA, what is wrong with the existing dma framework
>that makes it hard to use? You have seen all of the changes recently to it,
>right?
The current dma framework is powerful enough(to me, and more complex ^_^)
, CMA, IOMMU are all integrated, it's good. But buffer sharing, statistics, debug,
are not so friendly for media drivers(each driver has to do all, but duplicate jobs).
>
>thanks,
>
>greg k-h
>-----Original Message-----
>From: Dan Carpenter [mailto:[email protected]]
>Sent: Friday, March 29, 2019 7:03 PM
>To: Zengtao (B) <[email protected]>
>Cc: [email protected]; [email protected];
>[email protected]; Todd Kjos <[email protected]>; Greg
>Kroah-Hartman <[email protected]>;
>[email protected]; [email protected];
>[email protected]; Arve Hjønnevåg <[email protected]>;
>Joel Fernandes <[email protected]>; Martijn Coenen
><[email protected]>; Christian Brauner <[email protected]>
>Subject: Re: [PATCH] staging: android: ion: refactory ion_alloc for kernel
>driver use
>
>If we're going to export ion_alloc() for other drivers to use then let's make
>an ion_free() helper function as well.
>
Good catch, thanks.
Regards
Zengtao
>void ion_free(struct dma_buf *dmabuf)
>{
> dma_buf_put(dmabuf);
>}
>
>regards,
>dan carpenter
On Sat, Mar 30, 2019 at 02:32:35AM +0000, Zengtao (B) wrote:
> >-----Original Message-----
> >From: Greg Kroah-Hartman [mailto:[email protected]]
> >Sent: Saturday, March 30, 2019 12:04 AM
> >To: Zengtao (B) <[email protected]>
> >Cc: [email protected]; [email protected];
> >[email protected]; Todd Kjos <[email protected]>;
> >[email protected]; [email protected];
> >[email protected]; Arve Hj?nnev?g <[email protected]>;
> >Joel Fernandes <[email protected]>; Martijn Coenen
> ><[email protected]>; Christian Brauner <[email protected]>
> >Subject: Re: [PATCH] staging: android: ion: refactory ion_alloc for kernel
> >driver use
> >
> >On Sat, Mar 30, 2019 at 02:40:16AM +0800, 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.
> >
> >As this really is just DMA, what is wrong with the existing dma framework
> >that makes it hard to use? You have seen all of the changes recently to it,
> >right?
>
> The current dma framework is powerful enough(to me, and more complex ^_^)
> , CMA, IOMMU are all integrated, it's good. But buffer sharing, statistics, debug,
> are not so friendly for media drivers(each driver has to do all, but duplicate jobs).
Then go add statistics and debugging to the dma code so that everyone
benefits!
thanks,
greg k-h
Hi Zeng,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on staging/staging-testing]
[also build test WARNING on v5.1-rc2 next-20190329]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Zeng-Tao/staging-android-ion-refactory-ion_alloc-for-kernel-driver-use/20190330-071409
coccinelle warnings: (new ones prefixed by >>)
>> drivers/staging/android/ion/ion.c:427:9-16: WARNING: ERR_CAST can be used with buffer
Please review and possibly fold the followup patch.
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
From: kbuild test robot <[email protected]>
drivers/staging/android/ion/ion.c:427:9-16: WARNING: ERR_CAST can be used with buffer
Use ERR_CAST inlined function instead of ERR_PTR(PTR_ERR(...))
Generated by: scripts/coccinelle/api/err_cast.cocci
Fixes: d51f182d188a ("staging: android: ion: refactory ion_alloc for kernel driver use")
CC: Zeng Tao <[email protected]>
Signed-off-by: kbuild test robot <[email protected]>
---
url: https://github.com/0day-ci/linux/commits/Zeng-Tao/staging-android-ion-refactory-ion_alloc-for-kernel-driver-use/20190330-071409
ion.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -424,7 +424,7 @@ struct dma_buf *ion_alloc(size_t len, un
return ERR_PTR(-ENODEV);
if (IS_ERR(buffer))
- return ERR_PTR(PTR_ERR(buffer));
+ return ERR_CAST(buffer);
exp_info.ops = &dma_buf_ops;
exp_info.size = buffer->size;
On 3/29/19 7:26 PM, Zengtao (B) wrote:
> Hi laura:
>
>> -----Original Message-----
>> From: Laura Abbott [mailto:[email protected]]
>> Sent: Friday, March 29, 2019 9:27 PM
>> To: Zengtao (B) <[email protected]>; [email protected]
>> Cc: Greg Kroah-Hartman <[email protected]>; Arve Hjønnevåg
>> <[email protected]>; Todd Kjos <[email protected]>; Martijn Coenen
>> <[email protected]>; Joel Fernandes <[email protected]>;
>> Christian Brauner <[email protected]>; [email protected];
>> [email protected]; [email protected];
>> [email protected]
>> 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 <[email protected]>
>>> ---
>>> 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(&dev->lock);
>>> plist_for_each_entry(heap, &dev->heaps, node) { @@ -421,10
>> +421,10
>>> @@ static int ion_alloc(size_t len, unsigned int heap_id_mask,
>> unsigned int flags)
>>> up_read(&dev->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 = &dma_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(&exp_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;
>>>
>>>
>