On Wed, May 20, 2015 at 06:04:27PM +0200, maarten.lankhorst@xxxxxxxxxxxxxxx wrote: > From: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@xxxxxxxxx> > > To make this work we load the new hardware state into the > atomic_state, then swap it with the sw state. > > This lets us change the force restore path in setup_hw_state() > to use a single call to intel_mode_set() to restore all the > previous state. > > As a nice bonus this kills off encoder->new_encoder, > connector->new_enabled and crtc->new_enabled. They were used only > to restore the state after a modeset. > > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@xxxxxxxxx> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx> > --- > drivers/gpu/drm/i915/intel_atomic.c | 2 +- > drivers/gpu/drm/i915/intel_display.c | 377 ++++++++++++++++++++++------------- > drivers/gpu/drm/i915/intel_drv.h | 14 +- > 3 files changed, 241 insertions(+), 152 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c > index 83078763ba14..17bf9e80c557 100644 > --- a/drivers/gpu/drm/i915/intel_atomic.c > +++ b/drivers/gpu/drm/i915/intel_atomic.c > @@ -395,7 +395,7 @@ int intel_atomic_setup_scalers(struct drm_device *dev, > return 0; > } > > -static void > +void > intel_atomic_duplicate_dpll_state(struct drm_i915_private *dev_priv, > struct intel_shared_dpll_config *shared_dpll) > { > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index e7aa8610cbdc..a42e7d7bf86b 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -9639,7 +9639,7 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector, > retry: > ret = drm_modeset_lock(&config->connection_mutex, ctx); > if (ret) > - goto fail_unlock; > + goto fail; > > /* > * Algorithm gets a little messy: > @@ -9657,10 +9657,10 @@ retry: > > ret = drm_modeset_lock(&crtc->mutex, ctx); > if (ret) > - goto fail_unlock; > + goto fail; > ret = drm_modeset_lock(&crtc->primary->mutex, ctx); > if (ret) > - goto fail_unlock; > + goto fail; > > old->dpms_mode = connector->dpms; > old->load_detect_temp = false; > @@ -9679,9 +9679,6 @@ retry: > 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; > @@ -9692,20 +9689,17 @@ retry: > */ > if (!crtc) { > DRM_DEBUG_KMS("no pipe available for load-detect\n"); > - goto fail_unlock; > + goto fail; > } > > ret = drm_modeset_lock(&crtc->mutex, ctx); > if (ret) > - goto fail_unlock; > + goto fail; > ret = drm_modeset_lock(&crtc->primary->mutex, ctx); > if (ret) > - goto fail_unlock; > - intel_encoder->new_crtc = to_intel_crtc(crtc); > - to_intel_connector(connector)->new_encoder = intel_encoder; > + goto fail; > > 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; > @@ -9773,9 +9767,7 @@ retry: > intel_wait_for_vblank(dev, intel_crtc->pipe); > return true; > > - fail: > - intel_crtc->new_enabled = crtc->state->enable; > -fail_unlock: > +fail: > drm_atomic_state_free(state); > state = NULL; > > @@ -9821,10 +9813,6 @@ void intel_release_load_detect_pipe(struct drm_connector *connector, > 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; > > @@ -10969,33 +10957,6 @@ static const struct drm_crtc_helper_funcs intel_helper_funcs = { > .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. > @@ -11526,7 +11487,6 @@ intel_modeset_update_state(struct drm_atomic_state *state) > } > > 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) { > @@ -11832,11 +11792,14 @@ check_connector_state(struct drm_device *dev) > 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(&connector->new_encoder->base != connector->base.encoder, > + I915_STATE_WARN(state->best_encoder != encoder, > "connector's staged encoder doesn't match current encoder\n"); > } > } > @@ -11856,8 +11819,6 @@ check_encoder_state(struct drm_device *dev) > 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"); > > @@ -11867,6 +11828,9 @@ check_encoder_state(struct drm_device *dev) > 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 > @@ -12296,11 +12260,11 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc) > * 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->new_crtc->base != crtc) > + if (encoder->base.crtc != crtc) > continue; > > for_each_intel_connector(dev, connector) { > - if (connector->new_encoder != encoder) > + if (connector->base.state->best_encoder != &encoder->base) > continue; > > connector_state = drm_atomic_get_connector_state(state, &connector->base); > @@ -12313,14 +12277,10 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc) > } > > 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", > @@ -12329,9 +12289,6 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc) > 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); > } > @@ -14552,99 +14509,224 @@ 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.enabled) > return false; > > return I915_READ(DSPCNTR(crtc->plane)) & DISPLAY_PLANE_ENABLE; > } > > -static void intel_modeset_readout_hw_state(struct drm_device *dev) > +static int readout_hw_crtc_state(struct drm_atomic_state *state, > + struct intel_crtc *crtc) > { > - struct drm_i915_private *dev_priv = dev->dev_private; > - enum pipe pipe; > - struct intel_crtc *crtc; > - struct intel_encoder *encoder; > - struct intel_connector *connector; > - int i; > + 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; > > - for_each_intel_crtc(dev, crtc) { > - struct drm_plane *primary = crtc->base.primary; > - struct intel_plane_state *plane_state; > + crtc_state = intel_atomic_get_crtc_state(state, crtc); > + if (IS_ERR(crtc_state)) > + return PTR_ERR(crtc_state); > > - memset(crtc->config, 0, sizeof(*crtc->config)); > - crtc->config->base.crtc = &crtc->base; > + ret = drm_atomic_add_affected_planes(state, &crtc->base); > + if (ret) > + return ret; Do we actually try to handle non-primary planes in this function? If I'm following this properly, at bootup we won't add any sprite or cursor planes here, even though it's possible that a graphics firmware has turned some of them on. It seems like our sw and hw states will still be a bit inconsistent in that case. Matt > > - crtc->config->quirks |= PIPE_CONFIG_QUIRK_INHERITED_MODE; > + memset(crtc_state, 0, sizeof(*crtc_state)); > + crtc_state->base.crtc = &crtc->base; > + crtc_state->base.state = state; > > - crtc->active = dev_priv->display.get_pipe_config(crtc, > - crtc->config); > + crtc_state->quirks |= PIPE_CONFIG_QUIRK_INHERITED_MODE; > > - crtc->base.state->enable = crtc->active; > - crtc->base.state->active = crtc->active; > - crtc->base.enabled = crtc->active; > + crtc_state->base.enable = crtc_state->base.active = > + crtc->base.enabled = dev_priv->display.get_pipe_config(crtc, crtc_state); > > - plane_state = to_intel_plane_state(primary->state); > - plane_state->visible = primary_get_hw_state(crtc); > + /* update transitional state */ > + crtc->active = crtc_state->base.active; > + crtc->config = crtc_state; > > - DRM_DEBUG_KMS("[CRTC:%d] hw state readout: %s\n", > - crtc->base.base.id, > - crtc->active ? "enabled" : "disabled"); > - } > + 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) > + 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 intel_crtc *crtc; > + struct intel_crtc_state *crtc_state; > + int i; > > + shared_dpll = intel_atomic_get_shared_dpll_state(state); > 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, > - &pll->config.hw_state); > + &shared_dpll[i].hw_state); > + > pll->active = 0; > - pll->config.crtc_mask = 0; > - for_each_intel_crtc(dev, crtc) { > - if (crtc->active && intel_crtc_to_shared_dpll(crtc) == pll) { > + 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->active++; > - pll->config.crtc_mask |= 1 << crtc->pipe; > + shared_dpll[i].crtc_mask |= > + 1 << crtc->pipe; > } > } > > DRM_DEBUG_KMS("%s hw state readout: crtc_mask 0x%08x, on %i\n", > - pll->name, pll->config.crtc_mask, pll->on); > + pll->name, shared_dpll[i].crtc_mask, > + pll->on); > > - if (pll->config.crtc_mask) > + if (shared_dpll[i].crtc_mask) > intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS); > } > > - for_each_intel_encoder(dev, encoder) { > - pipe = 0; > + return 0; > +} > > - if (encoder->get_hw_state(encoder, &pipe)) { > - 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; > - } > +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; > > - encoder->connectors_active = false; > - DRM_DEBUG_KMS("[ENCODER:%d:%s] hw state readout: %s, pipe %c\n", > - encoder->base.base.id, > - encoder->base.name, > - encoder->base.crtc ? "enabled" : "disabled", > - pipe_name(pipe)); > - } > + 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->encoder->connectors_active = true; > 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; > + } else { > + encoder->base.crtc = NULL; > + } > + > + DRM_DEBUG_KMS("[ENCODER:%d:%s] hw state readout: %s, pipe %c\n", > + encoder->base.base.id, > + encoder->base.name, > + encoder->base.crtc ? "enabled" : "disabled", > + 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; > + } > + > + 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 > @@ -14653,37 +14735,57 @@ void intel_modeset_setup_hw_state(struct drm_device *dev, > bool force_restore) > { > struct drm_i915_private *dev_priv = dev->dev_private; > - enum pipe pipe; > - struct intel_crtc *crtc; > + struct drm_crtc *crtc; > + struct drm_crtc_state *crtc_state; > struct intel_encoder *encoder; > + struct drm_atomic_state *state; > + struct intel_shared_dpll_config shared_dplls[I915_NUM_PLLS]; > int i; > > - 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); > - } > + state = intel_modeset_readout_hw_state(dev); > + if (IS_ERR(state)) { > + DRM_ERROR("Failed to read out hw state\n"); > + return; > } > > + 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_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, > + 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), > "[setup_hw_state]"); > } > > @@ -14707,20 +14809,17 @@ void intel_modeset_setup_hw_state(struct drm_device *dev, > ilk_wm_get_hw_state(dev); > > if (force_restore) { > - i915_redisable_vga(dev); > + int ret; > > - /* > - * 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]; > + i915_redisable_vga(dev); > > - intel_crtc_restore_mode(crtc); > + ret = intel_set_mode(state); > + if (ret) { > + DRM_ERROR("Failed to restore previous mode\n"); > + drm_atomic_state_free(state); > } > } else { > - intel_modeset_update_staged_output_state(dev); > + drm_atomic_state_free(state); > } > > intel_modeset_check_state(dev); > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 7012c67de8d5..65f5f3d41b5a 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -130,11 +130,6 @@ struct intel_fbdev { > > struct intel_encoder { > struct drm_encoder base; > - /* > - * The new crtc this encoder will be driven from. Only differs from > - * base->crtc while a modeset is in progress. > - */ > - struct intel_crtc *new_crtc; > > enum intel_output_type type; > unsigned int cloneable; > @@ -195,12 +190,6 @@ struct intel_connector { > */ > struct intel_encoder *encoder; > > - /* > - * The new encoder this connector will be driven. Only differs from > - * encoder while a modeset is in progress. > - */ > - struct intel_encoder *new_encoder; > - > /* Reads out the current hw, returning true if the connector is enabled > * and active (i.e. dpms ON state). */ > bool (*get_hw_state)(struct intel_connector *); > @@ -534,7 +523,6 @@ struct intel_crtc { > > struct intel_initial_plane_config plane_config; > struct intel_crtc_state *config; > - bool new_enabled; > > /* reset counter value when the last flip was submitted */ > unsigned int reset_counter; > @@ -1414,6 +1402,8 @@ struct drm_atomic_state *intel_atomic_state_alloc(struct drm_device *dev); > void intel_atomic_state_clear(struct drm_atomic_state *); > struct intel_shared_dpll_config * > intel_atomic_get_shared_dpll_state(struct drm_atomic_state *s); > +void intel_atomic_duplicate_dpll_state(struct drm_i915_private *, > + struct intel_shared_dpll_config *); > > static inline struct intel_crtc_state * > intel_atomic_get_crtc_state(struct drm_atomic_state *state, > -- > 2.1.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Matt Roper Graphics Software Engineer IoTG Platform Enabling & Development Intel Corporation (916) 356-2795 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx