Re: [PATCH v2 10/20] drm/i915: Convert suspend/resume to atomic.

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

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