From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chris Wilson Subject: Re: Oops at i915_gem_retire_requests_ring Date: Thu, 17 Mar 2011 17:06:40 +0000 Message-ID: References: <20110317123906.GA1857@herton-IdeaPad-Y430> <20110317165444.GA3078@herton-IdeaPad-Y430> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20110317165444.GA3078@herton-IdeaPad-Y430> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: Herton Ronaldo Krzesinski Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Thu, 17 Mar 2011 13:54:45 -0300, Herton Ronaldo Krzesinski 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