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

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

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.
When handle == 0 and bo_size == 0, we end up with buf->size == 0 and buf->bo_size != 0.

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.