| Message ID | 20180906151407.9744-1-emil.l.velikov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [libdrm] intel: annotate the intel genx helpers as private | expand |
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
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 >
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
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
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
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)