From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <igt-dev-bounces@lists.freedesktop.org>
Received: from mga04.intel.com (mga04.intel.com [192.55.52.120])
 by gabe.freedesktop.org (Postfix) with ESMTPS id B519C6E7D1
 for <igt-dev@lists.freedesktop.org>; Fri,  6 Aug 2021 02:01:13 +0000 (UTC)
Date: Thu, 05 Aug 2021 18:43:56 -0700
Message-ID: <874kc3tkir.wl-ashutosh.dixit@intel.com>
From: "Dixit, Ashutosh" <ashutosh.dixit@intel.com>
In-Reply-To: <20210726200026.4815-19-zbigniew.kempczynski@intel.com>
References: <20210726200026.4815-1-zbigniew.kempczynski@intel.com>	<20210726200026.4815-19-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 v3 18/52] tests/gem_exec_async: Adopt to
 use allocator
List-Unsubscribe: <https://lists.freedesktop.org/mailman/options/igt-dev>,
 <mailto:igt-dev-request@lists.freedesktop.org?subject=unsubscribe>
List-Archive: <https://lists.freedesktop.org/archives/igt-dev>
List-Post: <mailto:igt-dev@lists.freedesktop.org>
List-Help: <mailto:igt-dev-request@lists.freedesktop.org?subject=help>
List-Subscribe: <https://lists.freedesktop.org/mailman/listinfo/igt-dev>,
 <mailto:igt-dev-request@lists.freedesktop.org?subject=subscribe>
Errors-To: igt-dev-bounces@lists.freedesktop.org
Sender: "igt-dev" <igt-dev-bounces@lists.freedesktop.org>
To: Zbigniew =?ISO-8859-2?Q?Kempczy=F1ski?= <zbigniew.kempczynski@intel.com>
Cc: igt-dev@lists.freedesktop.org, Petri Latvala <petri.latvala@intel.com>
List-ID: <igt-dev@lists.freedesktop.org>

On Mon, 26 Jul 2021 12:59:52 -0700, Zbigniew Kempczy=F1ski wrote:
>
> For newer gens we're not able to rely on relocations. Adopt to use
> offsets acquired from the allocator.

Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com>

But a couple of questions/comments below.

> +static void store_dword(int fd, int id, const intel_ctx_t *ctx,
> +			 unsigned ring, uint32_t target, uint64_t target_offset,
> +			 uint32_t offset, uint32_t value)
>  {
>	const unsigned int gen =3D intel_gen(intel_get_drm_devid(fd));
>	struct drm_i915_gem_exec_object2 obj[2];
> @@ -50,6 +53,15 @@ static void store_dword(int fd, const intel_ctx_t *ctx=
, unsigned ring,
>	obj[0].flags =3D EXEC_OBJECT_ASYNC;
>	obj[1].handle =3D gem_create(fd, 4096);
>
> +	if (id) {
> +		obj[0].offset =3D target_offset;
> +		obj[0].flags |=3D EXEC_OBJECT_PINNED | EXEC_OBJECT_WRITE |
> +				EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
> +		obj[1].offset =3D (id + 1) * SZ_1M;

So this is where I think we are assigning new offsets to successive batches
to avoid stalls, correct? Though I don't know why we don't get these
offsets from the allocator (though I guess this will work since we have a
4K scratch buffer and whatever the spin needs)?

Maybe we can add a one line comment above, something like:

/* Assign new offsets to successive batches to prevent stalls */

> @@ -89,6 +101,8 @@ static void one(int fd, const intel_ctx_t *ctx,
>	uint32_t scratch =3D gem_create(fd, 4096);
>	igt_spin_t *spin;
>	uint32_t *result;
> +	uint64_t ahnd =3D get_simple_l2h_ahnd(fd, ctx->id);

Is there a particular reason for using simple rather than the reloc
allocator here?