Re: [PATCH v5 20/27] drm: Scope all DRM IOCTLs with drm_dev_enter/exit

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Apr 28, 2021 at 11:12:00AM -0400, Andrey Grodzovsky wrote:
> With this calling drm_dev_unplug will flush and block
> all in flight IOCTLs
> 
> Also, add feature such that if device supports graceful unplug
> we enclose entire IOCTL in SRCU critical section.
> 
> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>

Nope.

The idea of drm_dev_enter/exit is to mark up hw access. Not entire ioctl.

Especially not with an opt-in flag so that it could be shrugged of as a
driver hack. Most of these ioctls should have absolutely no problem
working after hotunplug.

Also, doing this defeats the point since it pretty much guarantees
userspace will die in assert()s and stuff. E.g. on i915 the rough contract
is that only execbuf (and even that only when userspace has indicated
support for non-recoverable hw ctx) is allowed to fail. Anything else
might crash userspace.

You probably need similar (and very precisely defined) rules for amdgpu.
And those must definitely exclude any shard ioctls from randomly failing
with EIO, because that just kills the box and defeats the point of trying
to gracefully handling hotunplug and making sure userspace has a chance of
survival. E.g. for atomic everything should continue, including flip
completion, but we set all outputs to "disconnected" and send out the
uevent. Maybe crtc enabling can fail too, but that can also be handled
through the async status we're using to signal DP link failures to
userspace.

I guess we should clarify this in the hotunplug doc?

Cheers, Daniel

> ---
>  drivers/gpu/drm/drm_ioctl.c | 15 +++++++++++++--
>  include/drm/drm_drv.h       |  6 ++++++
>  2 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index d273d1a8603a..5882ef2183bb 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -815,7 +815,7 @@ long drm_ioctl(struct file *filp,
>  	const struct drm_ioctl_desc *ioctl = NULL;
>  	drm_ioctl_t *func;
>  	unsigned int nr = DRM_IOCTL_NR(cmd);
> -	int retcode = -EINVAL;
> +	int idx, retcode = -EINVAL;
>  	char stack_kdata[128];
>  	char *kdata = NULL;
>  	unsigned int in_size, out_size, drv_size, ksize;
> @@ -884,7 +884,18 @@ long drm_ioctl(struct file *filp,
>  	if (ksize > in_size)
>  		memset(kdata + in_size, 0, ksize - in_size);
>  
> -	retcode = drm_ioctl_kernel(filp, func, kdata, ioctl->flags);
> +	if (drm_core_check_feature(dev, DRIVER_HOTUNPLUG_SUPPORT)) {
> +		if (drm_dev_enter(dev, &idx)) {
> +			retcode = drm_ioctl_kernel(filp, func, kdata, ioctl->flags);
> +			drm_dev_exit(idx);
> +		} else {
> +			retcode = -ENODEV;
> +			goto err_i1;
> +		}
> +	} else {
> +		retcode = drm_ioctl_kernel(filp, func, kdata, ioctl->flags);
> +	}
> +
>  	if (copy_to_user((void __user *)arg, kdata, out_size) != 0)
>  		retcode = -EFAULT;
>  
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index b439ae1921b8..63e05cec46c1 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -94,6 +94,12 @@ enum drm_driver_feature {
>  	 * synchronization of command submission.
>  	 */
>  	DRIVER_SYNCOBJ_TIMELINE         = BIT(6),
> +	/**
> +	 * @DRIVER_NO_HOTUNPLUG_SUPPORT:
> +	 *
> +	 * Driver support gracefull remove.
> +	 */
> +	DRIVER_HOTUNPLUG_SUPPORT         = BIT(7),
>  
>  	/* IMPORTANT: Below are all the legacy flags, add new ones above. */
>  
> -- 
> 2.25.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel