| Message ID | 1436252911-5703-11-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jul 07, 2015 at 09:08:21AM +0200, Maarten Lankhorst wrote: > Instead of all the ad-hoc updating, duplicate the old state first before > reading out the hw state, then restore it. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_drv.c | 2 +- > drivers/gpu/drm/i915/i915_drv.h | 3 +- > drivers/gpu/drm/i915/intel_display.c | 153 +++++++++++++++++------------------ > drivers/gpu/drm/i915/intel_drv.h | 12 --- > drivers/gpu/drm/i915/intel_lvds.c | 2 +- > 5 files changed, 76 insertions(+), 96 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index e44dc0d6656f..db48aee7f140 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -741,7 +741,7 @@ static int i915_drm_resume(struct drm_device *dev) > spin_unlock_irq(&dev_priv->irq_lock); > > drm_modeset_lock_all(dev); > - intel_modeset_setup_hw_state(dev, true); > + intel_display_resume(dev); > drm_modeset_unlock_all(dev); > > intel_dp_mst_resume(dev); > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 093d6421dddf..2a78a0ee0f97 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -3278,8 +3278,7 @@ extern void intel_modeset_gem_init(struct drm_device *dev); > extern void intel_modeset_cleanup(struct drm_device *dev); > extern void intel_connector_unregister(struct intel_connector *); > extern int intel_modeset_vga_set_state(struct drm_device *dev, bool state); > -extern void intel_modeset_setup_hw_state(struct drm_device *dev, > - bool force_restore); > +extern void intel_display_resume(struct drm_device *dev); > extern void i915_redisable_vga(struct drm_device *dev); > extern void i915_redisable_vga_power_on(struct drm_device *dev); > extern bool ironlake_set_drps(struct drm_device *dev, u8 val); > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index dc4bdb91ad4d..222d587ed4ea 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -110,6 +110,7 @@ static void skl_init_scalers(struct drm_device *dev, struct intel_crtc *intel_cr > static int i9xx_get_refclk(const struct intel_crtc_state *crtc_state, > int num_connectors); > static void intel_pre_disable_primary(struct drm_crtc *crtc); > +static void intel_modeset_setup_hw_state(struct drm_device *dev); > > static struct intel_encoder *intel_find_encoder(struct intel_connector *connector, int pipe) > { > @@ -3247,7 +3248,7 @@ void intel_finish_reset(struct drm_device *dev) > dev_priv->display.hpd_irq_setup(dev); > spin_unlock_irq(&dev_priv->irq_lock); > > - intel_modeset_setup_hw_state(dev, true); > + intel_display_resume(dev); > > intel_hpd_init(dev_priv); > > @@ -10239,7 +10240,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: > @@ -10257,10 +10258,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; > @@ -10279,9 +10280,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; > @@ -10292,20 +10290,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; > @@ -10373,9 +10368,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; > > @@ -10421,10 +10414,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; load_detect changes should be a separate patch. Or the commit message needs to explain why this needs to be one. > - > connector_state->best_encoder = NULL; > connector_state->crtc = NULL; > > @@ -11827,37 +11816,6 @@ static const struct drm_crtc_helper_funcs intel_helper_funcs = { > .atomic_check = intel_crtc_atomic_check, > }; > > -/** > - * 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; > - } > -} Hm, more stuff squashed in which can be only removed as a consequence of the restore code rework. Separate patch again imo. > - > -/* 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. > - */ > static void intel_modeset_update_connector_atomic_state(struct drm_device *dev) > { > struct intel_connector *connector; > @@ -12297,7 +12255,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) { > @@ -12688,11 +12645,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"); > } > } > @@ -12712,8 +12672,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"); > > @@ -12723,6 +12681,10 @@ 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 Same for adapting the check functions. Probably ok in the removeal of the legacy new_* pointers though. > @@ -13282,11 +13244,12 @@ 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); > @@ -13299,7 +13262,6 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc) > } > > connector_state->crtc = crtc; > - connector_state->best_encoder = &encoder->base; > } > } > > @@ -13311,9 +13273,6 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc) > return; > } > > - crtc_state->base.active = crtc_state->base.enable = > - to_intel_crtc(crtc)->new_enabled; > - > drm_mode_copy(&crtc_state->base.mode, &crtc->mode); > > intel_modeset_setup_plane_state(state, crtc, &crtc->mode, Same about restore_mode & new_* state. > @@ -15112,7 +15071,7 @@ void intel_modeset_init(struct drm_device *dev) > intel_fbc_disable(dev); > > drm_modeset_lock_all(dev); > - intel_modeset_setup_hw_state(dev, false); > + intel_modeset_setup_hw_state(dev); > drm_modeset_unlock_all(dev); > > for_each_intel_crtc(dev, crtc) { > @@ -15500,10 +15459,11 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) > } > } > > -/* Scan out the current hw modeset state, sanitizes it and maps it into the drm > - * and i915 state tracking structures. */ > -void intel_modeset_setup_hw_state(struct drm_device *dev, > - bool force_restore) > +/* Scan out the current hw modeset state, > + * and sanitizes it to the current state > + */ > +static void > +intel_modeset_setup_hw_state(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > enum pipe pipe; > @@ -15545,25 +15505,58 @@ void intel_modeset_setup_hw_state(struct drm_device *dev, > skl_wm_get_hw_state(dev); > else if (HAS_PCH_SPLIT(dev)) > ilk_wm_get_hw_state(dev); > +} > > - if (force_restore) { > - i915_redisable_vga(dev); > +void intel_display_resume(struct drm_device *dev) > +{ > + struct drm_atomic_state *state = drm_atomic_state_alloc(dev); Putting real functions into initializers is a bit surprising, imo better on it's own line right before the check. > + struct intel_connector *conn; > + struct intel_plane *plane; > + struct drm_crtc *crtc; > + 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]; > + if (!state) debug output missing that the state alloc failed. Perhaps just goto fail; since state_free can cope with a NULL state. > + return; > > - intel_crtc_restore_mode(crtc); > - } > - } else { > - intel_modeset_update_staged_output_state(dev); > + state->acquire_ctx = dev->mode_config.acquire_ctx; > + > + /* preserve complete old state, including dpll */ > + intel_atomic_get_shared_dpll_state(state); > + > + for_each_crtc(dev, crtc) { > + struct drm_crtc_state *crtc_state = > + drm_atomic_get_crtc_state(state, crtc); > + > + ret = PTR_ERR_OR_ZERO(crtc_state); > + if (ret) > + goto err; > + > + /* force a restore */ > + crtc_state->mode_changed = true; > + } > + > + for_each_intel_plane(dev, plane) { > + ret = PTR_ERR_OR_ZERO(drm_atomic_get_plane_state(state, &plane->base)); > + if (ret) > + goto err; > + } > + > + for_each_intel_connector(dev, conn) { > + ret = PTR_ERR_OR_ZERO(drm_atomic_get_connector_state(state, &conn->base)); > + if (ret) > + goto err; > } > > - intel_modeset_check_state(dev); > + intel_modeset_setup_hw_state(dev); > + > + i915_redisable_vga(dev); Since we've only badly bruised escape this trap I think this deserves a comment: /* * WARNING: We can't do a full atomic modeset including * compute/check phase here since especially encoder * compute_config functions depend upon output detection state. * And that's just not yet available at driver load. Therefore we * must read out the entire relevant hw state (including any * driver internal state) faithfully here and only apply the * commit side. */ Hm, makes me think ... should we end up calling just dev->atomic_commit(state) here once atomic modeset is fully working? > + ret = intel_set_mode(state); > + if (!ret) > + return; > + > +err: > + DRM_ERROR("Restoring old state failed with %i\n", ret); > + drm_atomic_state_free(state); > } > > void intel_modeset_gem_init(struct drm_device *dev) > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index c3cea178c809..97b65749f472 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 *); > @@ -550,7 +539,6 @@ struct intel_crtc { > uint32_t cursor_base; > > struct intel_crtc_state *config; > - bool new_enabled; > > /* reset counter value when the last flip was submitted */ > unsigned int reset_counter; > diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c > index ea85547611a5..a63d18680623 100644 > --- a/drivers/gpu/drm/i915/intel_lvds.c > +++ b/drivers/gpu/drm/i915/intel_lvds.c > @@ -452,7 +452,7 @@ static int intel_lid_notify(struct notifier_block *nb, unsigned long val, > */ > if (!HAS_PCH_SPLIT(dev)) { > drm_modeset_lock_all(dev); > - intel_modeset_setup_hw_state(dev, true); > + intel_display_resume(dev); Would imo be nice to mention this small refactoring in the commit message. -Daniel > drm_modeset_unlock_all(dev); > } > > -- > 2.1.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Op 07-07-15 om 11:57 schreef Daniel Vetter: > On Tue, Jul 07, 2015 at 09:08:21AM +0200, Maarten Lankhorst wrote: >> Instead of all the ad-hoc updating, duplicate the old state first before >> reading out the hw state, then restore it. >> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> --- >> drivers/gpu/drm/i915/i915_drv.c | 2 +- >> drivers/gpu/drm/i915/i915_drv.h | 3 +- >> drivers/gpu/drm/i915/intel_display.c | 153 +++++++++++++++++------------------ >> drivers/gpu/drm/i915/intel_drv.h | 12 --- >> drivers/gpu/drm/i915/intel_lvds.c | 2 +- >> 5 files changed, 76 insertions(+), 96 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c >> index e44dc0d6656f..db48aee7f140 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.c >> +++ b/drivers/gpu/drm/i915/i915_drv.c >> @@ -741,7 +741,7 @@ static int i915_drm_resume(struct drm_device *dev) >> spin_unlock_irq(&dev_priv->irq_lock); >> >> drm_modeset_lock_all(dev); >> - intel_modeset_setup_hw_state(dev, true); >> + intel_display_resume(dev); >> drm_modeset_unlock_all(dev); >> >> intel_dp_mst_resume(dev); >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> index 093d6421dddf..2a78a0ee0f97 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -3278,8 +3278,7 @@ extern void intel_modeset_gem_init(struct drm_device *dev); >> extern void intel_modeset_cleanup(struct drm_device *dev); >> extern void intel_connector_unregister(struct intel_connector *); >> extern int intel_modeset_vga_set_state(struct drm_device *dev, bool state); >> -extern void intel_modeset_setup_hw_state(struct drm_device *dev, >> - bool force_restore); >> +extern void intel_display_resume(struct drm_device *dev); >> extern void i915_redisable_vga(struct drm_device *dev); >> extern void i915_redisable_vga_power_on(struct drm_device *dev); >> extern bool ironlake_set_drps(struct drm_device *dev, u8 val); >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index dc4bdb91ad4d..222d587ed4ea 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -110,6 +110,7 @@ static void skl_init_scalers(struct drm_device *dev, struct intel_crtc *intel_cr >> static int i9xx_get_refclk(const struct intel_crtc_state *crtc_state, >> int num_connectors); >> static void intel_pre_disable_primary(struct drm_crtc *crtc); >> +static void intel_modeset_setup_hw_state(struct drm_device *dev); >> >> static struct intel_encoder *intel_find_encoder(struct intel_connector *connector, int pipe) >> { >> @@ -3247,7 +3248,7 @@ void intel_finish_reset(struct drm_device *dev) >> dev_priv->display.hpd_irq_setup(dev); >> spin_unlock_irq(&dev_priv->irq_lock); >> >> - intel_modeset_setup_hw_state(dev, true); >> + intel_display_resume(dev); >> >> intel_hpd_init(dev_priv); >> >> @@ -10239,7 +10240,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: >> @@ -10257,10 +10258,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; >> @@ -10279,9 +10280,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; >> @@ -10292,20 +10290,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; >> @@ -10373,9 +10368,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; >> >> @@ -10421,10 +10414,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; > load_detect changes should be a separate patch. Or the commit message > needs to explain why this needs to be one. These members no longer exist. ;-) all the new_ stuff was to restore things pre-atomic, the atomic updates are good enough here. Making it a separate patch's probably ok. >> - >> connector_state->best_encoder = NULL; >> connector_state->crtc = NULL; >> >> @@ -11827,37 +11816,6 @@ static const struct drm_crtc_helper_funcs intel_helper_funcs = { >> .atomic_check = intel_crtc_atomic_check, >> }; >> >> -/** >> - * 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; >> - } >> -} > Hm, more stuff squashed in which can be only removed as a consequence of > the restore code rework. Separate patch again imo. > >> - >> -/* 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. >> - */ >> static void intel_modeset_update_connector_atomic_state(struct drm_device *dev) >> { >> struct intel_connector *connector; >> @@ -12297,7 +12255,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) { >> @@ -12688,11 +12645,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"); >> } >> } >> @@ -12712,8 +12672,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"); >> >> @@ -12723,6 +12681,10 @@ 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 > Same for adapting the check functions. Probably ok in the removeal of the > legacy new_* pointers though. > >> @@ -13282,11 +13244,12 @@ 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); >> @@ -13299,7 +13262,6 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc) >> } >> >> connector_state->crtc = crtc; >> - connector_state->best_encoder = &encoder->base; >> } >> } >> >> @@ -13311,9 +13273,6 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc) >> return; >> } >> >> - crtc_state->base.active = crtc_state->base.enable = >> - to_intel_crtc(crtc)->new_enabled; >> - >> drm_mode_copy(&crtc_state->base.mode, &crtc->mode); >> >> intel_modeset_setup_plane_state(state, crtc, &crtc->mode, > Same about restore_mode & new_* state. They were for tracking purposes and now gone. >> @@ -15112,7 +15071,7 @@ void intel_modeset_init(struct drm_device *dev) >> intel_fbc_disable(dev); >> >> drm_modeset_lock_all(dev); >> - intel_modeset_setup_hw_state(dev, false); >> + intel_modeset_setup_hw_state(dev); >> drm_modeset_unlock_all(dev); >> >> for_each_intel_crtc(dev, crtc) { >> @@ -15500,10 +15459,11 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) >> } >> } >> >> -/* Scan out the current hw modeset state, sanitizes it and maps it into the drm >> - * and i915 state tracking structures. */ >> -void intel_modeset_setup_hw_state(struct drm_device *dev, >> - bool force_restore) >> +/* Scan out the current hw modeset state, >> + * and sanitizes it to the current state >> + */ >> +static void >> +intel_modeset_setup_hw_state(struct drm_device *dev) >> { >> struct drm_i915_private *dev_priv = dev->dev_private; >> enum pipe pipe; >> @@ -15545,25 +15505,58 @@ void intel_modeset_setup_hw_state(struct drm_device *dev, >> skl_wm_get_hw_state(dev); >> else if (HAS_PCH_SPLIT(dev)) >> ilk_wm_get_hw_state(dev); >> +} >> >> - if (force_restore) { >> - i915_redisable_vga(dev); >> +void intel_display_resume(struct drm_device *dev) >> +{ >> + struct drm_atomic_state *state = drm_atomic_state_alloc(dev); > Putting real functions into initializers is a bit surprising, imo better > on it's own line right before the check. Ok. >> + struct intel_connector *conn; >> + struct intel_plane *plane; >> + struct drm_crtc *crtc; >> + 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]; >> + if (!state) > debug output missing that the state alloc failed. Perhaps just goto fail; > since state_free can cope with a NULL state. It can only fail because of kmalloc, which prints its own warnings. >> + return; >> >> - intel_crtc_restore_mode(crtc); >> - } >> - } else { >> - intel_modeset_update_staged_output_state(dev); >> + state->acquire_ctx = dev->mode_config.acquire_ctx; >> + >> + /* preserve complete old state, including dpll */ >> + intel_atomic_get_shared_dpll_state(state); >> + >> + for_each_crtc(dev, crtc) { >> + struct drm_crtc_state *crtc_state = >> + drm_atomic_get_crtc_state(state, crtc); >> + >> + ret = PTR_ERR_OR_ZERO(crtc_state); >> + if (ret) >> + goto err; >> + >> + /* force a restore */ >> + crtc_state->mode_changed = true; >> + } >> + >> + for_each_intel_plane(dev, plane) { >> + ret = PTR_ERR_OR_ZERO(drm_atomic_get_plane_state(state, &plane->base)); >> + if (ret) >> + goto err; >> + } >> + >> + for_each_intel_connector(dev, conn) { >> + ret = PTR_ERR_OR_ZERO(drm_atomic_get_connector_state(state, &conn->base)); >> + if (ret) >> + goto err; >> } >> >> - intel_modeset_check_state(dev); >> + intel_modeset_setup_hw_state(dev); >> + >> + i915_redisable_vga(dev); > Since we've only badly bruised escape this trap I think this deserves a > comment: > > /* > * WARNING: We can't do a full atomic modeset including > * compute/check phase here since especially encoder > * compute_config functions depend upon output detection state. > * And that's just not yet available at driver load. Therefore we > * must read out the entire relevant hw state (including any > * driver internal state) faithfully here and only apply the > * commit side. > */ > > Hm, makes me think ... should we end up calling just dev->atomic_commit(state) here > once atomic modeset is fully working? Not for initial hw readout unless you want to call detect in this function for all encoders.. resume's fine probably. >> + ret = intel_set_mode(state); >> + if (!ret) >> + return; >> + >> +err: >> + DRM_ERROR("Restoring old state failed with %i\n", ret); >> + drm_atomic_state_free(state); >> } >> >> void intel_modeset_gem_init(struct drm_device *dev) >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >> index c3cea178c809..97b65749f472 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 *); >> @@ -550,7 +539,6 @@ struct intel_crtc { >> uint32_t cursor_base; >> >> struct intel_crtc_state *config; >> - bool new_enabled; >> >> /* reset counter value when the last flip was submitted */ >> unsigned int reset_counter; >> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c >> index ea85547611a5..a63d18680623 100644 >> --- a/drivers/gpu/drm/i915/intel_lvds.c >> +++ b/drivers/gpu/drm/i915/intel_lvds.c >> @@ -452,7 +452,7 @@ static int intel_lid_notify(struct notifier_block *nb, unsigned long val, >> */ >> if (!HAS_PCH_SPLIT(dev)) { >> drm_modeset_lock_all(dev); >> - intel_modeset_setup_hw_state(dev, true); >> + intel_display_resume(dev); > Would imo be nice to mention this small refactoring in the commit message. > Ok. ~Maarten
On Tue, Jul 07, 2015 at 12:33:25PM +0200, Maarten Lankhorst wrote: > Op 07-07-15 om 11:57 schreef Daniel Vetter: > > On Tue, Jul 07, 2015 at 09:08:21AM +0200, Maarten Lankhorst wrote: > >> @@ -10421,10 +10414,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; > > load_detect changes should be a separate patch. Or the commit message > > needs to explain why this needs to be one. > These members no longer exist. ;-) all the new_ stuff was to restore things pre-atomic, the atomic updates are good enough here. > > Making it a separate patch's probably ok. Yeah I think splitting out the new_* removal would address most of my concerns here. [snip] > >> + struct intel_connector *conn; > >> + struct intel_plane *plane; > >> + struct drm_crtc *crtc; > >> + 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]; > >> + if (!state) > > debug output missing that the state alloc failed. Perhaps just goto fail; > > since state_free can cope with a NULL state. > It can only fail because of kmalloc, which prints its own warnings. Might still be useful just to have unified error reporting - you need to guess the caller otherwise which would make debug (if this ever happesn) harder. But really just a bikeshed. > > >> + return; > >> > >> - intel_crtc_restore_mode(crtc); > >> - } > >> - } else { > >> - intel_modeset_update_staged_output_state(dev); > >> + state->acquire_ctx = dev->mode_config.acquire_ctx; > >> + > >> + /* preserve complete old state, including dpll */ > >> + intel_atomic_get_shared_dpll_state(state); > >> + > >> + for_each_crtc(dev, crtc) { > >> + struct drm_crtc_state *crtc_state = > >> + drm_atomic_get_crtc_state(state, crtc); > >> + > >> + ret = PTR_ERR_OR_ZERO(crtc_state); > >> + if (ret) > >> + goto err; > >> + > >> + /* force a restore */ > >> + crtc_state->mode_changed = true; > >> + } > >> + > >> + for_each_intel_plane(dev, plane) { > >> + ret = PTR_ERR_OR_ZERO(drm_atomic_get_plane_state(state, &plane->base)); > >> + if (ret) > >> + goto err; > >> + } > >> + > >> + for_each_intel_connector(dev, conn) { > >> + ret = PTR_ERR_OR_ZERO(drm_atomic_get_connector_state(state, &conn->base)); > >> + if (ret) > >> + goto err; > >> } > >> > >> - intel_modeset_check_state(dev); > >> + intel_modeset_setup_hw_state(dev); > >> + > >> + i915_redisable_vga(dev); > > Since we've only badly bruised escape this trap I think this deserves a > > comment: > > > > /* > > * WARNING: We can't do a full atomic modeset including > > * compute/check phase here since especially encoder > > * compute_config functions depend upon output detection state. > > * And that's just not yet available at driver load. Therefore we > > * must read out the entire relevant hw state (including any > > * driver internal state) faithfully here and only apply the > > * commit side. > > */ > > > > Hm, makes me think ... should we end up calling just dev->atomic_commit(state) here > > once atomic modeset is fully working? > Not for initial hw readout unless you want to call detect in this function for all encoders.. resume's fine probably. I meant calling dev->mode_config.funcs->atomic_commit(state) directly, without calling ->atomic_check at all. That should avoid any state recomputation (otherwise our check/commit split is botched) and hence be exactly what we need here. I didn't check how close intel_set_mode is compared our ->atomic_commit implementation after this series (didn't apply them all). But I think from a semantic pov those two should match. -Daniel
On Tue, Jul 07, 2015 at 03:14:57PM +0200, Daniel Vetter wrote: > On Tue, Jul 07, 2015 at 12:33:25PM +0200, Maarten Lankhorst wrote: > > Op 07-07-15 om 11:57 schreef Daniel Vetter: > > > On Tue, Jul 07, 2015 at 09:08:21AM +0200, Maarten Lankhorst wrote: > > > Since we've only badly bruised escape this trap I think this deserves a > > > comment: > > > > > > /* > > > * WARNING: We can't do a full atomic modeset including > > > * compute/check phase here since especially encoder > > > * compute_config functions depend upon output detection state. > > > * And that's just not yet available at driver load. Therefore we > > > * must read out the entire relevant hw state (including any > > > * driver internal state) faithfully here and only apply the > > > * commit side. > > > */ > > > > > > Hm, makes me think ... should we end up calling just dev->atomic_commit(state) here > > > once atomic modeset is fully working? > > Not for initial hw readout unless you want to call detect in this function for all encoders.. resume's fine probably. > > I meant calling dev->mode_config.funcs->atomic_commit(state) directly, > without calling ->atomic_check at all. That should avoid any state > recomputation (otherwise our check/commit split is botched) and hence be > exactly what we need here. I didn't check how close intel_set_mode is > compared our ->atomic_commit implementation after this series (didn't > apply them all). But I think from a semantic pov those two should match. Ah I mixed up intel_set_mode with intel_crtc_set_config, which does a more elaborate compute_config. I guess this is something we still need to untangle when we replace all the existing legacy entry points with the legacy2atomic helpers. Probably needs some shuffling of responsibilities betwen atomic_check and atomic_commit. -Daniel
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index e44dc0d6656f..db48aee7f140 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -741,7 +741,7 @@ static int i915_drm_resume(struct drm_device *dev) spin_unlock_irq(&dev_priv->irq_lock); drm_modeset_lock_all(dev); - intel_modeset_setup_hw_state(dev, true); + intel_display_resume(dev); drm_modeset_unlock_all(dev); intel_dp_mst_resume(dev); diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 093d6421dddf..2a78a0ee0f97 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -3278,8 +3278,7 @@ extern void intel_modeset_gem_init(struct drm_device *dev); extern void intel_modeset_cleanup(struct drm_device *dev); extern void intel_connector_unregister(struct intel_connector *); extern int intel_modeset_vga_set_state(struct drm_device *dev, bool state); -extern void intel_modeset_setup_hw_state(struct drm_device *dev, - bool force_restore); +extern void intel_display_resume(struct drm_device *dev); extern void i915_redisable_vga(struct drm_device *dev); extern void i915_redisable_vga_power_on(struct drm_device *dev); extern bool ironlake_set_drps(struct drm_device *dev, u8 val); diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index dc4bdb91ad4d..222d587ed4ea 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -110,6 +110,7 @@ static void skl_init_scalers(struct drm_device *dev, struct intel_crtc *intel_cr static int i9xx_get_refclk(const struct intel_crtc_state *crtc_state, int num_connectors); static void intel_pre_disable_primary(struct drm_crtc *crtc); +static void intel_modeset_setup_hw_state(struct drm_device *dev); static struct intel_encoder *intel_find_encoder(struct intel_connector *connector, int pipe) { @@ -3247,7 +3248,7 @@ void intel_finish_reset(struct drm_device *dev) dev_priv->display.hpd_irq_setup(dev); spin_unlock_irq(&dev_priv->irq_lock); - intel_modeset_setup_hw_state(dev, true); + intel_display_resume(dev); intel_hpd_init(dev_priv); @@ -10239,7 +10240,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: @@ -10257,10 +10258,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; @@ -10279,9 +10280,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; @@ -10292,20 +10290,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; @@ -10373,9 +10368,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; @@ -10421,10 +10414,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; @@ -11827,37 +11816,6 @@ static const struct drm_crtc_helper_funcs intel_helper_funcs = { .atomic_check = intel_crtc_atomic_check, }; -/** - * 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. - */ static void intel_modeset_update_connector_atomic_state(struct drm_device *dev) { struct intel_connector *connector; @@ -12297,7 +12255,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) { @@ -12688,11 +12645,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"); } } @@ -12712,8 +12672,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"); @@ -12723,6 +12681,10 @@ 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 @@ -13282,11 +13244,12 @@ 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); @@ -13299,7 +13262,6 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc) } connector_state->crtc = crtc; - connector_state->best_encoder = &encoder->base; } } @@ -13311,9 +13273,6 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc) return; } - crtc_state->base.active = crtc_state->base.enable = - to_intel_crtc(crtc)->new_enabled; - drm_mode_copy(&crtc_state->base.mode, &crtc->mode); intel_modeset_setup_plane_state(state, crtc, &crtc->mode, @@ -15112,7 +15071,7 @@ void intel_modeset_init(struct drm_device *dev) intel_fbc_disable(dev); drm_modeset_lock_all(dev); - intel_modeset_setup_hw_state(dev, false); + intel_modeset_setup_hw_state(dev); drm_modeset_unlock_all(dev); for_each_intel_crtc(dev, crtc) { @@ -15500,10 +15459,11 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) } } -/* Scan out the current hw modeset state, sanitizes it and maps it into the drm - * and i915 state tracking structures. */ -void intel_modeset_setup_hw_state(struct drm_device *dev, - bool force_restore) +/* Scan out the current hw modeset state, + * and sanitizes it to the current state + */ +static void +intel_modeset_setup_hw_state(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; enum pipe pipe; @@ -15545,25 +15505,58 @@ void intel_modeset_setup_hw_state(struct drm_device *dev, skl_wm_get_hw_state(dev); else if (HAS_PCH_SPLIT(dev)) ilk_wm_get_hw_state(dev); +} - if (force_restore) { - i915_redisable_vga(dev); +void intel_display_resume(struct drm_device *dev) +{ + struct drm_atomic_state *state = drm_atomic_state_alloc(dev); + struct intel_connector *conn; + struct intel_plane *plane; + struct drm_crtc *crtc; + 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]; + if (!state) + return; - intel_crtc_restore_mode(crtc); - } - } else { - intel_modeset_update_staged_output_state(dev); + state->acquire_ctx = dev->mode_config.acquire_ctx; + + /* preserve complete old state, including dpll */ + intel_atomic_get_shared_dpll_state(state); + + for_each_crtc(dev, crtc) { + struct drm_crtc_state *crtc_state = + drm_atomic_get_crtc_state(state, crtc); + + ret = PTR_ERR_OR_ZERO(crtc_state); + if (ret) + goto err; + + /* force a restore */ + crtc_state->mode_changed = true; + } + + for_each_intel_plane(dev, plane) { + ret = PTR_ERR_OR_ZERO(drm_atomic_get_plane_state(state, &plane->base)); + if (ret) + goto err; + } + + for_each_intel_connector(dev, conn) { + ret = PTR_ERR_OR_ZERO(drm_atomic_get_connector_state(state, &conn->base)); + if (ret) + goto err; } - intel_modeset_check_state(dev); + intel_modeset_setup_hw_state(dev); + + i915_redisable_vga(dev); + ret = intel_set_mode(state); + if (!ret) + return; + +err: + DRM_ERROR("Restoring old state failed with %i\n", ret); + drm_atomic_state_free(state); } void intel_modeset_gem_init(struct drm_device *dev) diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index c3cea178c809..97b65749f472 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 *); @@ -550,7 +539,6 @@ struct intel_crtc { uint32_t cursor_base; struct intel_crtc_state *config; - bool new_enabled; /* reset counter value when the last flip was submitted */ unsigned int reset_counter; diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c index ea85547611a5..a63d18680623 100644 --- a/drivers/gpu/drm/i915/intel_lvds.c +++ b/drivers/gpu/drm/i915/intel_lvds.c @@ -452,7 +452,7 @@ static int intel_lid_notify(struct notifier_block *nb, unsigned long val, */ if (!HAS_PCH_SPLIT(dev)) { drm_modeset_lock_all(dev); - intel_modeset_setup_hw_state(dev, true); + intel_display_resume(dev); drm_modeset_unlock_all(dev); }
Instead of all the ad-hoc updating, duplicate the old state first before reading out the hw state, then restore it. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/i915/i915_drv.c | 2 +- drivers/gpu/drm/i915/i915_drv.h | 3 +- drivers/gpu/drm/i915/intel_display.c | 153 +++++++++++++++++------------------ drivers/gpu/drm/i915/intel_drv.h | 12 --- drivers/gpu/drm/i915/intel_lvds.c | 2 +- 5 files changed, 76 insertions(+), 96 deletions(-)