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: Wed, 13 Nov 2019 17:27:29 +0100	[thread overview]
Message-ID: <20191113162729.GQ23790@phenom.ffwll.local> (raw)
In-Reply-To: <20191113135145.itdsbojzrj5elzvu@sirius.home.kraxel.org>

On Wed, Nov 13, 2019 at 02:51:45PM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> > > ... but after looking again I think we are all green here.  Given that
> > > only self-import works we'll only see vram gem objects in the mmap code
> > > path, which should have everything set up correctly.  Same goes for qxl.
> > >
> > > All other ttm drivers still use the old mmap code path, so all green
> > > there too I think.  Also I somehow doubt dma-buf mmap vs. drm mmap ends
> > > up using different f_mapping, ttm code has a WARN_ON in ttm_bo_vm_open()
> > > which would fire should that be the case.
> > >
> > > Do imported dma-bufs hit the drm mmap code path in the first place?
> > > Wouldn't mmap be handled by the exporting driver?
> > 
> > drm_gem_dmabuf_mmap -> obj->funcs->mmap -> ttm_bo_mmap_obj
> > 
> > And I'm not seeing anyone adjusting vm_file->f_mapping anywhere here at all.
> 
> [ some more code browsing ]
> 
> Ok, I see.  dma-bufs get their own file, their own anon inode and
> thereby their own address space.  So that it used when mmaping the
> dma-buf.
> 
> drm filehandle's get the shared address space instead, drm_open() sets
> it.
> 
> So, yes, I see the problem.  It's not new though, as far I can see the
> old dma-buf mmap code path doesn't adjust f_mapping anywhere either ...
> 
> > Note to hit this you need userspace to
> > - handle2fd on a buffer to create a dma-buf fd
> > - call mmap directly on that dma-buf fd
> 
> Hmm, seems for handle2fd I need a dummy gem_prime_get_sg_table function
> wired up even when not actually exporting/importing anything.  So I
> think neither qxl nor any of the vram drivers allow to trigger that (and
> no other ttm driver uses the new ttm mmap code yet).
> 
> So, $subject patch should not make things worse in ttm land.
> 
> When hacking the bochs driver to have export callbacks (without
> supporting actual exports) handle2fd + mmap() callback works fine.
> Didn't verify yet I actually get the correct pages mapped.  But maybe
> mmap() isn't the problem when the correct address space is important for
> unmap only.
> 
> Is there a good test case?

You need memory pressure, to force ttm to unmap the bo, not userspace. So
roughly
1. create bo
2. mmap it through drm fd, write some stuff
3. export to dma-buf, mmap it, verify stuff is there
4. create a pile more bo, mmap them write to them
5. once you've thrashed all of vram enough, recheck your original bo. If
I'm right you should get the following:
   - drm fd mmap still show right content
   - dma-buf fd mmap shows random crap that you've written into other
     buffers

Ofc you need to make sure that an mmap actually forces the buffer into
vram. So might need a combo of modeset+mmap, to make that happen. Plain
mmap might just give you ptes that point into system memory, which is not
managed by ttm like vram.

munmap called by userspace isn't a problem, since that starts from the
right struct mm_struct. It's when you start with the object (i.e. in the
driver), and need to figure out where all the various vma that mmap it are
where this matters.
-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