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@xxxxxxxxxxxxxx> 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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
