[PATCH weston v5 09/11] drm: Don't hang onto the backend config object post-backend_init

[Pekka Paalanen]

On Wed, 13 Apr 2016 03:25:13 -0700
Bryce Harrington <bryce at osg.samsung.com> wrote:

> The drm backend was copying most everything out of the config object
> already, but now also copy the use_current_mode parameter and the
> config_output function pointer, so that there are no remaining
> references to the config object passed into backend_init().
> 
> By decoupling the config struct to the backend, if in the future if the
> structure definition changes in non-backwards compatible ways, then any
> version compatibility adaptation will be limited to just the
> backend_init() routine.
> 
> Signed-off-by: Bryce Harrington <bryce at osg.samsung.com>
> ---
>  src/compositor-drm.c | 24 +++++++++++++++---------
>  src/compositor-drm.h |  3 ++-
>  src/main.c           | 18 ++++++++----------
>  3 files changed, 25 insertions(+), 20 deletions(-)
> 
> diff --git a/src/compositor-drm.c b/src/compositor-drm.c
> index 38296e2..722cc6a 100644
> --- a/src/compositor-drm.c
> +++ b/src/compositor-drm.c
> @@ -121,7 +121,17 @@ struct drm_backend {
>  	int32_t cursor_width;
>  	int32_t cursor_height;
>  
> -	struct weston_drm_backend_config *config;
> +        /** Callback used to configure the outputs.
> +	 *
> +         * This function will be called by the backend when a new DRM
> +         * output needs to be configured.
> +         */
> +        enum weston_drm_backend_output_mode

Some whitespace damage here?

> +	(*configure_output)(struct weston_compositor *compositor,
> +			    bool use_current_mode,
> +			    const char *name,
> +			    struct weston_drm_backend_output_config *output_config);
> +	bool use_current_mode;
>  };
>  
>  struct drm_mode {
> @@ -2311,8 +2321,8 @@ create_output_for_connector(struct drm_backend *b,
>  	output->base.serial_number = "unknown";
>  	wl_list_init(&output->base.mode_list);
>  
> -	mode = b->config->configure_output(b->compositor, b->config,
> -					   output->base.name, &config);
> +	mode = b->configure_output(b->compositor, b->use_current_mode,
> +				   output->base.name, &config);
>  	if (parse_gbm_format(config.gbm_format, b->gbm_format, &output->gbm_format) == -1)
>  		output->gbm_format = b->gbm_format;
>  
> @@ -2699,11 +2709,6 @@ drm_destroy(struct weston_compositor *ec)
>  	weston_launcher_destroy(ec->launcher);
>  
>  	close(b->drm.fd);
> -
> -	free(b->config->gbm_format);
> -	free(b->config->seat_id);
> -	free(b->config);
> -
>  	free(b);
>  }
>  
> @@ -3054,7 +3059,8 @@ drm_backend_create(struct weston_compositor *compositor,
>  	b->sprites_are_broken = 1;
>  	b->compositor = compositor;
>  	b->use_pixman = config->use_pixman;
> -	b->config = config;
> +	b->configure_output = config->configure_output;
> +	b->use_current_mode = config->use_current_mode;
>  
>  	if (parse_gbm_format(config->gbm_format, GBM_FORMAT_XRGB8888, &b->gbm_format) < 0)
>  		goto err_compositor;
> diff --git a/src/compositor-drm.h b/src/compositor-drm.h
> index 3b54ca5..7e0913a 100644
> --- a/src/compositor-drm.h
> +++ b/src/compositor-drm.h
> @@ -112,9 +112,10 @@ struct weston_drm_backend_config {
>  	 */
>  	enum weston_drm_backend_output_mode
>  		(*configure_output)(struct weston_compositor *compositor,
> -				    struct weston_drm_backend_config *backend_config,
> +				    bool use_current_mode,
>  				    const char *name,
>  				    struct weston_drm_backend_output_config *output_config);
> +	bool use_current_mode;
>  };
>  
>  #ifdef  __cplusplus
> diff --git a/src/main.c b/src/main.c
> index f652701..c499fd0 100644
> --- a/src/main.c
> +++ b/src/main.c
> @@ -683,11 +683,10 @@ struct drm_config {
>  
>  static enum weston_drm_backend_output_mode
>  drm_configure_output(struct weston_compositor *c,
> -		     struct weston_drm_backend_config *backend_config,
> +		     bool use_current_mode,
>  		     const char *name,
>  		     struct weston_drm_backend_output_config *config)
>  {
> -	struct drm_config *drm_config = (struct drm_config *)backend_config;
>  	struct weston_config *wc = weston_compositor_get_user_data(c);
>  	struct weston_config_section *section;
>  	char *s;
> @@ -702,7 +701,7 @@ drm_configure_output(struct weston_compositor *c,
>  		return WESTON_DRM_BACKEND_OUTPUT_OFF;
>  	}
>  
> -	if (drm_config->use_current_mode || strcmp(s, "current") == 0) {
> +	if (use_current_mode || strcmp(s, "current") == 0) {
>  		mode = WESTON_DRM_BACKEND_OUTPUT_CURRENT;
>  	} else if (strcmp(s, "preferred") != 0) {
>  		config->modeline = s;
> @@ -756,13 +755,12 @@ load_drm_backend(struct weston_compositor *c, const char *backend,
>  	config->base.base.struct_size = sizeof(struct weston_drm_backend_config);
>  	config->base.configure_output = drm_configure_output;
>  
> -	if (load_backend_new(c, backend,
> -			     (struct weston_backend_config *)(&config->base)) < 0) {
> -		ret = -1;
> -		free(config->base.gbm_format);
> -		free(config->base.seat_id);
> -		free(config);
> -	}
> +	ret = load_backend_new(c, backend,
> +			       (struct weston_backend_config *)(&config->base));
> +
> +	free(config->base.gbm_format);
> +	free(config->base.seat_id);
> +	free(config);
>  
>  	return ret;
>  }

Hi,

I never thought it this way. I assumed the backend would take ownership
of the config elements if the init succeeds, but this is indeed more
consistent for the caller (main.c).

The backend would need to make copies of all strings passed in, except
if they are just turned into enum values, there is no need to keep the
string.

Yeah, this is good, I think.


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160413/383c13a5/attachment.sig>