[35/58] drm/i915: introduce struct intel_set_config

Message ID	1345403595-9678-36-git-send-email-daniel.vetter@ffwll.ch (mailing list archive)
State	Accepted
Headers	show Return-Path: <intel-gfx-bounces+patchwork-intel-gfx=patchwork.kernel.org@lists.freedesktop.org> From: Daniel Vetter <daniel.vetter@ffwll.ch> To: Intel Graphics Development <intel-gfx@lists.freedesktop.org> Date: Sun, 19 Aug 2012 21:12:52 +0200 Message-Id: <1345403595-9678-36-git-send-email-daniel.vetter@ffwll.ch> In-Reply-To: <1345403595-9678-1-git-send-email-daniel.vetter@ffwll.ch> References: <1345403595-9678-1-git-send-email-daniel.vetter@ffwll.ch> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Subject: [Intel-gfx] [PATCH 35/58] drm/i915: introduce struct intel_set_config Precedence: list MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: intel-gfx-bounces+patchwork-intel-gfx=patchwork.kernel.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+patchwork-intel-gfx=patchwork.kernel.org@lists.freedesktop.org

Message ID

1345403595-9678-36-git-send-email-daniel.vetter@ffwll.ch (mailing list archive)

State

Accepted

Headers

From: Daniel Vetter <daniel.vetter@ffwll.ch>
To: Intel Graphics Development <intel-gfx@lists.freedesktop.org>
Date: Sun, 19 Aug 2012 21:12:52 +0200
Message-Id: <1345403595-9678-36-git-send-email-daniel.vetter@ffwll.ch>
In-Reply-To: <1345403595-9678-1-git-send-email-daniel.vetter@ffwll.ch>
References: <1345403595-9678-1-git-send-email-daniel.vetter@ffwll.ch>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Subject: [Intel-gfx] [PATCH 35/58] drm/i915: introduce struct
	intel_set_config
Precedence: list
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Sender: intel-gfx-bounces+patchwork-intel-gfx=patchwork.kernel.org@lists.freedesktop.org
Errors-To: intel-gfx-bounces+patchwork-intel-gfx=patchwork.kernel.org@lists.freedesktop.org

Commit Message

Daniel Vetter Aug. 19, 2012, 7:12 p.m. UTC

intel_crtc_set_config is an unwidly beast and is in serious need of
some function extraction. To facilitate that, introduce a struct to
keep track of all the state involved. Atm it doesn't do much more than
keep track of all the allocated memory.

Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 73 ++++++++++++++++++++----------------
 drivers/gpu/drm/i915/intel_drv.h     |  6 +++
 2 files changed, 47 insertions(+), 32 deletions(-)

Comments

Jesse Barnes Sept. 5, 2012, 4:34 p.m. UTC | #1

On Sun, 19 Aug 2012 21:12:52 +0200
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> intel_crtc_set_config is an unwidly beast and is in serious need of
> some function extraction. To facilitate that, introduce a struct to
> keep track of all the state involved. Atm it doesn't do much more than
> keep track of all the allocated memory.
> 
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 73 ++++++++++++++++++++----------------
>  drivers/gpu/drm/i915/intel_drv.h     |  6 +++
>  2 files changed, 47 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index ab4fa7f..63bcc37 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6723,17 +6723,28 @@ done:
>  	return ret;
>  }
>  
> +static void intel_set_config_free(struct intel_set_config *config)
> +{
> +	if (config) {
> +		kfree(config->save_connectors);
> +		kfree(config->save_encoders);
> +		kfree(config->save_crtcs);
> +	}
> +	kfree(config);
> +}

	if (!config)
		return;
?

My eyes always hurt a little when I see if blocks that take up the
whole function. :)  Just a nit pick though.

> +
>  static int intel_crtc_set_config(struct drm_mode_set *set)
>  {
>  	struct drm_device *dev;
> -	struct drm_crtc *save_crtcs, *new_crtc, *crtc;
> -	struct drm_encoder *save_encoders, *new_encoder, *encoder;
> +	struct drm_crtc *new_crtc, *crtc;
> +	struct drm_encoder *new_encoder, *encoder;
>  	struct drm_framebuffer *old_fb = NULL;
>  	bool mode_changed = false; /* if true do a full mode set */
>  	bool fb_changed = false; /* if true and !mode_changed just do a flip */
> -	struct drm_connector *save_connectors, *connector;
> +	struct drm_connector *connector;
>  	int count = 0, ro;
>  	struct drm_mode_set save_set;
> +	struct intel_set_config *config;
>  	int ret;
>  	int i;
>  
> @@ -6762,27 +6773,27 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
>  
>  	dev = set->crtc->dev;
>  
> +	ret = -ENOMEM;
> +	config = kzalloc(sizeof(*config), GFP_KERNEL);
> +	if (!config)
> +		goto out_config;
> +
>  	/* Allocate space for the backup of all (non-pointer) crtc, encoder and
>  	 * connector data. */
> -	save_crtcs = kzalloc(dev->mode_config.num_crtc *
> -			     sizeof(struct drm_crtc), GFP_KERNEL);
> -	if (!save_crtcs)
> -		return -ENOMEM;
> +	config->save_crtcs = kzalloc(dev->mode_config.num_crtc *
> +				     sizeof(struct drm_crtc), GFP_KERNEL);
> +	if (!config->save_crtcs)
> +		goto out_config;
>  
> -	save_encoders = kzalloc(dev->mode_config.num_encoder *
> -				sizeof(struct drm_encoder), GFP_KERNEL);
> -	if (!save_encoders) {
> -		kfree(save_crtcs);
> -		return -ENOMEM;
> -	}
> +	config->save_encoders = kzalloc(dev->mode_config.num_encoder *
> +					sizeof(struct drm_encoder), GFP_KERNEL);
> +	if (!config->save_encoders)
> +		goto out_config;
>  
> -	save_connectors = kzalloc(dev->mode_config.num_connector *
> -				sizeof(struct drm_connector), GFP_KERNEL);
> -	if (!save_connectors) {
> -		kfree(save_crtcs);
> -		kfree(save_encoders);
> -		return -ENOMEM;
> -	}
> +	config->save_connectors = kzalloc(dev->mode_config.num_connector *
> +					  sizeof(struct drm_connector), GFP_KERNEL);
> +	if (!config->save_connectors)
> +		goto out_config;
>  
>  	/* Copy data. Note that driver private data is not affected.
>  	 * Should anything bad happen only the expected state is
> @@ -6790,17 +6801,17 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
>  	 */
>  	count = 0;
>  	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> -		save_crtcs[count++] = *crtc;
> +		config->save_crtcs[count++] = *crtc;
>  	}
>  
>  	count = 0;
>  	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) {
> -		save_encoders[count++] = *encoder;
> +		config->save_encoders[count++] = *encoder;
>  	}
>  
>  	count = 0;
>  	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
> -		save_connectors[count++] = *connector;
> +		config->save_connectors[count++] = *connector;
>  	}
>  
>  	save_set.crtc = set->crtc;
> @@ -6936,26 +6947,25 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
>  		}
>  	}
>  
> -	kfree(save_connectors);
> -	kfree(save_encoders);
> -	kfree(save_crtcs);
> +	intel_set_config_free(config);
> +
>  	return 0;
>  
>  fail:
>  	/* Restore all previous data. */
>  	count = 0;
>  	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> -		*crtc = save_crtcs[count++];
> +		*crtc = config->save_crtcs[count++];
>  	}
>  
>  	count = 0;
>  	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) {
> -		*encoder = save_encoders[count++];
> +		*encoder = config->save_encoders[count++];
>  	}
>  
>  	count = 0;
>  	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
> -		*connector = save_connectors[count++];
> +		*connector = config->save_connectors[count++];
>  	}
>  
>  	/* Try to restore the config */
> @@ -6964,9 +6974,8 @@ fail:
>  			    save_set.x, save_set.y, save_set.fb))
>  		DRM_ERROR("failed to restore config after modeset failure\n");
>  
> -	kfree(save_connectors);
> -	kfree(save_encoders);
> -	kfree(save_crtcs);
> +out_config:
> +	intel_set_config_free(config);
>  	return ret;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index c080c829..1cb64dd 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -423,6 +423,12 @@ extern void intel_panel_disable_backlight(struct drm_device *dev);
>  extern void intel_panel_destroy_backlight(struct drm_device *dev);
>  extern enum drm_connector_status intel_panel_detect(struct drm_device *dev);
>  
> +struct intel_set_config {
> +	struct drm_connector *save_connectors;
> +	struct drm_encoder *save_encoders;
> +	struct drm_crtc *save_crtcs;
> +};
> +
>  extern bool intel_set_mode(struct drm_crtc *crtc, struct drm_display_mode *mode,
>  			   int x, int y, struct drm_framebuffer *old_fb);
>  extern void intel_crtc_load_lut(struct drm_crtc *crtc);

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Daniel Vetter Sept. 5, 2012, 7:27 p.m. UTC | #2

On Wed, Sep 5, 2012 at 6:34 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>         if (!config)
>                 return;
> ?
>
> My eyes always hurt a little when I see if blocks that take up the
> whole function. :)  Just a nit pick though.

Yeah, looks better. Bikeshed applied locally since it resulted in some
conflicts later on.
-Daniel

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ab4fa7f..63bcc37 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6723,17 +6723,28 @@  done:
 	return ret;
 }
 
+static void intel_set_config_free(struct intel_set_config *config)
+{
+	if (config) {
+		kfree(config->save_connectors);
+		kfree(config->save_encoders);
+		kfree(config->save_crtcs);
+	}
+	kfree(config);
+}
+
 static int intel_crtc_set_config(struct drm_mode_set *set)
 {
 	struct drm_device *dev;
-	struct drm_crtc *save_crtcs, *new_crtc, *crtc;
-	struct drm_encoder *save_encoders, *new_encoder, *encoder;
+	struct drm_crtc *new_crtc, *crtc;
+	struct drm_encoder *new_encoder, *encoder;
 	struct drm_framebuffer *old_fb = NULL;
 	bool mode_changed = false; /* if true do a full mode set */
 	bool fb_changed = false; /* if true and !mode_changed just do a flip */
-	struct drm_connector *save_connectors, *connector;
+	struct drm_connector *connector;
 	int count = 0, ro;
 	struct drm_mode_set save_set;
+	struct intel_set_config *config;
 	int ret;
 	int i;
 
@@ -6762,27 +6773,27 @@  static int intel_crtc_set_config(struct drm_mode_set *set)
 
 	dev = set->crtc->dev;
 
+	ret = -ENOMEM;
+	config = kzalloc(sizeof(*config), GFP_KERNEL);
+	if (!config)
+		goto out_config;
+
 	/* Allocate space for the backup of all (non-pointer) crtc, encoder and
 	 * connector data. */
-	save_crtcs = kzalloc(dev->mode_config.num_crtc *
-			     sizeof(struct drm_crtc), GFP_KERNEL);
-	if (!save_crtcs)
-		return -ENOMEM;
+	config->save_crtcs = kzalloc(dev->mode_config.num_crtc *
+				     sizeof(struct drm_crtc), GFP_KERNEL);
+	if (!config->save_crtcs)
+		goto out_config;
 
-	save_encoders = kzalloc(dev->mode_config.num_encoder *
-				sizeof(struct drm_encoder), GFP_KERNEL);
-	if (!save_encoders) {
-		kfree(save_crtcs);
-		return -ENOMEM;
-	}
+	config->save_encoders = kzalloc(dev->mode_config.num_encoder *
+					sizeof(struct drm_encoder), GFP_KERNEL);
+	if (!config->save_encoders)
+		goto out_config;
 
-	save_connectors = kzalloc(dev->mode_config.num_connector *
-				sizeof(struct drm_connector), GFP_KERNEL);
-	if (!save_connectors) {
-		kfree(save_crtcs);
-		kfree(save_encoders);
-		return -ENOMEM;
-	}
+	config->save_connectors = kzalloc(dev->mode_config.num_connector *
+					  sizeof(struct drm_connector), GFP_KERNEL);
+	if (!config->save_connectors)
+		goto out_config;
 
 	/* Copy data. Note that driver private data is not affected.
 	 * Should anything bad happen only the expected state is
@@ -6790,17 +6801,17 @@  static int intel_crtc_set_config(struct drm_mode_set *set)
 	 */
 	count = 0;
 	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
-		save_crtcs[count++] = *crtc;
+		config->save_crtcs[count++] = *crtc;
 	}
 
 	count = 0;
 	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) {
-		save_encoders[count++] = *encoder;
+		config->save_encoders[count++] = *encoder;
 	}
 
 	count = 0;
 	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
-		save_connectors[count++] = *connector;
+		config->save_connectors[count++] = *connector;
 	}
 
 	save_set.crtc = set->crtc;
@@ -6936,26 +6947,25 @@  static int intel_crtc_set_config(struct drm_mode_set *set)
 		}
 	}
 
-	kfree(save_connectors);
-	kfree(save_encoders);
-	kfree(save_crtcs);
+	intel_set_config_free(config);
+
 	return 0;
 
 fail:
 	/* Restore all previous data. */
 	count = 0;
 	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
-		*crtc = save_crtcs[count++];
+		*crtc = config->save_crtcs[count++];
 	}
 
 	count = 0;
 	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) {
-		*encoder = save_encoders[count++];
+		*encoder = config->save_encoders[count++];
 	}
 
 	count = 0;
 	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
-		*connector = save_connectors[count++];
+		*connector = config->save_connectors[count++];
 	}
 
 	/* Try to restore the config */
@@ -6964,9 +6974,8 @@  fail:
 			    save_set.x, save_set.y, save_set.fb))
 		DRM_ERROR("failed to restore config after modeset failure\n");
 
-	kfree(save_connectors);
-	kfree(save_encoders);
-	kfree(save_crtcs);
+out_config:
+	intel_set_config_free(config);
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index c080c829..1cb64dd 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -423,6 +423,12 @@  extern void intel_panel_disable_backlight(struct drm_device *dev);
 extern void intel_panel_destroy_backlight(struct drm_device *dev);
 extern enum drm_connector_status intel_panel_detect(struct drm_device *dev);
 
+struct intel_set_config {
+	struct drm_connector *save_connectors;
+	struct drm_encoder *save_encoders;
+	struct drm_crtc *save_crtcs;
+};
+
 extern bool intel_set_mode(struct drm_crtc *crtc, struct drm_display_mode *mode,
 			   int x, int y, struct drm_framebuffer *old_fb);
 extern void intel_crtc_load_lut(struct drm_crtc *crtc);

[35/58] drm/i915: introduce struct intel_set_config

Commit Message

Comments

Patch