[PATCH weston v5 09/11] drm: Don't hang onto the backend config object post-backend_init
Bryce Harrington
bryce at osg.samsung.com
Wed Apr 13 18:23:29 UTC 2016
On Wed, Apr 13, 2016 at 04:54:04PM +0300, Pekka Paalanen wrote:
> 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.
Right. It was ambiguous who held the responsibility for freeing the
strings, and in the previous patches each was making its own assumptions
that weren't the same. This way is clearer - treat the config object as
a const that goes out of scope at the end of the function call, and copy
what you need but don't hang onto anything.
You're right that in a lot of cases the strings are just being converted
to enums or other non-string data, so having to copy the strings you do
want to keep is not that a big deal.
Bryce
> Yeah, this is good, I think.
>
>
> Thanks,
> pq
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v2
>
> iQIVAwUBVw5PfCNf5bQRqqqnAQilMg/+JY5DDABhbCNqhktubXC00NsauST4Oq1x
> oWs1IFX+1odbcg6AyzRCl2oG904M0n4BcddYrv6GUlnOJql8uDUsBr41CM8ZuOlT
> 2k+7PFRAyW45at0Yvk1MbSdl0XESDugpDGcA5/lqQTqejAZu5tufdDgPPbmEeLCF
> Y+ixLnO/KW8rzcESGc1jV83Z0lhD1tCVPvzTrtclVJZOHbI5e56sj6puM0/7SAX/
> qIXd1uRmsPmyRw3w9cAVwQdfUiQ/WPvRQfNLbZHbS5VvczI+AWAWvk3S6PWGLpUy
> BmGsd8myrfvQk/p31T62nPL9S646KDTZgZU4r9z74mhL1Ghy9FUgMmyvY5o4mth0
> ekN2ejd3lila9w/FhLxcNq1TCTJdj7Pn15vRMyrDtpy1S4gqIGrImgFkdlxDkoIG
> 71olC6Bg7lA+xUV1uC5MFmhTQx/43immwT8jvr2qtnfeshsa6m4YEX9C8XoeGLHh
> UT31FNpbTok8mwQONdCO/JpHBwoMibWnx8nAs9g8Cl7ya8CWXGIJQ0ZFqqvSCTdY
> l3S9Xb/Z4Tmtm3rJI5L09GeRUObuTbYTJ1j6Nrk2uFzvvWsiuERdtzJ4I0s0mjSw
> Z0zn+1mvNUH6bXKtWKaidcTeY+aUYuCyaHv6TwQ7Ncr/6ZbzLwlvJskaW4jS/9dA
> sL/HXC9Pn3M=
> =kj80
> -----END PGP SIGNATURE-----
More information about the wayland-devel
mailing list