From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ben Widawsky Subject: Re: [PATCH 05/18] drm/i915: context switch implementation Date: Thu, 29 Mar 2012 11:43:12 -0700 Message-ID: <20120329114312.4f3f9a9a@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> 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 7504C9E78C for ; Thu, 29 Mar 2012 11:43:22 -0700 (PDT) In-Reply-To: <20120329182411.GA27737@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: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.