diff mbox

[RFC,6/8] drm/i915: Remove intel_crtc->new_config pointer

Message ID 1418052069-14831-7-git-send-email-ander.conselvan.de.oliveira@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ander Conselvan de Oliveira Dec. 8, 2014, 3:21 p.m. UTC 
There are no more users of that pointer since the new config is now
passed down the call chain during mode set. Also, when the switch to
atomic happens, the right config (state) should be derived from an
atomic state structure.
---
 drivers/gpu/drm/i915/intel_display.c | 46 ++++++++----------------------------
 drivers/gpu/drm/i915/intel_drv.h     |  1 -
 2 files changed, 10 insertions(+), 37 deletions(-)

Comments

Daniel Vetter Dec. 8, 2014, 4:36 p.m. UTC | #1 
On Mon, Dec 08, 2014 at 05:21:07PM +0200, Ander Conselvan de Oliveira wrote:
> There are no more users of that pointer since the new config is now
> passed down the call chain during mode set. Also, when the switch to
> atomic happens, the right config (state) should be derived from an
> atomic state structure.

Ah, doesn't seem much work actually to remove our usage of ->new_config.
Which is nice since it'll align us more with how the helpers work.

So overall I think this patch series is good to go (but I've done only a
rather cursory high-level reading).

Thanks, Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c | 46 ++++++++----------------------------
>  drivers/gpu/drm/i915/intel_drv.h     |  1 -
>  2 files changed, 10 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a9f3034..a032a1d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8583,7 +8583,6 @@ retry:
>  
>  	intel_crtc = to_intel_crtc(crtc);
>  	intel_crtc->new_enabled = true;
> -	intel_crtc->new_config = &intel_crtc->config;
>  	old->dpms_mode = connector->dpms;
>  	old->load_detect_temp = true;
>  	old->release_fb = NULL;
> @@ -8623,10 +8622,6 @@ retry:
>  
>   fail:
>  	intel_crtc->new_enabled = crtc->enabled;
> -	if (intel_crtc->new_enabled)
> -		intel_crtc->new_config = &intel_crtc->config;
> -	else
> -		intel_crtc->new_config = NULL;
>  fail_unlock:
>  	if (ret == -EDEADLK) {
>  		drm_modeset_backoff(ctx);
> @@ -8653,7 +8648,6 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
>  		to_intel_connector(connector)->new_encoder = NULL;
>  		intel_encoder->new_crtc = NULL;
>  		intel_crtc->new_enabled = false;
> -		intel_crtc->new_config = NULL;
>  		intel_set_mode(crtc, NULL, 0, 0, NULL);
>  
>  		if (old->release_fb) {
> @@ -9839,14 +9833,8 @@ static void intel_modeset_update_staged_output_state(struct drm_device *dev)
>  			to_intel_crtc(encoder->base.crtc);
>  	}
>  
> -	for_each_intel_crtc(dev, crtc) {
> +	for_each_intel_crtc(dev, crtc)
>  		crtc->new_enabled = crtc->base.enabled;
> -
> -		if (crtc->new_enabled)
> -			crtc->new_config = &crtc->config;
> -		else
> -			crtc->new_config = NULL;
> -	}
>  }
>  
>  /**
> @@ -10355,12 +10343,8 @@ intel_modeset_update_state(struct drm_device *dev, unsigned prepare_pipes)
>  	intel_modeset_commit_output_state(dev);
>  
>  	/* Double check state. */
> -	for_each_intel_crtc(dev, intel_crtc) {
> +	for_each_intel_crtc(dev, intel_crtc)
>  		WARN_ON(intel_crtc->base.enabled != intel_crtc_in_use(&intel_crtc->base));
> -		WARN_ON(intel_crtc->new_config &&
> -			intel_crtc->new_config != &intel_crtc->config);
> -		WARN_ON(intel_crtc->base.enabled != !!intel_crtc->new_config);
> -	}
>  
>  	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
>  		if (!connector->encoder || !connector->encoder->crtc)
> @@ -10957,9 +10941,6 @@ static int __intel_set_mode(struct drm_crtc *crtc,
>  
>  	*saved_mode = crtc->mode;
>  
> -	if (modeset_pipes)
> -		to_intel_crtc(crtc)->new_config = pipe_config;
> -
>  	/*
>  	 * See if the config requires any additional preparation, e.g.
>  	 * to adjust global state with pipes off.  We need to do this
> @@ -10984,7 +10965,13 @@ static int __intel_set_mode(struct drm_crtc *crtc,
>  			goto done;
>  
>  		for_each_intel_crtc_masked(dev, modeset_pipes, intel_crtc) {
> -			struct intel_crtc_state *state = intel_crtc->new_config;
> +			struct intel_crtc_state *state;
> +
> +			if (&intel_crtc->base == crtc)
> +				state = pipe_config;
> +			else
> +				state = intel_crtc->config;
> +
>  			ret = dev_priv->display.crtc_compute_clock(intel_crtc,
>  								   state);
>  			if (ret) {
> @@ -11014,7 +11001,6 @@ static int __intel_set_mode(struct drm_crtc *crtc,
>  		/* mode_set/enable/disable functions rely on a correct pipe
>  		 * config. */
>  		to_intel_crtc(crtc)->config = *pipe_config;
> -		to_intel_crtc(crtc)->new_config = &to_intel_crtc(crtc)->config;
>  
>  		/*
>  		 * Calculate and store various constants which
> @@ -11177,15 +11163,9 @@ static void intel_set_config_restore_state(struct drm_device *dev,
>  	int count;
>  
>  	count = 0;
> -	for_each_intel_crtc(dev, crtc) {
> +	for_each_intel_crtc(dev, crtc)
>  		crtc->new_enabled = config->save_crtc_enabled[count++];
>  
> -		if (crtc->new_enabled)
> -			crtc->new_config = &crtc->config;
> -		else
> -			crtc->new_config = NULL;
> -	}
> -
>  	count = 0;
>  	for_each_intel_encoder(dev, encoder) {
>  		encoder->new_crtc =
> @@ -11391,11 +11371,6 @@ intel_modeset_stage_output_state(struct drm_device *dev,
>  				      crtc->new_enabled ? "en" : "dis");
>  			config->mode_changed = true;
>  		}
> -
> -		if (crtc->new_enabled)
> -			crtc->new_config = &crtc->config;
> -		else
> -			crtc->new_config = NULL;
>  	}
>  
>  	return 0;
> @@ -11422,7 +11397,6 @@ static void disable_crtc_nofb(struct intel_crtc *crtc)
>  	}
>  
>  	crtc->new_enabled = false;
> -	crtc->new_config = NULL;
>  }
>  
>  static int intel_crtc_set_config(struct drm_mode_set *set)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 0a84667..175b853 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -440,7 +440,6 @@ struct intel_crtc {
>  
>  	struct intel_plane_config plane_config;
>  	struct intel_crtc_state config;
> -	struct intel_crtc_state *new_config;
>  	bool new_enabled;
>  
>  	/* reset counter value when the last flip was submitted */
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ander Conselvan de Oliveira Dec. 10, 2014, 1:48 p.m. UTC | #2 
On 12/08/2014 06:36 PM, Daniel Vetter wrote:
> On Mon, Dec 08, 2014 at 05:21:07PM +0200, Ander Conselvan de Oliveira wrote:
>> There are no more users of that pointer since the new config is now
>> passed down the call chain during mode set. Also, when the switch to
>> atomic happens, the right config (state) should be derived from an
>> atomic state structure.
>
> Ah, doesn't seem much work actually to remove our usage of ->new_config.
> Which is nice since it'll align us more with how the helpers work.

Except for the ugliness in patch 5, it was pretty straight forward. But 
that can be fixed once we have a global state object.

> So overall I think this patch series is good to go (but I've done only a
> rather cursory high-level reading).

For some reason PRTS ignored this one (maybe because it is an RFC?), so 
I submitted a task manually. If that succeeds, I'll resend without the 
RFC status.

Cheers,
Ander

>
> Thanks, Daniel
>
>> ---
>>   drivers/gpu/drm/i915/intel_display.c | 46 ++++++++----------------------------
>>   drivers/gpu/drm/i915/intel_drv.h     |  1 -
>>   2 files changed, 10 insertions(+), 37 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index a9f3034..a032a1d 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -8583,7 +8583,6 @@ retry:
>>
>>   	intel_crtc = to_intel_crtc(crtc);
>>   	intel_crtc->new_enabled = true;
>> -	intel_crtc->new_config = &intel_crtc->config;
>>   	old->dpms_mode = connector->dpms;
>>   	old->load_detect_temp = true;
>>   	old->release_fb = NULL;
>> @@ -8623,10 +8622,6 @@ retry:
>>
>>    fail:
>>   	intel_crtc->new_enabled = crtc->enabled;
>> -	if (intel_crtc->new_enabled)
>> -		intel_crtc->new_config = &intel_crtc->config;
>> -	else
>> -		intel_crtc->new_config = NULL;
>>   fail_unlock:
>>   	if (ret == -EDEADLK) {
>>   		drm_modeset_backoff(ctx);
>> @@ -8653,7 +8648,6 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
>>   		to_intel_connector(connector)->new_encoder = NULL;
>>   		intel_encoder->new_crtc = NULL;
>>   		intel_crtc->new_enabled = false;
>> -		intel_crtc->new_config = NULL;
>>   		intel_set_mode(crtc, NULL, 0, 0, NULL);
>>
>>   		if (old->release_fb) {
>> @@ -9839,14 +9833,8 @@ static void intel_modeset_update_staged_output_state(struct drm_device *dev)
>>   			to_intel_crtc(encoder->base.crtc);
>>   	}
>>
>> -	for_each_intel_crtc(dev, crtc) {
>> +	for_each_intel_crtc(dev, crtc)
>>   		crtc->new_enabled = crtc->base.enabled;
>> -
>> -		if (crtc->new_enabled)
>> -			crtc->new_config = &crtc->config;
>> -		else
>> -			crtc->new_config = NULL;
>> -	}
>>   }
>>
>>   /**
>> @@ -10355,12 +10343,8 @@ intel_modeset_update_state(struct drm_device *dev, unsigned prepare_pipes)
>>   	intel_modeset_commit_output_state(dev);
>>
>>   	/* Double check state. */
>> -	for_each_intel_crtc(dev, intel_crtc) {
>> +	for_each_intel_crtc(dev, intel_crtc)
>>   		WARN_ON(intel_crtc->base.enabled != intel_crtc_in_use(&intel_crtc->base));
>> -		WARN_ON(intel_crtc->new_config &&
>> -			intel_crtc->new_config != &intel_crtc->config);
>> -		WARN_ON(intel_crtc->base.enabled != !!intel_crtc->new_config);
>> -	}
>>
>>   	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
>>   		if (!connector->encoder || !connector->encoder->crtc)
>> @@ -10957,9 +10941,6 @@ static int __intel_set_mode(struct drm_crtc *crtc,
>>
>>   	*saved_mode = crtc->mode;
>>
>> -	if (modeset_pipes)
>> -		to_intel_crtc(crtc)->new_config = pipe_config;
>> -
>>   	/*
>>   	 * See if the config requires any additional preparation, e.g.
>>   	 * to adjust global state with pipes off.  We need to do this
>> @@ -10984,7 +10965,13 @@ static int __intel_set_mode(struct drm_crtc *crtc,
>>   			goto done;
>>
>>   		for_each_intel_crtc_masked(dev, modeset_pipes, intel_crtc) {
>> -			struct intel_crtc_state *state = intel_crtc->new_config;
>> +			struct intel_crtc_state *state;
>> +
>> +			if (&intel_crtc->base == crtc)
>> +				state = pipe_config;
>> +			else
>> +				state = intel_crtc->config;
>> +
>>   			ret = dev_priv->display.crtc_compute_clock(intel_crtc,
>>   								   state);
>>   			if (ret) {
>> @@ -11014,7 +11001,6 @@ static int __intel_set_mode(struct drm_crtc *crtc,
>>   		/* mode_set/enable/disable functions rely on a correct pipe
>>   		 * config. */
>>   		to_intel_crtc(crtc)->config = *pipe_config;
>> -		to_intel_crtc(crtc)->new_config = &to_intel_crtc(crtc)->config;
>>
>>   		/*
>>   		 * Calculate and store various constants which
>> @@ -11177,15 +11163,9 @@ static void intel_set_config_restore_state(struct drm_device *dev,
>>   	int count;
>>
>>   	count = 0;
>> -	for_each_intel_crtc(dev, crtc) {
>> +	for_each_intel_crtc(dev, crtc)
>>   		crtc->new_enabled = config->save_crtc_enabled[count++];
>>
>> -		if (crtc->new_enabled)
>> -			crtc->new_config = &crtc->config;
>> -		else
>> -			crtc->new_config = NULL;
>> -	}
>> -
>>   	count = 0;
>>   	for_each_intel_encoder(dev, encoder) {
>>   		encoder->new_crtc =
>> @@ -11391,11 +11371,6 @@ intel_modeset_stage_output_state(struct drm_device *dev,
>>   				      crtc->new_enabled ? "en" : "dis");
>>   			config->mode_changed = true;
>>   		}
>> -
>> -		if (crtc->new_enabled)
>> -			crtc->new_config = &crtc->config;
>> -		else
>> -			crtc->new_config = NULL;
>>   	}
>>
>>   	return 0;
>> @@ -11422,7 +11397,6 @@ static void disable_crtc_nofb(struct intel_crtc *crtc)
>>   	}
>>
>>   	crtc->new_enabled = false;
>> -	crtc->new_config = NULL;
>>   }
>>
>>   static int intel_crtc_set_config(struct drm_mode_set *set)
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 0a84667..175b853 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -440,7 +440,6 @@ struct intel_crtc {
>>
>>   	struct intel_plane_config plane_config;
>>   	struct intel_crtc_state config;
>> -	struct intel_crtc_state *new_config;
>>   	bool new_enabled;
>>
>>   	/* reset counter value when the last flip was submitted */
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a9f3034..a032a1d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8583,7 +8583,6 @@  retry:
 
 	intel_crtc = to_intel_crtc(crtc);
 	intel_crtc->new_enabled = true;
-	intel_crtc->new_config = &intel_crtc->config;
 	old->dpms_mode = connector->dpms;
 	old->load_detect_temp = true;
 	old->release_fb = NULL;
@@ -8623,10 +8622,6 @@  retry:
 
  fail:
 	intel_crtc->new_enabled = crtc->enabled;
-	if (intel_crtc->new_enabled)
-		intel_crtc->new_config = &intel_crtc->config;
-	else
-		intel_crtc->new_config = NULL;
 fail_unlock:
 	if (ret == -EDEADLK) {
 		drm_modeset_backoff(ctx);
@@ -8653,7 +8648,6 @@  void intel_release_load_detect_pipe(struct drm_connector *connector,
 		to_intel_connector(connector)->new_encoder = NULL;
 		intel_encoder->new_crtc = NULL;
 		intel_crtc->new_enabled = false;
-		intel_crtc->new_config = NULL;
 		intel_set_mode(crtc, NULL, 0, 0, NULL);
 
 		if (old->release_fb) {
@@ -9839,14 +9833,8 @@  static void intel_modeset_update_staged_output_state(struct drm_device *dev)
 			to_intel_crtc(encoder->base.crtc);
 	}
 
-	for_each_intel_crtc(dev, crtc) {
+	for_each_intel_crtc(dev, crtc)
 		crtc->new_enabled = crtc->base.enabled;
-
-		if (crtc->new_enabled)
-			crtc->new_config = &crtc->config;
-		else
-			crtc->new_config = NULL;
-	}
 }
 
 /**
@@ -10355,12 +10343,8 @@  intel_modeset_update_state(struct drm_device *dev, unsigned prepare_pipes)
 	intel_modeset_commit_output_state(dev);
 
 	/* Double check state. */
-	for_each_intel_crtc(dev, intel_crtc) {
+	for_each_intel_crtc(dev, intel_crtc)
 		WARN_ON(intel_crtc->base.enabled != intel_crtc_in_use(&intel_crtc->base));
-		WARN_ON(intel_crtc->new_config &&
-			intel_crtc->new_config != &intel_crtc->config);
-		WARN_ON(intel_crtc->base.enabled != !!intel_crtc->new_config);
-	}
 
 	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
 		if (!connector->encoder || !connector->encoder->crtc)
@@ -10957,9 +10941,6 @@  static int __intel_set_mode(struct drm_crtc *crtc,
 
 	*saved_mode = crtc->mode;
 
-	if (modeset_pipes)
-		to_intel_crtc(crtc)->new_config = pipe_config;
-
 	/*
 	 * See if the config requires any additional preparation, e.g.
 	 * to adjust global state with pipes off.  We need to do this
@@ -10984,7 +10965,13 @@  static int __intel_set_mode(struct drm_crtc *crtc,
 			goto done;
 
 		for_each_intel_crtc_masked(dev, modeset_pipes, intel_crtc) {
-			struct intel_crtc_state *state = intel_crtc->new_config;
+			struct intel_crtc_state *state;
+
+			if (&intel_crtc->base == crtc)
+				state = pipe_config;
+			else
+				state = intel_crtc->config;
+
 			ret = dev_priv->display.crtc_compute_clock(intel_crtc,
 								   state);
 			if (ret) {
@@ -11014,7 +11001,6 @@  static int __intel_set_mode(struct drm_crtc *crtc,
 		/* mode_set/enable/disable functions rely on a correct pipe
 		 * config. */
 		to_intel_crtc(crtc)->config = *pipe_config;
-		to_intel_crtc(crtc)->new_config = &to_intel_crtc(crtc)->config;
 
 		/*
 		 * Calculate and store various constants which
@@ -11177,15 +11163,9 @@  static void intel_set_config_restore_state(struct drm_device *dev,
 	int count;
 
 	count = 0;
-	for_each_intel_crtc(dev, crtc) {
+	for_each_intel_crtc(dev, crtc)
 		crtc->new_enabled = config->save_crtc_enabled[count++];
 
-		if (crtc->new_enabled)
-			crtc->new_config = &crtc->config;
-		else
-			crtc->new_config = NULL;
-	}
-
 	count = 0;
 	for_each_intel_encoder(dev, encoder) {
 		encoder->new_crtc =
@@ -11391,11 +11371,6 @@  intel_modeset_stage_output_state(struct drm_device *dev,
 				      crtc->new_enabled ? "en" : "dis");
 			config->mode_changed = true;
 		}
-
-		if (crtc->new_enabled)
-			crtc->new_config = &crtc->config;
-		else
-			crtc->new_config = NULL;
 	}
 
 	return 0;
@@ -11422,7 +11397,6 @@  static void disable_crtc_nofb(struct intel_crtc *crtc)
 	}
 
 	crtc->new_enabled = false;
-	crtc->new_config = NULL;
 }
 
 static int intel_crtc_set_config(struct drm_mode_set *set)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 0a84667..175b853 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -440,7 +440,6 @@  struct intel_crtc {
 
 	struct intel_plane_config plane_config;
 	struct intel_crtc_state config;
-	struct intel_crtc_state *new_config;
 	bool new_enabled;
 
 	/* reset counter value when the last flip was submitted */