Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

51 participants
6528 discussions

Re: [Linaro-mm-sig] [PATCH] staging: android: ion: add buffer flag update ioctl

by Laura Abbott

On 1/3/19 5:42 PM, Zengtao (B) wrote: >> -----Original Message----- >> From: Laura Abbott [mailto:labbott@redhat.com] >> Sent: Thursday, January 03, 2019 6:31 AM >> To: Zengtao (B) <prime.zeng(a)hisilicon.com>; sumit.semwal(a)linaro.org >> Cc: Greg Kroah-Hartman <gregkh(a)linuxfoundation.org>; Arve Hjønnevåg >> <arve(a)android.com>; Todd Kjos <tkjos(a)android.com>; Martijn Coenen >> <maco(a)android.com>; Joel Fernandes <joel(a)joelfernandes.org>; >> devel(a)driverdev.osuosl.org; dri-devel(a)lists.freedesktop.org; >> linaro-mm-sig(a)lists.linaro.org; linux-kernel(a)vger.kernel.org >> 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@redhat.com] >>>> Sent: Friday, December 21, 2018 4:50 AM >>>> To: Zengtao (B) <prime.zeng(a)hisilicon.com>; >> sumit.semwal(a)linaro.org >>>> Cc: Greg Kroah-Hartman <gregkh(a)linuxfoundation.org>; Arve >> Hjønnevåg >>>> <arve(a)android.com>; Todd Kjos <tkjos(a)android.com>; Martijn >> Coenen >>>> <maco(a)android.com>; Joel Fernandes <joel(a)joelfernandes.org>; >>>> devel(a)driverdev.osuosl.org; dri-devel(a)lists.freedesktop.org; >>>> linaro-mm-sig(a)lists.linaro.org; linux-kernel(a)vger.kernel.org >>>> 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@redhat.com] >>>>>> Sent: Thursday, December 20, 2018 2:10 AM >>>>>> To: Zengtao (B) <prime.zeng(a)hisilicon.com>; >>>> sumit.semwal(a)linaro.org >>>>>> Cc: Greg Kroah-Hartman <gregkh(a)linuxfoundation.org>; Arve >>>> Hjønnevåg >>>>>> <arve(a)android.com>; Todd Kjos <tkjos(a)android.com>; Martijn >>>> Coenen >>>>>> <maco(a)android.com>; Joel Fernandes <joel(a)joelfernandes.org>; >>>>>> devel(a)driverdev.osuosl.org; dri-devel(a)lists.freedesktop.org; >>>>>> linaro-mm-sig(a)lists.linaro.org; linux-kernel(a)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/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. > 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@DESKTOP-E1NTVV… 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? Laura > Thanks > Zengtao > >> >>>> >>>>>> >>>>>>> Signed-off-by: Zeng Tao <prime.zeng(a)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 >>>>>>> >>>>> >>> >

7 years, 4 months

Re: [Linaro-mm-sig] [PATCH] staging: android: ion: move map_kernel to ion_dma_buf_kmap

by Laura Abbott

On 12/24/18 12:19 AM, Qing Xia wrote: > Now, as Google's user guide, if userspace need clean ION buffer's > cache, they should call ioctl(fd, DMA_BUF_IOCTL_SYNC, sync). Then > we found that ion_dma_buf_begin_cpu_access/ion_dma_buf_end_cpu_access > will do ION buffer's map_kernel, it's not necessary. And if usersapce > only need clean cache, they will call ion_dma_buf_end_cpu_access by > dmabuf's ioctl, ION buffer's kmap_cnt will be wrong value -1, then > driver could not get right kernel vaddr by dma_buf_kmap. > The problem is this subtly violates how dma_buf is supposed to work. All setup is supposed to happen in begin_cpu_access. I agree calling map kernel each time isn't great but I think this needs to be worked out with dma_buf. Thanks, Laura > Signed-off-by: Qing Xia <saberlily.xia(a)hisilicon.com> > --- > drivers/staging/android/ion/ion.c | 46 ++++++++++++++++++--------------------- > 1 file changed, 21 insertions(+), 25 deletions(-) > > diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c > index 9907332..f7e2812 100644 > --- a/drivers/staging/android/ion/ion.c > +++ b/drivers/staging/android/ion/ion.c > @@ -303,45 +303,47 @@ static void ion_dma_buf_release(struct dma_buf *dmabuf) > static void *ion_dma_buf_kmap(struct dma_buf *dmabuf, unsigned long offset) > { > struct ion_buffer *buffer = dmabuf->priv; > + void *vaddr; > > - return buffer->vaddr + offset * PAGE_SIZE; > + if (buffer->heap->ops->map_kernel) { > + mutex_lock(&buffer->lock); > + vaddr = ion_buffer_kmap_get(buffer); > + mutex_unlock(&buffer->lock); > + if (IS_ERR(vaddr)) > + return vaddr; > + > + return vaddr + offset * PAGE_SIZE; > + } > + > + return NULL; > } > > static void ion_dma_buf_kunmap(struct dma_buf *dmabuf, unsigned long offset, > void *ptr) > { > + struct ion_buffer *buffer = dmabuf->priv; > + > + if (buffer->heap->ops->map_kernel) { > + mutex_lock(&buffer->lock); > + ion_buffer_kmap_put(buffer); > + mutex_unlock(&buffer->lock); > + } > } > > static int ion_dma_buf_begin_cpu_access(struct dma_buf *dmabuf, > enum dma_data_direction direction) > { > struct ion_buffer *buffer = dmabuf->priv; > - void *vaddr; > struct ion_dma_buf_attachment *a; > - int ret = 0; > - > - /* > - * TODO: Move this elsewhere because we don't always need a vaddr > - */ > - if (buffer->heap->ops->map_kernel) { > - mutex_lock(&buffer->lock); > - vaddr = ion_buffer_kmap_get(buffer); > - if (IS_ERR(vaddr)) { > - ret = PTR_ERR(vaddr); > - goto unlock; > - } > - mutex_unlock(&buffer->lock); > - } > > mutex_lock(&buffer->lock); > list_for_each_entry(a, &buffer->attachments, list) { > dma_sync_sg_for_cpu(a->dev, a->table->sgl, a->table->nents, > direction); > } > - > -unlock: > mutex_unlock(&buffer->lock); > - return ret; > + > + return 0; > } > > static int ion_dma_buf_end_cpu_access(struct dma_buf *dmabuf, > @@ -350,12 +352,6 @@ static int ion_dma_buf_end_cpu_access(struct dma_buf *dmabuf, > struct ion_buffer *buffer = dmabuf->priv; > struct ion_dma_buf_attachment *a; > > - if (buffer->heap->ops->map_kernel) { > - mutex_lock(&buffer->lock); > - ion_buffer_kmap_put(buffer); > - mutex_unlock(&buffer->lock); > - } > - > mutex_lock(&buffer->lock); > list_for_each_entry(a, &buffer->attachments, list) { > dma_sync_sg_for_device(a->dev, a->table->sgl, a->table->nents, >

7 years, 4 months

Re: [Linaro-mm-sig] [PATCH] staging: android: ion: add buffer flag update ioctl

by Laura Abbott

On 12/23/18 6:47 PM, Zengtao (B) wrote: > Hi laura: > >> -----Original Message----- >> From: Laura Abbott [mailto:labbott@redhat.com] >> Sent: Friday, December 21, 2018 4:50 AM >> To: Zengtao (B) <prime.zeng(a)hisilicon.com>; sumit.semwal(a)linaro.org >> Cc: Greg Kroah-Hartman <gregkh(a)linuxfoundation.org>; Arve Hjønnevåg >> <arve(a)android.com>; Todd Kjos <tkjos(a)android.com>; Martijn Coenen >> <maco(a)android.com>; Joel Fernandes <joel(a)joelfernandes.org>; >> devel(a)driverdev.osuosl.org; dri-devel(a)lists.freedesktop.org; >> linaro-mm-sig(a)lists.linaro.org; linux-kernel(a)vger.kernel.org >> 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@redhat.com] >>>> Sent: Thursday, December 20, 2018 2:10 AM >>>> To: Zengtao (B) <prime.zeng(a)hisilicon.com>; >> sumit.semwal(a)linaro.org >>>> Cc: Greg Kroah-Hartman <gregkh(a)linuxfoundation.org>; Arve >> Hjønnevåg >>>> <arve(a)android.com>; Todd Kjos <tkjos(a)android.com>; Martijn >> Coenen >>>> <maco(a)android.com>; Joel Fernandes <joel(a)joelfernandes.org>; >>>> devel(a)driverdev.osuosl.org; dri-devel(a)lists.freedesktop.org; >>>> linaro-mm-sig(a)lists.linaro.org; linux-kernel(a)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-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. Thanks, Laura >> >>>> >>>>> Signed-off-by: Zeng Tao <prime.zeng(a)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 >>>>> >>> >

7 years, 4 months

Re: [Linaro-mm-sig] [PATCH] dma-buf: Change to use DEFINE_SHOW_ATTRIBUTE macro

by Daniel Vetter

On Fri, Nov 30, 2018 at 11:11:01AM -0500, Yangtao Li wrote: > Use DEFINE_SHOW_ATTRIBUTE macro to simplify the code. > > Signed-off-by: Yangtao Li <tiny.windzz(a)gmail.com> Sorry, fell through the cracks, applied for 4.22, thanks for your patch. For next time around pls ping again after 1-2 weeks already, instead of 1-2 months. Thanks, Daniel > --- > drivers/dma-buf/dma-buf.c | 12 +----------- > drivers/dma-buf/sync_debug.c | 16 +++------------- > 2 files changed, 4 insertions(+), 24 deletions(-) > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index 02f7f9a89979..7c858020d14b 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -1093,17 +1093,7 @@ static int dma_buf_debug_show(struct seq_file *s, void *unused) > return 0; > } > > -static int dma_buf_debug_open(struct inode *inode, struct file *file) > -{ > - return single_open(file, dma_buf_debug_show, NULL); > -} > - > -static const struct file_operations dma_buf_debug_fops = { > - .open = dma_buf_debug_open, > - .read = seq_read, > - .llseek = seq_lseek, > - .release = single_release, > -}; > +DEFINE_SHOW_ATTRIBUTE(dma_buf_debug); > > static struct dentry *dma_buf_debugfs_dir; > > diff --git a/drivers/dma-buf/sync_debug.c b/drivers/dma-buf/sync_debug.c > index c4c8ecb24aa9..c0abf37df88b 100644 > --- a/drivers/dma-buf/sync_debug.c > +++ b/drivers/dma-buf/sync_debug.c > @@ -147,7 +147,7 @@ static void sync_print_sync_file(struct seq_file *s, > } > } > > -static int sync_debugfs_show(struct seq_file *s, void *unused) > +static int sync_info_debugfs_show(struct seq_file *s, void *unused) > { > struct list_head *pos; > > @@ -178,17 +178,7 @@ static int sync_debugfs_show(struct seq_file *s, void *unused) > return 0; > } > > -static int sync_info_debugfs_open(struct inode *inode, struct file *file) > -{ > - return single_open(file, sync_debugfs_show, inode->i_private); > -} > - > -static const struct file_operations sync_info_debugfs_fops = { > - .open = sync_info_debugfs_open, > - .read = seq_read, > - .llseek = seq_lseek, > - .release = single_release, > -}; > +DEFINE_SHOW_ATTRIBUTE(sync_info_debugfs); > > static __init int sync_debugfs_init(void) > { > @@ -218,7 +208,7 @@ void sync_dump(void) > }; > int i; > > - sync_debugfs_show(&s, NULL); > + sync_info_debugfs_show(&s, NULL); > > for (i = 0; i < s.count; i += DUMP_CHUNK) { > if ((s.count - i) > DUMP_CHUNK) { > -- > 2.17.0 > > _______________________________________________ > dri-devel mailing list > dri-devel(a)lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

7 years, 4 months

Re: [Linaro-mm-sig] [PATCH] staging: android: ion: add buffer flag update ioctl

by Laura Abbott

On 12/19/18 5:39 PM, Zengtao (B) wrote: > Hi laura: > >> -----Original Message----- >> From: Laura Abbott [mailto:labbott@redhat.com] >> Sent: Thursday, December 20, 2018 2:10 AM >> To: Zengtao (B) <prime.zeng(a)hisilicon.com>; sumit.semwal(a)linaro.org >> Cc: Greg Kroah-Hartman <gregkh(a)linuxfoundation.org>; Arve Hjønnevåg >> <arve(a)android.com>; Todd Kjos <tkjos(a)android.com>; Martijn Coenen >> <maco(a)android.com>; Joel Fernandes <joel(a)joelfernandes.org>; >> devel(a)driverdev.osuosl.org; dri-devel(a)lists.freedesktop.org; >> linaro-mm-sig(a)lists.linaro.org; linux-kernel(a)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-Nove…) What's your usecase for uncached buffers? >> >>> Signed-off-by: Zeng Tao <prime.zeng(a)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 >>> >

7 years, 4 months

Re: [Linaro-mm-sig] [PATCH] staging: android: ion: add buffer flag update ioctl

by Laura Abbott

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? > Signed-off-by: Zeng Tao <prime.zeng(a)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 >

7 years, 4 months

[PATCH] dma-buf: fix debugfs versus rcu and fence dumping v2

by jglisse＠redhat.com

From: Jérôme Glisse <jglisse(a)redhat.com> The debugfs take reference on fence without dropping them. Also the rcu section are not well balance. Fix all that ... Changed since v1: - moved fobj logic around to be rcu safe Signed-off-by: Jérôme Glisse <jglisse(a)redhat.com> Cc: Christian König <christian.koenig(a)amd.com> Cc: Daniel Vetter <daniel.vetter(a)ffwll.ch> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: linux-media(a)vger.kernel.org Cc: dri-devel(a)lists.freedesktop.org Cc: linaro-mm-sig(a)lists.linaro.org Cc: Stéphane Marchesin <marcheu(a)chromium.org> Cc: stable(a)vger.kernel.org --- drivers/dma-buf/dma-buf.c | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 13884474d158..9688d99894d6 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -1048,27 +1048,38 @@ static int dma_buf_debug_show(struct seq_file *s, void *unused) while (true) { seq = read_seqcount_begin(&robj->seq); rcu_read_lock(); - fobj = rcu_dereference(robj->fence); - shared_count = fobj ? fobj->shared_count : 0; fence = rcu_dereference(robj->fence_excl); + fence = dma_fence_get_rcu(fence); if (!read_seqcount_retry(&robj->seq, seq)) break; rcu_read_unlock(); } - - if (fence) + if (fence) { seq_printf(s, "\tExclusive fence: %s %s %ssignalled\n", fence->ops->get_driver_name(fence), fence->ops->get_timeline_name(fence), dma_fence_is_signaled(fence) ? "" : "un"); - for (i = 0; i < shared_count; i++) { + dma_fence_put(fence); + } + + rcu_read_lock(); + fobj = rcu_dereference(robj->fence); + shared_count = fobj ? fobj->shared_count : 0; + for (i = 0, fence = NULL; i < shared_count; i++) { fence = rcu_dereference(fobj->shared[i]); if (!dma_fence_get_rcu(fence)) continue; + rcu_read_unlock(); + seq_printf(s, "\tShared fence: %s %s %ssignalled\n", fence->ops->get_driver_name(fence), fence->ops->get_timeline_name(fence), dma_fence_is_signaled(fence) ? "" : "un"); + dma_fence_put(fence); + + rcu_read_lock(); + fobj = rcu_dereference(robj->fence); + shared_count = fobj ? fobj->shared_count : 0; } rcu_read_unlock(); -- 2.17.2

7 years, 5 months

[PATCH] dma-buf: balance refcount inbalance

by jglisse＠redhat.com

From: Jérôme Glisse <jglisse(a)redhat.com> The debugfs take reference on fence without dropping them. Signed-off-by: Jérôme Glisse <jglisse(a)redhat.com> Cc: Christian König <christian.koenig(a)amd.com> Cc: Daniel Vetter <daniel.vetter(a)ffwll.ch> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: linux-media(a)vger.kernel.org Cc: dri-devel(a)lists.freedesktop.org Cc: linaro-mm-sig(a)lists.linaro.org Cc: Stéphane Marchesin <marcheu(a)chromium.org> Cc: stable(a)vger.kernel.org --- drivers/dma-buf/dma-buf.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 13884474d158..69842145c223 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -1069,6 +1069,7 @@ static int dma_buf_debug_show(struct seq_file *s, void *unused) fence->ops->get_driver_name(fence), fence->ops->get_timeline_name(fence), dma_fence_is_signaled(fence) ? "" : "un"); + dma_fence_put(fence); } rcu_read_unlock(); -- 2.17.2

7 years, 5 months

[PATCH] dma-buf: fix debugfs versus rcu and fence dumping

by jglisse＠redhat.com

From: Jérôme Glisse <jglisse(a)redhat.com> The debugfs take reference on fence without dropping them. Also the rcu section are not well balance. Fix all that ... Signed-off-by: Jérôme Glisse <jglisse(a)redhat.com> Cc: Christian König <christian.koenig(a)amd.com> Cc: Daniel Vetter <daniel.vetter(a)ffwll.ch> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: linux-media(a)vger.kernel.org Cc: dri-devel(a)lists.freedesktop.org Cc: linaro-mm-sig(a)lists.linaro.org Cc: Stéphane Marchesin <marcheu(a)chromium.org> Cc: stable(a)vger.kernel.org --- drivers/dma-buf/dma-buf.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 13884474d158..f6f4de42ac49 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -1051,24 +1051,31 @@ static int dma_buf_debug_show(struct seq_file *s, void *unused) fobj = rcu_dereference(robj->fence); shared_count = fobj ? fobj->shared_count : 0; fence = rcu_dereference(robj->fence_excl); + fence = dma_fence_get_rcu(fence); if (!read_seqcount_retry(&robj->seq, seq)) break; rcu_read_unlock(); } - - if (fence) + if (fence) { seq_printf(s, "\tExclusive fence: %s %s %ssignalled\n", fence->ops->get_driver_name(fence), fence->ops->get_timeline_name(fence), dma_fence_is_signaled(fence) ? "" : "un"); + dma_fence_put(fence); + } + + rcu_read_lock(); for (i = 0; i < shared_count; i++) { fence = rcu_dereference(fobj->shared[i]); if (!dma_fence_get_rcu(fence)) continue; + rcu_read_unlock(); seq_printf(s, "\tShared fence: %s %s %ssignalled\n", fence->ops->get_driver_name(fence), fence->ops->get_timeline_name(fence), dma_fence_is_signaled(fence) ? "" : "un"); + dma_fence_put(fence); + rcu_read_lock(); } rcu_read_unlock(); -- 2.17.2

7 years, 5 months

Re: [Linaro-mm-sig] [PATCH] staging: android: ion: Remove unused header files

by Laura Abbott

On 11/30/18 6:43 AM, Yangtao Li wrote: > seq_file.h does not need to be included,so remove it. > Acked-by: Laura Abbott <labbott(a)redhat.com> > Signed-off-by: Yangtao Li <tiny.windzz(a)gmail.com> > --- > drivers/staging/android/ion/ion.c | 1 - > drivers/staging/android/ion/ion_system_heap.c | 1 - > 2 files changed, 2 deletions(-) > > diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c > index 99073325b0c0..0d61e9cd0887 100644 > --- a/drivers/staging/android/ion/ion.c > +++ b/drivers/staging/android/ion/ion.c > @@ -23,7 +23,6 @@ > #include <linux/mm_types.h> > #include <linux/rbtree.h> > #include <linux/sched/task.h> > -#include <linux/seq_file.h> > #include <linux/slab.h> > #include <linux/uaccess.h> > #include <linux/vmalloc.h> > diff --git a/drivers/staging/android/ion/ion_system_heap.c b/drivers/staging/android/ion/ion_system_heap.c > index 548bb02c0ca6..9ce2c0d7ac17 100644 > --- a/drivers/staging/android/ion/ion_system_heap.c > +++ b/drivers/staging/android/ion/ion_system_heap.c > @@ -11,7 +11,6 @@ > #include <linux/highmem.h> > #include <linux/mm.h> > #include <linux/scatterlist.h> > -#include <linux/seq_file.h> > #include <linux/slab.h> > #include <linux/vmalloc.h> > #include "ion.h" >

7 years, 5 months

Jump to page:

2026

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig