On Mon, 04 Oct 2021 23:52:18 -0700, Zbigniew Kempczyński wrote:

On Mon, Oct 04, 2021 at 04:20:24PM -0700, Dixit, Ashutosh wrote:

quoted

On Sun, 03 Oct 2021 22:40:56 -0700, Zbigniew Kempczyński wrote:

quoted

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.

quoted

2. gem bo buffer size - intel_buf_bo_size() - function returns how big
   underlying gem object is

quoted

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;
}

quoted

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.

quoted

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...

Sorry of course you are right, I completely missed size is being set as you
indicate above. So this is also:

Reviewed-by: Ashutosh Dixit <redacted>