[PATCH weston v7 2/3] drm: port the drm backend to the new init api

[Pekka Paalanen]

On Sat, 30 Apr 2016 11:35:22 +0200
Benoit Gschwind <gschwind at gnu-log.net> wrote:

> Hello Bryce,
> 
> I have some comments after a trivial rebase.
> 
> Le 30/04/2016 00:40, Bryce Harrington a écrit :
> > From: Giulio Camuffo <giuliocamuffo at gmail.com>
> > 
> > Signed-off-by: Bryce Harrington <bryce at osg.samsung.com>
> > Reviewed-by: Quentin Glidic <sardemff7+git at sardemff7.net>
> > Acked-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> > Tested-by: Benoit Gschwind <gschwind at gnu-log.net>
> > 
> > Signed-off-by: Bryce Harrington <bryce at osg.samsung.com>
> > ---
> > 
> > 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160510/792d8aef/attachment.sig>