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