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

From: Rob Herring <robh@kernel.org>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>,
	Gerd Hoffmann <kraxel@redhat.com>,
	dri-devel <dri-devel@lists.freedesktop.org>,
	Thomas Zimmermann <tzimmermann@suse.de>
Subject: Re: [PATCH v2] drm/gem: Fix mmap fake offset handling for drm_gem_object_funcs.mmap
Date: Tue, 12 Nov 2019 09:37:45 -0600	[thread overview]
Message-ID: <CAL_JsqL1qghRUQu2QuVwkCszuLPzqy_1eTDCu9_tpc0euy3TcQ@mail.gmail.com> (raw)
In-Reply-To: <20191112093518.GE23790@phenom.ffwll.local>

On Tue, Nov 12, 2019 at 3:35 AM Daniel Vetter <daniel@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@redhat.com>
> > > > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > > > Signed-off-by: Rob Herring <robh@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...

> > 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?

> - 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel