[04/42] drm/i915: use intel_crtc_control everywhere
diff mbox

Message ID 1431354318-11995-5-git-send-email-maarten.lankhorst@linux.intel.com
State New
Headers show

Commit Message

Maarten Lankhorst May 11, 2015, 2:24 p.m. UTC
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(-)

Comments

Daniel Vetter May 11, 2015, 5:11 p.m. UTC | #1
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
Maarten Lankhorst May 12, 2015, 12:06 p.m. UTC | #2
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
Daniel Vetter May 12, 2015, 1:16 p.m. UTC | #3
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
Daniel Stone May 12, 2015, 2:38 p.m. UTC | #4
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

Patch
diff mbox

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 *);