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

Daniel Vetter daniel at ffwll.ch
Wed Nov 13 08:17:47 UTC 2019


On Wed, Nov 13, 2019 at 8:39 AM Gerd Hoffmann <kraxel at redhat.com> wrote:
>
>   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?

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.

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

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

Yup. But I think that would be quite a pile of work, since we need an
anon inode for each gem bo.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch


More information about the dri-devel mailing list