From mboxrd@z Thu Jan 1 00:00:00 1970 From: Aaron Plattner Subject: Re: [PATCH 1/4] drm: add prime helpers Date: Fri, 7 Dec 2012 14:07:03 -0800 Message-ID: <50C26887.7080802@nvidia.com> References: <1354817271-5121-1-git-send-email-aplattner@nvidia.com> <1354817271-5121-2-git-send-email-aplattner@nvidia.com> <20121206214630.GV11556@phenom.ffwll.local> <50C22E4E.1060400@nvidia.com> <20121207184833.GY11556@phenom.ffwll.local> <50C252A1.7040209@nvidia.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from hqemgate03.nvidia.com (hqemgate03.nvidia.com [216.228.121.140]) by gabe.freedesktop.org (Postfix) with ESMTP id 47ED8E5D54 for ; Fri, 7 Dec 2012 14:07:08 -0800 (PST) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: Daniel Vetter Cc: "dri-devel@lists.freedesktop.org" List-Id: dri-devel@lists.freedesktop.org On 12/07/2012 01:38 PM, Daniel Vetter wrote: > On Fri, Dec 7, 2012 at 9:33 PM, Aaron Plattner 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