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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Jan 27, 2016 at 02:08:21PM +0000, Emil Velikov wrote:
> On 27 January 2016 at 13:50, Daniel Vetter <daniel@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@amd.com>
> >> Cc: Michel Dänzer <michel.daenzer@amd.com>
> >> Cc: Ben Skeggs <bskeggs@redhat.com>
> >> Cc: Dave Airlie <airlied@redhat.com>
> >> Cc: Daniel Stone <daniels@collabora.com>
> >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >> Cc: Inki Dae <inki.dae@samsung.com>
> >> Cc: Rob Clark <robclark@freedesktop.org>
> >> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> >> Cc: Daniel Kurtz <djkurtz@chromium.org>
> >> Signed-off-by: Emil Velikov <emil.l.velikov@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.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel