Re: [Intel-gfx] [PATCHv4 3/3] drm/vgem: Enable dmabuf import interfaces
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
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
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
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
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, &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 *o