On Thu, Aug 05, 2021 at 06:43:56PM -0700, Dixit, Ashutosh wrote:

On Mon, 26 Jul 2021 12:59:52 -0700, Zbigniew Kempczyński wrote:

quoted

For newer gens we're not able to rely on relocations. Adopt to use
offsets acquired from the allocator.

Reviewed-by: Ashutosh Dixit <redacted>

But a couple of questions/comments below.

quoted

+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 = 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 = EXEC_OBJECT_ASYNC;
obj[1].handle = gem_create(fd, 4096);

+	if (id) {
+		obj[0].offset = target_offset;
+		obj[0].flags |= EXEC_OBJECT_PINNED | EXEC_OBJECT_WRITE |
+				EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
+		obj[1].offset = (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)?

In store_dword() we got gem_close(fd, obj[1].handle) so we risk in
igt_fork() we got same offset for different batches. 

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

Ok, I'm going to add comment with the description why we need simple
and why we pass 'id' to store_word().

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

quoted

@@ -89,6 +101,8 @@ static void one(int fd, const intel_ctx_t *ctx,

uint32_t scratch = gem_create(fd, 4096);
igt_spin_t *spin;
uint32_t *result;
+	uint64_t ahnd = get_simple_l2h_ahnd(fd, ctx->id);

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

.dependency (scratch). We want to get same offset for same handle so
reloc allocator cannot be used here.

--
Zbigniew