[PATCH weston v7 3/3] drm: Don't hang onto the backend config object post-backend_init
Benoit Gschwind
gschwind at gnu-log.net
Fri May 6 06:51:01 UTC 2016
Hello Bryce,
Ok I can live with this patches ;)
Reviewed-by: Benoit Gschwind <gschwind at gnu-log.net>
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 <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
> _______________________________________________
> 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