Re: [PATCH] drm/i915: Always reset vma->ggtt_view.pages cache on unbinding

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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