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

[Jesse Barnes <jbarnes@virtuousgeek.org>]

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>

-- 
Jesse Barnes, Intel Open Source Technology Center