[PATCH libdrm] Update/fix the {copy,commit}-headers targets

[Emil Velikov]

On 27 January 2016 at 14:47, Ville Syrjälä
<ville.syrjala at linux.intel.com> wrote:
> On Wed, Jan 27, 2016 at 02:08:21PM +0000, Emil Velikov wrote:
>> On 27 January 2016 at 13:50, Daniel Vetter <daniel at ffwll.ch> wrote:
>> > On Wed, Jan 27, 2016 at 01:23:11PM +0000, Emil Velikov wrote:
>> >> As some headers do not reside in include/drm we need to tweak our rules,
>> >> and exclude headers that shouldn't be distributed [XXX: clarify why ?].
>> >>
>> >> To avoid the extra magic of diving into the kernel tree running `make
>> >> headers_install', just sed out the only reason why we need it - __user.
>> >>
>> >> Cc: Alex Deucher <alexander.deucher at amd.com>
>> >> Cc: Michel Dänzer <michel.daenzer at amd.com>
>> >> Cc: Ben Skeggs <bskeggs at redhat.com>
>> >> Cc: Dave Airlie <airlied at redhat.com>
>> >> Cc: Daniel Stone <daniels at collabora.com>
>> >> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
>> >> Cc: Inki Dae <inki.dae at samsung.com>
>> >> Cc: Rob Clark <robclark at freedesktop.org>
>> >> Cc: Tomi Valkeinen <tomi.valkeinen at ti.com>
>> >> Cc: Daniel Kurtz <djkurtz at chromium.org>
>> >> Signed-off-by: Emil Velikov <emil.l.velikov at gmail.com>
>> >> ---
>> >>
>> >> Gents,
>> >> As one runs `make copy-headers' we get a massive diff stat (+1500/-800)
>> >> and a handful of issues gets pointed out.
>> >> Please let me know of your prefered solution (regardless if one will get
>> >> to it soon) and if we should consider it a blocker (B) or not (N).
>> >>
>> >> Thanks
>> >> Emil
>> >>
>> >>  - (N) Header license miss-match - omap, msm, exynos. Update the kernel
>> >> ones ?
>> >>
>> >>  - (N) Broken compat ioctls - exynos (and the UMS drivers) - using
>> >> unsigned int as opposed to _u32/64. Considering they're 32bit only, we
>> >> can get away with 'breaking' the ABI and using the proper ones ?
>> >>
>> >>  - (N, keep local for now) C++ compat - libdrm has a hack/workaround
>> >> (virtual is a keyword in C++), which I'd like us to upstream plus some
>> >> extern C wrappers.
>> >>
>> >>  - (?) Missing UMS symbols - see _DRM_GEM
>> >>
>> >>  - (?) Non C89 compliant (see DRM_DRAWABLE_CLIPRECTS) - do we still
>> >> want/need that ?
>> >>
>> >>  - (B) Using include <drm/...> as opposed to include "..." - drm.h,
>> >> nouveau_drm.h. Should be fixed in kernel.
>> >>
>> >>  - (N) ABI 'break'
>> >>   + drm - struct drm_mode_get_connector extra pad
>> >>   + tegra - struct drm_tegra_gem_mmap extra pad
>> >>
>> >>  - (B) API break
>> >>   + drm - missing DRM_MODE_OBJECT_*
>> >>   + nouveau - missing (gs)etparam - both structs and macros. everything
>> >> else is fine/unused.
>> >>   + radeon - RADEON_TILING_R600_NO_SCANOUT, CIK_TILE_MODE_COLOR_2D* and
>> >> CIK_TILE_MODE_DEPTH_STENCIL_2D_TILESPLIT_* - quick grep shows no users
>> >>   + omap - struct drm_omap_get_base, DRM_OMAP_GET_BASE + IOCTL
>> >>
>> >>  - (N) (unneeded?) API additions - nouveau's DRM_IOCTL_NOUVEAU_GEM_*
>> >>
>> >>  - (N) __KERNEL__ condiditionals. Is it really an issue - sure if looks
>> >> a bit spurious but that's about it.
>> >>
>> >>
>> >>  Makefile.am | 9 ++++++++-
>> >>  1 file changed, 8 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/Makefile.am b/Makefile.am
>> >> index ca41508..6c71d3a 100644
>> >> --- a/Makefile.am
>> >> +++ b/Makefile.am
>> >> @@ -126,7 +126,14 @@ endif
>> >>
>> >>  copy-headers :
>> >>       cp -r $(kernel_source)/include/uapi/drm/*.h $(top_srcdir)/include/drm/
>> >> +     sed -i "s/__user //g" $(top_srcdir)/include/drm/*.h
>> >> +     mv $(top_srcdir)/include/drm/exynos_drm.h $(top_srcdir)/exynos/
>> >> +     mv $(top_srcdir)/include/drm/msm_drm.h $(top_srcdir)/freedreno/msm/
>> >> +     mv $(top_srcdir)/include/drm/omap_drm.h $(top_srcdir)/omap/
>> >> +     rm $(top_srcdir)/include/drm/armada_drm.h
>> >> +     rm $(top_srcdir)/include/drm/etnaviv_drm.h
>> >> +     rm $(top_srcdir)/include/drm/i810_drm.h
>> >> +     rm $(top_srcdir)/include/drm/vc4_drm.h
>> >
>> > You can't copy headers directly. First you need to generate them in the
>> > kernel sources:
>> >
>> > $ make headers_install
>> >
>> > Then you can copy them from usr/include/*
>> >
>> > That should at least get rid of the __KERNEL__ stuff. Then any serious
>> > issues should be fixed in the upstream kernel first.
>> I agree that copying the headers isn't that good of an idea. Then
>> again as that causes zero problems - it brings in __KERNEL__ and
>> __user, latter of which sed out. So wouldn't it be better to focus on
>> the serious issues first ? Of course, that doesn't mean we have to
>> forget about this one :-P
>>
>> -Emil
>> P.S. Diving into other projects and executing make foo/bar is frowned
>> upon, normally.
>
> I doubt anyone was suggesting that you would do that.
>
> I would probably just nuke the copy-headers rule. But if people want
> to keep it, I'd make it look at the INSTALL_HDR_PATH variable (which
> is how the kernel decides where to install the headers), bail if it's
> not set (or maybe default to /usr, like the kernel), and copy the
> headers from there. Then the user can go and do the
> 'make INSTALL_HDR_PATH=... headers_install' himself in the kernel
> tree, and then do the 'make INSTALL_HDR_PATH=... copy-headers' in
> libdrm. Or you could use some other variable name for that in libdrm,
> but reusing the kernel one would seem like the easiest solution.
>
The idea is to avoid jumping back and forth, if that can be avoided.
And based on how long and how badly this has been broken I'd go with
"the quickest, simplest, idiot proof solution".

Then again... I would love to hear on the more important, imho, topics. Please ?

Thanks
Emil