Re: [Intel-gfx] [PATCH v6 3/5] drm/i915: Introduce guard pages to i915_vma

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

On 02/12/2022 11:11, Andi Shyti wrote:
> Hi Tvrtko,
> 
> On Fri, Dec 02, 2022 at 10:20:11AM +0000, Tvrtko Ursulin wrote:
>>
>> On 01/12/2022 20:39, Andi Shyti wrote:
>>> From: Chris Wilson <chris@chris-wilson.co.uk>
>>>
>>> Introduce the concept of padding the i915_vma with guard pages before
>>> and after. The major consequence is that all ordinary uses of i915_vma
>>> must use i915_vma_offset/i915_vma_size and not i915_vma.node.start/size
>>> directly, as the drm_mm_node will include the guard pages that surround
>>> our object.
>>>
>>> The biggest connundrum is how exactly to mix requesting a fixed address
>>> with guard pages, particularly through the existing uABI. The user does
>>> not know about guard pages, so such must be transparent to the user, and
>>> so the execobj.offset must be that of the object itself excluding the
>>> guard. So a PIN_OFFSET_FIXED must then be exclusive of the guard pages.
>>> The caveat is that some placements will be impossible with guard pages,
>>> as wrap arounds need to be avoided, and the vma itself will require a
>>> larger node. We must not report EINVAL but ENOSPC as these are unavailable
>>> locations within the GTT rather than conflicting user requirements.
>>>
>>> In the next patch, we start using guard pages for scanout objects. While
>>> these are limited to GGTT vma, on a few platforms these vma (or at least
>>> an alias of the vma) is shared with userspace, so we may leak the
>>> existence of such guards if we are not careful to ensure that the
>>> execobj.offset is transparent and excludes the guards. (On such platforms
>>> like ivb, without full-ppgtt, userspace has to use relocations so the
>>> presence of more untouchable regions within its GTT such be of no further
>>> issue.)
>>>
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Signed-off-by: Tejas Upadhyay <tejaskumarx.surendrakumar.upadhyay@intel.com>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> Signed-off-by: Andi Shyti <andi.shyti@linux.intel.com>
>>> ---
>>> Hi Tvrtko,
>>>
>>> I removed your r-b in this version because I restored the original value
>>> of the guard being aligned with the vma size alignment. Turns out that
>>> CI failed with the latest version because the guard was becoming too big
>>> (we would have hit the GEM_BUG_ON)[*].
>>>
>>> The reason why now the guard is aligned with the vma alignment is that
>>> the area is already aligned and if we use as a starting address start +
>>> guard, guard needs to be aligned, otherwise we screw up all the memory
>>> alignment.
>>>
>>> Let me know if it makes sense to you.
>>
>> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Conditional to promise of a prioritised follow up improvement, if it turns
>> out GGTT wastage due a bit over zealous guard size comes to bite.
> 
> Sure! I'll be alert!
> 
> There are some unrelated failures from CI, just to be sure I sent
> last night a trybot run.

Trybot looked okay, and I just pressed re-test for the intel-gfx series 
so lets see that too.

Regards,

Tvrtko