[Intel-xe] [PATCH 2/3] drm/xe: split kernel vs permanent engine flags

[Matthew Brost]

On Thu, Jul 27, 2023 at 11:59:44AM -0700, Daniele Ceraolo Spurio wrote:
> If an engine is only destroyed on driver unload, we can skip its
> clean-up steps with the GuC because the GuC is going to be tuned off as
> well, so it doesn't matter if we're in sync with it or not. Currently,
> we apply this optimization to all engines marked as kernel, but this
> stops us to supporting kernel engines that don't stick around until
> unload. To remove this limitation, add a separate flag to indicate if
> the engine is expected to only be destryed on driver unload and use that
> to trigger the optimization.
> 
> While at it, add a small comment to explain what each engine flag
> represents.
> 
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> Cc: Matthew Brost <matthew.brost at intel.com>
> ---
>  drivers/gpu/drm/xe/xe_engine.c       |  3 +++
>  drivers/gpu/drm/xe/xe_engine_types.h | 20 ++++++++++++++------
>  drivers/gpu/drm/xe/xe_guc_submit.c   | 12 ++++++------
>  drivers/gpu/drm/xe/xe_migrate.c      |  4 ++--
>  4 files changed, 25 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_engine.c b/drivers/gpu/drm/xe/xe_engine.c
> index c30810a687b1..a08001f85e39 100644
> --- a/drivers/gpu/drm/xe/xe_engine.c
> +++ b/drivers/gpu/drm/xe/xe_engine.c
> @@ -33,6 +33,9 @@ static struct xe_engine *__xe_engine_create(struct xe_device *xe,
>  	int err;
>  	int i;
>  
> +	/* only kernel engines can be permanent */
> +	XE_BUG_ON((flags & ENGINE_FLAG_PERMANENT) && !(flags & ENGINE_FLAG_KERNEL));

Same as last patch, use XE_WARN_ON here.

> +
>  	e = kzalloc(sizeof(*e) + sizeof(struct xe_lrc) * width, GFP_KERNEL);
>  	if (!e)
>  		return ERR_PTR(-ENOMEM);
> diff --git a/drivers/gpu/drm/xe/xe_engine_types.h b/drivers/gpu/drm/xe/xe_engine_types.h
> index 7aa5d9ef7896..6802fdc10ec8 100644
> --- a/drivers/gpu/drm/xe/xe_engine_types.h
> +++ b/drivers/gpu/drm/xe/xe_engine_types.h
> @@ -52,14 +52,22 @@ struct xe_engine {
>  	/** @fence_irq: fence IRQ used to signal job completion */
>  	struct xe_hw_fence_irq *fence_irq;
>  
> +/* engine no longer allowed to submit */
>  #define ENGINE_FLAG_BANNED		BIT(0)
> +/* engine used for kernel submission only */
>  #define ENGINE_FLAG_KERNEL		BIT(1)
> -#define ENGINE_FLAG_PERSISTENT		BIT(2)
> -#define ENGINE_FLAG_COMPUTE_MODE	BIT(3)
> -/* Caller needs to hold rpm ref when creating engine with ENGINE_FLAG_VM */
> -#define ENGINE_FLAG_VM			BIT(4)
> -#define ENGINE_FLAG_BIND_ENGINE_CHILD	BIT(5)
> -#define ENGINE_FLAG_WA			BIT(6)
> +/* kernel engine only destroyed at driver unload */
> +#define ENGINE_FLAG_PERMANENT		BIT(2)
> +/* engine keeps running pending jobs after destroy ioctl */
> +#define ENGINE_FLAG_PERSISTENT		BIT(3)
> +/* engine for use with compute VMs */
> +#define ENGINE_FLAG_COMPUTE_MODE	BIT(4)
> +/* for VM jobs. Caller needs to hold rpm ref when creating engine with this flag */
> +#define ENGINE_FLAG_VM			BIT(5)
> +/* child of VM engine for multi-tile VM jobs */
> +#define ENGINE_FLAG_BIND_ENGINE_CHILD	BIT(6)
> +/* engine used for WA setup */
> +#define ENGINE_FLAG_WA			BIT(7)
>  
>  	/**
>  	 * @flags: flags for this engine, should statically setup aside from ban
> diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
> index c5b265624344..6893a19e6676 100644
> --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> @@ -943,7 +943,7 @@ static void __guc_engine_fini_async(struct work_struct *w)
>  	drm_sched_entity_fini(&ge->entity);
>  	drm_sched_fini(&ge->sched);
>  
> -	if (!(e->flags & ENGINE_FLAG_KERNEL)) {
> +	if (!(e->flags & ENGINE_FLAG_PERMANENT)) {

This will conflict with one my patches in flight [1] but easy rebase
either way.

[1] https://patchwork.freedesktop.org/patch/549741/?series=121464&rev=1

With the s/BUG/WARN/:
Reviewed-by: Matthew Brost <matthew.brost at intel.com>

>  		kfree(ge);
>  		xe_engine_fini(e);
>  	}
> @@ -951,13 +951,13 @@ static void __guc_engine_fini_async(struct work_struct *w)
>  
>  static void guc_engine_fini_async(struct xe_engine *e)
>  {
> -	bool kernel = e->flags & ENGINE_FLAG_KERNEL;
> +	bool permanent = e->flags & ENGINE_FLAG_PERMANENT;
>  
>  	INIT_WORK(&e->guc->fini_async, __guc_engine_fini_async);
>  	queue_work(system_wq, &e->guc->fini_async);
>  
> -	/* We must block on kernel engines so slabs are empty on driver unload */
> -	if (kernel) {
> +	/* We must block on permanent kernel engines so slabs are empty on driver unload */
> +	if (permanent) {
>  		struct xe_guc_engine *ge = e->guc;
>  
>  		flush_work(&ge->fini_async);
> @@ -983,7 +983,7 @@ static void __guc_engine_process_msg_cleanup(struct drm_sched_msg *msg)
>  	struct xe_engine *e = msg->private_data;
>  	struct xe_guc *guc = engine_to_guc(e);
>  
> -	XE_BUG_ON(e->flags & ENGINE_FLAG_KERNEL);
> +	XE_BUG_ON(e->flags & ENGINE_FLAG_PERMANENT);
>  	trace_xe_engine_cleanup_entity(e);
>  
>  	if (engine_registered(e))
> @@ -1218,7 +1218,7 @@ static void guc_engine_fini(struct xe_engine *e)
>  {
>  	struct drm_sched_msg *msg = e->guc->static_msgs + STATIC_MSG_CLEANUP;
>  
> -	if (!(e->flags & ENGINE_FLAG_KERNEL))
> +	if (!(e->flags & ENGINE_FLAG_PERMANENT))
>  		guc_engine_add_msg(e, msg, CLEANUP);
>  	else
>  		__guc_engine_fini(engine_to_guc(e), e);
> diff --git a/drivers/gpu/drm/xe/xe_migrate.c b/drivers/gpu/drm/xe/xe_migrate.c
> index aad76a6a8094..efaf35cd8963 100644
> --- a/drivers/gpu/drm/xe/xe_migrate.c
> +++ b/drivers/gpu/drm/xe/xe_migrate.c
> @@ -343,11 +343,11 @@ struct xe_migrate *xe_migrate_init(struct xe_tile *tile)
>  
>  		m->eng = xe_engine_create(xe, vm,
>  					  BIT(hwe->logical_instance), 1,
> -					  hwe, ENGINE_FLAG_KERNEL);
> +					  hwe, ENGINE_FLAG_KERNEL | ENGINE_FLAG_PERMANENT);
>  	} else {
>  		m->eng = xe_engine_create_class(xe, primary_gt, vm,
>  						XE_ENGINE_CLASS_COPY,
> -						ENGINE_FLAG_KERNEL);
> +						ENGINE_FLAG_KERNEL | ENGINE_FLAG_PERMANENT);
>  	}
>  	if (IS_ERR(m->eng)) {
>  		xe_vm_close_and_put(vm);
> -- 
> 2.41.0
>