[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.

I'm going to see how it looks broken out as a separate function as pq
suggests.  All this code only gets executed once and there's no loops or
anything, so the extra level of indirection isn't really a performance
issue.  But since it deals with command options it's likely to get
tinkered with by a large variety of people in the future, so clarity and
maintainability seem like the main concerns to worry about.  From that
viewpoint I'm not sure which of "keeping everything together in one
place" or "having things nicely organized into short functions" becomes
the stronger argument.

The other goal is keeping things stylistically consistent with the
other backends; if we break the options out into their own function
here, then it should be done that same way in all the backends.

Bryce