From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jani Nikula Subject: Re: [Intel-gfx] [PATCH] drm/i915: Always reset vma->ggtt_view.pages cache on unbinding Date: Thu, 11 Jun 2015 14:56:31 +0300 Message-ID: <87k2va5uhc.fsf@intel.com> References: <1434006368-26742-1-git-send-email-chris@chris-wilson.co.uk> <55795BFC.8010508@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain Return-path: In-Reply-To: <55795BFC.8010508@linux.intel.com> Sender: stable-owner@vger.kernel.org To: Tvrtko Ursulin , Chris Wilson , intel-gfx@lists.freedesktop.org Cc: Daniel Vetter , stable@vger.kernel.org List-Id: intel-gfx@lists.freedesktop.org On Thu, 11 Jun 2015, Tvrtko Ursulin wrote: > On 06/11/2015 08:06 AM, Chris Wilson wrote: >> With the introduction of multiple views of an obj in the same vm, each >> vma was taught to cache its copy of the pages (so that different views >> could have different page arrangements). However, this missed decoupling >> those vma->ggtt_view.pages when the vma released its reference on the >> obj->pages. As we don't always free the vma, this leads to a possible >> scenario (e.g. execbuffer interrupted by the shrinker) where the vma >> points to a stale obj->pages, and explodes. >> >> Fixes regression from commit fe14d5f4e5468c5b80a24f1a64abcbe116143670 >> Author: Tvrtko Ursulin >> Date: Wed Dec 10 17:27:58 2014 +0000 >> >> drm/i915: Infrastructure for supporting different GGTT views per object >> >> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1227892 >> Signed-off-by: Chris Wilson >> Cc: Tvrtko Ursulin >> Cc: Daniel Vetter >> Cc: Michel Thierry >> Cc: stable@vger.kernel.org >> --- >> drivers/gpu/drm/i915/i915_gem.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c >> index 9ae98b00ff56..377a6da31a1c 100644 >> --- a/drivers/gpu/drm/i915/i915_gem.c >> +++ b/drivers/gpu/drm/i915/i915_gem.c >> @@ -3214,8 +3214,8 @@ int i915_vma_unbind(struct i915_vma *vma) >> } else if (vma->ggtt_view.pages) { >> sg_free_table(vma->ggtt_view.pages); >> kfree(vma->ggtt_view.pages); >> - vma->ggtt_view.pages = NULL; >> } >> + vma->ggtt_view.pages = NULL; >> } >> >> drm_mm_remove_node(&vma->node); > > Nasty, thanks for fixing this. > > Reviewed-by: Tvrtko Ursulin > > If someone else will be confused how this can happen, key is the > reservation execbuffer path. That puts the VMA on the exec_list which > prevents i915_vma_unbind and i915_gem_vma_destroy from fully destroying > the VMA. So the VMA is left existing as an empty object in the list - > unbound and disassociated with the backing store. Kind of a cached > memory object. And then re-using it needs to clear the cached pages > pointer which is fixed above. Pushed to drm-intel-fixes with the above text added to commit message. Thanks for the patch and review. BR, Jani. > > Regards, > > Tvrtko > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Jani Nikula, Intel Open Source Technology Center