- Linaro-mm-sig - lists.linaro.org

Re: [Linaro-mm-sig] [PATCH RFC 1/1] uio: Add dma-buf import ioctls

by Daniel Vetter

On Tue, Feb 26, 2019 at 11:20 PM Hyun Kwon <hyun.kwon(a)xilinx.com> wrote: > > Hi Daniel, > > Thanks for the comment. > > On Tue, 2019-02-26 at 04:06:13 -0800, Daniel Vetter wrote: > > On Tue, Feb 26, 2019 at 12:53 PM Greg Kroah-Hartman > > <gregkh(a)linuxfoundation.org> wrote: > > > > > > On Sat, Feb 23, 2019 at 12:28:17PM -0800, Hyun Kwon wrote: > > > > Add the dmabuf map / unmap interfaces. This allows the user driver > > > > to be able to import the external dmabuf and use it from user space. > > > > > > > > Signed-off-by: Hyun Kwon <hyun.kwon(a)xilinx.com> > > > > --- > > > > drivers/uio/Makefile | 2 +- > > > > drivers/uio/uio.c | 43 +++++++++ > > > > drivers/uio/uio_dmabuf.c | 210 +++++++++++++++++++++++++++++++++++++++++++ > > > > drivers/uio/uio_dmabuf.h | 26 ++++++ > > > > include/uapi/linux/uio/uio.h | 33 +++++++ > > > > 5 files changed, 313 insertions(+), 1 deletion(-) > > > > create mode 100644 drivers/uio/uio_dmabuf.c > > > > create mode 100644 drivers/uio/uio_dmabuf.h > > > > create mode 100644 include/uapi/linux/uio/uio.h > > > > > > > > diff --git a/drivers/uio/Makefile b/drivers/uio/Makefile > > > > index c285dd2..5da16c7 100644 > > > > --- a/drivers/uio/Makefile > > > > +++ b/drivers/uio/Makefile > > > > @@ -1,5 +1,5 @@ > > > > # SPDX-License-Identifier: GPL-2.0 > > > > -obj-$(CONFIG_UIO) += uio.o > > > > +obj-$(CONFIG_UIO) += uio.o uio_dmabuf.o > > > > obj-$(CONFIG_UIO_CIF) += uio_cif.o > > > > obj-$(CONFIG_UIO_PDRV_GENIRQ) += uio_pdrv_genirq.o > > > > obj-$(CONFIG_UIO_DMEM_GENIRQ) += uio_dmem_genirq.o > > > > diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c > > > > index 1313422..6841f98 100644 > > > > --- a/drivers/uio/uio.c > > > > +++ b/drivers/uio/uio.c > > > > @@ -24,6 +24,12 @@ > > > > #include <linux/kobject.h> > > > > #include <linux/cdev.h> > > > > #include <linux/uio_driver.h> > > > > +#include <linux/list.h> > > > > +#include <linux/mutex.h> > > > > + > > > > +#include <uapi/linux/uio/uio.h> > > > > + > > > > +#include "uio_dmabuf.h" > > > > > > > > #define UIO_MAX_DEVICES (1U << MINORBITS) > > > > > > > > @@ -454,6 +460,8 @@ static irqreturn_t uio_interrupt(int irq, void *dev_id) > > > > struct uio_listener { > > > > struct uio_device *dev; > > > > s32 event_count; > > > > + struct list_head dbufs; > > > > + struct mutex dbufs_lock; /* protect @dbufs */ > > > > }; > > > > > > > > static int uio_open(struct inode *inode, struct file *filep) > > > > @@ -500,6 +508,9 @@ static int uio_open(struct inode *inode, struct file *filep) > > > > if (ret) > > > > goto err_infoopen; > > > > > > > > + INIT_LIST_HEAD(&listener->dbufs); > > > > + mutex_init(&listener->dbufs_lock); > > > > + > > > > return 0; > > > > > > > > err_infoopen: > > > > @@ -529,6 +540,10 @@ static int uio_release(struct inode *inode, struct file *filep) > > > > struct uio_listener *listener = filep->private_data; > > > > struct uio_device *idev = listener->dev; > > > > > > > > + ret = uio_dmabuf_cleanup(idev, &listener->dbufs, &listener->dbufs_lock); > > > > + if (ret) > > > > + dev_err(&idev->dev, "failed to clean up the dma bufs\n"); > > > > + > > > > mutex_lock(&idev->info_lock); > > > > if (idev->info && idev->info->release) > > > > ret = idev->info->release(idev->info, inode); > > > > @@ -652,6 +667,33 @@ static ssize_t uio_write(struct file *filep, const char __user *buf, > > > > return retval ? retval : sizeof(s32); > > > > } > > > > > > > > +static long uio_ioctl(struct file *filep, unsigned int cmd, unsigned long arg) > > > > > > We have resisted adding a uio ioctl for a long time, can't you do this > > > through sysfs somehow? > > > > > > A meta-comment about your ioctl structure: > > > > > > > +#define UIO_DMABUF_DIR_BIDIR 1 > > > > +#define UIO_DMABUF_DIR_TO_DEV 2 > > > > +#define UIO_DMABUF_DIR_FROM_DEV 3 > > > > +#define UIO_DMABUF_DIR_NONE 4 > > > > > > enumerated type? > > > > > > > + > > > > +struct uio_dmabuf_args { > > > > + __s32 dbuf_fd; > > > > + __u64 dma_addr; > > > > + __u64 size; > > > > + __u32 dir; > > > > > > Why the odd alignment? Are you sure this is the best packing for such a > > > structure? > > > > > > Why is dbuf_fd __s32? dir can be __u8, right? > > > > > > I don't know that dma layer very well, it would be good to get some > > > review from others to see if this really is even a viable thing to do. > > > The fd handling seems a bit "odd" here, but maybe I just do not > > > understand it. > > > > Frankly looks like a ploy to sidestep review by graphics folks. We'd > > ask for the userspace first :-) > > Please refer to pull request [1]. > > For any interest in more details, the libmetal is the abstraction layer > which provides platform independent APIs. The backend implementation > can be selected per different platforms: ex, rtos, linux, > standalone (xilinx),,,. For Linux, it supports UIO / vfio as of now. > The actual user space drivers sit on top of libmetal. Such drivers can be > found in [2]. This is why I try to avoid any device specific code in > Linux kernel. > > > > > Also, exporting dma_addr to userspace is considered a very bad idea. > > I agree, hence the RFC to pick some brains. :-) Would it make sense > if this call doesn't export the physicall address, but instead takes > only the dmabuf fd and register offsets to be programmed? > > > If you want to do this properly, you need a minimal in-kernel memory > > manager, and those tend to be based on top of drm_gem.c and merged > > through the gpu tree. The last place where we accidentally leaked a > > dma addr for gpu buffers was in the fbdev code, and we plugged that > > one with > > Could you please help me understand how having a in-kernel memory manager > helps? Isn't it just moving same dmabuf import / paddr export functionality > in different modules: kernel memory manager vs uio. In fact, Xilinx does have > such memory manager based on drm gem in downstream. But for this time we took > the approach of implementing this through generic dmabuf allocator, ION, and > enabling the import capability in the UIO infrastructure instead. There's a group of people working on upstreaming a xilinx drm driver already. Which driver are we talking about? Can you pls provide a link to that xilinx drm driver? Thanks, Daniel > Thanks, > -hyun > > [1] https://github.com/OpenAMP/libmetal/pull/82/commits/951e2762bd487c98919ad12… > [2] https://github.com/Xilinx/embeddedsw/tree/master/XilinxProcessorIPLib/drive… > > > > > commit 4be9bd10e22dfc7fc101c5cf5969ef2d3a042d8a (tag: > > drm-misc-next-fixes-2018-10-03) > > Author: Neil Armstrong <narmstrong(a)baylibre.com> > > Date: Fri Sep 28 14:05:55 2018 +0200 > > > > drm/fb_helper: Allow leaking fbdev smem_start > > > > Together with cuse the above patch should be enough to implement a drm > > driver entirely in userspace at least. > > > > Cheers, Daniel > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch

6 years, 3 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH RFC 1/1] uio: Add dma-buf import ioctls

by Greg Kroah-Hartman

On Sat, Feb 23, 2019 at 12:28:17PM -0800, Hyun Kwon wrote: > Add the dmabuf map / unmap interfaces. This allows the user driver > to be able to import the external dmabuf and use it from user space. > > Signed-off-by: Hyun Kwon <hyun.kwon(a)xilinx.com> > --- > drivers/uio/Makefile | 2 +- > drivers/uio/uio.c | 43 +++++++++ > drivers/uio/uio_dmabuf.c | 210 +++++++++++++++++++++++++++++++++++++++++++ > drivers/uio/uio_dmabuf.h | 26 ++++++ > include/uapi/linux/uio/uio.h | 33 +++++++ > 5 files changed, 313 insertions(+), 1 deletion(-) > create mode 100644 drivers/uio/uio_dmabuf.c > create mode 100644 drivers/uio/uio_dmabuf.h > create mode 100644 include/uapi/linux/uio/uio.h > > diff --git a/drivers/uio/Makefile b/drivers/uio/Makefile > index c285dd2..5da16c7 100644 > --- a/drivers/uio/Makefile > +++ b/drivers/uio/Makefile > @@ -1,5 +1,5 @@ > # SPDX-License-Identifier: GPL-2.0 > -obj-$(CONFIG_UIO) += uio.o > +obj-$(CONFIG_UIO) += uio.o uio_dmabuf.o > obj-$(CONFIG_UIO_CIF) += uio_cif.o > obj-$(CONFIG_UIO_PDRV_GENIRQ) += uio_pdrv_genirq.o > obj-$(CONFIG_UIO_DMEM_GENIRQ) += uio_dmem_genirq.o > diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c > index 1313422..6841f98 100644 > --- a/drivers/uio/uio.c > +++ b/drivers/uio/uio.c > @@ -24,6 +24,12 @@ > #include <linux/kobject.h> > #include <linux/cdev.h> > #include <linux/uio_driver.h> > +#include <linux/list.h> > +#include <linux/mutex.h> > + > +#include <uapi/linux/uio/uio.h> > + > +#include "uio_dmabuf.h" > > #define UIO_MAX_DEVICES (1U << MINORBITS) > > @@ -454,6 +460,8 @@ static irqreturn_t uio_interrupt(int irq, void *dev_id) > struct uio_listener { > struct uio_device *dev; > s32 event_count; > + struct list_head dbufs; > + struct mutex dbufs_lock; /* protect @dbufs */ > }; > > static int uio_open(struct inode *inode, struct file *filep) > @@ -500,6 +508,9 @@ static int uio_open(struct inode *inode, struct file *filep) > if (ret) > goto err_infoopen; > > + INIT_LIST_HEAD(&listener->dbufs); > + mutex_init(&listener->dbufs_lock); > + > return 0; > > err_infoopen: > @@ -529,6 +540,10 @@ static int uio_release(struct inode *inode, struct file *filep) > struct uio_listener *listener = filep->private_data; > struct uio_device *idev = listener->dev; > > + ret = uio_dmabuf_cleanup(idev, &listener->dbufs, &listener->dbufs_lock); > + if (ret) > + dev_err(&idev->dev, "failed to clean up the dma bufs\n"); > + > mutex_lock(&idev->info_lock); > if (idev->info && idev->info->release) > ret = idev->info->release(idev->info, inode); > @@ -652,6 +667,33 @@ static ssize_t uio_write(struct file *filep, const char __user *buf, > return retval ? retval : sizeof(s32); > } > > +static long uio_ioctl(struct file *filep, unsigned int cmd, unsigned long arg) We have resisted adding a uio ioctl for a long time, can't you do this through sysfs somehow? A meta-comment about your ioctl structure: > +#define UIO_DMABUF_DIR_BIDIR 1 > +#define UIO_DMABUF_DIR_TO_DEV 2 > +#define UIO_DMABUF_DIR_FROM_DEV 3 > +#define UIO_DMABUF_DIR_NONE 4 enumerated type? > + > +struct uio_dmabuf_args { > + __s32 dbuf_fd; > + __u64 dma_addr; > + __u64 size; > + __u32 dir; Why the odd alignment? Are you sure this is the best packing for such a structure? Why is dbuf_fd __s32? dir can be __u8, right? I don't know that dma layer very well, it would be good to get some review from others to see if this really is even a viable thing to do. The fd handling seems a bit "odd" here, but maybe I just do not understand it. thanks, greg k-h

6 years, 3 months

2
1
0 0

Re: [Linaro-mm-sig] [PATCH] staging: android: ion: fix sys heap pool's gfp_flags

by Laura Abbott

On 1/31/19 10:59 PM, Qing Xia wrote: > In the first loop, gfp_flags will be modified to high_order_gfp_flags, > and there will be no chance to change back to low_order_gfp_flags. > > Fixes: e7f63771 ("ION: Sys_heap: Add cached pool to spead up cached buffer alloc") > Signed-off-by: Qing Xia <saberlily.xia(a)hisilicon.com> > --- > drivers/staging/android/ion/ion_system_heap.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/android/ion/ion_system_heap.c b/drivers/staging/android/ion/ion_system_heap.c > index 0383f75..20f2103 100644 > --- a/drivers/staging/android/ion/ion_system_heap.c > +++ b/drivers/staging/android/ion/ion_system_heap.c > @@ -223,10 +223,10 @@ static void ion_system_heap_destroy_pools(struct ion_page_pool **pools) > static int ion_system_heap_create_pools(struct ion_page_pool **pools) > { > int i; > - gfp_t gfp_flags = low_order_gfp_flags; > > for (i = 0; i < NUM_ORDERS; i++) { > struct ion_page_pool *pool; > + gfp_t gfp_flags = low_order_gfp_flags; > > if (orders[i] > 4) > gfp_flags = high_order_gfp_flags; > Acked-by: Laura Abbott <labbott(a)redhat.com>

6 years, 3 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH 1/1] staging: android: ion: Add the GPL exception for syscalls

by Greg Kroah-Hartman

On Wed, Feb 13, 2019 at 04:01:46PM -0800, Hyun Kwon wrote: > Add "WITH Linux-syscall-note" to the license to not put GPL > restrictions on user space programs using this header. > > Signed-off-by: Hyun Kwon <hyun.kwon(a)xilinx.com> > --- > drivers/staging/android/uapi/ion.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/android/uapi/ion.h b/drivers/staging/android/uapi/ion.h > index 5d70098..46c93fc 100644 > --- a/drivers/staging/android/uapi/ion.h > +++ b/drivers/staging/android/uapi/ion.h > @@ -1,4 +1,4 @@ > -/* SPDX-License-Identifier: GPL-2.0 */ > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ > /* > * drivers/staging/android/uapi/ion.h > * > -- > 2.7.4 > Yes, that is the correct thing to do, let me go queue this up. thanks, greg k-h

6 years, 3 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH] staging: android: ion: Use low_order_gfp_flags for smaller allocations

by Laura Abbott

On 2/11/19 11:09 PM, Jing Xia wrote: > gfp_flags is always set high_order_gfp_flags even if allocations of > order 0 are made.But for smaller allocations, the system should be able > to reclaim some memory. > > Signed-off-by: Jing Xia <jing.xia(a)unisoc.com> > Reviewed-by: Yuming Han <yuming.han(a)unisoc.com> > Reviewed-by: Zhaoyang Huang <zhaoyang.huang(a)unisoc.com> > Reviewed-by: Orson Zhai <orson.zhai(a)unisoc.com> > --- > drivers/staging/android/ion/ion_system_heap.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/android/ion/ion_system_heap.c b/drivers/staging/android/ion/ion_system_heap.c > index 0383f75..20f2103 100644 > --- a/drivers/staging/android/ion/ion_system_heap.c > +++ b/drivers/staging/android/ion/ion_system_heap.c > @@ -223,10 +223,10 @@ static void ion_system_heap_destroy_pools(struct ion_page_pool **pools) > static int ion_system_heap_create_pools(struct ion_page_pool **pools) > { > int i; > - gfp_t gfp_flags = low_order_gfp_flags; > > for (i = 0; i < NUM_ORDERS; i++) { > struct ion_page_pool *pool; > + gfp_t gfp_flags = low_order_gfp_flags; > > if (orders[i] > 4) > gfp_flags = high_order_gfp_flags; > This was already submitted in https://lore.kernel.org/lkml/1549004386-38778-1-git-send-email-saberlily.xi… (I'm also very behind on Ion e-mail and need to catch up...) Laura

6 years, 3 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH] staging: android: ion: fix sys heap pool's gfp_flags

by Dan Carpenter

On Fri, Feb 01, 2019 at 02:59:46PM +0800, Qing Xia wrote: > In the first loop, gfp_flags will be modified to high_order_gfp_flags, > and there will be no chance to change back to low_order_gfp_flags. > > Fixes: e7f63771 ("ION: Sys_heap: Add cached pool to spead up cached buffer alloc") Huh... Presumably you found this bug just by reading the code. I wonder how it affects runtime? regards, dan carpenter

6 years, 4 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH] staging: android: ion: fix sys heap pool's gfp_flags

by Chen Feng

Thanks. On 2019/2/1 14:59, Qing Xia wrote: > In the first loop, gfp_flags will be modified to high_order_gfp_flags, > and there will be no chance to change back to low_order_gfp_flags. > > Fixes: e7f63771 ("ION: Sys_heap: Add cached pool to spead up cached buffer alloc") > Signed-off-by: Qing Xia <saberlily.xia(a)hisilicon.com> > --- > drivers/staging/android/ion/ion_system_heap.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/android/ion/ion_system_heap.c b/drivers/staging/android/ion/ion_system_heap.c > index 0383f75..20f2103 100644 > --- a/drivers/staging/android/ion/ion_system_heap.c > +++ b/drivers/staging/android/ion/ion_system_heap.c > @@ -223,10 +223,10 @@ static void ion_system_heap_destroy_pools(struct ion_page_pool **pools) > static int ion_system_heap_create_pools(struct ion_page_pool **pools) > { > int i; > - gfp_t gfp_flags = low_order_gfp_flags; > > for (i = 0; i < NUM_ORDERS; i++) { > struct ion_page_pool *pool; > + gfp_t gfp_flags = low_order_gfp_flags; > > if (orders[i] > 4) > gfp_flags = high_order_gfp_flags; > -- Cheers, - Feng

6 years, 4 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH 3/4] dma-buf: add support for mapping with dma mapping attributes

by Liam Mark

On Tue, 22 Jan 2019, Andrew F. Davis wrote: > On 1/21/19 4:12 PM, Liam Mark wrote: > > On Mon, 21 Jan 2019, Christoph Hellwig wrote: > > > >> On Mon, Jan 21, 2019 at 11:44:10AM -0800, Liam Mark wrote: > >>> The main use case is for allowing clients to pass in > >>> DMA_ATTR_SKIP_CPU_SYNC in order to skip the default cache maintenance > >>> which happens in dma_buf_map_attachment and dma_buf_unmap_attachment. In > >>> ION the buffers aren't usually accessed from the CPU so this allows > >>> clients to often avoid doing unnecessary cache maintenance. > >> > >> This can't work. The cpu can still easily speculate into this area. > > > > Can you provide more detail on your concern here. > > The use case I am thinking about here is a cached buffer which is accessed > > by a non IO-coherent device (quite a common use case for ION). > > > > Guessing on your concern: > > The speculative access can be an issue if you are going to access the > > buffer from the CPU after the device has written to it, however if you > > know you aren't going to do any CPU access before the buffer is again > > returned to the device then I don't think the speculative access is a > > concern. > > > >> Moreover in general these operations should be cheap if the addresses > >> aren't cached. > >> > > > > I am thinking of use cases with cached buffers here, so CMO isn't cheap. > > > > These buffers are cacheable, not cached, if you haven't written anything > the data wont actually be in cache. That's true > And in the case of speculative cache > filling the lines are marked clean. In either case the only cost is the > little 7 instruction loop calling the clean/invalidate instruction (dc > civac for ARMv8) for the cache-lines. Unless that is the cost you are > trying to avoid? > This is the cost I am trying to avoid and this comes back to our previous discussion. We have a coherent system cache so if you are doing this for every cache line on a large buffer it adds up with this work and the going to the bus. For example I believe 1080P buffers are 8MB, and 4K buffers are even larger. I also still think you would want to solve this properly such that invalidates aren't being done unnecessarily. > In that case if you are mapping and unmapping so much that the little > CMO here is hurting performance then I would argue your usage is broken > and needs to be re-worked a bit. > I am not sure I would say it is broken, the large buffers (example 1080P buffers) are mapped and unmapped on every frame. I don't think there is any clean way to avoid that in a pipelining framework, you could ask clients to keep the buffers dma mapped but there isn't necessarily a good time to tell them to unmap. It would be unfortunate to not consider this something legitimate for usespace to do in a pipelining use case. Requiring devices to stay attached doesn't seem very clean to me as there isn't necessarily a nice place to tell them when to detach. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

6 years, 4 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH 3/4] dma-buf: add support for mapping with dma mapping attributes

by Liam Mark

On Tue, 22 Jan 2019, Andrew F. Davis wrote: > On 1/21/19 4:18 PM, Liam Mark wrote: > > On Mon, 21 Jan 2019, Andrew F. Davis wrote: > > > >> On 1/21/19 2:20 PM, Liam Mark wrote: > >>> On Mon, 21 Jan 2019, Andrew F. Davis wrote: > >>> > >>>> On 1/21/19 1:44 PM, Liam Mark wrote: > >>>>> On Mon, 21 Jan 2019, Christoph Hellwig wrote: > >>>>> > >>>>>> On Sat, Jan 19, 2019 at 08:50:41AM -0800, Laura Abbott wrote: > >>>>>>>> And who is going to decide which ones to pass? And who documents > >>>>>>>> which ones are safe? > >>>>>>>> > >>>>>>>> I'd much rather have explicit, well documented dma-buf flags that > >>>>>>>> might get translated to the DMA API flags, which are not error checked, > >>>>>>>> not very well documented and way to easy to get wrong. > >>>>>>>> > >>>>>>> > >>>>>>> I'm not sure having flags in dma-buf really solves anything > >>>>>>> given drivers can use the attributes directly with dma_map > >>>>>>> anyway, which is what we're looking to do. The intention > >>>>>>> is for the driver creating the dma_buf attachment to have > >>>>>>> the knowledge of which flags to use. > >>>>>> > >>>>>> Well, there are very few flags that you can simply use for all calls of > >>>>>> dma_map*. And given how badly these flags are defined I just don't want > >>>>>> people to add more places where they indirectly use these flags, as > >>>>>> it will be more than enough work to clean up the current mess. > >>>>>> > >>>>>> What flag(s) do you want to pass this way, btw? Maybe that is where > >>>>>> the problem is. > >>>>>> > >>>>> > >>>>> The main use case is for allowing clients to pass in > >>>>> DMA_ATTR_SKIP_CPU_SYNC in order to skip the default cache maintenance > >>>>> which happens in dma_buf_map_attachment and dma_buf_unmap_attachment. In > >>>>> ION the buffers aren't usually accessed from the CPU so this allows > >>>>> clients to often avoid doing unnecessary cache maintenance. > >>>>> > >>>> > >>>> How can a client know that no CPU access has occurred that needs to be > >>>> flushed out? > >>>> > >>> > >>> I have left this to clients, but if they own the buffer they can have the > >>> knowledge as to whether CPU access is needed in that use case (example for > >>> post-processing). > >>> > >>> For example with the previous version of ION we left all decisions of > >>> whether cache maintenance was required up to the client, they would use > >>> the ION cache maintenance IOCTL to force cache maintenance only when it > >>> was required. > >>> In these cases almost all of the access was being done by the device and > >>> in the rare cases CPU access was required clients would initiate the > >>> required cache maintenance before and after the CPU access. > >>> > >> > >> I think we have different definitions of "client", I'm talking about the > >> DMA-BUF client (the importer), that is who can set this flag. It seems > >> you mean the userspace application, which has no control over this flag. > >> > > > > I am also talking about dma-buf clients, I am referring to both the > > userspace and kernel component of the client. For example our Camera ION > > client has both a usersapce and kernel component and they have ION > > buffers, which they control the access to, which may or may not be > > accessed by the CPU in certain uses cases. > > > > I know they often work together, but for this discussion it would be > good to keep kernel clients and usperspace clients separate. There are > three types of actors at play here, userspace clients, kernel clients, > and exporters. > > DMA-BUF only provides the basic sync primitive + mmap directly to > userspace, Well dma-buf does provide dma_buf_kmap/dma_buf_begin_cpu_access which allows the same fucntionality in the kernel, but I don't think that changes your argument. > both operations are fulfilled by the exporter. This patch is > about adding more control to the kernel side clients. The kernel side > clients cannot know what userspace or other kernel side clients have > done with the buffer, *only* the exporter has the whole picture. > > Therefor neither type of client should be deciding if the CPU needs > flushed or not, only the exporter, based on the type of buffer, the > current set attachments, and previous actions (is this first attachment, > CPU get access in-between, etc...) can make this decision. > > You goal seems to be to avoid unneeded CPU side CMOs when a device > detaches and another attaches with no CPU access in-between, right? > That's reasonable to me, but it must be the exporter who keeps track and > skips the CMO. This patch allows the client to tell the exporter the CMO > is not needed and that is not safe. > I agree it would be better have this logic in the exporter, but I just haven't heard an upstreamable way to make that work. But maybe to explore that a bit more. If we consider having CPU access with no devices attached a legitimate use case: The pipelining use case I am thinking of is 1) dev 1 attach, map, access, unmap 2) dev 1 detach 3) (maybe) CPU access 4) dev 2 attach 5) dev 2 map, access 6) ... It would be unfortunate to not consider this something legitimate for userspace to do in a pipelining use case. Requiring devices to stay attached doesn't seem very clean to me as there isn't necessarily a nice place to tell them when to detach. If we considered the above a supported use case I think we could support it in dma-buf (based on past discussions) if we had 2 things #1 if we tracked the state of the buffer (example if it has had a previous cached/uncached write and no following CMO). Then when either the CPU or a device was going to access a buffer it could decide, based on the previous access if any CMO needs to be applied first. #2 we had a non-architecture specific way to apply cache maintenance without a device, so that in step #3 the begin_cpu_acess call could successfully invalidate the buffer. I think #1 is doable since we can tell tell if devices are IO coherent or not and we know the direction of accesses in dma map and begin cpu access. I think we would probably agree that #2 is a problem though, getting the kernel to expose that API seems like a hard argument. Liam Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

6 years, 4 months

1
0
0 0

[PATCH 0/4] ION stability and perf changes

by Liam Mark

Some stability changes to improve ION robustness and a perf related change to make it easier for clients to avoid unnecessary cache maintenance, such as when buffers are clean and haven't had any CPU access. Liam Mark (4): staging: android: ion: Support cpu access during dma_buf_detach staging: android: ion: Restrict cache maintenance to dma mapped memory dma-buf: add support for mapping with dma mapping attributes staging: android: ion: Support for mapping with dma mapping attributes drivers/staging/android/ion/ion.c | 33 +++++++++++++++++++++++++-------- include/linux/dma-buf.h | 3 +++ 2 files changed, 28 insertions(+), 8 deletions(-) -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

6 years, 4 months

4
16
0 0

Re: [Linaro-mm-sig] [PATCH 3/4] dma-buf: add support for mapping with dma mapping attributes

by Liam Mark

On Mon, 21 Jan 2019, Andrew F. Davis wrote: > On 1/21/19 2:20 PM, Liam Mark wrote: > > On Mon, 21 Jan 2019, Andrew F. Davis wrote: > > > >> On 1/21/19 1:44 PM, Liam Mark wrote: > >>> On Mon, 21 Jan 2019, Christoph Hellwig wrote: > >>> > >>>> On Sat, Jan 19, 2019 at 08:50:41AM -0800, Laura Abbott wrote: > >>>>>> And who is going to decide which ones to pass? And who documents > >>>>>> which ones are safe? > >>>>>> > >>>>>> I'd much rather have explicit, well documented dma-buf flags that > >>>>>> might get translated to the DMA API flags, which are not error checked, > >>>>>> not very well documented and way to easy to get wrong. > >>>>>> > >>>>> > >>>>> I'm not sure having flags in dma-buf really solves anything > >>>>> given drivers can use the attributes directly with dma_map > >>>>> anyway, which is what we're looking to do. The intention > >>>>> is for the driver creating the dma_buf attachment to have > >>>>> the knowledge of which flags to use. > >>>> > >>>> Well, there are very few flags that you can simply use for all calls of > >>>> dma_map*. And given how badly these flags are defined I just don't want > >>>> people to add more places where they indirectly use these flags, as > >>>> it will be more than enough work to clean up the current mess. > >>>> > >>>> What flag(s) do you want to pass this way, btw? Maybe that is where > >>>> the problem is. > >>>> > >>> > >>> The main use case is for allowing clients to pass in > >>> DMA_ATTR_SKIP_CPU_SYNC in order to skip the default cache maintenance > >>> which happens in dma_buf_map_attachment and dma_buf_unmap_attachment. In > >>> ION the buffers aren't usually accessed from the CPU so this allows > >>> clients to often avoid doing unnecessary cache maintenance. > >>> > >> > >> How can a client know that no CPU access has occurred that needs to be > >> flushed out? > >> > > > > I have left this to clients, but if they own the buffer they can have the > > knowledge as to whether CPU access is needed in that use case (example for > > post-processing). > > > > For example with the previous version of ION we left all decisions of > > whether cache maintenance was required up to the client, they would use > > the ION cache maintenance IOCTL to force cache maintenance only when it > > was required. > > In these cases almost all of the access was being done by the device and > > in the rare cases CPU access was required clients would initiate the > > required cache maintenance before and after the CPU access. > > > > I think we have different definitions of "client", I'm talking about the > DMA-BUF client (the importer), that is who can set this flag. It seems > you mean the userspace application, which has no control over this flag. > I am also talking about dma-buf clients, I am referring to both the userspace and kernel component of the client. For example our Camera ION client has both a usersapce and kernel component and they have ION buffers, which they control the access to, which may or may not be accessed by the CPU in certain uses cases. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

6 years, 4 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH 3/4] dma-buf: add support for mapping with dma mapping attributes

by Liam Mark

On Mon, 21 Jan 2019, Andrew F. Davis wrote: > On 1/21/19 1:44 PM, Liam Mark wrote: > > On Mon, 21 Jan 2019, Christoph Hellwig wrote: > > > >> On Sat, Jan 19, 2019 at 08:50:41AM -0800, Laura Abbott wrote: > >>>> And who is going to decide which ones to pass? And who documents > >>>> which ones are safe? > >>>> > >>>> I'd much rather have explicit, well documented dma-buf flags that > >>>> might get translated to the DMA API flags, which are not error checked, > >>>> not very well documented and way to easy to get wrong. > >>>> > >>> > >>> I'm not sure having flags in dma-buf really solves anything > >>> given drivers can use the attributes directly with dma_map > >>> anyway, which is what we're looking to do. The intention > >>> is for the driver creating the dma_buf attachment to have > >>> the knowledge of which flags to use. > >> > >> Well, there are very few flags that you can simply use for all calls of > >> dma_map*. And given how badly these flags are defined I just don't want > >> people to add more places where they indirectly use these flags, as > >> it will be more than enough work to clean up the current mess. > >> > >> What flag(s) do you want to pass this way, btw? Maybe that is where > >> the problem is. > >> > > > > The main use case is for allowing clients to pass in > > DMA_ATTR_SKIP_CPU_SYNC in order to skip the default cache maintenance > > which happens in dma_buf_map_attachment and dma_buf_unmap_attachment. In > > ION the buffers aren't usually accessed from the CPU so this allows > > clients to often avoid doing unnecessary cache maintenance. > > > > How can a client know that no CPU access has occurred that needs to be > flushed out? > I have left this to clients, but if they own the buffer they can have the knowledge as to whether CPU access is needed in that use case (example for post-processing). For example with the previous version of ION we left all decisions of whether cache maintenance was required up to the client, they would use the ION cache maintenance IOCTL to force cache maintenance only when it was required. In these cases almost all of the access was being done by the device and in the rare cases CPU access was required clients would initiate the required cache maintenance before and after the CPU access. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

6 years, 4 months

2
2
0 0

[PATCH v4 0/4] locking, drm: Fix ww mutex naming / algorithm inconsistency

by Thomas Hellstrom

This is a small fallout from a work to allow batching WW mutex locks and unlocks. Our Wound-Wait mutexes actually don't use the Wound-Wait algorithm but the Wait-Die algorithm. One could perhaps rename those mutexes tree-wide to "Wait-Die mutexes" or "Deadlock Avoidance mutexes". Another approach suggested here is to implement also the "Wound-Wait" algorithm as a per-WW-class choice, as it has advantages in some cases. See for example http://www.mathcs.emory.edu/~cheung/Courses/554/Syllabus/8-recv+serial/dead… Now Wound-Wait is a preemptive algorithm, and the preemption is implemented using a lazy scheme: If a wounded transaction is about to go to sleep on a contended WW mutex, we return -EDEADLK. That is sufficient for deadlock prevention. Since with WW mutexes we also require the aborted transaction to sleep waiting to lock the WW mutex it was aborted on, this choice also provides a suitable WW mutex to sleep on. If we were to return -EDEADLK on the first WW mutex lock after the transaction was wounded whether the WW mutex was contended or not, the transaction might frequently be restarted without a wait, which is far from optimal. Note also that with the lazy preemption scheme, contrary to Wait-Die there will be no rollbacks on lock contention of locks held by a transaction that has completed its locking sequence. The modeset locks are then changed from Wait-Die to Wound-Wait since the typical locking pattern of those locks very well matches the criterion for a substantial reduction in the number of rollbacks. For reservation objects, the benefit is more unclear at this point and they remain using Wait-Die. Cc: Ingo Molnar <mingo(a)redhat.com> Cc: Jonathan Corbet <corbet(a)lwn.net> Cc: Gustavo Padovan <gustavo(a)padovan.org> Cc: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com> Cc: Sean Paul <seanpaul(a)chromium.org> Cc: David Airlie <airlied(a)linux.ie> Cc: Davidlohr Bueso <dave(a)stgolabs.net> Cc: "Paul E. McKenney" <paulmck(a)linux.vnet.ibm.com> Cc: Josh Triplett <josh(a)joshtriplett.org> Cc: Thomas Gleixner <tglx(a)linutronix.de> Cc: Kate Stewart <kstewart(a)linuxfoundation.org> Cc: Philippe Ombredanne <pombredanne(a)nexb.com> Cc: Greg Kroah-Hartman <gregkh(a)linuxfoundation.org> Cc: linux-doc(a)vger.kernel.org Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org v2: Updated DEFINE_WW_CLASS() API, minor code- and comment fixes as detailed in each patch. v3: Included cleanup patch from Peter Zijlstra. Documentation fixes and a small correctness fix. v4: Reworked the correctness fix.

6 years, 4 months

3
11
0 0

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

6 years, 5 months

1
0
0 0

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

6 years, 5 months

1
0
0 0

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

6 years, 5 months

1
0
0 0

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

6 years, 5 months

1
0
0 0

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

6 years, 5 months

1
0
0 0

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 >

6 years, 5 months

1
0
0 0

[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

6 years, 6 months

3
2
0 0

[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

6 years, 6 months

2
1
0 0

[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

6 years, 6 months

3
4
0 0

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

6 years, 6 months

1
0
0 0

[RFC] android: ion: How to properly clean caches for uncached allocations

by Liam Mark

The issue: Currently in ION if you allocate uncached memory it is possible that there are still dirty lines in the cache. And often these dirty lines in the cache are the zeros which were meant to clear out any sensitive kernel data. What this means is that if you allocate uncached memory from ION, and then subsequently write to that buffer (using the uncached mapping you are provided by ION) then the data you have written could be corrupted at some point in the future if a dirty line is evicted from the cache. Also this means there is a potential security issue. If an un-privileged userspace user allocated uncached memory (for example from the system heap) and then if they were to read from that buffer (through the un-cached mapping they are provided by ION), and if some of the zeros which were written to that memory are still in the cache then this un-privileged userspace user could read potentially sensitive kernel data. An unacceptable fix: I have included some sample code which should resolve this issue for the system heap and the cma heap on some architectures, however this code would not be acceptable for upstreaming since it uses hacks to clean the cache. Similar changes would need to be made to carveout heap and chunk heap. I would appreciate some feedback on the proper way for ION to clean the caches for memory it has allocated that is intended for uncached access. I realize that it may be tempting, as a solution to this problem, to simply strip uncached support from ION. I hope that we can try to find a solution which preserves uncached memory support as ION uncached memory is often used (though perhaps not in upstreamed code) in cases such as multimedia use cases where there is no CPU access required, in secure heap allocations, and in some cases where there is minimal CPU access and therefore uncached memory performs better. Signed-off-by: Liam Mark <lmark(a)codeaurora.org> --- drivers/staging/android/ion/ion.c | 16 ++++++++++++++++ drivers/staging/android/ion/ion.h | 5 ++++- drivers/staging/android/ion/ion_cma_heap.c | 3 +++ drivers/staging/android/ion/ion_page_pool.c | 8 ++++++-- drivers/staging/android/ion/ion_system_heap.c | 7 ++++++- 5 files changed, 35 insertions(+), 4 deletions(-) diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c index 57e0d8035b2e..10e967b0a0f4 100644 --- a/drivers/staging/android/ion/ion.c +++ b/drivers/staging/android/ion/ion.c @@ -38,6 +38,22 @@ bool ion_buffer_cached(struct ion_buffer *buffer) return !!(buffer->flags & ION_FLAG_CACHED); } +void ion_pages_sync_for_device(struct page *page, size_t size, + enum dma_data_direction dir) +{ + struct scatterlist sg; + struct device dev = {0}; + + /* hack, use dummy device */ + arch_setup_dma_ops(&dev, 0, 0, NULL, false); + + sg_init_table(&sg, 1); + sg_set_page(&sg, page, size, 0); + /* hack, use phys address for dma address */ + sg_dma_address(&sg) = page_to_phys(page); + dma_sync_sg_for_device(&dev, &sg, 1, dir); +} + /* this function should only be called while dev->lock is held */ static void ion_buffer_add(struct ion_device *dev, struct ion_buffer *buffer) diff --git a/drivers/staging/android/ion/ion.h b/drivers/staging/android/ion/ion.h index a238f23c9116..227b9928d185 100644 --- a/drivers/staging/android/ion/ion.h +++ b/drivers/staging/android/ion/ion.h @@ -192,6 +192,9 @@ struct ion_heap { */ bool ion_buffer_cached(struct ion_buffer *buffer); +void ion_pages_sync_for_device(struct page *page, size_t size, + enum dma_data_direction dir); + /** * ion_buffer_fault_user_mappings - fault in user mappings of this buffer * @buffer: buffer @@ -333,7 +336,7 @@ struct ion_page_pool { struct ion_page_pool *ion_page_pool_create(gfp_t gfp_mask, unsigned int order, bool cached); void ion_page_pool_destroy(struct ion_page_pool *pool); -struct page *ion_page_pool_alloc(struct ion_page_pool *pool); +struct page *ion_page_pool_alloc(struct ion_page_pool *pool, bool *from_pool); void ion_page_pool_free(struct ion_page_pool *pool, struct page *page); /** ion_page_pool_shrink - shrinks the size of the memory cached in the pool diff --git a/drivers/staging/android/ion/ion_cma_heap.c b/drivers/staging/android/ion/ion_cma_heap.c index 49718c96bf9e..82e80621d114 100644 --- a/drivers/staging/android/ion/ion_cma_heap.c +++ b/drivers/staging/android/ion/ion_cma_heap.c @@ -59,6 +59,9 @@ static int ion_cma_allocate(struct ion_heap *heap, struct ion_buffer *buffer, memset(page_address(pages), 0, size); } + if (!ion_buffer_cached(buffer)) + ion_pages_sync_for_device(pages, size, DMA_BIDIRECTIONAL); + table = kmalloc(sizeof(*table), GFP_KERNEL); if (!table) goto err; diff --git a/drivers/staging/android/ion/ion_page_pool.c b/drivers/staging/android/ion/ion_page_pool.c index b3017f12835f..169a321778ed 100644 --- a/drivers/staging/android/ion/ion_page_pool.c +++ b/drivers/staging/android/ion/ion_page_pool.c @@ -63,7 +63,7 @@ static struct page *ion_page_pool_remove(struct ion_page_pool *pool, bool high) return page; } -struct page *ion_page_pool_alloc(struct ion_page_pool *pool) +struct page *ion_page_pool_alloc(struct ion_page_pool *pool, bool *from_pool) { struct page *page = NULL; @@ -76,8 +76,12 @@ struct page *ion_page_pool_alloc(struct ion_page_pool *pool) page = ion_page_pool_remove(pool, false); mutex_unlock(&pool->mutex); - if (!page) + if (!page) { page = ion_page_pool_alloc_pages(pool); + *from_pool = false; + } else { + *from_pool = true; + } return page; } diff --git a/drivers/staging/android/ion/ion_system_heap.c b/drivers/staging/android/ion/ion_system_heap.c index bc19cdd30637..3bb4604e032b 100644 --- a/drivers/staging/android/ion/ion_system_heap.c +++ b/drivers/staging/android/ion/ion_system_heap.c @@ -57,13 +57,18 @@ static struct page *alloc_buffer_page(struct ion_system_heap *heap, bool cached = ion_buffer_cached(buffer); struct ion_page_pool *pool; struct page *page; + bool from_pool; if (!cached) pool = heap->uncached_pools[order_to_index(order)]; else pool = heap->cached_pools[order_to_index(order)]; - page = ion_page_pool_alloc(pool); + page = ion_page_pool_alloc(pool, &from_pool); + + if (!from_pool && !ion_buffer_cached(buffer)) + ion_pages_sync_for_device(page, PAGE_SIZE << order, + DMA_BIDIRECTIONAL); return page; } -- 1.8.5.2 Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

6 years, 6 months

4
13
0 0

Re: [Linaro-mm-sig] [PATCH] udmabuf: set read/write flag when exporting

by Gerd Hoffmann

On Thu, Nov 15, 2018 at 05:46:30PM -0800, Gurchetan Singh wrote: > Reviewed-by: Gurchetan Singh <gurchetansingh(a)chromium.org> > Tested-by: Gurchetan Singh <gurchetansingh(a)chromium.org> Thanks. Pushed to drm-misc-fixes. cheers, Gerd

6 years, 6 months

1
0
0 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig