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

[Daniel Vetter <daniel@ffwll.ch>]

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 :-)

Just want to make sure we're not carrying complexity around for nothing,
since this predates the relocation removal.
-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
> > > 
> > 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch