| Message ID | 1386858989-1487-1-git-send-email-treding@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Dec 12, 2013 at 03:36:29PM +0100, Thierry Reding wrote: > This is a simple test module that can be used to allocate, export and > delete DMA-BUF objects. It can be used to test DMA-BUF sharing in > systems that lack a real second driver. > > Signed-off-by: Thierry Reding <treding@nvidia.com> > --- > drivers/base/Kconfig | 4 + > drivers/base/Makefile | 1 + > drivers/base/dma-buf-test.c | 308 ++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 313 insertions(+) > create mode 100644 drivers/base/dma-buf-test.c And attached is a small test program that I've been using to test this new module. It can be built with: $ gcc -O2 -g -Wall -Werror -I/usr/include/libdrm -o dma-buf-test dma-buf-test.c -ldrm Thierry
On Thu, Dec 12, 2013 at 03:36:29PM +0100, Thierry Reding wrote: > This is a simple test module that can be used to allocate, export and > delete DMA-BUF objects. It can be used to test DMA-BUF sharing in > systems that lack a real second driver. > > Signed-off-by: Thierry Reding <treding@nvidia.com> > --- > drivers/base/Kconfig | 4 + > drivers/base/Makefile | 1 + > drivers/base/dma-buf-test.c | 308 ++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 313 insertions(+) > create mode 100644 drivers/base/dma-buf-test.c > > diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig > index e373671652b0..bed2abb9491b 100644 > --- a/drivers/base/Kconfig > +++ b/drivers/base/Kconfig > @@ -200,6 +200,10 @@ config DMA_SHARED_BUFFER > APIs extension; the file's descriptor can then be passed on to other > driver. > > +config DMA_BUF_TEST > + tristate "DMA-BUF test module" > + depends on DMA_SHARED_BUFFER > + > config DMA_CMA > bool "DMA Contiguous Memory Allocator" > depends on HAVE_DMA_CONTIGUOUS && CMA > diff --git a/drivers/base/Makefile b/drivers/base/Makefile > index 94e8a80e87f8..cad983b6626f 100644 > --- a/drivers/base/Makefile > +++ b/drivers/base/Makefile > @@ -25,3 +25,4 @@ obj-$(CONFIG_PINCTRL) += pinctrl.o > > ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG > > +obj-$(CONFIG_DMA_BUF_TEST) += dma-buf-test.o > diff --git a/drivers/base/dma-buf-test.c b/drivers/base/dma-buf-test.c > new file mode 100644 > index 000000000000..f5498b74a09b > --- /dev/null > +++ b/drivers/base/dma-buf-test.c > @@ -0,0 +1,308 @@ > +#include <linux/dma-buf.h> > +#include <linux/err.h> > +#include <linux/fs.h> > +#include <linux/miscdevice.h> > +#include <linux/module.h> > +#include <linux/slab.h> > +#include <linux/uaccess.h> > + > +struct dmabuf_create { > + __u32 flags; > + __u32 size; > +}; > + > +#define DMABUF_IOCTL_BASE 'D' > +#define DMABUF_IOCTL_CREATE _IOWR(DMABUF_IOCTL_BASE, 0, struct dmabuf_create) > +#define DMABUF_IOCTL_DELETE _IOWR(DMABUF_IOCTL_BASE, 1, int) > +#define DMABUF_IOCTL_EXPORT _IOWR(DMABUF_IOCTL_BASE, 2, int) Shouldn't we put this into an uapi header somewhere? Also I think the ioctl interface is a bit convoluted - I'd just make one single interface which directly returns a new dma-buf fd. Removing it can be handled with close, no other ioctls required imo. The explicit export step is kinda only for subsystems that already have an existing buffer handle space, but we don't have this here. Otherwise very much wanted. -Daniel > + > +struct dmabuf_file { > + struct dma_buf *buf; > + dma_addr_t phys; > + size_t size; > + void *virt; > +}; > + > +static int dmabuf_attach(struct dma_buf *buf, struct device *dev, > + struct dma_buf_attachment *attach) > +{ > + return 0; > +} > + > +static void dmabuf_detach(struct dma_buf *buf, > + struct dma_buf_attachment *attach) > +{ > +} > + > +static struct sg_table *dmabuf_map_dma_buf(struct dma_buf_attachment *attach, > + enum dma_data_direction dir) > +{ > + struct dmabuf_file *priv = attach->dmabuf->priv; > + struct sg_table *sgt; > + > + sgt = kmalloc(sizeof(*sgt), GFP_KERNEL); > + if (!sgt) > + return NULL; > + > + if (sg_alloc_table(sgt, 1, GFP_KERNEL)) { > + kfree(sgt); > + return NULL; > + } > + > + sg_dma_address(sgt->sgl) = priv->phys; > + sg_dma_len(sgt->sgl) = priv->size; > + > + return sgt; > +} > + > +static void dmabuf_unmap_dma_buf(struct dma_buf_attachment *attach, > + struct sg_table *sgt, > + enum dma_data_direction dir) > +{ > + sg_free_table(sgt); > + kfree(sgt); > +} > + > +static void dmabuf_release(struct dma_buf *buf) > +{ > +} > + > +static int dmabuf_begin_cpu_access(struct dma_buf *buf, size_t size, > + size_t length, > + enum dma_data_direction direction) > +{ > + return 0; > +} > + > +static void dmabuf_end_cpu_access(struct dma_buf *buf, size_t size, > + size_t length, > + enum dma_data_direction direction) > +{ > +} > + > +static void *dmabuf_kmap_atomic(struct dma_buf *buf, unsigned long page) > +{ > + return NULL; > +} > + > +static void dmabuf_kunmap_atomic(struct dma_buf *buf, unsigned long page, > + void *vaddr) > +{ > +} > + > +static void *dmabuf_kmap(struct dma_buf *buf, unsigned long page) > +{ > + return NULL; > +} > + > +static void dmabuf_kunmap(struct dma_buf *buf, unsigned long page, void *vaddr) > +{ > +} > + > +static void dmabuf_vm_open(struct vm_area_struct *vma) > +{ > +} > + > +static void dmabuf_vm_close(struct vm_area_struct *vma) > +{ > +} > + > +static int dmabuf_vm_fault(struct vm_area_struct *vma, struct vm_fault *vmf) > +{ > + return 0; > +} > + > +static const struct vm_operations_struct dmabuf_vm_ops = { > + .open = dmabuf_vm_open, > + .close = dmabuf_vm_close, > + .fault = dmabuf_vm_fault, > +}; > + > +static int dmabuf_mmap(struct dma_buf *buf, struct vm_area_struct *vma) > +{ > + pgprot_t prot = vm_get_page_prot(vma->vm_flags); > + struct dmabuf_file *priv = buf->priv; > + > + vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP; > + vma->vm_ops = &dmabuf_vm_ops; > + vma->vm_private_data = priv; > + vma->vm_page_prot = pgprot_writecombine(prot); > + > + return remap_pfn_range(vma, vma->vm_start, priv->phys >> PAGE_SHIFT, > + vma->vm_end - vma->vm_start, vma->vm_page_prot); > +} > + > +static void *dmabuf_vmap(struct dma_buf *buf) > +{ > + return NULL; > +} > + > +static void dmabuf_vunmap(struct dma_buf *buf, void *vaddr) > +{ > +} > + > +static const struct dma_buf_ops dmabuf_ops = { > + .attach = dmabuf_attach, > + .detach = dmabuf_detach, > + .map_dma_buf = dmabuf_map_dma_buf, > + .unmap_dma_buf = dmabuf_unmap_dma_buf, > + .release = dmabuf_release, > + .begin_cpu_access = dmabuf_begin_cpu_access, > + .end_cpu_access = dmabuf_end_cpu_access, > + .kmap_atomic = dmabuf_kmap_atomic, > + .kunmap_atomic = dmabuf_kunmap_atomic, > + .kmap = dmabuf_kmap, > + .kunmap = dmabuf_kunmap, > + .mmap = dmabuf_mmap, > + .vmap = dmabuf_vmap, > + .vunmap = dmabuf_vunmap, > +}; > + > +static int dmabuf_file_open(struct inode *inode, struct file *file) > +{ > + struct dmabuf_file *priv; > + int ret = 0; > + > + priv = kzalloc(sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + file->private_data = priv; > + > + return ret; > +} > + > +static int dmabuf_file_release(struct inode *inode, struct file *file) > +{ > + struct dmabuf_file *priv = file->private_data; > + int ret = 0; > + > + if (priv->virt) > + dma_free_writecombine(NULL, priv->size, priv->virt, priv->phys); > + > + if (priv->buf) > + dma_buf_put(priv->buf); > + > + kfree(priv); > + > + return ret; > +} > + > +static int dmabuf_ioctl_create(struct dmabuf_file *priv, const void __user *data) > +{ > + struct dmabuf_create args; > + int ret = 0; > + > + if (priv->buf || priv->virt) > + return -EBUSY; > + > + if (copy_from_user(&args, data, sizeof(args))) > + return -EFAULT; > + > + priv->virt = dma_alloc_writecombine(NULL, args.size, &priv->phys, > + GFP_KERNEL | __GFP_NOWARN); > + if (!priv->virt) > + return -ENOMEM; > + > + priv->buf = dma_buf_export(priv, &dmabuf_ops, args.size, args.flags); > + if (!priv->buf) { > + ret = -ENOMEM; > + goto free; > + } > + > + if (IS_ERR(priv->buf)) { > + ret = PTR_ERR(priv->buf); > + goto free; > + } > + > + priv->size = args.size; > + > + return 0; > + > +free: > + dma_free_writecombine(NULL, priv->size, priv->virt, priv->phys); > + priv->virt = NULL; > + return ret; > +} > + > +static int dmabuf_ioctl_delete(struct dmabuf_file *priv, unsigned long flags) > +{ > + dma_free_writecombine(NULL, priv->size, priv->virt, priv->phys); > + priv->virt = NULL; > + priv->phys = 0; > + priv->size = 0; > + > + dma_buf_put(priv->buf); > + priv->buf = NULL; > + > + return 0; > +} > + > +static int dmabuf_ioctl_export(struct dmabuf_file *priv, unsigned long flags) > +{ > + int err; > + > + get_dma_buf(priv->buf); > + > + err = dma_buf_fd(priv->buf, flags); > + if (err < 0) > + dma_buf_put(priv->buf); > + > + return err; > +} > + > +static long dmabuf_file_ioctl(struct file *file, unsigned int cmd, > + unsigned long arg) > +{ > + struct dmabuf_file *priv = file->private_data; > + long ret = 0; > + > + switch (cmd) { > + case DMABUF_IOCTL_CREATE: > + ret = dmabuf_ioctl_create(priv, (const void __user *)arg); > + break; > + > + case DMABUF_IOCTL_DELETE: > + ret = dmabuf_ioctl_delete(priv, arg); > + break; > + > + case DMABUF_IOCTL_EXPORT: > + ret = dmabuf_ioctl_export(priv, arg); > + break; > + > + default: > + ret = -ENOTTY; > + break; > + } > + > + return ret; > +} > + > +static const struct file_operations dmabuf_fops = { > + .owner = THIS_MODULE, > + .open = dmabuf_file_open, > + .release = dmabuf_file_release, > + .unlocked_ioctl = dmabuf_file_ioctl, > +}; > + > +static struct miscdevice dmabuf_device = { > + .minor = 128, > + .name = "dmabuf", > + .fops = &dmabuf_fops, > +}; > + > +static int __init dmabuf_init(void) > +{ > + return misc_register(&dmabuf_device); > +} > +module_init(dmabuf_init); > + > +static void __exit dmabuf_exit(void) > +{ > + misc_deregister(&dmabuf_device); > +} > +module_exit(dmabuf_exit); > + > +MODULE_AUTHOR("Thierry Reding <treding@nvidia.com>"); > +MODULE_DESCRIPTION("DMA-BUF test driver"); > +MODULE_LICENSE("GPL v2"); > -- > 1.8.4.2 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, Dec 12, 2013 at 03:53:05PM +0100, Daniel Vetter wrote: > On Thu, Dec 12, 2013 at 03:36:29PM +0100, Thierry Reding wrote: [...] > > diff --git a/drivers/base/dma-buf-test.c b/drivers/base/dma-buf-test.c [...] > > +struct dmabuf_create { > > + __u32 flags; > > + __u32 size; > > +}; > > + > > +#define DMABUF_IOCTL_BASE 'D' > > +#define DMABUF_IOCTL_CREATE _IOWR(DMABUF_IOCTL_BASE, 0, struct dmabuf_create) > > +#define DMABUF_IOCTL_DELETE _IOWR(DMABUF_IOCTL_BASE, 1, int) > > +#define DMABUF_IOCTL_EXPORT _IOWR(DMABUF_IOCTL_BASE, 2, int) > > Shouldn't we put this into an uapi header somewhere? Yes, definitely. I just didn't want to go through all that trouble before checking that anyone else actually thought this was a good idea. There are a few other things that probably need to be solved before this can be merged, though. For instance, it currently doesn't compile on x86 because it uses dma_alloc_writecombine(). So there's ample room for improvement. > Also I think the ioctl interface is a bit convoluted - I'd just make > one single interface which directly returns a new dma-buf fd. Removing > it can be handled with close, no other ioctls required imo. The > explicit export step is kinda only for subsystems that already have an > existing buffer handle space, but we don't have this here. I initially thought that this allowed more flexibility, but on the other hand having to keep track of a number of buffers would add complexity to the implementation and like you said it's probably not worth it. Also, it's not like the code currently copes well with multiple buffers being created. It just chickens out by returning -EBUSY. The equivalent could be achieved by beefing up the DMABUF_IOCTL_CREATE thusly: struct dmabuf_create { __u32 flags; __u32 size; __u32 fd; __u32 pad; }; Then use the size and flags parameters as before, but return the file descriptor to the DMA-BUF in .fd. Then simply keep it open until the descriptor is closed. That would also seem to be a more reliable interface since closing the fd just decrements the reference count on the DMA-BUF and therefore would keep it alive if somebody still kept around a reference (the importer for instance). Thierry
On 12/12/2013 03:36 PM, Thierry Reding wrote: > This is a simple test module that can be used to allocate, export and > delete DMA-BUF objects. It can be used to test DMA-BUF sharing in > systems that lack a real second driver. > > Looks nice. I wonder whether this could be extended to create a "streaming" dma-buf from a user space mapping. That could be used as a generic way to implement streaming (user) buffer objects, rather than to add explicit support for those in, for example, TTM. /Thomas
On Thu, Dec 12, 2013 at 8:34 PM, Thomas Hellstrom <thellstrom@vmware.com> wrote: > On 12/12/2013 03:36 PM, Thierry Reding wrote: >> >> This is a simple test module that can be used to allocate, export and >> delete DMA-BUF objects. It can be used to test DMA-BUF sharing in >> systems that lack a real second driver. >> >> > > Looks nice. I wonder whether this could be extended to create a "streaming" > dma-buf from a user space mapping. That could be used as a generic way to > implement streaming (user) buffer objects, rather than to add explicit > support for those in, for example, TTM. Atm there's no way to get gpus to unbind their dma-buf mappings, so their essentially pinned forever from first use on. Userptr won't really make this worse, but imo we should fix this first before expanding the use-cases too much. And getting dma-bufs to integrate better into existing memory mangers like ttm will be a lot of pain. -Daniel
On Thu, Dec 12, 2013 at 03:42:12PM +0100, Thierry Reding wrote: > On Thu, Dec 12, 2013 at 03:36:29PM +0100, Thierry Reding wrote: > > This is a simple test module that can be used to allocate, export and > > delete DMA-BUF objects. It can be used to test DMA-BUF sharing in > > systems that lack a real second driver. > > > > Signed-off-by: Thierry Reding <treding@nvidia.com> > > --- > > drivers/base/Kconfig | 4 + > > drivers/base/Makefile | 1 + > > drivers/base/dma-buf-test.c | 308 ++++++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 313 insertions(+) > > create mode 100644 drivers/base/dma-buf-test.c > > And attached is a small test program that I've been using to test this > new module. It can be built with: > > $ gcc -O2 -g -Wall -Werror -I/usr/include/libdrm -o dma-buf-test dma-buf-test.c -ldrm Please put this in the patch as well (scripts/tests/ ?) Also fix up the uapi stuff for the ioctls. thanks, greg k-h
On Thu, Dec 12, 2013 at 03:36:29PM +0100, Thierry Reding wrote: > This is a simple test module that can be used to allocate, export and > delete DMA-BUF objects. It can be used to test DMA-BUF sharing in > systems that lack a real second driver. > > Signed-off-by: Thierry Reding <treding@nvidia.com> > --- > drivers/base/Kconfig | 4 + > drivers/base/Makefile | 1 + > drivers/base/dma-buf-test.c | 308 ++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 313 insertions(+) > create mode 100644 drivers/base/dma-buf-test.c > > diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig > index e373671652b0..bed2abb9491b 100644 > --- a/drivers/base/Kconfig > +++ b/drivers/base/Kconfig > @@ -200,6 +200,10 @@ config DMA_SHARED_BUFFER > APIs extension; the file's descriptor can then be passed on to other > driver. > > +config DMA_BUF_TEST > + tristate "DMA-BUF test module" > + depends on DMA_SHARED_BUFFER We need some good documentation here. > > +static struct miscdevice dmabuf_device = { > + .minor = 128, Why did you pick this minor? Why not just make it dynamic? thanks, greg k-h
On Thu, Dec 12, 2013 at 06:04:13PM -0800, Greg Kroah-Hartman wrote: > On Thu, Dec 12, 2013 at 03:36:29PM +0100, Thierry Reding wrote: > > This is a simple test module that can be used to allocate, export and > > delete DMA-BUF objects. It can be used to test DMA-BUF sharing in > > systems that lack a real second driver. > > > > Signed-off-by: Thierry Reding <treding@nvidia.com> > > --- > > drivers/base/Kconfig | 4 + > > drivers/base/Makefile | 1 + > > drivers/base/dma-buf-test.c | 308 ++++++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 313 insertions(+) > > create mode 100644 drivers/base/dma-buf-test.c > > > > diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig > > index e373671652b0..bed2abb9491b 100644 > > --- a/drivers/base/Kconfig > > +++ b/drivers/base/Kconfig > > @@ -200,6 +200,10 @@ config DMA_SHARED_BUFFER > > APIs extension; the file's descriptor can then be passed on to other > > driver. > > > > +config DMA_BUF_TEST > > + tristate "DMA-BUF test module" > > + depends on DMA_SHARED_BUFFER > > We need some good documentation here. I agree. The description should go into more details about what exactly this is meant to address. > > > +static struct miscdevice dmabuf_device = { > > + .minor = 128, > > Why did you pick this minor? Why not just make it dynamic? It seemed like minors are statically allocated for miscdevice and 128 seemed to be as good as any. The tentative plan was to go through the official way of having one allocated as explained in the comment in include/linux/miscdevice.h. Reading that comment again, there's MISC_DYNAMIC_MINOR, which I guess would be appropriate here. Chances are that if you want to test DMA-BUF you'll have a reasonably modern userspace that will create the device dynamically. Alternatively I guess I could instead turn this into a more full-fledged cdev and do the whole alloc_chrdev_region() dance. Although that will only allocate the major dynamically. I'm not aware of any function that just allocates a single major/minor pair completely dynamically. Is there one that you could point me to? Thierry
On Thu, Dec 12, 2013 at 06:02:58PM -0800, Greg Kroah-Hartman wrote: > On Thu, Dec 12, 2013 at 03:42:12PM +0100, Thierry Reding wrote: > > On Thu, Dec 12, 2013 at 03:36:29PM +0100, Thierry Reding wrote: > > > This is a simple test module that can be used to allocate, export and > > > delete DMA-BUF objects. It can be used to test DMA-BUF sharing in > > > systems that lack a real second driver. > > > > > > Signed-off-by: Thierry Reding <treding@nvidia.com> > > > --- > > > drivers/base/Kconfig | 4 + > > > drivers/base/Makefile | 1 + > > > drivers/base/dma-buf-test.c | 308 ++++++++++++++++++++++++++++++++++++++++++++ > > > 3 files changed, 313 insertions(+) > > > create mode 100644 drivers/base/dma-buf-test.c > > > > And attached is a small test program that I've been using to test this > > new module. It can be built with: > > > > $ gcc -O2 -g -Wall -Werror -I/usr/include/libdrm -o dma-buf-test dma-buf-test.c -ldrm > > Please put this in the patch as well (scripts/tests/ ?) There is also samples/ which already contains various test programs of a similar nature. > Also fix up the uapi stuff for the ioctls. Will do. Thanks, Thierry
On Thu, Dec 12, 2013 at 11:30:23PM +0100, Daniel Vetter wrote: > On Thu, Dec 12, 2013 at 8:34 PM, Thomas Hellstrom <thellstrom@vmware.com> wrote: > > On 12/12/2013 03:36 PM, Thierry Reding wrote: > >> > >> This is a simple test module that can be used to allocate, export and > >> delete DMA-BUF objects. It can be used to test DMA-BUF sharing in > >> systems that lack a real second driver. > >> > >> > > > > Looks nice. I wonder whether this could be extended to create a "streaming" > > dma-buf from a user space mapping. That could be used as a generic way to > > implement streaming (user) buffer objects, rather than to add explicit > > support for those in, for example, TTM. > > Atm there's no way to get gpus to unbind their dma-buf mappings, so > their essentially pinned forever from first use on. Shouldn't this work by simply calling the GEM_CLOSE IOCTL on the handle returned by drmPrimeFDToHandle()? I mean that should drop the last reference on the GEM object and cause it to be cleaned up (which should include detaching the DMA-BUF). Thierry
On Thu, Dec 12, 2013 at 08:34:28PM +0100, Thomas Hellstrom wrote: > On 12/12/2013 03:36 PM, Thierry Reding wrote: > >This is a simple test module that can be used to allocate, export and > >delete DMA-BUF objects. It can be used to test DMA-BUF sharing in > >systems that lack a real second driver. > > > > > > Looks nice. I wonder whether this could be extended to create a "streaming" > dma-buf from a user space mapping. That could be used as a generic way to > implement streaming (user) buffer objects, rather than to add explicit > support for those in, for example, TTM. I'm somewhat reluctant to beef this up needlessly to prevent it from being used for purposes other than testing. Thierry
On 12/14/2013 01:37 PM, Thierry Reding wrote: > On Thu, Dec 12, 2013 at 11:30:23PM +0100, Daniel Vetter wrote: >> On Thu, Dec 12, 2013 at 8:34 PM, Thomas Hellstrom <thellstrom@vmware.com> wrote: >>> On 12/12/2013 03:36 PM, Thierry Reding wrote: >>>> This is a simple test module that can be used to allocate, export and >>>> delete DMA-BUF objects. It can be used to test DMA-BUF sharing in >>>> systems that lack a real second driver. >>>> >>>> >>> Looks nice. I wonder whether this could be extended to create a "streaming" >>> dma-buf from a user space mapping. That could be used as a generic way to >>> implement streaming (user) buffer objects, rather than to add explicit >>> support for those in, for example, TTM. >> Atm there's no way to get gpus to unbind their dma-buf mappings, so >> their essentially pinned forever from first use on. > Shouldn't this work by simply calling the GEM_CLOSE IOCTL on the handle > returned by drmPrimeFDToHandle()? I mean that should drop the last > reference on the GEM object and cause it to be cleaned up (which should > include detaching the DMA-BUF). Actually, while the GEM prime implementation appears to pin an exported dma-buf on first attach, from the dma-buf documentation it seems sufficient to pin it on map or cpu access. But what I assume Daniel is referring to is that there is no way for exporters to tell importers to force unmap() the dma-buf, so that it can be unpinned? Daniel, maybe you could elaborate a bit on this? Thomas > > Thierry > > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Sat, Dec 14, 2013 at 7:47 AM, Thomas Hellstrom <thomas@shipmail.org> wrote: > On 12/14/2013 01:37 PM, Thierry Reding wrote: >> >> On Thu, Dec 12, 2013 at 11:30:23PM +0100, Daniel Vetter wrote: >>> >>> On Thu, Dec 12, 2013 at 8:34 PM, Thomas Hellstrom <thellstrom@vmware.com> >>> wrote: >>>> >>>> On 12/12/2013 03:36 PM, Thierry Reding wrote: >>>>> >>>>> This is a simple test module that can be used to allocate, export and >>>>> delete DMA-BUF objects. It can be used to test DMA-BUF sharing in >>>>> systems that lack a real second driver. >>>>> >>>>> >>>> Looks nice. I wonder whether this could be extended to create a >>>> "streaming" >>>> dma-buf from a user space mapping. That could be used as a generic way >>>> to >>>> implement streaming (user) buffer objects, rather than to add explicit >>>> support for those in, for example, TTM. >>> >>> Atm there's no way to get gpus to unbind their dma-buf mappings, so >>> their essentially pinned forever from first use on. >> >> Shouldn't this work by simply calling the GEM_CLOSE IOCTL on the handle >> returned by drmPrimeFDToHandle()? I mean that should drop the last >> reference on the GEM object and cause it to be cleaned up (which should >> include detaching the DMA-BUF). > > > Actually, while the GEM prime implementation appears to pin an exported > dma-buf on first attach, from the dma-buf documentation it seems sufficient > to pin it on map or cpu access. > > But what I assume Daniel is referring to is that there is no way for > exporters to tell importers to force unmap() the dma-buf, so that it can be > unpinned? yeah, or some way for importers to opportunistically keep around a mapping rather than map/unmap on each use.. maybe we need something shrinker-ish for dmabuf? BR, -R > Daniel, maybe you could elaborate a bit on this? > > Thomas > >> >> Thierry >> >> >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/dri-devel > > > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On 12/14/2013 02:02 PM, Rob Clark wrote: > On Sat, Dec 14, 2013 at 7:47 AM, Thomas Hellstrom <thomas@shipmail.org> wrote: >> On 12/14/2013 01:37 PM, Thierry Reding wrote: >>> On Thu, Dec 12, 2013 at 11:30:23PM +0100, Daniel Vetter wrote: >>>> On Thu, Dec 12, 2013 at 8:34 PM, Thomas Hellstrom <thellstrom@vmware.com> >>>> wrote: >>>>> On 12/12/2013 03:36 PM, Thierry Reding wrote: >>>>>> This is a simple test module that can be used to allocate, export and >>>>>> delete DMA-BUF objects. It can be used to test DMA-BUF sharing in >>>>>> systems that lack a real second driver. >>>>>> >>>>>> >>>>> Looks nice. I wonder whether this could be extended to create a >>>>> "streaming" >>>>> dma-buf from a user space mapping. That could be used as a generic way >>>>> to >>>>> implement streaming (user) buffer objects, rather than to add explicit >>>>> support for those in, for example, TTM. >>>> Atm there's no way to get gpus to unbind their dma-buf mappings, so >>>> their essentially pinned forever from first use on. >>> Shouldn't this work by simply calling the GEM_CLOSE IOCTL on the handle >>> returned by drmPrimeFDToHandle()? I mean that should drop the last >>> reference on the GEM object and cause it to be cleaned up (which should >>> include detaching the DMA-BUF). >> >> Actually, while the GEM prime implementation appears to pin an exported >> dma-buf on first attach, from the dma-buf documentation it seems sufficient >> to pin it on map or cpu access. >> >> But what I assume Daniel is referring to is that there is no way for >> exporters to tell importers to force unmap() the dma-buf, so that it can be >> unpinned? > yeah, or some way for importers to opportunistically keep around a > mapping rather than map/unmap on each use.. > > maybe we need something shrinker-ish for dmabuf? Yes, I think that's needed for both the memory-shortage case where we want to unpin, and I guess it would desirable for iommu space management as well. /Thomas > > BR, > -R > >
On Sat, Dec 14, 2013 at 1:37 PM, Thierry Reding <thierry.reding@gmail.com> wrote: > On Thu, Dec 12, 2013 at 11:30:23PM +0100, Daniel Vetter wrote: >> On Thu, Dec 12, 2013 at 8:34 PM, Thomas Hellstrom <thellstrom@vmware.com> wrote: >> > On 12/12/2013 03:36 PM, Thierry Reding wrote: >> >> >> >> This is a simple test module that can be used to allocate, export and >> >> delete DMA-BUF objects. It can be used to test DMA-BUF sharing in >> >> systems that lack a real second driver. >> >> >> >> >> > >> > Looks nice. I wonder whether this could be extended to create a "streaming" >> > dma-buf from a user space mapping. That could be used as a generic way to >> > implement streaming (user) buffer objects, rather than to add explicit >> > support for those in, for example, TTM. >> >> Atm there's no way to get gpus to unbind their dma-buf mappings, so >> their essentially pinned forever from first use on. > > Shouldn't this work by simply calling the GEM_CLOSE IOCTL on the handle > returned by drmPrimeFDToHandle()? I mean that should drop the last > reference on the GEM object and cause it to be cleaned up (which should > include detaching the DMA-BUF). Yeah, but that has the side-effect of also dropping the reference. And requires an explicit action from userspace. The entire point of kernel buffer managers is that this is all done transparently in the kernel and when low on memory some objects get swapped out behind userspaces back (and then pulled in again if needed). -Daniel
On Sat, Dec 14, 2013 at 01:16:21PM +0100, Thierry Reding wrote: > On Thu, Dec 12, 2013 at 06:04:13PM -0800, Greg Kroah-Hartman wrote: > > On Thu, Dec 12, 2013 at 03:36:29PM +0100, Thierry Reding wrote: > > > This is a simple test module that can be used to allocate, export and > > > delete DMA-BUF objects. It can be used to test DMA-BUF sharing in > > > systems that lack a real second driver. > > > > > > Signed-off-by: Thierry Reding <treding@nvidia.com> > > > --- > > > drivers/base/Kconfig | 4 + > > > drivers/base/Makefile | 1 + > > > drivers/base/dma-buf-test.c | 308 ++++++++++++++++++++++++++++++++++++++++++++ > > > 3 files changed, 313 insertions(+) > > > create mode 100644 drivers/base/dma-buf-test.c > > > > > > diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig > > > index e373671652b0..bed2abb9491b 100644 > > > --- a/drivers/base/Kconfig > > > +++ b/drivers/base/Kconfig > > > @@ -200,6 +200,10 @@ config DMA_SHARED_BUFFER > > > APIs extension; the file's descriptor can then be passed on to other > > > driver. > > > > > > +config DMA_BUF_TEST > > > + tristate "DMA-BUF test module" > > > + depends on DMA_SHARED_BUFFER > > > > We need some good documentation here. > > I agree. The description should go into more details about what exactly > this is meant to address. > > > > > +static struct miscdevice dmabuf_device = { > > > + .minor = 128, > > > > Why did you pick this minor? Why not just make it dynamic? > > It seemed like minors are statically allocated for miscdevice and 128 > seemed to be as good as any. The tentative plan was to go through the > official way of having one allocated as explained in the comment in > include/linux/miscdevice.h. > > Reading that comment again, there's MISC_DYNAMIC_MINOR, which I guess > would be appropriate here. Chances are that if you want to test DMA-BUF > you'll have a reasonably modern userspace that will create the device > dynamically. > > Alternatively I guess I could instead turn this into a more full-fledged > cdev and do the whole alloc_chrdev_region() dance. Although that will > only allocate the major dynamically. > > I'm not aware of any function that just allocates a single major/minor > pair completely dynamically. Is there one that you could point me to? miscdev seems appropriate for me, I don't think we'll ever need a 2nd dma-buf node. After all the entire point of sharing is to have system-wide access, so having a 2nd dma-buf domain/thing doesn't look useful at all. -Daniel
On Sat, Dec 14, 2013 at 01:16:21PM +0100, Thierry Reding wrote: > On Thu, Dec 12, 2013 at 06:04:13PM -0800, Greg Kroah-Hartman wrote: > > On Thu, Dec 12, 2013 at 03:36:29PM +0100, Thierry Reding wrote: > > > This is a simple test module that can be used to allocate, export and > > > delete DMA-BUF objects. It can be used to test DMA-BUF sharing in > > > systems that lack a real second driver. > > > > > > Signed-off-by: Thierry Reding <treding@nvidia.com> > > > --- > > > drivers/base/Kconfig | 4 + > > > drivers/base/Makefile | 1 + > > > drivers/base/dma-buf-test.c | 308 ++++++++++++++++++++++++++++++++++++++++++++ > > > 3 files changed, 313 insertions(+) > > > create mode 100644 drivers/base/dma-buf-test.c > > > > > > diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig > > > index e373671652b0..bed2abb9491b 100644 > > > --- a/drivers/base/Kconfig > > > +++ b/drivers/base/Kconfig > > > @@ -200,6 +200,10 @@ config DMA_SHARED_BUFFER > > > APIs extension; the file's descriptor can then be passed on to other > > > driver. > > > > > > +config DMA_BUF_TEST > > > + tristate "DMA-BUF test module" > > > + depends on DMA_SHARED_BUFFER > > > > We need some good documentation here. > > I agree. The description should go into more details about what exactly > this is meant to address. > > > > > +static struct miscdevice dmabuf_device = { > > > + .minor = 128, > > > > Why did you pick this minor? Why not just make it dynamic? > > It seemed like minors are statically allocated for miscdevice and 128 > seemed to be as good as any. The tentative plan was to go through the > official way of having one allocated as explained in the comment in > include/linux/miscdevice.h. > > Reading that comment again, there's MISC_DYNAMIC_MINOR, which I guess > would be appropriate here. Chances are that if you want to test DMA-BUF > you'll have a reasonably modern userspace that will create the device > dynamically. > > Alternatively I guess I could instead turn this into a more full-fledged > cdev and do the whole alloc_chrdev_region() dance. Although that will > only allocate the major dynamically. No, just use a dynamic misc device and all will be fine. Bonus is that you can create multiple misc devices if you ever really need to in the future, with no need to change much code at all (i.e. no chrdev crud.) > I'm not aware of any function that just allocates a single major/minor > pair completely dynamically. Is there one that you could point me to? Nope, that's what misc devices are for :) greg k-h
diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig index e373671652b0..bed2abb9491b 100644 --- a/drivers/base/Kconfig +++ b/drivers/base/Kconfig @@ -200,6 +200,10 @@ config DMA_SHARED_BUFFER APIs extension; the file's descriptor can then be passed on to other driver. +config DMA_BUF_TEST + tristate "DMA-BUF test module" + depends on DMA_SHARED_BUFFER + config DMA_CMA bool "DMA Contiguous Memory Allocator" depends on HAVE_DMA_CONTIGUOUS && CMA diff --git a/drivers/base/Makefile b/drivers/base/Makefile index 94e8a80e87f8..cad983b6626f 100644 --- a/drivers/base/Makefile +++ b/drivers/base/Makefile @@ -25,3 +25,4 @@ obj-$(CONFIG_PINCTRL) += pinctrl.o ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG +obj-$(CONFIG_DMA_BUF_TEST) += dma-buf-test.o diff --git a/drivers/base/dma-buf-test.c b/drivers/base/dma-buf-test.c new file mode 100644 index 000000000000..f5498b74a09b --- /dev/null +++ b/drivers/base/dma-buf-test.c @@ -0,0 +1,308 @@ +#include <linux/dma-buf.h> +#include <linux/err.h> +#include <linux/fs.h> +#include <linux/miscdevice.h> +#include <linux/module.h> +#include <linux/slab.h> +#include <linux/uaccess.h> + +struct dmabuf_create { + __u32 flags; + __u32 size; +}; + +#define DMABUF_IOCTL_BASE 'D' +#define DMABUF_IOCTL_CREATE _IOWR(DMABUF_IOCTL_BASE, 0, struct dmabuf_create) +#define DMABUF_IOCTL_DELETE _IOWR(DMABUF_IOCTL_BASE, 1, int) +#define DMABUF_IOCTL_EXPORT _IOWR(DMABUF_IOCTL_BASE, 2, int) + +struct dmabuf_file { + struct dma_buf *buf; + dma_addr_t phys; + size_t size; + void *virt; +}; + +static int dmabuf_attach(struct dma_buf *buf, struct device *dev, + struct dma_buf_attachment *attach) +{ + return 0; +} + +static void dmabuf_detach(struct dma_buf *buf, + struct dma_buf_attachment *attach) +{ +} + +static struct sg_table *dmabuf_map_dma_buf(struct dma_buf_attachment *attach, + enum dma_data_direction dir) +{ + struct dmabuf_file *priv = attach->dmabuf->priv; + struct sg_table *sgt; + + sgt = kmalloc(sizeof(*sgt), GFP_KERNEL); + if (!sgt) + return NULL; + + if (sg_alloc_table(sgt, 1, GFP_KERNEL)) { + kfree(sgt); + return NULL; + } + + sg_dma_address(sgt->sgl) = priv->phys; + sg_dma_len(sgt->sgl) = priv->size; + + return sgt; +} + +static void dmabuf_unmap_dma_buf(struct dma_buf_attachment *attach, + struct sg_table *sgt, + enum dma_data_direction dir) +{ + sg_free_table(sgt); + kfree(sgt); +} + +static void dmabuf_release(struct dma_buf *buf) +{ +} + +static int dmabuf_begin_cpu_access(struct dma_buf *buf, size_t size, + size_t length, + enum dma_data_direction direction) +{ + return 0; +} + +static void dmabuf_end_cpu_access(struct dma_buf *buf, size_t size, + size_t length, + enum dma_data_direction direction) +{ +} + +static void *dmabuf_kmap_atomic(struct dma_buf *buf, unsigned long page) +{ + return NULL; +} + +static void dmabuf_kunmap_atomic(struct dma_buf *buf, unsigned long page, + void *vaddr) +{ +} + +static void *dmabuf_kmap(struct dma_buf *buf, unsigned long page) +{ + return NULL; +} + +static void dmabuf_kunmap(struct dma_buf *buf, unsigned long page, void *vaddr) +{ +} + +static void dmabuf_vm_open(struct vm_area_struct *vma) +{ +} + +static void dmabuf_vm_close(struct vm_area_struct *vma) +{ +} + +static int dmabuf_vm_fault(struct vm_area_struct *vma, struct vm_fault *vmf) +{ + return 0; +} + +static const struct vm_operations_struct dmabuf_vm_ops = { + .open = dmabuf_vm_open, + .close = dmabuf_vm_close, + .fault = dmabuf_vm_fault, +}; + +static int dmabuf_mmap(struct dma_buf *buf, struct vm_area_struct *vma) +{ + pgprot_t prot = vm_get_page_prot(vma->vm_flags); + struct dmabuf_file *priv = buf->priv; + + vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP; + vma->vm_ops = &dmabuf_vm_ops; + vma->vm_private_data = priv; + vma->vm_page_prot = pgprot_writecombine(prot); + + return remap_pfn_range(vma, vma->vm_start, priv->phys >> PAGE_SHIFT, + vma->vm_end - vma->vm_start, vma->vm_page_prot); +} + +static void *dmabuf_vmap(struct dma_buf *buf) +{ + return NULL; +} + +static void dmabuf_vunmap(struct dma_buf *buf, void *vaddr) +{ +} + +static const struct dma_buf_ops dmabuf_ops = { + .attach = dmabuf_attach, + .detach = dmabuf_detach, + .map_dma_buf = dmabuf_map_dma_buf, + .unmap_dma_buf = dmabuf_unmap_dma_buf, + .release = dmabuf_release, + .begin_cpu_access = dmabuf_begin_cpu_access, + .end_cpu_access = dmabuf_end_cpu_access, + .kmap_atomic = dmabuf_kmap_atomic, + .kunmap_atomic = dmabuf_kunmap_atomic, + .kmap = dmabuf_kmap, + .kunmap = dmabuf_kunmap, + .mmap = dmabuf_mmap, + .vmap = dmabuf_vmap, + .vunmap = dmabuf_vunmap, +}; + +static int dmabuf_file_open(struct inode *inode, struct file *file) +{ + struct dmabuf_file *priv; + int ret = 0; + + priv = kzalloc(sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + + file->private_data = priv; + + return ret; +} + +static int dmabuf_file_release(struct inode *inode, struct file *file) +{ + struct dmabuf_file *priv = file->private_data; + int ret = 0; + + if (priv->virt) + dma_free_writecombine(NULL, priv->size, priv->virt, priv->phys); + + if (priv->buf) + dma_buf_put(priv->buf); + + kfree(priv); + + return ret; +} + +static int dmabuf_ioctl_create(struct dmabuf_file *priv, const void __user *data) +{ + struct dmabuf_create args; + int ret = 0; + + if (priv->buf || priv->virt) + return -EBUSY; + + if (copy_from_user(&args, data, sizeof(args))) + return -EFAULT; + + priv->virt = dma_alloc_writecombine(NULL, args.size, &priv->phys, + GFP_KERNEL | __GFP_NOWARN); + if (!priv->virt) + return -ENOMEM; + + priv->buf = dma_buf_export(priv, &dmabuf_ops, args.size, args.flags); + if (!priv->buf) { + ret = -ENOMEM; + goto free; + } + + if (IS_ERR(priv->buf)) { + ret = PTR_ERR(priv->buf); + goto free; + } + + priv->size = args.size; + + return 0; + +free: + dma_free_writecombine(NULL, priv->size, priv->virt, priv->phys); + priv->virt = NULL; + return ret; +} + +static int dmabuf_ioctl_delete(struct dmabuf_file *priv, unsigned long flags) +{ + dma_free_writecombine(NULL, priv->size, priv->virt, priv->phys); + priv->virt = NULL; + priv->phys = 0; + priv->size = 0; + + dma_buf_put(priv->buf); + priv->buf = NULL; + + return 0; +} + +static int dmabuf_ioctl_export(struct dmabuf_file *priv, unsigned long flags) +{ + int err; + + get_dma_buf(priv->buf); + + err = dma_buf_fd(priv->buf, flags); + if (err < 0) + dma_buf_put(priv->buf); + + return err; +} + +static long dmabuf_file_ioctl(struct file *file, unsigned int cmd, + unsigned long arg) +{ + struct dmabuf_file *priv = file->private_data; + long ret = 0; + + switch (cmd) { + case DMABUF_IOCTL_CREATE: + ret = dmabuf_ioctl_create(priv, (const void __user *)arg); + break; + + case DMABUF_IOCTL_DELETE: + ret = dmabuf_ioctl_delete(priv, arg); + break; + + case DMABUF_IOCTL_EXPORT: + ret = dmabuf_ioctl_export(priv, arg); + break; + + default: + ret = -ENOTTY; + break; + } + + return ret; +} + +static const struct file_operations dmabuf_fops = { + .owner = THIS_MODULE, + .open = dmabuf_file_open, + .release = dmabuf_file_release, + .unlocked_ioctl = dmabuf_file_ioctl, +}; + +static struct miscdevice dmabuf_device = { + .minor = 128, + .name = "dmabuf", + .fops = &dmabuf_fops, +}; + +static int __init dmabuf_init(void) +{ + return misc_register(&dmabuf_device); +} +module_init(dmabuf_init); + +static void __exit dmabuf_exit(void) +{ + misc_deregister(&dmabuf_device); +} +module_exit(dmabuf_exit); + +MODULE_AUTHOR("Thierry Reding <treding@nvidia.com>"); +MODULE_DESCRIPTION("DMA-BUF test driver"); +MODULE_LICENSE("GPL v2");
This is a simple test module that can be used to allocate, export and delete DMA-BUF objects. It can be used to test DMA-BUF sharing in systems that lack a real second driver. Signed-off-by: Thierry Reding <treding@nvidia.com> --- drivers/base/Kconfig | 4 + drivers/base/Makefile | 1 + drivers/base/dma-buf-test.c | 308 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 313 insertions(+) create mode 100644 drivers/base/dma-buf-test.c