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.lankho...@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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
