On Wed, May 13, 2015 at 10:23:37PM +0200, Maarten Lankhorst wrote:
> crtc_state->enable means a crtc is configured, but it may be turned
> off for dpms. Until the previous commit crtc_state->active was not
> updated on crtc off, but now that we do we should use that for tracking
> whether a crtc is scanning out or not.
>
> At this point crtc->active should mirror crtc_state->active,
> so some paranoia from the crtc_disable functions can be removed.
>
> Note that intel_set_mode_setup_plls still checks for ->enable,
> because all resources that are needed have to be calculated, so
> dpms changes will still succeed.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
A few detail comments below.
-Daniel
> ---
> drivers/gpu/drm/i915/i915_irq.c | 2 +-
> drivers/gpu/drm/i915/intel_display.c | 44 ++++++++++++++++++------------------
> 2 files changed, 23 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 557af8877a2e..ca457317a8ac 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -796,7 +796,7 @@ static int i915_get_vblank_timestamp(struct drm_device *dev, int pipe,
> return -EINVAL;
> }
>
> - if (!crtc->state->enable) {
> + if (!crtc->state->active) {
This change looks unjustified I think.
> DRM_DEBUG_KMS("crtc %d is disabled\n", pipe);
> return -EBUSY;
> }
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index db1b47febb27..583c9105cf49 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4667,7 +4667,7 @@ static void intel_crtc_load_lut(struct drm_crtc *crtc)
> bool reenable_ips = false;
>
> /* The clocks have to be on to load the palette. */
> - if (!crtc->state->enable || !intel_crtc->active)
> + if (!crtc->state->active)
> return;
>
> if (HAS_GMCH_DISPLAY(dev_priv->dev)) {
> @@ -4872,9 +4872,7 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
> struct intel_encoder *encoder;
> int pipe = intel_crtc->pipe;
>
> - WARN_ON(!crtc->state->enable);
> -
> - if (intel_crtc->active)
> + if (WARN_ON(intel_crtc->active))
> return;
>
> if (intel_crtc->config->has_pch_encoder)
> @@ -4978,9 +4976,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
> struct intel_encoder *encoder;
> int pipe = intel_crtc->pipe;
>
> - WARN_ON(!crtc->state->enable);
> -
> - if (intel_crtc->active)
> + if (WARN_ON(intel_crtc->active))
> return;
>
> if (intel_crtc_to_shared_dpll(intel_crtc))
> @@ -5082,7 +5078,7 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
> int pipe = intel_crtc->pipe;
> u32 reg, temp;
>
> - if (!intel_crtc->active)
> + if (WARN_ON(!intel_crtc->active))
> return;
>
> for_each_encoder_on_crtc(dev, crtc, encoder)
> @@ -5144,7 +5140,7 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
> struct intel_encoder *encoder;
> enum transcoder cpu_transcoder = intel_crtc->config->cpu_transcoder;
>
> - if (!intel_crtc->active)
> + if (WARN_ON(!intel_crtc->active))
> return;
>
> for_each_encoder_on_crtc(dev, crtc, encoder) {
> @@ -5742,7 +5738,7 @@ static int valleyview_modeset_global_pipes(struct drm_atomic_state *state)
>
> /* add all active pipes to the state */
> for_each_crtc(state->dev, crtc) {
> - if (!crtc->state->enable)
> + if (!crtc->state->active)
This is a functional change to the cdclk code and might break it and/or
conflict with the ongoing cdclk work from Ville/Mika. Definitely needs to
be split out.
> continue;
>
> crtc_state = drm_atomic_get_crtc_state(state, crtc);
> @@ -5752,7 +5748,7 @@ static int valleyview_modeset_global_pipes(struct drm_atomic_state *state)
>
> /* disable/enable all currently active pipes while we change cdclk */
> for_each_crtc_in_state(state, crtc, crtc_state, i)
> - if (crtc_state->enable)
> + if (crtc_state->active)
> crtc_state->mode_changed = true;
Same here.
Hm, aside of all that maybe we should drop vlv_modeset_global_pipes and
instead just look at crtc_state->mode_changed? That way we don't need to
duplicate the same checks twice, once to set ->mode_changed and once to
for the prepare_pipes mask. Or is that duplication already getting
removed?
>
> return 0;
> @@ -5840,9 +5836,7 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
> int pipe = intel_crtc->pipe;
> bool is_dsi;
>
> - WARN_ON(!crtc->state->enable);
> -
> - if (intel_crtc->active)
> + if (WARN_ON(intel_crtc->active))
> return;
>
> is_dsi = intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DSI);
> @@ -5918,9 +5912,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
> struct intel_encoder *encoder;
> int pipe = intel_crtc->pipe;
>
> - WARN_ON(!crtc->state->enable);
> -
> - if (intel_crtc->active)
> + if (WARN_ON(intel_crtc->active))
> return;
>
> i9xx_set_pll_dividers(intel_crtc);
> @@ -5980,7 +5972,7 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
> struct intel_encoder *encoder;
> int pipe = intel_crtc->pipe;
>
> - if (!intel_crtc->active)
> + if (WARN_ON(!intel_crtc->active))
> return;
>
> /*
> @@ -11553,7 +11545,7 @@ intel_modeset_update_state(struct drm_atomic_state *state)
> if (!crtc_state || !needs_modeset(crtc->state))
> continue;
>
> - if (crtc->state->enable) {
> + if (crtc->state->active) {
> struct drm_property *dpms_property =
> dev->mode_config.dpms_property;
>
> @@ -11970,6 +11962,10 @@ check_crtc_state(struct drm_device *dev)
> "crtc active state doesn't match with hw state "
> "(expected %i, found %i)\n", crtc->active, active);
>
> + I915_STATE_WARN(crtc->active != crtc->base.state->active,
> + "transitional active state does not match atomic hw state "
> + "(expected %i, found %i)\n", crtc->base.state->active, crtc->active);
> +
> if (active &&
> !intel_pipe_config_compare(dev, crtc->config, &pipe_config)) {
> I915_STATE_WARN(1, "pipe state doesn't match!\n");
> @@ -12115,6 +12111,10 @@ intel_modeset_compute_config(struct drm_crtc *crtc,
> if (IS_ERR(pipe_config))
> return pipe_config;
>
> + if (!pipe_config->base.enable &&
> + WARN_ON(pipe_config->base.active))
> + pipe_config->base.active = false;
> +
> if (!pipe_config->base.enable)
> return pipe_config;
>
> @@ -12239,7 +12239,7 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc,
> return ret;
>
> for_each_crtc_in_state(state, crtc, crtc_state, i) {
> - if (!needs_modeset(crtc_state))
> + if (!needs_modeset(crtc_state) || !crtc->state->active)
> continue;
>
> intel_crtc_disable_planes(crtc);
> @@ -12261,7 +12261,7 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc,
>
> /* Now enable the clocks, plane, pipe, and connectors that we set up. */
> for_each_crtc_in_state(state, crtc, crtc_state, i) {
> - if (!needs_modeset(crtc->state) || !crtc->state->enable)
> + if (!needs_modeset(crtc->state) || !crtc->state->active)
> continue;
>
> update_scanline_offset(to_intel_crtc(crtc));
> @@ -14494,7 +14494,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
> * have active connectors/encoders. */
> intel_crtc_update_dpms(&crtc->base);
>
> - if (crtc->active != crtc->base.state->enable) {
> + if (crtc->active != crtc->base.state->active) {
> struct intel_encoder *encoder;
>
> /* This can happen either due to bugs in the get_hw_state
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
