On Wed, Feb 25, 2015 at 01:12:16PM -0800, Matt Roper wrote:
> 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 <[email protected]>
> 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 <[email protected]>
> Signed-off-by: Daniel Vetter <[email protected]>
>
> 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 <[email protected]>
> Cc: Ander Conselvan de Oliveira <[email protected]>
> Signed-off-by: Matt Roper <[email protected]>
Both patches applied, thanks.
-Daniel
> ---
> drivers/gpu/drm/i915/i915_irq.c | 2 +-
> drivers/gpu/drm/i915/intel_display.c | 58
> ++++++++++++++++++++----------------
> drivers/gpu/drm/i915/intel_lvds.c | 2 +-
> 3 files changed, 34 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 07e257c..9baecb7 100644
> --- 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 9bc54cc..acbb362 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3077,7 +3077,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;
> }
>
> @@ -4215,7 +4215,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)) {
> @@ -4328,7 +4328,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;
> @@ -4436,7 +4436,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;
> @@ -4783,7 +4783,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);
> @@ -5004,7 +5004,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);
> }
>
> @@ -5044,7 +5044,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;
> @@ -5127,7 +5127,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;
> @@ -5326,7 +5326,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);
> @@ -5404,7 +5404,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");
> @@ -8717,7 +8718,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)
> @@ -8785,7 +8786,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
> @@ -9945,7 +9946,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;
> @@ -9975,6 +9976,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;
> }
> }
> @@ -10386,7 +10388,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)
> @@ -10461,10 +10463,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) {
> @@ -10851,7 +10853,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) {
> @@ -10865,9 +10867,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);
> @@ -10931,7 +10934,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++;
> @@ -11117,7 +11120,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);
> }
>
> @@ -11173,7 +11176,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);
> @@ -11269,7 +11272,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;
> @@ -11503,7 +11506,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;
> @@ -13392,6 +13395,7 @@ static void intel_sanitize_crtc(struct intel_crtc
> *crtc)
> }
>
> WARN_ON(crtc->active);
> + crtc->base.state->enable = false;
> crtc->base.enabled = false;
> }
>
> @@ -13408,7 +13412,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
> @@ -13416,9 +13420,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 ->
> @@ -13555,6 +13560,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 071b96d..24e8730 100644
> --- 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.
> --
> 1.8.5.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
