>-----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/201 >>> 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. 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 >>>>>> >>>> >> _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
