From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by gabe.freedesktop.org (Postfix) with ESMTPS id E6B2610E7FB for ; Mon, 13 Dec 2021 16:45:25 +0000 (UTC) Date: Mon, 13 Dec 2021 08:45:24 -0800 Message-ID: <877dc8a1y3.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" In-Reply-To: <20211209200453.15885-3-zbigniew.kempczynski@intel.com> References: <20211209200453.15885-1-zbigniew.kempczynski@intel.com> <20211209200453.15885-3-zbigniew.kempczynski@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 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_regio= n.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, uint3= 2_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 returning 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 return 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 region. I think the previous patch "lib/i915/intel_memory_region: Provide system memory region stub" is fine and serves this purpose of returning a single system memory region. Do we even need this patch? > + > + return ret; > } > > /* gem_create_in_memory_region_list: > -- > 2.26.0 >