Re: [Intel-xe] [PATCH] drm/xe: Don't grab runtime PM ref in engine create IOCTL

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

On Mon, Apr 10, 2023 at 02:34:33PM -0700, Matthew Brost wrote:
> A VM had a runtime PM ref, a engine can't be created without a VM, and
> the engine holds a ref to the VM thus this is unnecessary. Beyond that
> taking a ref in the engine create IOCTL and dropping it in the destroy
> IOCTL is wrong as a user doesn't have to call the destroy IOCTL (e.g.
> they can just kill the process or close the driver FD). If a user does
> this PM refs are leaked.
> 
> Signed-off-by: Matthew Brost <matthew.brost@intel.com>

I could swear that I had already killed the pm ref from the engine!

we do only need in the vm...

I had previously faced this issue when we introduced the display...

it probably got lost in an old branch...

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

> ---
>  drivers/gpu/drm/xe/xe_engine.c | 43 ++++++++++------------------------
>  1 file changed, 13 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_engine.c b/drivers/gpu/drm/xe/xe_engine.c
> index 37209b13bcd6..094ec17d3004 100644
> --- a/drivers/gpu/drm/xe/xe_engine.c
> +++ b/drivers/gpu/drm/xe/xe_engine.c
> @@ -539,8 +539,6 @@ int xe_engine_create_ioctl(struct drm_device *dev, void *data,
>  	if (XE_IOCTL_ERR(xe, eci[0].gt_id >= xe->info.tile_count))
>  	       return -EINVAL;
>  
> -	xe_pm_runtime_get(xe);
> -
>  	if (eci[0].engine_class == DRM_XE_ENGINE_CLASS_VM_BIND) {
>  		for_each_gt(gt, xe, id) {
>  			struct xe_engine *new;
> @@ -552,16 +550,12 @@ int xe_engine_create_ioctl(struct drm_device *dev, void *data,
>  			logical_mask = bind_engine_logical_mask(xe, gt, eci,
>  								args->width,
>  								args->num_placements);
> -			if (XE_IOCTL_ERR(xe, !logical_mask)) {
> -				err = -EINVAL;
> -				goto put_rpm;
> -			}
> +			if (XE_IOCTL_ERR(xe, !logical_mask))
> +				return -EINVAL;
>  
>  			hwe = find_hw_engine(xe, eci[0]);
> -			if (XE_IOCTL_ERR(xe, !hwe)) {
> -				err = -EINVAL;
> -				goto put_rpm;
> -			}
> +			if (XE_IOCTL_ERR(xe, !hwe))
> +				return -EINVAL;
>  
>  			migrate_vm = xe_migrate_get_vm(gt->migrate);
>  			new = xe_engine_create(xe, migrate_vm, logical_mask,
> @@ -576,7 +570,7 @@ int xe_engine_create_ioctl(struct drm_device *dev, void *data,
>  				err = PTR_ERR(new);
>  				if (e)
>  					goto put_engine;
> -				goto put_rpm;
> +				return err;
>  			}
>  			if (id == 0)
>  				e = new;
> @@ -589,30 +583,22 @@ int xe_engine_create_ioctl(struct drm_device *dev, void *data,
>  		logical_mask = calc_validate_logical_mask(xe, gt, eci,
>  							  args->width,
>  							  args->num_placements);
> -		if (XE_IOCTL_ERR(xe, !logical_mask)) {
> -			err = -EINVAL;
> -			goto put_rpm;
> -		}
> +		if (XE_IOCTL_ERR(xe, !logical_mask))
> +			return -EINVAL;
>  
>  		hwe = find_hw_engine(xe, eci[0]);
> -		if (XE_IOCTL_ERR(xe, !hwe)) {
> -			err = -EINVAL;
> -			goto put_rpm;
> -		}
> +		if (XE_IOCTL_ERR(xe, !hwe))
> +			return -EINVAL;
>  
>  		vm = xe_vm_lookup(xef, args->vm_id);
> -		if (XE_IOCTL_ERR(xe, !vm)) {
> -			err = -ENOENT;
> -			goto put_rpm;
> -		}
> +		if (XE_IOCTL_ERR(xe, !vm))
> +			return -ENOENT;
>  
>  		e = xe_engine_create(xe, vm, logical_mask,
>  				     args->width, hwe, ENGINE_FLAG_PERSISTENT);
>  		xe_vm_put(vm);
> -		if (IS_ERR(e)) {
> -			err = PTR_ERR(e);
> -			goto put_rpm;
> -		}
> +		if (IS_ERR(e))
> +			return PTR_ERR(e);
>  	}
>  
>  	if (args->extensions) {
> @@ -642,8 +628,6 @@ int xe_engine_create_ioctl(struct drm_device *dev, void *data,
>  put_engine:
>  	xe_engine_kill(e);
>  	xe_engine_put(e);
> -put_rpm:
> -	xe_pm_runtime_put(xe);
>  	return err;
>  }
>  
> @@ -750,7 +734,6 @@ int xe_engine_destroy_ioctl(struct drm_device *dev, void *data,
>  
>  	trace_xe_engine_close(e);
>  	xe_engine_put(e);
> -	xe_pm_runtime_put(xe);
>  
>  	return 0;
>  }
> -- 
> 2.34.1
>