From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTPS id 9B4E210F065 for ; Wed, 5 Jan 2022 05:02:37 +0000 (UTC) Date: Tue, 04 Jan 2022 21:00:50 -0800 Message-ID: <87y23ubwu5.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" In-Reply-To: <20211230182613.35827-2-zbigniew.kempczynski@intel.com> References: <20211230182613.35827-1-zbigniew.kempczynski@intel.com> <20211230182613.35827-2-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 1/3] lib/intel_memory_region: Add start offset and alignment detection 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, 30 Dec 2021 10:26:11 -0800, Zbigniew Kempczy=F1ski wrote: > I am still trying to understand this patch so just some questions/comments for now. I am not even looking at the caching part for now, it would have been better to keep caching as a separate patch since that is performance improvement but anyway leave as is. I will just review it later. > With era of new gens we're enforced to use no-reloc (softpin). This > brings few problems like vm range limitations which were well solved > by the kernel. This can be handled also in userspace code by adding > gen related conditionals or by trying to detect the constraints. So why not just do this since the information is static and gen dependent? Also what does this offset/alignment really depend on? Is it the minimum page size supported by a particular gen for a region type? Or is there more to it? If we know the minimum page size for each region type per gen can we compute the min/safe offset/alignemnt values below or do we still need this dynamic detection? Also, as far as I see, here we are calculating how generated offsets from the allocator should be aligned. So rather than do this in the allocator itself (i.e. introduce a 'region' argument to the allocator) we have these functions to find the 'alignment' argument to allocator->alloc() function. But it would be ok to do this inside the allocator itself and introduce a region argument to the allocator, i.e. generate an offset for this region or this set of regions? Anyway I see the allocator API at present takes the alignment argument so it is probably ok as is too. So at a high level do we need to add these functions here or should we add them inside the allocator? > +/** > + * gem_detect_min_start_offset_for_region: > + * @i915: drm fd > + * @region: memory region > + * > + * Returns: minimum start offset at which kernel allows placing objects > + * for memory region. > + */ > +uint64_t gem_detect_min_start_offset_for_region(int i915, uint32_t regio= n) Should this be static, that is should we only expose gem_detect_safe_start_offset() to the tests? Is there any reason why any test will call this directly? Also I suggest: s/detect/get/ since 'detect' is an implementation detail (and with the cache we are not even detecting). > + batch =3D gem_mmap__device_coherent(i915, obj.handle, 0, bb_size, PROT_= WRITE); > + *batch =3D MI_BATCH_BUFFER_END; > + munmap(batch, bb_size); I prefer gem_write() everywhere since it's a single statement but ok as is = too. > + > + while (1) { > + obj.offset =3D start_offset; > + > + if (__gem_execbuf(i915, &eb) =3D=3D 0) > + break; > + > + if (start_offset) > + start_offset <<=3D 1; > + else > + start_offset =3D PAGE_SIZE; > + > + igt_assert(start_offset <=3D 1ull << 48); s/48/GEN8_GTT_ADDRESS_WIDTH/ or something like that? > +/** > + * gem_detect_safe_start_offset: > + * @i915: drm fd > + * > + * Returns: finds start offset which can be used as first one regardless > + * memory region. Useful if for some reason some regions don't = allow > + * starting from 0x0 offset. > + */ > +uint64_t gem_detect_safe_start_offset(int i915) Overall I am ok with gem_detect_safe_start_offset() and gem_detect_min_start_offset_for_region() and understand that we need them. > +/** > + * gem_detect_min_alignment_for_regions: > + * @i915: drm fd > + * @region1: first region > + * @region2: second region > + * > + * Returns: minimum alignment which must be used when objects from @regi= on1 and > + * @region2 are going to interact. > + */ > +uint64_t gem_detect_min_alignment_for_regions(int i915, Once again can this be static, that is we don't expose to the tests? > + uint32_t region1, > + uint32_t region2) Also we can probably pass the list of all regions here (similar to what we do for gem_create_in_memory_regions()) but probably ok as is too the way it's done in gem_detect_safe_alignment(). > + obj[0].offset =3D gem_detect_min_start_offset_for_region(i915, region1); > + > + /* Find appropriate alignment of object */ > + eb.buffer_count =3D ARRAY_SIZE(obj); > + igt_assert(__gem_create_in_memory_regions(i915, &obj[1].handle, > + &obj_size, region2) =3D=3D 0); > + obj[1].handle =3D gem_create_in_memory_regions(i915, PAGE_SIZE, region2= ); > + obj[1].flags =3D EXEC_OBJECT_PINNED; > + while (1) { > + obj[1].offset =3D ALIGN(obj[0].offset + bb_size, min_alignment); > + igt_assert(obj[1].offset <=3D 1ull << 32); > + > + if (__gem_execbuf(i915, &eb) =3D=3D 0) > + break; > + > + min_alignment <<=3D 1; > + } With this loop above I get very lost and don't clearly understand the reason for it (taking the regions pairwise). Why do we need to do it this way? Why is the value returned by gem_detect_safe_start_offset() (that is the max offset over all regions) or even gem_detect_min_start_offset_for_region() sufficient? Is this algorithm above always guaranteed to return a value which will work? Also, do we even need to expose gem_detect_safe_start_offset() to the tests or all the tests will ever need to call is gem_detect_safe_alignment() so everything else can be static? > +/** > + * gem_get_safe_alignment: Comment mismatches function name below. > + * @i915: drm fd > + * > + * Returns: safe alignment for all memory regions on @i915 device. > + * Safe in this case means max() from all minimum alignment for each > + * region. > + */ > +uint64_t gem_detect_safe_alignment(int i915)