[igt-dev] [PATCH i-g-t 49/79] tests/i915/gem_exec_balancer: Don't reset engines on a context

[Dixit, Ashutosh]

On Thu, 17 Jun 2021 12:12:56 -0700, Jason Ekstrand wrote:
>
> Instead of resetting the set of engines to break implicit dependencies,
> just use a new context.  This does drop a theoretical bit of test
> coverage in the bonded chain tests because we can't drop the timeline
> easily.

I don't know enough to comment on the seriousness of this. If this scenario
is of interest would you know if this is being tested elsewhere?

I guess the original code could "drop the timeline easily" through a
set_load_balancer() call on the context which we cannot do after this
series. Or is the timeline being replaced/dropped a different way?

> However, those tests also use the FENCE_SUBMIT pattern in a way that the
> set of engines is swapped out between the first and second half of the
> bonded pair.  That seems pretty sketchy.

I did not understand this comment either. As I mention above I thought this
is the way to replace/drop the timeline but maybe I am wrong.

The patch itself is fine. If you think the above issues are ok, I can give
a R-b after I hear from you. Thanks.

> @@ -1067,11 +1072,12 @@ static void __bonded_sema(int i915, uint32_t ctx,
>	struct drm_i915_gem_execbuffer2 execbuf = {
>		.buffers_ptr = to_user_pointer(&batch),
>		.buffer_count = 1,
> -		.rsvd1 = ctx,
>	};
>	igt_spin_t *spin;
>
>	for (int i = 0; i < ARRAY_SIZE(priorities); i++) {
> +		uint32_t ctx;
> +
>		/* A: spin forever on seperate render engine */
>		spin = igt_spin_new(i915,
>				    .flags = (IGT_SPIN_POLL_RUN |
> @@ -1086,16 +1092,21 @@ static void __bonded_sema(int i915, uint32_t ctx,
>		 */

We should probably delete the comment above here in the code similar to
__bonded_chain* since we are not replacing/dropping the timeline here
anymore?

> @@ -1818,8 +1824,8 @@ static void __bonded_early(int i915, uint32_t ctx,
>	struct drm_i915_gem_execbuffer2 execbuf = {
>		.buffers_ptr = to_user_pointer(&batch),
>		.buffer_count = 1,
> -		.rsvd1 = ctx,
>	};
> +	uint32_t vm, ctx;
>	igt_spin_t *spin;
>
>	memset(bonds, 0, sizeof(bonds));
> @@ -1833,6 +1839,11 @@ static void __bonded_early(int i915, uint32_t ctx,
>		bonds[n].engines[0] = siblings[(n + 1) % count];
>	}
>
> +	/* We share a VM so that the spin cancel will work without a reloc */

Didn't understand this comment either. Though both __bonded_early() and
bonded() are deleted later on in the series so if had reordered the patches
we could have avoided changing these functions. Anyway, leave as is, no
need to change.