[Mesa-dev] [PATCH] HUD: Add support for block I/O, network I/O and lmsensor stats

[Steven Toth]

Hello Emil,

>> +AC_ARG_ENABLE([gallium-extra-hud],
>> +    [AS_HELP_STRING([--enable-gallium-extra-hud],
>> +        [enable HUD block/NIC I/O HUD stats support @<:@default=disabled@:>@])],
>> +    [enable_gallium_extra_hud="$enableval"],
>> +    [enable_gallium_extra_hud=no])
> Looking at some of your other patches makes me wonder. Is it worth
> having this as a list - (maybe?) rename it to s/enable/with/ and have
> the user provide a list of required parts - sensors, cpu freq, etc.
>
> I'm thinking of cases where extra deps come to play and having a
> toggle for each one will be insane :-\

Thank you for raising this.

Today we have something that's OK for now. I'm happy to adapt/adopt whatever
you think makes better sense.

> If having X separate bits in the extra-hud sounds like and overkill,
> how do you feel if we do
> --with/enable-gallium-extra-hud={all,without-sensors}

I'm OK with both extra bits AND all. However, while I prefer enable
extras by name, (block-io, network-io, sensors), in practice, because
these are largely non-default performance profiling tools, any
developer concerned with performance will simply compile with = all
(or the default - none). So, extra bits starts to feel redundant.

all = everything, and an opt-out for sensors? I'm good with this.

>
>> +AM_CONDITIONAL(HAVE_GALLIUM_EXTRA_HUD, test "x$enable_gallium_extra_hud" = xyes)
>> +if test "x$enable_gallium_extra_hud" = xyes ; then
>> +    DEFINES="${DEFINES} -DHAVE_GALLIUM_EXTRA_HUD=1"
>> +fi
>> +
>> +#TODO: no pkgconfig .pc available for libsensors.
>> +#PKG_CHECK_MODULES([LIBSENSORS], [libsensors >= $LIBSENSORS_REQUIRED], [enable_lmsensors=yes], [enable_lmsensors=no])
>> +AC_ARG_ENABLE([lmsensors],
>> +    [AS_HELP_STRING([--enable-lmsensors],
>> +        [enable HUD lmsensor support @<:@default=disabled@:>@])],
>> +    [enable_lmsensors="$enableval"],
>> +    [enable_lmsensors=no])
>> +AM_CONDITIONAL(HAVE_LIBSENSORS, test "x$enable_lmsensors" = xyes)
>> +if test "x$enable_lmsensors" = xyes ; then
>> +    DEFINES="${DEFINES} -DHAVE_LIBSENSORS=1"
>> +    LIBSENSORS_LDFLAGS="-lsensors"
>> +else
>> +    LIBSENSORS_LDFLAGS=""
>> +fi
>> +AC_SUBST(LIBSENSORS_LDFLAGS)
> You want the LIBS variable, LDFLAGS is something else.

Ahh. hmm, OK. I'll look into this.

>
>
>>  endif
>>
>> +libgallium_la_LDFLAGS = $(LIBSENSORS_LDFLAGS)
>> +
> Please add the new variable to GALLIUM_COMMON_LIB_DEPS (in
> src/gallium/Automake.inc)

OK.

>
>
>> +
>> +#define LOCAL_DEBUG 0
>> +
> This and the related debugging code does not seem too useful past the
> development stage. Can we nuke it, please ?

Agreed.

-- 
Steven Toth - Kernel Labs
http://www.kernellabs.com