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

[Daniel Vetter]

On Tue, Dec 22, 2015 at 05:14:34PM +0000, Tvrtko Ursulin wrote:
> 
> 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..

Please only BUG_ON if imminent kernel death in the next few instructions
is a certainty. Otherwise I think it's much better to carry on and give
the user a change to grab logs and other useful information before the
system keels over completely.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch