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

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

On 01/11/2016 08:48, Chris Wilson wrote:
> 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).

Okay then, with the commit message fix:

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx