On Sun, 19 Aug 2012 21:12:52 +0200
Daniel Vetter <daniel.vetter at 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 at 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 at virtuousgeek.org>
--
Jesse Barnes, Intel Open Source Technology Center
