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

[Zengtao (B)]

Hi laura: 

>-----Original Message-----
>From: Laura Abbott [mailto:labbott@xxxxxxxxxx]
>Sent: Friday, March 29, 2019 9:27 PM
>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>;
>Christian Brauner <christian@xxxxxxxxxx>; devel@xxxxxxxxxxxxxxxxxxxx;
>dri-devel@xxxxxxxxxxxxxxxxxxxxx; linaro-mm-sig@xxxxxxxxxxxxxxxx;
>linux-kernel@xxxxxxxxxxxxxxx
>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 <prime.zeng@xxxxxxxxxxxxx>
>> ---
>>   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;
>>
>>