| Message ID | 20210202224704.2794318-1-emil.l.velikov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/fourcc: introduce DRM_FOURCC_STANDALONE guard | expand |
On Tue, Feb 2, 2021 at 2:47 PM Emil Velikov <emil.l.velikov@gmail.com> wrote: > > Currently, the drm_fourcc.h header depends on drm.h for __u32 and __u64. > At the same time drm.h pulls a lot of unneeded symbols. > > Add new guard DRM_FOURCC_STANDALONE, which when set will use local > declaration of said symbols. > > When used on linux - this is a trivial but only when building in strict c99 > mode. One is welcome to ignore the warning, silence it or use c11. If neither > of the three is an option, then do _not_ set the new guard. > > Cc: James Park <james.park@lagfreegames.com> > Cc: Pekka Paalanen <pekka.paalanen@collabora.com> > Cc: Simon Ser <contact@emersion.fr> > Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com> > --- > As mentioned before - there's little point in having yet another header > since keeping those in sync has been a PITA in the past. > --- > include/uapi/drm/drm_fourcc.h | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > > diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h > index 6f0628eb13a6..c1522902f6c9 100644 > --- a/include/uapi/drm/drm_fourcc.h > +++ b/include/uapi/drm/drm_fourcc.h > @@ -24,7 +24,26 @@ > #ifndef DRM_FOURCC_H > #define DRM_FOURCC_H > > +/* > + * Define DRM_FOURCC_STANDALONE you're interested only FOURCC and do not want > + * to pull drm.h into your application. > + */ > +#ifdef DRM_FOURCC_STANDALONE > +#if defined(__linux__) > + > +#include <linux/types.h> > + > +#else /* One of the BSDs */ > + > +#include <stdint.h> > +typedef uint32_t __u32; > +typedef uint64_t __u64; > + > +#endif /* __linux __ */ > + > +#else > #include "drm.h" > +#endif /* DRM_FOURCC_STANDALONE */ > > #if defined(__cplusplus) > extern "C" { > -- > 2.30.0 > One of my earlier solutions similarly would have forced people to deal with duplicate typedefs, and we arrived at the current solution because we didn't want to burden anyone with that. I feel like having the extra include-guarded file is the lesser of evils. If it helps the patch go through, then I'd drop my preference but I really think the current solution is better. - James
On Tue, 2 Feb 2021 at 23:25, James Park <james.park@lagfreegames.com> wrote: > > On Tue, Feb 2, 2021 at 2:47 PM Emil Velikov <emil.l.velikov@gmail.com> wrote: > > > > Currently, the drm_fourcc.h header depends on drm.h for __u32 and __u64. > > At the same time drm.h pulls a lot of unneeded symbols. > > > > Add new guard DRM_FOURCC_STANDALONE, which when set will use local > > declaration of said symbols. > > > > When used on linux - this is a trivial but only when building in strict c99 > > mode. One is welcome to ignore the warning, silence it or use c11. If neither > > of the three is an option, then do _not_ set the new guard. > > > > Cc: James Park <james.park@lagfreegames.com> > > Cc: Pekka Paalanen <pekka.paalanen@collabora.com> > > Cc: Simon Ser <contact@emersion.fr> > > Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com> > > --- > > As mentioned before - there's little point in having yet another header > > since keeping those in sync has been a PITA in the past. > > --- > > include/uapi/drm/drm_fourcc.h | 19 +++++++++++++++++++ > > 1 file changed, 19 insertions(+) > > > > diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h > > index 6f0628eb13a6..c1522902f6c9 100644 > > --- a/include/uapi/drm/drm_fourcc.h > > +++ b/include/uapi/drm/drm_fourcc.h > > @@ -24,7 +24,26 @@ > > #ifndef DRM_FOURCC_H > > #define DRM_FOURCC_H > > > > +/* > > + * Define DRM_FOURCC_STANDALONE you're interested only FOURCC and do not want > > + * to pull drm.h into your application. > > + */ > > +#ifdef DRM_FOURCC_STANDALONE > > +#if defined(__linux__) > > + > > +#include <linux/types.h> > > + > > +#else /* One of the BSDs */ > > + > > +#include <stdint.h> > > +typedef uint32_t __u32; > > +typedef uint64_t __u64; > > + > > +#endif /* __linux __ */ > > + > > +#else > > #include "drm.h" > > +#endif /* DRM_FOURCC_STANDALONE */ > > > > #if defined(__cplusplus) > > extern "C" { > > -- > > 2.30.0 > > > > One of my earlier solutions similarly would have forced people to deal > with duplicate typedefs, and we arrived at the current solution > because we didn't want to burden anyone with that. As summed in the commit message the burden is only applicable when all of the following are set: - non-linux - force DRM_FOURCC_STANDALONE - c99 -pedantic Even then, we're talking about a compilation warning. So yeah - let's keep things short and sweet. Side note: AFAICT MSVC will not trigger a warning so your logs should be clean. -Emil
On Tue, Feb 2, 2021 at 4:57 PM Emil Velikov <emil.l.velikov@gmail.com> wrote: > On Tue, 2 Feb 2021 at 23:25, James Park <james.park@lagfreegames.com> > wrote: > > > > On Tue, Feb 2, 2021 at 2:47 PM Emil Velikov <emil.l.velikov@gmail.com> > wrote: > > > > > > Currently, the drm_fourcc.h header depends on drm.h for __u32 and > __u64. > > > At the same time drm.h pulls a lot of unneeded symbols. > > > > > > Add new guard DRM_FOURCC_STANDALONE, which when set will use local > > > declaration of said symbols. > > > > > > When used on linux - this is a trivial but only when building in > strict c99 > > > mode. One is welcome to ignore the warning, silence it or use c11. If > neither > > > of the three is an option, then do _not_ set the new guard. > > > > > > Cc: James Park <james.park@lagfreegames.com> > > > Cc: Pekka Paalanen <pekka.paalanen@collabora.com> > > > Cc: Simon Ser <contact@emersion.fr> > > > Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com> > > > --- > > > As mentioned before - there's little point in having yet another header > > > since keeping those in sync has been a PITA in the past. > > > --- > > > include/uapi/drm/drm_fourcc.h | 19 +++++++++++++++++++ > > > 1 file changed, 19 insertions(+) > > > > > > diff --git a/include/uapi/drm/drm_fourcc.h > b/include/uapi/drm/drm_fourcc.h > > > index 6f0628eb13a6..c1522902f6c9 100644 > > > --- a/include/uapi/drm/drm_fourcc.h > > > +++ b/include/uapi/drm/drm_fourcc.h > > > @@ -24,7 +24,26 @@ > > > #ifndef DRM_FOURCC_H > > > #define DRM_FOURCC_H > > > > > > +/* > > > + * Define DRM_FOURCC_STANDALONE you're interested only FOURCC and do > not want > > > + * to pull drm.h into your application. > > > + */ > > > +#ifdef DRM_FOURCC_STANDALONE > > > +#if defined(__linux__) > > > + > > > +#include <linux/types.h> > > > + > > > +#else /* One of the BSDs */ > > > + > > > +#include <stdint.h> > > > +typedef uint32_t __u32; > > > +typedef uint64_t __u64; > > > + > > > +#endif /* __linux __ */ > > > + > > > +#else > > > #include "drm.h" > > > +#endif /* DRM_FOURCC_STANDALONE */ > > > > > > #if defined(__cplusplus) > > > extern "C" { > > > -- > > > 2.30.0 > > > > > > > One of my earlier solutions similarly would have forced people to deal > > with duplicate typedefs, and we arrived at the current solution > > because we didn't want to burden anyone with that. > > As summed in the commit message the burden is only applicable when all > of the following are set: > - non-linux > - force DRM_FOURCC_STANDALONE > - c99 -pedantic > > Even then, we're talking about a compilation warning. So yeah - let's > keep things short and sweet. > > Side note: AFAICT MSVC will not trigger a warning so your logs should be > clean. > > -Emil > I'm having trouble reading your commit message, this sentence in particular: "When used on linux - this is a trivial but only when building in strict c99 mode." This asymmetric copy/paste grosses me out so much. I don't think a patch like this should be opinionated about someone's build settings. Am I alone? Doesn't this bother anyone else? - James
On Wednesday, February 3rd, 2021 at 1:56 AM, Emil Velikov <emil.l.velikov@gmail.com> wrote: > As summed in the commit message the burden is only applicable when all > of the following are set: > - non-linux > - force DRM_FOURCC_STANDALONE > - c99 -pedantic > > Even then, we're talking about a compilation warning. So yeah - let's > keep things short and sweet. Sorry, I don't think this is acceptable. Regardless of your personal preference some projects have -Werror -pendantic, and we shouldn't make them fail the build because of a libdrm header.
On Wednesday, February 3rd, 2021 at 1:56 AM, Emil Velikov emil.l.velikov@gmail.com wrote: > As summed in the commit message the burden is only applicable when all > of the following are set: > - non-linux > - force DRM_FOURCC_STANDALONE > - c99 -pedantic Oh, and FWIW, this is not a theoretical situation at all. All of these conditions happen to be true on my compositor. It has FreeBSD CI, -Werror, and will use DRM_FOURCC_STANDALONE when available.
On Wed, 3 Feb 2021 at 09:27, Simon Ser <contact@emersion.fr> wrote: > > On Wednesday, February 3rd, 2021 at 1:56 AM, Emil Velikov emil.l.velikov@gmail.com wrote: > > > As summed in the commit message the burden is only applicable when all > > of the following are set: > > - non-linux > > - force DRM_FOURCC_STANDALONE > > - c99 -pedantic > > Oh, and FWIW, this is not a theoretical situation at all. All of these > conditions happen to be true on my compositor. It has FreeBSD CI, > -Werror, and will use DRM_FOURCC_STANDALONE when available. There are ways to disable [1] or silence [2] this - are you intentionally ignoring them? Or the goal here is to 'fix' the kernel for a very uncommon non-linux use-case? -Emil [1] pragma GCC diagnostic warning "-Wpedantic" [2] pragma GCC diagnostic ignored or -std=c11
On Wednesday, February 3rd, 2021 at 11:08 AM, Emil Velikov <emil.l.velikov@gmail.com> wrote: > On Wed, 3 Feb 2021 at 09:27, Simon Ser <contact@emersion.fr> wrote: > > > > On Wednesday, February 3rd, 2021 at 1:56 AM, Emil Velikov emil.l.velikov@gmail.com wrote: > > > > > As summed in the commit message the burden is only applicable when all > > > of the following are set: > > > - non-linux > > > - force DRM_FOURCC_STANDALONE > > > - c99 -pedantic > > > > Oh, and FWIW, this is not a theoretical situation at all. All of these > > conditions happen to be true on my compositor. It has FreeBSD CI, > > -Werror, and will use DRM_FOURCC_STANDALONE when available. > > There are ways to disable [1] or silence [2] this - are you > intentionally ignoring them? We have a policy against pragma. However we already use -std=c11, so in fact wouldn't be affected by this change. > Or the goal here is to 'fix' the kernel for a very uncommon non-linux use-case? If the kernel doesn't care about non-Linux, why is there an #ifdef __linux__ in the first place? > [1] pragma GCC diagnostic warning "-Wpedantic" > [2] pragma GCC diagnostic ignored or -std=c11
On Wed, 3 Feb 2021 at 10:15, Simon Ser <contact@emersion.fr> wrote: > > On Wednesday, February 3rd, 2021 at 11:08 AM, Emil Velikov <emil.l.velikov@gmail.com> wrote: > > > On Wed, 3 Feb 2021 at 09:27, Simon Ser <contact@emersion.fr> wrote: > > > > > > On Wednesday, February 3rd, 2021 at 1:56 AM, Emil Velikov emil.l.velikov@gmail.com wrote: > > > > > > > As summed in the commit message the burden is only applicable when all > > > > of the following are set: > > > > - non-linux > > > > - force DRM_FOURCC_STANDALONE > > > > - c99 -pedantic > > > > > > Oh, and FWIW, this is not a theoretical situation at all. All of these > > > conditions happen to be true on my compositor. It has FreeBSD CI, > > > -Werror, and will use DRM_FOURCC_STANDALONE when available. > > > > There are ways to disable [1] or silence [2] this - are you > > intentionally ignoring them? > > We have a policy against pragma. However we already use -std=c11, so in fact > wouldn't be affected by this change. > No issue then, great. Let's merge this trivial solution and move on to other things. -Emil
On Wednesday, February 3rd, 2021 at 12:03 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote: > No issue then, great. Let's merge this trivial solution and move on to > other things. Just because one compositor isn't affected isn't an excuse for the kernel to force its users to use a specific C standard.
On Wed, 3 Feb 2021 at 13:47, Simon Ser <contact@emersion.fr> wrote: > > On Wednesday, February 3rd, 2021 at 12:03 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote: > > > No issue then, great. Let's merge this trivial solution and move on to > > other things. > > Just because one compositor isn't affected isn't an excuse for the > kernel to force its users to use a specific C standard. As said before, there are multiple ways to handle this _without_ introducing yet another UAPI header. I don't see why you're dismissing all of them, can you elaborate? Thanks Emil
On Wednesday, February 3rd, 2021 at 3:13 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote: > As said before, there are multiple ways to handle this without > introducing yet another UAPI header. I don't see why you're dismissing > all of them, can you elaborate? Because I hate it when I have to adjust my compiler flags because of some third-party header. Can you explain what were the past issues with introducing a new header?
Apologies for anything I've said so far that has been harsh. I'd like this discussion to be civil. I'm not sure if Simon is still on board with a patch given his thumbs up to Erik's comment on the Mesa merge request (which I responded to), but I would also like to know why adding another header file is problematic. I would prefer to keep the definitions deduplicated and make the code robust even for edge cases unless there's a good reason not to. Avoiding an extra file doesn't seem like a good enough reason to me, but I also don't have to maintain codebases that rely on these headers, so maybe there's something I'm overlooking. Thanks, James On Wed, Feb 3, 2021 at 6:21 AM Simon Ser <contact@emersion.fr> wrote: > > On Wednesday, February 3rd, 2021 at 3:13 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote: > > > As said before, there are multiple ways to handle this without > > introducing yet another UAPI header. I don't see why you're dismissing > > all of them, can you elaborate? > > Because I hate it when I have to adjust my compiler flags because of > some third-party header. > > Can you explain what were the past issues with introducing a new > header?
On Wed, Feb 3, 2021 at 8:24 PM James Park <james.park@lagfreegames.com> wrote: > > Apologies for anything I've said so far that has been harsh. I'd like > this discussion to be civil. > > I'm not sure if Simon is still on board with a patch given his thumbs > up to Erik's comment on the Mesa merge request (which I responded to), > but I would also like to know why adding another header file is > problematic. I would prefer to keep the definitions deduplicated and > make the code robust even for edge cases unless there's a good reason > not to. Avoiding an extra file doesn't seem like a good enough reason > to me, but I also don't have to maintain codebases that rely on these > headers, so maybe there's something I'm overlooking. > > Thanks, > James > > On Wed, Feb 3, 2021 at 6:21 AM Simon Ser <contact@emersion.fr> wrote: > > > > On Wednesday, February 3rd, 2021 at 3:13 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote: > > > > > As said before, there are multiple ways to handle this without > > > introducing yet another UAPI header. I don't see why you're dismissing > > > all of them, can you elaborate? > > > > Because I hate it when I have to adjust my compiler flags because of > > some third-party header. > > > > Can you explain what were the past issues with introducing a new > > header? And sorry for top-posting. Gmail habits.
On Wed, 3 Feb 2021 at 14:21, Simon Ser <contact@emersion.fr> wrote: > > On Wednesday, February 3rd, 2021 at 3:13 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote: > > > As said before, there are multiple ways to handle this without > > introducing yet another UAPI header. I don't see why you're dismissing > > all of them, can you elaborate? > > Because I hate it when I have to adjust my compiler flags because of > some third-party header. > Mentioned it over IRC, but adding here for posterity: I think it's perfectly normal to be unhappy, angry, etc but please mention that upfront so that we know what we're working with. In this case, one gets to deal with the warning, if they _explicitly_ opts-in. That said, v2 should be warnings free in virtually any permutation. > Can you explain what were the past issues with introducing a new > header? Few that come to mind: - distros shipping partial header set - mixing headers, be that any of: - distros shipping kernel headers, as well as libdrm - system libdrm and local - be that imported or installed to /usr/local/ A bigger annoyance that just came to mind - having the same header name/guard within your project as the one introduced. So while it may seem like bikeshed, there are many subtle ways where things can go bad :-\ HTH Emil
diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h index 6f0628eb13a6..c1522902f6c9 100644 --- a/include/uapi/drm/drm_fourcc.h +++ b/include/uapi/drm/drm_fourcc.h @@ -24,7 +24,26 @@ #ifndef DRM_FOURCC_H #define DRM_FOURCC_H +/* + * Define DRM_FOURCC_STANDALONE you're interested only FOURCC and do not want + * to pull drm.h into your application. + */ +#ifdef DRM_FOURCC_STANDALONE +#if defined(__linux__) + +#include <linux/types.h> + +#else /* One of the BSDs */ + +#include <stdint.h> +typedef uint32_t __u32; +typedef uint64_t __u64; + +#endif /* __linux __ */ + +#else #include "drm.h" +#endif /* DRM_FOURCC_STANDALONE */ #if defined(__cplusplus) extern "C" {
Currently, the drm_fourcc.h header depends on drm.h for __u32 and __u64. At the same time drm.h pulls a lot of unneeded symbols. Add new guard DRM_FOURCC_STANDALONE, which when set will use local declaration of said symbols. When used on linux - this is a trivial but only when building in strict c99 mode. One is welcome to ignore the warning, silence it or use c11. If neither of the three is an option, then do _not_ set the new guard. Cc: James Park <james.park@lagfreegames.com> Cc: Pekka Paalanen <pekka.paalanen@collabora.com> Cc: Simon Ser <contact@emersion.fr> Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com> --- As mentioned before - there's little point in having yet another header since keeping those in sync has been a PITA in the past. --- include/uapi/drm/drm_fourcc.h | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)