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

[Souza, Jose]

On Wed, 2023-05-31 at 14:52 +0200, Zbigniew Kempczyński wrote:
> 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

In my opinion we should do 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;
> > > >   
> >