Re: [PATCH v3 15/22] drm/i915: Read hw state into an atomic state struct

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

Op 29-05-15 om 02:55 schreef Matt Roper:
> On Wed, May 20, 2015 at 06:04:27PM +0200, maarten.lankhorst@linux.intel.com wrote:
>> From: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
>>
>> To make this work we load the new hardware state into the
>> atomic_state, then swap it with the sw state.
>>
>> This lets us change the force restore path in setup_hw_state()
>> to use a single call to intel_mode_set() to restore all the
>> previous state.
>>
>> As a nice bonus this kills off encoder->new_encoder,
>> connector->new_enabled and crtc->new_enabled. They were used only
>> to restore the state after a modeset.
>>
>> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_atomic.c  |   2 +-
>>  drivers/gpu/drm/i915/intel_display.c | 377 ++++++++++++++++++++++-------------
>>  drivers/gpu/drm/i915/intel_drv.h     |  14 +-
>>  3 files changed, 241 insertions(+), 152 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
>> index 83078763ba14..17bf9e80c557 100644
>> --- a/drivers/gpu/drm/i915/intel_atomic.c
>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
>> @@ -395,7 +395,7 @@ int intel_atomic_setup_scalers(struct drm_device *dev,
>>  	return 0;
>>  }
>>  
>> -static void
>> +void
>>  intel_atomic_duplicate_dpll_state(struct drm_i915_private *dev_priv,
>>  				  struct intel_shared_dpll_config *shared_dpll)
>>  {
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index e7aa8610cbdc..a42e7d7bf86b 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -9639,7 +9639,7 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector,
>>  retry:
>>  	ret = drm_modeset_lock(&config->connection_mutex, ctx);
>>  	if (ret)
>> -		goto fail_unlock;
>> +		goto fail;
>>  
>>  	/*
>>  	 * Algorithm gets a little messy:
>> @@ -9657,10 +9657,10 @@ retry:
>>  
>>  		ret = drm_modeset_lock(&crtc->mutex, ctx);
>>  		if (ret)
>> -			goto fail_unlock;
>> +			goto fail;
>>  		ret = drm_modeset_lock(&crtc->primary->mutex, ctx);
>>  		if (ret)
>> -			goto fail_unlock;
>> +			goto fail;
>>  
>>  		old->dpms_mode = connector->dpms;
>>  		old->load_detect_temp = false;
>> @@ -9679,9 +9679,6 @@ retry:
>>  			continue;
>>  		if (possible_crtc->state->enable)
>>  			continue;
>> -		/* This can occur when applying the pipe A quirk on resume. */
>> -		if (to_intel_crtc(possible_crtc)->new_enabled)
>> -			continue;
>>  
>>  		crtc = possible_crtc;
>>  		break;
>> @@ -9692,20 +9689,17 @@ retry:
>>  	 */
>>  	if (!crtc) {
>>  		DRM_DEBUG_KMS("no pipe available for load-detect\n");
>> -		goto fail_unlock;
>> +		goto fail;
>>  	}
>>  
>>  	ret = drm_modeset_lock(&crtc->mutex, ctx);
>>  	if (ret)
>> -		goto fail_unlock;
>> +		goto fail;
>>  	ret = drm_modeset_lock(&crtc->primary->mutex, ctx);
>>  	if (ret)
>> -		goto fail_unlock;
>> -	intel_encoder->new_crtc = to_intel_crtc(crtc);
>> -	to_intel_connector(connector)->new_encoder = intel_encoder;
>> +		goto fail;
>>  
>>  	intel_crtc = to_intel_crtc(crtc);
>> -	intel_crtc->new_enabled = true;
>>  	old->dpms_mode = connector->dpms;
>>  	old->load_detect_temp = true;
>>  	old->release_fb = NULL;
>> @@ -9773,9 +9767,7 @@ retry:
>>  	intel_wait_for_vblank(dev, intel_crtc->pipe);
>>  	return true;
>>  
>> - fail:
>> -	intel_crtc->new_enabled = crtc->state->enable;
>> -fail_unlock:
>> +fail:
>>  	drm_atomic_state_free(state);
>>  	state = NULL;
>>  
>> @@ -9821,10 +9813,6 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
>>  		if (IS_ERR(crtc_state))
>>  			goto fail;
>>  
>> -		to_intel_connector(connector)->new_encoder = NULL;
>> -		intel_encoder->new_crtc = NULL;
>> -		intel_crtc->new_enabled = false;
>> -
>>  		connector_state->best_encoder = NULL;
>>  		connector_state->crtc = NULL;
>>  
>> @@ -10969,33 +10957,6 @@ static const struct drm_crtc_helper_funcs intel_helper_funcs = {
>>  	.atomic_flush = intel_finish_crtc_commit,
>>  };
>>  
>> -/**
>> - * intel_modeset_update_staged_output_state
>> - *
>> - * Updates the staged output configuration state, e.g. after we've read out the
>> - * current hw state.
>> - */
>> -static void intel_modeset_update_staged_output_state(struct drm_device *dev)
>> -{
>> -	struct intel_crtc *crtc;
>> -	struct intel_encoder *encoder;
>> -	struct intel_connector *connector;
>> -
>> -	for_each_intel_connector(dev, connector) {
>> -		connector->new_encoder =
>> -			to_intel_encoder(connector->base.encoder);
>> -	}
>> -
>> -	for_each_intel_encoder(dev, encoder) {
>> -		encoder->new_crtc =
>> -			to_intel_crtc(encoder->base.crtc);
>> -	}
>> -
>> -	for_each_intel_crtc(dev, crtc) {
>> -		crtc->new_enabled = crtc->base.state->enable;
>> -	}
>> -}
>> -
>>  /* Transitional helper to copy current connector/encoder state to
>>   * connector->state. This is needed so that code that is partially
>>   * converted to atomic does the right thing.
>> @@ -11526,7 +11487,6 @@ intel_modeset_update_state(struct drm_atomic_state *state)
>>  	}
>>  
>>  	drm_atomic_helper_update_legacy_modeset_state(state->dev, state);
>> -	intel_modeset_update_staged_output_state(state->dev);
>>  
>>  	/* Double check state. */
>>  	for_each_crtc(dev, crtc) {
>> @@ -11832,11 +11792,14 @@ check_connector_state(struct drm_device *dev)
>>  	struct intel_connector *connector;
>>  
>>  	for_each_intel_connector(dev, connector) {
>> +		struct drm_encoder *encoder = connector->base.encoder;
>> +		struct drm_connector_state *state = connector->base.state;
>> +
>>  		/* This also checks the encoder/connector hw state with the
>>  		 * ->get_hw_state callbacks. */
>>  		intel_connector_check_state(connector);
>>  
>> -		I915_STATE_WARN(&connector->new_encoder->base != connector->base.encoder,
>> +		I915_STATE_WARN(state->best_encoder != encoder,
>>  		     "connector's staged encoder doesn't match current encoder\n");
>>  	}
>>  }
>> @@ -11856,8 +11819,6 @@ check_encoder_state(struct drm_device *dev)
>>  			      encoder->base.base.id,
>>  			      encoder->base.name);
>>  
>> -		I915_STATE_WARN(&encoder->new_crtc->base != encoder->base.crtc,
>> -		     "encoder's stage crtc doesn't match current crtc\n");
>>  		I915_STATE_WARN(encoder->connectors_active && !encoder->base.crtc,
>>  		     "encoder's active_connectors set, but no crtc\n");
>>  
>> @@ -11867,6 +11828,9 @@ check_encoder_state(struct drm_device *dev)
>>  			enabled = true;
>>  			if (connector->base.dpms != DRM_MODE_DPMS_OFF)
>>  				active = true;
>> +
>> +			I915_STATE_WARN(connector->base.state->crtc != encoder->base.crtc,
>> +			     "encoder's stage crtc doesn't match current crtc\n");
>>  		}
>>  		/*
>>  		 * for MST connectors if we unplug the connector is gone
>> @@ -12296,11 +12260,11 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc)
>>  	 * need to copy the staged config to the atomic state, otherwise the
>>  	 * mode set will just reapply the state the HW is already in. */
>>  	for_each_intel_encoder(dev, encoder) {
>> -		if (&encoder->new_crtc->base != crtc)
>> +		if (encoder->base.crtc != crtc)
>>  			continue;
>>  
>>  		for_each_intel_connector(dev, connector) {
>> -			if (connector->new_encoder != encoder)
>> +			if (connector->base.state->best_encoder != &encoder->base)
>>  				continue;
>>  
>>  			connector_state = drm_atomic_get_connector_state(state, &connector->base);
>> @@ -12313,14 +12277,10 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc)
>>  			}
>>  
>>  			connector_state->crtc = crtc;
>> -			connector_state->best_encoder = &encoder->base;
>>  		}
>>  	}
>>  
>>  	for_each_intel_crtc(dev, intel_crtc) {
>> -		if (intel_crtc->new_enabled == intel_crtc->base.enabled)
>> -			continue;
>> -
>>  		crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
>>  		if (IS_ERR(crtc_state)) {
>>  			DRM_DEBUG_KMS("Failed to add [CRTC:%d] to state: %ld\n",
>> @@ -12329,9 +12289,6 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc)
>>  			continue;
>>  		}
>>  
>> -		crtc_state->base.active = crtc_state->base.enable =
>> -			intel_crtc->new_enabled;
>> -
>>  		if (&intel_crtc->base == crtc)
>>  			drm_mode_copy(&crtc_state->base.mode, &crtc->mode);
>>  	}
>> @@ -14552,99 +14509,224 @@ static bool primary_get_hw_state(struct intel_crtc *crtc)
>>  {
>>  	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
>>  
>> -	if (!crtc->active)
>> +	if (!crtc->base.enabled)
>>  		return false;
>>  
>>  	return I915_READ(DSPCNTR(crtc->plane)) & DISPLAY_PLANE_ENABLE;
>>  }
>>  
>> -static void intel_modeset_readout_hw_state(struct drm_device *dev)
>> +static int readout_hw_crtc_state(struct drm_atomic_state *state,
>> +				 struct intel_crtc *crtc)
>>  {
>> -	struct drm_i915_private *dev_priv = dev->dev_private;
>> -	enum pipe pipe;
>> -	struct intel_crtc *crtc;
>> -	struct intel_encoder *encoder;
>> -	struct intel_connector *connector;
>> -	int i;
>> +	struct drm_i915_private *dev_priv = to_i915(state->dev);
>> +	struct intel_crtc_state *crtc_state;
>> +	struct drm_plane *primary = crtc->base.primary;
>> +	struct drm_plane_state *drm_plane_state;
>> +	struct intel_plane_state *plane_state;
>> +	int ret;
>>  
>> -	for_each_intel_crtc(dev, crtc) {
>> -		struct drm_plane *primary = crtc->base.primary;
>> -		struct intel_plane_state *plane_state;
>> +	crtc_state = intel_atomic_get_crtc_state(state, crtc);
>> +	if (IS_ERR(crtc_state))
>> +		return PTR_ERR(crtc_state);
>>  
>> -		memset(crtc->config, 0, sizeof(*crtc->config));
>> -		crtc->config->base.crtc = &crtc->base;
>> +	ret = drm_atomic_add_affected_planes(state, &crtc->base);
>> +	if (ret)
>> +		return ret;
> Do we actually try to handle non-primary planes in this function?  If
> I'm following this properly, at bootup we won't add any sprite or cursor
> planes here, even though it's possible that a graphics firmware has
> turned some of them on.  It seems like our sw and hw states will still
> be a bit inconsistent in that case.
I'm afraid we don't, but probably should. Adding all possible planes might be better here,
even if we don't track the previous hw state it will cause at least everything to be disabled,
another way of making hw and sw state match up. :-)

~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx