* [PATCH] drm/i915: Always reset vma->ggtt_view.pages cache on unbinding
@ 2015-06-11 7:06 Chris Wilson
2015-06-11 9:59 ` Tvrtko Ursulin
0 siblings, 1 reply; 3+ messages in thread
From: Chris Wilson @ 2015-06-11 7:06 UTC (permalink / raw)
To: intel-gfx
Cc: Chris Wilson, Tvrtko Ursulin, Daniel Vetter, Michel Thierry,
stable
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 <tvrtko.ursulin@intel.com>
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 <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Michel Thierry <michel.thierry@intel.com>
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);
--
2.1.4
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] drm/i915: Always reset vma->ggtt_view.pages cache on unbinding
2015-06-11 7:06 [PATCH] drm/i915: Always reset vma->ggtt_view.pages cache on unbinding Chris Wilson
@ 2015-06-11 9:59 ` Tvrtko Ursulin
2015-06-11 11:56 ` [Intel-gfx] " Jani Nikula
0 siblings, 1 reply; 3+ messages in thread
From: Tvrtko Ursulin @ 2015-06-11 9:59 UTC (permalink / raw)
To: Chris Wilson, intel-gfx; +Cc: Daniel Vetter, stable
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 <tvrtko.ursulin@intel.com>
> 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 <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Michel Thierry <michel.thierry@intel.com>
> 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 <tvrtko.ursulin@intel.com>
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.
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [Intel-gfx] [PATCH] drm/i915: Always reset vma->ggtt_view.pages cache on unbinding
2015-06-11 9:59 ` Tvrtko Ursulin
@ 2015-06-11 11:56 ` Jani Nikula
0 siblings, 0 replies; 3+ messages in thread
From: Jani Nikula @ 2015-06-11 11:56 UTC (permalink / raw)
To: Tvrtko Ursulin, Chris Wilson, intel-gfx; +Cc: Daniel Vetter, stable
On Thu, 11 Jun 2015, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> 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 <tvrtko.ursulin@intel.com>
>> 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 <chris@chris-wilson.co.uk>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Cc: Michel Thierry <michel.thierry@intel.com>
>> 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 <tvrtko.ursulin@intel.com>
>
> 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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2015-06-11 11:56 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-06-11 7:06 [PATCH] drm/i915: Always reset vma->ggtt_view.pages cache on unbinding Chris Wilson
2015-06-11 9:59 ` Tvrtko Ursulin
2015-06-11 11:56 ` [Intel-gfx] " Jani Nikula
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox