[PATCH] drm/fourcc: introduce DRM_FOURCC_STANDALONE guard

[PATCH] drm/fourcc: introduce DRM_FOURCC_STANDALONE guard

[Emil Velikov]

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

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

Re: [PATCH] drm/fourcc: introduce DRM_FOURCC_STANDALONE guard

[James Park]

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

Re: [PATCH] drm/fourcc: introduce DRM_FOURCC_STANDALONE guard

[Emil Velikov]

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

Re: [PATCH] drm/fourcc: introduce DRM_FOURCC_STANDALONE guard

[James Park]

[-- Attachment #1.1: Type: text/plain, Size: 3081 bytes --]

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

[-- Attachment #1.2: Type: text/html, Size: 4528 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH] drm/fourcc: introduce DRM_FOURCC_STANDALONE guard

[Simon Ser]

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

Re: [PATCH] drm/fourcc: introduce DRM_FOURCC_STANDALONE guard

[Simon Ser]

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

Re: [PATCH] drm/fourcc: introduce DRM_FOURCC_STANDALONE guard

[Emil Velikov]

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

Re: [PATCH] drm/fourcc: introduce DRM_FOURCC_STANDALONE guard

[Simon Ser]

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

Re: [PATCH] drm/fourcc: introduce DRM_FOURCC_STANDALONE guard

[Emil Velikov]

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

Re: [PATCH] drm/fourcc: introduce DRM_FOURCC_STANDALONE guard

[Simon Ser]

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

Re: [PATCH] drm/fourcc: introduce DRM_FOURCC_STANDALONE guard

[Emil Velikov]

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

Re: [PATCH] drm/fourcc: introduce DRM_FOURCC_STANDALONE guard

[Simon Ser]

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

Re: [PATCH] drm/fourcc: introduce DRM_FOURCC_STANDALONE guard

[James Park]

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

Re: [PATCH] drm/fourcc: introduce DRM_FOURCC_STANDALONE guard

[James Park]

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

Re: [PATCH] drm/fourcc: introduce DRM_FOURCC_STANDALONE guard

[Emil Velikov]

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