| Message ID | 1431548627-2527-8-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@linux.intel.com> 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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Op 18-05-15 om 17:30 schreef Daniel Vetter: > 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@linux.intel.com> > 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. Why? Can you get a vblank on crtc that is dpms off? I think not.. Either way later on I use hwmode->crtc_clock which renders this moot. >> <snip> >> @@ -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. No this function just sets mode_changed on all active crtc's. This is done to turn them off while changing the clock. In case they're already turned off you don't have to turn them off. >> 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? I think we could get rid of a lot of those extra loops if we want to later, but for now readability is more important. ~Maarten
On Mon, May 18, 2015 at 06:35:59PM +0200, Maarten Lankhorst wrote: > Op 18-05-15 om 17:30 schreef Daniel Vetter: > > 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@linux.intel.com> > > 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. > Why? Can you get a vblank on crtc that is dpms off? I think not.. > > Either way later on I use hwmode->crtc_clock which renders this moot. > >> <snip> > >> @@ -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. > No this function just sets mode_changed on all active crtc's. > > This is done to turn them off while changing the clock. In case they're already turned off you don't have to turn them off. > > >> 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? > I think we could get rid of a lot of those extra loops if we want to later, > but for now readability is more important. Hm makes sense with your replies - there's a few cases indeed where we need to switch from checking ->enable to ->active if we start using the mode_set machinery for dpms. I think it'd be good to explain that a bit in the commit message for all the different cases, so that reviewers can go hunting for more and make sure there aren't ;-) -Daniel
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) { 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) 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; 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
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@linux.intel.com> --- drivers/gpu/drm/i915/i915_irq.c | 2 +- drivers/gpu/drm/i915/intel_display.c | 44 ++++++++++++++++++------------------ 2 files changed, 23 insertions(+), 23 deletions(-)