[PATCH] drm/amdgpu: fix drm leases being broken on radv

[Christian König]

Am 16.04.19 um 23:40 schrieb Andres Rodriguez:
> After a recent commit, access to the DRM_AUTH ioctls become more
> permissive. This resulted in a buggy check for drm_master capabilities
> inside radv stop working.
>
> This commit adds a backwards compatibility workaround so that the radv
> drm_master check keeps working as previously expected.
>
> This fixes SteamVR and other drm lease based apps being broken since
> v5.1-rc1
>
>  From the usermode side, radv will also be fixed to properly test for
> master capabilities:
> https://gitlab.freedesktop.org/mesa/mesa/merge_requests/669
>
> Fixes: 8059add0478e ("drm: allow render capable master with DRM_AUTH ioctls")
> Signed-off-by: Andres Rodriguez <andresx7 at gmail.com>
> Cc: Daniel Vetter <daniel at ffwll.ch>
> Cc: David Airlie <airlied at linux.ie>
> Cc: Emil Velikov <emil.velikov at collabora.com>
> Cc: Alex Deucher <alexander.deucher at amd.com>
> Cc: Keith Packard <keithp at keithp.com>
> Reviewed-by: Keith Packard <keithp at keithp.com>
> Reviewed-by: Daniel Vetter <daniel at ffwll.ch>

Well definitely a NAK. IIRC we have unit tests where the exactly first 
thing they do is querying AMDGPU_INFO_ACCEL_WORKING.

And I definitely not going to risk breaking those just to fix buggy 
behavior in radv.

Christian.

> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu.h     |  3 +++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |  2 ++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 22 ++++++++++++++++++++++
>   3 files changed, 27 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index 8d0d7f3dd5fb..e67bfee8d411 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -409,6 +409,9 @@ struct amdgpu_fpriv {
>   	struct mutex		bo_list_lock;
>   	struct idr		bo_list_handles;
>   	struct amdgpu_ctx_mgr	ctx_mgr;
> +
> +	/* Part of a backwards compatibility hack */
> +	bool			is_first_ioctl;
>   };
>   
>   int amdgpu_file_to_fpriv(struct file *filp, struct amdgpu_fpriv **fpriv);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 7419ea8a388b..cd438afa7092 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -1128,6 +1128,7 @@ long amdgpu_drm_ioctl(struct file *filp,
>   		      unsigned int cmd, unsigned long arg)
>   {
>   	struct drm_file *file_priv = filp->private_data;
> +	struct amdgpu_fpriv *fpriv = file_priv->driver_priv;
>   	struct drm_device *dev;
>   	long ret;
>   	dev = file_priv->minor->dev;
> @@ -1136,6 +1137,7 @@ long amdgpu_drm_ioctl(struct file *filp,
>   		return ret;
>   
>   	ret = drm_ioctl(filp, cmd, arg);
> +	fpriv->is_first_ioctl = false;
>   
>   	pm_runtime_mark_last_busy(dev->dev);
>   	pm_runtime_put_autosuspend(dev->dev);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> index e860412043bb..00c0a2fb2a69 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> @@ -455,6 +455,7 @@ static int amdgpu_hw_ip_info(struct amdgpu_device *adev,
>   static int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
>   {
>   	struct amdgpu_device *adev = dev->dev_private;
> +	struct amdgpu_fpriv *fpriv = filp->driver_priv;
>   	struct drm_amdgpu_info *info = data;
>   	struct amdgpu_mode_info *minfo = &adev->mode_info;
>   	void __user *out = (void __user *)(uintptr_t)info->return_pointer;
> @@ -470,6 +471,26 @@ static int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file
>   
>   	switch (info->query) {
>   	case AMDGPU_INFO_ACCEL_WORKING:
> +		/* HACK: radv has a fragile open-coded check for drm master
> +		 * The pattern for the call is:
> +		 *     open(DRM_NODE_PRIMARY)
> +		 *     drmModeCommandWrite( query=AMDGPU_INFO_ACCEL_WORKING )
> +		 *
> +		 * After commit 8059add04 this check stopped working due to
> +		 * operations on a primary node from non-master clients becoming
> +		 * more permissive.
> +		 *
> +		 * This backwards compatibility hack targets the calling pattern
> +		 * above to fail radv from thinking it has master access to the
> +		 * display system ( without reverting 8059add04 ).
> +		 *
> +		 * Users of libdrm will not be affected as some other ioctls are
> +		 * performed between open() and the AMDGPU_INFO_ACCEL_WORKING
> +		 * query.
> +		 */
> +		if (fpriv->is_first_ioctl && !filp->is_master)
> +			return -EACCES;
> +
>   		ui32 = adev->accel_working;
>   		return copy_to_user(out, &ui32, min(size, 4u)) ? -EFAULT : 0;
>   	case AMDGPU_INFO_CRTC_FROM_ID:
> @@ -987,6 +1008,7 @@ int amdgpu_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv)
>   			goto error_vm;
>   	}
>   
> +	fpriv->is_first_ioctl = true;
>   	mutex_init(&fpriv->bo_list_lock);
>   	idr_init(&fpriv->bo_list_handles);
>