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

[Gerd Hoffmann]

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