[v2,07/17] drm/i915: Use crtc_state->active instead of crtc_state->enable
diff mbox

Message ID 1431548627-2527-8-git-send-email-maarten.lankhorst@linux.intel.com
State New
Headers show

Commit Message

Maarten Lankhorst May 13, 2015, 8:23 p.m. UTC
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(-)

Comments

Daniel Vetter May 18, 2015, 3:30 p.m. UTC | #1
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
Maarten Lankhorst May 18, 2015, 4:35 p.m. UTC | #2
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
Daniel Vetter May 19, 2015, 8:09 a.m. UTC | #3
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

Patch
diff mbox

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