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

[Daniel Vetter]

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 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.
> > >>
> > >> 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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch