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

[Giulio Camuffo]

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.

Cheers,
Giulio

>
>
> Thanks,
> pq