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