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(-)
--
2.7.4
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 <[email protected]>
---
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);
}
--
2.7.4
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 <[email protected]>
---
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);
--
2.7.4
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 <[email protected]>
---
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,
--
2.7.4
On Wed, Apr 26, 2017 at 02:12:30PM -0700, Laura Abbott wrote:
> 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 <[email protected]>
> ---
> 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);
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 <[email protected]>
You could always get Joonas to nitpick over style...
> +
> + 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;
break;
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
On Wed, Apr 26, 2017 at 02:12:27PM -0700, Laura Abbott wrote:
> 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.
Note, that it will be good to cc:intel-gfx@ so that our CI does run it
over the existing vgem tests.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre