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

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@redhat.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Rob Herring <robh@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.

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

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.

Fixing ttm looks easy, there are not many calls to drm_vma_node_start()
the ttm code.  Can look into this when I'm back from kvm forum @ lyon.

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

Yes, that should keep ttm happy for now.  Survived a quick smoke test
with qemu and bochs.

Acked-by: Gerd Hoffmann <kraxel@redhat.com>

cheers,
  Gerd

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@redhat.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Rob Herring <robh@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.
> 
> 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@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
>

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@redhat.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Signed-off-by: Rob Herring <robh@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@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

On Fri, Nov 08, 2019 at 05:27:59PM +0100, Daniel Vetter wrote:
> 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@redhat.com>
> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Signed-off-by: Rob Herring <robh@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.

Correction, I missed the unmap_mapping_range in the vma node manager
header, so didn't spot the drivers using drm_vma_node_unmap. We did break
all the ttm stuff :-/

Plus panfrost, which is using drm_gem_shmem_purge_locked.
-Daniel

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

On Fri, Nov 08, 2019 at 05:55:28PM +0100, Daniel Vetter wrote:
> On Fri, Nov 08, 2019 at 05:27:59PM +0100, Daniel Vetter wrote:
> > 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@redhat.com>
> > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > Signed-off-by: Rob Herring <robh@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.
> 
> Correction, I missed the unmap_mapping_range in the vma node manager
> header, so didn't spot the drivers using drm_vma_node_unmap. We did break
> all the ttm stuff :-/

ttm still uses the offset, now added in ttm_bo_mmap_obj().  So, for
normal mmap behavior did not change I think.  The simplistic dma-buf
mmap did change, it now uses the offset because it goes through
ttm_bo_mmap_obj() too.

Not fully sure which address space is used for dma-bufs though.  As far
I can see neither the old nor the new dma-buf mmap code path touch
vma->vm_file (unless the driver does explicitly care, like msm does in
msm_gem_mmap_obj).

> Plus panfrost, which is using drm_gem_shmem_purge_locked.

Hmm, looking at drm_gem_shmem_purge_locked I'm wondering why it uses a
mix of dev->anon_inode->i_mapping and file_inode(obj->filp)->i_mapping.

Also shmem helpers used a zero vm_pgoff before, only difference is the
change is applied in drm_gem_mmap_obj() now instead of somewhere in the
shmem helper code.

slightly confused,
  Gerd

Hi

Am 08.11.19 um 17:27 schrieb 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@redhat.com>

>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>

>>> Signed-off-by: Rob Herring <robh@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.


VRAM helpers use drm_gem_ttm_mmap(), which wraps ttm_bo_mmap_obj().
These changes should be transparent.

Best regards
Thomas

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

> 


-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer

On Tue, Nov 12, 2019 at 10:26:44AM +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 08.11.19 um 17:27 schrieb 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@redhat.com>
> >>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>> Signed-off-by: Rob Herring <robh@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.
> 
> VRAM helpers use drm_gem_ttm_mmap(), which wraps ttm_bo_mmap_obj().
> These changes should be transparent.

There's still the issue that for dma-buf mmap vs drm mmap you use
different f_mapping, which means ttm's pte shootdown won't work correctly
for dma-buf mmaps. But yeah normal operation for ttm/vram helpers should
be fine.
-Daniel

> 
> Best regards
> Thomas
> 
> > -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@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
> > 
> 
> -- 
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Felix Imendörffer
>

On Tue, Nov 12, 2019 at 09:52:54AM +0100, Gerd Hoffmann wrote:
> On Fri, Nov 08, 2019 at 05:55:28PM +0100, Daniel Vetter wrote:
> > On Fri, Nov 08, 2019 at 05:27:59PM +0100, Daniel Vetter wrote:
> > > 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@redhat.com>
> > > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > > Signed-off-by: Rob Herring <robh@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.
> > 
> > Correction, I missed the unmap_mapping_range in the vma node manager
> > header, so didn't spot the drivers using drm_vma_node_unmap. We did break
> > all the ttm stuff :-/
> 
> ttm still uses the offset, now added in ttm_bo_mmap_obj().  So, for
> normal mmap behavior did not change I think.  The simplistic dma-buf
> mmap did change, it now uses the offset because it goes through
> ttm_bo_mmap_obj() too.
> 
> Not fully sure which address space is used for dma-bufs though.  As far
> I can see neither the old nor the new dma-buf mmap code path touch
> vma->vm_file (unless the driver does explicitly care, like msm does in
> msm_gem_mmap_obj).
> 
> > Plus panfrost, which is using drm_gem_shmem_purge_locked.
> 
> Hmm, looking at drm_gem_shmem_purge_locked I'm wondering why it uses a
> mix of dev->anon_inode->i_mapping and file_inode(obj->filp)->i_mapping.
> 
> Also shmem helpers used a zero vm_pgoff before, only difference is the
> change is applied in drm_gem_mmap_obj() now instead of somewhere in the
> shmem helper code.
> 
> slightly confused,

I think summary is:
- shmem helper pte shotdown is broken for all cases now with
  obj->funcs->mmap
- ttm/vram-helpers only for dma-buf mmap redirection (because of the wrong
  f/i_mapping).

Rob, are you looking into this? I guess there's two options:
- Go back to fake offset everywhere, and weep.
- Add a per-bo mapping struct, consistently use that everywhere (probably
  more work).

If we go with weeping maybe note the 2nd option as a todo item in
todo.rst?
-Daniel

>   Gerd
>

Hi

Am 12.11.19 um 10:32 schrieb Daniel Vetter:
> On Tue, Nov 12, 2019 at 10:26:44AM +0100, Thomas Zimmermann wrote:

>> Hi

>>

>> Am 08.11.19 um 17:27 schrieb 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@redhat.com>

>>>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>

>>>>> Signed-off-by: Rob Herring <robh@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.

>>

>> VRAM helpers use drm_gem_ttm_mmap(), which wraps ttm_bo_mmap_obj().

>> These changes should be transparent.

> 

> There's still the issue that for dma-buf mmap vs drm mmap you use

> different f_mapping, which means ttm's pte shootdown won't work correctly

> for dma-buf mmaps. But yeah normal operation for ttm/vram helpers should

> be fine.


VRAM helpers don't support dmabufs.

Best regards
Thomas

> -Daniel

> 

>>

>> Best regards

>> Thomas

>>

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

>>>

>>

>> -- 

>> Thomas Zimmermann

>> Graphics Driver Developer

>> SUSE Software Solutions Germany GmbH

>> Maxfeldstr. 5, 90409 Nürnberg, Germany

>> (HRB 36809, AG Nürnberg)

>> Geschäftsführer: Felix Imendörffer

>>

> 

> 

> 

> 


-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer

On Tue, Nov 12, 2019 at 10:49:21AM +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 12.11.19 um 10:32 schrieb Daniel Vetter:
> > On Tue, Nov 12, 2019 at 10:26:44AM +0100, Thomas Zimmermann wrote:
> >> Hi
> >>
> >> Am 08.11.19 um 17:27 schrieb 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@redhat.com>
> >>>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>>>> Signed-off-by: Rob Herring <robh@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.
> >>
> >> VRAM helpers use drm_gem_ttm_mmap(), which wraps ttm_bo_mmap_obj().
> >> These changes should be transparent.
> > 
> > There's still the issue that for dma-buf mmap vs drm mmap you use
> > different f_mapping, which means ttm's pte shootdown won't work correctly
> > for dma-buf mmaps. But yeah normal operation for ttm/vram helpers should
> > be fine.
> 
> VRAM helpers don't support dmabufs.

It's not that simple.  Even when not supporting dma-buf export and
import it is still possible to create dma-bufs, import them into the
same driver (which doesn't actually export+import but just grabs a gem
object reference instead) and also to mmap them via prime/dma-buf code
path ...

Can ttm use the same trick msm uses?  Now that ttm bo's are a gem object
superclass I think we should be able to switch vma->vm_file to
bo->base.filp, simliar to msm_gem_mmap_obj(), probably best done in
drm_gem_ttm_mmap().

cheers,
  Gerd

On Tue, Nov 12, 2019 at 11:38:19AM +0100, Gerd Hoffmann wrote:
> On Tue, Nov 12, 2019 at 10:49:21AM +0100, Thomas Zimmermann wrote:
> > Hi
> > 
> > Am 12.11.19 um 10:32 schrieb Daniel Vetter:
> > > On Tue, Nov 12, 2019 at 10:26:44AM +0100, Thomas Zimmermann wrote:
> > >> Hi
> > >>
> > >> Am 08.11.19 um 17:27 schrieb 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@redhat.com>
> > >>>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > >>>>> Signed-off-by: Rob Herring <robh@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.
> > >>
> > >> VRAM helpers use drm_gem_ttm_mmap(), which wraps ttm_bo_mmap_obj().
> > >> These changes should be transparent.
> > > 
> > > There's still the issue that for dma-buf mmap vs drm mmap you use
> > > different f_mapping, which means ttm's pte shootdown won't work correctly
> > > for dma-buf mmaps. But yeah normal operation for ttm/vram helpers should
> > > be fine.
> > 
> > VRAM helpers don't support dmabufs.
> 
> It's not that simple.  Even when not supporting dma-buf export and
> import it is still possible to create dma-bufs, import them into the
> same driver (which doesn't actually export+import but just grabs a gem
> object reference instead) and also to mmap them via prime/dma-buf code
> path ...
> 
> Can ttm use the same trick msm uses?  Now that ttm bo's are a gem object
> superclass I think we should be able to switch vma->vm_file to
> bo->base.filp, simliar to msm_gem_mmap_obj(), probably best done in
> drm_gem_ttm_mmap().

bo->base.filp is the shmem inode file, and I'm not too sure how much shmem
approves of us misappropriating the mapping. For shmem only objects it
probably doesn't matter (since both gem mmaps and shmem mmaps will point
at the same underlying struct page), but for vram/ttm/anything else the
gem mmap might point into iomem, and shmem then might go boom trying to do
stuff with that. I think having our own mapping would be the cleanest
long-term approach ...
-Daniel

On Tue, Nov 12, 2019 at 3:35 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Tue, Nov 12, 2019 at 09:52:54AM +0100, Gerd Hoffmann wrote:
> > On Fri, Nov 08, 2019 at 05:55:28PM +0100, Daniel Vetter wrote:
> > > On Fri, Nov 08, 2019 at 05:27:59PM +0100, Daniel Vetter wrote:
> > > > 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@redhat.com>
> > > > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > > > Signed-off-by: Rob Herring <robh@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.
> > >
> > > Correction, I missed the unmap_mapping_range in the vma node manager
> > > header, so didn't spot the drivers using drm_vma_node_unmap. We did break
> > > all the ttm stuff :-/
> >
> > ttm still uses the offset, now added in ttm_bo_mmap_obj().  So, for
> > normal mmap behavior did not change I think.  The simplistic dma-buf
> > mmap did change, it now uses the offset because it goes through
> > ttm_bo_mmap_obj() too.
> >
> > Not fully sure which address space is used for dma-bufs though.  As far
> > I can see neither the old nor the new dma-buf mmap code path touch
> > vma->vm_file (unless the driver does explicitly care, like msm does in
> > msm_gem_mmap_obj).
> >
> > > Plus panfrost, which is using drm_gem_shmem_purge_locked.
> >
> > Hmm, looking at drm_gem_shmem_purge_locked I'm wondering why it uses a
> > mix of dev->anon_inode->i_mapping and file_inode(obj->filp)->i_mapping.

Probably my copy-n-paste from other implementations...

> > Also shmem helpers used a zero vm_pgoff before, only difference is the
> > change is applied in drm_gem_mmap_obj() now instead of somewhere in the
> > shmem helper code.
> >
> > slightly confused,

Me too.

> I think summary is:
> - shmem helper pte shotdown is broken for all cases now with
>   obj->funcs->mmap

Does it help that userspace always does a munmap before making pages purgeable?

> - ttm/vram-helpers only for dma-buf mmap redirection (because of the wrong
>   f/i_mapping).
>
> Rob, are you looking into this?

Still trying to understand all this...

> I guess there's two options:
> - Go back to fake offset everywhere, and weep.
> - Add a per-bo mapping struct, consistently use that everywhere (probably
>   more work).
>
> If we go with weeping maybe note the 2nd option as a todo item in
> todo.rst?
> -Daniel
>
> >   Gerd
> >
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

On Tue, Nov 12, 2019 at 09:37:45AM -0600, Rob Herring wrote:
> On Tue, Nov 12, 2019 at 3:35 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Tue, Nov 12, 2019 at 09:52:54AM +0100, Gerd Hoffmann wrote:
> > > On Fri, Nov 08, 2019 at 05:55:28PM +0100, Daniel Vetter wrote:
> > > > On Fri, Nov 08, 2019 at 05:27:59PM +0100, Daniel Vetter wrote:
> > > > > 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@redhat.com>
> > > > > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > > > > Signed-off-by: Rob Herring <robh@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.
> > > >
> > > > Correction, I missed the unmap_mapping_range in the vma node manager
> > > > header, so didn't spot the drivers using drm_vma_node_unmap. We did break
> > > > all the ttm stuff :-/
> > >
> > > ttm still uses the offset, now added in ttm_bo_mmap_obj().  So, for
> > > normal mmap behavior did not change I think.  The simplistic dma-buf
> > > mmap did change, it now uses the offset because it goes through
> > > ttm_bo_mmap_obj() too.
> > >
> > > Not fully sure which address space is used for dma-bufs though.  As far
> > > I can see neither the old nor the new dma-buf mmap code path touch
> > > vma->vm_file (unless the driver does explicitly care, like msm does in
> > > msm_gem_mmap_obj).
> > >
> > > > Plus panfrost, which is using drm_gem_shmem_purge_locked.
> > >
> > > Hmm, looking at drm_gem_shmem_purge_locked I'm wondering why it uses a
> > > mix of dev->anon_inode->i_mapping and file_inode(obj->filp)->i_mapping.
> 
> Probably my copy-n-paste from other implementations...
> 
> > > Also shmem helpers used a zero vm_pgoff before, only difference is the
> > > change is applied in drm_gem_mmap_obj() now instead of somewhere in the
> > > shmem helper code.
> > >
> > > slightly confused,
> 
> Me too.
> 
> > I think summary is:
> > - shmem helper pte shotdown is broken for all cases now with
> >   obj->funcs->mmap
> 
> Does it help that userspace always does a munmap before making pages purgeable?

I guess ... but kinda awkward to leave this issue in here, it's really
surprising if you call the pte shootdown function, and it doesn't work as
advertised.
-Daniel

> 
> > - ttm/vram-helpers only for dma-buf mmap redirection (because of the wrong
> >   f/i_mapping).
> >
> > Rob, are you looking into this?
> 
> Still trying to understand all this...
> 
> > I guess there's two options:
> > - Go back to fake offset everywhere, and weep.
> > - Add a per-bo mapping struct, consistently use that everywhere (probably
> >   more work).
> >
> > If we go with weeping maybe note the 2nd option as a todo item in
> > todo.rst?
> > -Daniel
> >
> > >   Gerd
> > >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch

On Tue, Nov 12, 2019 at 1:06 PM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Tue, Nov 12, 2019 at 09:37:45AM -0600, Rob Herring wrote:
> > On Tue, Nov 12, 2019 at 3:35 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > >
> > > On Tue, Nov 12, 2019 at 09:52:54AM +0100, Gerd Hoffmann wrote:
> > > > On Fri, Nov 08, 2019 at 05:55:28PM +0100, Daniel Vetter wrote:
> > > > > On Fri, Nov 08, 2019 at 05:27:59PM +0100, Daniel Vetter wrote:
> > > > > > 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@redhat.com>
> > > > > > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > > > > > Signed-off-by: Rob Herring <robh@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.
> > > > >
> > > > > Correction, I missed the unmap_mapping_range in the vma node manager
> > > > > header, so didn't spot the drivers using drm_vma_node_unmap. We did break
> > > > > all the ttm stuff :-/
> > > >
> > > > ttm still uses the offset, now added in ttm_bo_mmap_obj().  So, for
> > > > normal mmap behavior did not change I think.  The simplistic dma-buf
> > > > mmap did change, it now uses the offset because it goes through
> > > > ttm_bo_mmap_obj() too.
> > > >
> > > > Not fully sure which address space is used for dma-bufs though.  As far
> > > > I can see neither the old nor the new dma-buf mmap code path touch
> > > > vma->vm_file (unless the driver does explicitly care, like msm does in
> > > > msm_gem_mmap_obj).
> > > >
> > > > > Plus panfrost, which is using drm_gem_shmem_purge_locked.
> > > >
> > > > Hmm, looking at drm_gem_shmem_purge_locked I'm wondering why it uses a
> > > > mix of dev->anon_inode->i_mapping and file_inode(obj->filp)->i_mapping.
> >
> > Probably my copy-n-paste from other implementations...

I checked and yes, this is just copy-n-paste from msm. BTW, the code
with the unmap_mapping_range comment mentioned above doesn't apply for
shmem helpers because it applies to cacheable mappings which are (yet)
supported.

> >
> > > > Also shmem helpers used a zero vm_pgoff before, only difference is the
> > > > change is applied in drm_gem_mmap_obj() now instead of somewhere in the
> > > > shmem helper code.
> > > >
> > > > slightly confused,
> >
> > Me too.
> >
> > > I think summary is:
> > > - shmem helper pte shotdown is broken for all cases now with
> > >   obj->funcs->mmap
> >
> > Does it help that userspace always does a munmap before making pages purgeable?
>
> I guess ... but kinda awkward to leave this issue in here, it's really
> surprising if you call the pte shootdown function, and it doesn't work as
> advertised.

I was mainly wondering how this worked for us and how to hit it not
working to test fixing it.

Is there a simple way to check if a BO is mmapped or not? I'd assume
so, just don't know the answer off hand. A simple fix would be to
either require buffer is not mapped before madvise or skip purging if
it is mapped.

Rob

On Tue, Nov 12, 2019 at 10:31 PM Rob Herring <robh@kernel.org> wrote:
>
> On Tue, Nov 12, 2019 at 1:06 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Tue, Nov 12, 2019 at 09:37:45AM -0600, Rob Herring wrote:
> > > On Tue, Nov 12, 2019 at 3:35 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > >
> > > > On Tue, Nov 12, 2019 at 09:52:54AM +0100, Gerd Hoffmann wrote:
> > > > > On Fri, Nov 08, 2019 at 05:55:28PM +0100, Daniel Vetter wrote:
> > > > > > On Fri, Nov 08, 2019 at 05:27:59PM +0100, Daniel Vetter wrote:
> > > > > > > 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@redhat.com>
> > > > > > > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > > > > > > Signed-off-by: Rob Herring <robh@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.
> > > > > >
> > > > > > Correction, I missed the unmap_mapping_range in the vma node manager
> > > > > > header, so didn't spot the drivers using drm_vma_node_unmap. We did break
> > > > > > all the ttm stuff :-/
> > > > >
> > > > > ttm still uses the offset, now added in ttm_bo_mmap_obj().  So, for
> > > > > normal mmap behavior did not change I think.  The simplistic dma-buf
> > > > > mmap did change, it now uses the offset because it goes through
> > > > > ttm_bo_mmap_obj() too.
> > > > >
> > > > > Not fully sure which address space is used for dma-bufs though.  As far
> > > > > I can see neither the old nor the new dma-buf mmap code path touch
> > > > > vma->vm_file (unless the driver does explicitly care, like msm does in
> > > > > msm_gem_mmap_obj).
> > > > >
> > > > > > Plus panfrost, which is using drm_gem_shmem_purge_locked.
> > > > >
> > > > > Hmm, looking at drm_gem_shmem_purge_locked I'm wondering why it uses a
> > > > > mix of dev->anon_inode->i_mapping and file_inode(obj->filp)->i_mapping.
> > >
> > > Probably my copy-n-paste from other implementations...
>
> I checked and yes, this is just copy-n-paste from msm. BTW, the code
> with the unmap_mapping_range comment mentioned above doesn't apply for
> shmem helpers because it applies to cacheable mappings which are (yet)
> supported.
>
> > >
> > > > > Also shmem helpers used a zero vm_pgoff before, only difference is the
> > > > > change is applied in drm_gem_mmap_obj() now instead of somewhere in the
> > > > > shmem helper code.
> > > > >
> > > > > slightly confused,
> > >
> > > Me too.
> > >
> > > > I think summary is:
> > > > - shmem helper pte shotdown is broken for all cases now with
> > > >   obj->funcs->mmap
> > >
> > > Does it help that userspace always does a munmap before making pages purgeable?
> >
> > I guess ... but kinda awkward to leave this issue in here, it's really
> > surprising if you call the pte shootdown function, and it doesn't work as
> > advertised.
>
> I was mainly wondering how this worked for us and how to hit it not
> working to test fixing it.
>
> Is there a simple way to check if a BO is mmapped or not? I'd assume
> so, just don't know the answer off hand. A simple fix would be to
> either require buffer is not mapped before madvise or skip purging if
> it is mapped.

Imo simplest fix is to just go back and readd the fake offset. Ugly,
but works at least. That leaves exported dma-bufs, and maybe those
shouldn't be quite so funny in their behaviour (or we just exchange
the mapping in the vma, should be a one-liner to address that).
Workarounds and hacks in drivers definitely sounds like the wrong
approach here to me.
-Daniel

Hi,

> > > >> VRAM helpers use drm_gem_ttm_mmap(), which wraps ttm_bo_mmap_obj().
> > > >> These changes should be transparent.
> > > > 
> > > > There's still the issue that for dma-buf mmap vs drm mmap you use
> > > > different f_mapping, which means ttm's pte shootdown won't work correctly
> > > > for dma-buf mmaps. But yeah normal operation for ttm/vram helpers should
> > > > be fine.
> > > 
> > > VRAM helpers don't support dmabufs.
> > 
> > It's not that simple.  Even when not supporting dma-buf export and
> > import it is still possible to create dma-bufs, import them into the
> > same driver (which doesn't actually export+import but just grabs a gem
> > object reference instead) and also to mmap them via prime/dma-buf code
> > path ...

... but after looking again I think we are all green here.  Given that
only self-import works we'll only see vram gem objects in the mmap code
path, which should have everything set up correctly.  Same goes for qxl.

All other ttm drivers still use the old mmap code path, so all green
there too I think.  Also I somehow doubt dma-buf mmap vs. drm mmap ends
up using different f_mapping, ttm code has a WARN_ON in ttm_bo_vm_open()
which would fire should that be the case.

Do imported dma-bufs hit the drm mmap code path in the first place?
Wouldn't mmap be handled by the exporting driver?

> > Can ttm use the same trick msm uses?  Now that ttm bo's are a gem object
> > superclass I think we should be able to switch vma->vm_file to
> > bo->base.filp, simliar to msm_gem_mmap_obj(), probably best done in
> > drm_gem_ttm_mmap().
> 
> bo->base.filp is the shmem inode file, and I'm not too sure how much shmem
> approves of us misappropriating the mapping. For shmem only objects it
> probably doesn't matter (since both gem mmaps and shmem mmaps will point
> at the same underlying struct page), but for vram/ttm/anything else the
> gem mmap might point into iomem, and shmem then might go boom trying to do
> stuff with that.

Yes, agreeing here after wading through the code ...

> I think having our own mapping would be the cleanest
> long-term approach ...

You mean using per drm object (instead of per drm device) address spaces?

cheers,
  Gerd

Hi,

> > > I guess ... but kinda awkward to leave this issue in here, it's really
> > > surprising if you call the pte shootdown function, and it doesn't work as
> > > advertised.
> >
> > I was mainly wondering how this worked for us and how to hit it not
> > working to test fixing it.
> >
> > Is there a simple way to check if a BO is mmapped or not? I'd assume
> > so, just don't know the answer off hand. A simple fix would be to
> > either require buffer is not mapped before madvise or skip purging if
> > it is mapped.
> 
> Imo simplest fix is to just go back and readd the fake offset. Ugly,
> but works at least.

Well, shmem helpers removed the fake offset before, so I'm likewise
starting to wonder what exactly broke by removing the offset elsewhere.

Maybe shmem helpers where already broken before that patch.  I can see
that removing the fake offset and continuing to use a shared address
space isn't going to fly.  Unlike ttm the shmem helpers should have no
problems using obj->filp, but I can't see the shmem helper code
switching vma->vm_file over to obj->filp anywhere ...

cheers,
  Gerd

On Wed, Nov 13, 2019 at 8:39 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>   Hi,
>
> > > > >> VRAM helpers use drm_gem_ttm_mmap(), which wraps ttm_bo_mmap_obj().
> > > > >> These changes should be transparent.
> > > > >
> > > > > There's still the issue that for dma-buf mmap vs drm mmap you use
> > > > > different f_mapping, which means ttm's pte shootdown won't work correctly
> > > > > for dma-buf mmaps. But yeah normal operation for ttm/vram helpers should
> > > > > be fine.
> > > >
> > > > VRAM helpers don't support dmabufs.
> > >
> > > It's not that simple.  Even when not supporting dma-buf export and
> > > import it is still possible to create dma-bufs, import them into the
> > > same driver (which doesn't actually export+import but just grabs a gem
> > > object reference instead) and also to mmap them via prime/dma-buf code
> > > path ...
>
> ... but after looking again I think we are all green here.  Given that
> only self-import works we'll only see vram gem objects in the mmap code
> path, which should have everything set up correctly.  Same goes for qxl.
>
> All other ttm drivers still use the old mmap code path, so all green
> there too I think.  Also I somehow doubt dma-buf mmap vs. drm mmap ends
> up using different f_mapping, ttm code has a WARN_ON in ttm_bo_vm_open()
> which would fire should that be the case.
>
> Do imported dma-bufs hit the drm mmap code path in the first place?
> Wouldn't mmap be handled by the exporting driver?

drm_gem_dmabuf_mmap -> obj->funcs->mmap -> ttm_bo_mmap_obj

And I'm not seeing anyone adjusting vm_file->f_mapping anywhere here at all.

Note to hit this you need userspace to
- handle2fd on a buffer to create a dma-buf fd
- call mmap directly on that dma-buf fd

> > > Can ttm use the same trick msm uses?  Now that ttm bo's are a gem object
> > > superclass I think we should be able to switch vma->vm_file to
> > > bo->base.filp, simliar to msm_gem_mmap_obj(), probably best done in
> > > drm_gem_ttm_mmap().
> >
> > bo->base.filp is the shmem inode file, and I'm not too sure how much shmem
> > approves of us misappropriating the mapping. For shmem only objects it
> > probably doesn't matter (since both gem mmaps and shmem mmaps will point
> > at the same underlying struct page), but for vram/ttm/anything else the
> > gem mmap might point into iomem, and shmem then might go boom trying to do
> > stuff with that.
>
> Yes, agreeing here after wading through the code ...
>
> > I think having our own mapping would be the cleanest
> > long-term approach ...
>
> You mean using per drm object (instead of per drm device) address spaces?

Yup. But I think that would be quite a pile of work, since we need an
anon inode for each gem bo.
-Daniel

Hi,

> > ... but after looking again I think we are all green here.  Given that
> > only self-import works we'll only see vram gem objects in the mmap code
> > path, which should have everything set up correctly.  Same goes for qxl.
> >
> > All other ttm drivers still use the old mmap code path, so all green
> > there too I think.  Also I somehow doubt dma-buf mmap vs. drm mmap ends
> > up using different f_mapping, ttm code has a WARN_ON in ttm_bo_vm_open()
> > which would fire should that be the case.
> >
> > Do imported dma-bufs hit the drm mmap code path in the first place?
> > Wouldn't mmap be handled by the exporting driver?
> 
> drm_gem_dmabuf_mmap -> obj->funcs->mmap -> ttm_bo_mmap_obj
> 
> And I'm not seeing anyone adjusting vm_file->f_mapping anywhere here at all.

[ some more code browsing ]

Ok, I see.  dma-bufs get their own file, their own anon inode and
thereby their own address space.  So that it used when mmaping the
dma-buf.

drm filehandle's get the shared address space instead, drm_open() sets
it.

So, yes, I see the problem.  It's not new though, as far I can see the
old dma-buf mmap code path doesn't adjust f_mapping anywhere either ...

> Note to hit this you need userspace to
> - handle2fd on a buffer to create a dma-buf fd
> - call mmap directly on that dma-buf fd

Hmm, seems for handle2fd I need a dummy gem_prime_get_sg_table function
wired up even when not actually exporting/importing anything.  So I
think neither qxl nor any of the vram drivers allow to trigger that (and
no other ttm driver uses the new ttm mmap code yet).

So, $subject patch should not make things worse in ttm land.

When hacking the bochs driver to have export callbacks (without
supporting actual exports) handle2fd + mmap() callback works fine.
Didn't verify yet I actually get the correct pages mapped.  But maybe
mmap() isn't the problem when the correct address space is important for
unmap only.

Is there a good test case?

cheers,
  Gerd

On Wed, Nov 13, 2019 at 2:18 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Wed, Nov 13, 2019 at 8:39 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
> >
> >   Hi,
> >
> > > > > >> VRAM helpers use drm_gem_ttm_mmap(), which wraps ttm_bo_mmap_obj().
> > > > > >> These changes should be transparent.
> > > > > >
> > > > > > There's still the issue that for dma-buf mmap vs drm mmap you use
> > > > > > different f_mapping, which means ttm's pte shootdown won't work correctly
> > > > > > for dma-buf mmaps. But yeah normal operation for ttm/vram helpers should
> > > > > > be fine.
> > > > >
> > > > > VRAM helpers don't support dmabufs.
> > > >
> > > > It's not that simple.  Even when not supporting dma-buf export and
> > > > import it is still possible to create dma-bufs, import them into the
> > > > same driver (which doesn't actually export+import but just grabs a gem
> > > > object reference instead) and also to mmap them via prime/dma-buf code
> > > > path ...
> >
> > ... but after looking again I think we are all green here.  Given that
> > only self-import works we'll only see vram gem objects in the mmap code
> > path, which should have everything set up correctly.  Same goes for qxl.
> >
> > All other ttm drivers still use the old mmap code path, so all green
> > there too I think.  Also I somehow doubt dma-buf mmap vs. drm mmap ends
> > up using different f_mapping, ttm code has a WARN_ON in ttm_bo_vm_open()
> > which would fire should that be the case.
> >
> > Do imported dma-bufs hit the drm mmap code path in the first place?
> > Wouldn't mmap be handled by the exporting driver?
>
> drm_gem_dmabuf_mmap -> obj->funcs->mmap -> ttm_bo_mmap_obj
>
> And I'm not seeing anyone adjusting vm_file->f_mapping anywhere here at all.
>
> Note to hit this you need userspace to
> - handle2fd on a buffer to create a dma-buf fd
> - call mmap directly on that dma-buf fd

That was exactly the vgem IGT test that prompted $subject fix. (With
vgem modified to use shmem helpers)

Rob

On Wed, Nov 13, 2019 at 02:51:45PM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> > > ... but after looking again I think we are all green here.  Given that
> > > only self-import works we'll only see vram gem objects in the mmap code
> > > path, which should have everything set up correctly.  Same goes for qxl.
> > >
> > > All other ttm drivers still use the old mmap code path, so all green
> > > there too I think.  Also I somehow doubt dma-buf mmap vs. drm mmap ends
> > > up using different f_mapping, ttm code has a WARN_ON in ttm_bo_vm_open()
> > > which would fire should that be the case.
> > >
> > > Do imported dma-bufs hit the drm mmap code path in the first place?
> > > Wouldn't mmap be handled by the exporting driver?
> > 
> > drm_gem_dmabuf_mmap -> obj->funcs->mmap -> ttm_bo_mmap_obj
> > 
> > And I'm not seeing anyone adjusting vm_file->f_mapping anywhere here at all.
> 
> [ some more code browsing ]
> 
> Ok, I see.  dma-bufs get their own file, their own anon inode and
> thereby their own address space.  So that it used when mmaping the
> dma-buf.
> 
> drm filehandle's get the shared address space instead, drm_open() sets
> it.
> 
> So, yes, I see the problem.  It's not new though, as far I can see the
> old dma-buf mmap code path doesn't adjust f_mapping anywhere either ...
> 
> > Note to hit this you need userspace to
> > - handle2fd on a buffer to create a dma-buf fd
> > - call mmap directly on that dma-buf fd
> 
> Hmm, seems for handle2fd I need a dummy gem_prime_get_sg_table function
> wired up even when not actually exporting/importing anything.  So I
> think neither qxl nor any of the vram drivers allow to trigger that (and
> no other ttm driver uses the new ttm mmap code yet).
> 
> So, $subject patch should not make things worse in ttm land.
> 
> When hacking the bochs driver to have export callbacks (without
> supporting actual exports) handle2fd + mmap() callback works fine.
> Didn't verify yet I actually get the correct pages mapped.  But maybe
> mmap() isn't the problem when the correct address space is important for
> unmap only.
> 
> Is there a good test case?

You need memory pressure, to force ttm to unmap the bo, not userspace. So
roughly
1. create bo
2. mmap it through drm fd, write some stuff
3. export to dma-buf, mmap it, verify stuff is there
4. create a pile more bo, mmap them write to them
5. once you've thrashed all of vram enough, recheck your original bo. If
I'm right you should get the following:
   - drm fd mmap still show right content
   - dma-buf fd mmap shows random crap that you've written into other
     buffers

Ofc you need to make sure that an mmap actually forces the buffer into
vram. So might need a combo of modeset+mmap, to make that happen. Plain
mmap might just give you ptes that point into system memory, which is not
managed by ttm like vram.

munmap called by userspace isn't a problem, since that starts from the
right struct mm_struct. It's when you start with the object (i.e. in the
driver), and need to figure out where all the various vma that mmap it are
where this matters.
-Daniel

On Wed, Nov 13, 2019 at 07:53:56AM -0600, Rob Herring wrote:
> On Wed, Nov 13, 2019 at 2:18 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Wed, Nov 13, 2019 at 8:39 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
> > >
> > >   Hi,
> > >
> > > > > > >> VRAM helpers use drm_gem_ttm_mmap(), which wraps ttm_bo_mmap_obj().
> > > > > > >> These changes should be transparent.
> > > > > > >
> > > > > > > There's still the issue that for dma-buf mmap vs drm mmap you use
> > > > > > > different f_mapping, which means ttm's pte shootdown won't work correctly
> > > > > > > for dma-buf mmaps. But yeah normal operation for ttm/vram helpers should
> > > > > > > be fine.
> > > > > >
> > > > > > VRAM helpers don't support dmabufs.
> > > > >
> > > > > It's not that simple.  Even when not supporting dma-buf export and
> > > > > import it is still possible to create dma-bufs, import them into the
> > > > > same driver (which doesn't actually export+import but just grabs a gem
> > > > > object reference instead) and also to mmap them via prime/dma-buf code
> > > > > path ...
> > >
> > > ... but after looking again I think we are all green here.  Given that
> > > only self-import works we'll only see vram gem objects in the mmap code
> > > path, which should have everything set up correctly.  Same goes for qxl.
> > >
> > > All other ttm drivers still use the old mmap code path, so all green
> > > there too I think.  Also I somehow doubt dma-buf mmap vs. drm mmap ends
> > > up using different f_mapping, ttm code has a WARN_ON in ttm_bo_vm_open()
> > > which would fire should that be the case.
> > >
> > > Do imported dma-bufs hit the drm mmap code path in the first place?
> > > Wouldn't mmap be handled by the exporting driver?
> >
> > drm_gem_dmabuf_mmap -> obj->funcs->mmap -> ttm_bo_mmap_obj
> >
> > And I'm not seeing anyone adjusting vm_file->f_mapping anywhere here at all.
> >
> > Note to hit this you need userspace to
> > - handle2fd on a buffer to create a dma-buf fd
> > - call mmap directly on that dma-buf fd
> 
> That was exactly the vgem IGT test that prompted $subject fix. (With
> vgem modified to use shmem helpers)

See my other mail, this only hits the right mmap paths. For the unmap side
you need even more (and that part is driver dependent, and vgem would need
some debug tricks to expose that for testing).
-Daniel

> You need memory pressure, to force ttm to unmap the bo, not userspace. So
> roughly
> 1. create bo
> 2. mmap it through drm fd, write some stuff
> 3. export to dma-buf, mmap it, verify stuff is there
> 4. create a pile more bo, mmap them write to them
> 5. once you've thrashed all of vram enough, recheck your original bo. If
> I'm right you should get the following:
>    - drm fd mmap still show right content
>    - dma-buf fd mmap shows random crap that you've written into other
>      buffers
> 
> Ofc you need to make sure that an mmap actually forces the buffer into
> vram. So might need a combo of modeset+mmap, to make that happen. Plain
> mmap might just give you ptes that point into system memory, which is not
> managed by ttm like vram.

Is any move buffer good enough to trigger this, i.e. will SYSTEM -> VRAM
work too?  That'll be easier because all I need to do is map the buffer
to a crtc to force pinning to vram, then check if the mappings are
intact still ...

cheers,
  Gerd

On Fri, Nov 15, 2019 at 10:37 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> > You need memory pressure, to force ttm to unmap the bo, not userspace. So
> > roughly
> > 1. create bo
> > 2. mmap it through drm fd, write some stuff
> > 3. export to dma-buf, mmap it, verify stuff is there
> > 4. create a pile more bo, mmap them write to them
> > 5. once you've thrashed all of vram enough, recheck your original bo. If
> > I'm right you should get the following:
> >    - drm fd mmap still show right content
> >    - dma-buf fd mmap shows random crap that you've written into other
> >      buffers
> >
> > Ofc you need to make sure that an mmap actually forces the buffer into
> > vram. So might need a combo of modeset+mmap, to make that happen. Plain
> > mmap might just give you ptes that point into system memory, which is not
> > managed by ttm like vram.
>
> Is any move buffer good enough to trigger this, i.e. will SYSTEM -> VRAM
> work too?  That'll be easier because all I need to do is map the buffer
> to a crtc to force pinning to vram, then check if the mappings are
> intact still ...

I think that should work too. Just make sure you actually write
through SYSTEM first (maybe with some printk or whatever).
-Daniel

On Fri, Nov 15, 2019 at 11:18:28AM +0100, Daniel Vetter wrote:
> On Fri, Nov 15, 2019 at 10:37 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
> >
> > > You need memory pressure, to force ttm to unmap the bo, not userspace. So
> > > roughly
> > > 1. create bo
> > > 2. mmap it through drm fd, write some stuff
> > > 3. export to dma-buf, mmap it, verify stuff is there
> > > 4. create a pile more bo, mmap them write to them
> > > 5. once you've thrashed all of vram enough, recheck your original bo. If
> > > I'm right you should get the following:
> > >    - drm fd mmap still show right content
> > >    - dma-buf fd mmap shows random crap that you've written into other
> > >      buffers
> > >
> > > Ofc you need to make sure that an mmap actually forces the buffer into
> > > vram. So might need a combo of modeset+mmap, to make that happen. Plain
> > > mmap might just give you ptes that point into system memory, which is not
> > > managed by ttm like vram.
> >
> > Is any move buffer good enough to trigger this, i.e. will SYSTEM -> VRAM
> > work too?  That'll be easier because all I need to do is map the buffer
> > to a crtc to force pinning to vram, then check if the mappings are
> > intact still ...
> 
> I think that should work too.

Seems to work ok for ttm/vram.

Test code:
  https://git.kraxel.org/cgit/drminfo/commit/?id=a9eb96504dc17376e07b5c6edf5296b41a48bbfe

> Just make sure you actually write
> through SYSTEM first (maybe with some printk or whatever).

That works fine.  Sprinkled ...

	system("cat /sys/kernel/debug/dri/0/vram-mm"); 

... into the test code, and drmModeSetCrtc() indeed makes the gem object show
up there.

cheers,
  Gerd

On Fri, Nov 15, 2019 at 11:56 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> On Fri, Nov 15, 2019 at 11:18:28AM +0100, Daniel Vetter wrote:
> > On Fri, Nov 15, 2019 at 10:37 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
> > >
> > > > You need memory pressure, to force ttm to unmap the bo, not userspace. So
> > > > roughly
> > > > 1. create bo
> > > > 2. mmap it through drm fd, write some stuff
> > > > 3. export to dma-buf, mmap it, verify stuff is there
> > > > 4. create a pile more bo, mmap them write to them
> > > > 5. once you've thrashed all of vram enough, recheck your original bo. If
> > > > I'm right you should get the following:
> > > >    - drm fd mmap still show right content
> > > >    - dma-buf fd mmap shows random crap that you've written into other
> > > >      buffers
> > > >
> > > > Ofc you need to make sure that an mmap actually forces the buffer into
> > > > vram. So might need a combo of modeset+mmap, to make that happen. Plain
> > > > mmap might just give you ptes that point into system memory, which is not
> > > > managed by ttm like vram.
> > >
> > > Is any move buffer good enough to trigger this, i.e. will SYSTEM -> VRAM
> > > work too?  That'll be easier because all I need to do is map the buffer
> > > to a crtc to force pinning to vram, then check if the mappings are
> > > intact still ...
> >
> > I think that should work too.
>
> Seems to work ok for ttm/vram.
>
> Test code:
>   https://git.kraxel.org/cgit/drminfo/commit/?id=a9eb96504dc17376e07b5c6edf5296b41a48bbfe

I think that's not nasty enough. If the mappings aren't updated, then
you'll still see the the same old pages with the right contents. You
need to change them somehow after they migrated (with vram eviction
that happens automatically since there'll b another object in the same
spot, for system memory I think you need the shrinker to kick in and
free the pages first). Easiest probably to wait for a key press so you
can appreciate the color, then write a different color (full screen)
when the buffer is in vram.

> > Just make sure you actually write
> > through SYSTEM first (maybe with some printk or whatever).
>
> That works fine.  Sprinkled ...
>
>         system("cat /sys/kernel/debug/dri/0/vram-mm");
>
> ... into the test code, and drmModeSetCrtc() indeed makes the gem object show
> up there.

You'd need to check the ptes themselves. Which I think not even proc
shows by default (only the file that's supposed to be mapped). But
good to confirm that the buffer moved at least.
-Daniel

Hi,

> > > > Is any move buffer good enough to trigger this, i.e. will SYSTEM -> VRAM
> > > > work too?  That'll be easier because all I need to do is map the buffer
> > > > to a crtc to force pinning to vram, then check if the mappings are
> > > > intact still ...
> > >
> > > I think that should work too.
> >
> > Seems to work ok for ttm/vram.
> >
> > Test code:
> >   https://git.kraxel.org/cgit/drminfo/commit/?id=a9eb96504dc17376e07b5c6edf5296b41a48bbfe
> 
> I think that's not nasty enough. If the mappings aren't updated, then
> you'll still see the the same old pages with the right contents. You
> need to change them somehow after they migrated (with vram eviction
> that happens automatically since there'll b another object in the same
> spot, for system memory I think you need the shrinker to kick in and
> free the pages first). Easiest probably to wait for a key press so you
> can appreciate the color, then write a different color (full screen)
> when the buffer is in vram.

update-object-after-move works fine.

try zap mappings with madvise(dontneed) has no bad effects (after vram
move, trying to force re-creating the ptes).

didn't try the memory pressure thing yet.

> You'd need to check the ptes themselves. Which I think not even proc
> shows by default (only the file that's supposed to be mapped). But
> good to confirm that the buffer moved at least.

One reproducable glitch found:  fork() while having a dma-buf mapped
triggers the WARN_ON in ttm_bo_vm_open().  Both old & new mmap code
paths, so this isn't something new coming from the
drm_gem_object_funcs.mmap switch.  Instead it is an old issue coming
from the address space handling being different in drm mmap and dmabuf
mmap code paths.

I can see now why you want a private address space for each object.
Does that imply we need an anon_inode for each gem object?  Or is
there some more lightweight way to do this?

cheers,
  Gerd

On Mon, Nov 18, 2019 at 11:40:26AM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> > > > > Is any move buffer good enough to trigger this, i.e. will SYSTEM -> VRAM
> > > > > work too?  That'll be easier because all I need to do is map the buffer
> > > > > to a crtc to force pinning to vram, then check if the mappings are
> > > > > intact still ...
> > > >
> > > > I think that should work too.
> > >
> > > Seems to work ok for ttm/vram.
> > >
> > > Test code:
> > >   https://git.kraxel.org/cgit/drminfo/commit/?id=a9eb96504dc17376e07b5c6edf5296b41a48bbfe
> > 
> > I think that's not nasty enough. If the mappings aren't updated, then
> > you'll still see the the same old pages with the right contents. You
> > need to change them somehow after they migrated (with vram eviction
> > that happens automatically since there'll b another object in the same
> > spot, for system memory I think you need the shrinker to kick in and
> > free the pages first). Easiest probably to wait for a key press so you
> > can appreciate the color, then write a different color (full screen)
> > when the buffer is in vram.
> 
> update-object-after-move works fine.
> 
> try zap mappings with madvise(dontneed) has no bad effects (after vram
> move, trying to force re-creating the ptes).

Well if it's broken the zapping wouldn't work :-)

> didn't try the memory pressure thing yet.

I'm surprised ... and I have no idea how/why it keeps working.

For my paranoia, can you instrument the ttm page fault handler, and double
check that we get new faults after the move, and set up new ptes for the
same old mapping, but now pointing at the new place in vram?

> > You'd need to check the ptes themselves. Which I think not even proc
> > shows by default (only the file that's supposed to be mapped). But
> > good to confirm that the buffer moved at least.
> 
> One reproducable glitch found:  fork() while having a dma-buf mapped
> triggers the WARN_ON in ttm_bo_vm_open().  Both old & new mmap code
> paths, so this isn't something new coming from the
> drm_gem_object_funcs.mmap switch.  Instead it is an old issue coming
> from the address space handling being different in drm mmap and dmabuf
> mmap code paths.
> 
> I can see now why you want a private address space for each object.
> Does that imply we need an anon_inode for each gem object?  Or is
> there some more lightweight way to do this?

I have no idea whether we can get a address_space struct without an inode
(and no disasters).
-Daniel

Hi,

> > I can see now why you want a private address space for each object.
> > Does that imply we need an anon_inode for each gem object?  Or is
> > there some more lightweight way to do this?
> 
> I have no idea whether we can get a address_space struct without an inode
> (and no disasters).

Anything building on shmem helpers should be able to use
obj->filp->f_mapping, right?  So allocating an inode unconditionally
doesn't look like a good plan.

Guess I'll go look at ttm-local changes for starters and see how it
goes.

cheers,
  Gerd

On Wed, Nov 20, 2019 at 09:05:32AM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> > > I can see now why you want a private address space for each object.
> > > Does that imply we need an anon_inode for each gem object?  Or is
> > > there some more lightweight way to do this?
> > 
> > I have no idea whether we can get a address_space struct without an inode
> > (and no disasters).
> 
> Anything building on shmem helpers should be able to use
> obj->filp->f_mapping, right?  So allocating an inode unconditionally
> doesn't look like a good plan.

Not sure the shmem helpers forward the mmap to the underlying shmem file,
so not sure this is the greatest way either.

> Guess I'll go look at ttm-local changes for starters and see how it
> goes.

I think for ttm just consistently using the one per-device mapping for
everything, with all the fake offsets, is probably the quickest route.
Maybe also for for shmem helpers.

I'm kinda leaning towards that we just restore the fake offset everywhere,
and document how it works. Until someone comes up with a really bright
idea.
-Daniel

Hi,

> > Anything building on shmem helpers should be able to use
> > obj->filp->f_mapping, right?  So allocating an inode unconditionally
> > doesn't look like a good plan.
> 
> Not sure the shmem helpers forward the mmap to the underlying shmem file,
> so not sure this is the greatest way either.

I think so, shmem helpers already zap the fake offset, and this would
not work without per-object address space I think.

> > Guess I'll go look at ttm-local changes for starters and see how it
> > goes.
> 
> I think for ttm just consistently using the one per-device mapping for
> everything, with all the fake offsets, is probably the quickest route.

Hmm, not clear how to fit dmabufs into this.  dmabufs already have their
own file, inode and address space.  I'm not sure you can switch that
over to the per-device mapping in the first place, and even if you can I
have my doubts this is a good idea from a security point of view ...

cheers,
  Gerd

On Wed, Nov 20, 2019 at 12:40 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>   Hi,
>
> > > Anything building on shmem helpers should be able to use
> > > obj->filp->f_mapping, right?  So allocating an inode unconditionally
> > > doesn't look like a good plan.
> >
> > Not sure the shmem helpers forward the mmap to the underlying shmem file,
> > so not sure this is the greatest way either.
>
> I think so, shmem helpers already zap the fake offset, and this would
> not work without per-object address space I think.

That's why I think shmem helpers have a problem right now, and why
it's probably best to go back to the fake offset for everything. But
we seem to have lost Rob Herring in all this thread, so readding him.

> > > Guess I'll go look at ttm-local changes for starters and see how it
> > > goes.
> >
> > I think for ttm just consistently using the one per-device mapping for
> > everything, with all the fake offsets, is probably the quickest route.
>
> Hmm, not clear how to fit dmabufs into this.  dmabufs already have their
> own file, inode and address space.  I'm not sure you can switch that
> over to the per-device mapping in the first place, and even if you can I
> have my doubts this is a good idea from a security point of view ...

You can (plenty drivers do that already), and not sure how that breaks
security? Imo it's more consistent, since everything ends up pointing
to the same underlying resource.
-Daniel

Hi,

> > > I think for ttm just consistently using the one per-device mapping for
> > > everything, with all the fake offsets, is probably the quickest route.
> >
> > Hmm, not clear how to fit dmabufs into this.  dmabufs already have their
> > own file, inode and address space.  I'm not sure you can switch that
> > over to the per-device mapping in the first place, and even if you can I
> > have my doubts this is a good idea from a security point of view ...
> 
> You can (plenty drivers do that already),

Have pointer(s) to code?

> and not sure how that breaks
> security?

Go try mmap(fake-offset) on the dma-buf file handle to access other
buffers of the drm device?  Hmm, thinking again, I guess the
verify-access restrictions should prevent that.

cheers,
  Gerd

On Wed, Nov 20, 2019 at 1:18 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>   Hi,
>
> > > > I think for ttm just consistently using the one per-device mapping for
> > > > everything, with all the fake offsets, is probably the quickest route.
> > >
> > > Hmm, not clear how to fit dmabufs into this.  dmabufs already have their
> > > own file, inode and address space.  I'm not sure you can switch that
> > > over to the per-device mapping in the first place, and even if you can I
> > > have my doubts this is a good idea from a security point of view ...
> >
> > You can (plenty drivers do that already),
>
> Have pointer(s) to code?

dma_buf_mmap() does the same trick, but the other way round: It
converts from gem mapping and fake offset to dma-buf mapping and 0
offset. I think we'd simply need the inverse.

> > and not sure how that breaks
> > security?
>
> Go try mmap(fake-offset) on the dma-buf file handle to access other
> buffers of the drm device?  Hmm, thinking again, I guess the
> verify-access restrictions should prevent that.

Ah, we're not going to replace the mapping on the dma-buf file. Only
the file of the vma structure. Doing the former would indeed be pretty
bad from a security pov. So don't do what amdgpu_gem_prime_export
does, that's the bad stuff :-)
-Daniel

Hi,

> Ah, we're not going to replace the mapping on the dma-buf file. Only
> the file of the vma structure. Doing the former would indeed be pretty
> bad from a security pov.

Now where do I get a filp from?  Can I just call drm_open?

cheers,
  Gerd

On Wed, Nov 20, 2019 at 2:08 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
> > Ah, we're not going to replace the mapping on the dma-buf file. Only
> > the file of the vma structure. Doing the former would indeed be pretty
> > bad from a security pov.
>
> Now where do I get a filp from?  Can I just call drm_open?

Hm, now I wonder whether it's maybe ok to just exchange the
filp->f_mapping. As long as we don't mix up the kinds of mapping and
page-cache management that can happon on a given address_space
structure (that's why I'm not keeon the shmem mapping reused, since
shmem uses the same address_space structure internally to manage the
page allocations - address_space both contains the page cache for a
file, and also the reverse mapping information). So kinda what
drm_open does, except we do that to the dma-buf file. So exactly what
amdgpu is doing and that I just complained about :-)

Aside: the amdgpu isn't great because it's racy, userspace could have
guessed the fd and already started an mmap before we managed to update
stuff. But aside from that maybe rolling out the amdgpu trick for
everyone is the right way?
-Daniel

Hi,

> > update-object-after-move works fine.
> > 
> > try zap mappings with madvise(dontneed) has no bad effects (after vram
> > move, trying to force re-creating the ptes).
> 
> Well if it's broken the zapping wouldn't work :-)
> 
> > didn't try the memory pressure thing yet.
> 
> I'm surprised ... and I have no idea how/why it keeps working.
> 
> For my paranoia, can you instrument the ttm page fault handler, and double
> check that we get new faults after the move, and set up new ptes for the
> same old mapping, but now pointing at the new place in vram?

Hmm, only the drm device mapping is faulted in after moving it,
the dma-buf mapping is not.  Fixed by:

-------------------------- cut here ------------------------
From 3a7f6c6dbf3b1e4b412c2b283b2ea4edff9f33b5 Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Thu, 21 Nov 2019 08:39:17 +0100
Subject: [PATCH] drm: share address space for dma bufs

---
 drivers/gpu/drm/drm_prime.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 0814211b0f3f..07c88d2aedee 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -240,6 +240,7 @@ void drm_prime_destroy_file_private(struct drm_prime_file_private *prime_fpriv)
 struct dma_buf *drm_gem_dmabuf_export(struct drm_device *dev,
 				      struct dma_buf_export_info *exp_info)
 {
+	struct drm_gem_object *obj = exp_info->priv;
 	struct dma_buf *dma_buf;
 
 	dma_buf = dma_buf_export(exp_info);
@@ -247,7 +248,8 @@ struct dma_buf *drm_gem_dmabuf_export(struct drm_device *dev,
 		return dma_buf;
 
 	drm_dev_get(dev);
-	drm_gem_object_get(exp_info->priv);
+	drm_gem_object_get(obj);
+	dma_buf->file->f_mapping = obj->dev->anon_inode->i_mapping;
 
 	return dma_buf;
 }

Hi,

> Aside: the amdgpu isn't great because it's racy, userspace could have
> guessed the fd and already started an mmap before we managed to update
> stuff.

Can't see that race.  When I read the code correctly the fd is created
and installed (using dma_buf_fd) only after dmabuf setup is finished.

cheers,
  Gerd

On Thu, Nov 21, 2019 at 09:10:21AM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> > Aside: the amdgpu isn't great because it's racy, userspace could have
> > guessed the fd and already started an mmap before we managed to update
> > stuff.
> 
> Can't see that race.  When I read the code correctly the fd is created
> and installed (using dma_buf_fd) only after dmabuf setup is finished.

Right, I mixed things up. Looks all good.
-Daniel

On Thu, Nov 21, 2019 at 09:02:59AM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> > > update-object-after-move works fine.
> > > 
> > > try zap mappings with madvise(dontneed) has no bad effects (after vram
> > > move, trying to force re-creating the ptes).
> > 
> > Well if it's broken the zapping wouldn't work :-)
> > 
> > > didn't try the memory pressure thing yet.
> > 
> > I'm surprised ... and I have no idea how/why it keeps working.
> > 
> > For my paranoia, can you instrument the ttm page fault handler, and double
> > check that we get new faults after the move, and set up new ptes for the
> > same old mapping, but now pointing at the new place in vram?
> 
> Hmm, only the drm device mapping is faulted in after moving it,
> the dma-buf mapping is not.  Fixed by:

Ah yes, that's more what I'd expect to happen, and the below is what I'd
expect to fix things up. I think we should move it up ahead of the device
callback (so that drivers can overwrite) and then push as a fix. Separate
from a possible patch to undo the fake offset removal.
-Daniel

> 
> -------------------------- cut here ------------------------
> From 3a7f6c6dbf3b1e4b412c2b283b2ea4edff9f33b5 Mon Sep 17 00:00:00 2001
> From: Gerd Hoffmann <kraxel@redhat.com>
> Date: Thu, 21 Nov 2019 08:39:17 +0100
> Subject: [PATCH] drm: share address space for dma bufs
> 
> ---
>  drivers/gpu/drm/drm_prime.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index 0814211b0f3f..07c88d2aedee 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -240,6 +240,7 @@ void drm_prime_destroy_file_private(struct drm_prime_file_private *prime_fpriv)
>  struct dma_buf *drm_gem_dmabuf_export(struct drm_device *dev,
>  				      struct dma_buf_export_info *exp_info)
>  {
> +	struct drm_gem_object *obj = exp_info->priv;
>  	struct dma_buf *dma_buf;
>  
>  	dma_buf = dma_buf_export(exp_info);
> @@ -247,7 +248,8 @@ struct dma_buf *drm_gem_dmabuf_export(struct drm_device *dev,
>  		return dma_buf;
>  
>  	drm_dev_get(dev);
> -	drm_gem_object_get(exp_info->priv);
> +	drm_gem_object_get(obj);
> +	dma_buf->file->f_mapping = obj->dev->anon_inode->i_mapping;
>  
>  	return dma_buf;
>  }
> -- 
> 2.18.1
> 
> -------------------------- cut here ------------------------
> 
> git branch: https://git.kraxel.org/cgit/linux/log/?h=drm-mmap-debug
> 
> cheers,
>   Gerd
>

On Thu, Nov 21, 2019 at 09:49:53AM +0100, Daniel Vetter wrote:
> On Thu, Nov 21, 2019 at 09:02:59AM +0100, Gerd Hoffmann wrote:
> >   Hi,
> > 
> > > > update-object-after-move works fine.
> > > > 
> > > > try zap mappings with madvise(dontneed) has no bad effects (after vram
> > > > move, trying to force re-creating the ptes).
> > > 
> > > Well if it's broken the zapping wouldn't work :-)
> > > 
> > > > didn't try the memory pressure thing yet.
> > > 
> > > I'm surprised ... and I have no idea how/why it keeps working.
> > > 
> > > For my paranoia, can you instrument the ttm page fault handler, and double
> > > check that we get new faults after the move, and set up new ptes for the
> > > same old mapping, but now pointing at the new place in vram?
> > 
> > Hmm, only the drm device mapping is faulted in after moving it,
> > the dma-buf mapping is not.  Fixed by:
> 
> Ah yes, that's more what I'd expect to happen, and the below is what I'd
> expect to fix things up. I think we should move it up ahead of the device
> callback (so that drivers can overwrite) and then push as a fix. Separate
> from a possible patch to undo the fake offset removal.
> -Daniel

Is there a more elegant way than copying the intel list on non-intel
patches to kick intel ci?

cheers,
  Gerd

On Thu, Nov 21, 2019 at 11:18 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> On Thu, Nov 21, 2019 at 09:49:53AM +0100, Daniel Vetter wrote:
> > On Thu, Nov 21, 2019 at 09:02:59AM +0100, Gerd Hoffmann wrote:
> > >   Hi,
> > >
> > > > > update-object-after-move works fine.
> > > > >
> > > > > try zap mappings with madvise(dontneed) has no bad effects (after vram
> > > > > move, trying to force re-creating the ptes).
> > > >
> > > > Well if it's broken the zapping wouldn't work :-)
> > > >
> > > > > didn't try the memory pressure thing yet.
> > > >
> > > > I'm surprised ... and I have no idea how/why it keeps working.
> > > >
> > > > For my paranoia, can you instrument the ttm page fault handler, and double
> > > > check that we get new faults after the move, and set up new ptes for the
> > > > same old mapping, but now pointing at the new place in vram?
> > >
> > > Hmm, only the drm device mapping is faulted in after moving it,
> > > the dma-buf mapping is not.  Fixed by:
> >
> > Ah yes, that's more what I'd expect to happen, and the below is what I'd
> > expect to fix things up. I think we should move it up ahead of the device
> > callback (so that drivers can overwrite) and then push as a fix. Separate
> > from a possible patch to undo the fake offset removal.
> > -Daniel
>
> Is there a more elegant way than copying the intel list on non-intel
> patches to kick intel ci?

Nope, not really.
-Daniel