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

[Lucas Stach]

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.

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

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

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.

Regards,
Lucas