[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:labbott at redhat.com]
>> Sent: Thursday, December 20, 2018 2:10 AM
>> To: Zengtao (B) <prime.zeng at hisilicon.com>; sumit.semwal at linaro.org
>> Cc: Greg Kroah-Hartman <gregkh at linuxfoundation.org>; Arve Hjønnevåg
>> <arve at android.com>; Todd Kjos <tkjos at android.com>; Martijn Coenen
>> <maco at android.com>; Joel Fernandes <joel at joelfernandes.org>;
>> devel at driverdev.osuosl.org; dri-devel at lists.freedesktop.org;
>> linaro-mm-sig at lists.linaro.org; linux-kernel at 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 <prime.zeng at hisilicon.com>
>>> ---
>>>    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
>>>
>