[Intel-gfx] [PATCH 08/10] drm/i915: Migrate stolen objects before hibernation

[Tvrtko Ursulin]

On 22/12/15 17:02, Chris Wilson wrote:
> On Tue, Dec 22, 2015 at 12:33:41PM +0000, Tvrtko Ursulin wrote:
>> On 22/12/15 06:20, ankitprasad.r.sharma at intel.com wrote:
>>> From: Chris Wilson <chris at chris-wilson.co.uk>
>>> +	/* Recreate any pinned binding with pointers to the new storage */
>>> +	if (!list_empty(&obj->vma_list)) {
>>> +		ret = i915_gem_object_get_pages_gtt(obj);
>>> +		if (ret) {
>>> +			obj->pages = stolen_pages;
>>> +			goto err_file;
>>> +		}
>>> +
>>> +		obj->get_page.sg = obj->pages->sgl;
>>> +		obj->get_page.last = 0;
>>> +
>>> +		list_for_each_entry(vma, &obj->vma_list, vma_link) {
>>> +			if (!drm_mm_node_allocated(&vma->node))
>>> +				continue;
>>> +
>>> +			WARN_ON(i915_vma_bind(vma,
>>> +					      obj->cache_level,
>>> +					      PIN_UPDATE));
>>
>> It looks like this should also fail (and restore) the migration.
>> Otherwise if it fails it leaves GTT mappings to pages which will be
>> released below.
>>
>> Or a big fat comment explaining why it cannot fail, ever.
>
> It is an impossible error, fortunately. The failure handling case would
> have to redo the previous rebindings which are then subject to exactly
> the same error.
>
> I take it WARN_ON isn't enough, you would rather we document impossible
> failure conditions with BUG_ON? And since this does leave hardware
> pointing into stolen, it should really be a full BUG_ON.

Ok ok BUG_ON for this one sounds better.

One day I'll try and sketch my I915_BUG_ON I mentioned a few times in 
the past..

Regards,

Tvrtko