Revert "drm/i915: Read hw state into an atomic state struct, v2."
This reverts commit 3bae26eb2991c00670df377cf6c3bc2b0577e82a.
Seems it introduces regressions for 3 different reasons, oh boy..
In bug #90868 as I can see the atomic state will be restored on
resume without the planes being set up properly. Because plane
setup here requires the atomic state, we'll have to settle
for committing atomic planes first.
In bug #90861 the failure appears to affect mostly DP devices,
and happens because reading out the atomic state prevents a modeset
on boot, which would require better hw state readout.
In bug #90874 it's shown that cdclk should be part of the atomic
state, so only performing a single modeset during resume excarbated
the issue.
It's better to fix those issues first, and then commit this patch,
so do that temporarily.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90868
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90861
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90874
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Acked-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index cc7fad9..5cc2263 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10300,7 +10300,7 @@
retry:
ret = drm_modeset_lock(&config->connection_mutex, ctx);
if (ret)
- goto fail;
+ goto fail_unlock;
/*
* Algorithm gets a little messy:
@@ -10318,10 +10318,10 @@
ret = drm_modeset_lock(&crtc->mutex, ctx);
if (ret)
- goto fail;
+ goto fail_unlock;
ret = drm_modeset_lock(&crtc->primary->mutex, ctx);
if (ret)
- goto fail;
+ goto fail_unlock;
old->dpms_mode = connector->dpms;
old->load_detect_temp = false;
@@ -10340,6 +10340,9 @@
continue;
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)
+ continue;
crtc = possible_crtc;
break;
@@ -10350,17 +10353,20 @@
*/
if (!crtc) {
DRM_DEBUG_KMS("no pipe available for load-detect\n");
- goto fail;
+ goto fail_unlock;
}
ret = drm_modeset_lock(&crtc->mutex, ctx);
if (ret)
- goto fail;
+ goto fail_unlock;
ret = drm_modeset_lock(&crtc->primary->mutex, ctx);
if (ret)
- goto fail;
+ goto fail_unlock;
+ intel_encoder->new_crtc = to_intel_crtc(crtc);
+ to_intel_connector(connector)->new_encoder = intel_encoder;
intel_crtc = to_intel_crtc(crtc);
+ intel_crtc->new_enabled = true;
old->dpms_mode = connector->dpms;
old->load_detect_temp = true;
old->release_fb = NULL;
@@ -10428,7 +10434,9 @@
intel_wait_for_vblank(dev, intel_crtc->pipe);
return true;
-fail:
+ fail:
+ intel_crtc->new_enabled = crtc->state->enable;
+fail_unlock:
drm_atomic_state_free(state);
state = NULL;
@@ -10474,6 +10482,10 @@
if (IS_ERR(crtc_state))
goto fail;
+ to_intel_connector(connector)->new_encoder = NULL;
+ intel_encoder->new_crtc = NULL;
+ intel_crtc->new_enabled = false;
+
connector_state->best_encoder = NULL;
connector_state->crtc = NULL;
@@ -11620,6 +11632,33 @@
.atomic_flush = intel_finish_crtc_commit,
};
+/**
+ * intel_modeset_update_staged_output_state
+ *
+ * Updates the staged output configuration state, e.g. after we've read out the
+ * current hw state.
+ */
+static void intel_modeset_update_staged_output_state(struct drm_device *dev)
+{
+ struct intel_crtc *crtc;
+ struct intel_encoder *encoder;
+ struct intel_connector *connector;
+
+ for_each_intel_connector(dev, connector) {
+ connector->new_encoder =
+ to_intel_encoder(connector->base.encoder);
+ }
+
+ for_each_intel_encoder(dev, encoder) {
+ encoder->new_crtc =
+ to_intel_crtc(encoder->base.crtc);
+ }
+
+ for_each_intel_crtc(dev, crtc) {
+ crtc->new_enabled = crtc->base.state->enable;
+ }
+}
+
/* Transitional helper to copy current connector/encoder state to
* connector->state. This is needed so that code that is partially
* converted to atomic does the right thing.
@@ -12149,6 +12188,7 @@
}
drm_atomic_helper_update_legacy_modeset_state(state->dev, state);
+ intel_modeset_update_staged_output_state(state->dev);
/* Double check state. */
for_each_crtc(dev, crtc) {
@@ -12458,14 +12498,11 @@
struct intel_connector *connector;
for_each_intel_connector(dev, connector) {
- struct drm_encoder *encoder = connector->base.encoder;
- struct drm_connector_state *state = connector->base.state;
-
/* This also checks the encoder/connector hw state with the
* ->get_hw_state callbacks. */
intel_connector_check_state(connector);
- I915_STATE_WARN(state->best_encoder != encoder,
+ I915_STATE_WARN(&connector->new_encoder->base != connector->base.encoder,
"connector's staged encoder doesn't match current encoder\n");
}
}
@@ -12485,6 +12522,8 @@
encoder->base.base.id,
encoder->base.name);
+ I915_STATE_WARN(&encoder->new_crtc->base != encoder->base.crtc,
+ "encoder's stage crtc doesn't match current crtc\n");
I915_STATE_WARN(encoder->connectors_active && !encoder->base.crtc,
"encoder's active_connectors set, but no crtc\n");
@@ -12494,9 +12533,6 @@
enabled = true;
if (connector->base.dpms != DRM_MODE_DPMS_OFF)
active = true;
-
- I915_STATE_WARN(connector->base.state->crtc != encoder->base.crtc,
- "encoder's stage crtc doesn't match current crtc\n");
}
/*
* for MST connectors if we unplug the connector is gone
@@ -13000,11 +13036,11 @@
* need to copy the staged config to the atomic state, otherwise the
* mode set will just reapply the state the HW is already in. */
for_each_intel_encoder(dev, encoder) {
- if (encoder->base.crtc != crtc)
+ if (&encoder->new_crtc->base != crtc)
continue;
for_each_intel_connector(dev, connector) {
- if (connector->base.state->best_encoder != &encoder->base)
+ if (connector->new_encoder != encoder)
continue;
connector_state = drm_atomic_get_connector_state(state, &connector->base);
@@ -13017,10 +13053,14 @@
}
connector_state->crtc = crtc;
+ connector_state->best_encoder = &encoder->base;
}
}
for_each_intel_crtc(dev, intel_crtc) {
+ if (intel_crtc->new_enabled == intel_crtc->base.enabled)
+ continue;
+
crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
if (IS_ERR(crtc_state)) {
DRM_DEBUG_KMS("Failed to add [CRTC:%d] to state: %ld\n",
@@ -13029,6 +13069,9 @@
continue;
}
+ crtc_state->base.active = crtc_state->base.enable =
+ intel_crtc->new_enabled;
+
if (&intel_crtc->base == crtc)
drm_mode_copy(&crtc_state->base.mode, &crtc->mode);
}
@@ -15105,7 +15148,6 @@
* ... */
plane = crtc->plane;
to_intel_plane_state(crtc->base.primary->state)->visible = true;
- crtc->base.primary->crtc = &crtc->base;
crtc->plane = !plane;
intel_crtc_control(&crtc->base, false);
crtc->plane = plane;
@@ -15269,182 +15311,78 @@
{
struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
- if (!crtc->base.enabled)
+ if (!crtc->active)
return false;
return I915_READ(DSPCNTR(crtc->plane)) & DISPLAY_PLANE_ENABLE;
}
-static int readout_hw_crtc_state(struct drm_atomic_state *state,
- struct intel_crtc *crtc)
+static void intel_modeset_readout_hw_state(struct drm_device *dev)
{
- struct drm_i915_private *dev_priv = to_i915(state->dev);
- struct intel_crtc_state *crtc_state;
- struct drm_plane *primary = crtc->base.primary;
- struct drm_plane_state *drm_plane_state;
- struct intel_plane_state *plane_state;
- int ret;
-
- crtc_state = intel_atomic_get_crtc_state(state, crtc);
- if (IS_ERR(crtc_state))
- return PTR_ERR(crtc_state);
-
- ret = drm_atomic_add_affected_planes(state, &crtc->base);
- if (ret)
- return ret;
-
- memset(crtc_state, 0, sizeof(*crtc_state));
- crtc_state->base.crtc = &crtc->base;
- crtc_state->base.state = state;
-
- crtc_state->quirks |= PIPE_CONFIG_QUIRK_INHERITED_MODE;
-
- crtc_state->base.enable = crtc_state->base.active =
- crtc->base.enabled = dev_priv->display.get_pipe_config(crtc, crtc_state);
-
- /* update transitional state */
- crtc->active = crtc_state->base.active;
- crtc->config = crtc_state;
-
- drm_plane_state = drm_atomic_get_plane_state(state, primary);
- if (IS_ERR(drm_plane_state))
- return PTR_ERR(drm_plane_state);
-
- plane_state = to_intel_plane_state(drm_plane_state);
- plane_state->visible = primary_get_hw_state(crtc);
-
- if (plane_state->visible) {
- primary->crtc = &crtc->base;
- crtc_state->base.plane_mask |= 1 << drm_plane_index(primary);
- } else
- crtc_state->base.plane_mask &= ~(1 << drm_plane_index(primary));
-
- DRM_DEBUG_KMS("[CRTC:%d] hw state readout: %s\n",
- crtc->base.base.id,
- crtc_state->base.active ? "enabled" : "disabled");
-
- return 0;
-}
-
-static int readout_hw_pll_state(struct drm_atomic_state *state)
-{
- struct drm_i915_private *dev_priv = to_i915(state->dev);
- struct intel_shared_dpll_config *shared_dpll;
+ struct drm_i915_private *dev_priv = dev->dev_private;
+ enum pipe pipe;
struct intel_crtc *crtc;
- struct intel_crtc_state *crtc_state;
+ struct intel_encoder *encoder;
+ struct intel_connector *connector;
int i;
- shared_dpll = intel_atomic_get_shared_dpll_state(state);
+ for_each_intel_crtc(dev, crtc) {
+ struct drm_plane *primary = crtc->base.primary;
+ struct intel_plane_state *plane_state;
+
+ memset(crtc->config, 0, sizeof(*crtc->config));
+ crtc->config->base.crtc = &crtc->base;
+
+ crtc->config->quirks |= PIPE_CONFIG_QUIRK_INHERITED_MODE;
+
+ 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;
+
+ plane_state = to_intel_plane_state(primary->state);
+ plane_state->visible = primary_get_hw_state(crtc);
+
+ DRM_DEBUG_KMS("[CRTC:%d] hw state readout: %s\n",
+ crtc->base.base.id,
+ crtc->active ? "enabled" : "disabled");
+ }
+
for (i = 0; i < dev_priv->num_shared_dpll; i++) {
struct intel_shared_dpll *pll = &dev_priv->shared_dplls[i];
pll->on = pll->get_hw_state(dev_priv, pll,
- &shared_dpll[i].hw_state);
-
+ &pll->config.hw_state);
pll->active = 0;
- shared_dpll[i].crtc_mask = 0;
-
- for_each_intel_crtc(state->dev, crtc) {
- crtc_state = intel_atomic_get_crtc_state(state, crtc);
- if (IS_ERR(crtc_state))
- return PTR_ERR(crtc_state);
-
- if (crtc_state->base.active &&
- crtc_state->shared_dpll == i) {
+ pll->config.crtc_mask = 0;
+ for_each_intel_crtc(dev, crtc) {
+ if (crtc->active && intel_crtc_to_shared_dpll(crtc) == pll) {
pll->active++;
- shared_dpll[i].crtc_mask |=
- 1 << crtc->pipe;
+ pll->config.crtc_mask |= 1 << crtc->pipe;
}
}
DRM_DEBUG_KMS("%s hw state readout: crtc_mask 0x%08x, on %i\n",
- pll->name, shared_dpll[i].crtc_mask,
- pll->on);
+ pll->name, pll->config.crtc_mask, pll->on);
- if (shared_dpll[i].crtc_mask)
+ if (pll->config.crtc_mask)
intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
}
- return 0;
-}
-
-static struct drm_connector_state *
-get_connector_state_for_encoder(struct drm_atomic_state *state,
- struct intel_encoder *encoder)
-{
- struct drm_connector *connector;
- struct drm_connector_state *connector_state;
- int i;
-
- for_each_connector_in_state(state, connector, connector_state, i)
- if (connector_state->best_encoder == &encoder->base)
- return connector_state;
-
- return NULL;
-}
-
-static int readout_hw_connector_encoder_state(struct drm_atomic_state *state)
-{
- struct drm_device *dev = state->dev;
- struct drm_i915_private *dev_priv = to_i915(state->dev);
- struct intel_crtc *crtc;
- struct drm_crtc_state *drm_crtc_state;
- struct intel_crtc_state *crtc_state;
- struct intel_encoder *encoder;
- struct intel_connector *connector;
- struct drm_connector_state *connector_state;
- enum pipe pipe;
-
- for_each_intel_connector(dev, connector) {
- connector_state =
- drm_atomic_get_connector_state(state, &connector->base);
- if (IS_ERR(connector_state))
- return PTR_ERR(connector_state);
-
- if (connector->get_hw_state(connector)) {
- connector->base.dpms = DRM_MODE_DPMS_ON;
- connector->base.encoder = &connector->encoder->base;
- } else {
- connector->base.dpms = DRM_MODE_DPMS_OFF;
- connector->base.encoder = NULL;
- }
-
- /* We'll update the crtc field when reading encoder state */
- connector_state->crtc = NULL;
-
- connector_state->best_encoder = connector->base.encoder;
-
- DRM_DEBUG_KMS("[CONNECTOR:%d:%s] hw state readout: %s\n",
- connector->base.base.id,
- connector->base.name,
- connector->base.encoder ? "enabled" : "disabled");
- }
-
for_each_intel_encoder(dev, encoder) {
pipe = 0;
- connector_state =
- get_connector_state_for_encoder(state, encoder);
-
- encoder->connectors_active = !!connector_state;
-
if (encoder->get_hw_state(encoder, &pipe)) {
- encoder->base.crtc =
- dev_priv->pipe_to_crtc_mapping[pipe];
- crtc = to_intel_crtc(encoder->base.crtc);
-
- drm_crtc_state =
- state->crtc_states[drm_crtc_index(&crtc->base)];
- crtc_state = to_intel_crtc_state(drm_crtc_state);
-
- encoder->get_config(encoder, crtc_state);
-
- if (connector_state)
- connector_state->crtc = &crtc->base;
+ crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
+ encoder->base.crtc = &crtc->base;
+ encoder->get_config(encoder, crtc->config);
} else {
encoder->base.crtc = NULL;
}
+ encoder->connectors_active = false;
DRM_DEBUG_KMS("[ENCODER:%d:%s] hw state readout: %s, pipe %c\n",
encoder->base.base.id,
encoder->base.name,
@@ -15452,42 +15390,20 @@
pipe_name(pipe));
}
- return 0;
-}
-
-static struct drm_atomic_state *
-intel_modeset_readout_hw_state(struct drm_device *dev)
-{
- struct intel_crtc *crtc;
- int ret = 0;
-
- struct drm_atomic_state *state;
-
- state = drm_atomic_state_alloc(dev);
- if (!state)
- return ERR_PTR(-ENOMEM);
-
- state->acquire_ctx = dev->mode_config.acquire_ctx;
-
- for_each_intel_crtc(dev, crtc) {
- ret = readout_hw_crtc_state(state, crtc);
- if (ret)
- goto err_free;
+ for_each_intel_connector(dev, connector) {
+ if (connector->get_hw_state(connector)) {
+ connector->base.dpms = DRM_MODE_DPMS_ON;
+ connector->encoder->connectors_active = true;
+ connector->base.encoder = &connector->encoder->base;
+ } else {
+ connector->base.dpms = DRM_MODE_DPMS_OFF;
+ connector->base.encoder = NULL;
+ }
+ DRM_DEBUG_KMS("[CONNECTOR:%d:%s] hw state readout: %s\n",
+ connector->base.base.id,
+ connector->base.name,
+ connector->base.encoder ? "enabled" : "disabled");
}
-
- ret = readout_hw_pll_state(state);
- if (ret)
- goto err_free;
-
- ret = readout_hw_connector_encoder_state(state);
- if (ret)
- goto err_free;
-
- return state;
-
-err_free:
- drm_atomic_state_free(state);
- return ERR_PTR(ret);
}
/* Scan out the current hw modeset state, sanitizes it and maps it into the drm
@@ -15496,57 +15412,37 @@
bool force_restore)
{
struct drm_i915_private *dev_priv = dev->dev_private;
- struct drm_crtc *crtc;
- struct drm_crtc_state *crtc_state;
+ enum pipe pipe;
+ struct intel_crtc *crtc;
struct intel_encoder *encoder;
- struct drm_atomic_state *state;
- struct intel_shared_dpll_config shared_dplls[I915_NUM_PLLS];
int i;
- state = intel_modeset_readout_hw_state(dev);
- if (IS_ERR(state)) {
- DRM_ERROR("Failed to read out hw state\n");
- return;
+ intel_modeset_readout_hw_state(dev);
+
+ /*
+ * Now that we have the config, copy it to each CRTC struct
+ * Note that this could go away if we move to using crtc_config
+ * checking everywhere.
+ */
+ for_each_intel_crtc(dev, crtc) {
+ if (crtc->active && i915.fastboot) {
+ intel_mode_from_pipe_config(&crtc->base.mode,
+ crtc->config);
+ DRM_DEBUG_KMS("[CRTC:%d] found active mode: ",
+ crtc->base.base.id);
+ drm_mode_debug_printmodeline(&crtc->base.mode);
+ }
}
- drm_atomic_helper_swap_state(dev, state);
-
- /* swap sw/hw dpll state */
- intel_atomic_duplicate_dpll_state(dev_priv, shared_dplls);
- intel_shared_dpll_commit(state);
- memcpy(to_intel_atomic_state(state)->shared_dpll,
- shared_dplls, sizeof(*shared_dplls) * dev_priv->num_shared_dpll);
-
/* HW state is read out, now we need to sanitize this mess. */
for_each_intel_encoder(dev, encoder) {
intel_sanitize_encoder(encoder);
}
- for_each_crtc_in_state(state, crtc, crtc_state, i) {
- struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-
- /* prevent unnneeded restores with force_restore */
- crtc_state->active_changed =
- crtc_state->mode_changed =
- crtc_state->planes_changed = false;
-
- if (crtc->enabled) {
- intel_mode_from_pipe_config(&crtc->state->mode,
- to_intel_crtc_state(crtc->state));
-
- drm_mode_copy(&crtc->mode, &crtc->state->mode);
- drm_mode_copy(&crtc->hwmode,
- &crtc->state->adjusted_mode);
- }
-
- intel_sanitize_crtc(intel_crtc);
-
- /*
- * sanitize_crtc may have forced an update of crtc->state,
- * so reload in intel_dump_pipe_config
- */
- intel_dump_pipe_config(intel_crtc,
- to_intel_crtc_state(crtc->state),
+ for_each_pipe(dev_priv, pipe) {
+ crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
+ intel_sanitize_crtc(crtc);
+ intel_dump_pipe_config(crtc, crtc->config,
"[setup_hw_state]");
}
@@ -15570,17 +15466,20 @@
ilk_wm_get_hw_state(dev);
if (force_restore) {
- int ret;
-
i915_redisable_vga(dev);
- ret = intel_set_mode(state);
- if (ret) {
- DRM_ERROR("Failed to restore previous mode\n");
- drm_atomic_state_free(state);
+ /*
+ * We need to use raw interfaces for restoring state to avoid
+ * checking (bogus) intermediate states.
+ */
+ for_each_pipe(dev_priv, pipe) {
+ struct drm_crtc *crtc =
+ dev_priv->pipe_to_crtc_mapping[pipe];
+
+ intel_crtc_restore_mode(crtc);
}
} else {
- drm_atomic_state_free(state);
+ intel_modeset_update_staged_output_state(dev);
}
intel_modeset_check_state(dev);