From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by gabe.freedesktop.org (Postfix) with ESMTPS id 9B3766EC17 for ; Tue, 5 Oct 2021 17:52:08 +0000 (UTC) Date: Tue, 05 Oct 2021 10:51:46 -0700 Message-ID: <874k9vnyvh.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" In-Reply-To: <20211005065218.GA3744@zkempczy-mobl2> References: <20211004054056.24346-1-zbigniew.kempczynski@intel.com> <20211004054056.24346-3-zbigniew.kempczynski@intel.com> <87bl44z8av.wl-ashutosh.dixit@intel.com> <20211005065218.GA3744@zkempczy-mobl2> MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=ISO-8859-2 Content-Transfer-Encoding: quoted-printable Subject: Re: [igt-dev] [PATCH i-g-t 2/2] lib/intel_bufops: Store gem bo size List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" To: Zbigniew =?ISO-8859-2?Q?Kempczy=F1ski?= Cc: igt-dev@lists.freedesktop.org, Petri Latvala List-ID: On Mon, 04 Oct 2021 23:52:18 -0700, Zbigniew Kempczy=F1ski wrote: > > On Mon, Oct 04, 2021 at 04:20:24PM -0700, Dixit, Ashutosh wrote: > > On Sun, 03 Oct 2021 22:40:56 -0700, Zbigniew Kempczy=F1ski wrote: > > > > > > intel_buf is keeping its size which may differ to underlying gem bo s= ize. > > > 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 d= ata > > > 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 *bo= ps, > > > size =3D 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 =3D size; > > > + buf->handle =3D handle; > > > > > > - if (handle) > > > - buf->handle =3D handle; > > > - else { > > > - if (!__gem_create_in_memory_regions(bops->fd, &handle, &size, regi= on)) > > > - buf->handle =3D handle; > > > - else > > > - buf->handle =3D 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 =3D size; > > > > The code after the above changes is like this: > > > > if (bo_size > 0) { > > igt_assert(bo_size >=3D size); > > size =3D bo_size; > > } > > > > /* Store buffer size to avoid mistakes in calculating it again = */ > > buf->size =3D size; > > buf->handle =3D handle; > > > > if (!handle) > > if (__gem_create_in_memory_regions(bops->fd, &buf->hand= le, &size, region)) > > igt_assert_eq(__gem_create(bops->fd, &size, &bu= f->handle), 0); > > > > /* Store gem bo size */ > > buf->bo_size =3D size; > > > > The function is called with: > > > > a. handle =3D=3D 0 or !=3D 0 > > b. bo_size =3D=3D 0 or !=3D 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, he= ight, bpp, alignment, > > intel_buf_init_in_region[868] __intel_buf_init(bops, 0, buf, width, he= ight, bpp, alignment, > > intel_buf_init_using_handle[927] __intel_buf_init(bops, handle, buf, wi= dth, height, bpp, alignment, > > intel_buf_create_using_handle_and_size[1013] __intel_buf_init(bops, han= dle, buf, width, height, bpp, alignment, > > > > So when handle !=3D 0 and bo_size =3D=3D 0, we end with both buf->size = =3D=3D buf->bo_size =3D=3D 0. > > But we're overwriting size only when bo_size > 0: > > if (bo_size > 0) { > igt_assert(bo_size >=3D size); > size =3D bo_size; > } > > > When handle =3D=3D 0 and bo_size =3D=3D 0, we end up with buf->size =3D= =3D 0 and buf->bo_size !=3D 0. > > Regardless handle we got size > 0 always. It comes from > > if (compression) { > ... > size =3D buf->ccs[0].offset + aux_width * aux_height; > } else { > ... > size =3D buf->surface[0].stride * ALIGN(height, align_h); > } > > or > > if (bo_size > 0) { > igt_assert(bo_size >=3D size); > size =3D bo_size; > } > > Asserts on the beginning guarantees we got size > 0: > > igt_assert(width > 0 && height > 0); > igt_assert(bpp =3D=3D 8 || bpp =3D=3D 16 || bpp =3D=3D 32 || bpp =3D=3D 6= 4); > > So initialization > > buf->size =3D size; > > won't be 0 here. > > > > > So this is not a new issue, maybe it's ok, but I just wanted to check w= ith > > 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 ti= me > 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