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

Fri Feb 28 16:27:13 PST 2014

On Fri, Feb 28, 2014 at 3:46 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> 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.

I hate to keep picking on this, but those are not the same defines.
They all key off of different conditions and guard different code,
they just happen to all have the same name.  If anything, it's part of
the dri_interface.h API, in that you can define it to prevent
dri_interface.h from including drm.h.  Nothing in loader.h includes
dri_interface.h so we can call it what we want.  And we already have
-DHAVE_DRM in the flags from configure, so we can just use that.  That
should also do the right thing for SConscript if we just delete the
CPPDEFINES append and for Android.mk we could just define HAVE_DRM in
the other if clause.

Kristian

> (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
>