[igt-dev] [PATCH i-g-t] i915/gem_exec_await: Avoid DG2 conflicts

Kamil Konieczny kamil.konieczny at linux.intel.com
Mon Jul 24 17:35:08 UTC 2023


Hi Andrzej,

small nit, see below.

On 2023-07-06 at 18:00:14 +0200, Andrzej Hajda wrote:
> From: Chris Wilson <chris.p.wilson at linux.intel.com>
> 
> DG2 is restricted in what contexts/engines can be run concurrently, if
> we submit a non-preemptible context on both rcs/ccs it will only run one
> at a time. Progress (heartbeats) along ccs will be blocked by rcs, and
> vice versa. This is independent of the ccs switch holdout w/a.
> Since this is not required for constructing a wide set of active fences
> (a fence is active until it has been signaled, whether it is running on
> HW or waiting to run is irrelevant to the signal state), refactor the
> context construction to be favourable for DG2.
> 
> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/5892
> Signed-off-by: Chris Wilson <chris.p.wilson at linux.intel.com>
> [ahajda: adjust to upstream driver]
> Signed-off-by: Andrzej Hajda <andrzej.hajda at intel.com>
> ---
>  tests/i915/gem_exec_await.c | 180 ++++++++++++++++--------------------
>  1 file changed, 80 insertions(+), 100 deletions(-)
> 
> diff --git a/tests/i915/gem_exec_await.c b/tests/i915/gem_exec_await.c
> index 53b7bac2f96..b87adcb4a18 100644
> --- a/tests/i915/gem_exec_await.c
> +++ b/tests/i915/gem_exec_await.c
> @@ -25,12 +25,23 @@
>  #include <sys/ioctl.h>
>  #include <sys/signal.h>
>  
> +#include "drmtest.h"
>  #include "i915/gem.h"
>  #include "i915/gem_create.h"
> -#include "igt.h"
> +#include "i915/gem_engine_topology.h"
> +#include "i915/gem_mman.h"
> +#include "i915/gem_submission.h"
> +#include "i915/gem_vm.h"
> +#include "igt_aux.h"
> +#include "igt_core.h"
>  #include "igt_rand.h"
>  #include "igt_sysfs.h"
> +#include "igt_types.h"
>  #include "igt_vgem.h"
> +#include "ioctl_wrappers.h"
------------ ^^
Move to last include (keep it sorted alphabetically).

> +#include "intel_chipset.h"
> +#include "intel_ctx.h"
> +#include "intel_gpu_commands.h"

Put newline here.

>  /**
>   * TEST: gem exec await
>   * Category: Infrastructure
> @@ -66,7 +77,7 @@ static void xchg_obj(void *array, unsigned i, unsigned j)
>  }
>  
>  #define CONTEXTS 0x1
> -static void wide(int fd, const intel_ctx_t *ctx, int ring_size,
> +static void wide(int fd, intel_ctx_cfg_t *cfg, int ring_size,
>  		 int timeout, unsigned int flags)
>  {
>  	const struct intel_execution_engine2 *engine;
> @@ -75,7 +86,6 @@ static void wide(int fd, const intel_ctx_t *ctx, int ring_size,
>  	struct {
>  		struct drm_i915_gem_exec_object2 *obj;
>  		struct drm_i915_gem_exec_object2 exec[2];
> -		struct drm_i915_gem_relocation_entry reloc;
>  		struct drm_i915_gem_execbuffer2 execbuf;
>  		const intel_ctx_t *ctx;
>  		uint32_t *cmd;
> @@ -83,9 +93,13 @@ static void wide(int fd, const intel_ctx_t *ctx, int ring_size,
>  	struct drm_i915_gem_exec_object2 *obj;
>  	struct drm_i915_gem_execbuffer2 execbuf;
>  	unsigned engines[I915_EXEC_RING_MASK + 1], nengine;
> +	const intel_ctx_t *ctx;
>  	unsigned long count;
>  	double time;
> -	uint64_t ahnd = get_reloc_ahnd(fd, 0); /* just offset provider */
> +
> +	__gem_vm_create(fd, &cfg->vm);
> +	if (__intel_ctx_create(fd, cfg, &ctx))
> +		ctx = intel_ctx_0(fd);
>  
>  	nengine = 0;
>  	for_each_ctx_engine(fd, ctx, engine) {
> @@ -102,7 +116,7 @@ static void wide(int fd, const intel_ctx_t *ctx, int ring_size,
>  	igt_assert(exec);
>  
>  	igt_require_memory(nengine*(2 + ring_size), 4096, CHECK_RAM);
> -	obj = calloc(nengine*ring_size + 1, sizeof(*obj));
> +	obj = calloc(nengine * (ring_size  + 1) + 1, sizeof(*obj));
>  	igt_assert(obj);
>  
>  	for (unsigned e = 0; e < nengine; e++) {
> @@ -111,69 +125,63 @@ static void wide(int fd, const intel_ctx_t *ctx, int ring_size,
>  		for (unsigned n = 0; n < ring_size; n++)  {
>  			exec[e].obj[n].handle = gem_create(fd, 4096);
>  			exec[e].obj[n].flags = EXEC_OBJECT_WRITE;
> -			exec[e].obj[n].offset = get_offset(ahnd, exec[e].obj[n].handle,
> -							   4096, 0);
> -			if (ahnd)
> -				exec[e].obj[n].flags |= EXEC_OBJECT_PINNED;
> -
> -			obj[e*ring_size + n].handle = exec[e].obj[n].handle;
> -			obj[e*ring_size + n].offset = exec[e].obj[n].offset;
> +			obj[e * ring_size + n] = exec[e].obj[n];
>  		}
>  
>  		exec[e].execbuf.buffers_ptr = to_user_pointer(exec[e].exec);
> -		exec[e].execbuf.buffer_count = 1;
> -		exec[e].execbuf.flags = (engines[e] |
> -					 I915_EXEC_NO_RELOC |
> -					 I915_EXEC_HANDLE_LUT);
> +		exec[e].execbuf.buffer_count = 2;
> +		exec[e].execbuf.flags = engines[e];
> +		exec[e].execbuf.rsvd1 = ctx->id;
>  
>  		if (flags & CONTEXTS) {
> -			exec[e].ctx = intel_ctx_create(fd, &ctx->cfg);
> +			exec[e].ctx = intel_ctx_create(fd, cfg);
>  			exec[e].execbuf.rsvd1 = exec[e].ctx->id;
> -		} else {
> -			exec[e].execbuf.rsvd1 = ctx->id;
>  		}
>  
> -		exec[e].exec[0].handle = gem_create(fd, 4096);
> -		exec[e].exec[0].offset = get_offset(ahnd, exec[e].exec[0].handle,
> -						    4096, 0);
> -		if (ahnd)
> -			exec[e].exec[0].flags = EXEC_OBJECT_PINNED;
> -
> -		exec[e].cmd = gem_mmap__device_coherent(fd, exec[e].exec[0].handle,
> -							0, 4096, PROT_WRITE);
> -
> -		gem_set_domain(fd, exec[e].exec[0].handle,
> -			       I915_GEM_DOMAIN_WC, I915_GEM_DOMAIN_WC);
> -		exec[e].cmd[0] = MI_BATCH_BUFFER_END;
> -
> -		gem_execbuf(fd, &exec[e].execbuf);
> -		exec[e].exec[1] = exec[e].exec[0];
> -		exec[e].execbuf.buffer_count = 2;
> -
> -		exec[e].reloc.target_handle = 1; /* recurse */
> -		exec[e].reloc.offset = sizeof(uint32_t);
> -		exec[e].reloc.read_domains = I915_GEM_DOMAIN_COMMAND;
> -		if (gen < 4)
> -			exec[e].reloc.delta = 1;
> -
> -		exec[e].exec[1].relocs_ptr = to_user_pointer(&exec[e].reloc);
> -		exec[e].exec[1].relocation_count = !ahnd ? 1 : 0;
> +		exec[e].exec[1].handle = gem_create(fd, 4096);
> +		obj[nengine * ring_size + e] = exec[e].exec[1];
>  	}
>  
> -	obj[nengine*ring_size].handle = gem_create(fd, 4096);
> -	gem_write(fd, obj[nengine*ring_size].handle, 0, &bbe, sizeof(bbe));
> -
> -	obj[nengine*ring_size].offset = get_offset(ahnd, obj[nengine*ring_size].handle,
> -						   4096, 0);
> -	if (ahnd)
> -		obj[nengine*ring_size].flags |= EXEC_OBJECT_PINNED;
> +	obj[nengine * (ring_size + 1)].handle = gem_create(fd, 4096);
> +	gem_write(fd, obj[nengine * (ring_size + 1)].handle, 0,
> +		  &bbe, sizeof(bbe));
>  
>  	memset(&execbuf, 0, sizeof(execbuf));
> -	execbuf.buffers_ptr = to_user_pointer(&obj[nengine*ring_size]);
> -	execbuf.buffer_count = 1;
> -	gem_execbuf(fd, &execbuf); /* tag the object as a batch in the GTT */
>  	execbuf.buffers_ptr = to_user_pointer(obj);
> -	execbuf.buffer_count = nengine*ring_size + 1;
> +	execbuf.buffer_count = nengine * (ring_size + 1) + 1;
> +	execbuf.rsvd1 = ctx->id;
> +	gem_execbuf(fd, &execbuf); /* tag the object as a batch in the GTT */
> +	for (unsigned e = 0; e < nengine; e++) {
> +		uint64_t address;
> +		uint32_t *cs;
> +
> +		for (unsigned n = 0; n < ring_size; n++) {
> +			obj[e * ring_size + n].flags |= EXEC_OBJECT_PINNED;
> +			exec[e].obj[n] = obj[e * ring_size + n];
> +		}
> +		exec[e].exec[1] = obj[nengine * ring_size + e];
> +		exec[e].exec[1].flags |= EXEC_OBJECT_PINNED;
> +		address = exec[e].exec[1].offset;
> +
> +		exec[e].cmd = gem_mmap__device_coherent(fd, exec[e].exec[1].handle,
> +							0, 4096, PROT_WRITE);
> +		cs = exec[e].cmd;
> +
> +		*cs++ = MI_NOOP;

Maybe place a comment here why it is needed as you later
place here MI_ARB_CHECK.

Regards,
Kamil

> +		if (gen >= 8) {
> +			*cs++ = MI_BATCH_BUFFER_START | 1 << 8 | 1;
> +			*cs++ = address;
> +			*cs++ = address >> 32;
> +		} else if (gen >= 6) {
> +			*cs++ = MI_BATCH_BUFFER_START | 1 << 8;
> +			*cs++ = address;
> +		} else {
> +			*cs++ = MI_BATCH_BUFFER_START | 2 << 6;
> +			if (gen < 4)
> +				address |= 1;
> +			*cs++ = address;
> +		}
> +	}
>  
>  	intel_detect_and_clear_missed_interrupts(fd);
>  
> @@ -182,42 +190,22 @@ static void wide(int fd, const intel_ctx_t *ctx, int ring_size,
>  	igt_until_timeout(timeout) {
>  		struct timespec start, now;
>  		for (unsigned e = 0; e < nengine; e++) {
> -			uint64_t address;
> -			int i;
> -
>  			if (flags & CONTEXTS) {
>  				intel_ctx_destroy(fd, exec[e].ctx);
> -				exec[e].ctx = intel_ctx_create(fd, &ctx->cfg);
> +				exec[e].ctx = intel_ctx_create(fd, cfg);
>  				exec[e].execbuf.rsvd1 = exec[e].ctx->id;
>  			}
>  
> -			exec[e].reloc.presumed_offset = exec[e].exec[1].offset;
> -			address = (exec[e].reloc.presumed_offset +
> -				   exec[e].reloc.delta);
>  			gem_set_domain(fd, exec[e].exec[1].handle,
>  				       I915_GEM_DOMAIN_WC, I915_GEM_DOMAIN_WC);
> +			exec[e].cmd[0] = MI_ARB_CHECK;
>  
> -			i = 0;
> -			exec[e].cmd[i] = MI_BATCH_BUFFER_START;
> -			if (gen >= 8) {
> -				exec[e].cmd[i] |= 1 << 8 | 1;
> -				exec[e].cmd[++i] = address;
> -				exec[e].cmd[++i] = address >> 32;
> -			} else if (gen >= 6) {
> -				exec[e].cmd[i] |= 1 << 8;
> -				exec[e].cmd[++i] = address;
> -			} else {
> -				exec[e].cmd[i] |= 2 << 6;
> -				exec[e].cmd[++i] = address;
> -			}
> -
> -			exec[e].exec[0] = obj[nengine*ring_size];
> +			exec[e].exec[0] = obj[nengine * (ring_size + 1)];
>  			gem_execbuf(fd, &exec[e].execbuf);
>  
>  			for (unsigned n = 0; n < ring_size; n++) {
>  				exec[e].exec[0] = exec[e].obj[n];
>  				gem_execbuf(fd, &exec[e].execbuf);
> -				exec[e].obj[n].offset = exec[e].exec[0].offset;
>  			}
>  		}
>  
> @@ -225,10 +213,7 @@ static void wide(int fd, const intel_ctx_t *ctx, int ring_size,
>  
>  		clock_gettime(CLOCK_MONOTONIC, &start);
>  		for (unsigned e = 0; e < nengine; e++) {
> -			execbuf.flags = (engines[e] |
> -					 I915_EXEC_NO_RELOC |
> -					 I915_EXEC_HANDLE_LUT);
> -			execbuf.rsvd1 = ctx->id;
> +			execbuf.flags = engines[e];
>  			gem_execbuf(fd, &execbuf);
>  		}
>  		clock_gettime(CLOCK_MONOTONIC, &now);
> @@ -245,43 +230,40 @@ static void wide(int fd, const intel_ctx_t *ctx, int ring_size,
>  	igt_info("%s: %'lu cycles: %.3fus\n",
>  		 __func__, count, time*1e6 / count);
>  
> -	gem_close(fd, obj[nengine*ring_size].handle);
> +	for (unsigned n = 0; n < nengine * (ring_size + 1) + 1; n++)
> +		gem_close(fd, obj[n].handle);
>  	free(obj);
>  
>  	for (unsigned e = 0; e < nengine; e++) {
>  		if (flags & CONTEXTS)
>  			intel_ctx_destroy(fd, exec[e].ctx);
>  
> -		for (unsigned n = 0; n < ring_size; n++) {
> -			gem_close(fd, exec[e].obj[n].handle);
> -			put_offset(ahnd, exec[e].obj[n].handle);
> -		}
> -		free(exec[e].obj);
> -
>  		munmap(exec[e].cmd, 4096);
> -		gem_close(fd, exec[e].exec[1].handle);
> -		put_offset(ahnd, exec[e].exec[1].handle);
> +		free(exec[e].obj);
>  	}
>  	free(exec);
> -	put_ahnd(ahnd);
> +
> +	intel_ctx_destroy(fd, ctx);
> +	__gem_vm_destroy(fd, cfg->vm);
> +	cfg->vm = 0;
>  }
>  
>  #define TIMEOUT 20
>  
>  igt_main
>  {
> +	intel_ctx_cfg_t cfg;
>  	int ring_size = 0;
> -	int device = -1;
> -	const intel_ctx_t *ctx;
> +	igt_fd_t(device);
>  
>  	igt_fixture {
>  
>  		device = drm_open_driver(DRIVER_INTEL);
>  		igt_require_gem(device);
>  		gem_submission_print_method(device);
> -		ctx = intel_ctx_create_all_physical(device);
> +		cfg = intel_ctx_cfg_all_physical(device);
>  
> -		ring_size = gem_submission_measure(device, &ctx->cfg, ALL_ENGINES);
> +		ring_size = gem_submission_measure(device, &cfg, ALL_ENGINES);
>  
>  		igt_info("Ring size: %d batches\n", ring_size);
>  		igt_require(ring_size > 0);
> @@ -290,16 +272,14 @@ igt_main
>  	}
>  
>  	igt_subtest("wide-all")
> -		wide(device, ctx, ring_size, TIMEOUT, 0);
> +		wide(device, &cfg, ring_size, TIMEOUT, 0);
>  
>  	igt_subtest("wide-contexts") {
>  		gem_require_contexts(device);
> -		wide(device, ctx, ring_size, TIMEOUT, CONTEXTS);
> +		wide(device, &cfg, ring_size, TIMEOUT, CONTEXTS);
>  	}
>  
>  	igt_fixture {
>  		igt_stop_hang_detector();
> -		intel_ctx_destroy(device, ctx);
> -		drm_close_driver(device);
>  	}
>  }
> -- 
> 2.34.1
> 


More information about the igt-dev mailing list