[Intel-gfx] [PATCH 1/2] drm/i915: Use enabled value from crtc_state rather than crtc
Daniel Vetter
daniel at ffwll.ch
Wed Feb 25 12:48:55 PST 2015
On Wed, Feb 25, 2015 at 11:43:25AM -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 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 at 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 at gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter at 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
> )
> </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.
>
> As is, this patch should fix a bunch of i-g-t regressions that were introduced
> by Daniel's patch above.
>
> Cc: Daniel Vetter <daniel at ffwll.ch>
> Cc: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira at intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
I think we can't outright switch over either yet, at least it seems a bit
risky. For now I think we need to keep both crtc->base.enabled and
crtc->base->state.enable updated.
> ---
> drivers/gpu/drm/i915/i915_irq.c | 2 +-
> drivers/gpu/drm/i915/intel_display.c | 62 +++++++++++++++++++-----------------
> drivers/gpu/drm/i915/intel_lvds.c | 2 +-
> 3 files changed, 34 insertions(+), 32 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..52f3aa4 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,7 +9976,7 @@ static void intel_modeset_commit_output_state(struct drm_device *dev)
> }
>
> for_each_intel_crtc(dev, crtc) {
> - crtc->base.enabled = crtc->new_enabled;
I.e. we'd need to keep this line.
> + crtc->base.state->enable = crtc->new_enabled;
> }
> }
>
> @@ -10386,7 +10387,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 +10462,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 +10852,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 +10866,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 +10933,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 +11119,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 +11175,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 +11271,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 +11505,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,7 +13394,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
> }
>
> WARN_ON(crtc->active);
> - crtc->base.enabled = false;
And this.
> + crtc->base.state->enable = false;
> }
>
> if (dev_priv->quirks & QUIRK_PIPEA_FORCE &&
> @@ -13408,7 +13410,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,10 +13418,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.enabled = crtc->active;
And this.
> + crtc->base.state->enable = crtc->active;
>
> /* Because we only establish the connector -> encoder ->
> * crtc links if something is active, this means the
> @@ -13555,7 +13557,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.enabled = crtc->active;
And also this one.
That would then match what the atomic helpers end up doing, which also
keep both legacy and new state updated.
Once everything has settled down a bit I think we can reaudit the drm core
for any access to legacy state left behind (there's still some I've
noticed) and then rip this all out from bot i915 modeset code and atomic
helpers.
Cheers, Daniel
> + crtc->base.state->enable = crtc->active;
> crtc->primary_enabled = primary_get_hw_state(crtc);
>
> DRM_DEBUG_KMS("[CRTC:%d] hw state readout: %s\n",
> 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
More information about the Intel-gfx
mailing list