| Message ID | 1431354318-11995-5-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, May 11, 2015 at 04:24:40PM +0200, Maarten Lankhorst wrote: > @@ -6079,26 +6059,29 @@ void intel_crtc_control(struct drm_crtc *crtc, bool enable) > enum intel_display_power_domain domain; > unsigned long domains; > > + if (enable == intel_crtc->active) > + return; > + > + if (enable && !crtc->state->enable) > + return; I think we only need to check for !state->enable here. Changing dpms while the crtc is fully of is totally legit. And at least -modesetting loves to do just that iirc. I'll will be caught by state->active implying state->enable, but that's hard to read imo. -Daniel
Op 11-05-15 om 19:11 schreef Daniel Vetter: > On Mon, May 11, 2015 at 04:24:40PM +0200, Maarten Lankhorst wrote: >> @@ -6079,26 +6059,29 @@ void intel_crtc_control(struct drm_crtc *crtc, bool enable) >> enum intel_display_power_domain domain; >> unsigned long domains; >> >> + if (enable == intel_crtc->active) >> + return; >> + >> + if (enable && !crtc->state->enable) >> + return; > I think we only need to check for !state->enable here. Changing dpms while > the crtc is fully of is totally legit. And at least -modesetting loves to > do just that iirc. > > I'll will be caught by state->active implying state->enable, but that's > hard to read imo. > -Daniel As discussed on irc it's not. :-) ~Maarten
On Tue, May 12, 2015 at 02:06:39PM +0200, Maarten Lankhorst wrote: > Op 11-05-15 om 19:11 schreef Daniel Vetter: > > On Mon, May 11, 2015 at 04:24:40PM +0200, Maarten Lankhorst wrote: > >> @@ -6079,26 +6059,29 @@ void intel_crtc_control(struct drm_crtc *crtc, bool enable) > >> enum intel_display_power_domain domain; > >> unsigned long domains; > >> > >> + if (enable == intel_crtc->active) > >> + return; > >> + > >> + if (enable && !crtc->state->enable) > >> + return; > > I think we only need to check for !state->enable here. Changing dpms while > > the crtc is fully of is totally legit. And at least -modesetting loves to > > do just that iirc. > > > > I'll will be caught by state->active implying state->enable, but that's > > hard to read imo. > > -Daniel > As discussed on irc it's not. :-) Hm there's actually a bug in drm_atomic_helper_connector_dpms I think ... we seem to unconditionally update crtc_state->active. Oh I'm missing that ->enable == false also implies that no connectors are connected to the crtc, so we can't ever end up setting this to true. So indeed changing active while enable == false is impossible. With that is it really possible to see have that early return at all? Should we wrap it in a WARN_ON as impossible? -Daniel
Hi, On 12 May 2015 at 14:16, Daniel Vetter <daniel@ffwll.ch> wrote: > On Tue, May 12, 2015 at 02:06:39PM +0200, Maarten Lankhorst wrote: >> Op 11-05-15 om 19:11 schreef Daniel Vetter: >> > On Mon, May 11, 2015 at 04:24:40PM +0200, Maarten Lankhorst wrote: >> >> @@ -6079,26 +6059,29 @@ void intel_crtc_control(struct drm_crtc *crtc, bool enable) >> >> enum intel_display_power_domain domain; >> >> unsigned long domains; >> >> >> >> + if (enable == intel_crtc->active) >> >> + return; >> >> + >> >> + if (enable && !crtc->state->enable) >> >> + return; >> > I think we only need to check for !state->enable here. Changing dpms while >> > the crtc is fully of is totally legit. And at least -modesetting loves to >> > do just that iirc. >> > >> > I'll will be caught by state->active implying state->enable, but that's >> > hard to read imo. >> > -Daniel >> As discussed on irc it's not. :-) > > Hm there's actually a bug in drm_atomic_helper_connector_dpms I think ... > we seem to unconditionally update crtc_state->active. > > Oh I'm missing that ->enable == false also implies that no connectors are > connected to the crtc, so we can't ever end up setting this to true. So > indeed changing active while enable == false is impossible. Not the only place we seem to rather carelessly do this, mind: https://git.collabora.com/cgit/user/daniels/linux.git/commit/?h=wip/4.1.x/unify-flip-modeset&id=132a1d4086e20cac95094c7fd568f992e4a0cb6d Cheers, Daniel
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index adbbddab42c6..acd4d2c7613a 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -3606,12 +3606,18 @@ static void hsw_trans_edp_pipe_A_crc_wa(struct drm_device *dev) */ if (crtc->config->cpu_transcoder == TRANSCODER_EDP && !crtc->config->pch_pfit.enabled) { + bool active = crtc->active; + + if (active) + intel_crtc_control(&crtc->base, false); + crtc->config->pch_pfit.force_thru = true; intel_display_power_get(dev_priv, POWER_DOMAIN_PIPE_PANEL_FITTER(PIPE_A)); - intel_crtc_reset(crtc); + if (active) + intel_crtc_control(&crtc->base, true); } drm_modeset_unlock_all(dev); } @@ -3630,12 +3636,18 @@ static void hsw_undo_trans_edp_pipe_A_crc_wa(struct drm_device *dev) * routing. */ if (crtc->config->pch_pfit.force_thru) { - crtc->config->pch_pfit.force_thru = false; + bool active = crtc->active; - intel_crtc_reset(crtc); + if (active) + intel_crtc_control(&crtc->base, false); + + crtc->config->pch_pfit.force_thru = false; intel_display_power_put(dev_priv, POWER_DOMAIN_PIPE_PANEL_FITTER(PIPE_A)); + + if (active) + intel_crtc_control(&crtc->base, true); } drm_modeset_unlock_all(dev); } diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 42d0cc329b37..70269da6a6b8 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3206,22 +3206,8 @@ static void intel_update_primary_planes(struct drm_device *dev) } } -void intel_crtc_reset(struct intel_crtc *crtc) -{ - struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); - - if (!crtc->active) - return; - - intel_crtc_disable_planes(&crtc->base); - dev_priv->display.crtc_disable(&crtc->base); - dev_priv->display.crtc_enable(&crtc->base); - intel_crtc_enable_planes(&crtc->base); -} - void intel_prepare_reset(struct drm_device *dev) { - struct drm_i915_private *dev_priv = to_i915(dev); struct intel_crtc *crtc; /* no reset support for gen2 */ @@ -3233,18 +3219,12 @@ void intel_prepare_reset(struct drm_device *dev) return; drm_modeset_lock_all(dev); - /* * Disabling the crtcs gracefully seems nicer. Also the * g33 docs say we should at least disable all the planes. */ - for_each_intel_crtc(dev, crtc) { - if (!crtc->active) - continue; - - intel_crtc_disable_planes(&crtc->base); - dev_priv->display.crtc_disable(&crtc->base); - } + for_each_intel_crtc(dev, crtc) + intel_crtc_control(&crtc->base, false); } void intel_finish_reset(struct drm_device *dev) @@ -6079,26 +6059,29 @@ void intel_crtc_control(struct drm_crtc *crtc, bool enable) enum intel_display_power_domain domain; unsigned long domains; + if (enable == intel_crtc->active) + return; + + if (enable && !crtc->state->enable) + return; + + crtc->state->active = enable; if (enable) { - if (!intel_crtc->active) { - domains = get_crtc_power_domains(crtc); - for_each_power_domain(domain, domains) - intel_display_power_get(dev_priv, domain); - intel_crtc->enabled_power_domains = domains; - - dev_priv->display.crtc_enable(crtc); - intel_crtc_enable_planes(crtc); - } + domains = get_crtc_power_domains(crtc); + for_each_power_domain(domain, domains) + intel_display_power_get(dev_priv, domain); + intel_crtc->enabled_power_domains = domains; + + dev_priv->display.crtc_enable(crtc); + intel_crtc_enable_planes(crtc); } else { - if (intel_crtc->active) { - intel_crtc_disable_planes(crtc); - dev_priv->display.crtc_disable(crtc); - - domains = intel_crtc->enabled_power_domains; - for_each_power_domain(domain, domains) - intel_display_power_put(dev_priv, domain); - intel_crtc->enabled_power_domains = 0; - } + intel_crtc_disable_planes(crtc); + dev_priv->display.crtc_disable(crtc); + + domains = intel_crtc->enabled_power_domains; + for_each_power_domain(domain, domains) + intel_display_power_put(dev_priv, domain); + intel_crtc->enabled_power_domains = 0; } } @@ -6115,8 +6098,6 @@ void intel_crtc_update_dpms(struct drm_crtc *crtc) enable |= intel_encoder->connectors_active; intel_crtc_control(crtc, enable); - - crtc->state->active = enable; } void intel_encoder_destroy(struct drm_encoder *encoder) @@ -14543,8 +14524,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc) plane = crtc->plane; to_intel_plane_state(crtc->base.primary->state)->visible = true; crtc->plane = !plane; - intel_crtc_disable_planes(&crtc->base); - dev_priv->display.crtc_disable(&crtc->base); + intel_crtc_control(&crtc->base, false); crtc->plane = plane; /* ... and break all links. */ diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 4c690403942c..7390fe9ba97d 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -993,7 +993,6 @@ void intel_mark_busy(struct drm_device *dev); void intel_mark_idle(struct drm_device *dev); void intel_crtc_restore_mode(struct drm_crtc *crtc); void intel_crtc_control(struct drm_crtc *crtc, bool enable); -void intel_crtc_reset(struct intel_crtc *crtc); void intel_crtc_update_dpms(struct drm_crtc *crtc); void intel_encoder_destroy(struct drm_encoder *encoder); int intel_connector_init(struct intel_connector *);
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/i915/i915_debugfs.c | 18 ++++++++-- drivers/gpu/drm/i915/intel_display.c | 68 +++++++++++++----------------------- drivers/gpu/drm/i915/intel_drv.h | 1 - 3 files changed, 39 insertions(+), 48 deletions(-)