From mboxrd@z Thu Jan 1 00:00:00 1970 From: Maarten Lankhorst Subject: Re: [PATCH 3/6] drm/ttm: call ttm_bo_cleanup_refs with reservation and lru lock held Date: Wed, 28 Nov 2012 13:28:04 +0100 Message-ID: <50B60354.5040206@canonical.com> References: <1354101944-10455-1-git-send-email-maarten.lankhorst@canonical.com> <1354101944-10455-3-git-send-email-maarten.lankhorst@canonical.com> <50B5FB82.9020904@vmware.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from youngberry.canonical.com (youngberry.canonical.com [91.189.89.112]) by gabe.freedesktop.org (Postfix) with ESMTP id 3792EE5BFA for ; Wed, 28 Nov 2012 04:28:06 -0800 (PST) In-Reply-To: <50B5FB82.9020904@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 28-11-12 12:54, Thomas Hellstrom schreef: > >> Signed-off-by: Maarten Lankhorst >> --- >> drivers/gpu/drm/ttm/ttm_bo.c | 112 +++++++++++++++++++++++-------------------- >> 1 file changed, 60 insertions(+), 52 deletions(-) >> >> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c >> index 202fc20..02b275b 100644 >> --- a/drivers/gpu/drm/ttm/ttm_bo.c >> +++ b/drivers/gpu/drm/ttm/ttm_bo.c >> @@ -486,14 +486,6 @@ static void ttm_bo_cleanup_memtype_use(struct ttm_buffer_object *bo) >> bo->ttm = NULL; >> } >> ttm_bo_mem_put(bo, &bo->mem); >> - >> - atomic_set(&bo->reserved, 0); >> - >> - /* >> - * Make processes trying to reserve really pick it up. >> - */ >> - smp_mb__after_atomic_dec(); >> - wake_up_all(&bo->event_queue); >> } >> static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo) >> @@ -515,6 +507,9 @@ static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo) >> put_count = ttm_bo_del_from_lru(bo); >> spin_unlock(&glob->lru_lock); >> + atomic_set(&bo->reserved, 0); >> + wake_up_all(&bo->event_queue); >> + > > I think (although I'm not 100% sure) that if we use atomic_set() to unreserve, and it's not followed by a spin_unlock(), we need to insert > a memory barrier, like is done above in the removed code, otherwise memory operations protected by reserve may be reordered until after reservation. Hm yeah, looks like ttm_bo_cleanup_memtype_use probably needs a smb_mb() at the end now. The original smp_mb__after_atomic_dec was a noop, since the wake_up_all call takes a spinlock too. Thanks for catching it, I'll await the reply to my other email then maybe reword, fix this and resend. ~Maarten