On Fri, Dec 7, 2012 at 9:33 PM, Aaron Plattner <aplattner@xxxxxxxxxx> wrote: > Ah, I see what you mean. This would also need either a pointer to the drv > directly so the helpers can lock drv->struct_mutex, or a helper function to > convert from a drm_prime_helper_obj* to a gem_buffer_object* in a > driver-specific way since the helper doesn't know the layout of the driver's > bo structure. Ah, locking, and it involves dev->struct_mutex. Another pet peeve of mine ;-) Tbh I haven't looked at locking issues when stitching together my quick proposal. The problem with the dev->struct_mutex lock is that it's everywhere and hence it's way to easy to create deadlocks with it. Especially with prime, where a simple rule like "take this lock first, always" are suddenly really more complicated since gem drivers can assume both the importer and exporter role ... I haven't done a full locking review, but I expect current prime to deadlock (through driver calls) when userspace starts doing funny things. So imo it's best to move dev->struct_mutex as far away as possible from helper functions and think really hard whether we really need it. I've noticed three functions: drm_gem_map_dma_buf: We can remove the locking here imo, if drivers need dev->struct_mutex for internal consistency, they can take it in their callbacks. The dma_buf_attachment is very much like a fancy sg list + backing storage pointer. If multiple callers concurrently interact with the same attachment, havoc might ensue. Importers should add their own locking if concurrent access is possible (e.g. similar to how filling in the same sg table concurrently and then calling dma_map_sg concurrently would lead to chaos). Otoh creating/destroying attachments with attach/detach is protected by the dma_buf->lock. I've checked dma-buf docs, and that this is not specced in the docs, but was very much the intention. So I think we can solve this with a simple doc update ;-) drm_gem_dmabuf_v(un)map: Beside that drivers might need to lock for internal consistency the lock only protects the vmapping_counter and pointer and avoids that we race concurrent vmap/vunmap calls to the driver. Now vmap/vunmap is very much like an attachment, just that we don't have an explicit struct for each client since there's only one cpu address space. So pretty much every driver is forced to reinvent vmap refcounting, which feels like an interface bug. What about moving this code to the dma-buf.c interface shim and protecting it with dma_buf->lock? Atm we only take that lock for attach/detach, and drivers using vmap place the vmap call at the same spot hw drivers would place the attach call, so this shouldn't introduce any nefarious locking issues. So I think this would be the right way to move forward. And with that we've weaned the prime helpers off their dependency of dev->struct_mutex, which should make them a notch more flexible and so useful (since fighting locking inversions is a royal pain best avoided). Comments? Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
