[Mesa-dev] [PATCH 0/8] use hwdb in loader v4

[Kristian Høgsberg]

On Fri, Feb 07, 2014 at 12:46:46AM +0000, Emil Velikov wrote:
> Helo Kristian,
> 
> Rather than nitpicking like an old bat, I took the liberty of respinning
> your patchset. Most patches are left as is, minus a reshuffle
> in their order + rebase.
> 
> Available in the hwdb-loader-v4 branch at https://github.com/evelikov/Mesa/
> 
> Highlights comparing to v3:

Hi Emil,

Thanks for reviewing and updating the patches.  On the whole it looks good,
you've caught a number of bugs in the series.

>  - Dropped "gallium-loader: Don't worry about PCI IDs in gallium-loader"
> The code that it was removing is used by the clover st.
>  - Dropped "loader: Invert __NOT_HAVE_DRM_H to __HAVE_DRM_H". There are
> a lot more cases in mesa (and xorg-server) that needs to be touched.

Let's look at make V=1:

libtool: compile:  gcc -DPACKAGE_NAME=\"Mesa\" -DPACKAGE_TARNAME=\"mesa\" -DPACKAGE_VERSION=\"10.2.0-devel\" "-DPACKAGE_STRING=\"Mesa 10.2.0-devel\"" "-DPACKAGE_BUGREPORT=\"https://bugs.freedesktop.org/enter_bug.cgi?product=Mesa\"" -DPACKAGE_URL=\"\" -DPACKAGE=\"mesa\" -DVERSION=\"10.2.0-devel\" -DSTDC_HEADERS=1 -DHAVE_SYS_TYPES_H=1 -DHAVE_SYS_STAT_H=1 -DHAVE_STDLIB_H=1 -DHAVE_STRING_H=1 -DHAVE_MEMORY_H=1 -DHAVE_STRINGS_H=1 -DHAVE_INTTYPES_H=1 -DHAVE_STDINT_H=1 -DHAVE_UNISTD_H=1 -DHAVE_DLFCN_H=1 -DLT_OBJDIR=\".libs/\" -DHAVE___BUILTIN_BSWAP32=1 -DHAVE___BUILTIN_BSWAP64=1 -DHAVE_CLOCK_GETTIME=1 -DHAVE_PTHREAD=1 -I. -D_GNU_SOURCE -DHAVE_PTHREAD -DDEBUG -DUSE_X86_64_ASM -DHAVE_DLOPEN -DHAVE_POSIX_MEMALIGN -DHAVE_LIBDRM -DHAVE_LIBUDEV -DGLX_INDIRECT_RENDERING -DGLX_DIRECT_RENDERING -DUSE_EXTERNAL_DXTN_LIB=1 -DHAVE_ALIAS -DHAVE_MINCORE -I../../include -fvisibility=hidden -I/usr/include/libdrm -g -O2 -Wall -std=c99 -Werror=implicit-function-declaration -Werror=missing-prototypes -fno-strict-aliasing -fno-builtin-memcmp -g -O0 -MT libloader_la-loader.lo -MD -MP -MF .deps/libloader_la-loader.Tpo -c loader.c  -fPIC -DPIC -o .libs/libloader_la-loader.o

I don't see a single negated condition there.  It's all posivites, eg
HAVE_LIBUDEV, USE_X86_64_ASM.  All uses of __NOT_HAVE_DRM_H are use with
and negation so we get a confusion double negation, eg:

  #ifndef __NOT_HAVE_DRM_H

which is confusing and error prone.  Looking through configure.ac I also don't
see a single negative -D (except for NDEBUG, I guess), so I'm not sure what
all these cases are.  And even if we do have cases in mesa that use negated
defines, I don't think that's a good reason for introducing new negated
symbols (__NOT_HAVE_DRM_H was introduced in your loader patches recently).
We also don't have to convert all other negative defines in order to fix this
one.  We're not making things more inconsistent.

>  - No more dangling files after make clean.
>  - Vendor/device id should be in uppercase hex.

Good catch.

>  - The hwdb code is moved to a separate function. It does not belong
> inside loader_get_pci_id_for_fd()

And it didn't end up there at the end of my series, the function was renamed
at the end.  I agree that your refactoring steps look more logical, but the
reason I did what I did was that I wanted to avoid creating and destroying
a struct udev and looking up the udev device and parent multiple times.
Can we keep your structure, but share the udev init logic?

Kristian

>  - Modalias builds now  - s/driver/modalias and special case for
> chip_id = 0 (nouveau) under android.
>  - Finally a couple of small cleaups in udev error paths
> 
> Note: The scons build is _broken_ with the modalias patch, but at least
> the automake and android should be working like a charm.
> 
> Feel free to test and review.
> 
> Cheers,
> Emil