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

From: Daniel Vetter <daniel@ffwll.ch>
To: Gerd Hoffmann <kraxel@redhat.com>
Cc: 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: Mon, 18 Nov 2019 17:49:27 +0100	[thread overview]
Message-ID: <20191118164927.GF23790@phenom.ffwll.local> (raw)
In-Reply-To: <20191118104026.zg5pt5yg2wkulmrr@sirius.home.kraxel.org>

On Mon, Nov 18, 2019 at 11:40:26AM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> > > > > Is any move buffer good enough to trigger this, i.e. will SYSTEM -> VRAM
> > > > > work too?  That'll be easier because all I need to do is map the buffer
> > > > > to a crtc to force pinning to vram, then check if the mappings are
> > > > > intact still ...
> > > >
> > > > I think that should work too.
> > >
> > > Seems to work ok for ttm/vram.
> > >
> > > Test code:
> > >   https://git.kraxel.org/cgit/drminfo/commit/?id=a9eb96504dc17376e07b5c6edf5296b41a48bbfe
> > 
> > I think that's not nasty enough. If the mappings aren't updated, then
> > you'll still see the the same old pages with the right contents. You
> > need to change them somehow after they migrated (with vram eviction
> > that happens automatically since there'll b another object in the same
> > spot, for system memory I think you need the shrinker to kick in and
> > free the pages first). Easiest probably to wait for a key press so you
> > can appreciate the color, then write a different color (full screen)
> > when the buffer is in vram.
> 
> update-object-after-move works fine.
> 
> try zap mappings with madvise(dontneed) has no bad effects (after vram
> move, trying to force re-creating the ptes).

Well if it's broken the zapping wouldn't work :-)

> didn't try the memory pressure thing yet.

I'm surprised ... and I have no idea how/why it keeps working.

For my paranoia, can you instrument the ttm page fault handler, and double
check that we get new faults after the move, and set up new ptes for the
same old mapping, but now pointing at the new place in vram?

> > You'd need to check the ptes themselves. Which I think not even proc
> > shows by default (only the file that's supposed to be mapped). But
> > good to confirm that the buffer moved at least.
> 
> One reproducable glitch found:  fork() while having a dma-buf mapped
> triggers the WARN_ON in ttm_bo_vm_open().  Both old & new mmap code
> paths, so this isn't something new coming from the
> drm_gem_object_funcs.mmap switch.  Instead it is an old issue coming
> from the address space handling being different in drm mmap and dmabuf
> mmap code paths.
> 
> I can see now why you want a private address space for each object.
> Does that imply we need an anon_inode for each gem object?  Or is
> there some more lightweight way to do this?

I have no idea whether we can get a address_space struct without an inode
(and no disasters).
-Daniel
-- 
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