| Message ID | 20190905185318.31363-1-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/atomic: Take the atomic toys away from X | expand |
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256 On Thu, 2019-09-05 at 20:53 +0200, Daniel Vetter wrote: > The -modesetting ddx has a totally broken idea of how atomic works: > - doesn't disable old connectors, assuming they get auto-disable like > with the legacy setcrtc > - assumes ASYNC_FLIP is wired through for the atomic ioctl > - not a single call to TEST_ONLY > > Iow the implementation is a 1:1 translation of legacy ioctls to > atomic, which is a) broken b) pointless. > > We already have bugs in both i915 and amdgpu-DC where this prevents us > from enabling neat features. > > If anyone ever cares about atomic in X we can easily add a new atomic > level (req->value == 2) for X to get back the shiny toys. > > Since these broken versions of -modesetting have been shipping, > there's really no other way to get out of this bind. Hi Daniel and Greg (especially). It seems that this patch was never applied to stable, maybe it fell through the cracks? It doesn't apply as-is in 4.19 branch but a small change in the context makes it apply. I'm experiencing issues with lightdm and vt-switch in Debian Buster (which has a 4.19 kernel) so I'd appreciate if the patch was included in at least that release. Regards, - -- Yves-Alexis -----BEGIN PGP SIGNATURE----- iQEzBAEBCAAdFiEE8vi34Qgfo83x35gF3rYcyPpXRFsFAl61ITAACgkQ3rYcyPpX RFvlaAf9HZ0DTX1fAkNeNFoAgn4pFztnFq0fAwGj5iVIL4q6upE1wE3E8cDgUHeT maQQvL3YHFXjgzgDHYNIuUMipFE1Djymoy+EB4ZoOftqsJ4CPy4pCMUAh57u7BrV T+eBtj4n0wY0SgvoPism3QdbxY7CLLgCMJKLNrCPlkDCdJyGsZX9RIgfqvbkGM36 ftwBKcyy1iW5cAv10ehiXi/1zszA8bx2gULim3abcSjjz12ckNvBPy/BDvfFx19V 8cGgG3qD9PLmxRl80H1/mX30Ddw8Md5Fu7I/ndh3EGXLu8p8zod0rQVCQjAEW4X4 ew4tajDD2l9vWzN0sZIlyjq9fNgXBw== =lPBO -----END PGP SIGNATURE-----
On Fri, May 08, 2020 at 11:06:56AM +0200, Yves-Alexis Perez wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA256 > > On Thu, 2019-09-05 at 20:53 +0200, Daniel Vetter wrote: > > The -modesetting ddx has a totally broken idea of how atomic works: > > - doesn't disable old connectors, assuming they get auto-disable like > > with the legacy setcrtc > > - assumes ASYNC_FLIP is wired through for the atomic ioctl > > - not a single call to TEST_ONLY > > > > Iow the implementation is a 1:1 translation of legacy ioctls to > > atomic, which is a) broken b) pointless. > > > > We already have bugs in both i915 and amdgpu-DC where this prevents us > > from enabling neat features. > > > > If anyone ever cares about atomic in X we can easily add a new atomic > > level (req->value == 2) for X to get back the shiny toys. > > > > Since these broken versions of -modesetting have been shipping, > > there's really no other way to get out of this bind. > > Hi Daniel and Greg (especially). It seems that this patch was never applied to > stable, maybe it fell through the cracks? What patch is "this patch"? > It doesn't apply as-is in 4.19 branch but a small change in the context makes > it apply. I'm experiencing issues with lightdm and vt-switch in Debian Buster > (which has a 4.19 kernel) so I'd appreciate if the patch was included in at > least that release. What is the git commit id of the patch in Linus's tree? If you have a working backport, that makes it much easier than hoping I can fix it up... thanks, greg k-h
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256 On Fri, 2020-05-08 at 11:54 +0200, Greg KH wrote: > > Hi Daniel and Greg (especially). It seems that this patch was never > > applied to > > stable, maybe it fell through the cracks? > > What patch is "this patch"? Sorry, the patch was in the mail I was replying to: commit 26b1d3b527e7bf3e24b814d617866ac5199ce68d Author: Daniel Vetter <daniel.vetter@ffwll.ch> Date: Thu Sep 5 20:53:18 2019 +0200 drm/atomic: Take the atomic toys away from X > > > It doesn't apply as-is in 4.19 branch but a small change in the context > > makes > > it apply. I'm experiencing issues with lightdm and vt-switch in Debian > > Buster > > (which has a 4.19 kernel) so I'd appreciate if the patch was included in > > at > > least that release. > > What is the git commit id of the patch in Linus's tree? If you have a > working backport, that makes it much easier than hoping I can fix it > up... The commit id is in Linus tree is 26b1d3b527e7bf3e24b814d617866ac5199ce68d. To apply properly 69fdf4206a8ba91a277b3d50a3a05b71247635b2 would need to be cherry-picked as well but it wasn't marked for stable so I didn't bother and only fixed the context. Here's the backport to 4.19, compile and runtime tested. It does fix the issue for me (like it did on mainline). So I guess Tested-By: Yves-Alexis Perez <corsac@debian.org> commit 8a99914f7b539542622dc571c82d6cd203bddf64 Author: Daniel Vetter <daniel.vetter@ffwll.ch> Date: Thu Sep 5 20:53:18 2019 +0200 drm/atomic: Take the atomic toys away from X The -modesetting ddx has a totally broken idea of how atomic works: - doesn't disable old connectors, assuming they get auto-disable like with the legacy setcrtc - assumes ASYNC_FLIP is wired through for the atomic ioctl - not a single call to TEST_ONLY Iow the implementation is a 1:1 translation of legacy ioctls to atomic, which is a) broken b) pointless. We already have bugs in both i915 and amdgpu-DC where this prevents us from enabling neat features. If anyone ever cares about atomic in X we can easily add a new atomic level (req->value == 2) for X to get back the shiny toys. Since these broken versions of -modesetting have been shipping, there's really no other way to get out of this bind. v2: - add an informational dmesg output (Rob, Ajax) - reorder after the DRIVER_ATOMIC check to avoid useless noise (Ilia) - allow req->value > 2 so that X can do another attempt at atomic in the future v3: Go with paranoid, insist that the X should be first (suggested by Rob) Cc: Ilia Mirkin <imirkin@alum.mit.edu> References: https://gitlab.freedesktop.org/xorg/xserver/issues/629 References: https://gitlab.freedesktop.org/xorg/xserver/merge_requests/180 References: abbc0697d5fb ("drm/fb: revert the i915 Actually configure untiled displays from master") Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> (v1) Reviewed-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com> (v1) Cc: Michel Dänzer <michel@daenzer.net> Cc: Alex Deucher <alexdeucher@gmail.com> Cc: Adam Jackson <ajax@redhat.com> Acked-by: Adam Jackson <ajax@redhat.com> Cc: Sean Paul <sean@poorly.run> Cc: David Airlie <airlied@linux.ie> Cc: Rob Clark <robdclark@gmail.com> Acked-by: Rob Clark <robdclark@gmail.com> Cc: stable@vger.kernel.org Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Link: https://patchwork.freedesktop.org/patch/msgid/20190905185318.31363-1-daniel.vetter@ffwll.ch diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index ba129b64b61f..b92682f037b2 100644 - --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -321,7 +321,12 @@ drm_setclientcap(struct drm_device *dev, void *data, struct drm_file *file_priv) case DRM_CLIENT_CAP_ATOMIC: if (!drm_core_check_feature(dev, DRIVER_ATOMIC)) return -EINVAL; - - if (req->value > 1) + /* The modesetting DDX has a totally broken idea of atomic. */ + if (current->comm[0] == 'X' && req->value == 1) { + pr_info("broken atomic modeset userspace detected, disabling atomic\n"); + return -EOPNOTSUPP; + } + if (req->value > 2) return -EINVAL; file_priv->atomic = req->value; file_priv->universal_planes = req->value; - -- Yves-Alexis -----BEGIN PGP SIGNATURE----- iQEzBAEBCAAdFiEE8vi34Qgfo83x35gF3rYcyPpXRFsFAl61SZUACgkQ3rYcyPpX RFvPfwgAzMyFqiV592RBKu4tx5Ivqa4EC/1OdR8DojyQlw4AP0bYc+4O67xYbvt5 r4JXuGbSu+jW/29V+2t8ZlLi4S7bAvAo2DEhuBdGVzdmD4gM9EC+69oqVeZWWZTm VUldLrRO8BoPxv14lX/K/kU/o5Pv8ivRoEKs385JU2p1AxNGJI2UUmIXKGtk7Cu/ Fu/flH627RHjTRk2QYsemqHqSAONaHYuSiYm783hz4wYiPOZQdGvS+ifHwMAhUqh scaCxv+pBOxaLuZAfUXFjDX+qAcuJCxaP9bT4soweIpYqjzcAdBSmny0+OLOnie/ HcBzKwpgitKR/cVadZgb0US1oHeo2A== =l8z1 -----END PGP SIGNATURE-----
On Fri, May 08, 2020 at 01:59:17PM +0200, Yves-Alexis Perez wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA256 > > On Fri, 2020-05-08 at 11:54 +0200, Greg KH wrote: > > > Hi Daniel and Greg (especially). It seems that this patch was never > > > applied to > > > stable, maybe it fell through the cracks? > > > > What patch is "this patch"? > > Sorry, the patch was in the mail I was replying to: > > commit 26b1d3b527e7bf3e24b814d617866ac5199ce68d > Author: Daniel Vetter <daniel.vetter@ffwll.ch> > Date: Thu Sep 5 20:53:18 2019 +0200 > > drm/atomic: Take the atomic toys away from X > > > > > > It doesn't apply as-is in 4.19 branch but a small change in the context > > > makes > > > it apply. I'm experiencing issues with lightdm and vt-switch in Debian > > > Buster > > > (which has a 4.19 kernel) so I'd appreciate if the patch was included in > > > at > > > least that release. > > > > What is the git commit id of the patch in Linus's tree? If you have a > > working backport, that makes it much easier than hoping I can fix it > > up... > > The commit id is in Linus tree is 26b1d3b527e7bf3e24b814d617866ac5199ce68d. To > apply properly 69fdf4206a8ba91a277b3d50a3a05b71247635b2 would need to be > cherry-picked as well but it wasn't marked for stable so I didn't bother and > only fixed the context. Here's the backport to 4.19, compile and runtime > tested. It does fix the issue for me (like it did on mainline). > > So I guess > Tested-By: Yves-Alexis Perez <corsac@debian.org> > > commit 8a99914f7b539542622dc571c82d6cd203bddf64 > Author: Daniel Vetter <daniel.vetter@ffwll.ch> > Date: Thu Sep 5 20:53:18 2019 +0200 > > drm/atomic: Take the atomic toys away from X > > The -modesetting ddx has a totally broken idea of how atomic works: > - doesn't disable old connectors, assuming they get auto-disable like > with the legacy setcrtc > - assumes ASYNC_FLIP is wired through for the atomic ioctl > - not a single call to TEST_ONLY > > Iow the implementation is a 1:1 translation of legacy ioctls to > atomic, which is a) broken b) pointless. > > We already have bugs in both i915 and amdgpu-DC where this prevents us > from enabling neat features. > > If anyone ever cares about atomic in X we can easily add a new atomic > level (req->value == 2) for X to get back the shiny toys. > > Since these broken versions of -modesetting have been shipping, > there's really no other way to get out of this bind. > > v2: > - add an informational dmesg output (Rob, Ajax) > - reorder after the DRIVER_ATOMIC check to avoid useless noise (Ilia) > - allow req->value > 2 so that X can do another attempt at atomic in > the future > > v3: Go with paranoid, insist that the X should be first (suggested by > Rob) > > Cc: Ilia Mirkin <imirkin@alum.mit.edu> > References: https://gitlab.freedesktop.org/xorg/xserver/issues/629 > References: https://gitlab.freedesktop.org/xorg/xserver/merge_requests/180 > References: abbc0697d5fb ("drm/fb: revert the i915 Actually configure > untiled displays from master") > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> (v1) > Reviewed-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com> (v1) > Cc: Michel Dänzer <michel@daenzer.net> > Cc: Alex Deucher <alexdeucher@gmail.com> > Cc: Adam Jackson <ajax@redhat.com> > Acked-by: Adam Jackson <ajax@redhat.com> > Cc: Sean Paul <sean@poorly.run> > Cc: David Airlie <airlied@linux.ie> > Cc: Rob Clark <robdclark@gmail.com> > Acked-by: Rob Clark <robdclark@gmail.com> > Cc: stable@vger.kernel.org > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > Link: > https://patchwork.freedesktop.org/patch/msgid/20190905185318.31363-1-daniel.vetter@ffwll.ch > > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c > index ba129b64b61f..b92682f037b2 100644 > - --- a/drivers/gpu/drm/drm_ioctl.c > +++ b/drivers/gpu/drm/drm_ioctl.c > @@ -321,7 +321,12 @@ drm_setclientcap(struct drm_device *dev, void *data, > struct drm_file *file_priv) > case DRM_CLIENT_CAP_ATOMIC: > if (!drm_core_check_feature(dev, DRIVER_ATOMIC)) > return -EINVAL; > - - if (req->value > 1) > + /* The modesetting DDX has a totally broken idea of atomic. */ > + if (current->comm[0] == 'X' && req->value == 1) { > + pr_info("broken atomic modeset userspace detected, > disabling atomic\n"); > + return -EOPNOTSUPP; > + } > + if (req->value > 2) > return -EINVAL; > file_priv->atomic = req->value; > file_priv->universal_planes = req->value; > This is line-wrapped and can not be applied :( Ugh, let me see if I can do this by hand... greg k-h
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index 2c120c58f72d..1cd5cc492df1 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -336,7 +336,12 @@ drm_setclientcap(struct drm_device *dev, void *data, struct drm_file *file_priv) case DRM_CLIENT_CAP_ATOMIC: if (!drm_core_check_feature(dev, DRIVER_ATOMIC)) return -EOPNOTSUPP; - if (req->value > 1) + /* The modesetting DDX has a totally broken idea of atomic. */ + if (current->comm[0] == 'X' && req->value == 1) { + pr_info("broken atomic modeset userspace detected, disabling atomic\n"); + return -EOPNOTSUPP; + } + if (req->value > 2) return -EINVAL; file_priv->atomic = req->value; file_priv->universal_planes = req->value;
The -modesetting ddx has a totally broken idea of how atomic works: - doesn't disable old connectors, assuming they get auto-disable like with the legacy setcrtc - assumes ASYNC_FLIP is wired through for the atomic ioctl - not a single call to TEST_ONLY Iow the implementation is a 1:1 translation of legacy ioctls to atomic, which is a) broken b) pointless. We already have bugs in both i915 and amdgpu-DC where this prevents us from enabling neat features. If anyone ever cares about atomic in X we can easily add a new atomic level (req->value == 2) for X to get back the shiny toys. Since these broken versions of -modesetting have been shipping, there's really no other way to get out of this bind. v2: - add an informational dmesg output (Rob, Ajax) - reorder after the DRIVER_ATOMIC check to avoid useless noise (Ilia) - allow req->value > 2 so that X can do another attempt at atomic in the future v3: Go with paranoid, insist that the X should be first (suggested by Rob) Cc: Ilia Mirkin <imirkin@alum.mit.edu> References: https://gitlab.freedesktop.org/xorg/xserver/issues/629 References: https://gitlab.freedesktop.org/xorg/xserver/merge_requests/180 References: abbc0697d5fb ("drm/fb: revert the i915 Actually configure untiled displays from master") Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> (v1) Reviewed-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com> (v1) Cc: Michel Dänzer <michel@daenzer.net> Cc: Alex Deucher <alexdeucher@gmail.com> Cc: Adam Jackson <ajax@redhat.com> Acked-by: Adam Jackson <ajax@redhat.com> Cc: Sean Paul <sean@poorly.run> Cc: David Airlie <airlied@linux.ie> Cc: Rob Clark <robdclark@gmail.com> Acked-by: Rob Clark <robdclark@gmail.com> Cc: stable@vger.kernel.org Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/gpu/drm/drm_ioctl.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)