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

[Lucas Stach]

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.

Regards,
Lucas