Re: [PATCH] drm/prime: Fix mmap fake offset for drm_gem_object_funcs.mmap

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Oct 24, 2019 at 11:02:40AM +0200, Gerd Hoffmann wrote:
> On Wed, Oct 23, 2019 at 05:22:26PM -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.
> 
> Long-term it is probably a good idea to just remove the fake offset
> handling from drivers.  But that'll only work once all drivers switched
> away from custom fops->mmap handlers so we can handle the offset -> obj
> lookup in the drm core for everybody.
> 
> So let's go this way for now.
> 
> Acked-by: Gerd Hoffmann <kraxel@redhat.com>

Uh this sounds like doubling down on rather horrible semantics. Can we at
least stop the mess instead of baking it in for real? The hook is very
very new after all. I.e.
- Document that obj->funcs->mmap will have 0 offset in the kerneldoc.
- Remove the subtracting from the shmem helper
- In ttm_bo_mmap_obj re-add the offset with a huge FIXME comment.
- Adjust drm_gem_mmap_obj to do that same for obj->funcs->mmap and also
  document the expectation there too.

This feels like very much going the wrong direction ...

Also I guess Gerd didn't really test this prime mmap support?

Thanks, Daniel
> 
> > However, for mmap() on dmabufs, there is not
> > a fake offset. To fix this, we need to add the fake offset just like the
> > driver->fops->mmap() code path.
> > 
> > Fixes: c40069cb7bd6 ("drm: add mmap() to drm_gem_object_funcs")
> > Cc: Gerd Hoffmann <kraxel@redhat.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Signed-off-by: Rob Herring <robh@kernel.org>
> > ---
> > I ran into this while working on converting vgem to shmem helpers and
> > the IGT vgem_basic dmabuf-mmap test failed. This fixes shmem, but I
> > have checked any other users of the new mmap hook.
> > Rob
> > 
> >  drivers/gpu/drm/drm_prime.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> > index 0814211b0f3f..5d06690a2e9d 100644
> > --- a/drivers/gpu/drm/drm_prime.c
> > +++ b/drivers/gpu/drm/drm_prime.c
> > @@ -713,6 +713,8 @@ int drm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
> >  	struct file *fil;
> >  	int ret;
> >  
> > +	vma->vm_pgoff += drm_vma_node_start(&obj->vma_node);
> > +
> >  	if (obj->funcs && obj->funcs->mmap) {
> >  		ret = obj->funcs->mmap(obj, vma);
> >  		if (ret)
> > @@ -737,8 +739,6 @@ int drm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
> >  	if (ret)
> >  		goto out;
> >  
> > -	vma->vm_pgoff += drm_vma_node_start(&obj->vma_node);
> > -
> >  	ret = obj->dev->driver->fops->mmap(fil, vma);
> >  
> >  	drm_vma_node_revoke(&obj->vma_node, priv);
> > -- 
> > 2.20.1
> > 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel