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

[Zbigniew Kempczyński]

On Wed, Apr 12, 2023 at 04:07:38PM +0200, Maarten Lankhorst wrote:
> 
> On 2023-04-03 18:48, Mauro Carvalho Chehab wrote:
> > On Tue, 28 Mar 2023 15:08:26 +0200
> > Zbigniew Kempczyński <zbigniew.kempczynski at 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 at 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 at 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;
> 
> Hey,
> 
> This looks much better. Can we also make sure that if the kernel tries to
> allocate a 4k buffer we get a louder warning?

Hi.

Currently there's ~ dozen+ of places where kernel tries to allocate
smaller buffers (xe_bo_create_pin_map() path). I tried to fix all
of them but I still have no success with loading the driver this way.

Mauro suggested to forbid aligning for bo create ioctl path, I've tried
two changes:
1. introduce XE_BO_IOCTL_CREATE flag to distinct creation the buffer,
2. allow align for type != ttm_bo_type_device.

Both changes works for me.

According to 4K buffer - I've observed kernel allocates for example
scratch pages in system memory and allocation == 4K, so I'm not sure
we should warn this.

Anyway - I would like to be unblocked to prepare igt kms changes,
so I'm planning to send minimal patch which will start returning EINVAL
for misaligned size. I think we can polish this later, as for me
this uapi contract is most important at the moment.

--
Zbigniew

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