drm/i915: Use enabled value from crtc_state rather than crtc (v2)

As vendors transition their drivers from legacy to atomic there's some
duplication of data between drm_crtc and drm_crtc_state (since
unconverted drivers likely won't have a state structure).

i915 is partially converted and does have a crtc->state structure, but
still uses direct crtc fields internally in many places, which causes
the two sets of data to get out of sync.  As of commit

        commit 31c946e85ce6b48ce0f25e3cdca8362e4fe8b300
        Author: Daniel Vetter <daniel.vetter@ffwll.ch>
        Date:   Sun Feb 22 12:24:17 2015 +0100

            drm: If available use atomic state in getcrtc ioctl

            This way drivers fully converted to atomic don't need to update these
            legacy state variables in their modeset code any more.

            Reviewed-by: Rob Clark <robdclark@gmail.com>
            Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
the DRM core starts assuming that the presence of a ->state structure
implies that it should make use of the values stored there which, on
i915, leads to the core code using stale values for CRTC 'enabled'
status.

Let's switch over to using the state value of 'enable' internally rather
than using the drm_crtc field.  This ensures that our driver internals
are working from the same data that the DRM core is, avoiding
mismatches.

This patch was generated with Coccinelle using the following semantic
patch:

        <smpl>
        @@
        struct drm_crtc C;
        struct drm_crtc *CP;
        @@
        (
        - C.enabled
        + C.state->enable
        |
        - CP->enabled
        + CP->state->enable
        )

        // For assignments, we still update the legacy value as well as the state value
        // so add an extra assignment statement for that.
        @@
        struct drm_crtc C;
        struct drm_crtc *CP;
        expression E;
        @@
        (
          C.state->enable = E;
        + C.enabled = E;
        |
          CP->state->enable = E;
        + CP->enabled = E;
        )
        </smpl>

The crtc->mode and crtc->hwmode fields should probably be transitioned
over as well eventually, but we seem to do an okay job of keeping those
up-to-date already so I want to minimize the changes that will clash
with Ander's in-progress atomic work.

v2: Don't remove the assignments to the legacy value when we assign to
    the state value.  A second cocci stanza takes care of adding the
    legacy assignment back where appropriate.  (Daniel)

Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 1faccba588754f7295f36f0e2e0cdf7f2f60a537..6ebea4614204293f9b5107310e8cc8730b4414cb 100644 (file)
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -791,7 +791,7 @@ static int i915_get_vblank_timestamp(struct drm_device *dev, int pipe,
                return -EINVAL;
        }
 
-       if (!crtc->enabled) {
+       if (!crtc->state->enable) {
                DRM_DEBUG_KMS("crtc %d is disabled\n", pipe);
                return -EBUSY;
        }

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2ac93909cfc5315cddcd9e9cd7f0601439364a53..25ee6fa8fc45fc1e73b53ce8bec7654b447b0cef 100644 (file)
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3062,7 +3062,7 @@ static void intel_fdi_normal_train(struct drm_crtc *crtc)
 
 static bool pipe_has_enabled_pch(struct intel_crtc *crtc)
 {
-       return crtc->base.enabled && crtc->active &&
+       return crtc->base.state->enable && crtc->active &&
                crtc->config->has_pch_encoder;
 }
 
@@ -4200,7 +4200,7 @@ static void intel_crtc_load_lut(struct drm_crtc *crtc)
        bool reenable_ips = false;
 
        /* The clocks have to be on to load the palette. */
-       if (!crtc->enabled || !intel_crtc->active)
+       if (!crtc->state->enable || !intel_crtc->active)
                return;
 
        if (!HAS_PCH_SPLIT(dev_priv->dev)) {
@@ -4313,7 +4313,7 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
        struct intel_encoder *encoder;
        int pipe = intel_crtc->pipe;
 
-       WARN_ON(!crtc->enabled);
+       WARN_ON(!crtc->state->enable);
 
        if (intel_crtc->active)
                return;
@@ -4421,7 +4421,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
        struct intel_encoder *encoder;
        int pipe = intel_crtc->pipe;
 
-       WARN_ON(!crtc->enabled);
+       WARN_ON(!crtc->state->enable);
 
        if (intel_crtc->active)
                return;
@@ -4768,7 +4768,7 @@ static void modeset_update_crtc_power_domains(struct drm_device *dev)
        for_each_intel_crtc(dev, crtc) {
                enum intel_display_power_domain domain;
 
-               if (!crtc->base.enabled)
+               if (!crtc->base.state->enable)
                        continue;
 
                pipe_domains[crtc->pipe] = get_crtc_power_domains(&crtc->base);
@@ -4989,7 +4989,7 @@ static void valleyview_modeset_global_pipes(struct drm_device *dev,
 
        /* disable/enable all currently active pipes while we change cdclk */
        for_each_intel_crtc(dev, intel_crtc)
-               if (intel_crtc->base.enabled)
+               if (intel_crtc->base.state->enable)
                        *prepare_pipes |= (1 << intel_crtc->pipe);
 }
 
@@ -5029,7 +5029,7 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
        int pipe = intel_crtc->pipe;
        bool is_dsi;
 
-       WARN_ON(!crtc->enabled);
+       WARN_ON(!crtc->state->enable);
 
        if (intel_crtc->active)
                return;
@@ -5112,7 +5112,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
        struct intel_encoder *encoder;
        int pipe = intel_crtc->pipe;
 
-       WARN_ON(!crtc->enabled);
+       WARN_ON(!crtc->state->enable);
 
        if (intel_crtc->active)
                return;
@@ -5311,7 +5311,7 @@ static void intel_crtc_disable(struct drm_crtc *crtc)
        struct drm_i915_private *dev_priv = dev->dev_private;
 
        /* crtc should still be enabled when we disable it. */
-       WARN_ON(!crtc->enabled);
+       WARN_ON(!crtc->state->enable);
 
        dev_priv->display.crtc_disable(crtc);
        dev_priv->display.off(crtc);
@@ -5389,7 +5389,8 @@ static void intel_connector_check_state(struct intel_connector *connector)
 
                        crtc = encoder->base.crtc;
 
-                       I915_STATE_WARN(!crtc->enabled, "crtc not enabled\n");
+                       I915_STATE_WARN(!crtc->state->enable,
+                                       "crtc not enabled\n");
                        I915_STATE_WARN(!to_intel_crtc(crtc)->active, "crtc not active\n");
                        I915_STATE_WARN(pipe != to_intel_crtc(crtc)->pipe,
                             "encoder active on the wrong pipe\n");
@@ -8702,7 +8703,7 @@ retry:
                i++;
                if (!(encoder->possible_crtcs & (1 << i)))
                        continue;
-               if (possible_crtc->enabled)
+               if (possible_crtc->state->enable)
                        continue;
                /* This can occur when applying the pipe A quirk on resume. */
                if (to_intel_crtc(possible_crtc)->new_enabled)
@@ -8770,7 +8771,7 @@ retry:
        return true;
 
  fail:
-       intel_crtc->new_enabled = crtc->enabled;
+       intel_crtc->new_enabled = crtc->state->enable;
        if (intel_crtc->new_enabled)
                intel_crtc->new_config = intel_crtc->config;
        else
@@ -9930,7 +9931,7 @@ static void intel_modeset_update_staged_output_state(struct drm_device *dev)
        }
 
        for_each_intel_crtc(dev, crtc) {
-               crtc->new_enabled = crtc->base.enabled;
+               crtc->new_enabled = crtc->base.state->enable;
 
                if (crtc->new_enabled)
                        crtc->new_config = crtc->config;
@@ -9960,6 +9961,7 @@ static void intel_modeset_commit_output_state(struct drm_device *dev)
        }
 
        for_each_intel_crtc(dev, crtc) {
+               crtc->base.state->enable = crtc->new_enabled;
                crtc->base.enabled = crtc->new_enabled;
        }
 }
@@ -10371,7 +10373,7 @@ intel_modeset_affected_pipes(struct drm_crtc *crtc, unsigned *modeset_pipes,
 
        /* Check for pipes that will be enabled/disabled ... */
        for_each_intel_crtc(dev, intel_crtc) {
-               if (intel_crtc->base.enabled == intel_crtc->new_enabled)
+               if (intel_crtc->base.state->enable == intel_crtc->new_enabled)
                        continue;
 
                if (!intel_crtc->new_enabled)
@@ -10446,10 +10448,10 @@ intel_modeset_update_state(struct drm_device *dev, unsigned prepare_pipes)
 
        /* Double check state. */
        for_each_intel_crtc(dev, intel_crtc) {
-               WARN_ON(intel_crtc->base.enabled != intel_crtc_in_use(&intel_crtc->base));
+               WARN_ON(intel_crtc->base.state->enable != intel_crtc_in_use(&intel_crtc->base));
                WARN_ON(intel_crtc->new_config &&
                        intel_crtc->new_config != intel_crtc->config);
-               WARN_ON(intel_crtc->base.enabled != !!intel_crtc->new_config);
+               WARN_ON(intel_crtc->base.state->enable != !!intel_crtc->new_config);
        }
 
        list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
@@ -10836,7 +10838,7 @@ check_crtc_state(struct drm_device *dev)
                DRM_DEBUG_KMS("[CRTC:%d]\n",
                              crtc->base.base.id);
 
-               I915_STATE_WARN(crtc->active && !crtc->base.enabled,
+               I915_STATE_WARN(crtc->active && !crtc->base.state->enable,
                     "active crtc, but not enabled in sw tracking\n");
 
                for_each_intel_encoder(dev, encoder) {
@@ -10850,9 +10852,10 @@ check_crtc_state(struct drm_device *dev)
                I915_STATE_WARN(active != crtc->active,
                     "crtc's computed active state doesn't match tracked active state "
                     "(expected %i, found %i)\n", active, crtc->active);
-               I915_STATE_WARN(enabled != crtc->base.enabled,
+               I915_STATE_WARN(enabled != crtc->base.state->enable,
                     "crtc's computed enabled state doesn't match tracked enabled state "
-                    "(expected %i, found %i)\n", enabled, crtc->base.enabled);
+                    "(expected %i, found %i)\n", enabled,
+                               crtc->base.state->enable);
 
                active = dev_priv->display.get_pipe_config(crtc,
                                                           &pipe_config);
@@ -10916,7 +10919,7 @@ check_shared_dpll_state(struct drm_device *dev)
                     pll->on, active);
 
                for_each_intel_crtc(dev, crtc) {
-                       if (crtc->base.enabled && intel_crtc_to_shared_dpll(crtc) == pll)
+                       if (crtc->base.state->enable && intel_crtc_to_shared_dpll(crtc) == pll)
                                enabled_crtcs++;
                        if (crtc->active && intel_crtc_to_shared_dpll(crtc) == pll)
                                active_crtcs++;
@@ -11102,7 +11105,7 @@ static int __intel_set_mode(struct drm_crtc *crtc,
                intel_crtc_disable(&intel_crtc->base);
 
        for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc) {
-               if (intel_crtc->base.enabled)
+               if (intel_crtc->base.state->enable)
                        dev_priv->display.crtc_disable(&intel_crtc->base);
        }
 
@@ -11158,7 +11161,7 @@ static int __intel_set_mode(struct drm_crtc *crtc,
 
        /* FIXME: add subpixel order */
 done:
-       if (ret && crtc->enabled)
+       if (ret && crtc->state->enable)
                crtc->mode = *saved_mode;
 
        kfree(saved_mode);
@@ -11254,7 +11257,7 @@ static int intel_set_config_save_state(struct drm_device *dev,
         */
        count = 0;
        for_each_crtc(dev, crtc) {
-               config->save_crtc_enabled[count++] = crtc->enabled;
+               config->save_crtc_enabled[count++] = crtc->state->enable;
        }
 
        count = 0;
@@ -11488,7 +11491,7 @@ intel_modeset_stage_output_state(struct drm_device *dev,
                        }
                }
 
-               if (crtc->new_enabled != crtc->base.enabled) {
+               if (crtc->new_enabled != crtc->base.state->enable) {
                        DRM_DEBUG_KMS("crtc %sabled, full mode switch\n",
                                      crtc->new_enabled ? "en" : "dis");
                        config->mode_changed = true;
@@ -13377,6 +13380,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
                        }
 
                WARN_ON(crtc->active);
+               crtc->base.state->enable = false;
                crtc->base.enabled = false;
        }
 
@@ -13393,7 +13397,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
         * have active connectors/encoders. */
        intel_crtc_update_dpms(&crtc->base);
 
-       if (crtc->active != crtc->base.enabled) {
+       if (crtc->active != crtc->base.state->enable) {
                struct intel_encoder *encoder;
 
                /* This can happen either due to bugs in the get_hw_state
@@ -13401,9 +13405,10 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
                 * pipe A quirk. */
                DRM_DEBUG_KMS("[CRTC:%d] hw state adjusted, was %s, now %s\n",
                              crtc->base.base.id,
-                             crtc->base.enabled ? "enabled" : "disabled",
+                             crtc->base.state->enable ? "enabled" : "disabled",
                              crtc->active ? "enabled" : "disabled");
 
+               crtc->base.state->enable = crtc->active;
                crtc->base.enabled = crtc->active;
 
                /* Because we only establish the connector -> encoder ->
@@ -13540,6 +13545,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
                crtc->active = dev_priv->display.get_pipe_config(crtc,
                                                                 crtc->config);
 
+               crtc->base.state->enable = crtc->active;
                crtc->base.enabled = crtc->active;
                crtc->primary_enabled = primary_get_hw_state(crtc);
 

diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index 071b96d6e14642778f25a89e81b4ca908c0b64ea..24e8730dc18907b62bfc875c7899e305469b58ba 100644 (file)
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -509,7 +509,7 @@ static int intel_lvds_set_property(struct drm_connector *connector,
                intel_connector->panel.fitting_mode = value;
 
                crtc = intel_attached_encoder(connector)->base.crtc;
-               if (crtc && crtc->enabled) {
+               if (crtc && crtc->state->enable) {
                        /*
                         * If the CRTC is enabled, the display will be changed
                         * according to the new panel fitting mode.