Re: [igt-dev] [PATCH i-g-t v2] lib/intel_bb: Enable custom engine support for xe

["Grzegorzek, Dominik" <dominik.grzegorzek@intel.com>]

On Wed, 2023-05-24 at 13:38 +0200, Christoph Manszewski wrote:
> Currently the 'ctx' field in batch buffer creation is interpreted as
> a vm id for xe. In i915 it is interpreted as a context id. Since a xe
> engine more closely resembles an i915 context, change the current
> behaviour and interpret the 'ctx' fied as an xe engine id. This also
> allows us to use the compute engine on xe, which currently is not
> possible, due to reliance on legacy i915 flags.
> 
> v2:
>  - don't destroy user provided engine in 'intel_bb_destroy' (Zbigniew)
>  - destroy internally created engine before creating a new one
> 
> Signed-off-by: Christoph Manszewski <christoph.manszewski@intel.com>
> Cc: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
> ---
>  lib/intel_batchbuffer.c | 27 +++++++++++++++++++--------
>  tests/xe/xe_intel_bb.c  |  7 +++++--
>  2 files changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/lib/intel_batchbuffer.c b/lib/intel_batchbuffer.c
> index dfccc4f4..d6a44d7e 100644
> --- a/lib/intel_batchbuffer.c
> +++ b/lib/intel_batchbuffer.c
> @@ -946,15 +946,13 @@ __intel_bb_create(int fd, uint32_t ctx, const intel_ctx_cfg_t *cfg,
>  		ibb->gtt_size = 1ull << min_t(uint32_t, xe_va_bits(fd), 48);
>  		end = ibb->gtt_size;
>  
> -		if (!ctx)
> -			ctx = xe_vm_create(fd, DRM_XE_VM_CREATE_ASYNC_BIND_OPS, 0);
> +		ibb->vm_id = xe_vm_create(fd, DRM_XE_VM_CREATE_ASYNC_BIND_OPS, 0);

As I see ibb->vm_id is used for vm_bind. So we pass xe_engine, which has it own vm under the hood,
and we use it in execbuf. At the same time we are binding objects to new, completly different vm. 
Do I miss sth here? 

In i915, an intel_bb could live without knowledge which vm hides behind the context,
as it was not using vm_bind. Here I think we would need to have both passed by the caller.

Regards,
Dominik



>  
>  		ibb->uses_full_ppgtt = true;
>  		ibb->allocator_handle =
> -			intel_allocator_open_full(fd, ctx, start, end,
> +			intel_allocator_open_full(fd, ibb->vm_id, start, end,
>  						  allocator_type, strategy,
>  						  ibb->alignment);
> -		ibb->vm_id = ctx;
>  		ibb->last_engine = ~0U;
>  	}
>  
> @@ -1228,6 +1226,14 @@ static void __intel_bb_remove_intel_bufs(struct intel_bb *ibb)
>  		intel_bb_remove_intel_buf(ibb, entry);
>  }
>  
> +static void __xe_bb_destroy(struct intel_bb *ibb)
> +{
> +	if (ibb->engine_id)
> +		xe_engine_destroy(ibb->fd, ibb->engine_id);
> +	if (ibb->vm_id)
> +		xe_vm_destroy(ibb->fd, ibb->vm_id);
> +}
> +
>  /**
>   * intel_bb_destroy:
>   * @ibb: pointer to intel_bb
> @@ -1262,8 +1268,8 @@ void intel_bb_destroy(struct intel_bb *ibb)
>  		close(ibb->fence);
>  	if (ibb->engine_syncobj)
>  		syncobj_destroy(ibb->fd, ibb->engine_syncobj);
> -	if (ibb->vm_id && !ibb->ctx)
> -		xe_vm_destroy(ibb->fd, ibb->vm_id);
> +	if (ibb->driver == INTEL_DRIVER_XE)
> +		__xe_bb_destroy(ibb);
>  
>  	free(ibb->batch);
>  	free(ibb->cfg);
> @@ -2298,7 +2304,9 @@ __xe_bb_exec(struct intel_bb *ibb, uint64_t flags, bool sync)
>  	igt_assert_eq(ibb->num_relocs, 0);
>  	igt_assert_eq(ibb->xe_bound, false);
>  
> -	if (ibb->last_engine != engine) {
> +	if (ibb->ctx) {
> +		engine_id = ibb->ctx;
> +	} else if (ibb->last_engine != engine) {
>  		struct drm_xe_engine_class_instance inst = { };
>  
>  		inst.engine_instance =
> @@ -2323,12 +2331,15 @@ __xe_bb_exec(struct intel_bb *ibb, uint64_t flags, bool sync)
>  		}
>  		igt_debug("Run on %s\n", xe_engine_class_string(inst.engine_class));
>  
> +		if (ibb->engine_id)
> +			xe_engine_destroy(ibb->fd, ibb->engine_id);
> +
>  		ibb->engine_id = engine_id =
>  			xe_engine_create(ibb->fd, ibb->vm_id, &inst, 0);
> +		ibb->last_engine = engine;
>  	} else {
>  		engine_id = ibb->engine_id;
>  	}
> -	ibb->last_engine = engine;
>  
>  	map = xe_bo_map(ibb->fd, ibb->handle, ibb->size);
>  	memcpy(map, ibb->batch, ibb->size);
> diff --git a/tests/xe/xe_intel_bb.c b/tests/xe/xe_intel_bb.c
> index 35d61608..4fcc3144 100644
> --- a/tests/xe/xe_intel_bb.c
> +++ b/tests/xe/xe_intel_bb.c
> @@ -177,6 +177,7 @@ static void simple_bb(struct buf_ops *bops, bool new_context)
>  	int xe = buf_ops_get_fd(bops);
>  	struct intel_bb *ibb;
>  	uint32_t ctx = 0;
> +	uint32_t vm = 0;
>  
>  	ibb = intel_bb_create_with_allocator(xe, ctx, NULL, PAGE_SIZE,
>  					     INTEL_ALLOCATOR_SIMPLE);
> @@ -195,7 +196,8 @@ static void simple_bb(struct buf_ops *bops, bool new_context)
>  	intel_bb_reset(ibb, true);
>  
>  	if (new_context) {
> -		ctx = xe_vm_create(xe, DRM_XE_VM_CREATE_ASYNC_BIND_OPS, 0);
> +		vm = xe_vm_create(xe, DRM_XE_VM_CREATE_ASYNC_BIND_OPS, 0);
> +		ctx = xe_engine_create(xe, vm, xe_hw_engine(xe, 0), 0);
>  		intel_bb_destroy(ibb);
>  		ibb = intel_bb_create_with_context(xe, ctx, NULL, PAGE_SIZE);
>  		intel_bb_out(ibb, MI_BATCH_BUFFER_END);
> @@ -203,7 +205,8 @@ static void simple_bb(struct buf_ops *bops, bool new_context)
>  		intel_bb_exec(ibb, intel_bb_offset(ibb),
>  			      I915_EXEC_DEFAULT | I915_EXEC_NO_RELOC,
>  			      true);
> -		xe_vm_destroy(xe, ctx);
> +		xe_engine_destroy(xe, ctx);
> +		xe_vm_destroy(xe, vm);
>  	}
>  
>  	intel_bb_destroy(ibb);