drm/i915: Protect dev_priv->atomic_cdclk_freq with all the crtc locks

A modeset on one pipe can update dev_priv->atomic_cdclk_freq without
actually touching the hardware, in which case we won't force a modeset
on all the pipes, and thus won't lock any of the other pipes either.
That means a parallel plane update on another pipe could be looking at
a stale dev_priv->atomic_cdcdlk_freq and thus fail to notice when the
plane configuration is invalid, or potentially reject a valid update.

To overcome this we must protect writes to atomic_cdclk_freq with
all the crtc locks, and thus for reads any single crtc lock will
be sufficient protection.

Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/1479141311-11904-3-git-send-email-ville.syrjala@linux.intel.com
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 239d88c..e3f1a4b 100644 (file)
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2016,7 +2016,14 @@ struct drm_i915_private {
 
        unsigned int fsb_freq, mem_freq, is_ddr3;
        unsigned int skl_preferred_vco_freq;
-       unsigned int cdclk_freq, max_cdclk_freq, atomic_cdclk_freq;
+       unsigned int cdclk_freq, max_cdclk_freq;
+
+       /*
+        * For reading holding any crtc lock is sufficient,
+        * for writing must hold all of them.
+        */
+       unsigned int atomic_cdclk_freq;
+
        unsigned int max_dotclk_freq;
        unsigned int rawclk_freq;
        unsigned int hpll_freq;

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 383775e..8290ca9 100644 (file)
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13914,13 +13914,32 @@ static int haswell_mode_set_planes_workaround(struct drm_atomic_state *state)
        return 0;
 }
 
+static int intel_lock_all_pipes(struct drm_atomic_state *state)
+{
+       struct drm_crtc *crtc;
+
+       /* Add all pipes to the state */
+       for_each_crtc(state->dev, crtc) {
+               struct drm_crtc_state *crtc_state;
+
+               crtc_state = drm_atomic_get_crtc_state(state, crtc);
+               if (IS_ERR(crtc_state))
+                       return PTR_ERR(crtc_state);
+       }
+
+       return 0;
+}
+
 static int intel_modeset_all_pipes(struct drm_atomic_state *state)
 {
        struct drm_crtc *crtc;
        struct drm_crtc_state *crtc_state;
        int ret = 0;
 
-       /* add all active pipes to the state */
+       /*
+        * Add all pipes to the state, and force
+        * a modeset on all the active ones.
+        */
        for_each_crtc(state->dev, crtc) {
                crtc_state = drm_atomic_get_crtc_state(state, crtc);
                if (IS_ERR(crtc_state))
@@ -13986,12 +14005,24 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
                if (ret < 0)
                        return ret;
 
+               /*
+                * Writes to dev_priv->atomic_cdclk_freq must protected by
+                * holding all the crtc locks, even if we don't end up
+                * touching the hardware
+                */
+               if (intel_state->cdclk != dev_priv->atomic_cdclk_freq) {
+                       ret = intel_lock_all_pipes(state);
+                       if (ret < 0)
+                               return ret;
+               }
+
+               /* All pipes must be switched off while we change the cdclk. */
                if (intel_state->dev_cdclk != dev_priv->cdclk_freq ||
-                   intel_state->cdclk_pll_vco != dev_priv->cdclk_pll.vco)
+                   intel_state->cdclk_pll_vco != dev_priv->cdclk_pll.vco) {
                        ret = intel_modeset_all_pipes(state);
-
-               if (ret < 0)
-                       return ret;
+                       if (ret < 0)
+                               return ret;
+               }
 
                DRM_DEBUG_KMS("New cdclk calculated to be atomic %u, actual %u\n",
                              intel_state->cdclk, intel_state->dev_cdclk);