[PATCH weston v7 3/3] drm: Don't hang onto the backend config object post-backend_init
Benoit Gschwind
gschwind at gnu-log.net
Sat Apr 30 09:41:15 UTC 2016
Hello Bryce,
I think my comments on previous patch apply here, while you did some
useful update here.
Best regards.
Le 30/04/2016 00:40, Bryce Harrington a écrit :
> 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.
>
> With the use_current_mode moved into the main config class, the
> drm_config wrapper is redundant. Dropping it helps make the drm backend
> config initialization more consistent with the other backends.
>
> Also, enforce destruction of the backend config object after
> initialization. Since the backend config struct versioning implies that
> there we expect potential future descrepancy between main's definition
> of the config object and the backend's, don't allow the backend to hang
> onto the config object outside the initialization scope.
>
> Signed-off-by: Bryce Harrington <bryce at osg.samsung.com>
> Reviewed-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
>
> ---
> v6:
> - Drop use of drm_config config wrapper
> v7:
> - Update to master
> - Put backend configs on stack instead of zalloc()
> - Enforce destruction of backend config object
> (Squashed patch as requested by pq)
>
> src/compositor-drm.c | 24 +++++++++++++++---------
> src/compositor-drm.h | 3 ++-
> src/main.c | 46 ++++++++++++++++------------------------------
> 3 files changed, 33 insertions(+), 40 deletions(-)
>
> diff --git a/src/compositor-drm.c b/src/compositor-drm.c
> index 2384fd2..6ef706a 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
> + (*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 fdf5154..3b2dc17 100644
> --- a/src/compositor-drm.h
> +++ b/src/compositor-drm.h
> @@ -114,9 +114,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 02b3278..745d527 100644
> --- a/src/main.c
> +++ b/src/main.c
> @@ -688,18 +688,12 @@ load_backend_new(struct weston_compositor *compositor, const char *backend,
> return backend_init(compositor, NULL, NULL, NULL, config_base);
> }
>
> -struct drm_config {
> - struct weston_drm_backend_config base;
> - bool use_current_mode;
> -};
> -
> 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;
> @@ -714,7 +708,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;
> @@ -740,41 +734,33 @@ static int
> load_drm_backend(struct weston_compositor *c, const char *backend,
> int *argc, char **argv, struct weston_config *wc)
> {
> - struct drm_config *config;
> + struct weston_drm_backend_config config = {{ 0, }};
> struct weston_config_section *section;
> int ret = 0;
>
> - config = zalloc(sizeof (struct drm_config));
> - if (!config)
> - return -1;
> -
> const struct weston_option options[] = {
> - { WESTON_OPTION_INTEGER, "connector", 0, &config->base.connector },
> - { WESTON_OPTION_STRING, "seat", 0, &config->base.seat_id },
> - { WESTON_OPTION_INTEGER, "tty", 0, &config->base.tty },
> - { WESTON_OPTION_BOOLEAN, "current-mode", 0,
> - &config->use_current_mode },
> - { WESTON_OPTION_BOOLEAN, "use-pixman", 0, &config->base.use_pixman },
> + { WESTON_OPTION_INTEGER, "connector", 0, &config.connector },
> + { WESTON_OPTION_STRING, "seat", 0, &config.seat_id },
> + { WESTON_OPTION_INTEGER, "tty", 0, &config.tty },
> + { WESTON_OPTION_BOOLEAN, "current-mode", 0, &config.use_current_mode },
> + { WESTON_OPTION_BOOLEAN, "use-pixman", 0, &config.use_pixman },
> };
>
> parse_options(options, ARRAY_LENGTH(options), argc, argv);
>
> section = weston_config_get_section(wc, "core", NULL, NULL);
> weston_config_section_get_string(section,
> - "gbm-format", &config->base.gbm_format,
> + "gbm-format", &config.gbm_format,
> NULL);
>
> - config->base.base.struct_version = WESTON_DRM_BACKEND_CONFIG_VERSION;
> - config->base.base.struct_size = sizeof(struct weston_drm_backend_config);
> - config->base.configure_output = drm_configure_output;
> + config.base.struct_version = WESTON_DRM_BACKEND_CONFIG_VERSION;
> + config.base.struct_size = sizeof(struct weston_drm_backend_config);
> + config.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, &config.base);
> +
> + free(config.gbm_format);
> + free(config.seat_id);
>
> return ret;
> }
>
More information about the wayland-devel
mailing list