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

[Daniel Vetter <daniel@ffwll.ch>]

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
> 

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