Re: [Intel-gfx] [PATCH 03/14] drm/i915/xehpsdv: enforce min GTT alignment

[Matthew Auld <matthew.auld@intel.com>]

On 14/10/2021 14:33, Daniel Vetter wrote:
> On Wed, Oct 13, 2021 at 03:13:33PM +0100, Matthew Auld wrote:
>> On 13/10/2021 14:38, Daniel Vetter wrote:
>>> On Mon, Oct 11, 2021 at 09:41:44PM +0530, Ramalingam C wrote:
>>>> From: Matthew Auld <matthew.auld@intel.com>
>>>>
>>>> For local-memory objects we need to align the GTT addresses to 64K, both
>>>> for the ppgtt and ggtt.
>>>>
>>>> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
>>>> Signed-off-by: Stuart Summers <stuart.summers@intel.com>
>>>> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
>>>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>>
>>> Do we still need this with relocations removed? Userspace is picking all
>>> the addresses for us, so all we have to check is whether userspace got it
>>> right.
>>
>> Yeah, for OFFSET_FIXED this just validates that the provided address is
>> correctly aligned to 64K, while for the in-kernel insertion stuff we still
>> need to allocate an address that is aligned to 64K. Setting the alignment
>> here handles both cases.
> 
> Can't we just teach any in-kernel allocators to align to 2M and call it a
> day? Ofc the code can still validate we don't have bugs (always good to
> check your work). Ofc if the benefits is "no code can be removed anyway
> since we still need to check" then ofc no point :-)

Yeah, if we want to make it 2M, then we still need to add that here, 
like with 64K.

> 
> Just want to make sure we're not carrying complexity around for nothing,
> since this predates the relocation removal.

If we were to just make everything 2M then we can in theory ditch all 
the colouring stuff. Assuming userspace is happy with 2M or nothing, 
then it just means potentially terrible utilisation of the ppGTT for the 
kernel, but maybe that doesn't matter much really? For userspace maybe 
they will have some kind of sub-allocation scheme?

Well actually I guess it would be 2M alignment + 2M vma padding for 
anything that can be placed in I915_MEMORY_CLASS_DEVICE, including mixed 
objects. And then the usual 4K alignment for I915_MEMORY_CLASS_SYSTEM 
only objects.


> -Daniel
> 
>>
>>> -Daniel
>>>
>>>
>>>> ---
>>>>    drivers/gpu/drm/i915/i915_vma.c | 9 +++++++--
>>>>    1 file changed, 7 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
>>>> index 4b7fc4647e46..1ea1fa08efdf 100644
>>>> --- a/drivers/gpu/drm/i915/i915_vma.c
>>>> +++ b/drivers/gpu/drm/i915/i915_vma.c
>>>> @@ -670,8 +670,13 @@ i915_vma_insert(struct i915_vma *vma, u64 size, u64 alignment, u64 flags)
>>>>    	}
>>>>    	color = 0;
>>>> -	if (vma->obj && i915_vm_has_cache_coloring(vma->vm))
>>>> -		color = vma->obj->cache_level;
>>>> +	if (vma->obj) {
>>>> +		if (HAS_64K_PAGES(vma->vm->i915) && i915_gem_object_is_lmem(vma->obj))
>>>> +			alignment = max(alignment, I915_GTT_PAGE_SIZE_64K);
>>>> +
>>>> +		if (i915_vm_has_cache_coloring(vma->vm))
>>>> +			color = vma->obj->cache_level;
>>>> +	}
>>>>    	if (flags & PIN_OFFSET_FIXED) {
>>>>    		u64 offset = flags & PIN_OFFSET_MASK;
>>>> -- 
>>>> 2.20.1
>>>>
>>>
>