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 <labbott@xxxxxxxxxx>
> ---
> 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 <chris@xxxxxxxxxxxxxxxxxx>

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