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

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

[Rodrigo Vivi]

From: Emil Velikov <emil.velikov@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.

Cc: Eric Engestrom <eric.engestrom@intel.com>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Fixes: 4e81d4f9c9b ("intel: add generic functions to check PCI ID")
Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
Acked-by: Lucas De Marchi <lucas.demarchi@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 intel/intel_chipset.c | 4 ++--
 intel/intel_chipset.h | 6 ++++--
 2 files changed, 6 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..990a81e8 100644
--- a/intel/intel_chipset.h
+++ b/intel/intel_chipset.h
@@ -28,6 +28,8 @@
 #ifndef _INTEL_CHIPSET_H
 #define _INTEL_CHIPSET_H
 
+#include <libdrm_macros.h>
+
 #define PCI_CHIP_I810			0x7121
 #define PCI_CHIP_I810_DC100		0x7123
 #define PCI_CHIP_I810_E			0x7125
@@ -330,8 +332,8 @@
 /* New platforms use kernel pci ids */
 #include <stdbool.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)
-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Eric Engestrom]

On Tuesday, 2018-09-11 14:22:24 -0700, Rodrigo Vivi wrote:
> From: Emil Velikov <emil.velikov@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.
> 
> Cc: Eric Engestrom <eric.engestrom@intel.com>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Fixes: 4e81d4f9c9b ("intel: add generic functions to check PCI ID")
> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> Acked-by: Lucas De Marchi <lucas.demarchi@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  intel/intel_chipset.c | 4 ++--
>  intel/intel_chipset.h | 6 ++++--
>  2 files changed, 6 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..990a81e8 100644
> --- a/intel/intel_chipset.h
> +++ b/intel/intel_chipset.h
> @@ -28,6 +28,8 @@
>  #ifndef _INTEL_CHIPSET_H
>  #define _INTEL_CHIPSET_H
>  
> +#include <libdrm_macros.h>

Good catch on the missing #include :)
Might want to move it to keep it grouped with the other #include below,
but other than that:
Reviewed-by: Eric Engestrom <eric.engestrom@intel.com>

> +
>  #define PCI_CHIP_I810			0x7121
>  #define PCI_CHIP_I810_DC100		0x7123
>  #define PCI_CHIP_I810_E			0x7125
> @@ -330,8 +332,8 @@
>  /* New platforms use kernel pci ids */
>  #include <stdbool.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)
> -- 
> 2.17.1
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Rodrigo Vivi]

From: Emil Velikov <emil.velikov@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@intel.com>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Fixes: 4e81d4f9c9b ("intel: add generic functions to check PCI ID")
Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
Acked-by: Lucas De Marchi <lucas.demarchi@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Reviewed-by: Eric Engestrom <eric.engestrom@intel.com>
---
 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)
-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Rodrigo Vivi]

On Wed, Sep 12, 2018 at 08:50:54AM -0700, Rodrigo Vivi wrote:
> From: Emil Velikov <emil.velikov@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@intel.com>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Fixes: 4e81d4f9c9b ("intel: add generic functions to check PCI ID")
> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> Acked-by: Lucas De Marchi <lucas.demarchi@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Reviewed-by: Eric Engestrom <eric.engestrom@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)
> -- 
> 2.17.1
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Chih-Wei Huang]

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.

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@intel.com>:
> On Wed, Sep 12, 2018 at 08:50:54AM -0700, Rodrigo Vivi wrote:
>> From: Emil Velikov <emil.velikov@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@intel.com>
>> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Fixes: 4e81d4f9c9b ("intel: add generic functions to check PCI ID")
>> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
>> Acked-by: Lucas De Marchi <lucas.demarchi@intel.com>
>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Reviewed-by: Eric Engestrom <eric.engestrom@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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Eric Engestrom]

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?

> 
> 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@intel.com>:
> > On Wed, Sep 12, 2018 at 08:50:54AM -0700, Rodrigo Vivi wrote:
> >> From: Emil Velikov <emil.velikov@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@intel.com>
> >> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> >> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >> Fixes: 4e81d4f9c9b ("intel: add generic functions to check PCI ID")
> >> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> >> Acked-by: Lucas De Marchi <lucas.demarchi@intel.com>
> >> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >> Reviewed-by: Eric Engestrom <eric.engestrom@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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Rodrigo Vivi]

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@intel.com>:
> > > On Wed, Sep 12, 2018 at 08:50:54AM -0700, Rodrigo Vivi wrote:
> > >> From: Emil Velikov <emil.velikov@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@intel.com>
> > >> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > >> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > >> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > >> Fixes: 4e81d4f9c9b ("intel: add generic functions to check PCI ID")
> > >> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> > >> Acked-by: Lucas De Marchi <lucas.demarchi@intel.com>
> > >> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > >> Reviewed-by: Eric Engestrom <eric.engestrom@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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Eric Engestrom]

On Thursday, 2018-09-13 10:43:04 -0700, Rodrigo Vivi wrote:
> 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.

Deprecating autotools in libdrm is a worthwhile discussion, but it
should probably be its own thread, otherwise most people will miss it.

As for Android.mk, it isn't going anywhere in the near future; it would
require AOSP to support meson in their build system, or someone to write
a compatibility layer. There's already a thread somewhere on mesa-dev@
about this.

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

The CI runs the same tests you have locally ;)

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

`meson test` wouldn't have passed, but there is a bug in the autotools
test scripts that Emil has identified that made them not actually run...

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

This isn't a different build system, this is an external project that
uses libdrm (and depended on the macros that were rewritten).

> 
> > 
> > > 
> > > 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?)
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Lucas De Marchi]

+Chris

On 9/13/18 12:19 AM, 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.
> 
> Since IS_GEN{n} are public APIs, I don't see

IS_GEN() is *not* public API and should not be. It's an internal macro.

DESTDIR=/tmp/inst ninja -C build install
grep -r IS_GEN /tmp/inst/
Binary file /tmp/inst/usr/local/lib64/libdrm_intel.so.1.0.0 matches

   [  same thing when using autotools ]

Grepping https://android.googlesource.com/platform/external/drm_gralloc/ 
I see IS_GEN* is being used, but I don't see where it's getting it from, 
unless it's using private headers... Let's see by grepping it:

$ git grep intel_chipset
gralloc_drm_intel.c:#include "intel_chipset.h" /* for platform detection 
macros */


oh. You're a using a private header :(. How many places and libraries 
will we need to update to support different platforms? This is crazy.
AFAICS it only uses that to get the max_stride for each platform... this 
info should be coming from somewhere else. Chris, any idea here?


Lucas De Marchi

> 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@intel.com>:
>> On Wed, Sep 12, 2018 at 08:50:54AM -0700, Rodrigo Vivi wrote:
>>> From: Emil Velikov <emil.velikov@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@intel.com>
>>> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>> Fixes: 4e81d4f9c9b ("intel: add generic functions to check PCI ID")
>>> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
>>> Acked-by: Lucas De Marchi <lucas.demarchi@intel.com>
>>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>> Reviewed-by: Eric Engestrom <eric.engestrom@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)
>>> --
> 
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Chris Wilson]

Quoting Lucas De Marchi (2018-09-13 19:23:49)
> +Chris
> 
> On 9/13/18 12:19 AM, 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.
> > 
> > Since IS_GEN{n} are public APIs, I don't see
> 
> IS_GEN() is *not* public API and should not be. It's an internal macro.
> 
> DESTDIR=/tmp/inst ninja -C build install
> grep -r IS_GEN /tmp/inst/
> Binary file /tmp/inst/usr/local/lib64/libdrm_intel.so.1.0.0 matches
> 
>    [  same thing when using autotools ]
> 
> Grepping https://android.googlesource.com/platform/external/drm_gralloc/ 
> I see IS_GEN* is being used, but I don't see where it's getting it from, 
> unless it's using private headers... Let's see by grepping it:
> 
> $ git grep intel_chipset
> gralloc_drm_intel.c:#include "intel_chipset.h" /* for platform detection 
> macros */
> 
> 
> oh. You're a using a private header :(. How many places and libraries 
> will we need to update to support different platforms? This is crazy.
> AFAICS it only uses that to get the max_stride for each platform... this 
> info should be coming from somewhere else. Chris, any idea here?

Correct. They pulled libdrm into their library, they have the power to do
whatever they like. They should already be statically linking to
libdrm_intel.a and libdrm.a, so redefining drm_private to suit shouldn't
be an issue.

Just looking at the intel code suggests that drm_gralloc is but a toy.
-Chris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Chih-Wei Huang]

2018-09-14 2:23 GMT+08:00 Lucas De Marchi <lucas.demarchi@intel.com>:
> +Chris
>>
>> That's because drm_gralloc use the IS_GEN9 macro
>> (and other IS_GEN{n} macros) directly.
>>
>> Since IS_GEN{n} are public APIs, I don't see
>
>
> IS_GEN() is *not* public API and should not be. It's an internal macro.
>
> DESTDIR=/tmp/inst ninja -C build install
> grep -r IS_GEN /tmp/inst/
> Binary file /tmp/inst/usr/local/lib64/libdrm_intel.so.1.0.0 matches
>
>   [  same thing when using autotools ]
>
> Grepping https://android.googlesource.com/platform/external/drm_gralloc/ I
> see IS_GEN* is being used, but I don't see where it's getting it from,
> unless it's using private headers... Let's see by grepping it:
>
> $ git grep intel_chipset
> gralloc_drm_intel.c:#include "intel_chipset.h" /* for platform detection
> macros */
>
> oh. You're a using a private header :(. How many places and libraries will
> we need to update to support different platforms? This is crazy.
> AFAICS it only uses that to get the max_stride for each platform... this
> info should be coming from somewhere else. Chris, any idea here?

Hmm... There is no private declaration in this header.
Why is it private?

If so, what is the correct way to get the gen of Intel's GPU?
Or the userspace should not know it?


-- 
Chih-Wei
Android-x86 project
http://www.android-x86.org
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Lucas De Marchi]

On Wed, Sep 19, 2018 at 03:47:48PM +0800, Chih-Wei Huang wrote:
> 2018-09-14 2:23 GMT+08:00 Lucas De Marchi <lucas.demarchi@intel.com>:
> > +Chris
> >>
> >> That's because drm_gralloc use the IS_GEN9 macro
> >> (and other IS_GEN{n} macros) directly.
> >>
> >> Since IS_GEN{n} are public APIs, I don't see
> >
> >
> > IS_GEN() is *not* public API and should not be. It's an internal macro.
> >
> > DESTDIR=/tmp/inst ninja -C build install
> > grep -r IS_GEN /tmp/inst/
> > Binary file /tmp/inst/usr/local/lib64/libdrm_intel.so.1.0.0 matches
> >
> >   [  same thing when using autotools ]
> >
> > Grepping https://android.googlesource.com/platform/external/drm_gralloc/ I
> > see IS_GEN* is being used, but I don't see where it's getting it from,
> > unless it's using private headers... Let's see by grepping it:
> >
> > $ git grep intel_chipset
> > gralloc_drm_intel.c:#include "intel_chipset.h" /* for platform detection
> > macros */
> >
> > oh. You're a using a private header :(. How many places and libraries will
> > we need to update to support different platforms? This is crazy.
> > AFAICS it only uses that to get the max_stride for each platform... this
> > info should be coming from somewhere else. Chris, any idea here?
> 
> Hmm... There is no private declaration in this header.

???

> Why is it private?

All headers are private unless they are exported/installed. Android does
things a little differently by incorporating libdrm inside this drm_gralloc.

> 
> If so, what is the correct way to get the gen of Intel's GPU?
> Or the userspace should not know it?

libdrm *is* userspace. Better question: why do you need to know the gen? Can
this be decided in another way by using a properly exported function from
libdrm?

If you only want to hack it to work again, just link with the static library
since you are already incorporating the library, as Chris suggested. If you
want to do it right you may need to look into your library to see why it
is doing that.

Lucas De Marchi

> 
> 
> -- 
> Chih-Wei
> Android-x86 project
> http://www.android-x86.org
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Chih-Wei Huang]

2018-09-20 7:18 GMT+08:00 Lucas De Marchi <lucas.demarchi@intel.com>:
> On Wed, Sep 19, 2018 at 03:47:48PM +0800, Chih-Wei Huang wrote:
>> 2018-09-14 2:23 GMT+08:00 Lucas De Marchi <lucas.demarchi@intel.com>:
>> > +Chris
>> >>
>> >> That's because drm_gralloc use the IS_GEN9 macro
>> >> (and other IS_GEN{n} macros) directly.
>> >>
>> >> Since IS_GEN{n} are public APIs, I don't see
>> >
>> >
>> > IS_GEN() is *not* public API and should not be. It's an internal macro.
>> >
>> > DESTDIR=/tmp/inst ninja -C build install
>> > grep -r IS_GEN /tmp/inst/
>> > Binary file /tmp/inst/usr/local/lib64/libdrm_intel.so.1.0.0 matches
>> >
>> >   [  same thing when using autotools ]
>> >
>> > Grepping https://android.googlesource.com/platform/external/drm_gralloc/ I
>> > see IS_GEN* is being used, but I don't see where it's getting it from,
>> > unless it's using private headers... Let's see by grepping it:
>> >
>> > $ git grep intel_chipset
>> > gralloc_drm_intel.c:#include "intel_chipset.h" /* for platform detection
>> > macros */
>> >
>> > oh. You're a using a private header :(. How many places and libraries will
>> > we need to update to support different platforms? This is crazy.
>> > AFAICS it only uses that to get the max_stride for each platform... this
>> > info should be coming from somewhere else. Chris, any idea here?
>>
>> Hmm... There is no private declaration in this header.
>
> ???
>
>> Why is it private?
>
> All headers are private unless they are exported/installed. Android does
> things a little differently by incorporating libdrm inside this drm_gralloc.
>
>> If so, what is the correct way to get the gen of Intel's GPU?
>> Or the userspace should not know it?
>
> libdrm *is* userspace.

Sorry.
I meant an app which uses libdrm.
Shouldn't the app know the gen?

One interesting I just found.
I noticed Mesa has its own intel_chipset.h
which defines the IS_GEN3 macro.
(~src/mesa/drivers/dri/i915/intel_chipset.h)

So... to get the gen, I need to write my own header?
Or just copy libdrm's header?

> Better question: why do you need to know the gen? Can
> this be decided in another way by using a properly exported function from
> libdrm?

OK. That's another question.
The simple answer is I don't know.
That code is not written by me.
The code was written by some Google and Intel engineers.
So Intel engineer didn't think that header is private,
at least at that time he wrote the code...

> If you only want to hack it to work again, just link with the static library
> since you are already incorporating the library, as Chris suggested. If you
> want to do it right you may need to look into your library to see why it
> is doing that.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Emil Velikov]

From: Emil Velikov <emil.velikov@collabora.com>

They're used internally and never meant to be part of the API.
Add the drm_private notation, which should resolve that.

Cc: Eric Engestrom <eric.engestrom@intel.com>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Fixes: 4e81d4f9c9b ("intel: add generic functions to check PCI ID")
Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
---
 intel/intel_chipset.c | 4 ++--
 intel/intel_chipset.h | 4 ++--
 2 files changed, 4 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..63ddde7a 100644
--- a/intel/intel_chipset.h
+++ b/intel/intel_chipset.h
@@ -330,8 +330,8 @@
 /* New platforms use kernel pci ids */
 #include <stdbool.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)
-- 
2.18.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Chris Wilson]

Quoting Emil Velikov (2018-09-06 16:14:07)
> From: Emil Velikov <emil.velikov@collabora.com>
> 
> They're used internally and never meant to be part of the API.
> Add the drm_private notation, which should resolve that.
> 
> Cc: Eric Engestrom <eric.engestrom@intel.com>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Fixes: 4e81d4f9c9b ("intel: add generic functions to check PCI ID")
> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> ---
>  intel/intel_chipset.c | 4 ++--
>  intel/intel_chipset.h | 4 ++--
>  2 files changed, 4 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)

We should only need to put the attribute in the header, right?
-Chris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Lucas De Marchi]

On Thu, Sep 06, 2018 at 06:38:52PM +0100, Chris Wilson wrote:
> Quoting Emil Velikov (2018-09-06 16:14:07)
> > From: Emil Velikov <emil.velikov@collabora.com>
> > 
> > They're used internally and never meant to be part of the API.
> > Add the drm_private notation, which should resolve that.
> > 
> > Cc: Eric Engestrom <eric.engestrom@intel.com>
> > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Fixes: 4e81d4f9c9b ("intel: add generic functions to check PCI ID")
> > Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> > ---
> >  intel/intel_chipset.c | 4 ++--
> >  intel/intel_chipset.h | 4 ++--
> >  2 files changed, 4 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)
> 
> We should only need to put the attribute in the header, right?

IMO it actually makes more sense to be in the .c. Reason is that if we
are going to change to be hidden by default and annotate the public
ones, then we don't need to play with macros to hide them from other
projects including the header.

A declaration for a private symbol should not be in an exported header
so we know that all the functions in that header should actually be
public.


Lucas De Marchi

> -Chris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Chris Wilson]

Quoting Lucas De Marchi (2018-09-06 18:51:37)
> On Thu, Sep 06, 2018 at 06:38:52PM +0100, Chris Wilson wrote:
> > Quoting Emil Velikov (2018-09-06 16:14:07)
> > > From: Emil Velikov <emil.velikov@collabora.com>
> > > 
> > > They're used internally and never meant to be part of the API.
> > > Add the drm_private notation, which should resolve that.
> > > 
> > > Cc: Eric Engestrom <eric.engestrom@intel.com>
> > > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Fixes: 4e81d4f9c9b ("intel: add generic functions to check PCI ID")
> > > Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> > > ---
> > >  intel/intel_chipset.c | 4 ++--
> > >  intel/intel_chipset.h | 4 ++--
> > >  2 files changed, 4 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)
> > 
> > We should only need to put the attribute in the header, right?
> 
> IMO it actually makes more sense to be in the .c. Reason is that if we
> are going to change to be hidden by default and annotate the public
> ones, then we don't need to play with macros to hide them from other
> projects including the header.
> 
> A declaration for a private symbol should not be in an exported header
> so we know that all the functions in that header should actually be
> public.

And we definitely should not be and are not exporting intel_chipset.h.

I'd would rather have visibility declared in the header because that's
where I expect interface documentation.
-Chris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Lucas De Marchi]

On Thu, Sep 06, 2018 at 06:59:33PM +0100, Chris Wilson wrote:
> Quoting Lucas De Marchi (2018-09-06 18:51:37)
> > On Thu, Sep 06, 2018 at 06:38:52PM +0100, Chris Wilson wrote:
> > > Quoting Emil Velikov (2018-09-06 16:14:07)
> > > > From: Emil Velikov <emil.velikov@collabora.com>
> > > > 
> > > > They're used internally and never meant to be part of the API.
> > > > Add the drm_private notation, which should resolve that.
> > > > 
> > > > Cc: Eric Engestrom <eric.engestrom@intel.com>
> > > > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > Fixes: 4e81d4f9c9b ("intel: add generic functions to check PCI ID")
> > > > Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> > > > ---
> > > >  intel/intel_chipset.c | 4 ++--
> > > >  intel/intel_chipset.h | 4 ++--
> > > >  2 files changed, 4 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)
> > > 
> > > We should only need to put the attribute in the header, right?
> > 
> > IMO it actually makes more sense to be in the .c. Reason is that if we
> > are going to change to be hidden by default and annotate the public
> > ones, then we don't need to play with macros to hide them from other
> > projects including the header.
> > 
> > A declaration for a private symbol should not be in an exported header
> > so we know that all the functions in that header should actually be
> > public.
> 
> And we definitely should not be and are not exporting intel_chipset.h.
> 
> I'd would rather have visibility declared in the header because that's
> where I expect interface documentation.

but then if the header is exported, you need to export the definition of
the macro as well.. and undef it when it's not for internal use. I've
seen nasty things on projects that went this route, because then
you not only depend on the compiler version you are compiled with, but
you also need to keep the flexibility for other projects that are
including you. Example:


#ifdef EAPI
# undef EAPI
#endif

#ifdef _WIN32
# ifdef EFL_BUILD
#  ifdef DLL_EXPORT
#   define EAPI __declspec(dllexport)
#  else
#   define EAPI
#  endif
# else
#  define EAPI __declspec(dllimport)
# endif
# define EAPI_WEAK
#else
# ifdef __GNUC__
#  if __GNUC__ >= 4
#   define EAPI __attribute__ ((visibility("default")))
#   define EAPI_WEAK __attribute__ ((weak))
#  else
#   define EAPI
#   define EAPI_WEAK
#  endif
# else
#  define EAPI
#  define EAPI_WEAK
# endif
#endif


Lucas De Marchi
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Lucas De Marchi]

On Thu, Sep 06, 2018 at 04:14:07PM +0100, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov@collabora.com>
> 
> They're used internally and never meant to be part of the API.
> Add the drm_private notation, which should resolve that.
> 
> Cc: Eric Engestrom <eric.engestrom@intel.com>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Fixes: 4e81d4f9c9b ("intel: add generic functions to check PCI ID")
> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> ---
>  intel/intel_chipset.c | 4 ++--
>  intel/intel_chipset.h | 4 ++--
>  2 files changed, 4 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..63ddde7a 100644
> --- a/intel/intel_chipset.h
> +++ b/intel/intel_chipset.h
> @@ -330,8 +330,8 @@
>  /* New platforms use kernel pci ids */
>  #include <stdbool.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)
> -- 

As a short-term fix,

Acked-by: Lucas De Marchi <lucas.demarchi@intel.com>


I think we should really migrate to making them hidden by default and
only export the ones we want.  When drm_public was removed, it was
basically a nop since the visibility was still set as default. I will
take a look on this.


Lucas De Marchi

> 2.18.0
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel