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

[Emil Velikov]

On 28/02/14 21:01, Kristian Høgsberg wrote:
> 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-stri
ct-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.
> 
I suggest grepping the whole of mesa for "__NOT_HAVE_DRM_H" :)

This behemoth was introduced by George Sapountzis with commit
0b932284f2294, back in 2010. Unfortunately It was until recently that
people noticed it's existence. IIRC Eric used it in the classic
megadrivers work, then I've reused while updating on Rob's initial
loader work.

I have absolutely _no_ objection of nuking it, as long as one audits the
code that things continue to build/work on drm.h-less systems.

(not sure why I trew xserver in there I've assumed that it used the
silly define, which evidently it does not)

>>  - 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?
> 
Definitely I'm always for code sharing. I've reordered and split things
to minimise the confusion of what each function does. Presently it
claims one thing and does that plus a couple more.

FWIW some of the Android/Scons people may want to take a look, as I'm
pretty sure that things are somewhat broken outside of automake.

Adrian, Jakob

Gents can you check (or forward to someone in your team) the android and
scons build, respectively. I'm pretty sure that there are a few things
that need ironing out, esp with patches 3 and 6.

Series is available in the hwdb-loader-v4 branch at
https://github.com/evelikov/Mesa/

Cheers
-Emil
> 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