On Thu, Aug 05, 2021 at 02:14:12PM -0700, Dixit, Ashutosh wrote:

On Thu, 05 Aug 2021 01:02:41 -0700, Zbigniew Kempczyński wrote:

quoted

Please fix the put_ahnd. With that this patch is:

Reviewed-by: Ashutosh Dixit <redacted>

But I want to discuss the intel_allocator_multiprocess_start/stop issue a
bit more below.

quoted

On Wed, Aug 04, 2021 at 07:07:41PM -0700, Dixit, Ashutosh wrote:

quoted

On Mon, 26 Jul 2021 12:59:44 -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.

Signed-off-by: Zbigniew Kempczyński <redacted>
Cc: Petri Latvala <redacted>
Cc: Ashutosh Dixit <redacted>
---
 tests/i915/gem_busy.c | 35 +++++++++++++++++++++++++++++++----
 1 file changed, 31 insertions(+), 4 deletions(-)

diff --git a/tests/i915/gem_busy.c b/tests/i915/gem_busy.c
index f0fca0e8a..51ec5ad04 100644
--- a/tests/i915/gem_busy.c
+++ b/tests/i915/gem_busy.c

@@ -108,6 +108,7 @@ static void semaphore(int fd, const intel_ctx_t *ctx,

uint32_t handle[3];
uint32_t read, write;
uint32_t active;
+	uint64_t ahnd = get_reloc_ahnd(fd, ctx->id);
unsigned i;

handle[TEST] = gem_create(fd, 4096);

@@ -117,6 +118,7 @@ static void semaphore(int fd, const intel_ctx_t *ctx,

/* Create a long running batch which we can use to hog the GPU */
handle[BUSY] = gem_create(fd, 4096);
spin = igt_spin_new(fd,
+			    .ahnd = ahnd,
		    .ctx = ctx,
		    .engine = e->flags,
		    .dependency = handle[BUSY]);

Missing put_ahnd.

Good catch.

quoted

quoted

@@ -428,6 +442,7 @@ igt_main

igt_subtest_group {
	igt_fixture {
+			intel_allocator_multiprocess_start();
		igt_fork_hang_detector(fd);
	}

@@ -445,6 +460,21 @@ igt_main

		}
	}

Just above here is basic() which doesn't have a fork. Is it ok to do
intel_allocator_multiprocess_start/stop when we don't have a fork? If yes,
then can we _always_ do intel_allocator_multiprocess_start/stop rather than
only when we have fork? Thanks.

intel_allocator_multiprocess_start() creates allocator thread which acts
for children (igt_fork) to alloc/free offsets. If you use alloc/free within
same process (from which thread was spawned) internal structure is mutexed
and no IPCs are called. So only consequence of this here is additional thread
in system/memory (which does nothing for basic() tests). It will be stopped
with intel_allocator_multiprocess_stop().

OK, in that case let me ask the question I asked above in another way. Can
we add intel_allocator_multiprocess_start() to common_init() (the program
entry point) and similarly say intel_allocator_multiprocess_stop() to
igt_exit() (or common_exit_handler(), basically the program exit point) so
that these always run and we don't have to add them only for specific tests
which fork? What would be the disadvantage of doing this? Thanks.

At the moment likely not. Currently each test which completes reinitialize
data structures (intel_allocator_init(), called in exit_subtest()). It
doesn't perform any sysvipc calls (recreating msgqueue). I should
happen to clean the mess from previous (likely failed) run - what
intel_allocator_multiprocess_stop() does now. Starting/stopping allocator
which is designed for Intel i915 mostly would also be called in kms
tests which are vendor agnostic if I'm not wrong.

I still queued to handle situations when child calls assert. This generates
problem now because we're stopping test now without stopping allocator thread
properly.

I would stick at the moment to spawn allocator thread when it is necessary.
Currently allocator implementation  multiprocess environment is fragile and 
handle errors in limited manner.

Putting intel_allocator_multiprocess_start()/stop() in fixture is imo best
way of handling error situations. We ensure something we create/destroy ipc
regardless tests results.

--
Zbigniew

quoted

But for purity test should work without additional dependencies so I'll fix
this - it will be sent in v4.

--
Zbigniew