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

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

On Wed, Jul 26, 2023 at 10:23:49AM +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. The second issue is when destroying the actual
> engine, which requires GuC CT to deregister the context.
> 
> v2 (Rodrigo):
>   - Just use ENGINE_FLAG_VM as the indicator that we need to hold an rpm
>     ref. This also handles the case in xe_vm_create() where we create
>     default bind engines.
> 
> Closes: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/499
> Closes: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/504
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> ---
>  drivers/gpu/drm/xe/xe_engine.c       | 19 +++++++++++++++++++
>  drivers/gpu/drm/xe/xe_engine_types.h |  1 +
>  2 files changed, 20 insertions(+)
> 
> diff --git a/drivers/gpu/drm/xe/xe_engine.c b/drivers/gpu/drm/xe/xe_engine.c
> index 0102dad16e29..c30810a687b1 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_VM)
> +		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_VM)
> +		xe_device_mem_access_put(gt_to_xe(e->gt));
>  
>  	kfree(e);
>  }
> @@ -560,6 +573,9 @@ 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,
> @@ -568,6 +584,9 @@ int xe_engine_create_ioctl(struct drm_device *dev, void *data,
>  					       (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..7aa5d9ef7896 100644
> --- a/drivers/gpu/drm/xe/xe_engine_types.h
> +++ b/drivers/gpu/drm/xe/xe_engine_types.h
> @@ -56,6 +56,7 @@ struct xe_engine {
>  #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)
> -- 
> 2.41.0
>