On Wed, Apr 17, 2019 at 09:09:27AM +0200, Christian König wrote:
> 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 <andre...@gmail.com>
> > Cc: Daniel Vetter <dan...@ffwll.ch>
> > Cc: David Airlie <airl...@linux.ie>
> > Cc: Emil Velikov <emil.veli...@collabora.com>
> > Cc: Alex Deucher <alexander.deuc...@amd.com>
> > Cc: Keith Packard <kei...@keithp.com>
> > Reviewed-by: Keith Packard <kei...@keithp.com>
> > Reviewed-by: Daniel Vetter <dan...@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.
s/buggy/fragile :-)
Option B would be to disable 8059add0478e ("drm: allow render capable
master with DRM_AUTH ioctls") just for amdgpu.ko, but that's a bit more
tricky to implement I guess (since it'd leak all over the place).
Option C is to revert 8059add04, but that's a bit silly since it seems to
work everywhere.
Breaking radv isn't an option, because no regression. Aside: No one is
stopping amd folks from reviewing radv patches and making sure there's no
fragile stuff in there.
We discussed this quite a bit on irc with Ben and Keith and others, and
figured option A is the most promising to go forward with. Anything using
amdgpu_device_init (which I think are all the umds, but I didn't check)
will keep working, as will radv leases/vkdisplay, plus we can keep
8059add04 for everyone (not just everony except amdgpu). If that means
breaking a few unit tests ... *shrugh*. Needs patches to fix them ofc, but
shouldn't be that much work really.
-Daniel
>
> 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);
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
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
