Re: [Intel-xe] [PATCH] drm/xe/engine: add missing rpm for bind engines

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Tue, Jul 25, 2023 at 12:01:17PM +0100, Matthew Auld wrote:
> Bind engines need to use the migration vm, however we don't have any rpm
> for such a vm, otherwise the kernel would prevent rpm suspend-resume.
> There are two issues here, first is the actual engine create which needs
> to touch the lrc, but since that is in VRAM we trigger loads of missing
> mem_access asserts. 

with this in mind, should we really create the new ENGINE_FLAG_HOLD_RPM,
or should we simply use the existent ENGINE_FLAG_VM?

> The second issue is when destroying the actual
> engine, which requires GuC CT to deregister the context.
> Closes: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/499

I wonder if we should get the mem_acceess from inside the ct path
to be sure?

> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_engine.c       | 20 ++++++++++++++++++++
>  drivers/gpu/drm/xe/xe_engine_types.h |  1 +
>  2 files changed, 21 insertions(+)
> 
> diff --git a/drivers/gpu/drm/xe/xe_engine.c b/drivers/gpu/drm/xe/xe_engine.c
> index 59e0a9e085ba..dba71f53e53e 100644
> --- a/drivers/gpu/drm/xe/xe_engine.c
> +++ b/drivers/gpu/drm/xe/xe_engine.c
> @@ -76,6 +76,17 @@ static struct xe_engine *__xe_engine_create(struct xe_device *xe,
>  	if (err)
>  		goto err_lrc;
>  
> +	/*
> +	 * Normally the user vm holds an rpm ref to keep the device awake, and
> +	 * the context holds a ref for the vm, however for some engines we use
> +	 * the kernels migrate vm underneath which offers no such rpm ref. Make
> +	 * sure we keep a ref here, so we can perform GuC CT actions when
> +	 * needed. Caller is expected to have already grabbed the rpm ref
> +	 * outside any sensitive locks.
> +	 */
> +	if (e->flags & ENGINE_FLAG_HOLD_RPM)
> +		drm_WARN_ON(&xe->drm, !xe_device_mem_access_get_if_ongoing(xe));
> +
>  	return e;
>  
>  err_lrc:
> @@ -152,6 +163,8 @@ void xe_engine_fini(struct xe_engine *e)
>  		xe_lrc_finish(e->lrc + i);
>  	if (e->vm)
>  		xe_vm_put(e->vm);
> +	if (e->flags & ENGINE_FLAG_HOLD_RPM)
> +		xe_device_mem_access_put(gt_to_xe(e->gt));
>  
>  	kfree(e);
>  }
> @@ -560,14 +573,21 @@ int xe_engine_create_ioctl(struct drm_device *dev, void *data,
>  			if (XE_IOCTL_DBG(xe, !hwe))
>  				return -EINVAL;
>  
> +			/* The migration vm doesn't hold rpm ref */
> +			xe_device_mem_access_get(xe);
> +
>  			migrate_vm = xe_migrate_get_vm(gt_to_tile(gt)->migrate);
>  			new = xe_engine_create(xe, migrate_vm, logical_mask,
>  					       args->width, hwe,
> +					       ENGINE_FLAG_HOLD_RPM |
>  					       ENGINE_FLAG_PERSISTENT |
>  					       ENGINE_FLAG_VM |
>  					       (id ?
>  					       ENGINE_FLAG_BIND_ENGINE_CHILD :
>  					       0));
> +
> +			xe_device_mem_access_put(xe); /* now held by engine */
> +
>  			xe_vm_put(migrate_vm);
>  			if (IS_ERR(new)) {
>  				err = PTR_ERR(new);
> diff --git a/drivers/gpu/drm/xe/xe_engine_types.h b/drivers/gpu/drm/xe/xe_engine_types.h
> index 36bfaeec23f4..a3867e4db0bb 100644
> --- a/drivers/gpu/drm/xe/xe_engine_types.h
> +++ b/drivers/gpu/drm/xe/xe_engine_types.h
> @@ -59,6 +59,7 @@ struct xe_engine {
>  #define ENGINE_FLAG_VM			BIT(4)
>  #define ENGINE_FLAG_BIND_ENGINE_CHILD	BIT(5)
>  #define ENGINE_FLAG_WA			BIT(6)
> +#define ENGINE_FLAG_HOLD_RPM		BIT(7)
>  
>  	/**
>  	 * @flags: flags for this engine, should statically setup aside from ban
> -- 
> 2.41.0
>