Re: [Intel-xe] [PATCH] drm/xe/bo: Return object bo size on create

["Zbigniew Kempczyński" <zbigniew.kempczynski@intel.com>]

On Mon, Apr 03, 2023 at 06:48:07PM +0200, Mauro Carvalho Chehab wrote:
> On Tue, 28 Mar 2023 15:08:26 +0200
> Zbigniew Kempczyński <zbigniew.kempczynski@intel.com> wrote:
> 
> > Driver may alter bo size requested by the user. Return real object
> > size to make userspace aware how to arrange vm bindings.
> > 
> > Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_bo.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
> > index e4d079b61d52..410e797931ac 100644
> > --- a/drivers/gpu/drm/xe/xe_bo.c
> > +++ b/drivers/gpu/drm/xe/xe_bo.c
> > @@ -1564,6 +1564,7 @@ int xe_gem_create_ioctl(struct drm_device *dev, void *data,
> >  		return err;
> >  
> >  	args->handle = handle;
> > +	args->size = bo->size;
> >  
> >  	return 0;
> >  }
> 
> An alternative approach would be instead to return -EINVAL if the
> buffers aren't aligned. Something like this (untested) patch.
> 
> Regards,
> Mauro
> 
> [PATCH] xe/xe_bo: Don't allow creating unaligned buffers
> 
> Ensure that bo will always be properly aligned.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>
> 
> diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
> index f25257bbfc65..ecc04b887790 100644
> --- a/drivers/gpu/drm/xe/xe_bo.c
> +++ b/drivers/gpu/drm/xe/xe_bo.c
> @@ -985,6 +985,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 */
> @@ -993,24 +994,28 @@ 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;
> -	}
> -
> -	bo->requested_size = size;
>  	if (flags & (XE_BO_CREATE_VRAM0_BIT | XE_BO_CREATE_VRAM1_BIT |
>  		     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);
>  		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 (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/drivers/gpu/drm/xe/xe_ggtt.c b/drivers/gpu/drm/xe/xe_ggtt.c
> index a430d1568890..f8a55ef9e789 100644
> --- a/drivers/gpu/drm/xe/xe_ggtt.c
> +++ b/drivers/gpu/drm/xe/xe_ggtt.c
> @@ -273,10 +273,8 @@ void xe_ggtt_map_bo(struct xe_ggtt *ggtt, struct xe_bo *bo)
>  		xe_ggtt_set_pte(ggtt, start + offset, pte);
>  	}
>  
> -	if (bo->size == bo->requested_size) {
> -		pte = xe_ggtt_pte_encode(ggtt->scratch ?: bo, 0);
> -		xe_ggtt_set_pte(ggtt, start + bo->size, pte);
> -	}
> +	pte = xe_ggtt_pte_encode(ggtt->scratch ?: bo, 0);
> +	xe_ggtt_set_pte(ggtt, start + bo->size, pte);
>  
>  	if (ggtt->invalidate) {
>  		xe_ggtt_invalidate(ggtt->gt);
> @@ -309,11 +307,9 @@ static int __xe_ggtt_insert_bo_at(struct xe_ggtt *ggtt, struct xe_bo *bo,
>  	 * fact we have programmed this GGTT entry to a valid entry. BO aligned
>  	 * to 64k already have padding so no need to add an extra page.
>  	 */
> -	if (bo->size == bo->requested_size) {
> -		size += SZ_4K;
> -		if (end != U64_MAX)
> -			end += SZ_4K;
> -	}
> +        size += alignment;
> +        if (end != U64_MAX)
> +                end += alignment;
>  
>  	mutex_lock(&ggtt->lock);
>  	err = drm_mm_insert_node_in_range(&ggtt->mm, &bo->ggtt_node, size,

I'm going to test this patch and let you know.

At the moment I got two concerns:

1. take a look to xe_query.c:query_memory_usage()
   min_page_size is not always 64K whereas current bo create
   path doesn't honour this configuration.

2. xe_drm.h also should be changed to reflect changes
   which disallows kernel modification of size

   struct drm_xe_gem_create {
   ...
	/**
	 * @size: Requested size for the object
	 *
	 * The (page-aligned) allocated size for the object will be returned.
	 */
	__u64 size;

--
Zbigniew