| Message ID | 20191024191859.31700-1-robh@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/gem: Fix mmap fake offset handling for drm_gem_object_funcs.mmap | expand |
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 >
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 >> > > > >
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
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. * */ int (*mmap)(struct drm_gem_object *obj, struct vm_area_struct *vma);
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(-)