RE: [PATCH] staging: android: ion: add buffer flag update ioctl

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



>-----Original Message-----
>From: Laura Abbott [mailto:labbott@xxxxxxxxxx]
>Sent: Friday, January 04, 2019 9:53 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 1/3/19 5:42 PM, Zengtao (B) wrote:
>>> -----Original Message-----
>>> From: Laura Abbott [mailto:labbott@xxxxxxxxxx]
>>> Sent: Thursday, January 03, 2019 6:31 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/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/2
>>>>> 01
>>>>> 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@DESK
>TOP-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?
>

Thanks for the detail info, I get your point, and we know the aliasing issue 
but we have never met, so it's hard to explain that we really have that issue.

>Laura
>
>> Thanks
>> Zengtao
>>
>>>
>>>>>
>>>>>>>
>>>>>>>> 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
>>>>>>>>
>>>>>>
>>>>
>>

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




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux