[Intel-gfx] [PATCH 33/42] drm/i915: remove crtc->active tracking completely
Daniel Vetter
daniel at ffwll.ch
Tue May 12 02:55:13 PDT 2015
On Mon, May 11, 2015 at 04:25:09PM +0200, Maarten Lankhorst wrote:
> Small behavioral change: DPLL_DVO_2X_MODE may stay enabled during modeset
> for I830 if new state requires it, instead of being disabled and enabled
> again.
Imo this should be split in the remaing changes to get rid of ->active and
then the actual removal.
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> ---
> drivers/gpu/drm/i915/intel_display.c | 140 ++++++++++-------------------------
> drivers/gpu/drm/i915/intel_drv.h | 6 --
> 2 files changed, 40 insertions(+), 106 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 7a79659dca00..004067bd0b6c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1680,7 +1680,7 @@ static int intel_num_dvo_pipes(struct drm_device *dev)
> int count = 0;
>
> for_each_intel_crtc(dev, crtc)
> - count += crtc->active &&
> + count += crtc->base.state->active &&
> intel_pipe_has_type(crtc, INTEL_OUTPUT_DVO);
>
> return count;
> @@ -1703,7 +1703,7 @@ static void i9xx_enable_pll(struct intel_crtc *crtc, struct intel_crtc_state *pi
> assert_panel_unlocked(dev_priv, crtc->pipe);
>
> /* Enable DVO 2x clock on both PLLs if necessary */
> - if (IS_I830(dev) && intel_num_dvo_pipes(dev) > 0) {
> + if (IS_I830(dev) && intel_num_dvo_pipes(dev)) {
> /*
> * It appears to be important that we don't enable this
> * for the current pipe before otherwise configuring the
> @@ -1762,7 +1762,7 @@ static void i9xx_disable_pll(struct intel_crtc *crtc,
> /* Disable DVO 2x clock on both PLLs if necessary */
> if (IS_I830(dev) &&
> intel_pipe_has_type(crtc, INTEL_OUTPUT_DVO) &&
> - intel_num_dvo_pipes(dev) == 1) {
> + !intel_num_dvo_pipes(dev)) {
The super-correct fix would be to pass the old_state around and use those
connectors. Probably not worth the trouble.
> I915_WRITE(DPLL(PIPE_B),
> I915_READ(DPLL(PIPE_B)) & ~DPLL_DVO_2X_MODE);
> I915_WRITE(DPLL(PIPE_A),
> @@ -2583,7 +2583,7 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
> if (c == &intel_crtc->base)
> continue;
>
> - if (!i->active)
> + if (!c->state->active)
> continue;
>
> fb = c->primary->fb;
> @@ -3167,11 +3167,9 @@ static void intel_update_primary_planes(struct drm_device *dev)
> struct drm_crtc *crtc;
>
> for_each_crtc(dev, crtc) {
> - struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -
> drm_modeset_lock(&crtc->mutex, NULL);
>
> - if (intel_crtc->active) {
> + if (crtc->state->active) {
> const struct intel_plane_state *state =
> to_intel_plane_state(crtc->primary->state);
>
> @@ -4619,7 +4617,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->state->active || !intel_crtc->active)
> + if (!crtc->state->active)
> return;
>
> if (HAS_GMCH_DISPLAY(dev_priv->dev)) {
> @@ -4778,10 +4776,8 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
> int pipe = intel_crtc->pipe;
> struct intel_crtc_state *pipe_config;
>
> - WARN_ON(!crtc->state->enable);
> + WARN_ON(!crtc->state->active);
>
> - if (WARN_ON(intel_crtc->active))
> - return;
> pipe_config = to_intel_crtc_state(crtc->state);
>
> if (pipe_config->has_pch_encoder)
> @@ -4799,8 +4795,6 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
>
> ironlake_set_pipeconf(crtc);
>
> - intel_crtc->active = true;
> -
> intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
> intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, true);
>
> @@ -4808,7 +4802,7 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
> if (encoder->pre_enable)
> encoder->pre_enable(encoder);
>
> - if (intel_crtc->config->has_pch_encoder) {
> + if (pipe_config->has_pch_encoder) {
Misplace hunk?
> /* Note: FDI PLL enabling _must_ be done before we enable the
> * cpu pipes, hence this is separate from all the other fdi/pch
> * enabling. */
> @@ -4859,9 +4853,6 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>
> WARN_ON(!crtc->state->enable);
>
> - if (WARN_ON(intel_crtc->active))
> - return;
> -
> pipe_config = to_intel_crtc_state(crtc->state);
> if (intel_crtc_to_shared_dpll(intel_crtc))
> intel_enable_shared_dpll(intel_crtc);
> @@ -4885,8 +4876,6 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>
> intel_set_pipe_csc(crtc);
>
> - intel_crtc->active = true;
> -
> intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
> for_each_encoder_on_crtc(dev, crtc, encoder)
> if (encoder->pre_enable)
> @@ -5743,8 +5732,6 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
>
> WARN_ON(!crtc->state->active);
>
> - if (WARN_ON(intel_crtc->active))
> - return;
> pipe_config = to_intel_crtc_state(crtc->state);
>
> is_dsi = intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DSI);
> @@ -5770,8 +5757,6 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
>
> i9xx_set_pipeconf(intel_crtc);
>
> - intel_crtc->active = true;
> -
> intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
>
> for_each_encoder_on_crtc(dev, crtc, encoder)
> @@ -5824,9 +5809,6 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
> pipe_config = to_intel_crtc_state(crtc->state);
> WARN_ON(!pipe_config->base.active);
>
> - if (WARN_ON(intel_crtc->active))
> - return;
> -
> i9xx_set_pll_dividers(intel_crtc);
>
> if (intel_crtc->config->has_dp_encoder)
> @@ -5836,8 +5818,6 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
>
> i9xx_set_pipeconf(intel_crtc);
>
> - intel_crtc->active = true;
> -
> if (!IS_GEN2(dev))
> intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
>
> @@ -5930,7 +5910,7 @@ void intel_crtc_control(struct drm_crtc *crtc, bool enable)
> struct drm_atomic_state *state;
> int ret;
>
> - if (enable == intel_crtc->active)
> + if (enable == crtc->state->active)
> return;
>
> if (enable && !crtc->state->enable)
> @@ -6045,9 +6025,9 @@ static void intel_connector_check_state(struct intel_connector *connector)
>
> crtc = encoder->base.crtc;
>
> - I915_STATE_WARN(!crtc->state->active,
> + 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(!crtc->state->active, "crtc not active\n");
> I915_STATE_WARN(pipe != to_intel_crtc(crtc)->pipe,
> "encoder active on the wrong pipe\n");
> }
> @@ -8798,7 +8778,7 @@ static void assert_can_disable_lcpll(struct drm_i915_private *dev_priv)
> struct intel_crtc *crtc;
>
> for_each_intel_crtc(dev, crtc)
> - I915_STATE_WARN(crtc->active, "CRTC for pipe %c enabled\n",
> + I915_STATE_WARN(crtc->base.state->active, "CRTC for pipe %c enabled\n",
> pipe_name(crtc->pipe));
>
> I915_STATE_WARN(I915_READ(HSW_PWR_WELL_DRIVER), "Power well on\n");
> @@ -11738,7 +11718,7 @@ static void check_wm_state(struct drm_device *dev)
> struct skl_ddb_entry *hw_entry, *sw_entry;
> const enum pipe pipe = intel_crtc->pipe;
>
> - if (!intel_crtc->active)
> + if (!intel_crtc->base.state->active)
> continue;
>
> /* planes */
> @@ -11871,9 +11851,6 @@ check_crtc_state(struct drm_device *dev)
> DRM_DEBUG_KMS("[CRTC:%d]\n",
> crtc->base.base.id);
>
> - I915_STATE_WARN(crtc->active && !crtc->base.state->enable,
> - "active crtc, but not enabled in sw tracking\n");
> -
> for_each_intel_encoder(dev, encoder) {
> if (encoder->base.crtc != &crtc->base)
> continue;
> @@ -11882,9 +11859,9 @@ check_crtc_state(struct drm_device *dev)
> active = true;
> }
>
> - I915_STATE_WARN(active != crtc->active,
> + I915_STATE_WARN(active != crtc->base.state->active,
> "crtc's computed active state doesn't match tracked active state "
> - "(expected %i, found %i)\n", active, crtc->active);
> + "(expected %i, found %i)\n", active, crtc->base.state->active);
> 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,
> @@ -11896,7 +11873,7 @@ check_crtc_state(struct drm_device *dev)
> /* hw state is inconsistent with the pipe quirk */
> if ((crtc->pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE) ||
> (crtc->pipe == PIPE_B && dev_priv->quirks & QUIRK_PIPEB_FORCE))
> - active = crtc->active;
> + active = crtc->base.state->active;
>
> for_each_intel_encoder(dev, encoder) {
> enum pipe pipe;
> @@ -11906,9 +11883,9 @@ check_crtc_state(struct drm_device *dev)
> encoder->get_config(encoder, &pipe_config);
> }
>
> - I915_STATE_WARN(crtc->active != active,
> + I915_STATE_WARN(crtc->base.state->active != active,
> "crtc active state doesn't match with hw state "
> - "(expected %i, found %i)\n", crtc->active, active);
> + "(expected %i, found %i)\n", crtc->base.state->active, active);
>
> if (active &&
> !intel_pipe_config_compare(dev, crtc->config, &pipe_config)) {
> @@ -11931,7 +11908,7 @@ check_shared_dpll_state(struct drm_device *dev)
>
> for (i = 0; i < dev_priv->num_shared_dpll; i++) {
> struct intel_shared_dpll *pll = &dev_priv->shared_dplls[i];
> - int enabled_crtcs = 0, active_crtcs = 0;
> + int active_crtcs = 0;
> bool active;
>
> memset(&dpll_hw_state, 0, sizeof(dpll_hw_state));
> @@ -11953,16 +11930,14 @@ check_shared_dpll_state(struct drm_device *dev)
>
> for_each_intel_crtc(dev, crtc) {
> if (crtc->base.state->active && intel_crtc_to_shared_dpll(crtc) == pll)
> - enabled_crtcs++;
> - if (crtc->active && intel_crtc_to_shared_dpll(crtc) == pll)
> active_crtcs++;
> }
> I915_STATE_WARN(pll->active != active_crtcs,
> "pll active crtcs mismatch (expected %i, found %i)\n",
> pll->active, active_crtcs);
> - I915_STATE_WARN(hweight32(pll->config.crtc_mask) != enabled_crtcs,
> + I915_STATE_WARN(hweight32(pll->config.crtc_mask) != active_crtcs,
> "pll enabled crtcs mismatch (expected %i, found %i)\n",
> - hweight32(pll->config.crtc_mask), enabled_crtcs);
> + hweight32(pll->config.crtc_mask), active_crtcs);
>
> I915_STATE_WARN(pll->on && memcmp(&pll->config.hw_state, &dpll_hw_state,
> sizeof(dpll_hw_state)),
> @@ -12264,10 +12239,10 @@ static void __intel_set_mode_update_planes(struct drm_device *dev,
> to_intel_plane_state(plane->state)->hw_enabled = false;
> DRM_DEBUG_ATOMIC("Plane %i is visible: %i\n", i, visible);
>
> - if (!visible)
> + if (crtc && needs_modeset(crtc->state))
> + intel_plane->disable_plane(plane, crtc, !visible);
> + else if (!visible)
> funcs->atomic_update(plane, old_plane_state);
> - else if (needs_modeset(crtc->state))
> - intel_plane->disable_plane(plane, crtc, true);
> }
> }
>
> @@ -12349,8 +12324,6 @@ static int __intel_set_mode(struct drm_atomic_state *state)
> intel_crtc_dpms_overlay_disable(to_intel_crtc(crtc));
> dev_priv->display.crtc_disable(crtc, pipe_config);
>
> - intel_crtc->active = false;
> -
> if (intel_crtc_to_shared_dpll(intel_crtc))
> intel_disable_shared_dpll(intel_crtc);
> }
> @@ -13035,11 +13008,9 @@ static int intel_atomic_check_crtc(struct drm_crtc *crtc,
> memset(&intel_crtc->atomic, 0, sizeof(intel_crtc->atomic));
> intel_crtc->atomic.update_wm = mode_changed;
> intel_crtc->atomic.disable_fbc = mode_changed;
> + intel_crtc->atomic.evade = !mode_changed && was_crtc_enabled;
>
> idx = crtc->base.id;
> - I915_STATE_WARN(crtc->state->active != intel_crtc->active,
> - "Crtc %i mismatch between state->active(%i) and crtc->active (%i)\n",
> - idx, crtc->state->active, intel_crtc->active);
>
> DRM_DEBUG_ATOMIC("Crtc %i was enabled %i now enabled: %i\n",
> idx, was_crtc_enabled, is_crtc_enabled);
> @@ -13229,7 +13200,7 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc)
> intel_runtime_pm_get(dev_priv);
>
> /* Perform vblank evasion around commit operation */
> - if (intel_crtc->active && !needs_modeset(crtc->state) &&
> + if (intel_crtc->atomic.evade &&
> !dev_priv->power_domains.init_power_on)
> intel_crtc->atomic.evade =
> intel_pipe_update_start(intel_crtc,
> @@ -14537,7 +14508,7 @@ void intel_modeset_init(struct drm_device *dev)
> drm_modeset_unlock_all(dev);
>
> for_each_intel_crtc(dev, crtc) {
> - if (!crtc->active)
> + if (!crtc->base.state->active)
> continue;
>
> /*
> @@ -14615,7 +14586,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>
> /* restore vblank interrupts to correct state */
> drm_crtc_vblank_reset(&crtc->base);
> - if (crtc->active) {
> + if (crtc->base.state->active) {
> update_scanline_offset(crtc);
> drm_crtc_vblank_on(&crtc->base);
> }
> @@ -14655,14 +14626,14 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
> connector->encoder->connectors_active = false;
> }
>
> - WARN_ON(crtc->active);
> + WARN_ON(crtc->base.state->active);
> crtc->base.state->enable = false;
> crtc->base.state->active = false;
> crtc->base.enabled = false;
> }
>
> if (dev_priv->quirks & QUIRK_PIPEA_FORCE &&
> - crtc->pipe == PIPE_A && !crtc->active) {
> + crtc->pipe == PIPE_A && !crtc->base.state->active) {
> /* BIOS forgot to enable pipe A, this mostly happens after
> * resume. Force-enable the pipe to fix this, the update_dpms
> * call below we restore the pipe to the right state, but leave
> @@ -14674,35 +14645,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.state->active) {
> - struct intel_encoder *encoder;
> -
> - /* This can happen either due to bugs in the get_hw_state
> - * functions or because the pipe is force-enabled due to the
> - * pipe A quirk. */
> - DRM_DEBUG_KMS("[CRTC:%d] hw state adjusted, was %s, now %s\n",
> - crtc->base.base.id,
> - crtc->base.state->enable ? "enabled" : "disabled",
> - crtc->active ? "enabled" : "disabled");
> -
> - crtc->base.state->enable = crtc->active;
> - crtc->base.state->active = crtc->active;
> - crtc->base.enabled = crtc->active;
> -
> - /* Because we only establish the connector -> encoder ->
> - * crtc links if something is active, this means the
> - * crtc is now deactivated. Break the links. connector
> - * -> encoder links are only establish when things are
> - * actually up, hence no need to break them. */
> - WARN_ON(crtc->active);
> -
> - for_each_encoder_on_crtc(dev, &crtc->base, encoder) {
> - WARN_ON(encoder->connectors_active);
> - encoder->base.crtc = NULL;
> - }
> - }
I don't think you can nuke this. Essentially we have two bits here:
- is the pipe enabled (bit31 in pipeconf)?
- is any encoder using this pipe and enabled?
The later is computed/updated in intel_crtc_update_dpms. I'm not sure
where exactly, but something in these pile of changes might have broken
this.
If not I think it should be extracted into a separate patch together with
the other changes (to intel_crtc_control maybe?) to make it easier to
follow why this code isn't needed any more. At least silently removing
code with scary comments without explaining why it's dead now (and without
moving the comments to the new places) isn't good.
-Daniel
> -
> - if (crtc->active || HAS_GMCH_DISPLAY(dev)) {
> + if (crtc->base.state->active || HAS_GMCH_DISPLAY(dev)) {
> /*
> * We start out with underrun reporting disabled to avoid races.
> * For correct bookkeeping mark this on active crtcs.
> @@ -14730,7 +14673,7 @@ static void intel_sanitize_encoder(struct intel_encoder *encoder)
> /* We need to check both for a crtc link (meaning that the
> * encoder is active and trying to read from a pipe) and the
> * pipe itself being active. */
> - bool has_active_crtc = crtc && to_intel_crtc(crtc)->active;
> + bool has_active_crtc = crtc && crtc->state->active;
>
> if (encoder->connectors_active && !has_active_crtc) {
> DRM_DEBUG_KMS("[ENCODER:%d:%s] has active connectors but no active pipe!\n",
> @@ -14800,7 +14743,7 @@ static bool primary_get_hw_state(struct intel_crtc *crtc)
> {
> struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
>
> - if (!crtc->active)
> + if (!crtc->base.state->active)
> return false;
>
> return I915_READ(DSPCNTR(crtc->plane)) & DISPLAY_PLANE_ENABLE;
> @@ -14828,12 +14771,9 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev,
> if (crtc->base.state->active)
> *crtc_mask |= drm_crtc_index(&crtc->base);
>
> - crtc->active = dev_priv->display.get_pipe_config(crtc,
> - crtc->config);
> -
> - crtc->base.state->enable = crtc->active;
> - crtc->base.state->active = crtc->active;
> - crtc->base.enabled = crtc->active;
> + crtc->base.enabled = crtc->base.state->enable =
> + crtc->base.state->active =
> + dev_priv->display.get_pipe_config(crtc, crtc->config);
>
> plane_state = to_intel_plane_state(primary->state);
> plane_state->hw_enabled = plane_state->visible =
> @@ -14847,7 +14787,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev,
>
> DRM_DEBUG_KMS("[CRTC:%d] hw state readout: %s\n",
> crtc->base.base.id,
> - crtc->active ? "enabled" : "disabled");
> + crtc->base.state->active ? "enabled" : "disabled");
> }
>
> for (i = 0; i < dev_priv->num_shared_dpll; i++) {
> @@ -14858,7 +14798,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev,
> pll->active = 0;
> pll->config.crtc_mask = 0;
> for_each_intel_crtc(dev, crtc) {
> - if (crtc->active && intel_crtc_to_shared_dpll(crtc) == pll) {
> + if (crtc->base.state->active && intel_crtc_to_shared_dpll(crtc) == pll) {
> pll->active++;
> pll->config.crtc_mask |= 1 << crtc->pipe;
> }
> @@ -14927,9 +14867,9 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
> * checking everywhere.
> */
> for_each_intel_crtc(dev, crtc) {
> - if (crtc->active && i915.fastboot) {
> + if (crtc->base.state->active && i915.fastboot) {
> intel_mode_from_pipe_config(&crtc->base.mode,
> - crtc->config);
> + to_intel_crtc_state(crtc->base.state));
> DRM_DEBUG_KMS("[CRTC:%d] found active mode: ",
> crtc->base.base.id);
> drm_mode_debug_printmodeline(&crtc->base.mode);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 6c2ba5dbcd79..1a0b0cd857c3 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -492,12 +492,6 @@ struct intel_crtc {
> enum pipe pipe;
> enum plane plane;
> u8 lut_r[256], lut_g[256], lut_b[256];
> - /*
> - * Whether the crtc and the connected output pipeline is active. Implies
> - * that crtc->enabled is set, i.e. the current mode configuration has
> - * some outputs connected to this crtc.
> - */
> - bool active;
> unsigned long enabled_power_domains;
> bool lowfreq_avail;
> struct intel_overlay *overlay;
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the Intel-gfx
mailing list