[PATCH libdrm] intel: annotate the intel genx helpers as private

Rodrigo Vivi rodrigo.vivi at intel.com
Thu Sep 13 17:43:04 UTC 2018 

On Thu, Sep 13, 2018 at 09:45:47AM +0100, Eric Engestrom wrote:
> On Thursday, 2018-09-13 15:19:00 +0800, Chih-Wei Huang wrote:
> > Note this patch breaks drm_gralloc:
> > 
> > FAILED: out/target/product/x86_64/obj_x86/SHARED_LIBRARIES/libgralloc_drm_intermediates/LINKED/libgralloc_drm.so
> > /bin/bash -c "prebuilts/clang/host/linux-x86/clang-4053586/bin/clang++
> > -nostdlib -Wl,-soname,libgralloc_drm.so -Wl,--gc-sections -shared
> > out/target/product/x86_64/obj_x86/lib/crtbegin_so.o
> > out/target/product/x86_64/obj_x86/SHARED_LIBRARIES/libgralloc_drm_intermediates/gralloc_drm.o
> > out/target/product/x86_64/obj_x86/SHARED_LIBRARIES/libgralloc_drm_intermediates/gralloc_drm_kms.o
> > out/target/product/x86_64/obj_x86/SHARED_LIBRARIES/libgralloc_drm_intermediates/gralloc_drm_intel.o
> > out/target/product/x86_64/obj_x86/SHARED_LIBRARIES/libgralloc_drm_intermediates/gralloc_drm_radeon.o
> > out/target/product/x86_64/obj_x86/SHARED_LIBRARIES/libgralloc_drm_intermediates/gralloc_drm_nouveau.o
> > out/target/product/x86_64/obj_x86/SHARED_LIBRARIES/libgralloc_drm_intermediates/gralloc_drm_pipe.o
> > -Wl,--whole-archive  -Wl,--no-whole-archive
> > out/target/product/x86_64/obj_x86/STATIC_LIBRARIES/libcompiler_rt-extras_intermediates/libcompiler_rt-extras.a
> >   out/target/product/x86_64/obj_x86/STATIC_LIBRARIES/libatomic_intermediates/libatomic.a
> > out/target/product/x86_64/obj_x86/STATIC_LIBRARIES/libgcc_intermediates/libgcc.a
> > -Wl,-z,noexecstack -Wl,-z,relro -Wl,-z,now -Wl,--build-id=md5
> > -Wl,--warn-shared-textrel -Wl,--fatal-warnings -Wl,--gc-sections
> > -Wl,--hash-style=gnu -Wl,--no-undefined-version -m32  -target
> > i686-linux-android
> > -Bprebuilts/gcc/linux-x86/x86/x86_64-linux-android-4.9/x86_64-linux-android/bin
> > -Wl,--no-undefined out/target/product/x86_64/obj_x86/lib/libdrm.so
> > out/target/product/x86_64/obj_x86/lib/liblog.so
> > out/target/product/x86_64/obj_x86/lib/libcutils.so
> > out/target/product/x86_64/obj_x86/lib/libhardware_legacy.so
> > out/target/product/x86_64/obj_x86/lib/libdrm_intel.so
> > out/target/product/x86_64/obj_x86/lib/libdrm_radeon.so
> > out/target/product/x86_64/obj_x86/lib/libdrm_nouveau.so
> > out/target/product/x86_64/obj_x86/lib/libc++.so
> > out/target/product/x86_64/obj_x86/lib/libc.so
> > out/target/product/x86_64/obj_x86/lib/libm.so
> > out/target/product/x86_64/obj_x86/lib/libdl.so -o
> > out/target/product/x86_64/obj_x86/SHARED_LIBRARIES/libgralloc_drm_intermediates/LINKED/libgralloc_drm.so
> > out/target/product/x86_64/obj_x86/lib/crtend_so.o"
> > external/drm_gralloc/gralloc_drm_intel.c:631: error: undefined
> > reference to 'intel_is_genx'
> > external/drm_gralloc/gralloc_drm_intel.c:630: error: undefined
> > reference to 'intel_get_genx'
> > clang.real: error: linker command failed with exit code 1 (use -v to
> > see invocation)
> > 
> > 
> > That's because drm_gralloc use the IS_GEN9 macro
> > (and other IS_GEN{n} macros) directly.
> 
> Yeah, I thought some other stuff used the IS_GEN* macros ¯\_(ツ)_/¯
> 
> I assume my other patch ("intel: use drm namespace for exported functions",
> https://patchwork.kernel.org/patch/10590653/) works fine with drm_gralloc?

What a castle of cards we have here....

We should pick one build system and support only this one build system.

And the default build all should include tests. We shouldn't need a web
remote CI to inform us that we could be breaking the build.

When I pushed the first series I build locally using autotools and meson
and everything passed locally.

But then gitlab CI warned that ninja buildir test fail...

Than Daniel requested to use gitlab fork to make sure we pass CI
and here I went:

local ninja -C builddir test was happy and CI was green as we can see here:
https://gitlab.freedesktop.org/vivijim/drm/commit/b2c25182f29a0000b010afbb11748c213c9a3e8a/pipelines?ref=master

But then I just discovered that this broken autotools build!

> 
> > 
> > Since IS_GEN{n} are public APIs, I don't see
> > the rationale of this change.
> > Unless you are going to privatize all IS_GEN{n} macros.
> > (are you?)
> > 
> > 
> > 2018-09-13 0:03 GMT+08:00 Rodrigo Vivi <rodrigo.vivi at intel.com>:
> > > On Wed, Sep 12, 2018 at 08:50:54AM -0700, Rodrigo Vivi wrote:
> > >> From: Emil Velikov <emil.velikov at collabora.com>
> > >>
> > >> They're used internally and never meant to be part of the API.
> > >> Add the drm_private notation, which should resolve that.
> > >>
> > >> v2: (Rodrigo) Add missing include.
> > >> v3: (Rodrigo) Keep includes grouped per Eric suggestion.
> > >>
> > >> Cc: Eric Engestrom <eric.engestrom at intel.com>
> > >> Cc: Lucas De Marchi <lucas.demarchi at intel.com>
> > >> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> > >> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > >> Fixes: 4e81d4f9c9b ("intel: add generic functions to check PCI ID")
> > >> Signed-off-by: Emil Velikov <emil.velikov at collabora.com>
> > >> Acked-by: Lucas De Marchi <lucas.demarchi at intel.com>
> > >> Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > >> Reviewed-by: Eric Engestrom <eric.engestrom at intel.com>
> > >
> > > And pushed...
> > > thanks for patch, ack and review ;)
> > >
> > >> ---
> > >>  intel/intel_chipset.c | 4 ++--
> > >>  intel/intel_chipset.h | 5 +++--
> > >>  2 files changed, 5 insertions(+), 4 deletions(-)
> > >>
> > >> diff --git a/intel/intel_chipset.c b/intel/intel_chipset.c
> > >> index d5c33cc5..5aa4a2f2 100644
> > >> --- a/intel/intel_chipset.c
> > >> +++ b/intel/intel_chipset.c
> > >> @@ -44,7 +44,7 @@ static const struct pci_device {
> > >>       INTEL_SKL_IDS(9),
> > >>  };
> > >>
> > >> -bool intel_is_genx(unsigned int devid, int gen)
> > >> +drm_private bool intel_is_genx(unsigned int devid, int gen)
> > >>  {
> > >>       const struct pci_device *p,
> > >>                 *pend = pciids + sizeof(pciids) / sizeof(pciids[0]);
> > >> @@ -66,7 +66,7 @@ bool intel_is_genx(unsigned int devid, int gen)
> > >>       return false;
> > >>  }
> > >>
> > >> -bool intel_get_genx(unsigned int devid, int *gen)
> > >> +drm_private bool intel_get_genx(unsigned int devid, int *gen)
> > >>  {
> > >>       const struct pci_device *p,
> > >>                 *pend = pciids + sizeof(pciids) / sizeof(pciids[0]);
> > >> diff --git a/intel/intel_chipset.h b/intel/intel_chipset.h
> > >> index 9b1e64f1..5db207cc 100644
> > >> --- a/intel/intel_chipset.h
> > >> +++ b/intel/intel_chipset.h
> > >> @@ -329,9 +329,10 @@
> > >>
> > >>  /* New platforms use kernel pci ids */
> > >>  #include <stdbool.h>
> > >> +#include <libdrm_macros.h>
> > >>
> > >> -bool intel_is_genx(unsigned int devid, int gen);
> > >> -bool intel_get_genx(unsigned int devid, int *gen);
> > >> +drm_private bool intel_is_genx(unsigned int devid, int gen);
> > >> +drm_private bool intel_get_genx(unsigned int devid, int *gen);
> > >>
> > >>  #define IS_GEN9(devid) intel_is_genx(devid, 9)
> > >>  #define IS_GEN10(devid) intel_is_genx(devid, 10)
> > >> --
> > 
> > 
> > -- 
> > Chih-Wei
> > Android-x86 project
> > http://www.android-x86.org

More information about the dri-devel mailing list