On Tue, 09 Jun 2015, Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx> 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@xxxxxxxxx> >>> >>> 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@xxxxxxxxx> >>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx> >> >> 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@xxxxxxxxxxxxxxx> > 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@xxxxxxxxx> > 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@xxxxxxxxxxxxxxx> > > 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@xxxxxxxxxxxxxxx> > 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@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
