Re: [PATCH 1/4] drm: add prime helpers

[Aaron Plattner <aplattner@nvidia.com>]

On 12/07/2012 01:38 PM, Daniel Vetter wrote:
> On Fri, Dec 7, 2012 at 9:33 PM, Aaron Plattner <aplattner@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