[Intel-xe] [PATCH] xe/xe_bo: Reject bo creation of unaligned size

[Souza, Jose]

On Thu, 2023-04-20 at 10:52 +0200, Thomas Hellström wrote:
> +Maarten
> 
> Hi, Zbigniew, Mauro
> 
> On 4/13/23 08:56, Zbigniew Kempczyński wrote:
> > From: Mauro Carvalho Chehab <mauro.chehab at linux.intel.com>
> > 
> > For xe bo creation we request passing size which matches system or
> > vram minimum page alignment. This way we want to ensure userspace
> > is aware of region constraints and not aligned allocations will be
> > rejected returning EINVAL.
> 
> Personally, I think the alignment constraints is a property / 
> restriction of the Xe GPU binding that should not interfere with bo 
> creation / size that should instead reflect the size of the data we need 
> to store. So the alignment constraints should apply only to the GPU VA 
> space allocation. This would also make the semantics of importing 
> dma-bufs of unaligned size straightforward.

Seconded!
In my opinion it should only set the size allocated by Xe KMD in drm_xe_gem_create.size and leave the rest as is.

> 
> Although admittedly I'm not fully up-to-date with the discussion 
> preceding the choice to return -EINVAL if the bo size is not aligned to 
> our GPU binding constraints. (Maarten, could you perhaps fill in?)
> 
> Thanks,
> 
> Thomas
> 
> 
> 
> 
> > 
> > Signed-off-by: Mauro Carvalho Chehab <mauro.chehab at linux.intel.com>
> > Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
> > ---
> >   drivers/gpu/drm/xe/xe_bo.c | 23 ++++++++++++++++-------
> >   include/uapi/drm/xe_drm.h  |  5 ++---
> >   2 files changed, 18 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
> > index 3ab404e33fae..d3972c84bcce 100644
> > --- a/drivers/gpu/drm/xe/xe_bo.c
> > +++ b/drivers/gpu/drm/xe/xe_bo.c
> > @@ -1090,6 +1090,7 @@ struct xe_bo *__xe_bo_create_locked(struct xe_device *xe, struct xe_bo *bo,
> >   	};
> >   	struct ttm_placement *placement;
> >   	uint32_t alignment;
> > +	size_t aligned_size;
> >   	int err;
> >   
> >   	/* Only kernel objects should set GT */
> > @@ -1098,22 +1099,30 @@ struct xe_bo *__xe_bo_create_locked(struct xe_device *xe, struct xe_bo *bo,
> >   	if (XE_WARN_ON(!size))
> >   		return ERR_PTR(-EINVAL);
> >   
> > -	if (!bo) {
> > -		bo = xe_bo_alloc();
> > -		if (IS_ERR(bo))
> > -			return bo;
> > -	}
> > -
> >   	if (flags & (XE_BO_CREATE_VRAM_MASK | XE_BO_CREATE_STOLEN_BIT) &&
> >   	    !(flags & XE_BO_CREATE_IGNORE_MIN_PAGE_SIZE_BIT) &&
> >   	    xe->info.vram_flags & XE_VRAM_FLAGS_NEED64K) {
> > -		size = ALIGN(size, SZ_64K);
> > +		aligned_size = ALIGN(size, SZ_64K);
> > +		if (type != ttm_bo_type_device)
> > +			size = ALIGN(size, SZ_64K);
> >   		flags |= XE_BO_INTERNAL_64K;
> >   		alignment = SZ_64K >> PAGE_SHIFT;
> > +
> >   	} else {
> > +		aligned_size = ALIGN(size, SZ_4K);
> > +		flags &= ~XE_BO_INTERNAL_64K;
> >   		alignment = SZ_4K >> PAGE_SHIFT;
> >   	}
> >   
> > +	if (type == ttm_bo_type_device && aligned_size != size)
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	if (!bo) {
> > +		bo = xe_bo_alloc();
> > +		if (IS_ERR(bo))
> > +			return bo;
> > +	}
> > +
> >   	bo->gt = gt;
> >   	bo->size = size;
> >   	bo->flags = flags;
> > diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
> > index b0b80aae3ee8..d3e64af7eabb 100644
> > --- a/include/uapi/drm/xe_drm.h
> > +++ b/include/uapi/drm/xe_drm.h
> > @@ -259,9 +259,8 @@ struct drm_xe_gem_create {
> >   	__u64 extensions;
> >   
> >   	/**
> > -	 * @size: Requested size for the object
> > -	 *
> > -	 * The (page-aligned) allocated size for the object will be returned.
> > +	 * @size: Size of the object to be created, must match region
> > +	 * (system or vram) minimum alignment.
> >   	 */
> >   	__u64 size;
> >