[Intel-gfx] [PATCH 2/2] drm/i915: Drop i915_gem_obj_is_pinned() from set-cache-level

[Tvrtko Ursulin]

On 09/10/15 11:34, Chris Wilson wrote:
> On Fri, Oct 09, 2015 at 11:17:19AM +0100, Tvrtko Ursulin wrote:
>>>>>>> -		list_for_each_entry(vma, &obj->vma_list, vma_link)
>>>>>>> -			if (drm_mm_node_allocated(&vma->node)) {
>>>>>>> -				ret = i915_vma_bind(vma, cache_level,
>>>>>>> -						    PIN_UPDATE);
>>>>>>> -				if (ret)
>>>>>>> -					return ret;
>>>>>>> -			}
>>>>>>> +		/* Access to snoopable pages through the GTT is incoherent. */
>>>>>>> +		if (cache_level != I915_CACHE_NONE && !HAS_LLC(dev))
>>>>>>> +			i915_gem_release_mmap(obj);
>>>>>>
>>>>>> Don't fully understand this one - but my question is this.
>>>>>> Previously userspace would lose mappings on cache level changes any
>>>>>> time, after this only on !LLC when turning on caching mode. So this
>>>>>> means userspace needs to know about this change and modify it's
>>>>>> behavior? Or what exactly would happen in practice?
>>>>>
>>>>> No. Userspace has no knowledge of the kernel handling the PTEs, its
>>>>> mapping is persistent (i.e. the obj->mmap_offset inside the dev->mappping).
>>>>> Otoh, we are improving the situation so even if userspace tries to avoid
>>>>> set-cache-level nothing is lost.
>>>>
>>>> Hm so if a VMA is re-bound in this process and it could have gotten
>>>> a new GGTT address, why it is not necessary to always release mmaps
>>>> and so to update CPU PTEs?
>>>
>>> The VMA are not moved by this function, only the PTEs are rewritten. The
>>> GTT ignores the bits we are changing on llc architectures. On !llc using
>>> the GTT to access snoopable PTE is verboten and does cause machine hangs.
>>
>> How come they are not moved when they can be unbound and then bound again?
>
> The only relevant vma here are rebound with PIN_UPDATE. If we have to
> unbind any due to subsequent placement errors, the behaviour doesn't
> change in this patch. So I'm not understanding your concern and can't
> address it adequately. :(

I started to understand how this works after a chat on IRC. Before I had 
a completely wrong assumptions.

(This also demonstrates this code should really have a good high level 
comment.)

Unless I missed something it really looks the behaviour is unchanged, 
just a trip to the fault handler is avoided if not needed.

But I still think you need to improve the commit message to be clearer 
on pin_display (un)usage and SandyBridge referencing.

Regards,

Tvrtko