Hi,
This is v2 of my proposal to add dma_buf import functions for vgem. Big changes from v1:
- A device is required for dma_buf attach to work. The existing vgem driver intentionally does not use one as it provides a good way to test the DRM framework. This approach instead puts a dummy platform device in the existing drm_device->platformdev field and uses that for attaching. - Native vgem buffers can still be faulted in a page at a time without requiring the entire buffer be resident in memory.
I'm still marking this as RFC as I haven't had a chance to finish a userspace test that can be integrated into igt.
Feedback appreciated as always.
Thanks, Laura
Laura Abbott (3): drm/vgem: Add a dummy platform device drm/prime: Introduce drm_gem_prime_import_platform drm/vgem: Enable dmabuf import interfaces
drivers/gpu/drm/drm_prime.c | 23 +++++- drivers/gpu/drm/vgem/vgem_drv.c | 150 +++++++++++++++++++++++++++++++--------- drivers/gpu/drm/vgem/vgem_drv.h | 2 + include/drm/drmP.h | 5 ++ 4 files changed, 145 insertions(+), 35 deletions(-)
The vgem driver is currently registered independent of any actual device. Some usage of the dmabuf APIs require an actual device structure to do anything. Register a dummy platform device for use with dmabuf.
Signed-off-by: Laura Abbott labbott@redhat.com --- v2: Store the platform device in the platformdev field instead of the regular device structure. --- drivers/gpu/drm/vgem/vgem_drv.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c index a1f42d1..1b02e56 100644 --- a/drivers/gpu/drm/vgem/vgem_drv.c +++ b/drivers/gpu/drm/vgem/vgem_drv.c @@ -335,11 +335,20 @@ static int __init vgem_init(void) int ret;
vgem_device = drm_dev_alloc(&vgem_driver, NULL); - if (IS_ERR(vgem_device)) { - ret = PTR_ERR(vgem_device); + if (IS_ERR(vgem_device)) + return PTR_ERR(vgem_device); + + vgem_device->platformdev = platform_device_register_simple("vgem", + -1, NULL, 0); + + if (!vgem_device->platformdev) { + ret = -ENODEV; goto out; }
+ dma_coerce_mask_and_coherent(&vgem_device->platformdev->dev, + DMA_BIT_MASK(64)); + ret = drm_dev_register(vgem_device, 0); if (ret) goto out_unref; @@ -347,13 +356,15 @@ static int __init vgem_init(void) return 0;
out_unref: - drm_dev_unref(vgem_device); + platform_device_unregister(vgem_device->platformdev); out: + drm_dev_unref(vgem_device); return ret; }
static void __exit vgem_exit(void) { + platform_device_unregister(vgem_device->platformdev); drm_dev_unregister(vgem_device); drm_dev_unref(vgem_device); }
The existing drm_gem_prime_import function uses the underlying struct device of a drm_device for attaching to a dma_buf. Some drivers (notably vgem) may not have an underlying device structure. Offer an alternate function to attach using a platform device associated with drm_device.
Signed-off-by: Laura Abbott labbott@redhat.com --- New approach for v2 --- drivers/gpu/drm/drm_prime.c | 23 ++++++++++++++++++++--- include/drm/drmP.h | 5 +++++ 2 files changed, 25 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index 25aa455..5486248 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -601,8 +601,9 @@ EXPORT_SYMBOL(drm_gem_prime_handle_to_fd); * This is the implementation of the gem_prime_import functions for GEM drivers * using the PRIME helpers. */ -struct drm_gem_object *drm_gem_prime_import(struct drm_device *dev, - struct dma_buf *dma_buf) +static struct drm_gem_object *__drm_gem_prime_import(struct drm_device *dev, + struct dma_buf *dma_buf, + struct device *attach_dev) { struct dma_buf_attachment *attach; struct sg_table *sgt; @@ -624,7 +625,7 @@ struct drm_gem_object *drm_gem_prime_import(struct drm_device *dev, if (!dev->driver->gem_prime_import_sg_table) return ERR_PTR(-EINVAL);
- attach = dma_buf_attach(dma_buf, dev->dev); + attach = dma_buf_attach(dma_buf, attach_dev); if (IS_ERR(attach)) return ERR_CAST(attach);
@@ -654,8 +655,24 @@ struct drm_gem_object *drm_gem_prime_import(struct drm_device *dev,
return ERR_PTR(ret); } + +struct drm_gem_object *drm_gem_prime_import(struct drm_device *dev, + struct dma_buf *dma_buf) +{ + return __drm_gem_prime_import(dev, dma_buf, dev->dev); +} EXPORT_SYMBOL(drm_gem_prime_import);
+struct drm_gem_object *drm_gem_prime_import_platform(struct drm_device *dev, + struct dma_buf *dma_buf) +{ + if (WARN_ON_ONCE(!dev->platformdev)) + return NULL; + + return __drm_gem_prime_import(dev, dma_buf, &dev->platformdev->dev); +} +EXPORT_SYMBOL(drm_gem_prime_import_platform); + /** * drm_gem_prime_fd_to_handle - PRIME import function for GEM drivers * @dev: dev to export the buffer from diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 6105c05..f4ac30f 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -769,6 +769,11 @@ extern int drm_gem_prime_handle_to_fd(struct drm_device *dev, int *prime_fd); extern struct drm_gem_object *drm_gem_prime_import(struct drm_device *dev, struct dma_buf *dma_buf); + +extern struct drm_gem_object *drm_gem_prime_import_platform( + struct drm_device *dev, + struct dma_buf *dma_buf); + extern int drm_gem_prime_fd_to_handle(struct drm_device *dev, struct drm_file *file_priv, int prime_fd, uint32_t *handle); struct dma_buf *drm_gem_dmabuf_export(struct drm_device *dev,
On Wed, Apr 26, 2017 at 02:12:29PM -0700, Laura Abbott wrote:
This conflicts with patches already in drm-misc.git/drm-misc-next. Can you pls rebase (and then also pls add the missing kerneldoc for the new drm_gem_prime_import_platform function, plus links between the two any why they exists/when to use each)?
Noticed this when I tried to apply them.
Pls keep Chris' r-b on the patch when resending.
Thanks, Daniel
Enable the GEM dma-buf import interfaces in addition to the export interfaces. This lets vgem be used as a test source for other allocators (e.g. Ion).
Signed-off-by: Laura Abbott labbott@redhat.com --- v2: Don't require vgem allocated buffers to existing in memory before importing. --- drivers/gpu/drm/vgem/vgem_drv.c | 133 +++++++++++++++++++++++++++++++--------- drivers/gpu/drm/vgem/vgem_drv.h | 2 + 2 files changed, 106 insertions(+), 29 deletions(-)
diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c index 1b02e56..73a619a 100644 --- a/drivers/gpu/drm/vgem/vgem_drv.c +++ b/drivers/gpu/drm/vgem/vgem_drv.c @@ -34,6 +34,9 @@ #include <linux/ramfs.h> #include <linux/shmem_fs.h> #include <linux/dma-buf.h> + +#include <drm/drmP.h> + #include "vgem_drv.h"
#define DRIVER_NAME "vgem" @@ -46,6 +49,12 @@ static void vgem_gem_free_object(struct drm_gem_object *obj) { struct drm_vgem_gem_object *vgem_obj = to_vgem_bo(obj);
+ if (vgem_obj->pages) + drm_free_large(vgem_obj->pages); + + if (obj->import_attach) + drm_prime_gem_destroy(obj, vgem_obj->table); + drm_gem_object_release(obj); kfree(vgem_obj); } @@ -56,26 +65,48 @@ static int vgem_gem_fault(struct vm_fault *vmf) struct drm_vgem_gem_object *obj = vma->vm_private_data; /* We don't use vmf->pgoff since that has the fake offset */ unsigned long vaddr = vmf->address; - struct page *page; - - page = shmem_read_mapping_page(file_inode(obj->base.filp)->i_mapping, - (vaddr - vma->vm_start) >> PAGE_SHIFT); - if (!IS_ERR(page)) { - vmf->page = page; - return 0; - } else switch (PTR_ERR(page)) { - case -ENOSPC: - case -ENOMEM: - return VM_FAULT_OOM; - case -EBUSY: - return VM_FAULT_RETRY; - case -EFAULT: - case -EINVAL: - return VM_FAULT_SIGBUS; - default: - WARN_ON_ONCE(PTR_ERR(page)); - return VM_FAULT_SIGBUS; + int ret; + loff_t num_pages; + pgoff_t page_offset; + page_offset = (vaddr - vma->vm_start) >> PAGE_SHIFT; + + num_pages = DIV_ROUND_UP(obj->base.size, PAGE_SIZE); + + if (page_offset > num_pages) + return VM_FAULT_SIGBUS; + + if (obj->pages) { + get_page(obj->pages[page_offset]); + vmf->page = obj->pages[page_offset]; + ret = 0; + } else { + struct page *page; + + page = shmem_read_mapping_page( + file_inode(obj->base.filp)->i_mapping, + page_offset); + if (!IS_ERR(page)) { + vmf->page = page; + ret = 0; + } else switch (PTR_ERR(page)) { + case -ENOSPC: + case -ENOMEM: + ret = VM_FAULT_OOM; + break; + case -EBUSY: + ret = VM_FAULT_RETRY; + break; + case -EFAULT: + case -EINVAL: + ret = VM_FAULT_SIGBUS; + break; + default: + WARN_ON(PTR_ERR(page)); + ret = VM_FAULT_SIGBUS; + } + } + return ret; }
static const struct vm_operations_struct vgem_gem_vm_ops = { @@ -112,12 +143,8 @@ static void vgem_preclose(struct drm_device *dev, struct drm_file *file) kfree(vfile); }
-/* ioctls */ - -static struct drm_gem_object *vgem_gem_create(struct drm_device *dev, - struct drm_file *file, - unsigned int *handle, - unsigned long size) +static struct drm_vgem_gem_object *__vgem_gem_create(struct drm_device *dev, + unsigned long size) { struct drm_vgem_gem_object *obj; int ret; @@ -127,8 +154,31 @@ static struct drm_gem_object *vgem_gem_create(struct drm_device *dev, return ERR_PTR(-ENOMEM);
ret = drm_gem_object_init(dev, &obj->base, roundup(size, PAGE_SIZE)); - if (ret) - goto err_free; + if (ret) { + kfree(obj); + return ERR_PTR(ret); + } + + return obj; +} + +static void __vgem_gem_destroy(struct drm_vgem_gem_object *obj) +{ + drm_gem_object_release(&obj->base); + kfree(obj); +} + +static struct drm_gem_object *vgem_gem_create(struct drm_device *dev, + struct drm_file *file, + unsigned int *handle, + unsigned long size) +{ + struct drm_vgem_gem_object *obj; + int ret; + + obj = __vgem_gem_create(dev, size); + if (IS_ERR(obj)) + return ERR_CAST(obj);
ret = drm_gem_handle_create(file, &obj->base, handle); drm_gem_object_unreference_unlocked(&obj->base); @@ -137,9 +187,8 @@ static struct drm_gem_object *vgem_gem_create(struct drm_device *dev,
return &obj->base;
-err_free: - kfree(obj); err: + __vgem_gem_destroy(obj); return ERR_PTR(ret); }
@@ -256,6 +305,29 @@ static struct sg_table *vgem_prime_get_sg_table(struct drm_gem_object *obj) return st; }
+static struct drm_gem_object *vgem_prime_import_sg_table(struct drm_device *dev, + struct dma_buf_attachment *attach, struct sg_table *sg) +{ + struct drm_vgem_gem_object *obj; + int npages; + + obj = __vgem_gem_create(dev, attach->dmabuf->size); + if (IS_ERR(obj)) + return ERR_CAST(obj); + + npages = PAGE_ALIGN(attach->dmabuf->size) / PAGE_SIZE; + + obj->table = sg; + obj->pages = drm_malloc_ab(npages, sizeof(struct page *)); + if (!obj->pages) { + __vgem_gem_destroy(obj); + return ERR_PTR(-ENOMEM); + } + drm_prime_sg_to_page_addr_arrays(obj->table, obj->pages, NULL, + npages); + return &obj->base; +} + static void *vgem_prime_vmap(struct drm_gem_object *obj) { long n_pages = obj->size >> PAGE_SHIFT; @@ -314,8 +386,11 @@ static struct drm_driver vgem_driver = { .dumb_map_offset = vgem_gem_dumb_map,
.prime_handle_to_fd = drm_gem_prime_handle_to_fd, + .prime_fd_to_handle = drm_gem_prime_fd_to_handle, .gem_prime_pin = vgem_prime_pin, + .gem_prime_import = drm_gem_prime_import_platform, .gem_prime_export = drm_gem_prime_export, + .gem_prime_import_sg_table = vgem_prime_import_sg_table, .gem_prime_get_sg_table = vgem_prime_get_sg_table, .gem_prime_vmap = vgem_prime_vmap, .gem_prime_vunmap = vgem_prime_vunmap, diff --git a/drivers/gpu/drm/vgem/vgem_drv.h b/drivers/gpu/drm/vgem/vgem_drv.h index cb59c7a..1aae014 100644 --- a/drivers/gpu/drm/vgem/vgem_drv.h +++ b/drivers/gpu/drm/vgem/vgem_drv.h @@ -43,6 +43,8 @@ struct vgem_file { #define to_vgem_bo(x) container_of(x, struct drm_vgem_gem_object, base) struct drm_vgem_gem_object { struct drm_gem_object base; + struct page **pages; + struct sg_table *table; };
int vgem_fence_open(struct vgem_file *file);
On Wed, Apr 26, 2017 at 02:12:30PM -0700, Laura Abbott wrote:
Just drm_free_large(vgem_obj->pages); NULL -> kfree() which is NULL safe.
The series lgtm, doesn't compromise on any of the existing tests.
All 3, Reviewed-by: Chris Wilson chris@chris-wilson.co.uk
You could always get Joonas to nitpick over style...
break; -Chris
dri-devel@lists.freedesktop.org
-
Chris Wilson -
Daniel Vetter -
Laura Abbott