[Mesa-dev] [PATCH mesa 03/10] egl+glx: turn LIBGL_ALWAYS_SOFTWARE into a boolean

[Eric Engestrom]

On 11 September 2017 18:33:27 BST, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> On 8 September 2017 at 18:36, Eric Engestrom
> <eric.engestrom at imgtec.com> wrote:
> > On Friday, 2017-09-08 08:13:54 -0700, Kenneth Graunke wrote:
> >> On Friday, September 8, 2017 6:33:44 AM PDT Emil Velikov wrote:
> >> > Hi Eric,
> >> >
> >> > On 8 September 2017 at 13:40, Eric Engestrom
> <eric.engestrom at imgtec.com> wrote:
> >> > > Instead of setting based on set/unset, allow users to use
> boolean values.
> >> > > In the docs, use `ALWAYS=true` instead of `ALWAYS=1` as it's
> clearer IMO.
> >> > >
> >> > I'm a bit weary about this series. In the past we had cases where
> >> > people will set the variable to empty string and/or 0 and expect
> >> > things to work.
> >> >
> >> > IIRC the older Intel QA team was using it, not sure about now.
> Either
> >> > way it's great to confirm with a few people before pushing this.
> >>
> >> That team is gone.  I like the consistency of using
> env_var_as_boolean
> >> across the codebase - it might break a few things, but I think
> that's
> >> okay.  Most people don't use this, and most of those that do use
> =1.
> >
> > Agreed.
> >
> >>
> >> > That aside, env_var_as_boolean() treats 'yes', '1' and 'true' in
> >> > identical manner. Perhaps we can state what a "boolean variable"
> is
> >> > and use that reference through the documentation?
> >> >
> >> > -Emil
> >>
> >> That seems like a good idea.
> >
> > Yes indeed, and I'll do that, but as a follow up patch.
> Sure thing.
> 
> > I'm thinking of adding something like this at the top of
> envvar.html:
> >
> >         There are different kind of environment variables:
> >           - BOOL: expects 1/true/yes or 0/false/no.
> >           - INT: expects an integer value.
> >           - ENUM: expects an string, but only a limited set of
> values is accepted.
> >           - PATH: expects a file or folder path.
> >           - NAME: expects a string name (like a driver name), but
> not a full path.
> >           - NON-NULL: any value enables it (including empty string);
> use `unset` to disable.
> >         Note: some variables expect more complicated values, like
> >         MESA_EXTENSION_OVERRIDE.
> >
> > and then each env var in the doc would get an annotation:
> >         LIBGL_ALWAYS_SOFTWARE (BOOL)
> >         LIBGL_SHOW_FPS (INT)
> This should really be a bool, but that's for another time.

If memory serves, this is the number of frames to average the FPS over, but
we could fix it and turn this env into a bool.

> 
> >         EGL_LOG_LEVEL (ENUM: debug, info, warning, fatal)
> >         EGL_DRIVERS_PATH (PATH)
> >         EGL_DRIVER (PATH or NAME)
> >         EGL_PLATFORM (NAME)
> > ... etc.
> >
> > Hopefully, this should be clear enough. The last option (NON-NULL)
> would
> > be applied to the old-style toggles.
> >
> Seems pretty clear over here. Although damn man... that's really nice
> and comprehensive.
> 
> > For now, there seems to be another issue; I sent this series to
> travis,
> > and EGL is failing to link [1] because every exported symbol in
> mesautil
> > appears twice; some sort of double-link issue?
> > I don't know if I'm tired or thick, but I can't figure it out :]
> >
> > [1] https://travis-ci.org/1ace/mesa/jobs/273302410#L6797
> >
> > I'd appreciate any help :)
> >
> I've spotted it - libEGL_common conditionally uses libmesautil.la (for
> the wayland code).
> I'd just make that unconditionally and omit it from
> libEGL/libEGL_mesa.
> 
> With the above fix, the patch is
> Reviewed-by: Emil Velikov <emil.velikov at collabora.com>
> 
> -Emil