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

[Rob Herring]

On Tue, Nov 12, 2019 at 1:06 PM Daniel Vetter <daniel at 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 at 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 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.
> >
> > 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