From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by gabe.freedesktop.org (Postfix) with ESMTPS id 413C710E9CA for ; Tue, 14 Dec 2021 04:58:42 +0000 (UTC) Date: Mon, 13 Dec 2021 20:58:41 -0800 Message-ID: <87wnk793zy.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" In-Reply-To: References: <20211209200453.15885-1-zbigniew.kempczynski@intel.com> <20211209200453.15885-3-zbigniew.kempczynski@intel.com> <877dc8a1y3.wl-ashutosh.dixit@intel.com> 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/i915/intel_memory_region: Add fallback for creating gem bo 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, 13 Dec 2021 09:38:36 -0800, Zbigniew Kempczy=F1ski wrote: > > On Mon, Dec 13, 2021 at 08:45:24AM -0800, Dixit, Ashutosh wrote: > > On Thu, 09 Dec 2021 12:04:53 -0800, Zbigniew Kempczy=F1ski wrote: > > > > > > diff --git a/lib/i915/intel_memory_region.c b/lib/i915/intel_memory_r= egion.c > > > index fd29eec90..2263f1984 100644 > > > --- a/lib/i915/intel_memory_region.c > > > +++ b/lib/i915/intel_memory_region.c > > > @@ -197,8 +197,23 @@ int __gem_create_in_memory_region_list(int fd, u= int32_t *handle, uint64_t *size, > > > .num_regions =3D num_regions, > > > .regions =3D to_user_pointer(mem_regions), > > > }; > > > + int ret; > > > > > > - return __gem_create_ext(fd, size, handle, &ext_regions.base); > > > + ret =3D __gem_create_ext(fd, size, handle, &ext_regions.base); > > > + > > > + /* > > > + * Provide fallback for stable kernels if region passed in the array > > > + * can be system memory. In this case we get -ENODEV but still > > > + * we're able to allocate gem bo in system memory using legacy call. > > > + */ > > > + if (ret =3D=3D -ENODEV) > > > + for (int i =3D 0; i < num_regions; i++) > > > + if (mem_regions[i].memory_class =3D=3D I915_MEMORY_CLASS_SYSTEM) { > > > + ret =3D __gem_create(fd, size, handle); > > > + break; > > > + } > > > > Shouldn't this function return success if mem_regions[] array contains = only > > a single system memory region entry (that is the sime of mem_regions[] > > array is 1 and the only entry is system memory)? But here we are return= ing > > success if /any/ of the regions passed in are system memory? Won't this > > cause an error in the caller? I am not sure if checking for -ENODEV ret= urn > > saves us in any way but I think the code should be changed to return > > success if mem_regions[] array contains only a single system memory reg= ion. > > If user want to create in one of memory region from the list > [device memory, system memory] I think we should create gem object in sys= tem > memory as normally kernel would do if it couldn't create within local mem= ory. > That's why I decided to iterate over request regions and create within > system memory if it is possible. > > Please take a look to conditional in stable kernels: > > if (!IS_ENABLED(CONFIG_DRM_I915_UNSTABLE_FAKE_LMEM)) > > So query + create_ext should be covered to avoid failures on IGT code > which works with not drm-tip code. OK, I think you are right, it's ok to create a smem-only object as long as just one of the permissible regions for the object is smem. Thanks. > > > > > I think the previous patch "lib/i915/intel_memory_region: Provide system > > memory region stub" is fine and serves this purpose of returning a sing= le > > system memory region. Do we even need this patch? > > Take a look to results: > https://intel-gfx-ci.01.org/tree/linux-stable/index-alt.html? > > gem_exec_basic requires properly running code: > > static uint32_t batch_create(int fd, uint32_t batch_size, uint32_t region) > { > const uint32_t bbe =3D MI_BATCH_BUFFER_END; > uint32_t handle; > > handle =3D gem_create_in_memory_regions(fd, batch_size, region); > gem_write(fd, handle, 0, &bbe, sizeof(bbe)); > > return handle; > } > > which will fail on gem_create_in_memory_regions() with -ENODEV. I just > want to cover also creating objects globally only when system memory > is available. > > -- > Zbigniew > > > > > > > > + > > > + return ret; > > > } > > > > > > /* gem_create_in_memory_region_list: > > > -- > > > 2.26.0 > > >