From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by gabe.freedesktop.org (Postfix) with ESMTPS id 2D0616E9DD for ; Thu, 19 Aug 2021 21:16:15 +0000 (UTC) Date: Thu, 19 Aug 2021 14:16:13 -0700 Message-ID: <87v941b0f6.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" In-Reply-To: <20210819044941.7699-2-zbigniew.kempczynski@intel.com> References: <20210819044941.7699-1-zbigniew.kempczynski@intel.com> <20210819044941.7699-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] 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 Wed, 18 Aug 2021 21:49:39 -0700, Zbigniew Kempczy=F1ski wrote: > > @@ -518,12 +533,24 @@ static void store_dword(int i915, const intel_ctx_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)? > -static void unplug_show_queue(int i915, struct igt_cork *c, > +static void unplug_show_queue(int i915, struct igt_cork *c, uint64_t ahn= d, > 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 good 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 ? > @@ -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_RELOC), I915_EXEC_NO_RELOC needed? Also !! has gone out of fashion :) > @@ -831,6 +903,8 @@ static void smoketest(int i915, const intel_ctx_cfg_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? Please take a look at the nits above and if anything needs to be done. Otherwise this is: Reviewed-by: Ashutosh Dixit