[Intel-gfx] [RFC PATCH 4/7] drm/i915: enable fastboot for everyone
Daniel Vetter
daniel at ffwll.ch
Mon Jun 22 08:21:00 PDT 2015
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 at 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the Intel-gfx
mailing list