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 -f no-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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
