Re: [PATCH 05/18] drm/i915: context switch implementation

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Mar 29, 2012 at 11:43:12AM -0700, Ben Widawsky wrote:
> On Thu, 29 Mar 2012 20:24:11 +0200
> Daniel Vetter <daniel@ffwll.ch> 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 <ben@bwidawsk.net>
> > 
> > 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
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48