[Intel-gfx] [PATCH 05/18] drm/i915: context switch implementation

[Ben Widawsky]

On Thu, 29 Mar 2012 20:24:11 +0200
Daniel Vetter <daniel at 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 at 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.