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@xxxxxxxx> 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@xxxxxxxxxx>
> > > > > > > Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> > > > > > > Signed-off-by: Rob Herring <robh@xxxxxxxxxx>
> > > > > > > ---
> > > > > > > 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...
>
> > > 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.
-Daniel
>
> > - ttm/vram-helpers only for dma-buf mmap redirection (because of the wrong
> > f/i_mapping).
> >
> > Rob, are you looking into this?
>
> Still trying to understand all 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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
