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

[Gerd Hoffmann]

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().

cheers,
  Gerd