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

[Zbigniew Kempczyński]

On Fri, May 26, 2023 at 05:40:08PM +0200, Souza, Jose wrote:
> 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.
> 

I'm sorry but I don't follow. Which strategy then should apply? This
with reject (in this patch) or in

https://patchwork.freedesktop.org/patch/529204/?series=115726&rev=1

?
--
Zbigniew

> > 
> > 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;
> > >   
>