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

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