On 2019-09-03 3:06 p.m., Daniel Vetter wrote:

It's the only flag anyone actually cares about. Plus if we're unlucky, the atomic ioctl might need a different flag for async flips. So better to abstract this away from the uapi a bit.

Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Michel Dänzer michel@daenzer.net Cc: Alex Deucher alexdeucher@gmail.com Cc: Adam Jackson ajax@redhat.com Cc: Sean Paul sean@poorly.run Cc: David Airlie airlied@linux.ie Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Maxime Ripard maxime.ripard@bootlin.com Cc: Daniel Vetter daniel@ffwll.ch Cc: Nicholas Kazlauskas nicholas.kazlauskas@amd.com Cc: Leo Li sunpeng.li@amd.com Cc: Harry Wentland harry.wentland@amd.com Cc: David Francis David.Francis@amd.com Cc: Mario Kleiner mario.kleiner.de@gmail.com Cc: Bhawanpreet Lakha Bhawanpreet.Lakha@amd.com Cc: Ben Skeggs bskeggs@redhat.com Cc: "Christian König" christian.koenig@amd.com Cc: Ilia Mirkin imirkin@alum.mit.edu Cc: Sam Ravnborg sam@ravnborg.org Cc: Chris Wilson chris@chris-wilson.co.uk

---

Series is:

Reviewed-by: Nicholas Kazlauskas nicholas.kazlauskas@amd.com

I would like to see a new flag eventually show up for atomic as well, but the existing one is effectively broken at this point and I would hope that no userspace is setting it expecting that it actually does something.

At this point we don't really gain anything from enabling atomic in DDX I think, most drivers already make use of DRM helpers to map these legacy IOCTLs to atomic anyway.

Nicholas Kazlauskas

drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 ++--- drivers/gpu/drm/drm_atomic_helper.c | 2 +- drivers/gpu/drm/drm_atomic_state_helper.c | 2 +- drivers/gpu/drm/nouveau/dispnv50/wndw.c | 4 ++-- include/drm/drm_crtc.h | 8 ++++---- 5 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c index 0a71ed1e7762..2f0ef0820f00 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -5756,8 +5756,7 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state, * change FB pitch, DCC state, rotation or mirroing. */ bundle->flip_addrs[planes_count].flip_immediate =

• 	(crtc->state->pageflip_flags &
  

• 	 DRM_MODE_PAGE_FLIP_ASYNC) != 0 &&
  

• 	crtc->state->async_flip &&
  acrtc_state->update_type == UPDATE_TYPE_FAST;
  
  

  timestamp_ns = ktime_get_ns();

@@ -6334,7 +6333,7 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state) amdgpu_dm_enable_crtc_interrupts(dev, state, true);

for_each_new_crtc_in_state(state, crtc, new_crtc_state, j)

• if (new_crtc_state->pageflip_flags & DRM_MODE_PAGE_FLIP_ASYNC)
  

• if (new_crtc_state->async_flip)
  wait_for_vblank = false;
  
  

  /* update planes when needed per crtc*/

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index e9c6112e7f73..1e5293eb66e3 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -3263,7 +3263,7 @@ static int page_flip_common(struct drm_atomic_state *state, return PTR_ERR(crtc_state);

crtc_state->event = event;

• crtc_state->pageflip_flags = flags;

• crtc_state->async_flip = flags & DRM_MODE_PAGE_FLIP_ASYNC;

  plane_state = drm_atomic_get_plane_state(state, plane); if (IS_ERR(plane_state))

diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c index 46dc264a248b..d0a937fb0c56 100644 --- a/drivers/gpu/drm/drm_atomic_state_helper.c +++ b/drivers/gpu/drm/drm_atomic_state_helper.c @@ -128,7 +128,7 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc, state->zpos_changed = false; state->commit = NULL; state->event = NULL;

• state->pageflip_flags = 0;

• state->async_flip = false;

  /* Self refresh should be canceled when a new update is available */ state->active = drm_atomic_crtc_effectively_active(state);

diff --git a/drivers/gpu/drm/nouveau/dispnv50/wndw.c b/drivers/gpu/drm/nouveau/dispnv50/wndw.c index 2db029371c91..5193b6257061 100644 --- a/drivers/gpu/drm/nouveau/dispnv50/wndw.c +++ b/drivers/gpu/drm/nouveau/dispnv50/wndw.c @@ -267,7 +267,7 @@ nv50_wndw_atomic_check_acquire(struct nv50_wndw *wndw, bool modeset, asyw->image.pitch[0] = fb->base.pitches[0]; }

• if (!(asyh->state.pageflip_flags & DRM_MODE_PAGE_FLIP_ASYNC))
  

• if (!asyh->state.async_flip)
  asyw->image.interval = 1;
  

  else asyw->image.interval = 0;

@@ -383,7 +383,7 @@ nv50_wndw_atomic_check_lut(struct nv50_wndw *wndw, }

/* Can't do an immediate flip while changing the LUT. */

• asyh->state.pageflip_flags &= ~DRM_MODE_PAGE_FLIP_ASYNC;

• asyh->state.async_flip = false; }

  static int

diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 7e2963cad543..900ae8d452b8 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -284,12 +284,12 @@ struct drm_crtc_state { u32 target_vblank;

/**

• * @pageflip_flags:
  

• * @async_flip:
  

• * DRM_MODE_PAGE_FLIP_* flags, as passed to the page flip ioctl.
  

• * Zero in any other case.
  

• * This is set when DRM_MODE_PAGE_FLIP_ASYNC is set in the legacy
  

• * PAGE_FLIP IOCTL. It's not wired up for the atomic IOCTL itself yet.
  

  */

• u32 pageflip_flags;

• bool async_flip;

  /**

  • @vrr_enabled:

On Thu, Sep 5, 2019 at 2:33 PM Mario Kleiner mario.kleiner.de@gmail.com wrote:

On Wed, Sep 4, 2019 at 2:57 PM Kazlauskas, Nicholas Nicholas.Kazlauskas@amd.com wrote:

...

On 2019-09-03 3:06 p.m., Daniel Vetter wrote:

...

It's the only flag anyone actually cares about. Plus if we're unlucky, the atomic ioctl might need a different flag for async flips. So better to abstract this away from the uapi a bit.

Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Michel Dänzer michel@daenzer.net Cc: Alex Deucher alexdeucher@gmail.com Cc: Adam Jackson ajax@redhat.com Cc: Sean Paul sean@poorly.run Cc: David Airlie airlied@linux.ie Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Maxime Ripard maxime.ripard@bootlin.com Cc: Daniel Vetter daniel@ffwll.ch Cc: Nicholas Kazlauskas nicholas.kazlauskas@amd.com Cc: Leo Li sunpeng.li@amd.com Cc: Harry Wentland harry.wentland@amd.com Cc: David Francis David.Francis@amd.com Cc: Mario Kleiner mario.kleiner.de@gmail.com Cc: Bhawanpreet Lakha Bhawanpreet.Lakha@amd.com Cc: Ben Skeggs bskeggs@redhat.com Cc: "Christian König" christian.koenig@amd.com Cc: Ilia Mirkin imirkin@alum.mit.edu Cc: Sam Ravnborg sam@ravnborg.org Cc: Chris Wilson chris@chris-wilson.co.uk

---

Series is:

Reviewed-by: Nicholas Kazlauskas nicholas.kazlauskas@amd.com

I would like to see a new flag eventually show up for atomic as well, but the existing one is effectively broken at this point and I would hope that no userspace is setting it expecting that it actually does something.

You mean it is generally broken? My software uses non-vsync'ed flips for diagnostic purpose and iirc some gpu + driver combo didn't work as expected anymore. But i thought that was one specific driver bug (maybe on AMD + DC)?

atomic ioctl + FLIP_ASYNC does not do anything special. FLIP_ASYNC with the legacy page_flip ioctl works correctly. This is in the core ioctl handler (the flag simply did nothing for atomic), so irrespective of whether the driver has a bug or not with FLIP_ASYNC.

...

At this point we don't really gain anything from enabling atomic in DDX I think, most drivers already make use of DRM helpers to map these legacy IOCTLs to atomic anyway.

One thing i wanted to try, once i hopefully find some time in late 2019 / early 2020 (if nobody else starts working on such a thing earlier), would be to add the ability to pass in a target flip time to the pageflip ioctl for use with VRR. For that i thought adding a new pageflip flag a la DRM_MODE_PAGE_FLIP_TARGETTIME) would be a good way to reuse the existing page_flip_target ioctl and redefine the "uint32 sequence" field of struct drm_mode_crtc_page_flip_target to pass in the target time - or at least the lower 32 bits of a target time.

Yeah target time (instead of just target vblank that we currently have) has been discussed before. It's just that no one yet typed an implementation (driver and userspace and all that).

So that would be one more page flip flag for the future. I'd like this to be workable from X11, and the current DDX don't use the atomic interface, apart from the modesetting DDX where it just got disabled by default in xserver master due to various unresolved bugs afaik?

It was a bit too simplistic a conversion. But for specific features (like doing a page flip with atomic instead of the legacy page_flip ioctl because you need that one new feature) you can just use the atomic ioctl - legacy ioctl and atomic can be mixed like that, because underneath it's all atomic anyway.

The trouble with the modesetting atomic is that atomic isn't a simple 1:1 translation of the old ioctls, and the implementation that landed was simply broken. And no one has put in the work to make it solid, so it got backed out. -Daniel

thanks, -mario

...

Nicholas Kazlauskas

...

drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 ++--- drivers/gpu/drm/drm_atomic_helper.c | 2 +- drivers/gpu/drm/drm_atomic_state_helper.c | 2 +- drivers/gpu/drm/nouveau/dispnv50/wndw.c | 4 ++-- include/drm/drm_crtc.h | 8 ++++---- 5 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c index 0a71ed1e7762..2f0ef0820f00 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -5756,8 +5756,7 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state, * change FB pitch, DCC state, rotation or mirroing. */ bundle->flip_addrs[planes_count].flip_immediate =

•                 (crtc->state->pageflip_flags &
  

•                  DRM_MODE_PAGE_FLIP_ASYNC) != 0 &&
  

•                 crtc->state->async_flip &&
                  acrtc_state->update_type == UPDATE_TYPE_FAST;
  
          timestamp_ns = ktime_get_ns();
  

@@ -6334,7 +6333,7 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state) amdgpu_dm_enable_crtc_interrupts(dev, state, true);

  for_each_new_crtc_in_state(state, crtc, new_crtc_state, j)

•         if (new_crtc_state->pageflip_flags & DRM_MODE_PAGE_FLIP_ASYNC)
  

•         if (new_crtc_state->async_flip)
                  wait_for_vblank = false;
  
  /* update planes when needed per crtc*/
  

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index e9c6112e7f73..1e5293eb66e3 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -3263,7 +3263,7 @@ static int page_flip_common(struct drm_atomic_state *state, return PTR_ERR(crtc_state);

  crtc_state->event = event;

• crtc_state->pageflip_flags = flags;
  

• crtc_state->async_flip = flags & DRM_MODE_PAGE_FLIP_ASYNC;
  
  plane_state = drm_atomic_get_plane_state(state, plane);
  if (IS_ERR(plane_state))
  

diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c index 46dc264a248b..d0a937fb0c56 100644 --- a/drivers/gpu/drm/drm_atomic_state_helper.c +++ b/drivers/gpu/drm/drm_atomic_state_helper.c @@ -128,7 +128,7 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc, state->zpos_changed = false; state->commit = NULL; state->event = NULL;

• state->pageflip_flags = 0;
  

• state->async_flip = false;
  
  /* Self refresh should be canceled when a new update is available */
  state->active = drm_atomic_crtc_effectively_active(state);
  

diff --git a/drivers/gpu/drm/nouveau/dispnv50/wndw.c b/drivers/gpu/drm/nouveau/dispnv50/wndw.c index 2db029371c91..5193b6257061 100644 --- a/drivers/gpu/drm/nouveau/dispnv50/wndw.c +++ b/drivers/gpu/drm/nouveau/dispnv50/wndw.c @@ -267,7 +267,7 @@ nv50_wndw_atomic_check_acquire(struct nv50_wndw *wndw, bool modeset, asyw->image.pitch[0] = fb->base.pitches[0]; }

•         if (!(asyh->state.pageflip_flags & DRM_MODE_PAGE_FLIP_ASYNC))
  

•         if (!asyh->state.async_flip)
                  asyw->image.interval = 1;
          else
                  asyw->image.interval = 0;
  

@@ -383,7 +383,7 @@ nv50_wndw_atomic_check_lut(struct nv50_wndw *wndw, }

  /* Can't do an immediate flip while changing the LUT. */

• asyh->state.pageflip_flags &= ~DRM_MODE_PAGE_FLIP_ASYNC;
  

• asyh->state.async_flip = false;
  

  }

  static int

diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 7e2963cad543..900ae8d452b8 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -284,12 +284,12 @@ struct drm_crtc_state { u32 target_vblank;

  /**

•  * @pageflip_flags:
  

•  * @async_flip:
   *
  

•  * DRM_MODE_PAGE_FLIP_* flags, as passed to the page flip ioctl.
  

•  * Zero in any other case.
  

•  * This is set when DRM_MODE_PAGE_FLIP_ASYNC is set in the legacy
  

•  * PAGE_FLIP IOCTL. It's not wired up for the atomic IOCTL itself yet.
   */
  

• u32 pageflip_flags;
  

• bool async_flip;
  
  /**
   * @vrr_enabled:
  
  

On Thu, Sep 5, 2019 at 4:19 PM Maarten Lankhorst maarten.lankhorst@linux.intel.com 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 maarten.lankhorst@linux.intel.com Cc: Michel Dänzer michel@daenzer.net Cc: Alex Deucher alexdeucher@gmail.com Cc: Adam Jackson ajax@redhat.com Cc: Sean Paul sean@poorly.run Cc: David Airlie airlied@linux.ie Cc: stable@vger.kernel.org Signed-off-by: Daniel Vetter daniel.vetter@intel.com

---

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 airlied@redhat.com 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 <airlied@redhat.com>

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 maarten.lankhorst@linux.intel.com

Thanks, Daniel

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....

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