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

From: Gerd Hoffmann <kraxel@redhat.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>,
	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 08:39:02 +0100	[thread overview]
Message-ID: <20191113073902.343vfunbjuzy725y@sirius.home.kraxel.org> (raw)
In-Reply-To: <20191112144435.GK23790@phenom.ffwll.local>

  Hi,

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

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

> > 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().
> 
> bo->base.filp is the shmem inode file, and I'm not too sure how much shmem
> approves of us misappropriating the mapping. For shmem only objects it
> probably doesn't matter (since both gem mmaps and shmem mmaps will point
> at the same underlying struct page), but for vram/ttm/anything else the
> gem mmap might point into iomem, and shmem then might go boom trying to do
> stuff with that.

Yes, agreeing here after wading through the code ...

> I think having our own mapping would be the cleanest
> long-term approach ...

You mean using per drm object (instead of per drm device) address spaces?

cheers,
  Gerd

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel