[igt-dev] [PATCH i-g-t 2/2] lib/intel_bufops: Store gem bo size

[Zbigniew Kempczyński]

On Mon, Oct 04, 2021 at 04:20:24PM -0700, Dixit, Ashutosh wrote:
> On Sun, 03 Oct 2021 22:40:56 -0700, Zbigniew Kempczyński wrote:
> >
> > intel_buf is keeping its size which may differ to underlying gem bo size.
> > Introduce keeping bo_size field which is used along with softpin mode
> > - like in intel_bb.
> >
> > Patch also should remove previous discrepancy where intel_buf_bo_size()
> > returned requested (not gem bo size).
> >
> > From now on user has an access to:
> > 1. raw buffer size - intel_buf_size() - function returns how buffer data
> >    really takes in the memory
> 
> Not sure what we mean by this since intel_buf_size() can return 0 even with
> a non-zero handle. See below.
> 
> > 2. gem bo buffer size - intel_buf_bo_size() - function returns how big
> >    underlying gem object is
> 
> > diff --git a/lib/intel_bufops.c b/lib/intel_bufops.c
> > index 7199723bb..80c5bb80b 100644
> > --- a/lib/intel_bufops.c
> > +++ b/lib/intel_bufops.c
> > @@ -813,17 +813,16 @@ static void __intel_buf_init(struct buf_ops *bops,
> >		size = bo_size;
> >	}
> >
> > -	/* Store real bo size to avoid mistakes in calculating it again */
> > +	/* Store buffer size to avoid mistakes in calculating it again */
> >	buf->size = size;
> > +	buf->handle = handle;
> >
> > -	if (handle)
> > -		buf->handle = handle;
> > -	else {
> > -		if (!__gem_create_in_memory_regions(bops->fd, &handle, &size, region))
> > -			buf->handle = handle;
> > -		else
> > -			buf->handle = gem_create(bops->fd, size);
> > -	}
> > +	if (!handle)
> > +		if (__gem_create_in_memory_regions(bops->fd, &buf->handle, &size, region))
> > +			igt_assert_eq(__gem_create(bops->fd, &size, &buf->handle), 0);
> > +
> > +	/* Store gem bo size */
> > +	buf->bo_size = size;
> 
> The code after the above changes is like this:
> 
>         if (bo_size > 0) {
>                 igt_assert(bo_size >= size);
>                 size = bo_size;
>         }
> 
>         /* Store buffer size to avoid mistakes in calculating it again */
>         buf->size = size;
>         buf->handle = handle;
> 
>         if (!handle)
>                 if (__gem_create_in_memory_regions(bops->fd, &buf->handle, &size, region))
>                         igt_assert_eq(__gem_create(bops->fd, &size, &buf->handle), 0);
> 
>         /* Store gem bo size */
>         buf->bo_size = size;
> 
> The function is called with:
> 
> a. handle == 0 or != 0
> b. bo_size == 0 or != 0
> 
> As seen in __intel_buf_init callers:
> 
> *** lib/intel_bufops.c:
> __intel_buf_init[728]          static void __intel_buf_init(struct buf_ops *bops,
> intel_buf_init[851]            __intel_buf_init(bops, 0, buf, width, height, bpp, alignment,
> intel_buf_init_in_region[868]  __intel_buf_init(bops, 0, buf, width, height, bpp, alignment,
> intel_buf_init_using_handle[927] __intel_buf_init(bops, handle, buf, width, height, bpp, alignment,
> intel_buf_create_using_handle_and_size[1013] __intel_buf_init(bops, handle, buf, width, height, bpp, alignment,
> 
> So when handle != 0 and bo_size == 0, we end with both buf->size == buf->bo_size == 0.

But we're overwriting size only when bo_size > 0:

	if (bo_size > 0) {
		igt_assert(bo_size >= size);
		size = bo_size;
	}

> When handle == 0 and bo_size == 0, we end up with buf->size == 0 and buf->bo_size != 0.

Regardless handle we got size > 0 always. It comes from 

	if (compression) {
		...
		size = buf->ccs[0].offset + aux_width * aux_height;	
	} else {
		...
		size = buf->surface[0].stride * ALIGN(height, align_h);
	}

or

	if (bo_size > 0) {
		igt_assert(bo_size >= size);
		size = bo_size;
	}

Asserts on the beginning guarantees we got size > 0:

	igt_assert(width > 0 && height > 0);
	igt_assert(bpp == 8 || bpp == 16 || bpp == 32 || bpp == 64);

So initialization

	buf->size = size; 

won't be 0 here.

> 
> So this is not a new issue, maybe it's ok, but I just wanted to check with
> you if you think all these scenarios work out ok even after introducing
> separate buf->size and buf->bo_size. Thanks.

Thank you're carefully looking at the code. Please go over it one more time
and verify what I've written. Maybe I just don't see something obvious...

--
Zbigniew