Hello Bryce, Ok I can live with this patches ;)
Reviewed-by: Benoit Gschwind <[email protected]> On 05/05/2016 00:24, Bryce Harrington wrote: > 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 <[email protected]> >>> Reviewed-by: Pekka Paalanen <[email protected]> >>> >>> --- >>> 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 >> [email protected] >> https://lists.freedesktop.org/mailman/listinfo/wayland-devel > _______________________________________________ > wayland-devel mailing list > [email protected] > https://lists.freedesktop.org/mailman/listinfo/wayland-devel > _______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
