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 <[email protected]>
>> ---
>> 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
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
