Re: [RFC PATCHv2 3/3] drm/vgem: Enable dmabuf import interfaces

[Chris Wilson]

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 
> ---
> 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 
>  #include 
>  #include 
> +
> +#include 
> +
>  #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 

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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC PATCHv2 3/3] drm/vgem: Enable dmabuf import interfaces

[Laura Abbott]

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 
---
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 
 #include 
 #include 
+
+#include 
+
 #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, >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(>base);
+   kfree(obj);
+}
+
+static struct drm_gem_object *vgem_gem_create(struct drm_device *dev,
+ struct drm_file *file,
+