[Intel-gfx] [PATCH 3/6] drm/i915: Track pages pinned due to swizzling quirk

[Chris Wilson]

On Tue, Nov 01, 2016 at 08:39:14AM +0000, Tvrtko Ursulin wrote:
> 
> On 31/10/2016 10:26, Chris Wilson wrote:
> >If we have a tiled object and an unknown CPU swizzle pattern, we pin the
> >pages to prevent the object from being swapped out (and us corrupting
> >the contents as we do not know the access pattern and so cannot convert
> >it to linear and back to tiled on reuse). This requires us to remember
> >to drop the extra pinning when freeing the object, or else we trigger
> >warnings about the pin leak. In commit fbbd37b36fa5 ("drm/i915: Move
> >object release to a freelist + worker"), the object free path was
> >deferred to a work, but the unpinning of the quirk, along with marking
> >the object as reclaimable, was left on the immediate path (so that if
> >required we could reclaim the pages under memory pressure as early as
> >possible). However, this split introduced a bug where the pages we no
> >longer being unpinned if they were marked as unneeded.
> 
> Last sentence is broken.

Almost the right words in the wrong order.

> > 	if (obj->mm.madv != __I915_MADV_PURGED)
> >@@ -4335,14 +4342,12 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj)
> > {
> > 	struct drm_i915_gem_object *obj = to_intel_bo(gem_obj);
> >
> >+	if (obj->mm.quirked)
> >+		__i915_gem_object_unpin_pages(obj);
> >+
> > 	if (discard_backing_storage(obj))
> > 		obj->mm.madv = I915_MADV_DONTNEED;
> >
> >-	if (obj->mm.pages && obj->mm.madv == I915_MADV_WILLNEED &&
> >-	    to_i915(obj->base.dev)->quirks & QUIRK_PIN_SWIZZLED_PAGES &&
> >-	    i915_gem_object_is_tiled(obj))
> >-		__i915_gem_object_unpin_pages(obj);
> >-
> 
> This reordering would not have been enough to fix this?

Yes. I was trying to avoid the repeated if() conditions and thought a
flag would eliminate a few of them. It only managed to kill this one and
provide assertions elsewhere.

Still we reduce the 3 [of 4] tests done to one for all devices but some
odd gen3/gen4 (and gain a sanity check for uncommon code paths).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre