[RFC,4/7] drm/i915: enable fastboot for everyone
diff mbox

Message ID 1434700950-16242-5-git-send-email-maarten.lankhorst@linux.intel.com
State New
Headers show

Commit Message

Maarten Lankhorst June 19, 2015, 8:02 a.m. UTC
Now that we read out the full atomic state we can force fastboot without hacks.
The only thing that we worry about is preventing a modeset. This can be easily
done by calculating if sw state matches hw state, with exception for pfit and
pipe size. Since the original fastboot code only touched pipe size and panel
fitter we keep those, and compare full sw state against hw state.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_params.c   |   5 -
 drivers/gpu/drm/i915/intel_display.c | 271 ++++++++++++++++++++++-------------
 drivers/gpu/drm/i915/intel_fbdev.c   |  10 +-
 3 files changed, 169 insertions(+), 117 deletions(-)

Comments

Daniel Vetter June 22, 2015, 3:21 p.m. UTC | #1
On Fri, Jun 19, 2015 at 10:02:27AM +0200, Maarten Lankhorst wrote:
> Now that we read out the full atomic state we can force fastboot without hacks.
> The only thing that we worry about is preventing a modeset. This can be easily
> done by calculating if sw state matches hw state, with exception for pfit and
> pipe size. Since the original fastboot code only touched pipe size and panel
> fitter we keep those, and compare full sw state against hw state.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_params.c   |   5 -
>  drivers/gpu/drm/i915/intel_display.c | 271 ++++++++++++++++++++++-------------
>  drivers/gpu/drm/i915/intel_fbdev.c   |  10 +-
>  3 files changed, 169 insertions(+), 117 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index 8ac5a1b29ac0..9b344a956ba6 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -41,7 +41,6 @@ struct i915_params i915 __read_mostly = {
>  	.preliminary_hw_support = IS_ENABLED(CONFIG_DRM_I915_PRELIMINARY_HW_SUPPORT),
>  	.disable_power_well = 1,
>  	.enable_ips = 1,
> -	.fastboot = 0,
>  	.prefault_disable = 0,
>  	.load_detect_test = 0,
>  	.reset = true,
> @@ -137,10 +136,6 @@ MODULE_PARM_DESC(disable_power_well,
>  module_param_named(enable_ips, i915.enable_ips, int, 0600);
>  MODULE_PARM_DESC(enable_ips, "Enable IPS (default: true)");
>  
> -module_param_named(fastboot, i915.fastboot, bool, 0600);
> -MODULE_PARM_DESC(fastboot,
> -	"Try to skip unnecessary mode sets at boot time (default: false)");
> -
>  module_param_named_unsafe(prefault_disable, i915.prefault_disable, bool, 0600);
>  MODULE_PARM_DESC(prefault_disable,
>  	"Disable page prefaulting for pread/pwrite/reloc (default:false). "
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 9b2acc7b4e29..de975ef09e23 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 skylake_pfit_enable(struct intel_crtc *crtc);
>  static struct drm_atomic_state *
>  intel_modeset_setup_hw_state(struct drm_device *dev, bool force_restore);
>  
> @@ -3289,14 +3290,17 @@ static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc)
>  	return pending;
>  }
>  
> -static void intel_update_pipe_size(struct intel_crtc *crtc)
> +static void intel_update_fastboot(struct intel_crtc *crtc,
> +				  struct intel_crtc_state *old_crtc_state)

That's not a useful rename imo - updating pfit/pipe_size clearly tells us
what's going on, fastboot is much more a marketing thing really. And I
expect that eventually we'll have to grow a lot more update code to fix up
fastboot.

>  {
>  	struct drm_device *dev = crtc->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	const struct drm_display_mode *adjusted_mode;
> +	struct intel_crtc_state *pipe_config =
> +		to_intel_crtc_state(crtc->base.state);
>  
> -	if (!i915.fastboot)
> -		return;
> +	DRM_DEBUG_KMS("Applying fastboot quirks, %ix%i -> %ix%i\n",
> +		      old_crtc_state->pipe_src_w, old_crtc_state->pipe_src_h,
> +		      pipe_config->pipe_src_w, pipe_config->pipe_src_h);
>  
>  	/*
>  	 * Update pipe size and adjust fitter if needed: the reason for this is
> @@ -3305,27 +3309,27 @@ static void intel_update_pipe_size(struct intel_crtc *crtc)
>  	 * fastboot case, we'll flip, but if we don't update the pipesrc and
>  	 * pfit state, we'll end up with a big fb scanned out into the wrong
>  	 * sized surface.
> -	 *
> -	 * To fix this properly, we need to hoist the checks up into
> -	 * compute_mode_changes (or above), check the actual pfit state and
> -	 * whether the platform allows pfit disable with pipe active, and only
> -	 * then update the pipesrc and pfit state, even on the flip path.
>  	 */
>  
> -	adjusted_mode = &crtc->config->base.adjusted_mode;
> -
>  	I915_WRITE(PIPESRC(crtc->pipe),
> -		   ((adjusted_mode->crtc_hdisplay - 1) << 16) |
> -		   (adjusted_mode->crtc_vdisplay - 1));
> -	if (!crtc->config->pch_pfit.enabled &&
> -	    (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS) ||
> -	     intel_pipe_has_type(crtc, INTEL_OUTPUT_EDP))) {
> +		   ((pipe_config->pipe_src_w - 1) << 16) |
> +		   (pipe_config->pipe_src_h - 1));
> +
> +	/* on skylake this is done by detaching scalers */
> +	if (INTEL_INFO(dev)->gen == 9) {
> +		skl_detach_scalers(crtc);
> +
> +		if (pipe_config->pch_pfit.enabled)
> +			skylake_pfit_enable(crtc);
> +	}
> +	else if (!pipe_config->pch_pfit.enabled &&
> +	    old_crtc_state->pch_pfit.enabled) {
> +		DRM_DEBUG_KMS("Disabling panel fitter\n");
> +
>  		I915_WRITE(PF_CTL(crtc->pipe), 0);
>  		I915_WRITE(PF_WIN_POS(crtc->pipe), 0);
>  		I915_WRITE(PF_WIN_SZ(crtc->pipe), 0);
>  	}
> -	crtc->config->pipe_src_w = adjusted_mode->crtc_hdisplay;
> -	crtc->config->pipe_src_h = adjusted_mode->crtc_vdisplay;
>  }
>  
>  static void intel_fdi_normal_train(struct drm_crtc *crtc)
> @@ -12244,19 +12248,6 @@ encoder_retry:
>  	DRM_DEBUG_KMS("plane bpp: %i, pipe bpp: %i, dithering: %i\n",
>  		      base_bpp, pipe_config->pipe_bpp, pipe_config->dither);
>  
> -	/* Check if we need to force a modeset */
> -	if (pipe_config->has_audio !=
> -	    to_intel_crtc_state(crtc->state)->has_audio) {
> -		pipe_config->base.mode_changed = true;
> -		ret = drm_atomic_add_affected_planes(state, crtc);
> -	}
> -
> -	/*
> -	 * Note we have an issue here with infoframes: current code
> -	 * only updates them on the full mode set path per hw
> -	 * requirements.  So here we should be checking for any
> -	 * required changes and forcing a mode set.
> -	 */
>  fail:
>  	return ret;
>  }
> @@ -12500,29 +12491,21 @@ intel_pipe_config_compare(struct drm_device *dev,
>  				      DRM_MODE_FLAG_NVSYNC);
>  	}
>  
> -	PIPE_CONF_CHECK_I(pipe_src_w);
> -	PIPE_CONF_CHECK_I(pipe_src_h);
> -
> -	/*
> -	 * FIXME: BIOS likes to set up a cloned config with lvds+external
> -	 * screen. Since we don't yet re-compute the pipe config when moving
> -	 * just the lvds port away to another pipe the sw tracking won't match.
> -	 *
> -	 * Proper atomic modesets with recomputed global state will fix this.
> -	 * Until then just don't check gmch state for inherited modes.
> -	 */
>  	if (!PIPE_CONF_QUIRK(PIPE_CONFIG_QUIRK_INHERITED_MODE)) {

This seems like a separate change - with recomputing state on all pipes
unconditionally we should be able to drop this?

> -		PIPE_CONF_CHECK_I(gmch_pfit.control);
> -		/* pfit ratios are autocomputed by the hw on gen4+ */
> -		if (INTEL_INFO(dev)->gen < 4)
> -			PIPE_CONF_CHECK_I(gmch_pfit.pgm_ratios);
> -		PIPE_CONF_CHECK_I(gmch_pfit.lvds_border_bits);
> -	}
> +		PIPE_CONF_CHECK_I(pipe_src_w);
> +		PIPE_CONF_CHECK_I(pipe_src_h);
> +
> +		PIPE_CONF_CHECK_I(pch_pfit.enabled);
> +		if (current_config->pch_pfit.enabled) {
> +			PIPE_CONF_CHECK_I(pch_pfit.pos);
> +			PIPE_CONF_CHECK_I(pch_pfit.size);
>  
> -	PIPE_CONF_CHECK_I(pch_pfit.enabled);
> -	if (current_config->pch_pfit.enabled) {
> -		PIPE_CONF_CHECK_I(pch_pfit.pos);
> -		PIPE_CONF_CHECK_I(pch_pfit.size);
> +			PIPE_CONF_CHECK_I(gmch_pfit.control);
> +			/* pfit ratios are autocomputed by the hw on gen4+ */
> +			if (INTEL_INFO(dev)->gen < 4)
> +				PIPE_CONF_CHECK_I(gmch_pfit.pgm_ratios);
> +			PIPE_CONF_CHECK_I(gmch_pfit.lvds_border_bits);
> +		}
>  	}
>  
>  	PIPE_CONF_CHECK_I(scaler_state.scaler_id);
> @@ -13057,26 +13040,18 @@ intel_modeset_compute_config(struct drm_atomic_state *state)
>  		return ret;
>  
>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> -		if (!crtc_state->enable) {
> -			if (needs_modeset(crtc_state))
> -				any_ms = true;
> +		if (!needs_modeset(crtc_state))
>  			continue;
> -		}
>  
> -		if (!needs_modeset(crtc_state)) {
> -			ret = drm_atomic_add_affected_connectors(state, crtc);
> -			if (ret)
> -				return ret;
> -		}
> +		any_ms = true;
> +		if (!crtc_state->enable)
> +			continue;
>  
>  		ret = intel_modeset_pipe_config(crtc,
>  					to_intel_crtc_state(crtc_state));
>  		if (ret)
>  			return ret;
>  
> -		if (needs_modeset(crtc_state))
> -			any_ms = true;
> -
>  		intel_dump_pipe_config(to_intel_crtc(crtc),
>  				       to_intel_crtc_state(crtc_state),
>  				       "[modeset]");
> @@ -13147,7 +13122,10 @@ static int __intel_set_mode(struct drm_atomic_state *state)
>  		if (needs_modeset(crtc->state) && crtc->state->active) {
>  			update_scanline_offset(to_intel_crtc(crtc));
>  			dev_priv->display.crtc_enable(crtc);
> -		}
> +		} else if (to_intel_crtc_state(crtc_state)->quirks &
> +			   PIPE_CONFIG_QUIRK_INHERITED_MODE)
> +			/* force hw readout check */
> +			any_ms = true;

Imo this should be a separate patch since forcing a modeset check after
the initial takeover is a good thing. Also maybe do a
s/any_ms/do_modeset_checks/ for clarity of what this does.

>  
>  		drm_atomic_helper_commit_planes_on_crtc(crtc_state);
>  	}
> @@ -13430,8 +13408,6 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
>  	if (ret)
>  		goto out;
>  
> -	intel_update_pipe_size(to_intel_crtc(set->crtc));
> -
>  	ret = intel_set_mode_checked(state);
>  	if (ret) {
>  		DRM_DEBUG_KMS("failed to set mode on [CRTC:%d], err = %d\n",
> @@ -13718,10 +13694,6 @@ intel_commit_primary_plane(struct drm_plane *plane,
>  	if (!crtc->state->active)
>  		return;
>  
> -	if (state->visible)
> -		/* FIXME: kill this fastboot hack */
> -		intel_update_pipe_size(intel_crtc);
> -
>  	dev_priv->display.update_primary_plane(crtc, fb, crtc->x, crtc->y);
>  }
>  
> @@ -13741,6 +13713,8 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct intel_crtc_state *old_intel_state =
> +		to_intel_crtc_state(old_crtc_state);
>  
>  	if (!needs_modeset(crtc->state))
>  		intel_pre_plane_update(intel_crtc);
> @@ -13756,7 +13730,10 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
>  			intel_pipe_update_start(intel_crtc,
>  						&intel_crtc->atomic.start_vbl_count);
>  
> -	if (!needs_modeset(crtc->state) && INTEL_INFO(dev)->gen >= 9)
> +	if (!needs_modeset(crtc->state) && crtc->state->active &&
> +	    old_intel_state->quirks & PIPE_CONFIG_QUIRK_INHERITED_MODE)
> +		intel_update_fastboot(intel_crtc, old_intel_state);

Nack, fastboot only for the initial mode is not what I want. We should be
able to speed up _any_ modeset where we just disable the pfit. Allowing
fast pfit removal unconditionally will also allow us to easily test this
part of fastboot with an igt, which I'd also like to see.

> +	else if (!needs_modeset(crtc->state) && INTEL_INFO(dev)->gen >= 9)
>  		skl_detach_scalers(intel_crtc);

Also I think it'd be clearer if we have state flags (like for the pageflip
stuff) in crtc_state to enable these fixup functions on an as-needed
basis.

>  }
>  
> @@ -15096,6 +15073,114 @@ intel_check_plane_mapping(struct intel_crtc *crtc)
>  	return true;
>  }
>  
> +static bool
> +intel_compare_m_n(unsigned int m, unsigned int n,
> +		  unsigned int m2, unsigned int n2)
> +{
> +	if (m == m2 && n == n2)
> +		return true;
> +
> +	if (!m || !n || !m2 || !n2)
> +		return false;
> +
> +	if (m > m2) {
> +		while (m > m2) {
> +			m >>= 1;
> +			n >>= 1;
> +		}
> +	} else if (m < m2) {
> +		while (m < m2) {
> +			m2 >>= 1;
> +			n2 >>= 1;
> +		}
> +	}
> +
> +	return m == m2 && n == n2;
> +}
> +
> +static bool
> +intel_compare_link_m_n(const struct intel_link_m_n *m_n,
> +		       const struct intel_link_m_n *m2_n2)
> +{
> +	if (m_n->tu == m2_n2->tu &&
> +	    intel_compare_m_n(m_n->gmch_m, m_n->gmch_n,
> +			      m2_n2->gmch_m, m2_n2->gmch_n) &&
> +	    intel_compare_m_n(m_n->link_m, m_n->link_n,
> +			      m2_n2->link_m, m2_n2->link_n))
> +		return true;
> +
> +	return false;
> +}
> +
> +static void intel_fastboot_calc_modeset(struct intel_crtc *crtc,
> +					struct intel_crtc_state *hw_state,
> +					struct intel_crtc_state *pipe_config)
> +{
> +	struct drm_device *dev = crtc->base.dev;
> +	struct intel_crtc_state sw_state;
> +
> +	if (needs_modeset(&pipe_config->base)) {
> +		DRM_DEBUG_KMS("Skipping fastboot checks, modeset forced\n");
> +		return;
> +	}
> +
> +	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);
> +	sw_state.quirks = hw_state->quirks & PIPE_CONFIG_QUIRK_MODE_SYNC_FLAGS;
> +
> +	if (intel_compare_link_m_n(&sw_state.fdi_m_n, &hw_state->fdi_m_n))
> +		sw_state.fdi_m_n = hw_state->fdi_m_n;
> +
> +	if (INTEL_INFO(dev)->gen < 8) {
> +		if (intel_compare_link_m_n(&sw_state.dp_m_n,
> +					   &hw_state->dp_m_n))
> +			sw_state.dp_m_n = hw_state->dp_m_n;
> +
> +		if (intel_compare_link_m_n(&sw_state.dp_m2_n2,
> +					   &hw_state->dp_m2_n2))
> +			sw_state.dp_m2_n2 = hw_state->dp_m2_n2;
> +	} else {
> +		if (intel_compare_link_m_n(&sw_state.dp_m_n,
> +					   &hw_state->dp_m_n)) {
> +			sw_state.dp_m_n = hw_state->dp_m_n;
> +			hw_state->dp_m2_n2 = sw_state.dp_m2_n2;
> +		} else if (intel_compare_link_m_n(&sw_state.dp_m2_n2,
> +						  &hw_state->dp_m_n)) {
> +			sw_state.dp_m2_n2 = hw_state->dp_m_n;
> +
> +			hw_state->dp_m_n = sw_state.dp_m_n;
> +			hw_state->dp_m2_n2 = sw_state.dp_m2_n2;
> +		}
> +	}

Not too happy with splitting up the modeset state compare code like this.
If we add enum mode { EXACT, COMPATIBLE } to intel_pipe_config_compare we
could tie things together well and have all the pipe config code grouped
in one place. But then the _compare() is a bit misleading since it'd also
update the sw_state, so maybe better to call it compare_and_ajust.

The macros would then either copy the hw_state to sw_state (if it's some
intermediate state we don't care about) or compare it (with maybe some
adjustments made). But I'd really like to have all that compare/adjust
logic in one place if possible.

> +
> +	/* Check if we need to force a modeset */
> +	if (!intel_pipe_config_compare(dev, &sw_state, hw_state, true)) {
> +		DRM_INFO("[CRTC:%i] sw state incompatible, forcing modeset\n",
> +			 crtc->base.base.id);
> +		pipe_config->base.mode_changed = true;
> +	} else {
> +		DRM_INFO("[CRTC:%i] sw state and hw state fastboot compatible\n",
> +			 crtc->base.base.id);
> +
> +		*pipe_config = sw_state;
> +		intel_dump_pipe_config(crtc, pipe_config,
> +				       "[new fastboot state]");
> +
> +		/* prevent a modeset by patching up mode struct */
> +
> +		hw_state->base.mode.type = pipe_config->base.mode.type =
> +		hw_state->base.adjusted_mode.type = pipe_config->base.adjusted_mode.type;
> +
> +		/* clock readout can be approximate, just copy it. */
> +		hw_state->base.mode.clock = pipe_config->base.mode.clock = pipe_config->base.adjusted_mode.clock;

Why do we still need this? If we use intel_pipe_config_compare to compute
mode_changed then we hopefully don't need to hack around things any more.
I guess this is where we need your plan to split out connector_changed
from mode_changed and then replace the mode_changed computed by the
helpers by what intel_pipe_config_compare things is justified.

> +	}
> +}
> +
>  static void intel_sanitize_crtc(struct intel_crtc *crtc,
>  				struct intel_crtc_state *pipe_config,
>  				bool force_restore)
> @@ -15135,37 +15220,11 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc,
>  		pipe_config->base.active = !!enable;
>  	}
>  
> -	if (hw_state->quirks & PIPE_CONFIG_QUIRK_WRONG_PLANE) {
> -		if (force_restore || i915.fastboot)
> -			pipe_config->base.mode_changed = true;
> -		else
> -			pipe_config->base.active = false;
> -	}
> -
> -	/* XXX: This is needs to be modified for making fastboot possible
> -	 * by default without requiring a modeset.
> -	 */
> -	if (hw_state->base.active && pipe_config->base.active) {
> -		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, !i915.fastboot)) {
> -			DRM_DEBUG_KMS("sw and hw state differ, forcing modeset\n");
> -			pipe_config->base.mode_changed = true;
> -		} else {
> -			*pipe_config = sw_state;
> -		}
> -	}
> +	if (hw_state->quirks & PIPE_CONFIG_QUIRK_WRONG_PLANE)
> +		pipe_config->base.mode_changed = true;
>  
> +	else if (hw_state->base.active && pipe_config->base.active)
> +		intel_fastboot_calc_modeset(crtc, hw_state, pipe_config);
>  
>  	if (dev_priv->quirks & QUIRK_PIPEA_FORCE && !hw_state->base.active &&
>  	    crtc->pipe == PIPE_A && !pipe_config->base.active) {
> @@ -15584,6 +15643,7 @@ intel_modeset_setup_hw_state(struct drm_device *dev, bool force_restore)
>  		crtc_state->planes_changed = false;
>  
>  		if (crtc->state->enable) {
> +			memset(&crtc->mode, 0, sizeof(crtc->mode));
>  			intel_mode_from_pipe_config(&crtc->mode,
>  				to_intel_crtc_state(crtc->state));
>  			intel_mode_from_pipe_config(&crtc->state->adjusted_mode,
> @@ -15678,9 +15738,12 @@ void intel_modeset_gem_init(struct drm_device *dev, struct drm_atomic_state *sta
>  		int j;
>  		struct drm_plane *primary = c->primary;
>  
> +		if (!c->state->active)
> +			continue;
> +
>  		obj = intel_fb_obj(primary->state->fb);
>  		if (obj == NULL)
> -			goto disable;
> +			continue;
>  
>  		mutex_lock(&dev->struct_mutex);
>  		ret = intel_pin_and_fence_fb_obj(primary, primary->state->fb,
> @@ -15715,6 +15778,8 @@ void intel_modeset_gem_init(struct drm_device *dev, struct drm_atomic_state *sta
>  		 */
>  
>  disable:
> +		DRM_DEBUG_KMS("[CRTC:%i] No fb or forced inactive, disabling.\n",
> +			      c->base.id);
>  		crtc_state->active = crtc_state->enable = false;
>  
>  		ret = drm_atomic_set_mode_for_crtc(crtc_state, NULL);
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 6ac4990a0c11..2c494d78652a 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -486,15 +486,10 @@ retry:
>  			 * config, not the input mode, which is what crtc->mode
>  			 * usually contains. But since our current fastboot
>  			 * code puts a mode derived from the post-pfit timings
> -			 * into crtc->mode this works out correctly. We don't
> -			 * use hwmode anywhere right now, so use it for this
> -			 * since the fb helper layer wants a pointer to
> -			 * something we own.
> +			 * into crtc->mode this works out correctly.
>  			 */
>  			DRM_DEBUG_KMS("looking for current mode on connector %s\n",
>  				      connector->name);
> -			intel_mode_from_pipe_config(&encoder->crtc->hwmode,
> -						    to_intel_crtc(encoder->crtc)->config);
>  			modes[i] = &encoder->crtc->hwmode;
>  		}
>  		crtcs[i] = new_crtc;
> @@ -584,9 +579,6 @@ static bool intel_fbdev_init_bios(struct drm_device *dev,
>  	struct intel_crtc *intel_crtc;
>  	unsigned int max_size = 0;
>  
> -	if (!i915.fastboot)
> -		return false;

I think it'd be useful to split the fbdev part of the fastboot removal out
into a separate patch. Or do I miss a depency here? Assuming ofc that we
enable the pfit trick for modesets in general.
-Daniel

> -
>  	/* Find the largest fb */
>  	for_each_crtc(dev, crtc) {
>  		struct intel_framebuffer *plane_fb =
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Maarten Lankhorst June 23, 2015, 10:38 a.m. UTC | #2
Op 22-06-15 om 17:21 schreef Daniel Vetter:
> On Fri, Jun 19, 2015 at 10:02:27AM +0200, Maarten Lankhorst wrote:
>> Now that we read out the full atomic state we can force fastboot without hacks.
>> The only thing that we worry about is preventing a modeset. This can be easily
>> done by calculating if sw state matches hw state, with exception for pfit and
>> pipe size. Since the original fastboot code only touched pipe size and panel
>> fitter we keep those, and compare full sw state against hw state.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_params.c   |   5 -
>>  drivers/gpu/drm/i915/intel_display.c | 271 ++++++++++++++++++++++-------------
>>  drivers/gpu/drm/i915/intel_fbdev.c   |  10 +-
>>  3 files changed, 169 insertions(+), 117 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
>> index 8ac5a1b29ac0..9b344a956ba6 100644
>> --- a/drivers/gpu/drm/i915/i915_params.c
>> +++ b/drivers/gpu/drm/i915/i915_params.c
>> @@ -41,7 +41,6 @@ struct i915_params i915 __read_mostly = {
>>  	.preliminary_hw_support = IS_ENABLED(CONFIG_DRM_I915_PRELIMINARY_HW_SUPPORT),
>>  	.disable_power_well = 1,
>>  	.enable_ips = 1,
>> -	.fastboot = 0,
>>  	.prefault_disable = 0,
>>  	.load_detect_test = 0,
>>  	.reset = true,
>> @@ -137,10 +136,6 @@ MODULE_PARM_DESC(disable_power_well,
>>  module_param_named(enable_ips, i915.enable_ips, int, 0600);
>>  MODULE_PARM_DESC(enable_ips, "Enable IPS (default: true)");
>>  
>> -module_param_named(fastboot, i915.fastboot, bool, 0600);
>> -MODULE_PARM_DESC(fastboot,
>> -	"Try to skip unnecessary mode sets at boot time (default: false)");
>> -
>>  module_param_named_unsafe(prefault_disable, i915.prefault_disable, bool, 0600);
>>  MODULE_PARM_DESC(prefault_disable,
>>  	"Disable page prefaulting for pread/pwrite/reloc (default:false). "
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 9b2acc7b4e29..de975ef09e23 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 skylake_pfit_enable(struct intel_crtc *crtc);
>>  static struct drm_atomic_state *
>>  intel_modeset_setup_hw_state(struct drm_device *dev, bool force_restore);
>>  
>> @@ -3289,14 +3290,17 @@ static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc)
>>  	return pending;
>>  }
>>  
>> -static void intel_update_pipe_size(struct intel_crtc *crtc)
>> +static void intel_update_fastboot(struct intel_crtc *crtc,
>> +				  struct intel_crtc_state *old_crtc_state)
> That's not a useful rename imo - updating pfit/pipe_size clearly tells us
> what's going on, fastboot is much more a marketing thing really. And I
> expect that eventually we'll have to grow a lot more update code to fix up
> fastboot.
>
>>  {
>>  	struct drm_device *dev = crtc->base.dev;
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>> -	const struct drm_display_mode *adjusted_mode;
>> +	struct intel_crtc_state *pipe_config =
>> +		to_intel_crtc_state(crtc->base.state);
>>  
>> -	if (!i915.fastboot)
>> -		return;
>> +	DRM_DEBUG_KMS("Applying fastboot quirks, %ix%i -> %ix%i\n",
>> +		      old_crtc_state->pipe_src_w, old_crtc_state->pipe_src_h,
>> +		      pipe_config->pipe_src_w, pipe_config->pipe_src_h);
>>  
>>  	/*
>>  	 * Update pipe size and adjust fitter if needed: the reason for this is
>> @@ -3305,27 +3309,27 @@ static void intel_update_pipe_size(struct intel_crtc *crtc)
>>  	 * fastboot case, we'll flip, but if we don't update the pipesrc and
>>  	 * pfit state, we'll end up with a big fb scanned out into the wrong
>>  	 * sized surface.
>> -	 *
>> -	 * To fix this properly, we need to hoist the checks up into
>> -	 * compute_mode_changes (or above), check the actual pfit state and
>> -	 * whether the platform allows pfit disable with pipe active, and only
>> -	 * then update the pipesrc and pfit state, even on the flip path.
>>  	 */
>>  
>> -	adjusted_mode = &crtc->config->base.adjusted_mode;
>> -
>>  	I915_WRITE(PIPESRC(crtc->pipe),
>> -		   ((adjusted_mode->crtc_hdisplay - 1) << 16) |
>> -		   (adjusted_mode->crtc_vdisplay - 1));
>> -	if (!crtc->config->pch_pfit.enabled &&
>> -	    (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS) ||
>> -	     intel_pipe_has_type(crtc, INTEL_OUTPUT_EDP))) {
>> +		   ((pipe_config->pipe_src_w - 1) << 16) |
>> +		   (pipe_config->pipe_src_h - 1));
>> +
>> +	/* on skylake this is done by detaching scalers */
>> +	if (INTEL_INFO(dev)->gen == 9) {
>> +		skl_detach_scalers(crtc);
>> +
>> +		if (pipe_config->pch_pfit.enabled)
>> +			skylake_pfit_enable(crtc);
>> +	}
>> +	else if (!pipe_config->pch_pfit.enabled &&
>> +	    old_crtc_state->pch_pfit.enabled) {
>> +		DRM_DEBUG_KMS("Disabling panel fitter\n");
>> +
>>  		I915_WRITE(PF_CTL(crtc->pipe), 0);
>>  		I915_WRITE(PF_WIN_POS(crtc->pipe), 0);
>>  		I915_WRITE(PF_WIN_SZ(crtc->pipe), 0);
>>  	}
>> -	crtc->config->pipe_src_w = adjusted_mode->crtc_hdisplay;
>> -	crtc->config->pipe_src_h = adjusted_mode->crtc_vdisplay;
>>  }
>>  
>>  static void intel_fdi_normal_train(struct drm_crtc *crtc)
>> @@ -12244,19 +12248,6 @@ encoder_retry:
>>  	DRM_DEBUG_KMS("plane bpp: %i, pipe bpp: %i, dithering: %i\n",
>>  		      base_bpp, pipe_config->pipe_bpp, pipe_config->dither);
>>  
>> -	/* Check if we need to force a modeset */
>> -	if (pipe_config->has_audio !=
>> -	    to_intel_crtc_state(crtc->state)->has_audio) {
>> -		pipe_config->base.mode_changed = true;
>> -		ret = drm_atomic_add_affected_planes(state, crtc);
>> -	}
>> -
>> -	/*
>> -	 * Note we have an issue here with infoframes: current code
>> -	 * only updates them on the full mode set path per hw
>> -	 * requirements.  So here we should be checking for any
>> -	 * required changes and forcing a mode set.
>> -	 */
>>  fail:
>>  	return ret;
>>  }
>> @@ -12500,29 +12491,21 @@ intel_pipe_config_compare(struct drm_device *dev,
>>  				      DRM_MODE_FLAG_NVSYNC);
>>  	}
>>  
>> -	PIPE_CONF_CHECK_I(pipe_src_w);
>> -	PIPE_CONF_CHECK_I(pipe_src_h);
>> -
>> -	/*
>> -	 * FIXME: BIOS likes to set up a cloned config with lvds+external
>> -	 * screen. Since we don't yet re-compute the pipe config when moving
>> -	 * just the lvds port away to another pipe the sw tracking won't match.
>> -	 *
>> -	 * Proper atomic modesets with recomputed global state will fix this.
>> -	 * Until then just don't check gmch state for inherited modes.
>> -	 */
>>  	if (!PIPE_CONF_QUIRK(PIPE_CONFIG_QUIRK_INHERITED_MODE)) {
> This seems like a separate change - with recomputing state on all pipes
> unconditionally we should be able to drop this?
We repurpose inherited mode here to skip some checks on first boot that will be patched up on first atomic commit.

>> -		PIPE_CONF_CHECK_I(gmch_pfit.control);
>> -		/* pfit ratios are autocomputed by the hw on gen4+ */
>> -		if (INTEL_INFO(dev)->gen < 4)
>> -			PIPE_CONF_CHECK_I(gmch_pfit.pgm_ratios);
>> -		PIPE_CONF_CHECK_I(gmch_pfit.lvds_border_bits);
>> -	}
>> +		PIPE_CONF_CHECK_I(pipe_src_w);
>> +		PIPE_CONF_CHECK_I(pipe_src_h);
>> +
>> +		PIPE_CONF_CHECK_I(pch_pfit.enabled);
>> +		if (current_config->pch_pfit.enabled) {
>> +			PIPE_CONF_CHECK_I(pch_pfit.pos);
>> +			PIPE_CONF_CHECK_I(pch_pfit.size);
>>  
>> -	PIPE_CONF_CHECK_I(pch_pfit.enabled);
>> -	if (current_config->pch_pfit.enabled) {
>> -		PIPE_CONF_CHECK_I(pch_pfit.pos);
>> -		PIPE_CONF_CHECK_I(pch_pfit.size);
>> +			PIPE_CONF_CHECK_I(gmch_pfit.control);
>> +			/* pfit ratios are autocomputed by the hw on gen4+ */
>> +			if (INTEL_INFO(dev)->gen < 4)
>> +				PIPE_CONF_CHECK_I(gmch_pfit.pgm_ratios);
>> +			PIPE_CONF_CHECK_I(gmch_pfit.lvds_border_bits);
>> +		}
>>  	}
>>  
>>  	PIPE_CONF_CHECK_I(scaler_state.scaler_id);
>> @@ -13057,26 +13040,18 @@ intel_modeset_compute_config(struct drm_atomic_state *state)
>>  		return ret;
>>  
>>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>> -		if (!crtc_state->enable) {
>> -			if (needs_modeset(crtc_state))
>> -				any_ms = true;
>> +		if (!needs_modeset(crtc_state))
>>  			continue;
>> -		}
>>  
>> -		if (!needs_modeset(crtc_state)) {
>> -			ret = drm_atomic_add_affected_connectors(state, crtc);
>> -			if (ret)
>> -				return ret;
>> -		}
>> +		any_ms = true;
>> +		if (!crtc_state->enable)
>> +			continue;
>>  
>>  		ret = intel_modeset_pipe_config(crtc,
>>  					to_intel_crtc_state(crtc_state));
>>  		if (ret)
>>  			return ret;
>>  
>> -		if (needs_modeset(crtc_state))
>> -			any_ms = true;
>> -
>>  		intel_dump_pipe_config(to_intel_crtc(crtc),
>>  				       to_intel_crtc_state(crtc_state),
>>  				       "[modeset]");
>> @@ -13147,7 +13122,10 @@ static int __intel_set_mode(struct drm_atomic_state *state)
>>  		if (needs_modeset(crtc->state) && crtc->state->active) {
>>  			update_scanline_offset(to_intel_crtc(crtc));
>>  			dev_priv->display.crtc_enable(crtc);
>> -		}
>> +		} else if (to_intel_crtc_state(crtc_state)->quirks &
>> +			   PIPE_CONFIG_QUIRK_INHERITED_MODE)
>> +			/* force hw readout check */
>> +			any_ms = true;
> Imo this should be a separate patch since forcing a modeset check after
> the initial takeover is a good thing. Also maybe do a
> s/any_ms/do_modeset_checks/ for clarity of what this does.
It's used for other reasons too, like updating power domains when crtc states change. So do_modeset_checks would be misleading.
>>  
>>  		drm_atomic_helper_commit_planes_on_crtc(crtc_state);
>>  	}
>> @@ -13430,8 +13408,6 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
>>  	if (ret)
>>  		goto out;
>>  
>> -	intel_update_pipe_size(to_intel_crtc(set->crtc));
>> -
>>  	ret = intel_set_mode_checked(state);
>>  	if (ret) {
>>  		DRM_DEBUG_KMS("failed to set mode on [CRTC:%d], err = %d\n",
>> @@ -13718,10 +13694,6 @@ intel_commit_primary_plane(struct drm_plane *plane,
>>  	if (!crtc->state->active)
>>  		return;
>>  
>> -	if (state->visible)
>> -		/* FIXME: kill this fastboot hack */
>> -		intel_update_pipe_size(intel_crtc);
>> -
>>  	dev_priv->display.update_primary_plane(crtc, fb, crtc->x, crtc->y);
>>  }
>>  
>> @@ -13741,6 +13713,8 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
>>  	struct drm_device *dev = crtc->dev;
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>> +	struct intel_crtc_state *old_intel_state =
>> +		to_intel_crtc_state(old_crtc_state);
>>  
>>  	if (!needs_modeset(crtc->state))
>>  		intel_pre_plane_update(intel_crtc);
>> @@ -13756,7 +13730,10 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
>>  			intel_pipe_update_start(intel_crtc,
>>  						&intel_crtc->atomic.start_vbl_count);
>>  
>> -	if (!needs_modeset(crtc->state) && INTEL_INFO(dev)->gen >= 9)
>> +	if (!needs_modeset(crtc->state) && crtc->state->active &&
>> +	    old_intel_state->quirks & PIPE_CONFIG_QUIRK_INHERITED_MODE)
>> +		intel_update_fastboot(intel_crtc, old_intel_state);
> Nack, fastboot only for the initial mode is not what I want. We should be
> able to speed up _any_ modeset where we just disable the pfit. Allowing
> fast pfit removal unconditionally will also allow us to easily test this
> part of fastboot with an igt, which I'd also like to see.
You're right, but I think because this needs more infrastructure. I only want to convert fastboot at first.
For MST we might also need a variant which only performs a pipe modeset, rather than a full modeset.
Still there are cases where we will need to force a modeset regardless, for example when device frequency
changes.

In either case this probably requires a lot of code may differ on each platform. :(

>> +	else if (!needs_modeset(crtc->state) && INTEL_INFO(dev)->gen >= 9)
>>  		skl_detach_scalers(intel_crtc);
> Also I think it'd be clearer if we have state flags (like for the pageflip
> stuff) in crtc_state to enable these fixup functions on an as-needed
> basis.
I think this call should be moved to the plane commit function instead, with a single
detach_scaler to disable the relevant scaler, if the scaler's not used afterwards.

fastboot updates would disable it for the crtc, eliminating the need to call it separately.

>>  }
>>  
>> @@ -15096,6 +15073,114 @@ intel_check_plane_mapping(struct intel_crtc *crtc)
>>  	return true;
>>  }
>>  
>> +static bool
>> +intel_compare_m_n(unsigned int m, unsigned int n,
>> +		  unsigned int m2, unsigned int n2)
>> +{
>> +	if (m == m2 && n == n2)
>> +		return true;
>> +
>> +	if (!m || !n || !m2 || !n2)
>> +		return false;
>> +
>> +	if (m > m2) {
>> +		while (m > m2) {
>> +			m >>= 1;
>> +			n >>= 1;
>> +		}
>> +	} else if (m < m2) {
>> +		while (m < m2) {
>> +			m2 >>= 1;
>> +			n2 >>= 1;
>> +		}
>> +	}
>> +
>> +	return m == m2 && n == n2;
>> +}
>> +
>> +static bool
>> +intel_compare_link_m_n(const struct intel_link_m_n *m_n,
>> +		       const struct intel_link_m_n *m2_n2)
>> +{
>> +	if (m_n->tu == m2_n2->tu &&
>> +	    intel_compare_m_n(m_n->gmch_m, m_n->gmch_n,
>> +			      m2_n2->gmch_m, m2_n2->gmch_n) &&
>> +	    intel_compare_m_n(m_n->link_m, m_n->link_n,
>> +			      m2_n2->link_m, m2_n2->link_n))
>> +		return true;
>> +
>> +	return false;
>> +}
>> +
>> +static void intel_fastboot_calc_modeset(struct intel_crtc *crtc,
>> +					struct intel_crtc_state *hw_state,
>> +					struct intel_crtc_state *pipe_config)
>> +{
>> +	struct drm_device *dev = crtc->base.dev;
>> +	struct intel_crtc_state sw_state;
>> +
>> +	if (needs_modeset(&pipe_config->base)) {
>> +		DRM_DEBUG_KMS("Skipping fastboot checks, modeset forced\n");
>> +		return;
>> +	}
>> +
>> +	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);
>> +	sw_state.quirks = hw_state->quirks & PIPE_CONFIG_QUIRK_MODE_SYNC_FLAGS;
>> +
>> +	if (intel_compare_link_m_n(&sw_state.fdi_m_n, &hw_state->fdi_m_n))
>> +		sw_state.fdi_m_n = hw_state->fdi_m_n;
>> +
>> +	if (INTEL_INFO(dev)->gen < 8) {
>> +		if (intel_compare_link_m_n(&sw_state.dp_m_n,
>> +					   &hw_state->dp_m_n))
>> +			sw_state.dp_m_n = hw_state->dp_m_n;
>> +
>> +		if (intel_compare_link_m_n(&sw_state.dp_m2_n2,
>> +					   &hw_state->dp_m2_n2))
>> +			sw_state.dp_m2_n2 = hw_state->dp_m2_n2;
>> +	} else {
>> +		if (intel_compare_link_m_n(&sw_state.dp_m_n,
>> +					   &hw_state->dp_m_n)) {
>> +			sw_state.dp_m_n = hw_state->dp_m_n;
>> +			hw_state->dp_m2_n2 = sw_state.dp_m2_n2;
>> +		} else if (intel_compare_link_m_n(&sw_state.dp_m2_n2,
>> +						  &hw_state->dp_m_n)) {
>> +			sw_state.dp_m2_n2 = hw_state->dp_m_n;
>> +
>> +			hw_state->dp_m_n = sw_state.dp_m_n;
>> +			hw_state->dp_m2_n2 = sw_state.dp_m2_n2;
>> +		}
>> +	}
> Not too happy with splitting up the modeset state compare code like this.
> If we add enum mode { EXACT, COMPATIBLE } to intel_pipe_config_compare we
> could tie things together well and have all the pipe config code grouped
> in one place. But then the _compare() is a bit misleading since it'd also
> update the sw_state, so maybe better to call it compare_and_ajust.
Or pass a bool adjust, since checking state after a modeset needs to match exactly,
and no adjustment can be done.

> The macros would then either copy the hw_state to sw_state (if it's some
> intermediate state we don't care about) or compare it (with maybe some
> adjustments made). But I'd really like to have all that compare/adjust
> logic in one place if possible.
>
>> +
>> +	/* Check if we need to force a modeset */
>> +	if (!intel_pipe_config_compare(dev, &sw_state, hw_state, true)) {
>> +		DRM_INFO("[CRTC:%i] sw state incompatible, forcing modeset\n",
>> +			 crtc->base.base.id);
>> +		pipe_config->base.mode_changed = true;
>> +	} else {
>> +		DRM_INFO("[CRTC:%i] sw state and hw state fastboot compatible\n",
>> +			 crtc->base.base.id);
>> +
>> +		*pipe_config = sw_state;
>> +		intel_dump_pipe_config(crtc, pipe_config,
>> +				       "[new fastboot state]");
>> +
>> +		/* prevent a modeset by patching up mode struct */
>> +
>> +		hw_state->base.mode.type = pipe_config->base.mode.type =
>> +		hw_state->base.adjusted_mode.type = pipe_config->base.adjusted_mode.type;
>> +
>> +		/* clock readout can be approximate, just copy it. */
>> +		hw_state->base.mode.clock = pipe_config->base.mode.clock = pipe_config->base.adjusted_mode.clock;
> Why do we still need this? If we use intel_pipe_config_compare to compute
> mode_changed then we hopefully don't need to hack around things any more.
> I guess this is where we need your plan to split out connector_changed
> from mode_changed and then replace the mode_changed computed by the
> helpers by what intel_pipe_config_compare things is justified.
My display has a slightly different clock and mode->type in edid. Patching this up prevented the modeset.
There can also be other reasons for modeset, like CDCLK changes.

>> +	}
>> +}
>> +
>>  static void intel_sanitize_crtc(struct intel_crtc *crtc,
>>  				struct intel_crtc_state *pipe_config,
>>  				bool force_restore)
>> @@ -15135,37 +15220,11 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc,
>>  		pipe_config->base.active = !!enable;
>>  	}
>>  
>> -	if (hw_state->quirks & PIPE_CONFIG_QUIRK_WRONG_PLANE) {
>> -		if (force_restore || i915.fastboot)
>> -			pipe_config->base.mode_changed = true;
>> -		else
>> -			pipe_config->base.active = false;
>> -	}
>> -
>> -	/* XXX: This is needs to be modified for making fastboot possible
>> -	 * by default without requiring a modeset.
>> -	 */
>> -	if (hw_state->base.active && pipe_config->base.active) {
>> -		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, !i915.fastboot)) {
>> -			DRM_DEBUG_KMS("sw and hw state differ, forcing modeset\n");
>> -			pipe_config->base.mode_changed = true;
>> -		} else {
>> -			*pipe_config = sw_state;
>> -		}
>> -	}
>> +	if (hw_state->quirks & PIPE_CONFIG_QUIRK_WRONG_PLANE)
>> +		pipe_config->base.mode_changed = true;
>>  
>> +	else if (hw_state->base.active && pipe_config->base.active)
>> +		intel_fastboot_calc_modeset(crtc, hw_state, pipe_config);
>>  
>>  	if (dev_priv->quirks & QUIRK_PIPEA_FORCE && !hw_state->base.active &&
>>  	    crtc->pipe == PIPE_A && !pipe_config->base.active) {
>> @@ -15584,6 +15643,7 @@ intel_modeset_setup_hw_state(struct drm_device *dev, bool force_restore)
>>  		crtc_state->planes_changed = false;
>>  
>>  		if (crtc->state->enable) {
>> +			memset(&crtc->mode, 0, sizeof(crtc->mode));
>>  			intel_mode_from_pipe_config(&crtc->mode,
>>  				to_intel_crtc_state(crtc->state));
>>  			intel_mode_from_pipe_config(&crtc->state->adjusted_mode,
>> @@ -15678,9 +15738,12 @@ void intel_modeset_gem_init(struct drm_device *dev, struct drm_atomic_state *sta
>>  		int j;
>>  		struct drm_plane *primary = c->primary;
>>  
>> +		if (!c->state->active)
>> +			continue;
>> +
>>  		obj = intel_fb_obj(primary->state->fb);
>>  		if (obj == NULL)
>> -			goto disable;
>> +			continue;
>>  
>>  		mutex_lock(&dev->struct_mutex);
>>  		ret = intel_pin_and_fence_fb_obj(primary, primary->state->fb,
>> @@ -15715,6 +15778,8 @@ void intel_modeset_gem_init(struct drm_device *dev, struct drm_atomic_state *sta
>>  		 */
>>  
>>  disable:
>> +		DRM_DEBUG_KMS("[CRTC:%i] No fb or forced inactive, disabling.\n",
>> +			      c->base.id);
>>  		crtc_state->active = crtc_state->enable = false;
>>  
>>  		ret = drm_atomic_set_mode_for_crtc(crtc_state, NULL);
>> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
>> index 6ac4990a0c11..2c494d78652a 100644
>> --- a/drivers/gpu/drm/i915/intel_fbdev.c
>> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
>> @@ -486,15 +486,10 @@ retry:
>>  			 * config, not the input mode, which is what crtc->mode
>>  			 * usually contains. But since our current fastboot
>>  			 * code puts a mode derived from the post-pfit timings
>> -			 * into crtc->mode this works out correctly. We don't
>> -			 * use hwmode anywhere right now, so use it for this
>> -			 * since the fb helper layer wants a pointer to
>> -			 * something we own.
>> +			 * into crtc->mode this works out correctly.
>>  			 */
>>  			DRM_DEBUG_KMS("looking for current mode on connector %s\n",
>>  				      connector->name);
>> -			intel_mode_from_pipe_config(&encoder->crtc->hwmode,
>> -						    to_intel_crtc(encoder->crtc)->config);
>>  			modes[i] = &encoder->crtc->hwmode;
>>  		}
>>  		crtcs[i] = new_crtc;
>> @@ -584,9 +579,6 @@ static bool intel_fbdev_init_bios(struct drm_device *dev,
>>  	struct intel_crtc *intel_crtc;
>>  	unsigned int max_size = 0;
>>  
>> -	if (!i915.fastboot)
>> -		return false;
> I think it'd be useful to split the fbdev part of the fastboot removal out
> into a separate patch. Or do I miss a depency here? Assuming ofc that we
> enable the pfit trick for modesets in general.
>
I think you did miss a dependency. Not competely sure though..

~Maarten
Daniel Vetter June 23, 2015, 11:50 a.m. UTC | #3
On Tue, Jun 23, 2015 at 12:38:50PM +0200, Maarten Lankhorst wrote:
> Op 22-06-15 om 17:21 schreef Daniel Vetter:
> >> -	/*
> >> -	 * FIXME: BIOS likes to set up a cloned config with lvds+external
> >> -	 * screen. Since we don't yet re-compute the pipe config when moving
> >> -	 * just the lvds port away to another pipe the sw tracking won't match.
> >> -	 *
> >> -	 * Proper atomic modesets with recomputed global state will fix this.
> >> -	 * Until then just don't check gmch state for inherited modes.
> >> -	 */
> >>  	if (!PIPE_CONF_QUIRK(PIPE_CONFIG_QUIRK_INHERITED_MODE)) {
> > This seems like a separate change - with recomputing state on all pipes
> > unconditionally we should be able to drop this?
> We repurpose inherited mode here to skip some checks on first boot that
> will be patched up on first atomic commit.

Yeah, but since you delete the FIXME comment (I agree that's adressed)
there shouldn't be any reason any more for this. If there still is then we
need a new comment imo. And we do read out pfit state correctly, so this
shouldn't cause any pipe config mismatches on the initial state.

> >> @@ -13147,7 +13122,10 @@ static int __intel_set_mode(struct drm_atomic_state *state)
> >>  		if (needs_modeset(crtc->state) && crtc->state->active) {
> >>  			update_scanline_offset(to_intel_crtc(crtc));
> >>  			dev_priv->display.crtc_enable(crtc);
> >> -		}
> >> +		} else if (to_intel_crtc_state(crtc_state)->quirks &
> >> +			   PIPE_CONFIG_QUIRK_INHERITED_MODE)
> >> +			/* force hw readout check */
> >> +			any_ms = true;
> > Imo this should be a separate patch since forcing a modeset check after
> > the initial takeover is a good thing. Also maybe do a
> > s/any_ms/do_modeset_checks/ for clarity of what this does.
> It's used for other reasons too, like updating power domains when crtc
> states change. So do_modeset_checks would be misleading.

Yeah I got confused reading code since it looks so different. any_ms
totally does not what I thought it does, and the name is good as-is.

> >>  
> >>  		drm_atomic_helper_commit_planes_on_crtc(crtc_state);
> >>  	}
> >> @@ -13430,8 +13408,6 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
> >>  	if (ret)
> >>  		goto out;
> >>  
> >> -	intel_update_pipe_size(to_intel_crtc(set->crtc));
> >> -
> >>  	ret = intel_set_mode_checked(state);
> >>  	if (ret) {
> >>  		DRM_DEBUG_KMS("failed to set mode on [CRTC:%d], err = %d\n",
> >> @@ -13718,10 +13694,6 @@ intel_commit_primary_plane(struct drm_plane *plane,
> >>  	if (!crtc->state->active)
> >>  		return;
> >>  
> >> -	if (state->visible)
> >> -		/* FIXME: kill this fastboot hack */
> >> -		intel_update_pipe_size(intel_crtc);
> >> -
> >>  	dev_priv->display.update_primary_plane(crtc, fb, crtc->x, crtc->y);
> >>  }
> >>  
> >> @@ -13741,6 +13713,8 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
> >>  	struct drm_device *dev = crtc->dev;
> >>  	struct drm_i915_private *dev_priv = dev->dev_private;
> >>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >> +	struct intel_crtc_state *old_intel_state =
> >> +		to_intel_crtc_state(old_crtc_state);
> >>  
> >>  	if (!needs_modeset(crtc->state))
> >>  		intel_pre_plane_update(intel_crtc);
> >> @@ -13756,7 +13730,10 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
> >>  			intel_pipe_update_start(intel_crtc,
> >>  						&intel_crtc->atomic.start_vbl_count);
> >>  
> >> -	if (!needs_modeset(crtc->state) && INTEL_INFO(dev)->gen >= 9)
> >> +	if (!needs_modeset(crtc->state) && crtc->state->active &&
> >> +	    old_intel_state->quirks & PIPE_CONFIG_QUIRK_INHERITED_MODE)
> >> +		intel_update_fastboot(intel_crtc, old_intel_state);
> > Nack, fastboot only for the initial mode is not what I want. We should be
> > able to speed up _any_ modeset where we just disable the pfit. Allowing
> > fast pfit removal unconditionally will also allow us to easily test this
> > part of fastboot with an igt, which I'd also like to see.
> You're right, but I think because this needs more infrastructure. I only want to convert fastboot at first.
> For MST we might also need a variant which only performs a pipe modeset, rather than a full modeset.
> Still there are cases where we will need to force a modeset regardless, for example when device frequency
> changes.
> 
> In either case this probably requires a lot of code may differ on each platform. :(

This isn't about anything else but fastboot: I just want the pfit-disable
trick to work on _any_ modeset, since if we don't do that then we can't
test it with igt. And that's bad. pfit-disabling really should be
orthogonal to any inherited state cleanup, if it's not the good isn't yet
ready for prime time imo.

> >> +	else if (!needs_modeset(crtc->state) && INTEL_INFO(dev)->gen >= 9)
> >>  		skl_detach_scalers(intel_crtc);
> > Also I think it'd be clearer if we have state flags (like for the pageflip
> > stuff) in crtc_state to enable these fixup functions on an as-needed
> > basis.
> I think this call should be moved to the plane commit function instead, with a single
> detach_scaler to disable the relevant scaler, if the scaler's not used afterwards.
> 
> fastboot updates would disable it for the crtc, eliminating the need to call it separately.

Yeah that makes sense.
> 
> >>  }
> >>  
> >> @@ -15096,6 +15073,114 @@ intel_check_plane_mapping(struct intel_crtc *crtc)
> >>  	return true;
> >>  }
> >>  
> >> +static bool
> >> +intel_compare_m_n(unsigned int m, unsigned int n,
> >> +		  unsigned int m2, unsigned int n2)
> >> +{
> >> +	if (m == m2 && n == n2)
> >> +		return true;
> >> +
> >> +	if (!m || !n || !m2 || !n2)
> >> +		return false;
> >> +
> >> +	if (m > m2) {
> >> +		while (m > m2) {
> >> +			m >>= 1;
> >> +			n >>= 1;
> >> +		}
> >> +	} else if (m < m2) {
> >> +		while (m < m2) {
> >> +			m2 >>= 1;
> >> +			n2 >>= 1;
> >> +		}
> >> +	}
> >> +
> >> +	return m == m2 && n == n2;
> >> +}
> >> +
> >> +static bool
> >> +intel_compare_link_m_n(const struct intel_link_m_n *m_n,
> >> +		       const struct intel_link_m_n *m2_n2)
> >> +{
> >> +	if (m_n->tu == m2_n2->tu &&
> >> +	    intel_compare_m_n(m_n->gmch_m, m_n->gmch_n,
> >> +			      m2_n2->gmch_m, m2_n2->gmch_n) &&
> >> +	    intel_compare_m_n(m_n->link_m, m_n->link_n,
> >> +			      m2_n2->link_m, m2_n2->link_n))
> >> +		return true;
> >> +
> >> +	return false;
> >> +}
> >> +
> >> +static void intel_fastboot_calc_modeset(struct intel_crtc *crtc,
> >> +					struct intel_crtc_state *hw_state,
> >> +					struct intel_crtc_state *pipe_config)
> >> +{
> >> +	struct drm_device *dev = crtc->base.dev;
> >> +	struct intel_crtc_state sw_state;
> >> +
> >> +	if (needs_modeset(&pipe_config->base)) {
> >> +		DRM_DEBUG_KMS("Skipping fastboot checks, modeset forced\n");
> >> +		return;
> >> +	}
> >> +
> >> +	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);
> >> +	sw_state.quirks = hw_state->quirks & PIPE_CONFIG_QUIRK_MODE_SYNC_FLAGS;
> >> +
> >> +	if (intel_compare_link_m_n(&sw_state.fdi_m_n, &hw_state->fdi_m_n))
> >> +		sw_state.fdi_m_n = hw_state->fdi_m_n;
> >> +
> >> +	if (INTEL_INFO(dev)->gen < 8) {
> >> +		if (intel_compare_link_m_n(&sw_state.dp_m_n,
> >> +					   &hw_state->dp_m_n))
> >> +			sw_state.dp_m_n = hw_state->dp_m_n;
> >> +
> >> +		if (intel_compare_link_m_n(&sw_state.dp_m2_n2,
> >> +					   &hw_state->dp_m2_n2))
> >> +			sw_state.dp_m2_n2 = hw_state->dp_m2_n2;
> >> +	} else {
> >> +		if (intel_compare_link_m_n(&sw_state.dp_m_n,
> >> +					   &hw_state->dp_m_n)) {
> >> +			sw_state.dp_m_n = hw_state->dp_m_n;
> >> +			hw_state->dp_m2_n2 = sw_state.dp_m2_n2;
> >> +		} else if (intel_compare_link_m_n(&sw_state.dp_m2_n2,
> >> +						  &hw_state->dp_m_n)) {
> >> +			sw_state.dp_m2_n2 = hw_state->dp_m_n;
> >> +
> >> +			hw_state->dp_m_n = sw_state.dp_m_n;
> >> +			hw_state->dp_m2_n2 = sw_state.dp_m2_n2;
> >> +		}
> >> +	}
> > Not too happy with splitting up the modeset state compare code like this.
> > If we add enum mode { EXACT, COMPATIBLE } to intel_pipe_config_compare we
> > could tie things together well and have all the pipe config code grouped
> > in one place. But then the _compare() is a bit misleading since it'd also
> > update the sw_state, so maybe better to call it compare_and_ajust.
> Or pass a bool adjust, since checking state after a modeset needs to match exactly,
> and no adjustment can be done.

Yeah adjust is a better name. The point behind the enum is that if you
have a pile of bool arguments it's hard to know without looking at the
function what's going on. With enums you can go with self-descriptive
names. We're not doing this yet everywhere, but as a rule I'd like to see
it if there's more than one bool in the arg list.

> > The macros would then either copy the hw_state to sw_state (if it's some
> > intermediate state we don't care about) or compare it (with maybe some
> > adjustments made). But I'd really like to have all that compare/adjust
> > logic in one place if possible.
> >
> >> +
> >> +	/* Check if we need to force a modeset */
> >> +	if (!intel_pipe_config_compare(dev, &sw_state, hw_state, true)) {
> >> +		DRM_INFO("[CRTC:%i] sw state incompatible, forcing modeset\n",
> >> +			 crtc->base.base.id);
> >> +		pipe_config->base.mode_changed = true;
> >> +	} else {
> >> +		DRM_INFO("[CRTC:%i] sw state and hw state fastboot compatible\n",
> >> +			 crtc->base.base.id);
> >> +
> >> +		*pipe_config = sw_state;
> >> +		intel_dump_pipe_config(crtc, pipe_config,
> >> +				       "[new fastboot state]");
> >> +
> >> +		/* prevent a modeset by patching up mode struct */
> >> +
> >> +		hw_state->base.mode.type = pipe_config->base.mode.type =
> >> +		hw_state->base.adjusted_mode.type = pipe_config->base.adjusted_mode.type;
> >> +
> >> +		/* clock readout can be approximate, just copy it. */
> >> +		hw_state->base.mode.clock = pipe_config->base.mode.clock = pipe_config->base.adjusted_mode.clock;
> > Why do we still need this? If we use intel_pipe_config_compare to compute
> > mode_changed then we hopefully don't need to hack around things any more.
> > I guess this is where we need your plan to split out connector_changed
> > from mode_changed and then replace the mode_changed computed by the
> > helpers by what intel_pipe_config_compare things is justified.
> My display has a slightly different clock and mode->type in edid. Patching this up prevented the modeset.
> There can also be other reasons for modeset, like CDCLK changes.

Yeah I know we need to patch this up, but we should patch up the requested
mode. Instead allowing some slight mismatches in the adjusted mode (and
copying over the dpll state if that's the case) should be all that's
needed.

Maybe helpers should also look at adjusted modes instead of requested
modes? Would be a big change, but I think it makes sense in general -
adjusted_mode is the one the crtc is supposed to output. That would also
help with handling pfit changes correctly in i915.

> 
> >> +	}
> >> +}
> >> +
> >>  static void intel_sanitize_crtc(struct intel_crtc *crtc,
> >>  				struct intel_crtc_state *pipe_config,
> >>  				bool force_restore)
> >> @@ -15135,37 +15220,11 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc,
> >>  		pipe_config->base.active = !!enable;
> >>  	}
> >>  
> >> -	if (hw_state->quirks & PIPE_CONFIG_QUIRK_WRONG_PLANE) {
> >> -		if (force_restore || i915.fastboot)
> >> -			pipe_config->base.mode_changed = true;
> >> -		else
> >> -			pipe_config->base.active = false;
> >> -	}
> >> -
> >> -	/* XXX: This is needs to be modified for making fastboot possible
> >> -	 * by default without requiring a modeset.
> >> -	 */
> >> -	if (hw_state->base.active && pipe_config->base.active) {
> >> -		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, !i915.fastboot)) {
> >> -			DRM_DEBUG_KMS("sw and hw state differ, forcing modeset\n");
> >> -			pipe_config->base.mode_changed = true;
> >> -		} else {
> >> -			*pipe_config = sw_state;
> >> -		}
> >> -	}
> >> +	if (hw_state->quirks & PIPE_CONFIG_QUIRK_WRONG_PLANE)
> >> +		pipe_config->base.mode_changed = true;
> >>  
> >> +	else if (hw_state->base.active && pipe_config->base.active)
> >> +		intel_fastboot_calc_modeset(crtc, hw_state, pipe_config);
> >>  
> >>  	if (dev_priv->quirks & QUIRK_PIPEA_FORCE && !hw_state->base.active &&
> >>  	    crtc->pipe == PIPE_A && !pipe_config->base.active) {
> >> @@ -15584,6 +15643,7 @@ intel_modeset_setup_hw_state(struct drm_device *dev, bool force_restore)
> >>  		crtc_state->planes_changed = false;
> >>  
> >>  		if (crtc->state->enable) {
> >> +			memset(&crtc->mode, 0, sizeof(crtc->mode));
> >>  			intel_mode_from_pipe_config(&crtc->mode,
> >>  				to_intel_crtc_state(crtc->state));
> >>  			intel_mode_from_pipe_config(&crtc->state->adjusted_mode,
> >> @@ -15678,9 +15738,12 @@ void intel_modeset_gem_init(struct drm_device *dev, struct drm_atomic_state *sta
> >>  		int j;
> >>  		struct drm_plane *primary = c->primary;
> >>  
> >> +		if (!c->state->active)
> >> +			continue;
> >> +
> >>  		obj = intel_fb_obj(primary->state->fb);
> >>  		if (obj == NULL)
> >> -			goto disable;
> >> +			continue;
> >>  
> >>  		mutex_lock(&dev->struct_mutex);
> >>  		ret = intel_pin_and_fence_fb_obj(primary, primary->state->fb,
> >> @@ -15715,6 +15778,8 @@ void intel_modeset_gem_init(struct drm_device *dev, struct drm_atomic_state *sta
> >>  		 */
> >>  
> >>  disable:
> >> +		DRM_DEBUG_KMS("[CRTC:%i] No fb or forced inactive, disabling.\n",
> >> +			      c->base.id);
> >>  		crtc_state->active = crtc_state->enable = false;
> >>  
> >>  		ret = drm_atomic_set_mode_for_crtc(crtc_state, NULL);
> >> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> >> index 6ac4990a0c11..2c494d78652a 100644
> >> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> >> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> >> @@ -486,15 +486,10 @@ retry:
> >>  			 * config, not the input mode, which is what crtc->mode
> >>  			 * usually contains. But since our current fastboot
> >>  			 * code puts a mode derived from the post-pfit timings
> >> -			 * into crtc->mode this works out correctly. We don't
> >> -			 * use hwmode anywhere right now, so use it for this
> >> -			 * since the fb helper layer wants a pointer to
> >> -			 * something we own.
> >> +			 * into crtc->mode this works out correctly.
> >>  			 */
> >>  			DRM_DEBUG_KMS("looking for current mode on connector %s\n",
> >>  				      connector->name);
> >> -			intel_mode_from_pipe_config(&encoder->crtc->hwmode,
> >> -						    to_intel_crtc(encoder->crtc)->config);
> >>  			modes[i] = &encoder->crtc->hwmode;
> >>  		}
> >>  		crtcs[i] = new_crtc;
> >> @@ -584,9 +579,6 @@ static bool intel_fbdev_init_bios(struct drm_device *dev,
> >>  	struct intel_crtc *intel_crtc;
> >>  	unsigned int max_size = 0;
> >>  
> >> -	if (!i915.fastboot)
> >> -		return false;
> > I think it'd be useful to split the fbdev part of the fastboot removal out
> > into a separate patch. Or do I miss a depency here? Assuming ofc that we
> > enable the pfit trick for modesets in general.
> >
> I think you did miss a dependency. Not competely sure though..

Well yeah that's likely, but since I didn't see it you need to help me out
;-)
-Daniel

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 8ac5a1b29ac0..9b344a956ba6 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -41,7 +41,6 @@  struct i915_params i915 __read_mostly = {
 	.preliminary_hw_support = IS_ENABLED(CONFIG_DRM_I915_PRELIMINARY_HW_SUPPORT),
 	.disable_power_well = 1,
 	.enable_ips = 1,
-	.fastboot = 0,
 	.prefault_disable = 0,
 	.load_detect_test = 0,
 	.reset = true,
@@ -137,10 +136,6 @@  MODULE_PARM_DESC(disable_power_well,
 module_param_named(enable_ips, i915.enable_ips, int, 0600);
 MODULE_PARM_DESC(enable_ips, "Enable IPS (default: true)");
 
-module_param_named(fastboot, i915.fastboot, bool, 0600);
-MODULE_PARM_DESC(fastboot,
-	"Try to skip unnecessary mode sets at boot time (default: false)");
-
 module_param_named_unsafe(prefault_disable, i915.prefault_disable, bool, 0600);
 MODULE_PARM_DESC(prefault_disable,
 	"Disable page prefaulting for pread/pwrite/reloc (default:false). "
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9b2acc7b4e29..de975ef09e23 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 skylake_pfit_enable(struct intel_crtc *crtc);
 static struct drm_atomic_state *
 intel_modeset_setup_hw_state(struct drm_device *dev, bool force_restore);
 
@@ -3289,14 +3290,17 @@  static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc)
 	return pending;
 }
 
-static void intel_update_pipe_size(struct intel_crtc *crtc)
+static void intel_update_fastboot(struct intel_crtc *crtc,
+				  struct intel_crtc_state *old_crtc_state)
 {
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	const struct drm_display_mode *adjusted_mode;
+	struct intel_crtc_state *pipe_config =
+		to_intel_crtc_state(crtc->base.state);
 
-	if (!i915.fastboot)
-		return;
+	DRM_DEBUG_KMS("Applying fastboot quirks, %ix%i -> %ix%i\n",
+		      old_crtc_state->pipe_src_w, old_crtc_state->pipe_src_h,
+		      pipe_config->pipe_src_w, pipe_config->pipe_src_h);
 
 	/*
 	 * Update pipe size and adjust fitter if needed: the reason for this is
@@ -3305,27 +3309,27 @@  static void intel_update_pipe_size(struct intel_crtc *crtc)
 	 * fastboot case, we'll flip, but if we don't update the pipesrc and
 	 * pfit state, we'll end up with a big fb scanned out into the wrong
 	 * sized surface.
-	 *
-	 * To fix this properly, we need to hoist the checks up into
-	 * compute_mode_changes (or above), check the actual pfit state and
-	 * whether the platform allows pfit disable with pipe active, and only
-	 * then update the pipesrc and pfit state, even on the flip path.
 	 */
 
-	adjusted_mode = &crtc->config->base.adjusted_mode;
-
 	I915_WRITE(PIPESRC(crtc->pipe),
-		   ((adjusted_mode->crtc_hdisplay - 1) << 16) |
-		   (adjusted_mode->crtc_vdisplay - 1));
-	if (!crtc->config->pch_pfit.enabled &&
-	    (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS) ||
-	     intel_pipe_has_type(crtc, INTEL_OUTPUT_EDP))) {
+		   ((pipe_config->pipe_src_w - 1) << 16) |
+		   (pipe_config->pipe_src_h - 1));
+
+	/* on skylake this is done by detaching scalers */
+	if (INTEL_INFO(dev)->gen == 9) {
+		skl_detach_scalers(crtc);
+
+		if (pipe_config->pch_pfit.enabled)
+			skylake_pfit_enable(crtc);
+	}
+	else if (!pipe_config->pch_pfit.enabled &&
+	    old_crtc_state->pch_pfit.enabled) {
+		DRM_DEBUG_KMS("Disabling panel fitter\n");
+
 		I915_WRITE(PF_CTL(crtc->pipe), 0);
 		I915_WRITE(PF_WIN_POS(crtc->pipe), 0);
 		I915_WRITE(PF_WIN_SZ(crtc->pipe), 0);
 	}
-	crtc->config->pipe_src_w = adjusted_mode->crtc_hdisplay;
-	crtc->config->pipe_src_h = adjusted_mode->crtc_vdisplay;
 }
 
 static void intel_fdi_normal_train(struct drm_crtc *crtc)
@@ -12244,19 +12248,6 @@  encoder_retry:
 	DRM_DEBUG_KMS("plane bpp: %i, pipe bpp: %i, dithering: %i\n",
 		      base_bpp, pipe_config->pipe_bpp, pipe_config->dither);
 
-	/* Check if we need to force a modeset */
-	if (pipe_config->has_audio !=
-	    to_intel_crtc_state(crtc->state)->has_audio) {
-		pipe_config->base.mode_changed = true;
-		ret = drm_atomic_add_affected_planes(state, crtc);
-	}
-
-	/*
-	 * Note we have an issue here with infoframes: current code
-	 * only updates them on the full mode set path per hw
-	 * requirements.  So here we should be checking for any
-	 * required changes and forcing a mode set.
-	 */
 fail:
 	return ret;
 }
@@ -12500,29 +12491,21 @@  intel_pipe_config_compare(struct drm_device *dev,
 				      DRM_MODE_FLAG_NVSYNC);
 	}
 
-	PIPE_CONF_CHECK_I(pipe_src_w);
-	PIPE_CONF_CHECK_I(pipe_src_h);
-
-	/*
-	 * FIXME: BIOS likes to set up a cloned config with lvds+external
-	 * screen. Since we don't yet re-compute the pipe config when moving
-	 * just the lvds port away to another pipe the sw tracking won't match.
-	 *
-	 * Proper atomic modesets with recomputed global state will fix this.
-	 * Until then just don't check gmch state for inherited modes.
-	 */
 	if (!PIPE_CONF_QUIRK(PIPE_CONFIG_QUIRK_INHERITED_MODE)) {
-		PIPE_CONF_CHECK_I(gmch_pfit.control);
-		/* pfit ratios are autocomputed by the hw on gen4+ */
-		if (INTEL_INFO(dev)->gen < 4)
-			PIPE_CONF_CHECK_I(gmch_pfit.pgm_ratios);
-		PIPE_CONF_CHECK_I(gmch_pfit.lvds_border_bits);
-	}
+		PIPE_CONF_CHECK_I(pipe_src_w);
+		PIPE_CONF_CHECK_I(pipe_src_h);
+
+		PIPE_CONF_CHECK_I(pch_pfit.enabled);
+		if (current_config->pch_pfit.enabled) {
+			PIPE_CONF_CHECK_I(pch_pfit.pos);
+			PIPE_CONF_CHECK_I(pch_pfit.size);
 
-	PIPE_CONF_CHECK_I(pch_pfit.enabled);
-	if (current_config->pch_pfit.enabled) {
-		PIPE_CONF_CHECK_I(pch_pfit.pos);
-		PIPE_CONF_CHECK_I(pch_pfit.size);
+			PIPE_CONF_CHECK_I(gmch_pfit.control);
+			/* pfit ratios are autocomputed by the hw on gen4+ */
+			if (INTEL_INFO(dev)->gen < 4)
+				PIPE_CONF_CHECK_I(gmch_pfit.pgm_ratios);
+			PIPE_CONF_CHECK_I(gmch_pfit.lvds_border_bits);
+		}
 	}
 
 	PIPE_CONF_CHECK_I(scaler_state.scaler_id);
@@ -13057,26 +13040,18 @@  intel_modeset_compute_config(struct drm_atomic_state *state)
 		return ret;
 
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
-		if (!crtc_state->enable) {
-			if (needs_modeset(crtc_state))
-				any_ms = true;
+		if (!needs_modeset(crtc_state))
 			continue;
-		}
 
-		if (!needs_modeset(crtc_state)) {
-			ret = drm_atomic_add_affected_connectors(state, crtc);
-			if (ret)
-				return ret;
-		}
+		any_ms = true;
+		if (!crtc_state->enable)
+			continue;
 
 		ret = intel_modeset_pipe_config(crtc,
 					to_intel_crtc_state(crtc_state));
 		if (ret)
 			return ret;
 
-		if (needs_modeset(crtc_state))
-			any_ms = true;
-
 		intel_dump_pipe_config(to_intel_crtc(crtc),
 				       to_intel_crtc_state(crtc_state),
 				       "[modeset]");
@@ -13147,7 +13122,10 @@  static int __intel_set_mode(struct drm_atomic_state *state)
 		if (needs_modeset(crtc->state) && crtc->state->active) {
 			update_scanline_offset(to_intel_crtc(crtc));
 			dev_priv->display.crtc_enable(crtc);
-		}
+		} else if (to_intel_crtc_state(crtc_state)->quirks &
+			   PIPE_CONFIG_QUIRK_INHERITED_MODE)
+			/* force hw readout check */
+			any_ms = true;
 
 		drm_atomic_helper_commit_planes_on_crtc(crtc_state);
 	}
@@ -13430,8 +13408,6 @@  static int intel_crtc_set_config(struct drm_mode_set *set)
 	if (ret)
 		goto out;
 
-	intel_update_pipe_size(to_intel_crtc(set->crtc));
-
 	ret = intel_set_mode_checked(state);
 	if (ret) {
 		DRM_DEBUG_KMS("failed to set mode on [CRTC:%d], err = %d\n",
@@ -13718,10 +13694,6 @@  intel_commit_primary_plane(struct drm_plane *plane,
 	if (!crtc->state->active)
 		return;
 
-	if (state->visible)
-		/* FIXME: kill this fastboot hack */
-		intel_update_pipe_size(intel_crtc);
-
 	dev_priv->display.update_primary_plane(crtc, fb, crtc->x, crtc->y);
 }
 
@@ -13741,6 +13713,8 @@  static void intel_begin_crtc_commit(struct drm_crtc *crtc,
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct intel_crtc_state *old_intel_state =
+		to_intel_crtc_state(old_crtc_state);
 
 	if (!needs_modeset(crtc->state))
 		intel_pre_plane_update(intel_crtc);
@@ -13756,7 +13730,10 @@  static void intel_begin_crtc_commit(struct drm_crtc *crtc,
 			intel_pipe_update_start(intel_crtc,
 						&intel_crtc->atomic.start_vbl_count);
 
-	if (!needs_modeset(crtc->state) && INTEL_INFO(dev)->gen >= 9)
+	if (!needs_modeset(crtc->state) && crtc->state->active &&
+	    old_intel_state->quirks & PIPE_CONFIG_QUIRK_INHERITED_MODE)
+		intel_update_fastboot(intel_crtc, old_intel_state);
+	else if (!needs_modeset(crtc->state) && INTEL_INFO(dev)->gen >= 9)
 		skl_detach_scalers(intel_crtc);
 }
 
@@ -15096,6 +15073,114 @@  intel_check_plane_mapping(struct intel_crtc *crtc)
 	return true;
 }
 
+static bool
+intel_compare_m_n(unsigned int m, unsigned int n,
+		  unsigned int m2, unsigned int n2)
+{
+	if (m == m2 && n == n2)
+		return true;
+
+	if (!m || !n || !m2 || !n2)
+		return false;
+
+	if (m > m2) {
+		while (m > m2) {
+			m >>= 1;
+			n >>= 1;
+		}
+	} else if (m < m2) {
+		while (m < m2) {
+			m2 >>= 1;
+			n2 >>= 1;
+		}
+	}
+
+	return m == m2 && n == n2;
+}
+
+static bool
+intel_compare_link_m_n(const struct intel_link_m_n *m_n,
+		       const struct intel_link_m_n *m2_n2)
+{
+	if (m_n->tu == m2_n2->tu &&
+	    intel_compare_m_n(m_n->gmch_m, m_n->gmch_n,
+			      m2_n2->gmch_m, m2_n2->gmch_n) &&
+	    intel_compare_m_n(m_n->link_m, m_n->link_n,
+			      m2_n2->link_m, m2_n2->link_n))
+		return true;
+
+	return false;
+}
+
+static void intel_fastboot_calc_modeset(struct intel_crtc *crtc,
+					struct intel_crtc_state *hw_state,
+					struct intel_crtc_state *pipe_config)
+{
+	struct drm_device *dev = crtc->base.dev;
+	struct intel_crtc_state sw_state;
+
+	if (needs_modeset(&pipe_config->base)) {
+		DRM_DEBUG_KMS("Skipping fastboot checks, modeset forced\n");
+		return;
+	}
+
+	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);
+	sw_state.quirks = hw_state->quirks & PIPE_CONFIG_QUIRK_MODE_SYNC_FLAGS;
+
+	if (intel_compare_link_m_n(&sw_state.fdi_m_n, &hw_state->fdi_m_n))
+		sw_state.fdi_m_n = hw_state->fdi_m_n;
+
+	if (INTEL_INFO(dev)->gen < 8) {
+		if (intel_compare_link_m_n(&sw_state.dp_m_n,
+					   &hw_state->dp_m_n))
+			sw_state.dp_m_n = hw_state->dp_m_n;
+
+		if (intel_compare_link_m_n(&sw_state.dp_m2_n2,
+					   &hw_state->dp_m2_n2))
+			sw_state.dp_m2_n2 = hw_state->dp_m2_n2;
+	} else {
+		if (intel_compare_link_m_n(&sw_state.dp_m_n,
+					   &hw_state->dp_m_n)) {
+			sw_state.dp_m_n = hw_state->dp_m_n;
+			hw_state->dp_m2_n2 = sw_state.dp_m2_n2;
+		} else if (intel_compare_link_m_n(&sw_state.dp_m2_n2,
+						  &hw_state->dp_m_n)) {
+			sw_state.dp_m2_n2 = hw_state->dp_m_n;
+
+			hw_state->dp_m_n = sw_state.dp_m_n;
+			hw_state->dp_m2_n2 = sw_state.dp_m2_n2;
+		}
+	}
+
+	/* Check if we need to force a modeset */
+	if (!intel_pipe_config_compare(dev, &sw_state, hw_state, true)) {
+		DRM_INFO("[CRTC:%i] sw state incompatible, forcing modeset\n",
+			 crtc->base.base.id);
+		pipe_config->base.mode_changed = true;
+	} else {
+		DRM_INFO("[CRTC:%i] sw state and hw state fastboot compatible\n",
+			 crtc->base.base.id);
+
+		*pipe_config = sw_state;
+		intel_dump_pipe_config(crtc, pipe_config,
+				       "[new fastboot state]");
+
+		/* prevent a modeset by patching up mode struct */
+
+		hw_state->base.mode.type = pipe_config->base.mode.type =
+		hw_state->base.adjusted_mode.type = pipe_config->base.adjusted_mode.type;
+
+		/* clock readout can be approximate, just copy it. */
+		hw_state->base.mode.clock = pipe_config->base.mode.clock = pipe_config->base.adjusted_mode.clock;
+	}
+}
+
 static void intel_sanitize_crtc(struct intel_crtc *crtc,
 				struct intel_crtc_state *pipe_config,
 				bool force_restore)
@@ -15135,37 +15220,11 @@  static void intel_sanitize_crtc(struct intel_crtc *crtc,
 		pipe_config->base.active = !!enable;
 	}
 
-	if (hw_state->quirks & PIPE_CONFIG_QUIRK_WRONG_PLANE) {
-		if (force_restore || i915.fastboot)
-			pipe_config->base.mode_changed = true;
-		else
-			pipe_config->base.active = false;
-	}
-
-	/* XXX: This is needs to be modified for making fastboot possible
-	 * by default without requiring a modeset.
-	 */
-	if (hw_state->base.active && pipe_config->base.active) {
-		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, !i915.fastboot)) {
-			DRM_DEBUG_KMS("sw and hw state differ, forcing modeset\n");
-			pipe_config->base.mode_changed = true;
-		} else {
-			*pipe_config = sw_state;
-		}
-	}
+	if (hw_state->quirks & PIPE_CONFIG_QUIRK_WRONG_PLANE)
+		pipe_config->base.mode_changed = true;
 
+	else if (hw_state->base.active && pipe_config->base.active)
+		intel_fastboot_calc_modeset(crtc, hw_state, pipe_config);
 
 	if (dev_priv->quirks & QUIRK_PIPEA_FORCE && !hw_state->base.active &&
 	    crtc->pipe == PIPE_A && !pipe_config->base.active) {
@@ -15584,6 +15643,7 @@  intel_modeset_setup_hw_state(struct drm_device *dev, bool force_restore)
 		crtc_state->planes_changed = false;
 
 		if (crtc->state->enable) {
+			memset(&crtc->mode, 0, sizeof(crtc->mode));
 			intel_mode_from_pipe_config(&crtc->mode,
 				to_intel_crtc_state(crtc->state));
 			intel_mode_from_pipe_config(&crtc->state->adjusted_mode,
@@ -15678,9 +15738,12 @@  void intel_modeset_gem_init(struct drm_device *dev, struct drm_atomic_state *sta
 		int j;
 		struct drm_plane *primary = c->primary;
 
+		if (!c->state->active)
+			continue;
+
 		obj = intel_fb_obj(primary->state->fb);
 		if (obj == NULL)
-			goto disable;
+			continue;
 
 		mutex_lock(&dev->struct_mutex);
 		ret = intel_pin_and_fence_fb_obj(primary, primary->state->fb,
@@ -15715,6 +15778,8 @@  void intel_modeset_gem_init(struct drm_device *dev, struct drm_atomic_state *sta
 		 */
 
 disable:
+		DRM_DEBUG_KMS("[CRTC:%i] No fb or forced inactive, disabling.\n",
+			      c->base.id);
 		crtc_state->active = crtc_state->enable = false;
 
 		ret = drm_atomic_set_mode_for_crtc(crtc_state, NULL);
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 6ac4990a0c11..2c494d78652a 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -486,15 +486,10 @@  retry:
 			 * config, not the input mode, which is what crtc->mode
 			 * usually contains. But since our current fastboot
 			 * code puts a mode derived from the post-pfit timings
-			 * into crtc->mode this works out correctly. We don't
-			 * use hwmode anywhere right now, so use it for this
-			 * since the fb helper layer wants a pointer to
-			 * something we own.
+			 * into crtc->mode this works out correctly.
 			 */
 			DRM_DEBUG_KMS("looking for current mode on connector %s\n",
 				      connector->name);
-			intel_mode_from_pipe_config(&encoder->crtc->hwmode,
-						    to_intel_crtc(encoder->crtc)->config);
 			modes[i] = &encoder->crtc->hwmode;
 		}
 		crtcs[i] = new_crtc;
@@ -584,9 +579,6 @@  static bool intel_fbdev_init_bios(struct drm_device *dev,
 	struct intel_crtc *intel_crtc;
 	unsigned int max_size = 0;
 
-	if (!i915.fastboot)
-		return false;
-
 	/* Find the largest fb */
 	for_each_crtc(dev, crtc) {
 		struct intel_framebuffer *plane_fb =