Re: Oops at i915_gem_retire_requests_ring

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

On Thu, 17 Mar 2011 09:39:07 -0300, Herton Ronaldo Krzesinski <herton.krzesinski@canonical.com> wrote:
> I don't know if it's the most correct fix, but perhaps the simple fix
> is needed in the code. It's against latest Linus tree. We may have an
> already removed client_list, or we didn't add any item to client_list
> (file_priv NULL in i915_add_request)
> 
> Signed-off-by: Herton Ronaldo Krzesinski <herton.krzesinski@canonical.com>
> ---
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 36e66cc..6077c0d 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1749,8 +1749,10 @@ i915_gem_request_remove_from_client(struct drm_i915_gem_request *request)
>  		return;
>  
>  	spin_lock(&file_priv->mm.lock);
> -	list_del(&request->client_list);
> -	request->file_priv = NULL;
> +	if (request->file_priv) {
> +		list_del(&request->client_list);
> +		request->file_priv = NULL;
> +	}
>  	spin_unlock(&file_priv->mm.lock);
>  }

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().
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre