Re: [PATCH v4 05/17] drm: add mmap() to drm_gem_object_funcs

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Aug 08, 2019 at 03:44:05PM +0200, Gerd Hoffmann wrote:
> drm_gem_object_funcs->vm_ops alone can't handle
> everything mmap() needs.  Add a new callback for it.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  include/drm/drm_gem.h     | 9 +++++++++
>  drivers/gpu/drm/drm_gem.c | 6 ++++++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> index ae693c0666cd..ee3c4ad742c6 100644
> --- a/include/drm/drm_gem.h
> +++ b/include/drm/drm_gem.h
> @@ -150,6 +150,15 @@ struct drm_gem_object_funcs {
>  	 */
>  	void (*vunmap)(struct drm_gem_object *obj, void *vaddr);
>  
> +	/**
> +	 * @mmap:
> +	 *
> +	 * Called by drm_gem_mmap() for additional checks/setup.
> +	 *
> +	 * This callback is optional.
> +	 */
> +	int (*mmap)(struct drm_gem_object *obj, struct vm_area_struct *vma);

I think if we do an mmap callback, it should replace all the mmap handling
(except the drm_gem_object_get) that drm_gem_mmap_obj does. So maybe
something like the below:


diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 6854f5867d51..e8b7779633dd 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -1104,17 +1104,22 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
 	if (obj_size < vma->vm_end - vma->vm_start)
 		return -EINVAL;
 
-	if (obj->funcs && obj->funcs->vm_ops)
-		vma->vm_ops = obj->funcs->vm_ops;
-	else if (dev->driver->gem_vm_ops)
-		vma->vm_ops = dev->driver->gem_vm_ops;
-	else
-		return -EINVAL;
-
-	vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP;
 	vma->vm_private_data = obj;
-	vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
-	vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
+
+	if (obj->funcs && obj->funcs->mmap)
+		obj->funcs->mmap(obj, vma);
+	else {
+		if (obj->funcs && obj->funcs->vm_ops)
+			vma->vm_ops = obj->funcs->vm_ops;
+		else if (dev->driver->gem_vm_ops)
+			vma->vm_ops = dev->driver->gem_vm_ops;
+		else
+			return -EINVAL;
+
+		vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP;
+		vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
+		vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
+	}
 
 	/* Take a ref for this mapping of the object, so that the fault
 	 * handler can dereference the mmap offset's pointer to the object.

Since I remember quite a few discussions where the default vma flag
wrangling we're doing is seriously getting in the way of things too.

I think even better would be if this new ->mmap hook could also be used
directly by the dma-buf mmap code, without having to jump through hoops
creating a fake file and fake vma offset and everything. I think with that
we'd have a really solid case to add this ->mmap hook.
-Daniel


> +
>  	/**
>  	 * @vm_ops:
>  	 *
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index afc38cece3f5..84db8de217e1 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -1105,6 +1105,8 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
>  		vma->vm_ops = obj->funcs->vm_ops;
>  	else if (dev->driver->gem_vm_ops)
>  		vma->vm_ops = dev->driver->gem_vm_ops;
> +	else if (obj->funcs && obj->funcs->mmap)
> +		/* obj->funcs->mmap must set vma->vm_ops */;
>  	else
>  		return -EINVAL;
>  
> @@ -1192,6 +1194,10 @@ int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma)
>  	ret = drm_gem_mmap_obj(obj, drm_vma_node_size(node) << PAGE_SHIFT,
>  			       vma);
>  
> +	if (ret == 0)
> +		if (obj->funcs->mmap)
> +			ret = obj->funcs->mmap(obj, vma);
> +
>  	drm_gem_object_put_unlocked(obj);
>  
>  	return ret;
> -- 
> 2.18.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch