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

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

[Rob Herring]

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, 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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Gerd Hoffmann]

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>

> 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
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

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

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

[Rob Herring]

On Thu, Oct 24, 2019 at 7:32 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> 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.

Okay.

> This feels like very much going the wrong direction ...
>
> Also I guess Gerd didn't really test this prime mmap support?

Perhaps because at least parts of the IGT "vgem" tests really have
nothing specific for "vgem" and there doesn't seem to be another test
case that does run doing the same thing. And none of the IGT prime
tests run without an Intel driver. Looking at IGT always makes me sad,
and then I move on to other things... </rant>

BTW, are there IGT test results for vgem/vkms somewhere? I didn't have
any luck finding anything.

Rob
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

On Thu, Oct 24, 2019 at 4:39 PM Rob Herring <robh@kernel.org> wrote:
>
> On Thu, Oct 24, 2019 at 7:32 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > 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.
>
> Okay.
>
> > This feels like very much going the wrong direction ...
> >
> > Also I guess Gerd didn't really test this prime mmap support?
>
> Perhaps because at least parts of the IGT "vgem" tests really have
> nothing specific for "vgem" and there doesn't seem to be another test
> case that does run doing the same thing. And none of the IGT prime
> tests run without an Intel driver. Looking at IGT always makes me sad,
> and then I move on to other things... </rant>

The only prime test that could be made generic is the kms one, which
still requires crc to make sure the stuff actually works. Everything
else is kinda device specific (and vgem is just the test vehicle to
make writing a testcase possible without actually needing 2 gpus).
Maybe we could do a dumb buffer export, then mmap test, but that
doesn't exist. I think for the prime+kms one there's actually patches
floating around to make it generic.

For the others I really don't think you can make them much generic.

> BTW, are there IGT test results for vgem/vkms somewhere? I didn't have
> any luck finding anything.

Unfortunately not (yet). I have this dream that once we have the
kernels on gitlab I'll make igt runs on a qemu run with vkms+vgem only
happen, as the first baseline. But we're some ways off from that still
:-/
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Gerd Hoffmann]

On Thu, Oct 24, 2019 at 02:32:14PM +0200, Daniel Vetter wrote:
> 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.

Ah, right, we can also tweak the drivers which need it instead of doing
it for everybody in drm_gem_mmap_obj().

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

I did, but I think the test[1] is too simple.  It'll only try to mmap()
the dma-buf, but doesn't touch the pages, so it doesn't see the shmem
fault callback throwing SIGBUS.

cheers,
  Gerd

[1] https://git.kraxel.org/cgit/drminfo/tree/prime.c

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel