Re: [Intel-gfx] [PATCHv4 3/3] drm/vgem: Enable dmabuf import interfaces

2017-05-05 Thread Chris Wilson 
On Thu, May 04, 2017 at 09:09:54PM -0700, Joe Perches wrote:
> On Thu, 2017-05-04 at 21:25 +0100, Chris Wilson wrote:
> > On Thu, May 04, 2017 at 11:45:48AM -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).
> > > 
> > > Reviewed-by: Chris Wilson 
> > > Signed-off-by: Laura Abbott 
> > > ---
> > > v4: Use new drm_gem_prime_import_dev function
> > > ---
> > >  static const struct vm_operations_struct vgem_gem_vm_ops = {
> > > @@ -114,12 +142,8 @@ static void vgem_postclose(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)
> > 
> > I'm going to guess that doesn't line up anymore. If checkpatch isn't
> > complaining, then sorry for the noise.
> 
> Because of the very long identifiers, perhaps a
> nicer way to write this is like:
> 
> static struct drm_vgem_gem_object *
> __vgen_gem_create(struct drm_device *dev, unsigned long size);

Yes, we frequently use that pattern for very_long_function_names.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCHv4 3/3] drm/vgem: Enable dmabuf import interfaces

2017-05-05 Thread Daniel Vetter 
On Thu, May 04, 2017 at 09:25:03PM +0100, Chris Wilson wrote:
> On Thu, May 04, 2017 at 11:45:48AM -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).
> > 
> > Reviewed-by: Chris Wilson 
> > Signed-off-by: Laura Abbott 
> > ---
> > v4: Use new drm_gem_prime_import_dev function
> > ---
> >  static const struct vm_operations_struct vgem_gem_vm_ops = {
> > @@ -114,12 +142,8 @@ static void vgem_postclose(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)
> 
> I'm going to guess that doesn't line up anymore. If checkpatch isn't
> complaining, then sorry for the noise.
> 
> > +static struct drm_gem_object *vgem_gem_create(struct drm_device *dev,
> > + struct drm_file *file,
> > + unsigned int *handle,
> > + unsigned long size)
> 
> Ditto.
> 
> Lgtm, so r-b still good.

I applied all three as-is, I think we can polish more as follow-ups.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCHv4 3/3] drm/vgem: Enable dmabuf import interfaces

2017-05-04 Thread Joe Perches 
On Thu, 2017-05-04 at 21:25 +0100, Chris Wilson wrote:
> On Thu, May 04, 2017 at 11:45:48AM -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).
> > 
> > Reviewed-by: Chris Wilson 
> > Signed-off-by: Laura Abbott 
> > ---
> > v4: Use new drm_gem_prime_import_dev function
> > ---
> >  static const struct vm_operations_struct vgem_gem_vm_ops = {
> > @@ -114,12 +142,8 @@ static void vgem_postclose(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)
> 
> I'm going to guess that doesn't line up anymore. If checkpatch isn't
> complaining, then sorry for the noise.

Because of the very long identifiers, perhaps a
nicer way to write this is like:

static struct drm_vgem_gem_object *
__vgen_gem_create(struct drm_device *dev, unsigned long size);

> > +static struct drm_gem_object *vgem_gem_create(struct drm_device *dev,
> > + struct drm_file *file,
> > + unsigned int *handle,
> > + unsigned long size)
> 
> Ditto.

etc...

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCHv4 3/3] drm/vgem: Enable dmabuf import interfaces

2017-05-04 Thread Chris Wilson 
On Thu, May 04, 2017 at 11:45:48AM -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).
> 
> Reviewed-by: Chris Wilson 
> Signed-off-by: Laura Abbott 
> ---
> v4: Use new drm_gem_prime_import_dev function
> ---
>  static const struct vm_operations_struct vgem_gem_vm_ops = {
> @@ -114,12 +142,8 @@ static void vgem_postclose(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)

I'm going to guess that doesn't line up anymore. If checkpatch isn't
complaining, then sorry for the noise.

> +static struct drm_gem_object *vgem_gem_create(struct drm_device *dev,
> +   struct drm_file *file,
> +   unsigned int *handle,
> +   unsigned long size)

Ditto.

Lgtm, so r-b still good.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCHv4 3/3] drm/vgem: Enable dmabuf import interfaces

2017-05-04 Thread 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).

Reviewed-by: Chris Wilson 
Signed-off-by: Laura Abbott 
---
v4: Use new drm_gem_prime_import_dev function
---
 drivers/gpu/drm/vgem/vgem_drv.c | 136 +++-
 drivers/gpu/drm/vgem/vgem_drv.h |   2 +
 2 files changed, 109 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c
index d1d98af..c9381d45 100644
--- a/drivers/gpu/drm/vgem/vgem_drv.c
+++ b/drivers/gpu/drm/vgem/vgem_drv.c
@@ -48,6 +48,11 @@ static void vgem_gem_free_object(struct drm_gem_object *obj)
 {
struct drm_vgem_gem_object *vgem_obj = to_vgem_bo(obj);
 
+   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);
 }
@@ -58,26 +63,49 @@ 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;
+   }
+
}
+   return ret;
 }
 
 static const struct vm_operations_struct vgem_gem_vm_ops = {
@@ -114,12 +142,8 @@ static void vgem_postclose(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;
@@ -129,8 +153,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,
+ unsigned int *handle,
+ unsigned long size)
+{