>-----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
