[PATCH 2/5] drm: port the drm backend to the new init api

[Bryce Harrington]

On Fri, Apr 15, 2016 at 12:01:01PM +0300, Pekka Paalanen wrote:
> On Thu, 14 Apr 2016 19:09:34 +0300
> Giulio Camuffo <giuliocamuffo at gmail.com> wrote:
> 
> > 2016-04-13 14:30 GMT+03:00 Pekka Paalanen <ppaalanen at gmail.com>:
> > > On Tue, 12 Apr 2016 21:34:28 -0700
> > > Bryce Harrington <bryce at osg.samsung.com> wrote:
> > >  
> > >> On Wed, Apr 06, 2016 at 11:37:57AM +0300, Pekka Paalanen wrote:  
> > >> > On Wed,  9 Mar 2016 16:49:29 -0800
> > >> > Bryce Harrington <bryce at osg.samsung.com> wrote:
> > >> >  
> > >> > > 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>
> > >> > > ---
> > >> > > v4: Update to current trunk
> > >> > >     - Add missing param doc for mode in drm_output_choose_initial_mode
> > >> > >     - Rebase to account for code changes by 91880f1e to make vt
> > >> > >       switching configurable.
> > >> > >
> > >> > >  Makefile.am          |   3 +
> > >> > >  src/compositor-drm.c | 196 ++++++++++++++++++---------------------------------
> > >> > >  src/compositor.h     |   2 -
> > >> > >  src/main.c           |  94 +++++++++++++++++++++++-
> > >> > >  4 files changed, 165 insertions(+), 130 deletions(-)  
> > >> >
> > >> > Hi Giulio and Bryce,
> > >> >
> > >> > I'm sorry it has taken so long for me to come back to this.  
> > >  
> > >> > > +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_config_section *section;
> > >> > > + int ret = 0;
> > >> > > +
> > >> > > + config = zalloc(sizeof *config);
> > >> > > + if (!config)
> > >> > > +         return -1;  
> > >> >
> > >> > This function will be affected by the struct versioning too.  
> > >>
> > >> The subsequent patch adds the versioning, although later in the routine
> > >> than here.
> > >>  
> > >> > > + 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 },
> > >> > > + };  
> > >> >
> > >> > Mixed declarations and code.  
> > >>
> > >> Hmm, I'm not sure best how to address this.  Options is being
> > >> initialized with pointers that are created by the zalloc, so I don't
> > >> think I can just separate the declaration from the initialization.  Does
> > >> options need to be changed to be dynamically allocated as well?  
> > >
> > > Hi,
> > >
> > > I suppose the simplest solution is to make another function, define
> > > 'options[]' in it, and get 'config' as an argument. Then it can call
> > > the parser and return. Something like ...fill_from_command_line(struct
> > > drm_config *config, argc, argv...).
> > >
> > > It's fine to keep your changes as separate patches as long as every
> > > patch is bisectable.
> > >
> > > I'm going through the new series right now, and noticed these emails
> > > from you just now.  
> > 
> > I think this is one of the cases where strict style compliance goes in
> > the way of better code. Adding a function just adds an indirect level
> > but gives nothing in return, because it would really be just a wapper
> > around the options[] variable. I don't see it much different to a "int
> > value_1() { return 1; }".
> > But more importantly imho, it requires another revision.
> 
> No, it would be much more. The function would essentially be "fill in
> this config struct from these argc,argv". That color would actually
> please my eye more, as that is a logical single step to execute from a
> load_*() function, rather than stuffing it all inline in load_*().
> 
> But, I'm not the one building the shed here. If I really care, I can
> fix it after landing this stuff, since it doesn't seem to break the
> build.

One wrinkle I'm spotting as I try to implement this is that the headless
and x11 backends have options that are parsed but not stored in the
config structure.  Maybe I can shove them in anyway though.

> Thanks,
> pq

> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v2
> 
> iQIVAwUBVxCtzSNf5bQRqqqnAQiYHhAAmj+UCgMJGEj0vUglcKIx0FAsYQylqNm5
> zn0cYwGsi/mkbhCy5A6TNwTM5y21DGW7+/wcSwnlBZLH/7PDjX57ppwy+s7jcW5m
> 9dLi/qpOlepfQ8S0BF1epFlQAyxBqa1JLvfI3O3ta91ku5VBCCEnKLNyqwCGE8z3
> COUudldrCP6Ysb9XY2TXLhlwDZxDu1uVeEXS2G1UxEj/ealB5RXypzuPKTSYVl77
> WsDhRgP1GMo4g1XL7B5cvBkvrrzDkj31u+M+pffIcvBJjnpL7ja7QSkerylXyaP7
> 1z4XIqM0vsyzxEnh7ZhNLIHcP1kOhunjJJi9/ti36DRwcL/flMIQoYElQU1MvBmw
> Jv9tm2XlMjTIPMKgtdbSaz6fu6sJq0oEUQXYhpTMhaNx0H8Cg8uBpwQpaWsKP1A1
> QLM86aCpQsiNDPsvgTStX5M2hoJ0+MJbg/hpHy1ylhigaC3B8sx6nXX9JcGAEUBJ
> DeNOruOjAsybOUPyBaKIufmiUt6pEJYRaCM0ghXMCYWqG1dqf0RlcGm1lK5z/F3F
> 6SM+jciYqwN+/5yBWoUw0oeJiUbkoNnCgQwqfOKtitn2lPx3OY5seV9lBaLn4OnH
> sF4HPAvIh7/2x/k19GN2q0sbueISUcoUmvPHqQg2Tj4JalU0ByWrRBlMtHoaIPvL
> qM3v+5S/lP4=
> =z6pB
> -----END PGP SIGNATURE-----