From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ben Widawsky Subject: Re: [PATCH 05/18] drm/i915: context switch implementation Date: Fri, 30 Mar 2012 11:11:46 -0700 Message-ID: <20120330111146.02bb833b@bwidawsk.net> References: <1332103198-25852-1-git-send-email-ben@bwidawsk.net> <1332103198-25852-6-git-send-email-ben@bwidawsk.net> <20120329182411.GA27737@phenom.ffwll.local> <20120329114312.4f3f9a9a@bwidawsk.net> <20120329184900.GC27737@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from cloud01.chad-versace.us (184-106-247-128.static.cloud-ips.com [184.106.247.128]) by gabe.freedesktop.org (Postfix) with ESMTP id 8F4199EEF5 for ; Fri, 30 Mar 2012 11:11:56 -0700 (PDT) In-Reply-To: <20120329184900.GC27737@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Daniel Vetter Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Thu, 29 Mar 2012 20:49:00 +0200 Daniel Vetter wrote: > On Thu, Mar 29, 2012 at 11:43:12AM -0700, Ben Widawsky wrote: > > On Thu, 29 Mar 2012 20:24:11 +0200 > > Daniel Vetter wrote: > > > > > On Sun, Mar 18, 2012 at 01:39:45PM -0700, Ben Widawsky wrote: > > > > Implement the context switch code as well as the interfaces to do the > > > > context switch. This patch also doesn't match 1:1 with the RFC patches. > > > > The main difference is that from Daniel's responses the last context > > > > object is now stored instead of the last context. This aids in allows us > > > > to free the context data structure, and context object independently. > > > > > > > > There is room for optimization: this code will pin the context object > > > > until the next context is active. The optimal way to do it is to > > > > actually pin the object, move it to the active list, do the context > > > > switch, and then unpin it. This allows the eviction code to actually > > > > evict the context object if needed. > > > > > > > > The context switch code is missing workarounds, they will be implemented > > > > in future patches. > > > > > > > > Signed-off-by: Ben Widawsky > > > > > > Ok, I've looked at the use-sites of context_get and all this refcounting > > > and noticed that: > > > - we always hold dev->struct_mutex > > > - we always drop the acquired reference to the context structure in the > > > same function without dropping struct_mutex in between. > > > > > > So we don't seem to require any reference counting on these (and > > > additional locking on the idr). Additionally the idr locking seems to give > > > us a false sense of security because afaics the locking/refcounting would > > > be broken when we would _not_ hold struct_mutex. > > > > > > So can we just rip this out or do we need this (in which case it needs > > > some more work imo)? > > > -Daniel > > > > I think it can be ripped out. I was on the fence about this before > > submitting the patches and left it out of laziness; it doesn't hurt as > > there is no lock contention assuming your statement is true with no > > bugs in the code, and it follows the canonical use of idrs. > > > > Let me look it over some more to make sure after you finish reviewing > > the other stuff. The idea was supposed to be contexts can be created > > and looked up without struct mutex, but that isn't actually done in the > > code. > > Well, if you want to leave it I have to add some more review comments > about it - atm I think it would be buggy and racy without holding > struct_mutex ... > -Daniel As I said before, I'll either find a good use for it, or remove it. Context creation is the only currently viable case for it- but probably not worth the extra lock.