On 12/07/2012 01:38 PM, Daniel Vetter wrote: > On Fri, Dec 7, 2012 at 9:33 PM, Aaron Plattner <aplattner at nvidia.com> > 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?
This all sounds good, but it's getting well outside the realm of what I'm comfortable doing in the short term as a kernel noob. Would you object to going with a version of these changes that leaves the new hooks where they are now and then applying these suggestions later? I don't think the risk of the helpers turning into a required mid-layer is very high given that i915 won't use them and it's one of the most-tested drivers. -- Aaron
