On Sat, 30 Apr 2016 11:35:22 +0200 Benoit Gschwind <[email protected]> wrote:
> Hello Bryce, > > I have some comments after a trivial rebase. > > Le 30/04/2016 00:40, Bryce Harrington a écrit : > > From: Giulio Camuffo <[email protected]> > > > > Signed-off-by: Bryce Harrington <[email protected]> > > Reviewed-by: Quentin Glidic <[email protected]> > > Acked-by: Pekka Paalanen <[email protected]> > > Tested-by: Benoit Gschwind <[email protected]> > > > > Signed-off-by: Bryce Harrington <[email protected]> > > --- > > > > v5: > > - Update to reflect format rename to gdb_format > > - Initialize width/height (suggested by pq) > > - squash drm structure versioning (suggested by pq) > > v6: > > - Fix gcc warning about missing braces. Use double-brackets for config > > initializer for create_output_for_connector to avoid gcc warning, > > since first element is another struct. (See GCC bug 53119.) > > - Code and comments reformatting for consistency with other backend > > configs > > - Don't use underscore prefix in header guard > > - Add stub config_init_to_defaults() > > - Define the version number in the header > > - Allocate config on stack > > v7: > > - Update to master > > > > Makefile.am | 3 + > > src/compositor-drm.c | 212 > > +++++++++++++++++++++------------------------------ > > src/compositor-drm.h | 126 ++++++++++++++++++++++++++++++ > > src/compositor.h | 2 - > > src/main.c | 98 +++++++++++++++++++++++- > > 5 files changed, 311 insertions(+), 130 deletions(-) > > create mode 100644 src/compositor-drm.h > > > > - ec = weston_compositor_create(display, NULL); > > + ec = weston_compositor_create(display, config); > > This modification grabbed my attention. I think this is a bad choice. If > a backend need the configuration file structure, he should not use this > place to store it. The proper place, imo, is within the backend > structure that store others data about the backend. That also mean the > structure must be passed to the backend via the configuration structure. > And finally, as other backend often does not need to keep the > configuration file, the configuration should be copied in the case of > drm and freed as soon as it is not useful anymore. Hi, that particular thing has cought my eye every time this patch has been posted, and every time I have to remind myself, that the argument to weston_compositor_create() is void *user_data. The user_data will not be used by any backends - and they could not use a weston_config anyway. In libweston, weston_config does not exist. Also the backends won't touch user_data, they don't know what it is. It *is* used in drm_configure_output() where it is retrieved with weston_compositor_get_user_data(). This code is in main.c so it's fine. The only surprise here is that the weston_compositor user_data is just the weston_config, but once you get over that, it is ok. > > In more general manner, the following callback has a useless signature: > > drm_configure_output(struct weston_compositor *c, > struct weston_drm_backend_config *backend_config, > const char *name, > struct weston_drm_backend_output_config *config) > > It should take at less a void * as user data, the one you provided > within the configuration of the backend. Passing the full configuration > structure does look useless. weston_compositor has user_data and is already passed in, so neither backend_config or a user_data is absolutely necessary in the callback signature. Passing backend_config may actually be harmful, but the following patch in this series changes it again. However, it doesn't matter much at this point. I'm sure we will be revisiting this later. Right now the important thing is to remove all weston_config usage from the backends. Thanks, pq
pgpCzxZx8Julu.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
