| Message ID | 20190808134417.10610-6-kraxel@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: add gem ttm helpers, rework mmap workflow. | expand |
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 >
Hi, > 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: [ snip ] > 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. Yep, makes sense. > 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. Looks easy on a quick glance. So something like the patch below (quick sketch, not tested yet other than compiling it)? cheers, Gerd From c13b30d776fb593a03473fa3bc93baf470cba728 Mon Sep 17 00:00:00 2001 From: Gerd Hoffmann <kraxel@redhat.com> Date: Wed, 19 Jun 2019 14:26:51 +0200 Subject: [PATCH] drm: add mmap() to drm_gem_object_funcs drm_gem_object_funcs->vm_ops alone can't handle everything which needs to be done for mmap(), tweaking vm_flags for example. So add a new mmap() callback to drm_gem_object_funcs where this code can go to. Note that the vm_ops field is not used in case the mmap callback is presnt, it is expected that the callback sets vma->vm_ops instead. drm_gem_mmap_obj() will use the new callback for object specific mmap setup. With this in place the need for driver-speific fops->mmap callbacks goes away, drm_gem_mmap can be hooked instead. drm_gem_prime_mmap() will use the new callback too to just mmap gem objects directly instead of jumping though loops to make drm_gem_object_lookup() and fops->mmap work. Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- include/drm/drm_gem.h | 14 ++++++++++++++ drivers/gpu/drm/drm_gem.c | 26 +++++++++++++++++--------- drivers/gpu/drm/drm_prime.c | 9 +++++++++ 3 files changed, 40 insertions(+), 9 deletions(-) diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h index 6aaba14f5972..e71f75a2ab57 100644 --- a/include/drm/drm_gem.h +++ b/include/drm/drm_gem.h @@ -150,6 +150,20 @@ struct drm_gem_object_funcs { */ void (*vunmap)(struct drm_gem_object *obj, void *vaddr); + /** + * @mmap: + * + * Handle mmap() of the gem object, setup vma accordingly. + * + * This callback is optional. + * + * The callback is used by by both drm_gem_mmap_obj() and + * drm_gem_prime_mmap(). When @mmap is present @vm_ops is not + * used, the @mmap callback must set vma->vm_ops instead. + * + */ + int (*mmap)(struct drm_gem_object *obj, struct vm_area_struct *vma); + /** * @vm_ops: * diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 6854f5867d51..aabde003ac4a 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -1099,22 +1099,30 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size, struct vm_area_struct *vma) { struct drm_device *dev = obj->dev; + int ret; /* Check for valid 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; + if (obj->funcs && obj->funcs->mmap) { + ret = obj->funcs->mmap(obj, vma); + if (ret) + return ret; + } 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); + } - 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); /* Take a ref for this mapping of the object, so that the fault * handler can dereference the mmap offset's pointer to the object. diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index 0a2316e0e812..0814211b0f3f 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -713,6 +713,15 @@ int drm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma) struct file *fil; int ret; + if (obj->funcs && obj->funcs->mmap) { + ret = obj->funcs->mmap(obj, vma); + if (ret) + return ret; + vma->vm_private_data = obj; + drm_gem_object_get(obj); + return 0; + } + priv = kzalloc(sizeof(*priv), GFP_KERNEL); fil = kzalloc(sizeof(*fil), GFP_KERNEL); if (!priv || !fil) {
On Fri, Sep 6, 2019 at 2:13 PM Gerd Hoffmann <kraxel@redhat.com> wrote: > > Hi, > > > 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: > > [ snip ] > > > 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. > > Yep, makes sense. > > > 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. > > Looks easy on a quick glance. So something like the patch below (quick > sketch, not tested yet other than compiling it)? Yup, looks good, and if it works I'm happy to smash r-b and a-b tags over this. One thought below. > cheers, > Gerd > > From c13b30d776fb593a03473fa3bc93baf470cba728 Mon Sep 17 00:00:00 2001 > From: Gerd Hoffmann <kraxel@redhat.com> > Date: Wed, 19 Jun 2019 14:26:51 +0200 > Subject: [PATCH] drm: add mmap() to drm_gem_object_funcs > > drm_gem_object_funcs->vm_ops alone can't handle everything which needs > to be done for mmap(), tweaking vm_flags for example. So add a new > mmap() callback to drm_gem_object_funcs where this code can go to. > > Note that the vm_ops field is not used in case the mmap callback is > presnt, it is expected that the callback sets vma->vm_ops instead. > > drm_gem_mmap_obj() will use the new callback for object specific mmap > setup. With this in place the need for driver-speific fops->mmap > callbacks goes away, drm_gem_mmap can be hooked instead. > > drm_gem_prime_mmap() will use the new callback too to just mmap gem > objects directly instead of jumping though loops to make > drm_gem_object_lookup() and fops->mmap work. > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > --- > include/drm/drm_gem.h | 14 ++++++++++++++ > drivers/gpu/drm/drm_gem.c | 26 +++++++++++++++++--------- > drivers/gpu/drm/drm_prime.c | 9 +++++++++ > 3 files changed, 40 insertions(+), 9 deletions(-) > > diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h > index 6aaba14f5972..e71f75a2ab57 100644 > --- a/include/drm/drm_gem.h > +++ b/include/drm/drm_gem.h > @@ -150,6 +150,20 @@ struct drm_gem_object_funcs { > */ > void (*vunmap)(struct drm_gem_object *obj, void *vaddr); > > + /** > + * @mmap: > + * > + * Handle mmap() of the gem object, setup vma accordingly. > + * > + * This callback is optional. > + * > + * The callback is used by by both drm_gem_mmap_obj() and > + * drm_gem_prime_mmap(). When @mmap is present @vm_ops is not > + * used, the @mmap callback must set vma->vm_ops instead. > + * > + */ > + int (*mmap)(struct drm_gem_object *obj, struct vm_area_struct *vma); > + > /** > * @vm_ops: > * > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c > index 6854f5867d51..aabde003ac4a 100644 > --- a/drivers/gpu/drm/drm_gem.c > +++ b/drivers/gpu/drm/drm_gem.c > @@ -1099,22 +1099,30 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size, > struct vm_area_struct *vma) > { > struct drm_device *dev = obj->dev; > + int ret; > > /* Check for valid 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; > + if (obj->funcs && obj->funcs->mmap) { > + ret = obj->funcs->mmap(obj, vma); > + if (ret) > + return ret; > + } 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); > + } Totally unrelated discussion around HMM lead me to a bit a chase, and realizing that we probably want a WARN_ON(!(vma->vm_flags & VM_SPECIAL)); here, to make sure drivers set at least one of the "this is a special vma, don't try to treat it like pagecache/anon memory". Just to be on the safe side. Maybe we also want to keep the entire vma->vm_flags assignment in the common code, but at least the WARN_ON would be a good check I think. Maybe also check for VM_DONTEXPAND while at it (that would also break things badly if it's not set). VM_DONTDUMP I think is leftover from when drm drivers exposed register mmaps to userspace. Anyway, just some detail questions, I think this looks real good. Thanks, Daniel > - 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); > > /* Take a ref for this mapping of the object, so that the fault > * handler can dereference the mmap offset's pointer to the object. > diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c > index 0a2316e0e812..0814211b0f3f 100644 > --- a/drivers/gpu/drm/drm_prime.c > +++ b/drivers/gpu/drm/drm_prime.c > @@ -713,6 +713,15 @@ int drm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma) > struct file *fil; > int ret; > > + if (obj->funcs && obj->funcs->mmap) { > + ret = obj->funcs->mmap(obj, vma); > + if (ret) > + return ret; > + vma->vm_private_data = obj; > + drm_gem_object_get(obj); > + return 0; > + } > + > priv = kzalloc(sizeof(*priv), GFP_KERNEL); > fil = kzalloc(sizeof(*fil), GFP_KERNEL); > if (!priv || !fil) { > -- > 2.18.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Hi, > > + 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); > > + } > > Totally unrelated discussion around HMM lead me to a bit a chase, and > realizing that we probably want a > > WARN_ON(!(vma->vm_flags & VM_SPECIAL)); > > here, to make sure drivers set at least one of the "this is a special > vma, don't try to treat it like pagecache/anon memory". Just to be on > the safe side. Maybe we also want to keep the entire vma->vm_flags > assignment in the common code, but at least the WARN_ON would be a > good check I think. Maybe also check for VM_DONTEXPAND while at it Hmm. VM_SPECIAL is this: #define VM_SPECIAL (VM_IO | VM_DONTEXPAND | VM_PFNMAP | VM_MIXEDMAP) Requiring VM_DONTEXPAND makes sense for sure. Dunno about the other ones. For drm_gem_vram_helper VM_IO + VM_PFNMAP are needed. But we also have drm_gem_shmem_helper which backs objects with normal pages. In fact drm_gem_shmem_mmap does this: /* VM_PFNMAP was set by drm_gem_mmap() */ vma->vm_flags &= ~VM_PFNMAP; vma->vm_flags |= VM_MIXEDMAP; include/linux/mm.h isn't very helpful in explaining how VM_MIXEDMAP should be used, only saying can be both "struct page" and pfnmap, so I'm not sure why VM_MIXEDMAP is set here, it should always be "struct page" for shmem, no? cheers, Gerd
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); + /** * @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;
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(+)