[Intel-gfx] [PATCH v4 19/27] drm/i915: Read hw state into an atomic state struct, v2.

Jani Nikula jani.nikula at linux.intel.com
Tue Jun 9 07:24:28 PDT 2015


On Tue, 09 Jun 2015, Maarten Lankhorst <maarten.lankhorst at linux.intel.com> wrote:
> Hey,
>
> Op 09-06-15 om 13:48 schreef Tvrtko Ursulin:
>>
>> On 06/01/2015 11:50 AM, Maarten Lankhorst wrote:
>>> From: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira at 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.
>>>
>>> Changes since v1:
>>> - Make sure all possible planes are added with their crtc set,
>>>    so they will be turned off on first modeset.
>>>
>>> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira at intel.com>
>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>>
>> This breaks display for me, which is eDP on SKL. At least bisect points to it. A lot of these in the logs:
>>
>> *ERROR* mismatch in dp_m_n.link_m (expected 701594 or 0, found 350797)
>>
>> And the display does not light up.
>
> Yeah, it probably relies on better hw readout. This is partially mitigated by convert to atomic, part 3.
> But it requires 2 additional patches.

Maarten, I'd like to have the fallout from this series fixed before
moving on to merging another big series...

BR,
Jani.


>
> commit 5a97529becb25fabf18a3507a94f892c365a4a1d
> Author: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> Date:   Mon Jun 8 11:31:28 2015 +0200
>
>     drm/i915: update more sw state with hw state during atomic readout
>     
>     I've noticed the following during initial readout:
>     state->adjusted_mode's non crtc_* members were not set,
>     but some code relies hdisplay and vdisplay, so make sure it's
>     set correctly.
>     
>     Also vblank was not enabled because constants were not calculated,
>     this shows up in dmesg as:
>     [drm:drm_vblank_enable] enabling vblank on crtc 0, ret: 0
>     [drm:drm_calc_vbltimestamp_from_scanoutpos] crtc 0: Noop due to uninitialized mode.
>     
>     This commit fixes the regression from the following commit:
>     
>     commit 3bae26eb2991c00670df377cf6c3bc2b0577e82a
>     Author: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira at intel.com>
>     Date:   Mon Jun 1 12:50:03 2015 +0200
>     
>         drm/i915: Read hw state into an atomic state struct, v2.
>     
>     Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90861
>     Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index fb9f07b1e5ca..dc29bab527d7 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14859,8 +14859,9 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>  
>  	/* restore vblank interrupts to correct state */
>  	drm_crtc_vblank_reset(&crtc->base);
> -	if (crtc->active) {
> +	if (crtc->base.state->active) {
>  		update_scanline_offset(crtc);
> +		drm_calc_timestamping_constants(&crtc->base, &crtc->base.hwmode);
>  		drm_crtc_vblank_on(&crtc->base);
>  	}
>  
> @@ -15307,6 +15308,8 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
>  		if (crtc->enabled) {
>  			intel_mode_from_pipe_config(&crtc->state->mode,
>  				to_intel_crtc_state(crtc->state));
> +			intel_mode_from_pipe_config(&crtc->state->adjusted_mode,
> +				to_intel_crtc_state(crtc->state));
>  
>  			drm_mode_copy(&crtc->mode, &crtc->state->mode);
>  			drm_mode_copy(&crtc->hwmode,
>
> commit d0f7e7ae8a151e9d018e2dbf36a5afba812bab4f
> Author: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> Date:   Tue Jun 9 09:01:17 2015 +0200
>
>     drm/i915: only perform a single modeset in intel_modeset_setup_hw_state
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 29ae92e5c8a9..77a553e21a7a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11994,24 +11994,35 @@ static bool intel_fuzzy_clock_check(int clock1, int clock2)
>  static bool
>  intel_pipe_config_compare(struct drm_device *dev,
>  			  struct intel_crtc_state *current_config,
> -			  struct intel_crtc_state *pipe_config)
> +			  struct intel_crtc_state *pipe_config,
> +			  bool check_only)
>  {
> +	bool ret = true;
> +
> +#define INTEL_ERR_OR_DBG_KMS(fmt, ...) \
> +	do { \
> +		if (check_only) \
> +			DRM_ERROR(fmt, ##__VA_ARGS__); \
> +		else \
> +			DRM_DEBUG_KMS(fmt, ##__VA_ARGS__); \
> +	} while (0)
> +
>  #define PIPE_CONF_CHECK_X(name)	\
>  	if (current_config->name != pipe_config->name) { \
> -		DRM_ERROR("mismatch in " #name " " \
> +		INTEL_ERR_OR_DBG_KMS("mismatch in " #name " " \
>  			  "(expected 0x%08x, found 0x%08x)\n", \
>  			  current_config->name, \
>  			  pipe_config->name); \
> -		return false; \
> +		ret = false; \
>  	}
>  
>  #define PIPE_CONF_CHECK_I(name)	\
>  	if (current_config->name != pipe_config->name) { \
> -		DRM_ERROR("mismatch in " #name " " \
> +		INTEL_ERR_OR_DBG_KMS("mismatch in " #name " " \
>  			  "(expected %i, found %i)\n", \
>  			  current_config->name, \
>  			  pipe_config->name); \
> -		return false; \
> +		ret = false; \
>  	}
>  
>  /* This is required for BDW+ where there is only one set of registers for
> @@ -12022,30 +12033,30 @@ intel_pipe_config_compare(struct drm_device *dev,
>  #define PIPE_CONF_CHECK_I_ALT(name, alt_name) \
>  	if ((current_config->name != pipe_config->name) && \
>  		(current_config->alt_name != pipe_config->name)) { \
> -			DRM_ERROR("mismatch in " #name " " \
> +			INTEL_ERR_OR_DBG_KMS("mismatch in " #name " " \
>  				  "(expected %i or %i, found %i)\n", \
>  				  current_config->name, \
>  				  current_config->alt_name, \
>  				  pipe_config->name); \
> -			return false; \
> +			ret = false; \
>  	}
>  
>  #define PIPE_CONF_CHECK_FLAGS(name, mask)	\
>  	if ((current_config->name ^ pipe_config->name) & (mask)) { \
> -		DRM_ERROR("mismatch in " #name "(" #mask ") "	   \
> +		INTEL_ERR_OR_DBG_KMS("mismatch in " #name "(" #mask ") "	   \
>  			  "(expected %i, found %i)\n", \
>  			  current_config->name & (mask), \
>  			  pipe_config->name & (mask)); \
> -		return false; \
> +		ret = false; \
>  	}
>  
>  #define PIPE_CONF_CHECK_CLOCK_FUZZY(name) \
>  	if (!intel_fuzzy_clock_check(current_config->name, pipe_config->name)) { \
> -		DRM_ERROR("mismatch in " #name " " \
> +		INTEL_ERR_OR_DBG_KMS("mismatch in " #name " " \
>  			  "(expected %i, found %i)\n", \
>  			  current_config->name, \
>  			  pipe_config->name); \
> -		return false; \
> +		ret = false; \
>  	}
>  
>  #define PIPE_CONF_QUIRK(quirk)	\
> @@ -12179,8 +12190,9 @@ intel_pipe_config_compare(struct drm_device *dev,
>  #undef PIPE_CONF_CHECK_FLAGS
>  #undef PIPE_CONF_CHECK_CLOCK_FUZZY
>  #undef PIPE_CONF_QUIRK
> +#undef INTEL_ERR_OR_DBG_KMS
>  
> -	return true;
> +	return ret;
>  }
>  
>  static void check_wm_state(struct drm_device *dev)
> @@ -12377,7 +12389,7 @@ check_crtc_state(struct drm_device *dev)
>  		     "(expected %i, found %i)\n", crtc->base.state->active, crtc->active);
>  
>  		if (active &&
> -		    !intel_pipe_config_compare(dev, crtc->config, &pipe_config)) {
> +		    !intel_pipe_config_compare(dev, crtc->config, &pipe_config, false)) {
>  			I915_STATE_WARN(1, "pipe state doesn't match!\n");
>  			intel_dump_pipe_config(crtc, &pipe_config,
>  					       "[hw state]");
> @@ -12734,6 +12746,12 @@ static int intel_atomic_commit(struct drm_device *dev,
>  			intel_crtc->active = false;
>  			intel_disable_shared_dpll(intel_crtc);
>  		}
> +
> +		/* FIXME: Move this to i9xx_crtc_disable when it gets a pointer
> +		 * to the old crtc_state. */
> +		if (to_intel_crtc_state(crtc_state)->quirks &
> +		    PIPE_CONFIG_QUIRK_WRONG_PLANE)
> +			intel_crtc->plane = !intel_crtc->plane;
>  	}
>  
>  	/* Only after disabling all output pipelines that will be changed can we
> @@ -14464,13 +14482,16 @@ intel_check_plane_mapping(struct intel_crtc *crtc)
>  }
>  
>  static void intel_sanitize_crtc(struct intel_crtc *crtc,
> -				struct intel_crtc_state *pipe_config)
> +				struct intel_crtc_state *pipe_config,
> +				bool force_restore)
>  {
>  	struct drm_device *dev = crtc->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_encoder *intel_encoder;
> +	struct drm_atomic_state *state = pipe_config->base.state;
> +	struct intel_crtc_state *hw_state =
> +		to_intel_crtc_state(crtc->base.state);
>  	u32 reg;
> -	bool enable;
>  
>  	/* Clear any frame start delays used for debugging left by the BIOS */
>  	reg = PIPECONF(crtc->config->cpu_transcoder);
> @@ -14484,28 +14505,64 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc,
>  		drm_crtc_vblank_on(&crtc->base);
>  	}
>  
> +	/* set up current state */
> +	if (!force_restore && hw_state->base.active) {
> +		bool enable;
> +
> +		memcpy(pipe_config, hw_state, sizeof(*pipe_config));
> +		__drm_atomic_helper_crtc_duplicate_state(&crtc->base, &pipe_config->base);
> +		pipe_config->base.state = state;
> +
> +		enable = false;
> +		for_each_encoder_on_crtc(dev, &crtc->base, intel_encoder)
> +			enable |= intel_encoder->connectors_active;
> +
> +		pipe_config->base.active = !!enable;
> +	}
> +
>  	/* We need to sanitize the plane -> pipe mapping first because this will
>  	 * disable the crtc (and hence change the state) if it is wrong. Note
>  	 * that gen4+ has a fixed plane -> pipe mapping.  */
>  	if (INTEL_INFO(dev)->gen < 4 && !intel_check_plane_mapping(crtc)) {
> -		bool plane;
> -
>  		DRM_DEBUG_KMS("[CRTC:%d] wrong plane connection detected!\n",
>  			      crtc->base.base.id);
>  
>  		/* Pipe has the wrong plane attached and the plane is active.
>  		 * Temporarily change the plane mapping and disable everything
>  		 * ...  */
> -		plane = crtc->plane;
> -		to_intel_plane_state(crtc->base.primary->state)->visible = true;
> -		crtc->base.primary->crtc = &crtc->base;
> -		crtc->plane = !plane;
> -		intel_crtc_control(&crtc->base, false, true);
> -		crtc->plane = plane;
> +		hw_state->quirks |=
> +			PIPE_CONFIG_QUIRK_WRONG_PLANE;
> +
> +		crtc->plane = !crtc->plane;
> +
> +		if (force_restore)
> +			pipe_config->base.mode_changed = true;
> +		else
> +			pipe_config->base.active = false;
>  	}
>  
> -	if (dev_priv->quirks & QUIRK_PIPEA_FORCE &&
> -	    crtc->pipe == PIPE_A && (!pipe_config || !pipe_config->base.active)) {
> +	/* XXX: This is not active right now */
> +	if (hw_state->base.active && pipe_config->base.active &&
> +	    !i915.fastboot) {
> +		struct intel_crtc_state sw_state;
> +
> +		memset(&sw_state, 0, sizeof(sw_state));
> +		sw_state.base = pipe_config->base;
> +		sw_state.scaler_state = pipe_config->scaler_state;
> +		sw_state.shared_dpll = pipe_config->shared_dpll;
> +		sw_state.dpll_hw_state = pipe_config->dpll_hw_state;
> +		sw_state.ddi_pll_sel = pipe_config->ddi_pll_sel;
> +
> +		intel_modeset_pipe_config(&crtc->base, &sw_state);
> +
> +		/* Check if we need to force a modeset */
> +		if (!intel_pipe_config_compare(dev, &sw_state, hw_state, true))
> +			pipe_config->base.mode_changed = true;
> +	}
> +
> +
> +	if (dev_priv->quirks & QUIRK_PIPEA_FORCE && !hw_state->base.active &&
> +	    crtc->pipe == PIPE_A && !pipe_config->base.active) {
>  		/* BIOS forgot to enable pipe A, this mostly happens after
>  		 * resume. Force-enable the pipe to fix this, the update_dpms
>  		 * call below we restore the pipe to the right state, but leave
> @@ -14513,19 +14570,24 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc,
>  		intel_enable_pipe_a(dev);
>  	}
>  
> -	/* Adjust the state of the output pipe according to whether we
> -	 * have active connectors/encoders */
> -	enable = false;
> -	for_each_encoder_on_crtc(dev, &crtc->base, intel_encoder)
> -		enable |= intel_encoder->connectors_active;
> +	/* not restoring state, kill off all connectors and disable this thing */
> +	if (!force_restore && !pipe_config->base.active) {
> +		struct drm_connector_state *conn_state;
> +		struct drm_connector *connector;
> +		int i, ret;
>  
> -	/* only turn off separately if configuration's not restored,
> -	 * if it's restored it will change mode or turn off anyway.
> -	 */
> -	if (!enable && crtc->base.state->active && !pipe_config)
> -		intel_crtc_control(&crtc->base, false, true);
> +		ret = drm_atomic_set_mode_for_crtc(&pipe_config->base, NULL);
> +
> +		for_each_connector_in_state(state, connector, conn_state, i) {
> +			if (conn_state->crtc != &crtc->base)
> +				continue;
>  
> -	if (crtc->active || HAS_GMCH_DISPLAY(dev)) {
> +			ret = drm_atomic_set_crtc_for_connector(conn_state, NULL);
> +			WARN_ON(ret);
> +		}
> +	}
> +
> +	if (hw_state->base.active || HAS_GMCH_DISPLAY(dev)) {
>  		/*
>  		 * We start out with underrun reporting disabled to avoid races.
>  		 * For correct bookkeeping mark this on active crtcs.
> @@ -14581,6 +14643,7 @@ static void intel_sanitize_encoder(struct intel_encoder *encoder)
>  		for_each_intel_connector(dev, connector) {
>  			if (connector->encoder != encoder)
>  				continue;
> +
>  			connector->base.dpms = DRM_MODE_DPMS_OFF;
>  			connector->base.encoder = NULL;
>  		}
> @@ -14887,7 +14950,7 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
>  	struct intel_encoder *encoder;
>  	struct drm_atomic_state *state;
>  	struct intel_shared_dpll_config shared_dplls[I915_NUM_PLLS];
> -	int i;
> +	int i, ret;
>  
>  	state = intel_modeset_readout_hw_state(dev);
>  	if (IS_ERR(state)) {
> @@ -14897,6 +14960,7 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
>  
>  	/* swap sw/hw state */
>  	drm_atomic_helper_swap_state(dev, state);
> +
>  	intel_atomic_duplicate_dpll_state(dev_priv, shared_dplls);
>  	intel_shared_dpll_commit(state);
>  	memcpy(to_intel_atomic_state(state)->shared_dpll,
> @@ -14919,31 +14983,19 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
>  		crtc_state->planes_changed = false;
>  
>  		if (crtc->state->enable) {
> -			intel_mode_from_pipe_config(&crtc->state->mode,
> +			intel_mode_from_pipe_config(&crtc->mode,
>  				to_intel_crtc_state(crtc->state));
>  			intel_mode_from_pipe_config(&crtc->state->adjusted_mode,
>  				to_intel_crtc_state(crtc->state));
>  
> -			drm_mode_copy(&crtc->mode, &crtc->state->mode);
> +			if (drm_atomic_set_mode_for_crtc(crtc->state, &crtc->mode))
> +				drm_mode_copy(&crtc->state->mode, &crtc->mode);
> +
>  			drm_mode_copy(&crtc->hwmode,
>  				      &crtc->state->adjusted_mode);
> -
> -			/* Check if we need to force a modeset */
> -			if (to_intel_crtc_state(crtc_state)->has_audio !=
> -			    to_intel_crtc_state(crtc->state)->has_audio)
> -				crtc_state->mode_changed = true;
> -
> -			if (to_intel_crtc_state(crtc_state)->has_infoframe !=
> -			    to_intel_crtc_state(crtc->state)->has_infoframe)
> -				crtc_state->mode_changed = true;
>  		}
>  
> -		intel_sanitize_crtc(intel_crtc, !force_restore ? NULL :
> -				    to_intel_crtc_state(crtc_state));
> -
> -		/* turn CRTC off if a modeset is requested. */
> -		if (crtc_state->mode_changed && !force_restore)
> -			intel_crtc_control(crtc, false, true);
> +		intel_sanitize_crtc(intel_crtc, to_intel_crtc_state(crtc_state), force_restore);
>  
>  		/*
>  		 * sanitize_crtc may have forced an update of crtc->state,
> @@ -14973,17 +15025,10 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
>  	else if (HAS_PCH_SPLIT(dev))
>  		ilk_wm_get_hw_state(dev);
>  
> -	if (force_restore) {
> -		int ret;
> -
> -		i915_redisable_vga(dev);
> -
> -		ret = drm_atomic_commit(state);
> -		if (ret) {
> -			DRM_ERROR("Failed to restore previous mode\n");
> -			drm_atomic_state_free(state);
> -		}
> -	} else {
> +	i915_redisable_vga(dev);
> +	ret = drm_atomic_commit(state);
> +	if (ret) {
> +		DRM_ERROR("Failed to restore previous mode\n");
>  		drm_atomic_state_free(state);
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 2bf6873a5b89..2d19b5d67d9d 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -330,6 +330,7 @@ struct intel_crtc_state {
>  #define PIPE_CONFIG_QUIRK_MODE_SYNC_FLAGS	(1<<0) /* unreliable sync mode.flags */
>  #define PIPE_CONFIG_QUIRK_INHERITED_MODE	(1<<1) /* mode inherited from firmware */
>  #define PIPE_CONFIG_QUIRK_INITIAL_PLANES	(1<<2) /* planes are in unknown state */
> +#define PIPE_CONFIG_QUIRK_WRONG_PLANE		(1<<3) /* intel_crtc->plane is wrong */
>  	unsigned long quirks;
>  
>  	/* Pipe source size (ie. panel fitter input size)
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center


More information about the Intel-gfx mailing list