Linaro-mm-sig February 2019

linaro-mm-sig@lists.linaro.org

8 participants
9 discussions

Re: [Linaro-mm-sig] [PATCH 2/4] staging: android: ion: Restrict cache maintenance to dma mapped memory

by Liam Mark

On Fri, 18 Jan 2019, Andrew F. Davis wrote: > On 1/18/19 12:37 PM, Liam Mark wrote: > > The ION begin_cpu_access and end_cpu_access functions use the > > dma_sync_sg_for_cpu and dma_sync_sg_for_device APIs to perform cache > > maintenance. > > > > Currently it is possible to apply cache maintenance, via the > > begin_cpu_access and end_cpu_access APIs, to ION buffers which are not > > dma mapped. > > > > The dma sync sg APIs should not be called on sg lists which have not been > > dma mapped as this can result in cache maintenance being applied to the > > wrong address. If an sg list has not been dma mapped then its dma_address > > field has not been populated, some dma ops such as the swiotlb_dma_ops ops > > use the dma_address field to calculate the address onto which to apply > > cache maintenance. > > > > Also I don’t think we want CMOs to be applied to a buffer which is not > > dma mapped as the memory should already be coherent for access from the > > CPU. Any CMOs required for device access taken care of in the > > dma_buf_map_attachment and dma_buf_unmap_attachment calls. > > So really it only makes sense for begin_cpu_access and end_cpu_access to > > apply CMOs if the buffer is dma mapped. > > > > Fix the ION begin_cpu_access and end_cpu_access functions to only apply > > cache maintenance to buffers which are dma mapped. > > > > Fixes: 2a55e7b5e544 ("staging: android: ion: Call dma_map_sg for syncing and mapping") > > Signed-off-by: Liam Mark <lmark(a)codeaurora.org> > > --- > > drivers/staging/android/ion/ion.c | 26 +++++++++++++++++++++----- > > 1 file changed, 21 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c > > index 6f5afab7c1a1..1fe633a7fdba 100644 > > --- a/drivers/staging/android/ion/ion.c > > +++ b/drivers/staging/android/ion/ion.c > > @@ -210,6 +210,7 @@ struct ion_dma_buf_attachment { > > struct device *dev; > > struct sg_table *table; > > struct list_head list; > > + bool dma_mapped; > > }; > > > > static int ion_dma_buf_attach(struct dma_buf *dmabuf, > > @@ -231,6 +232,7 @@ static int ion_dma_buf_attach(struct dma_buf *dmabuf, > > > > a->table = table; > > a->dev = attachment->dev; > > + a->dma_mapped = false; > > INIT_LIST_HEAD(&a->list); > > > > attachment->priv = a; > > @@ -261,12 +263,18 @@ static struct sg_table *ion_map_dma_buf(struct dma_buf_attachment *attachment, > > { > > struct ion_dma_buf_attachment *a = attachment->priv; > > struct sg_table *table; > > + struct ion_buffer *buffer = attachment->dmabuf->priv; > > > > table = a->table; > > > > + mutex_lock(&buffer->lock); > > if (!dma_map_sg(attachment->dev, table->sgl, table->nents, > > - direction)) > > + direction)) { > > + mutex_unlock(&buffer->lock); > > return ERR_PTR(-ENOMEM); > > + } > > + a->dma_mapped = true; > > + mutex_unlock(&buffer->lock); > > > > return table; > > } > > @@ -275,7 +283,13 @@ static void ion_unmap_dma_buf(struct dma_buf_attachment *attachment, > > struct sg_table *table, > > enum dma_data_direction direction) > > { > > + struct ion_dma_buf_attachment *a = attachment->priv; > > + struct ion_buffer *buffer = attachment->dmabuf->priv; > > + > > + mutex_lock(&buffer->lock); > > dma_unmap_sg(attachment->dev, table->sgl, table->nents, direction); > > + a->dma_mapped = false; > > + mutex_unlock(&buffer->lock); > > } > > > > static int ion_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma) > > @@ -346,8 +360,9 @@ static int ion_dma_buf_begin_cpu_access(struct dma_buf *dmabuf, > > > > mutex_lock(&buffer->lock); > > list_for_each_entry(a, &buffer->attachments, list) { > > When no devices are attached then buffer->attachments is empty and the > below does not run, so if I understand this patch correctly then what > you are protecting against is CPU access in the window after > dma_buf_attach but before dma_buf_map. > Yes > This is the kind of thing that again makes me think a couple more > ordering requirements on DMA-BUF ops are needed. DMA-BUFs do not require > the backing memory to be allocated until map time, this is why the > dma_address field would still be null as you note in the commit message. > So why should the CPU be performing accesses on a buffer that is not > actually backed yet? > > I can think of two solutions: > > 1) Only allow CPU access (mmap, kmap, {begin,end}_cpu_access) while at > least one device is mapped. > Would be quite limiting to clients. > 2) Treat the CPU access request like the a device map request and > trigger the allocation of backing memory just like if a device map had > come in. > Which is, as you mention pretty much what we have now (though the buffer is allocated even earlier). > I know the current Ion heaps (and most other DMA-BUF exporters) all do > the allocation up front so the memory is already there, but DMA-BUF was > designed with late allocation in mind. I have a use-case I'm working on > that finally exercises this DMA-BUF functionality and I would like to > have it export through ION. This patch doesn't prevent that, but seems > like it is endorsing the the idea that buffers always need to be backed, > even before device attach/map is has occurred. > I didn't interpret the DMA-buf contract as requiring the dma-map to be called in order for a backing store to be provided, I interpreted it as meaning there could be a backing store before the dma-map but at the dma-map call the final backing store configuration would be decided (perhaps involving migrating the memory to the final backing store). I will let the dma-buf experts correct me on that. Limiting userspace clients to not be able to access buffers until after they are dma-mapped seems unfortuntate to me, dma-mapping usually means a change of ownership of the memory from the CPU to the device. So generally while a buffer is dma mapped you have the device access it (though of course it is supported for CPU to access to the buffer while dma mapped) and then once the buffer is dma-unmapped the CPU can access it. This is how the DMA APIs are frequently used, and the changes above make ION align more with the way the DMA APIs are used. Basically when the buffer is not dma-mapped the CPU doesn't need to do any CMOs to access the buffer (and ION ensures not CMOs are applied) but if the CPU does want to access the buffer while it is dma mapped then ION ensures that the appropriate CMOs are applied. It seems like a legitimate uses case to me to allow clients to access the buffer before (and after) dma-mapping, example post processing of buffers. > Either of the above two solutions would need to target the DMA-BUF > framework, > > Sumit, > > Any comment? > > Thanks, > Andrew > > > - dma_sync_sg_for_cpu(a->dev, a->table->sgl, a->table->nents, > > - direction); > > + if (a->dma_mapped) > > + dma_sync_sg_for_cpu(a->dev, a->table->sgl, > > + a->table->nents, direction); > > } > > > > unlock: > > @@ -369,8 +384,9 @@ static int ion_dma_buf_end_cpu_access(struct dma_buf *dmabuf, > > > > 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, > > - direction); > > + if (a->dma_mapped) > > + dma_sync_sg_for_device(a->dev, a->table->sgl, > > + a->table->nents, direction); > > } > > mutex_unlock(&buffer->lock); > > > > > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

5 years, 9 months

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

by Daniel Vetter

On Wed, Feb 27, 2019 at 04:36:06PM -0800, Hyun Kwon wrote: > Hi Daniel, > > On Wed, 2019-02-27 at 06:13:45 -0800, Daniel Vetter wrote: > > 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? > > > > The one I was pushing [1] is implemented purely for display, and not > intended for anything other than that as of now. What I'm refering to above > is part of Xilinx FPGA (acceleration) runtime [2]. As far as I know, > it's planned to be upstreamed, but not yet started. The Xilinx runtime > software has its own in-kernel memory manager based on drm_cma_gem with > its own ioctls [3]. > > Thanks, > -hyun > > [1] https://patchwork.kernel.org/patch/10513001/ > [2] https://github.com/Xilinx/XRT > [3] https://github.com/Xilinx/XRT/tree/master/src/runtime_src/driver/zynq/drm I've done a very quick look only, and yes this is kinda what I'd expect. Doing a small drm gem driver for an fpga/accelarator that needs lots of memories is the right architecture, since at the low level of kernel interfaces a gpu really isn't anything else than an accelarater. And from a very cursory look the gem driver you mentioned (I only scrolled through the ioctl handler quickly) looks reasonable. -Daniel > > > 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 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

5 years, 9 months

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

5 years, 9 months

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

5 years, 10 months

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>

5 years, 10 months

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

5 years, 10 months

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

5 years, 10 months

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

5 years, 10 months

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

5 years, 10 months

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig February 2019