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

[Jani Nikula <jani.nikula@linux.intel.com>]

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