[1/3] drm/atomic: Take the atomic toys away from X

Commit Message

Daniel Vetter Sept. 3, 2019, 7:06 p.m. UTC

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.

References: https://gitlab.freedesktop.org/xorg/xserver/issues/629
References: https://gitlab.freedesktop.org/xorg/xserver/merge_requests/180
Cc: Maarten Lankhorst <[e-mail]>
Cc: Michel Dänzer <[e-mail]>
Cc: Alex Deucher <[e-mail]>
Cc: Adam Jackson <[e-mail]>
Cc: Sean Paul <[e-mail]>
Cc: David Airlie <[e-mail]>
Cc: [e-mail]
Signed-off-by: Daniel Vetter <[e-mail]>
---
 drivers/gpu/drm/drm_ioctl.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Maarten Lankhorst Sept. 5, 2019, 2:19 p.m. UTC | #1

Op 03-09-2019 om 21:06 schreef Daniel Vetter:
> 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.
>
> References: https://gitlab.freedesktop.org/xorg/xserver/issues/629
> References: https://gitlab.freedesktop.org/xorg/xserver/merge_requests/180
> Cc: Maarten Lankhorst <[e-mail]>
> Cc: Michel Dänzer <[e-mail]>
> Cc: Alex Deucher <[e-mail]>
> Cc: Adam Jackson <[e-mail]>
> Cc: Sean Paul <[e-mail]>
> Cc: David Airlie <[e-mail]>
> Cc: [e-mail]
> Signed-off-by: Daniel Vetter <[e-mail]>
> ---
>  drivers/gpu/drm/drm_ioctl.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index 2c120c58f72d..1cb7b4c3c87c 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -334,6 +334,9 @@ drm_setclientcap(struct drm_device *dev, void *data, struct drm_file *file_priv)
>  		file_priv->universal_planes = req->value;
>  		break;
>  	case DRM_CLIENT_CAP_ATOMIC:
> +		/* The modesetting DDX has a totally broken idea of atomic. */
> +		if (strstr(current->comm, "X"))
> +			return -EOPNOTSUPP;
>  		if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
>  			return -EOPNOTSUPP;
>  		if (req->value > 1)

Good riddance!

Missing one more:
commit abbc0697d5fbf53f74ce0bcbe936670199764cfa
Author: Dave Airlie <[e-mail]>
Date:   Wed Apr 24 16:33:29 2019 +1000

    drm/fb: revert the i915 Actually configure untiled displays from master
   
    This code moved in here in master, so revert it the same way.
   
    This is the same revert as 9fa246256e09 ("Revert "drm/i915/fbdev:
    Actually configure untiled displays"") in drm-fixes.
   
    Signed-off-by: Dave Airlie <[e-mail]>

Can we unrevert that now?

With that fixed, on the whole series:

Reviewed-by: Maarten Lankhorst <[e-mail]>

Daniel Vetter Sept. 5, 2019, 2:25 p.m. UTC | #2

On Thu, Sep 5, 2019 at 4:19 PM Maarten Lankhorst
<[e-mail]> wrote:
>
> Op 03-09-2019 om 21:06 schreef Daniel Vetter:
> > 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.
> >
> > References: https://gitlab.freedesktop.org/xorg/xserver/issues/629
> > References: https://gitlab.freedesktop.org/xorg/xserver/merge_requests/180
> > Cc: Maarten Lankhorst <[e-mail]>
> > Cc: Michel Dänzer <[e-mail]>
> > Cc: Alex Deucher <[e-mail]>
> > Cc: Adam Jackson <[e-mail]>
> > Cc: Sean Paul <[e-mail]>
> > Cc: David Airlie <[e-mail]>
> > Cc: [e-mail]
> > Signed-off-by: Daniel Vetter <[e-mail]>
> > ---
> >  drivers/gpu/drm/drm_ioctl.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> > index 2c120c58f72d..1cb7b4c3c87c 100644
> > --- a/drivers/gpu/drm/drm_ioctl.c
> > +++ b/drivers/gpu/drm/drm_ioctl.c
> > @@ -334,6 +334,9 @@ drm_setclientcap(struct drm_device *dev, void *data, struct drm_file *file_priv)
> >               file_priv->universal_planes = req->value;
> >               break;
> >       case DRM_CLIENT_CAP_ATOMIC:
> > +             /* The modesetting DDX has a totally broken idea of atomic. */
> > +             if (strstr(current->comm, "X"))
> > +                     return -EOPNOTSUPP;
> >               if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
> >                       return -EOPNOTSUPP;
> >               if (req->value > 1)
>
> Good riddance!
>
> Missing one more:
> commit abbc0697d5fbf53f74ce0bcbe936670199764cfa
> Author: Dave Airlie <[e-mail]>
> Date:   Wed Apr 24 16:33:29 2019 +1000
>
>     drm/fb: revert the i915 Actually configure untiled displays from master
>
>     This code moved in here in master, so revert it the same way.
>
>     This is the same revert as 9fa246256e09 ("Revert "drm/i915/fbdev:
>     Actually configure untiled displays"") in drm-fixes.
>
>     Signed-off-by: Dave Airlie <[e-mail]>
>
> Can we unrevert that now?

My idea is to land this in drm-misc-fixes first (or maybe
drm-misc-next-fixes). And then we can land the revert of the revert
once that's backmerged into drm-intel. -fixes since this one here is
cc: stable.

And yes I'll add a reference to that one in the commit message when merging.

> With that fixed, on the whole series:
>
> Reviewed-by: Maarten Lankhorst <[e-mail]>

Thanks, Daniel

Rob Clark Sept. 5, 2019, 5:20 p.m. UTC | #3

On Tue, Sep 3, 2019 at 12:07 PM Daniel Vetter <[e-mail]> 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.
>
> References: https://gitlab.freedesktop.org/xorg/xserver/issues/629
> References: https://gitlab.freedesktop.org/xorg/xserver/merge_requests/180
> Cc: Maarten Lankhorst <[e-mail]>
> Cc: Michel Dänzer <[e-mail]>
> Cc: Alex Deucher <[e-mail]>
> Cc: Adam Jackson <[e-mail]>
> Cc: Sean Paul <[e-mail]>
> Cc: David Airlie <[e-mail]>
> Cc: [e-mail]
> Signed-off-by: Daniel Vetter <[e-mail]>
> ---
>  drivers/gpu/drm/drm_ioctl.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index 2c120c58f72d..1cb7b4c3c87c 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -334,6 +334,9 @@ drm_setclientcap(struct drm_device *dev, void *data, struct drm_file *file_priv)
>                 file_priv->universal_planes = req->value;
>                 break;
>         case DRM_CLIENT_CAP_ATOMIC:
> +               /* The modesetting DDX has a totally broken idea of atomic. */
> +               if (strstr(current->comm, "X"))
> +                       return -EOPNOTSUPP;

Seems like we can be a bit more targeted than "anything that has 'X'
in the name".. at a minimum restrict things to "starts with 'X'" seems
saner.  But I guess we could probably somehow look at the processes
memory map and look for modesetting_drv.so.

BR,
-R

>                 if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
>                         return -EOPNOTSUPP;
>                 if (req->value > 1)
> --
> 2.23.0
>
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Patch

diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 2c120c58f72d..1cb7b4c3c87c 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -334,6 +334,9 @@  drm_setclientcap(struct drm_device *dev, void *data, struct drm_file *file_priv)
 		file_priv->universal_planes = req->value;
 		break;
 	case DRM_CLIENT_CAP_ATOMIC:
+		/* The modesetting DDX has a totally broken idea of atomic. */
+		if (strstr(current->comm, "X"))
+			return -EOPNOTSUPP;
 		if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
 			return -EOPNOTSUPP;
 		if (req->value > 1)