Re: Oops at i915_gem_retire_requests_ring

[Chris Wilson <chris@chris-wilson.co.uk>]

On Thu, 17 Mar 2011 13:54:45 -0300, Herton Ronaldo Krzesinski <herton.krzesinski@canonical.com> wrote:
> On Thu, Mar 17, 2011 at 01:46:34PM +0000, Chris Wilson wrote:
> > This is the single chunk required. I had thought that the actual
> > insertion/deletion was serialised under the struct mutex and the intention
> > of the spinlock was to protect the unlocked list traversal during
> > throttling. However, I missed that i915_gem_release() is also called
> > without struct mutex and so we do need the double check for
> > i915_gem_request_remove_from_client().
> 
> Ok. I just still have one doubt though, if in i915_add_request
> file/file_priv is NULL, wouldn't be possible to have an oops also in
> i915_gem_release without the check? As in this case,
> request->client_list wouldn't have mm.request_list added to it, and if
> an error occurs and i915_reset is called, which ends up calling
> i915_gem_release, we would try to do a list_del on request->client_list
> without items.

If the file_priv is NULL, then the request is not added to the client
mm.request_list and so it is not seen during i915_gem_release.

The list is file_priv->mm.request_list, the nodes within that are
request->client_list.

> If the check really isn't needed in i915_gem_release, then please
> consider this patch:

Done, thanks,
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre