From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Hellstrom Subject: Re: [PATCH 04/10] drm/ttm: change fence_lock to inner lock, v3 Date: Tue, 20 Nov 2012 16:08:06 +0100 Message-ID: <50AB9CD6.9050507@vmware.com> References: <1352728811-21860-1-git-send-email-maarten.lankhorst@canonical.com> <1352728811-21860-4-git-send-email-maarten.lankhorst@canonical.com> <50AA3F89.5020905@vmware.com> <50AA4A6D.6060703@vmware.com> <50AA5160.7070106@gmail.com> <50AB35B6.4050605@shipmail.org> <50AB6A87.4000902@canonical.com> <50AB7182.3090809@vmware.com> <50AB8208.8070004@canonical.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from smtp-outbound-2.vmware.com (smtp-outbound-2.vmware.com [208.91.2.13]) by gabe.freedesktop.org (Postfix) with ESMTP id CAE09E632D for ; Tue, 20 Nov 2012 07:08:18 -0800 (PST) In-Reply-To: <50AB8208.8070004@canonical.com> 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: Maarten Lankhorst Cc: Maarten Lankhorst , dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org On 11/20/2012 02:13 PM, Maarten Lankhorst wrote: > Op 20-11-12 13:03, Thomas Hellstrom schreef: >> On 11/20/2012 12:33 PM, Maarten Lankhorst wrote: >>> Op 20-11-12 08:48, Thomas Hellstrom schreef: >>>> On 11/19/2012 04:33 PM, Maarten Lankhorst wrote: >>>>> Op 19-11-12 16:04, Thomas Hellstrom schreef: >>>>>> On 11/19/2012 03:17 PM, Thomas Hellstrom wrote: >>>>>>> Hi, >>>>>>> >>>>>>> This patch looks mostly good, although I think ttm_bo_cleanup_refs becomes overly complicated: >>>>>>> Could this do, or am I missing something? >>>>>>> >>>>>> Actually, my version is bad, because ttm_bo_wait() is called with the lru lock held. >>>>>> >>>>>> /Thomas >>>>> Oh digging through it made me remember why I had to release the reservation early and >>>>> had to allow move_notify to be called without reservation. >>>>> >>>>> Fortunately move_notify has a NULL parameter, which is the only time that happens, >>>>> so you can still check do BUG_ON(mem != NULL && !ttm_bo_reserved(bo)); in your >>>>> move_notify handler. >>>>> >>>>> 05/10 removed the loop and assumed no new fence could be attached after the driver has >>>>> declared the bo dead. >>>>> >>>>> However, at that point it may no longer hold a reservation to confirm this, that's why >>>>> I moved the cleanup to be done in the release_list handler. It could still be done in >>>>> ttm_bo_release, but we no longer have a reservation after we waited. Getting >>>>> a reservation can fail if the bo is imported for example. >>>>> >>>>> While it would be true that in that case a new fence may be attached as well, that >>>>> would be less harmful since that operation wouldn't involve this device, so the >>>>> ttm bo can still be removed in that case. When that time comes I should probably >>>>> fix up that WARN_ON(ret) in ttm_bo_cleanup_refs. :-) >>>>> >>>>> I did add a WARN_ON(!atomic_read(&bo->kref.refcount)); to >>>>> ttm_bo_reserve and ttm_eu_reserve_buffers to be sure nothing is done on the device >>>>> itself. If that is too paranoid, those WARN_ON's could be dropped. I prefer to leave them >>>>> in for a kernel release or 2. But according to the rules that would be the only time you >>>>> could attach a new fence and trigger the WARN_ON for now.. >>>> Hmm, I'd appreciate if you could group patches with functional changes that depend on eachother togeteher, >>>> and "this is done because ...", which makes it much easier to review, (and to follow the commit history in case >>>> something goes terribly wrong and we need to revert). >>>> >>>> Meanwhile I'll take a look at the final ttm_bo.c and see if I can spot any culprits. >>>> >>>> In general, as long as a bo is on a LRU list, we must be able to attach fences because of accelerated eviction. >>> I thought it was deliberately designed in such a way that it was kept on the lru list, >>> but since it's also on the ddestroy list it won't start accelerated eviction, >>> since it branches into cleanup_refs early, and lru_lock still protects all the list entries. >> I used bad wording. I meant that unbinding might be accelerated, but currently (quite inefficiently) >> do synchronized unbinding, assuming that only the CPU can do that. When we start to support >> unsynchronized moves, we need to be able to attach fences at least at the last move_notify(bo, NULL); > Would you need to wait in that case on fence_wait being completed before calling move_notify? > > If not, you would still only need to perform one wait, but you'd have to make sure move_notify only gets > called by 1 thread before checking the fence pointer and performing a wait. At that point you still hold the > lru_lock though, so it shouldn't be too hard to make something safe. I think typically a driver that wants to implement asynchronous moves don't want to wait before calling move_notify, but may wait in move_notify or move. Typically (upcoming vmwgfx) it would invalidate the buffer in move_notify(bo, NULL), attach a fence and then use the normal delayed destroy to wait on that fence before destroying the buffer. Otherwise, since binds / unbinds are handled in the GPU command stream there's never any need to wait for moves except when there's a CPU access. /Thomas > > ~Maarten >