drm/atomic-helper: Again check modeset *before* plane states

This essentially reverts

commit 934ce1c23624526d9d784e0499190bb48113e6f4
Author: Rob Clark <robdclark@gmail.com>
Date:   Wed Nov 19 16:41:33 2014 -0500

    drm/atomic: check mode_changed *after* atomic_check

Depending upon the driver both orders (or maybe even interleaving) is
required:
- If ->atomic_check updates ->mode_changed then helper_check_modeset
  must be run afters.
- If ->atomic_check depends upon accurate adjusted dotclock values for
  e.g. watermarks, then helper_check_modeset must be run first.

The failure mode in the first case is usually a totally angry hw
because the pixel format switching doesn't happen. The failure mode in
the later case is usually nothing, since in most cases the old
adjusted mode from the previous modeset wont be too far off to be a
problem. So just underruns and perhaps even just suboptimal (from a
power consumption) watermarks.

Furthermore in the transitional helpers we only call ->atomic_check
after the new modeset state has been fully set up (and hence
computed).

Given that asymmetry in expected failure modes I think it's safer to
go back to the older order. So do that and give msm a special check
function to compensate.

Also update kerneldoc to explain this a bit.

v2: Actually add the missing hunk Rob spotted.

v3: Move msm_atomic_check into msm_atomic.c, requested by Rob.

Cc: Rob Clark <robdclark@gmail.com>
Reviewed-by: Rob Clark <robdclark@gmail.com>
Tested-by: Rob Clark <robdclark@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 0cd71dac394e7e59f39bdcfd99f9983bd8717968..eb380ee2dc58cbd266721b67254c0674cfe28a39 100644 (file)
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -508,6 +508,12 @@ EXPORT_SYMBOL(drm_atomic_helper_check_planes);
  * Drivers without such needs can directly use this as their ->atomic_check()
  * callback.
  *
+ * This just wraps the two parts of the state checking for planes and modeset
+ * state in the default order: First it calls drm_atomic_helper_check_modeset()
+ * and then drm_atomic_helper_check_planes(). The assumption is that the
+ * ->atomic_check functions depend upon an updated adjusted_mode.clock to
+ * e.g. properly compute watermarks.
+ *
  * RETURNS
  * Zero for success or -errno
  */
@@ -516,11 +522,11 @@ int drm_atomic_helper_check(struct drm_device *dev,
 {
        int ret;
 
-       ret = drm_atomic_helper_check_planes(dev, state);
+       ret = drm_atomic_helper_check_modeset(dev, state);
        if (ret)
                return ret;
 
-       ret = drm_atomic_helper_check_modeset(dev, state);
+       ret = drm_atomic_helper_check_planes(dev, state);
        if (ret)
                return ret;
 

diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c
index f0de412e13dc78bbc01e9b6dd7482c74358a14e9..f8f18e882f97b92e39856baab23d47d14dffa96a 100644 (file)
--- a/drivers/gpu/drm/msm/msm_atomic.c
+++ b/drivers/gpu/drm/msm/msm_atomic.c
@@ -81,6 +81,26 @@ static void add_fb(struct msm_commit *c, struct drm_framebuffer *fb)
 }
 
 
+int msm_atomic_check(struct drm_device *dev,
+                    struct drm_atomic_state *state)
+{
+       int ret;
+
+       /*
+        * msm ->atomic_check can update ->mode_changed for pixel format
+        * changes, hence must be run before we check the modeset changes.
+        */
+       ret = drm_atomic_helper_check_planes(dev, state);
+       if (ret)
+               return ret;
+
+       ret = drm_atomic_helper_check_modeset(dev, state);
+       if (ret)
+               return ret;
+
+       return ret;
+}
+
 /**
  * drm_atomic_helper_commit - commit validated state object
  * @dev: DRM device

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index d3b791b7ddef014e35b76ec43db3be1f8f576cf2..473e4d6051e9f0b1e44828405dc1329c4657bb9b 100644 (file)
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -29,7 +29,7 @@ static void msm_fb_output_poll_changed(struct drm_device *dev)
 static const struct drm_mode_config_funcs mode_config_funcs = {
        .fb_create = msm_framebuffer_create,
        .output_poll_changed = msm_fb_output_poll_changed,
-       .atomic_check = drm_atomic_helper_check,
+       .atomic_check = msm_atomic_check,
        .atomic_commit = msm_atomic_commit,
 };
 

diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index 136303818436726004b2f294c6c90d69ccdd2f05..d408e02e4ee5bd4d36e0b1ab0f789917b8641454 100644 (file)
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -144,6 +144,8 @@ void __msm_fence_worker(struct work_struct *work);
                (_cb)->func = _func;                         \
        } while (0)
 
+int msm_atomic_check(struct drm_device *dev,
+                    struct drm_atomic_state *state);
 int msm_atomic_commit(struct drm_device *dev,
                struct drm_atomic_state *state, bool async);