Message ID | 5576E93C.3080308@linux.intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Tue, 09 Jun 2015, Maarten Lankhorst <maarten.lankhorst@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@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@intel.com> >>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@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@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@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@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@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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Hey, Op 09-06-15 om 16:24 schreef Jani Nikula: > On Tue, 09 Jun 2015, Maarten Lankhorst <maarten.lankhorst@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@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@intel.com> >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@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... Nasty, part of the fix is in the following series. Could we revert only 490f400db5d886fc28566af69b02f6497f31be4b and 3bae26eb2991c00670df377cf6c3bc2b0577e82a ? First commit for https://bugs.freedesktop.org/show_bug.cgi?id=90868 and because it would break not-atomic resume otherwise. Second commit for https://bugs.freedesktop.org/show_bug.cgi?id=90861 . I'll rework convert to atomic part 3 to incorporate the commits again. ~Maarten
On Tue, 09 Jun 2015, Maarten Lankhorst <maarten.lankhorst@linux.intel.com> wrote: > Hey, > > Op 09-06-15 om 16:24 schreef Jani Nikula: >> On Tue, 09 Jun 2015, Maarten Lankhorst <maarten.lankhorst@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@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@intel.com> >>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@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... > Nasty, part of the fix is in the following series. > > Could we revert only 490f400db5d886fc28566af69b02f6497f31be4b and 3bae26eb2991c00670df377cf6c3bc2b0577e82a ? > > First commit for https://bugs.freedesktop.org/show_bug.cgi?id=90868 and because it would break not-atomic resume otherwise. > Second commit for https://bugs.freedesktop.org/show_bug.cgi?id=90861 . > > I'll rework convert to atomic part 3 to incorporate the commits again. This sounds like a good plan to me. Please send the reverts as patches, citing the bugs and failures. BR, Jani. > > ~Maarten
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@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)