From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jerome Glisse Subject: Re: [PATCH] drm/ttm: remove fence_lock Date: Thu, 18 Oct 2012 13:04:32 -0400 Message-ID: <20121018170316.GA2086@gmail.com> References: <507835EF.2020806@canonical.com> <507FAF84.4060509@shipmail.org> <507FB6E7.1000403@shipmail.org> <507FBFB4.50004@canonical.com> <507FE1BB.3060104@shipmail.org> <507FEA2F.6090403@canonical.com> <507FEE3B.5090509@shipmail.org> <50801619.9020605@canonical.com> <508031BC.1010404@shipmail.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-pa0-f49.google.com (mail-pa0-f49.google.com [209.85.220.49]) by gabe.freedesktop.org (Postfix) with ESMTP id 713B59EAF2 for ; Thu, 18 Oct 2012 10:07:40 -0700 (PDT) Received: by mail-pa0-f49.google.com with SMTP id bi5so8789781pad.36 for ; Thu, 18 Oct 2012 10:07:40 -0700 (PDT) Content-Disposition: inline In-Reply-To: <508031BC.1010404@shipmail.org> 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 On Thu, Oct 18, 2012 at 06:43:40PM +0200, Thomas Hellstrom wrote: > On 10/18/2012 04:45 PM, Maarten Lankhorst wrote: > >Op 18-10-12 13:55, Thomas Hellstrom schreef: > >>On 10/18/2012 01:38 PM, Maarten Lankhorst wrote: > >>>Op 18-10-12 13:02, Thomas Hellstrom schreef: > >>>>On 10/18/2012 10:37 AM, Maarten Lankhorst wrote: > >>>>>Hey, > >>>>> > >>>>>Op 18-10-12 09:59, Thomas Hellstrom schreef: > >>>>>>On 10/18/2012 09:28 AM, Thomas Hellstrom wrote: > >>>>>>>Hi, Maarten, > >>>>>>> > >>>>>>>As you know I have been having my doubts about this change. > >>>>>>>To me it seems insane to be forced to read the fence pointer under a > >>>>>>>reserved lock, simply because when you take the reserve lock, another > >>>>>>>process may have it and there is a substantial chance that that process > >>>>>>>will also be waiting for idle while holding the reserve lock. > >>>>>I think it makes perfect sense, the only times you want to read the fence > >>>>>is when you want to change the members protected by the reservation. > >>>>No, that's not true. A typical case (or the only case) > >>>>is where you want to do a map with no_wait semantics. You will want > >>>>to be able to access a buffer's results even if the eviction code > >>>>is about to schedule an unbind from the GPU, and have the buffer > >>>>reserved? > >>>Well either block on reserve or return -EBUSY if reserved, presumably the > >>>former.. > >>> > >>>ttm_bo_vm_fault does the latter already, anyway > >>ttm_bo_vm_fault only trylocks to avoid a deadlock with mmap_sem, it's really > >>a waiting reserve but for different reasons. Typically a user-space app will keep > >>asynchronous maps to TTM during a buffer lifetime, and the buffer lifetime may > >>be long as user-space apps keep caches. > >>That's not the same as accessing a buffer after the GPU is done with it. > >Ah indeed. > >>>You don't need to hold the reservation while performing the wait itself though, > >>>you could check if ttm_bo_wait(no_wait_gpu = true) returns -EBUSY, and if so > >>>take a ref to the sync_obj member and then wait after unreserving. You won't > >>>reset sync_obj member to NULL in that case, but that should be harmless. > >>>This will allow you to keep the reservations fast and short. Maybe a helper > >>>would be appropriate for this since radeon and nouveau both seem to do this. > >>> > >>The problem is that as long as other users are waiting for idle with reservation > >>held, for example to switch GPU engine or to delete a GPU bind, waiting > >>for reserve will in many case mean wait for GPU. > >This example sounds inefficient, I know nouveau can do this, but this essentially > >just stalls the gpu entirely. I think guess you mean functions like nouveau_gem_object_close? > >It wouldn't surprise me if performance in nouveau could be improved simply by > >fixing those cases up instead, since it stalls the application completely too for other uses. > > > Please see the Nouveau cpu_prep patch as well. > > While there are a number of cases that can be fixed up, also in > Radeon, there's no way around that reservation > is a heavyweight lock that, particularly on simpler hardware without > support for fence ordering > with barriers and / or "semaphores" and accelerated eviction will be > held while waiting for idle. > > As such, it is unsuitable to protect read access to the fence > pointer. If the intention is to keep a single fence pointer > I think the best solution is to allow reading the fence pointer > outside reservation, but make sure this can be done > atomically. If the intention is to protect an array or list of fence > pointers, I think a spinlock is the better solution. > > /Thomas Just wanted to point out that like Thomas i am concern about having to have object reserved when waiting on its associated fence. I fear it will hurt us somehow. I will try to spend couple days stress testing your branch on radeon trying to see if it hurts performance anyhow with current use case. Cheers, Jerome