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

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

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.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch