From mboxrd@z Thu Jan 1 00:00:00 1970 From: Maarten Lankhorst Subject: Re: [PATCH 04/10] drm/ttm: change fence_lock to inner lock, v3 Date: Mon, 19 Nov 2012 16:33:52 +0100 Message-ID: <50AA5160.7070106@gmail.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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-la0-f49.google.com (mail-la0-f49.google.com [209.85.215.49]) by gabe.freedesktop.org (Postfix) with ESMTP id 4C1A7E6117 for ; Mon, 19 Nov 2012 07:33:57 -0800 (PST) Received: by mail-la0-f49.google.com with SMTP id r15so3600738lag.36 for ; Mon, 19 Nov 2012 07:33:56 -0800 (PST) In-Reply-To: <50AA4A6D.6060703@vmware.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: Thomas Hellstrom Cc: dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org 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.. ~Maarten