Re: [Intel-gfx] [PATCH v2 2/4] drm/i915: Introduce guard pages to i915_vma

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

On 23/11/2022 18:54, Andi Shyti wrote:
> Hi Tvrtko,
> 
> [...]
> 
>>> @@ -768,6 +768,9 @@ i915_vma_insert(struct i915_vma *vma, struct i915_gem_ww_ctx *ww,
>>>    	GEM_BUG_ON(!IS_ALIGNED(alignment, I915_GTT_MIN_ALIGNMENT));
>>>    	GEM_BUG_ON(!is_power_of_2(alignment));
>>> +	guard = vma->guard; /* retain guard across rebinds */
>>> +	guard = ALIGN(guard, alignment);
>>
>> Why does guard area needs the same alignment as the requested mapping? What about the fact on 32-bit builds guard is 32-bit and alignment u64?
> 
> I guess this just to round up/down guard to something, not
> necessarily to that alignment.
> 
> Shall I remove it?

Don't know, initially I thought it maybe needs a comment on what's it 
doing and why. If it is about aligning to "something" then should it be 
I915_GTT_MIN_ALIGNMENT?

>>> @@ -777,6 +780,7 @@ i915_vma_insert(struct i915_vma *vma, struct i915_gem_ww_ctx *ww,
>>>    	if (flags & PIN_ZONE_4G)
>>>    		end = min_t(u64, end, (1ULL << 32) - I915_GTT_PAGE_SIZE);
>>>    	GEM_BUG_ON(!IS_ALIGNED(end, I915_GTT_PAGE_SIZE));
>>> +	GEM_BUG_ON(2 * guard > end);
>>
>> End is the size of relevant VA area at this point so what and why is this checking?
> 
> I think because we want to make sure the padding is at least not
> bigger that the size. What is actually wrong with this.

Same as above - if there is subtle special meaning please add a comment. 
Otherwise, for the whole object and not just the guards, it is covered by:

+	if (size > end - 2 * guard) {

I don't follow what is the point on only checking the guards.

> 
> [...]
> 
>>> @@ -855,6 +869,7 @@ i915_vma_insert(struct i915_vma *vma, struct i915_gem_ww_ctx *ww,
>>>    	GEM_BUG_ON(!i915_gem_valid_gtt_space(vma, color));
>>>    	list_move_tail(&vma->vm_link, &vma->vm->bound_list);
>>> +	vma->guard = guard;
>>
>> unsigned long into u32 - what guarantees no truncation?
> 
> we are missing here this part above:
> 
> 	guard = vma->guard; /* retain guard across rebinds */
> 	if (flags & PIN_OFFSET_GUARD) {
> 		GEM_BUG_ON(overflows_type(flags & PIN_OFFSET_MASK, u32));
> 		guard = max_t(u32, guard, flags & PIN_OFFSET_MASK);
> 	}
> 
> that should make sure that we fit into 32 bits.

Hm okay. I guess the u64 alignment and that "guard = ALIGN(guard, 
alignment);" is what is bothering me to begin with. In other words with 
that there is a chance to overflow vma->guard with a small guard and 
large alignment.

> 
> [...]
> 
>>> @@ -197,14 +197,15 @@ struct i915_vma {
>>>    	struct i915_fence_reg *fence;
>>>    	u64 size;
>>> -	u64 display_alignment;
>>>    	struct i915_page_sizes page_sizes;
>>>    	/* mmap-offset associated with fencing for this vma */
>>>    	struct i915_mmap_offset	*mmo;
>>> +	u32 guard; /* padding allocated around vma->pages within the node */
>>>    	u32 fence_size;
>>>    	u32 fence_alignment;
>>> +	u32 display_alignment;
>>
>> u64 -> u32 for display_alignment looks unrelated change.
>>
>> ./display/intel_fb_pin.c:       vma->display_alignment = max_t(u64, vma->display_alignment, alignment);
>> ./gem/i915_gem_domain.c:        vma->display_alignment = max_t(u64, vma->display_alignment, alignment);
>>
>> These two sites need to be changed not to use u64.
>>
>> Do this part in a separate patch?
> 
> Right! will remove it.

Okay, to be clear, refactoring of vma->display_alignemnt to be u32 as a 
separate patch in the series. Thanks!

Regards,

Tvrtko

> 
>>>    	/**
>>>    	 * Count of the number of times this vma has been opened by different
>>
>> Regards,
> 
> Thanks,
> Andi
> 
>> Tvrtko