[PATCH 2/2] drm/etnaviv: properly implement mmaping of imported buffers

Wed Jul 3 13:16:55 UTC 2019

On Wed, Jul 3, 2019 at 12:47 PM Lucas Stach <l.stach at pengutronix.de> wrote:
>
> Hi Daniel,
>
> discussion on this somehow died quite a while ago. As the bug is still
> present in etnaviv, I would like to get it going again.
>
> Am Montag, den 18.06.2018, 10:06 +0200 schrieb Daniel Vetter:
> > On Thu, May 31, 2018 at 10:41:25AM +0200, Lucas Stach wrote:
> > > Am Dienstag, den 29.05.2018, 14:34 +0200 schrieb Daniel Vetter:
> > > > On Tue, May 29, 2018 at 11:08 AM, Lucas Stach <l.stach at pengutronix.de> wrote:
> > > > > Am Dienstag, den 29.05.2018, 10:20 +0200 schrieb Daniel Vetter:
> > > > > > On Fri, May 25, 2018 at 03:42:54PM +0200, Lucas Stach wrote:
> > > > > > > The intention of the existing code was to deflect the actual work
> > > > > > > of mmaping a dma-buf to the exporter, as that one probably knows best
> > > > > > > how to handle the buffer. Unfortunately the call to drm_gem_mmap did
> > > > > > > more than what etnaviv needs in this case by actually setting up the
> > > > > > > mapping.
> > > > > > >
> > > > > > > Move mapping setup to the shm buffer type mmap implementation so we
> > > > > > > only need to look up the BO and call the buffer type mmap function
> > > > > > > from the handler.
> > > > > > >
> > > > > > > Fixes mmap behavior with dma-buf imported and userptr buffers.
> > > > > >
> > > > > > You allow mmap on userptr buffers? That sounds really nasty ...
> > > > >
> > > > > No, we don't because that's obviously crazy, even more so on ARM with
> > > > > it's special rules about aliasing mappings. The current code is buggy
> > > > > in that it first sets up the mapping and then tells userspace the mmap
> > > > > failed. After this patch we properly reject userptr mmap outright.
> > > >
> > > > Ah, I didn't realize that. It sounded more like making userptr mmap
> > > > work, not rejecting it. Can't you instead just never register the mmap
> > > > offset for userptr objects? That would catch the invalid request even
> > > > earlier, and make it more obvious that mmap is not ok for userptr.
> > > > Would also remove the need to hand-roll an etnaviv version of
> > > > drm_gem_obj_mmap.
> > >
> > > Makes sense for userptr, not so much for imported dma-bufs, see below.
> > >
> > > > > > Also not really thrilled about dma-buf mmap forwarding either, since you
> > > > > > don't seem to forward the begin/end_cpu_access stuff either.
> > > > >
> > > > > Yeah, that's still missing, but IMHO more of a correctness fix (which
> > > > > can be done in a follow on patch), distinct of the bugfix in this
> > > > > patch.
> > > >
> > > > Yeah drm_gem_obj_mmap should check for obj->import_attach imo and
> > > > scream. Maybe we should even have that check when allocating the mmap
> > > > offset, since having an mmap offset for something you can never mmap
> > > > is silly. And obj->import_attach isn't allowed to change over the
> > > > lifetime of an object.
> > >
> > > Agreed for drm_gem_obj_mmap, but I don't follow why we would reject
> > > mmap of an imported dma-buf. This seems to be a feature we want today
> > > for drivers that need to talk to multiple DRM devices, like Etnaviv,
> > > Tegra and VC4 in some cases. Making the mmap look uniform by allowing
> > > the mmap on one device and internally deflecting to the exporter is a
> > > big pro from the userspace driver writer PoV.
> > >
> > > Also if you think about stuff like ION (not that I would agree that ION
> > > is a good idea in general, but whatever), a lot of buffers end up being
> > > allocated by ION. I don't want driver writers to care where a buffer
> > > was allocated, but rather call the usual mmap on the device they are
> > > working with and do the right thing in the kernel.
> > >
> > > Maybe we can just generalize the deflecting to the exporter and
> > > implement it in the DRM core, rather than rolling our own version in
> > > etnaviv.
> >
> > The problem is more that most driver uapi for mmap doesn't bother much
> > with cache coherency ioctls (I think yours does), which makes dma-buf mmap
> > in the general case a no-go. So relying on it working seems ill-advised.
>
> Cache coherency is something that should be dealt with in the
> begin/end_cpu_access functions. I guess every GPU driver has something
> like this, as you need it to synchronize CPU access with the GPU
> anyways.

Hm, still not a fan of allowing gem mmap on imported buffers. I have
no idea why you'd want that, since I expect userspace will only render
into such buffers with the gpu. But I guess if you use that already,
*shrug*

> Currently this isn't hooked up in DRM for exported dma-bufs, but
> shouldn't be a big deal to do.

Well the drm_prime "helpers" exist so the nv blob can dma-buf export
without touching EXPORT_SYMBOL_GPL stuff :-) Just ignore it and roll
your own stuff (you can still reuse most of it if you want, it's a
true helper in that regard), you can do begin/end_cpu_access easily
that way. And no problems for you with not being gpl licensed ...

> > But in the end it's a matter of making stuff work in reality, not for the
> > full general case, and for that I guess you sufficiently control all the
> > pieces (e.g. you know you'll only ever deal with coherent buffers) to make
> > this work.
> >
> > I guess if there's demand, a general mmap reflector for gem would be much
> > better than the state of the art of everyone copypasting the same logic.
>
> I don't think a generic mmap reflector will work out, specifically
> because drivers need to hook the dma-buf begin/end_cpu_access stuff
> into their drivers specific cpu_prep/fini ioctls to make this work
> properly.

For anyone who uses coherent dma (i.e. most display-only drivers)
it'll work perfectly. And since you don't have begin/end_cpu_access
wired up, that shouldn't be worse for you? Since we've had this
discussion this generic mmap reflector actually landed. But the
reflector only works for exporting, on importing we reject gem mmap on
dma-bufs now, at least in the helpers.

> That said would you be okay with me just merging this series and going
> from there? It's definitely an improvement of the status-quo on
> etnaviv, as currently we allow to mmap dma-bufs, but then leak a
> reference.

*shrug*

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch