[PATCH weston v7 3/3] drm: Don't hang onto the backend config object post-backend_init
Bryce Harrington
bryce at osg.samsung.com
Wed May 4 22:24:43 UTC 2016
On Sat, Apr 30, 2016 at 11:41:15AM +0200, Benoit Gschwind wrote:
> Hello Bryce,
>
> I think my comments on previous patch apply here, while you did some
> useful update here.
This patch essentially takes care of much of what you mentioned in the
previous review. Your other suggestions sound like refactoring work
that could be handled as followups.
Take a look again and see if there's anything you absolutely can't live
with. We're on v7 of this patch and I'd like to just get it landed and
move on with other tasks.
Bryce
> 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;
> > }
> >
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
More information about the wayland-devel
mailing list