From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by gabe.freedesktop.org (Postfix) with ESMTPS id E918E6E9FE for ; Fri, 20 Aug 2021 05:37:09 +0000 (UTC) Date: Thu, 19 Aug 2021 22:37:00 -0700 Message-ID: <87czq87k3n.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" In-Reply-To: <20210820044614.GA3286@zkempczy-mobl2> References: <20210819044941.7699-1-zbigniew.kempczynski@intel.com> <20210819044941.7699-2-zbigniew.kempczynski@intel.com> <87v941b0f6.wl-ashutosh.dixit@intel.com> <20210820044614.GA3286@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 1/3] tests/gem_ctx_shared: Adopt to use allocator 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, 19 Aug 2021 21:46:14 -0700, Zbigniew Kempczy=F1ski wrote: > > On Thu, Aug 19, 2021 at 02:16:13PM -0700, Dixit, Ashutosh wrote: > > On Wed, 18 Aug 2021 21:49:39 -0700, Zbigniew Kempczy=F1ski wrote: > > > > > > @@ -518,12 +533,24 @@ static void store_dword(int i915, const intel_c= tx_t *ctx, unsigned ring, > > > > > > memset(obj, 0, sizeof(obj)); > > > obj[0].handle =3D cork; > > > - obj[0].offset =3D cork << 20; > > > obj[1].handle =3D target; > > > - obj[1].offset =3D target << 20; > > > obj[2].handle =3D gem_create(i915, 4096); > > > - obj[2].offset =3D 256 << 10; > > > - obj[2].offset +=3D (random() % 128) << 12; > > > + if (ahnd) { > > > + obj[0].offset =3D cork_offset; > > > + obj[0].flags |=3D EXEC_OBJECT_PINNED; > > > + obj[1].offset =3D target_offset; > > > + obj[1].flags |=3D EXEC_OBJECT_PINNED; > > > + if (write_domain) > > > + obj[1].flags |=3D EXEC_OBJECT_WRITE; > > > + obj[2].offset =3D get_offset(ahnd, obj[2].handle, 4096, 0x0); > > > + obj[2].flags |=3D EXEC_OBJECT_PINNED; > > > + execbuf.flags |=3D I915_EXEC_NO_RELOC; > > > > I915_EXEC_NO_RELOC needed (since I think we weren't doing this earlier)? > > That's minor optimization for softpinning. We got relocation_count =3D=3D= 0 for > each gem object, so eb->relocs is empty list. We also don't expect offset > change on execbuf return. I thought I915_EXEC_NO_RELOC is only needed if we are using relocations, if we are softpinning it's not needed? > > > > > -static void unplug_show_queue(int i915, struct igt_cork *c, > > > +static void unplug_show_queue(int i915, struct igt_cork *c, uint64_t= ahnd, > > > const intel_ctx_cfg_t *cfg, unsigned int engine) > > > { > > > igt_spin_t *spin[MAX_ELSP_QLEN]; > > > > > > for (int n =3D 0; n < ARRAY_SIZE(spin); n++) { > > > const intel_ctx_t *ctx =3D create_highest_priority(i915, cfg); > > > - spin[n] =3D __igt_spin_new(i915, .ctx =3D ctx, .engine =3D engine); > > > + > > > + /* > > > + * When we're using same vm we should use allocator handle > > > + * passed by the caller. This is the case where cfg->vm > > > + * is not NULL. > > > + * > > > + * For cases where context use its own vm we need separate > > > + * ahnd for it. > > > + */ > > > + if (!cfg->vm) > > > + ahnd =3D get_reloc_ahnd(i915, ctx->id); > > > > Ok, good, even though in this code we always used shared vm this is goo= d in > > case something changes later. > > > > > @@ -599,8 +643,10 @@ static uint32_t store_timestamp(int i915, > > > uint32_t handle =3D gem_create(i915, 4096); > > > struct drm_i915_gem_exec_object2 obj =3D { > > > .handle =3D handle, > > > - .relocation_count =3D 1, > > > - .offset =3D (32 << 20) + (handle << 16), > > > + .relocation_count =3D !ahnd ? 1 : 0, > > > + .offset =3D !ahnd ? (32 << 20) + (handle << 16) : > > > + get_offset(ahnd, handle, 4096, 0), > > > + .flags =3D !ahnd ? 0 : EXEC_OBJECT_PINNED, > > > > | EXEC_OBJECT_WRITE ? > > No, see there's no write_domain in reloc struct. We're waiting for objects > explicitely - see gem_set_domain() /* no write hazard lies */. OK, got it. > > > > > @@ -612,7 +658,7 @@ static uint32_t store_timestamp(int i915, > > > struct drm_i915_gem_execbuffer2 execbuf =3D { > > > .buffers_ptr =3D to_user_pointer(&obj), > > > .buffer_count =3D 1, > > > - .flags =3D ring | I915_EXEC_FENCE_IN, > > > + .flags =3D ring | I915_EXEC_FENCE_IN | (!!ahnd * I915_EXEC_NO_RELO= C), > > > > I915_EXEC_NO_RELOC needed? Also !! has gone out of fashion :) > > I wanted to put minimal code changes here. If you really don't like !!ahnd > I will remove it, putting dedicated if () to add this flag later. No need to change, but I am still not clear about using I915_EXEC_NO_RELOC with softpin. > > > > > > @@ -831,6 +903,8 @@ static void smoketest(int i915, const intel_ctx_c= fg_t *cfg, > > > unsigned engine; > > > uint32_t scratch; > > > uint32_t *ptr; > > > + uint64_t ahnd =3D get_reloc_ahnd(i915, 0); /* same vm */ > > > + uint64_t scratch_offset; > > > > > > q_cfg =3D *cfg; > > > q_cfg.vm =3D gem_vm_create(i915); > > > @@ -848,9 +922,11 @@ static void smoketest(int i915, const intel_ctx_= cfg_t *cfg, > > > igt_require(nengine); > > > > > > scratch =3D gem_create(i915, 4096); > > > + scratch_offset =3D get_offset(ahnd, scratch, 4096, 0); > > > igt_fork(child, ncpus) { > > > unsigned long count =3D 0; > > > const intel_ctx_t *ctx; > > > + ahnd =3D get_reloc_ahnd(i915, 0); /* ahnd to same vm */ > > > > Can't we use the parent's ahnd here, since we are using > > intel_allocator_multiprocess_start/stop? > > No, you cannot use ahnd created within parent process in the child. > IPC communication rely on child_tid value which is properly assigned on > intel_allocator_open(). Idea of this is to send requests to the allocator > by the children with their own child_tid to distinguish them. Please track > > - intel_allocator.c: send_req_recv_resp() > - intel_allocator.c: send_req() > - intel_allocator_msgchannel.c: msgqueue_send_req() > > calls. Using child_tid in this communication will become clear then > (especially it's initialization necessity on intel_allocator_open()). > > That's why calling intel_allocator_open() in each child is necessary, > it just assigns that magic number for IPC which distinguishes requests. Ok, got it, thanks! > > -- > Zbigniew > > > > > Please take a look at the nits above and if anything needs to be > > done. Otherwise this is: > > > > Reviewed-by: Ashutosh Dixit