[PATCH v2] drm/gem: Fix mmap fake offset handling for drm_gem_object_funcs.mmap

[Daniel Vetter]

On Fri, Oct 25, 2019 at 09:30:42AM +0200, Daniel Vetter wrote:
> On Thu, Oct 24, 2019 at 02:18:59PM -0500, Rob Herring wrote:
> > Commit c40069cb7bd6 ("drm: add mmap() to drm_gem_object_funcs")
> > introduced a GEM object mmap() hook which is expected to subtract the
> > fake offset from vm_pgoff. However, for mmap() on dmabufs, there is not
> > a fake offset.
> > 
> > To fix this, let's always call mmap() object callback with an offset of 0,
> > and leave it up to drm_gem_mmap_obj() to remove the fake offset.
> > 
> > TTM still needs the fake offset, so we have to add it back until that's
> > fixed.
> > 
> > Fixes: c40069cb7bd6 ("drm: add mmap() to drm_gem_object_funcs")
> > Cc: Gerd Hoffmann <kraxel at redhat.com>
> > Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> > Signed-off-by: Rob Herring <robh at kernel.org>
> > ---
> > v2:
> > - Move subtracting the fake offset out of mmap() obj callbacks.
> > 
> > I've tested shmem, but not ttm. Hopefully, I understood what's needed 
> > for TTM.

So unfortunately I'm already having regrets on this. We might even have
broken some of the ttm drivers here.

Trouble is, if you need to shoot down userspace ptes of a bo (because it's
getting moved or whatever), then we do that with unmap_mapping_range.
Which means each bo needs to be mapping with a unique (offset,
adress_space), or it won't work. By remapping all the bo to 0 we've broken
this. We've also had this broken this for a while for the simplistic
dma-buf mmap, since without any further action we'll reuse the address
space of the dma-buf inode, not of the drm_device.

Strangely both etnaviv and msm have a comment to that effect - grep for
unmap_mapping_range. But neither actually uses it.

Not exactly sure what's the best course of action here now.

Also adding Thomas Zimmermann, who's worked on all the vram helpers.
-Daniel

> > 
> > Rob
> > 
> >  drivers/gpu/drm/drm_gem.c              | 3 +++
> >  drivers/gpu/drm/drm_gem_shmem_helper.c | 3 ---
> >  drivers/gpu/drm/ttm/ttm_bo_vm.c        | 7 +++++++
> >  include/drm/drm_gem.h                  | 4 +++-
> >  4 files changed, 13 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > index 56f42e0f2584..2f2b889096b0 100644
> > --- a/drivers/gpu/drm/drm_gem.c
> > +++ b/drivers/gpu/drm/drm_gem.c
> > @@ -1106,6 +1106,9 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
> >  		return -EINVAL;
> >  
> >  	if (obj->funcs && obj->funcs->mmap) {
> > +		/* Remove the fake offset */
> > +		vma->vm_pgoff -= drm_vma_node_start(&obj->vma_node);
> > +
> >  		ret = obj->funcs->mmap(obj, vma);
> >  		if (ret)
> >  			return ret;
> > diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> > index a878c787b867..e8061c64c480 100644
> > --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> > +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> > @@ -542,9 +542,6 @@ int drm_gem_shmem_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
> >  	vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
> >  	vma->vm_ops = &drm_gem_shmem_vm_ops;
> >  
> > -	/* Remove the fake offset */
> > -	vma->vm_pgoff -= drm_vma_node_start(&shmem->base.vma_node);
> > -
> >  	return 0;
> >  }
> >  EXPORT_SYMBOL_GPL(drm_gem_shmem_mmap);
> > diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c b/drivers/gpu/drm/ttm/ttm_bo_vm.c
> > index 1a9db691f954..08902c7290a5 100644
> > --- a/drivers/gpu/drm/ttm/ttm_bo_vm.c
> > +++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c
> > @@ -482,6 +482,13 @@ EXPORT_SYMBOL(ttm_bo_mmap);
> >  int ttm_bo_mmap_obj(struct vm_area_struct *vma, struct ttm_buffer_object *bo)
> >  {
> >  	ttm_bo_get(bo);
> > +
> > +	/*
> > +	 * FIXME: &drm_gem_object_funcs.mmap is called with the fake offset
> > +	 * removed. Add it back here until the rest of TTM works without it.
> > +	 */
> > +	vma->vm_pgoff += drm_vma_node_start(&bo->base.vma_node);
> > +
> >  	ttm_bo_mmap_vma_setup(bo, vma);
> >  	return 0;
> >  }
> > diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> > index e71f75a2ab57..c56cbb3509e0 100644
> > --- a/include/drm/drm_gem.h
> > +++ b/include/drm/drm_gem.h
> > @@ -159,7 +159,9 @@ struct drm_gem_object_funcs {
> >  	 *
> >  	 * 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.
> > +	 * used, the @mmap callback must set vma->vm_ops instead. The @mmap
> > +	 * callback is always called with a 0 offset. The caller will remove
> > +	 * the fake offset as necessary.
> >  	 *
> 
> Maybe remove this empty comment line here while at it. With that
> 
> Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> 
> I think I'll follow up with a patch to annotate drm_gem_mmap_obj as
> deprecated and that instead this here should be used.
> -Daniel
> 
> >  	 */
> >  	int (*mmap)(struct drm_gem_object *obj, struct vm_area_struct *vma);
> > -- 
> > 2.20.1
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

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