diff mbox series

drm: Fix drm.h uapi header for Windows

Message ID 1606816916-3724-2-git-send-email-jpark37@lagfreegames.com (mailing list archive)
State New, archived
Headers show
Series drm: Fix drm.h uapi header for Windows | expand

Commit Message

James Park Dec. 1, 2020, 10:01 a.m. UTC
This will allow Mesa to port code to Windows more easily.

Hide BSD header and drm_handle_t behind _WIN32 check.

Change __volatile__ to volatile, which is standard.
---
 include/uapi/drm/drm.h | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

Comments

Simon Ser Dec. 2, 2020, 8:46 a.m. UTC | #1
Can you add a Signed-off-by line to your commit message? This means
you agree to the Developer Certificate of Origin [1].

[1]: https://developercertificate.org/
James Park Dec. 2, 2020, 9:07 a.m. UTC | #2
Attempting to submit patch in response to https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/6162#note_712454

This will allow Mesa to port code to Windows more easily.

Hide BSD header and drm_handle_t behind _WIN32 check.

Change __volatile__ to volatile, which is standard.

Signed-off-by: James Park <jpark37@lagfreegames.com>

James Park (1):
  drm: Fix drm.h uapi header for Windows

 include/uapi/drm/drm.h | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)
Michel Dänzer Dec. 2, 2020, 11:42 a.m. UTC | #3
On 2020-12-01 11:01 a.m., James Park wrote:
> This will allow Mesa to port code to Windows more easily.

As discussed in 
https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/6162#note_712779 
, including drm.h makes no sense when building for Windows.
Daniel Vetter Dec. 2, 2020, 12:46 p.m. UTC | #4
On Wed, Dec 2, 2020 at 12:43 PM Michel Dänzer <michel@daenzer.net> wrote:
>
> On 2020-12-01 11:01 a.m., James Park wrote:
> > This will allow Mesa to port code to Windows more easily.
>
> As discussed in
> https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/6162#note_712779
> , including drm.h makes no sense when building for Windows.

Yeah I think it'd be cleanest if we can avoid this. If not I think the
right fix would be to split out the actually needed parts from drm.h
into a new header (still included by drm.h for backwards compat
reasons) which mesa can use. Since it looks like the problematic parts
are the legacy gunk, and not the new ioctl structures. Pulling out
drm_render.h for all the render stuff and mabe drm_vblank.h for the
vblank stuff (which would fit better in drm_mode.h but mistakes were
made, oops).
-Daniel

>
> --
> Earthling Michel Dänzer               |               https://redhat.com
> Libre software enthusiast             |             Mesa and X developer
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Michel Dänzer Dec. 2, 2020, 6:06 p.m. UTC | #5
On 2020-12-02 1:46 p.m., Daniel Vetter wrote:
> On Wed, Dec 2, 2020 at 12:43 PM Michel Dänzer <michel@daenzer.net> wrote:
>>
>> On 2020-12-01 11:01 a.m., James Park wrote:
>>> This will allow Mesa to port code to Windows more easily.
>>
>> As discussed in
>> https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/6162#note_712779
>> , including drm.h makes no sense when building for Windows.
> 
> Yeah I think it'd be cleanest if we can avoid this. If not I think the
> right fix would be to split out the actually needed parts from drm.h
> into a new header (still included by drm.h for backwards compat
> reasons) which mesa can use. Since it looks like the problematic parts
> are the legacy gunk, and not the new ioctl structures. Pulling out
> drm_render.h for all the render stuff and mabe drm_vblank.h for the
> vblank stuff (which would fit better in drm_mode.h but mistakes were
> made, oops).

If anything currently in drm.h is needed while building for Windows, it 
points to a broken abstraction somewhere in userspace. (Specifically, 
the Mesa Gallium/Vulkan winsys is supposed to abstract away platform 
details like these)
James Park Dec. 2, 2020, 7:47 p.m. UTC | #6
I can avoid modifying drm.h by doing this to drm_fourcc.h:

#ifdef _WIN32
#include <stdint.h>
typedef uint64_t __u64;
#else
#include "drm.h"
#endif

And this to amdgpu_drm.h:

#ifdef _WIN32
#include <stdint.h>
typedef int32_t  __s32;
typedef uint32_t __u32;
typedef uint64_t __u64;
#else
#include "drm.h"
#endif

But now I'm touching two files under drm-uapi instead of one, and weirdly.

If we're trying to cut ties with the drm-uapi folder entirely, the stuff
ac_surface.c need includes the AMD_FMT_MOD stuff in drm_fourcc.h,
and AMDGPU_TILING_* under amdgpu_drm.h. Is there a better spot for these
definitions?

Thanks,
James

On Wed, Dec 2, 2020 at 10:06 AM Michel Dänzer <michel@daenzer.net> wrote:

> On 2020-12-02 1:46 p.m., Daniel Vetter wrote:
> > On Wed, Dec 2, 2020 at 12:43 PM Michel Dänzer <michel@daenzer.net>
> wrote:
> >>
> >> On 2020-12-01 11:01 a.m., James Park wrote:
> >>> This will allow Mesa to port code to Windows more easily.
> >>
> >> As discussed in
> >>
> https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/6162#note_712779
> >> , including drm.h makes no sense when building for Windows.
> >
> > Yeah I think it'd be cleanest if we can avoid this. If not I think the
> > right fix would be to split out the actually needed parts from drm.h
> > into a new header (still included by drm.h for backwards compat
> > reasons) which mesa can use. Since it looks like the problematic parts
> > are the legacy gunk, and not the new ioctl structures. Pulling out
> > drm_render.h for all the render stuff and mabe drm_vblank.h for the
> > vblank stuff (which would fit better in drm_mode.h but mistakes were
> > made, oops).
>
> If anything currently in drm.h is needed while building for Windows, it
> points to a broken abstraction somewhere in userspace. (Specifically,
> the Mesa Gallium/Vulkan winsys is supposed to abstract away platform
> details like these)
>
>
> --
> Earthling Michel Dänzer               |               https://redhat.com
> Libre software enthusiast             |             Mesa and X developer
>
Daniel Vetter Dec. 2, 2020, 10:25 p.m. UTC | #7
On Wed, Dec 2, 2020 at 8:48 PM James Park <james.park@lagfreegames.com> wrote:
>
> I can avoid modifying drm.h by doing this to drm_fourcc.h:
>
> #ifdef _WIN32
> #include <stdint.h>
> typedef uint64_t __u64;
> #else
> #include "drm.h"
> #endif
>
> And this to amdgpu_drm.h:
>
> #ifdef _WIN32
> #include <stdint.h>
> typedef int32_t  __s32;
> typedef uint32_t __u32;
> typedef uint64_t __u64;
> #else
> #include "drm.h"
> #endif
>
> But now I'm touching two files under drm-uapi instead of one, and weirdly.
>
> If we're trying to cut ties with the drm-uapi folder entirely, the stuff ac_surface.c need includes the AMD_FMT_MOD stuff in drm_fourcc.h, and AMDGPU_TILING_* under amdgpu_drm.h. Is there a better spot for these definitions?

The drm_fourcc.h maybe makes some sense (I think in some places mesa
uses these internally, and many drivers use the modifiers directly in
the main driver). But the amdgpu header should be all ioctl stuff,
which should be all entirely in the winsys only.

Also kinda disappointing that drm_fourcc.h includes drm.h and isn't
standalone, but I guess that sailed (at least for linux).
-Daniel

> Thanks,
> James
>
> On Wed, Dec 2, 2020 at 10:06 AM Michel Dänzer <michel@daenzer.net> wrote:
>>
>> On 2020-12-02 1:46 p.m., Daniel Vetter wrote:
>> > On Wed, Dec 2, 2020 at 12:43 PM Michel Dänzer <michel@daenzer.net> wrote:
>> >>
>> >> On 2020-12-01 11:01 a.m., James Park wrote:
>> >>> This will allow Mesa to port code to Windows more easily.
>> >>
>> >> As discussed in
>> >> https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/6162#note_712779
>> >> , including drm.h makes no sense when building for Windows.
>> >
>> > Yeah I think it'd be cleanest if we can avoid this. If not I think the
>> > right fix would be to split out the actually needed parts from drm.h
>> > into a new header (still included by drm.h for backwards compat
>> > reasons) which mesa can use. Since it looks like the problematic parts
>> > are the legacy gunk, and not the new ioctl structures. Pulling out
>> > drm_render.h for all the render stuff and mabe drm_vblank.h for the
>> > vblank stuff (which would fit better in drm_mode.h but mistakes were
>> > made, oops).
>>
>> If anything currently in drm.h is needed while building for Windows, it
>> points to a broken abstraction somewhere in userspace. (Specifically,
>> the Mesa Gallium/Vulkan winsys is supposed to abstract away platform
>> details like these)
>>
>>
>> --
>> Earthling Michel Dänzer               |               https://redhat.com
>> Libre software enthusiast             |             Mesa and X developer
James Park Dec. 3, 2020, 1:24 a.m. UTC | #8
If the definitions in drm_fourcc.h make sense to live there, and we can't
remove drm.h from that header for backward compatibility, and the code that
I'm trying to compile on Windows needs the definitions in drm_fourcc.h,
then doesn't it make sense to adjust drm.h?

The patch that I'm proposing doesn't change very much. It might be easier
to read here:
https://github.com/jpark37/linux/commit/648e9281824ddc943c3ea6b34d6d6c154717a0a3

Thanks,
James

On Wed, Dec 2, 2020 at 2:26 PM Daniel Vetter <daniel@ffwll.ch> wrote:

> On Wed, Dec 2, 2020 at 8:48 PM James Park <james.park@lagfreegames.com>
> wrote:
> >
> > I can avoid modifying drm.h by doing this to drm_fourcc.h:
> >
> > #ifdef _WIN32
> > #include <stdint.h>
> > typedef uint64_t __u64;
> > #else
> > #include "drm.h"
> > #endif
> >
> > And this to amdgpu_drm.h:
> >
> > #ifdef _WIN32
> > #include <stdint.h>
> > typedef int32_t  __s32;
> > typedef uint32_t __u32;
> > typedef uint64_t __u64;
> > #else
> > #include "drm.h"
> > #endif
> >
> > But now I'm touching two files under drm-uapi instead of one, and
> weirdly.
> >
> > If we're trying to cut ties with the drm-uapi folder entirely, the stuff
> ac_surface.c need includes the AMD_FMT_MOD stuff in drm_fourcc.h, and
> AMDGPU_TILING_* under amdgpu_drm.h. Is there a better spot for these
> definitions?
>
> The drm_fourcc.h maybe makes some sense (I think in some places mesa
> uses these internally, and many drivers use the modifiers directly in
> the main driver). But the amdgpu header should be all ioctl stuff,
> which should be all entirely in the winsys only.
>
> Also kinda disappointing that drm_fourcc.h includes drm.h and isn't
> standalone, but I guess that sailed (at least for linux).
> -Daniel
>
> > Thanks,
> > James
> >
> > On Wed, Dec 2, 2020 at 10:06 AM Michel Dänzer <michel@daenzer.net>
> wrote:
> >>
> >> On 2020-12-02 1:46 p.m., Daniel Vetter wrote:
> >> > On Wed, Dec 2, 2020 at 12:43 PM Michel Dänzer <michel@daenzer.net>
> wrote:
> >> >>
> >> >> On 2020-12-01 11:01 a.m., James Park wrote:
> >> >>> This will allow Mesa to port code to Windows more easily.
> >> >>
> >> >> As discussed in
> >> >>
> https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/6162#note_712779
> >> >> , including drm.h makes no sense when building for Windows.
> >> >
> >> > Yeah I think it'd be cleanest if we can avoid this. If not I think the
> >> > right fix would be to split out the actually needed parts from drm.h
> >> > into a new header (still included by drm.h for backwards compat
> >> > reasons) which mesa can use. Since it looks like the problematic parts
> >> > are the legacy gunk, and not the new ioctl structures. Pulling out
> >> > drm_render.h for all the render stuff and mabe drm_vblank.h for the
> >> > vblank stuff (which would fit better in drm_mode.h but mistakes were
> >> > made, oops).
> >>
> >> If anything currently in drm.h is needed while building for Windows, it
> >> points to a broken abstraction somewhere in userspace. (Specifically,
> >> the Mesa Gallium/Vulkan winsys is supposed to abstract away platform
> >> details like these)
> >>
> >>
> >> --
> >> Earthling Michel Dänzer               |
> https://redhat.com
> >> Libre software enthusiast             |             Mesa and X developer
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
>
Michel Dänzer Dec. 3, 2020, 8:18 a.m. UTC | #9
On 2020-12-02 8:47 p.m., James Park wrote:
> 
> If we're trying to cut ties with the drm-uapi folder entirely, the stuff 
> ac_surface.c need includes the AMD_FMT_MOD stuff in drm_fourcc.h, 
> and AMDGPU_TILING_* under amdgpu_drm.h. Is there a better spot for these 
> definitions?

The Mesa src/amd/ code should use platform-neutral abstractions for 
these. This wasn't deemed necessary before, because nobody was trying to 
build these drivers for non-UNIX OSes. But now you are.
Pekka Paalanen Dec. 3, 2020, 9:05 a.m. UTC | #10
On Wed, 2 Dec 2020 23:25:58 +0100
Daniel Vetter <daniel@ffwll.ch> wrote:

> Also kinda disappointing that drm_fourcc.h includes drm.h and isn't
> standalone, but I guess that sailed (at least for linux).

Hi,

FWIW, libweston core needs drm_fourcc.h too, even if nothing would ever
use DRM or need libdrm otherwise. A stand-alone drm_fourcc.h
replacement would make sense, although distributing it through libdrm
would still make libweston require libdrm headers at build time, even
if it doesn't need libdrm.so. Not a big deal, and I don't know if
anyone actually builds libweston without DRM-backend.

Inventing yet another pixel format enumeration just because you don't
want to depend on a specific piece of other software really sucks, so
libweston went with DRM formats as the canonical enumeration. And
Wayland protocols use it too - Wayland clients rarely have any use for
libdrm otherwise.

Maybe a new header drm_formats.h that is what drm_fourcc.h should have
been, and make drm_fourcc.h include that to be backwards API compatible?


Thanks,
pq
Ville Syrjälä Dec. 3, 2020, 12:54 p.m. UTC | #11
On Wed, Dec 02, 2020 at 11:25:58PM +0100, Daniel Vetter wrote:
> On Wed, Dec 2, 2020 at 8:48 PM James Park <james.park@lagfreegames.com> wrote:
> >
> > I can avoid modifying drm.h by doing this to drm_fourcc.h:
> >
> > #ifdef _WIN32
> > #include <stdint.h>
> > typedef uint64_t __u64;
> > #else
> > #include "drm.h"
> > #endif
> >
> > And this to amdgpu_drm.h:
> >
> > #ifdef _WIN32
> > #include <stdint.h>
> > typedef int32_t  __s32;
> > typedef uint32_t __u32;
> > typedef uint64_t __u64;
> > #else
> > #include "drm.h"
> > #endif
> >
> > But now I'm touching two files under drm-uapi instead of one, and weirdly.
> >
> > If we're trying to cut ties with the drm-uapi folder entirely, the stuff ac_surface.c need includes the AMD_FMT_MOD stuff in drm_fourcc.h, and AMDGPU_TILING_* under amdgpu_drm.h. Is there a better spot for these definitions?
> 
> The drm_fourcc.h maybe makes some sense (I think in some places mesa
> uses these internally, and many drivers use the modifiers directly in
> the main driver). But the amdgpu header should be all ioctl stuff,
> which should be all entirely in the winsys only.
> 
> Also kinda disappointing that drm_fourcc.h includes drm.h and isn't
> standalone, but I guess that sailed (at least for linux).

Isn't the only thing it needs the __u32? I would think we could just
replace those with unsigned int (DRM_FORMAT_BIG_ENDIAN already assumes
int is 32bit it seems) and drop the drm.h.

Or are we're worried something already depends on getting drm.h via
just including drm_fourcc.h?
Simon Ser Dec. 3, 2020, 1:13 p.m. UTC | #12
On Thursday, December 3, 2020 1:54 PM, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:

> > The drm_fourcc.h maybe makes some sense (I think in some places mesa
> > uses these internally, and many drivers use the modifiers directly in
> > the main driver). But the amdgpu header should be all ioctl stuff,
> > which should be all entirely in the winsys only.
> > Also kinda disappointing that drm_fourcc.h includes drm.h and isn't
> > standalone, but I guess that sailed (at least for linux).
>
> Isn't the only thing it needs the __u32? I would think we could just
> replace those with unsigned int (DRM_FORMAT_BIG_ENDIAN already assumes
> int is 32bit it seems) and drop the drm.h.
>
> Or are we're worried something already depends on getting drm.h via
> just including drm_fourcc.h?

Yes, some user-space might only include drm_fourcc.h and use stuff
coming from drm.h.
Daniel Vetter Dec. 3, 2020, 2:52 p.m. UTC | #13
On Thu, Dec 3, 2020 at 9:18 AM Michel Dänzer <michel@daenzer.net> wrote:
>
> On 2020-12-02 8:47 p.m., James Park wrote:
> >
> > If we're trying to cut ties with the drm-uapi folder entirely, the stuff
> > ac_surface.c need includes the AMD_FMT_MOD stuff in drm_fourcc.h,
> > and AMDGPU_TILING_* under amdgpu_drm.h. Is there a better spot for these
> > definitions?
>
> The Mesa src/amd/ code should use platform-neutral abstractions for
> these. This wasn't deemed necessary before, because nobody was trying to
> build these drivers for non-UNIX OSes. But now you are.

I think that's a bit much busy work for not much gain. drm_fourcc.h is
even included as the official source of truth of some khr extensions,
making that header stand-alone and useable cross-platform sounds like
a good idea to me. Something like the below is imo perfectly fine:

diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
index ca48ed0e6bc1..0a121b3efb58 100644
--- a/include/uapi/drm/drm_fourcc.h
+++ b/include/uapi/drm/drm_fourcc.h
@@ -24,7 +24,11 @@
#ifndef DRM_FOURCC_H
#define DRM_FOURCC_H

+#ifndef DRM_FOURCC_STANDALONE_
+/* include the linux uapi types here */
+#else
#include "drm.h"
+#endif

#if defined(__cplusplus)
extern "C" {


Cheers, Daniel

>
>
> --
> Earthling Michel Dänzer               |               https://redhat.com
> Libre software enthusiast             |             Mesa and X developer
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
James Park Dec. 3, 2020, 6:55 p.m. UTC | #14
The trailing underscore for  DRM_FOURCC_STANDALONE_ isn't intentional,
right? Should I put all the integer types, or just the ones that are used
in that file?

Thanks,
James

On Thu, Dec 3, 2020 at 6:52 AM Daniel Vetter <daniel@ffwll.ch> wrote:

> On Thu, Dec 3, 2020 at 9:18 AM Michel Dänzer <michel@daenzer.net> wrote:
> >
> > On 2020-12-02 8:47 p.m., James Park wrote:
> > >
> > > If we're trying to cut ties with the drm-uapi folder entirely, the
> stuff
> > > ac_surface.c need includes the AMD_FMT_MOD stuff in drm_fourcc.h,
> > > and AMDGPU_TILING_* under amdgpu_drm.h. Is there a better spot for
> these
> > > definitions?
> >
> > The Mesa src/amd/ code should use platform-neutral abstractions for
> > these. This wasn't deemed necessary before, because nobody was trying to
> > build these drivers for non-UNIX OSes. But now you are.
>
> I think that's a bit much busy work for not much gain. drm_fourcc.h is
> even included as the official source of truth of some khr extensions,
> making that header stand-alone and useable cross-platform sounds like
> a good idea to me. Something like the below is imo perfectly fine:
>
> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> index ca48ed0e6bc1..0a121b3efb58 100644
> --- a/include/uapi/drm/drm_fourcc.h
> +++ b/include/uapi/drm/drm_fourcc.h
> @@ -24,7 +24,11 @@
> #ifndef DRM_FOURCC_H
> #define DRM_FOURCC_H
>
> +#ifndef DRM_FOURCC_STANDALONE_
> +/* include the linux uapi types here */
> +#else
> #include "drm.h"
> +#endif
>
> #if defined(__cplusplus)
> extern "C" {
>
>
> Cheers, Daniel
>
> >
> >
> > --
> > Earthling Michel Dänzer               |               https://redhat.com
> > Libre software enthusiast             |             Mesa and X developer
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
>
Daniel Vetter Dec. 3, 2020, 8:45 p.m. UTC | #15
On Thu, Dec 3, 2020 at 7:55 PM James Park <james.park@lagfreegames.com> wrote:
>
> The trailing underscore for  DRM_FOURCC_STANDALONE_ isn't intentional, right? Should I put all the integer types, or just the ones that are used in that file?

Yeah that trailing _ just slipped in. And I'd just do the types
already used. I don't think anything else than __u32 (for drm fourcc)
and __u64 (for drm modifier) is needed.
-Daniel

>
> Thanks,
> James
>
> On Thu, Dec 3, 2020 at 6:52 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>>
>> On Thu, Dec 3, 2020 at 9:18 AM Michel Dänzer <michel@daenzer.net> wrote:
>> >
>> > On 2020-12-02 8:47 p.m., James Park wrote:
>> > >
>> > > If we're trying to cut ties with the drm-uapi folder entirely, the stuff
>> > > ac_surface.c need includes the AMD_FMT_MOD stuff in drm_fourcc.h,
>> > > and AMDGPU_TILING_* under amdgpu_drm.h. Is there a better spot for these
>> > > definitions?
>> >
>> > The Mesa src/amd/ code should use platform-neutral abstractions for
>> > these. This wasn't deemed necessary before, because nobody was trying to
>> > build these drivers for non-UNIX OSes. But now you are.
>>
>> I think that's a bit much busy work for not much gain. drm_fourcc.h is
>> even included as the official source of truth of some khr extensions,
>> making that header stand-alone and useable cross-platform sounds like
>> a good idea to me. Something like the below is imo perfectly fine:
>>
>> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
>> index ca48ed0e6bc1..0a121b3efb58 100644
>> --- a/include/uapi/drm/drm_fourcc.h
>> +++ b/include/uapi/drm/drm_fourcc.h
>> @@ -24,7 +24,11 @@
>> #ifndef DRM_FOURCC_H
>> #define DRM_FOURCC_H
>>
>> +#ifndef DRM_FOURCC_STANDALONE_
>> +/* include the linux uapi types here */
>> +#else
>> #include "drm.h"
>> +#endif
>>
>> #if defined(__cplusplus)
>> extern "C" {
>>
>>
>> Cheers, Daniel
>>
>> >
>> >
>> > --
>> > Earthling Michel Dänzer               |               https://redhat.com
>> > Libre software enthusiast             |             Mesa and X developer
>> > _______________________________________________
>> > dri-devel mailing list
>> > dri-devel@lists.freedesktop.org
>> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>>
>>
>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch
James Park Dec. 4, 2020, 4:53 a.m. UTC | #16
Add DRM_FOURCC_STANDALONE guard to skip drm.h dependency.

This will allow Mesa to port code to Windows more easily.

Signed-off-by: James Park <jpark37@lagfreegames.com>

James Park (1):
  drm: Allow drm_fourcc.h without including drm.h

 include/uapi/drm/drm_fourcc.h | 6 ++++++
 1 file changed, 6 insertions(+)
Pekka Paalanen Dec. 4, 2020, 8:11 a.m. UTC | #17
On Thu, 3 Dec 2020 21:45:14 +0100
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Thu, Dec 3, 2020 at 7:55 PM James Park <james.park@lagfreegames.com> wrote:
> >
> > The trailing underscore for  DRM_FOURCC_STANDALONE_ isn't
> > intentional, right? Should I put all the integer types, or just the
> > ones that are used in that file?  
> 
> Yeah that trailing _ just slipped in. And I'd just do the types
> already used. I don't think anything else than __u32 (for drm fourcc)
> and __u64 (for drm modifier) is needed.

Hi,

can that create conflicts if userspace first includes drm_fourcc.h and
then drm.h?

I would find it natural to userspace have generic headers including
drm_fourcc.h and then DRM-specific C-files including drm.h as well
(through libdrm headers). I think Weston might already do this.

The generic userspace (weston) header would obviously #define
DRM_FOURCC_STANDALONE, because it is used by non-DRM C-files as well.


Thanks,
pq
Daniel Vetter Dec. 4, 2020, 3:58 p.m. UTC | #18
On Fri, Dec 4, 2020 at 9:12 AM Pekka Paalanen <ppaalanen@gmail.com> wrote:
>
> On Thu, 3 Dec 2020 21:45:14 +0100
> Daniel Vetter <daniel@ffwll.ch> wrote:
>
> > On Thu, Dec 3, 2020 at 7:55 PM James Park <james.park@lagfreegames.com> wrote:
> > >
> > > The trailing underscore for  DRM_FOURCC_STANDALONE_ isn't
> > > intentional, right? Should I put all the integer types, or just the
> > > ones that are used in that file?
> >
> > Yeah that trailing _ just slipped in. And I'd just do the types
> > already used. I don't think anything else than __u32 (for drm fourcc)
> > and __u64 (for drm modifier) is needed.
>
> Hi,
>
> can that create conflicts if userspace first includes drm_fourcc.h and
> then drm.h?
>
> I would find it natural to userspace have generic headers including
> drm_fourcc.h and then DRM-specific C-files including drm.h as well
> (through libdrm headers). I think Weston might already do this.
>
> The generic userspace (weston) header would obviously #define
> DRM_FOURCC_STANDALONE, because it is used by non-DRM C-files as well.

Hm yes that would break. I guess we could just include the linux types
header for this. And I guess on windows you'd need to have that from
somewhere. Or we just require that users of the standalone header pull
the right header or defines in first?
-Daniel
James Park Dec. 4, 2020, 7:07 p.m. UTC | #19
I could adjust the block to look like this:

#ifdef DRM_FOURCC_STANDALONE
#if defined(__linux__)
#include <linux/types.h>
#else
#include <stdint.h>
typedef uint32_t __u32;
typedef uint64_t __u64;
#endif
#else
#include "drm.h"
#endif

Alternatively, I could create a new common header to be included from both
drm.h and drm_fourcc.h, drm_base_types.h or something like that:

#ifdef DRM_FOURCC_STANDALONE
#include "drm_base_types.h"
#else
#include "drm.h"
#endif

On Fri, Dec 4, 2020 at 7:58 AM Daniel Vetter <daniel@ffwll.ch> wrote:

> On Fri, Dec 4, 2020 at 9:12 AM Pekka Paalanen <ppaalanen@gmail.com> wrote:
> >
> > On Thu, 3 Dec 2020 21:45:14 +0100
> > Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > > On Thu, Dec 3, 2020 at 7:55 PM James Park <james.park@lagfreegames.com>
> wrote:
> > > >
> > > > The trailing underscore for  DRM_FOURCC_STANDALONE_ isn't
> > > > intentional, right? Should I put all the integer types, or just the
> > > > ones that are used in that file?
> > >
> > > Yeah that trailing _ just slipped in. And I'd just do the types
> > > already used. I don't think anything else than __u32 (for drm fourcc)
> > > and __u64 (for drm modifier) is needed.
> >
> > Hi,
> >
> > can that create conflicts if userspace first includes drm_fourcc.h and
> > then drm.h?
> >
> > I would find it natural to userspace have generic headers including
> > drm_fourcc.h and then DRM-specific C-files including drm.h as well
> > (through libdrm headers). I think Weston might already do this.
> >
> > The generic userspace (weston) header would obviously #define
> > DRM_FOURCC_STANDALONE, because it is used by non-DRM C-files as well.
>
> Hm yes that would break. I guess we could just include the linux types
> header for this. And I guess on windows you'd need to have that from
> somewhere. Or we just require that users of the standalone header pull
> the right header or defines in first?
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
>
James Park Dec. 6, 2020, 12:39 a.m. UTC | #20
Create drm_basic_types.h to define types previously defined by drm.h.

Use DRM_FOURCC_STANDALONE to include drm_fourcc.h, replacing drm.h
dependency with drm_basic_types.h.

This will allow Mesa to port code to Windows more easily.

Signed-off-by: James Park <jpark37@lagfreegames.com>

James Park (1):
  drm: drm_basic_types.h, DRM_FOURCC_STANDALONE

 include/uapi/drm/drm.h             | 14 ++--------
 include/uapi/drm/drm_basic_types.h | 52 ++++++++++++++++++++++++++++++++++++++
 include/uapi/drm/drm_fourcc.h      |  4 +++
 3 files changed, 58 insertions(+), 12 deletions(-)
 create mode 100644 include/uapi/drm/drm_basic_types.h
Pekka Paalanen Dec. 7, 2020, 8:51 a.m. UTC | #21
On Fri, 4 Dec 2020 11:07:41 -0800
James Park <james.park@lagfreegames.com> wrote:

> I could adjust the block to look like this:
> 
> #ifdef DRM_FOURCC_STANDALONE
> #if defined(__linux__)
> #include <linux/types.h>
> #else
> #include <stdint.h>
> typedef uint32_t __u32;
> typedef uint64_t __u64;
> #endif
> #else
> #include "drm.h"
> #endif
> 
> Alternatively, I could create a new common header to be included from both
> drm.h and drm_fourcc.h, drm_base_types.h or something like that:
> 
> #ifdef DRM_FOURCC_STANDALONE
> #include "drm_base_types.h"
> #else
> #include "drm.h"
> #endif

Hi,

my point is, any solution relying on DRM_FOURCC_STANDALONE will fail
sometimes, because there is no reason why userspace would *not* #define
DRM_FOURCC_STANDALONE. Hence, #ifdef DRM_FOURCC_STANDALONE is
completely moot, you have to make the headers work in any include
order when DRM_FOURCC_STANDALONE is defined anyway.


Thanks.
pq

> On Fri, Dec 4, 2020 at 7:58 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > On Fri, Dec 4, 2020 at 9:12 AM Pekka Paalanen <ppaalanen@gmail.com> wrote:  
> > >
> > > On Thu, 3 Dec 2020 21:45:14 +0100
> > > Daniel Vetter <daniel@ffwll.ch> wrote:
> > >  
> > > > On Thu, Dec 3, 2020 at 7:55 PM James Park <james.park@lagfreegames.com>  
> > wrote:  
> > > > >
> > > > > The trailing underscore for  DRM_FOURCC_STANDALONE_ isn't
> > > > > intentional, right? Should I put all the integer types, or just the
> > > > > ones that are used in that file?  
> > > >
> > > > Yeah that trailing _ just slipped in. And I'd just do the types
> > > > already used. I don't think anything else than __u32 (for drm fourcc)
> > > > and __u64 (for drm modifier) is needed.  
> > >
> > > Hi,
> > >
> > > can that create conflicts if userspace first includes drm_fourcc.h and
> > > then drm.h?
> > >
> > > I would find it natural to userspace have generic headers including
> > > drm_fourcc.h and then DRM-specific C-files including drm.h as well
> > > (through libdrm headers). I think Weston might already do this.
> > >
> > > The generic userspace (weston) header would obviously #define
> > > DRM_FOURCC_STANDALONE, because it is used by non-DRM C-files as well.  
> >
> > Hm yes that would break. I guess we could just include the linux types
> > header for this. And I guess on windows you'd need to have that from
> > somewhere. Or we just require that users of the standalone header pull
> > the right header or defines in first?
> > -Daniel
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> >
James Park Dec. 7, 2020, 9:08 a.m. UTC | #22
I'm not completely sure what you're saying, but doesn't drm_base_types.h
(now drm_basic_types.h) make things robust to header order? The patch is in
another email. You can also see it here:
https://github.com/jpark37/linux/commit/0cc8ae750bfd9eab7e31c7de6aa84f24682f4f18

Thanks,
James

On Mon, Dec 7, 2020 at 12:51 AM Pekka Paalanen <ppaalanen@gmail.com> wrote:

> On Fri, 4 Dec 2020 11:07:41 -0800
> James Park <james.park@lagfreegames.com> wrote:
>
> > I could adjust the block to look like this:
> >
> > #ifdef DRM_FOURCC_STANDALONE
> > #if defined(__linux__)
> > #include <linux/types.h>
> > #else
> > #include <stdint.h>
> > typedef uint32_t __u32;
> > typedef uint64_t __u64;
> > #endif
> > #else
> > #include "drm.h"
> > #endif
> >
> > Alternatively, I could create a new common header to be included from
> both
> > drm.h and drm_fourcc.h, drm_base_types.h or something like that:
> >
> > #ifdef DRM_FOURCC_STANDALONE
> > #include "drm_base_types.h"
> > #else
> > #include "drm.h"
> > #endif
>
> Hi,
>
> my point is, any solution relying on DRM_FOURCC_STANDALONE will fail
> sometimes, because there is no reason why userspace would *not* #define
> DRM_FOURCC_STANDALONE. Hence, #ifdef DRM_FOURCC_STANDALONE is
> completely moot, you have to make the headers work in any include
> order when DRM_FOURCC_STANDALONE is defined anyway.
>
>
> Thanks.
> pq
>
> > On Fri, Dec 4, 2020 at 7:58 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > > On Fri, Dec 4, 2020 at 9:12 AM Pekka Paalanen <ppaalanen@gmail.com>
> wrote:
> > > >
> > > > On Thu, 3 Dec 2020 21:45:14 +0100
> > > > Daniel Vetter <daniel@ffwll.ch> wrote:
> > > >
> > > > > On Thu, Dec 3, 2020 at 7:55 PM James Park <
> james.park@lagfreegames.com>
> > > wrote:
> > > > > >
> > > > > > The trailing underscore for  DRM_FOURCC_STANDALONE_ isn't
> > > > > > intentional, right? Should I put all the integer types, or just
> the
> > > > > > ones that are used in that file?
> > > > >
> > > > > Yeah that trailing _ just slipped in. And I'd just do the types
> > > > > already used. I don't think anything else than __u32 (for drm
> fourcc)
> > > > > and __u64 (for drm modifier) is needed.
> > > >
> > > > Hi,
> > > >
> > > > can that create conflicts if userspace first includes drm_fourcc.h
> and
> > > > then drm.h?
> > > >
> > > > I would find it natural to userspace have generic headers including
> > > > drm_fourcc.h and then DRM-specific C-files including drm.h as well
> > > > (through libdrm headers). I think Weston might already do this.
> > > >
> > > > The generic userspace (weston) header would obviously #define
> > > > DRM_FOURCC_STANDALONE, because it is used by non-DRM C-files as
> well.
> > >
> > > Hm yes that would break. I guess we could just include the linux types
> > > header for this. And I guess on windows you'd need to have that from
> > > somewhere. Or we just require that users of the standalone header pull
> > > the right header or defines in first?
> > > -Daniel
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> > >
>
>
Simon Ser Dec. 7, 2020, 9:48 a.m. UTC | #23
On Monday, December 7th, 2020 at 9:57 AM, James Park <james.park@lagfreegames.com> wrote:

> I could adjust the block to look like this:
>
> #ifdef DRM_FOURCC_STANDALONE
> #if defined(__linux__)
> #include <linux/types.h>
> #else
> #include <stdint.h>
> typedef uint32_t __u32;
> typedef uint64_t __u64;
> #endif
> #else
> #include "drm.h"
> #endif

This approach still breaks on BSDs when DRM_FOURCC_STANDALONE is defined and
drm.h is included afterwards.
James Park Dec. 7, 2020, 10 a.m. UTC | #24
Not to make too big a deal of it, but the idea was that if you went out of
your way to define DRM_FOURCC_STANDALONE in your code base, that you would
also go through the pain of removing drm.h includes elsewhere. It's too
annoying of an implication to document/communicate, so I'm happier with the
other DRM_FOURCC_STANDALONE solution that pulls the basic types into a
common header.

Thanks,
James

On Mon, Dec 7, 2020 at 1:49 AM Simon Ser <contact@emersion.fr> wrote:

> On Monday, December 7th, 2020 at 9:57 AM, James Park <
> james.park@lagfreegames.com> wrote:
>
> > I could adjust the block to look like this:
> >
> > #ifdef DRM_FOURCC_STANDALONE
> > #if defined(__linux__)
> > #include <linux/types.h>
> > #else
> > #include <stdint.h>
> > typedef uint32_t __u32;
> > typedef uint64_t __u64;
> > #endif
> > #else
> > #include "drm.h"
> > #endif
>
> This approach still breaks on BSDs when DRM_FOURCC_STANDALONE is defined
> and
> drm.h is included afterwards.
>
Simon Ser Dec. 7, 2020, 10:02 a.m. UTC | #25
On Monday, December 7th, 2020 at 11:00 AM, James Park <james.park@lagfreegames.com> wrote:

> Not to make too big a deal of it, but the idea was that if you went
> out of your way to define DRM_FOURCC_STANDALONE in your code base,
> that you would also go through the pain of removing drm.h includes
> elsewhere. It's too annoying of an implication to
> document/communicate, so I'm happier with the other
> DRM_FOURCC_STANDALONE solution that pulls the basic types into a
> common header.

In my compositors I'd like to globally define DRM_FOURCC_STANDALONE
(to make sure I don't miss any #define) but I still may include drm.h
in the same files as well.
Pekka Paalanen Dec. 7, 2020, 10:35 a.m. UTC | #26
On Mon, 7 Dec 2020 01:08:58 -0800
James Park <james.park@lagfreegames.com> wrote:

> I'm not completely sure what you're saying, but doesn't drm_base_types.h
> (now drm_basic_types.h) make things robust to header order? The patch is in
> another email. You can also see it here:
> https://github.com/jpark37/linux/commit/0cc8ae750bfd9eab7e31c7de6aa84f24682f4f18

If that is robust (I don't know if it is, I haven't checked), then why
do you have #ifdef DRM_FOURCC_STANDALONE in it at all?

Like Simon said:

On Mon, 07 Dec 2020 10:02:36 +0000
Simon Ser <contact@emersion.fr> wrote:

> In my compositors I'd like to globally define DRM_FOURCC_STANDALONE
> (to make sure I don't miss any #define) but I still may include drm.h
> in the same files as well.

If any project #defines it globally, then what good does checking for
it do? Why not assume that everyone will always want what
DRM_FOURCC_STANDALONE would bring?


Thanks,
pq

> 
> Thanks,
> James
> 
> On Mon, Dec 7, 2020 at 12:51 AM Pekka Paalanen <ppaalanen@gmail.com> wrote:
> 
> > On Fri, 4 Dec 2020 11:07:41 -0800
> > James Park <james.park@lagfreegames.com> wrote:
> >  
> > > I could adjust the block to look like this:
> > >
> > > #ifdef DRM_FOURCC_STANDALONE
> > > #if defined(__linux__)
> > > #include <linux/types.h>
> > > #else
> > > #include <stdint.h>
> > > typedef uint32_t __u32;
> > > typedef uint64_t __u64;
> > > #endif
> > > #else
> > > #include "drm.h"
> > > #endif
> > >
> > > Alternatively, I could create a new common header to be included from  
> > both  
> > > drm.h and drm_fourcc.h, drm_base_types.h or something like that:
> > >
> > > #ifdef DRM_FOURCC_STANDALONE
> > > #include "drm_base_types.h"
> > > #else
> > > #include "drm.h"
> > > #endif  
> >
> > Hi,
> >
> > my point is, any solution relying on DRM_FOURCC_STANDALONE will fail
> > sometimes, because there is no reason why userspace would *not* #define
> > DRM_FOURCC_STANDALONE. Hence, #ifdef DRM_FOURCC_STANDALONE is
> > completely moot, you have to make the headers work in any include
> > order when DRM_FOURCC_STANDALONE is defined anyway.
> >
> >
> > Thanks.
> > pq
> >  
> > > On Fri, Dec 4, 2020 at 7:58 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > >  
> > > > On Fri, Dec 4, 2020 at 9:12 AM Pekka Paalanen <ppaalanen@gmail.com>  
> > wrote:  
> > > > >
> > > > > On Thu, 3 Dec 2020 21:45:14 +0100
> > > > > Daniel Vetter <daniel@ffwll.ch> wrote:
> > > > >  
> > > > > > On Thu, Dec 3, 2020 at 7:55 PM James Park <  
> > james.park@lagfreegames.com>  
> > > > wrote:  
> > > > > > >
> > > > > > > The trailing underscore for  DRM_FOURCC_STANDALONE_ isn't
> > > > > > > intentional, right? Should I put all the integer types, or just  
> > the  
> > > > > > > ones that are used in that file?  
> > > > > >
> > > > > > Yeah that trailing _ just slipped in. And I'd just do the types
> > > > > > already used. I don't think anything else than __u32 (for drm  
> > fourcc)  
> > > > > > and __u64 (for drm modifier) is needed.  
> > > > >
> > > > > Hi,
> > > > >
> > > > > can that create conflicts if userspace first includes drm_fourcc.h  
> > and  
> > > > > then drm.h?
> > > > >
> > > > > I would find it natural to userspace have generic headers including
> > > > > drm_fourcc.h and then DRM-specific C-files including drm.h as well
> > > > > (through libdrm headers). I think Weston might already do this.
> > > > >
> > > > > The generic userspace (weston) header would obviously #define
> > > > > DRM_FOURCC_STANDALONE, because it is used by non-DRM C-files as  
> > well.  
> > > >
> > > > Hm yes that would break. I guess we could just include the linux types
> > > > header for this. And I guess on windows you'd need to have that from
> > > > somewhere. Or we just require that users of the standalone header pull
> > > > the right header or defines in first?
> > > > -Daniel
> > > > --
> > > > Daniel Vetter
> > > > Software Engineer, Intel Corporation
> > > > http://blog.ffwll.ch
> > > >  
> >
> >
Pekka Paalanen Dec. 7, 2020, 10:44 a.m. UTC | #27
On Mon, 7 Dec 2020 12:35:14 +0200
Pekka Paalanen <ppaalanen@gmail.com> wrote:

> On Mon, 7 Dec 2020 01:08:58 -0800
> James Park <james.park@lagfreegames.com> wrote:
> 
> > I'm not completely sure what you're saying, but doesn't drm_base_types.h
> > (now drm_basic_types.h) make things robust to header order? The patch is in
> > another email. You can also see it here:
> > https://github.com/jpark37/linux/commit/0cc8ae750bfd9eab7e31c7de6aa84f24682f4f18  
> 
> If that is robust (I don't know if it is, I haven't checked), then why
> do you have #ifdef DRM_FOURCC_STANDALONE in it at all?
> 
> Like Simon said:
> 
> On Mon, 07 Dec 2020 10:02:36 +0000
> Simon Ser <contact@emersion.fr> wrote:
> 
> > In my compositors I'd like to globally define DRM_FOURCC_STANDALONE
> > (to make sure I don't miss any #define) but I still may include drm.h
> > in the same files as well.  
> 
> If any project #defines it globally, then what good does checking for
> it do? Why not assume that everyone will always want what
> DRM_FOURCC_STANDALONE would bring?

Sorry! Now I got it.

DRM_FOURCC_STANDALONE is a promise that the user is not relying on
drm_foucc.h to pull in drm.h. Nothing else. That's fine.

But then, the code in the header should be literally

#ifndef DRM_FOURCC_STANDALONE
#include "drm.h"
#endif

without an #else branch.


Thanks,
pq
Simon Ser Dec. 7, 2020, 10:47 a.m. UTC | #28
On Monday, December 7th, 2020 at 11:44 AM, Pekka Paalanen <ppaalanen@gmail.com> wrote:

> But then, the code in the header should be literally
>
> #ifndef DRM_FOURCC_STANDALONE
> #include "drm.h"
> #endif
>
> without an #else branch.

As long as there is a #include "drm_basic_types.h" right before
(drm_fourcc.h needs __u32 and __u64), I believe this can work too
indeed.
James Park Dec. 7, 2020, 10:49 a.m. UTC | #29
That would work, but that's kind of an annoying requirement. I would prefer
the header to be self-sufficient.

Thanks,
James

On Mon, Dec 7, 2020 at 2:47 AM Simon Ser <contact@emersion.fr> wrote:

> On Monday, December 7th, 2020 at 11:44 AM, Pekka Paalanen <
> ppaalanen@gmail.com> wrote:
>
> > But then, the code in the header should be literally
> >
> > #ifndef DRM_FOURCC_STANDALONE
> > #include "drm.h"
> > #endif
> >
> > without an #else branch.
>
> As long as there is a #include "drm_basic_types.h" right before
> (drm_fourcc.h needs __u32 and __u64), I believe this can work too
> indeed.
>
Simon Ser Dec. 7, 2020, 10:53 a.m. UTC | #30
On Monday, December 7th, 2020 at 11:49 AM, James Park <james.park@lagfreegames.com> wrote:

> That would work, but that's kind of an annoying requirement. I would
> prefer the header to be self-sufficient.

I don't want to make it more confusing than before, but here Pekka (I
think) suggests to replace this:

diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
index 82f3278..5eb07a5 100644
--- a/include/uapi/drm/drm_fourcc.h
+++ b/include/uapi/drm/drm_fourcc.h
@@ -24,7 +24,11 @@
 #ifndef DRM_FOURCC_H
 #define DRM_FOURCC_H

+#ifdef DRM_FOURCC_STANDALONE
+#include "drm_basic_types.h"
+#else
 #include "drm.h"
+#endif

 #if defined(__cplusplus)
 extern "C" {

With this:

diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
index 82f3278..5eb07a5 100644
--- a/include/uapi/drm/drm_fourcc.h
+++ b/include/uapi/drm/drm_fourcc.h
@@ -24,7 +24,11 @@
 #ifndef DRM_FOURCC_H
 #define DRM_FOURCC_H

+#include "drm_basic_types.h"
+
+#ifndef DRM_FOURCC_STANDALONE
 #include "drm.h"
+#endif

 #if defined(__cplusplus)
 extern "C" {

That wouldn't change whether the header is self-sufficient or not,
would it?
James Park Dec. 7, 2020, 11:01 a.m. UTC | #31
Oh, I thought you meant:

#include "drm_basic_types.h"
#include "drm_fourcc.h"

Yours should work too. I don't have a preference vs. what I already have,
so if no one says anything, I can switch over to yours.

Thanks,
James

On Mon, Dec 7, 2020 at 2:53 AM Simon Ser <contact@emersion.fr> wrote:

> On Monday, December 7th, 2020 at 11:49 AM, James Park <
> james.park@lagfreegames.com> wrote:
>
> > That would work, but that's kind of an annoying requirement. I would
> > prefer the header to be self-sufficient.
>
> I don't want to make it more confusing than before, but here Pekka (I
> think) suggests to replace this:
>
> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> index 82f3278..5eb07a5 100644
> --- a/include/uapi/drm/drm_fourcc.h
> +++ b/include/uapi/drm/drm_fourcc.h
> @@ -24,7 +24,11 @@
>  #ifndef DRM_FOURCC_H
>  #define DRM_FOURCC_H
>
> +#ifdef DRM_FOURCC_STANDALONE
> +#include "drm_basic_types.h"
> +#else
>  #include "drm.h"
> +#endif
>
>  #if defined(__cplusplus)
>  extern "C" {
>
> With this:
>
> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> index 82f3278..5eb07a5 100644
> --- a/include/uapi/drm/drm_fourcc.h
> +++ b/include/uapi/drm/drm_fourcc.h
> @@ -24,7 +24,11 @@
>  #ifndef DRM_FOURCC_H
>  #define DRM_FOURCC_H
>
> +#include "drm_basic_types.h"
> +
> +#ifndef DRM_FOURCC_STANDALONE
>  #include "drm.h"
> +#endif
>
>  #if defined(__cplusplus)
>  extern "C" {
>
> That wouldn't change whether the header is self-sufficient or not,
> would it?
>
Pekka Paalanen Dec. 7, 2020, 11:14 a.m. UTC | #32
On Mon, 07 Dec 2020 10:53:49 +0000
Simon Ser <contact@emersion.fr> wrote:

> On Monday, December 7th, 2020 at 11:49 AM, James Park <james.park@lagfreegames.com> wrote:
> 
> > That would work, but that's kind of an annoying requirement. I would
> > prefer the header to be self-sufficient.  
> 
> I don't want to make it more confusing than before, but here Pekka (I
> think) suggests to replace this:
> 
> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> index 82f3278..5eb07a5 100644
> --- a/include/uapi/drm/drm_fourcc.h
> +++ b/include/uapi/drm/drm_fourcc.h
> @@ -24,7 +24,11 @@
>  #ifndef DRM_FOURCC_H
>  #define DRM_FOURCC_H
> 
> +#ifdef DRM_FOURCC_STANDALONE
> +#include "drm_basic_types.h"
> +#else
>  #include "drm.h"
> +#endif
> 
>  #if defined(__cplusplus)
>  extern "C" {
> 
> With this:
> 
> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> index 82f3278..5eb07a5 100644
> --- a/include/uapi/drm/drm_fourcc.h
> +++ b/include/uapi/drm/drm_fourcc.h
> @@ -24,7 +24,11 @@
>  #ifndef DRM_FOURCC_H
>  #define DRM_FOURCC_H
> 
> +#include "drm_basic_types.h"
> +
> +#ifndef DRM_FOURCC_STANDALONE
>  #include "drm.h"
> +#endif
> 
>  #if defined(__cplusplus)
>  extern "C" {
> 
> That wouldn't change whether the header is self-sufficient or not,
> would it?

Exactly this.

This communicates properly that DRM_FOURCC_STANDALONE only affects
whether drm.h gets pulled in or not, and there are no other effects.

This also makes testing better: when you unconditionally include
drm_basic_types.h, you are more likely to catch breakage there.

For functionality, it makes no difference. Whether userspace does

#include "drm.h"
#define DRM_FOURCC_STANDALONE
#include "drm_fourcc.h"

or

#define DRM_FOURCC_STANDALONE
#include "drm_fourcc.h"
#include "drm.h"

the result must always be good.


Thanks,
pq
James Park Dec. 8, 2020, 1:08 a.m. UTC | #33
I updated the patch earlier today incorporating the suggestions. I also had
to bring back "#include <linux/types.h>" to drm.h because there's some
sanity check that fails, as if it doesn't scan past the first level of
#includes..

- James

On Mon, Dec 7, 2020 at 3:14 AM Pekka Paalanen <ppaalanen@gmail.com> wrote:

> On Mon, 07 Dec 2020 10:53:49 +0000
> Simon Ser <contact@emersion.fr> wrote:
>
> > On Monday, December 7th, 2020 at 11:49 AM, James Park <
> james.park@lagfreegames.com> wrote:
> >
> > > That would work, but that's kind of an annoying requirement. I would
> > > prefer the header to be self-sufficient.
> >
> > I don't want to make it more confusing than before, but here Pekka (I
> > think) suggests to replace this:
> >
> > diff --git a/include/uapi/drm/drm_fourcc.h
> b/include/uapi/drm/drm_fourcc.h
> > index 82f3278..5eb07a5 100644
> > --- a/include/uapi/drm/drm_fourcc.h
> > +++ b/include/uapi/drm/drm_fourcc.h
> > @@ -24,7 +24,11 @@
> >  #ifndef DRM_FOURCC_H
> >  #define DRM_FOURCC_H
> >
> > +#ifdef DRM_FOURCC_STANDALONE
> > +#include "drm_basic_types.h"
> > +#else
> >  #include "drm.h"
> > +#endif
> >
> >  #if defined(__cplusplus)
> >  extern "C" {
> >
> > With this:
> >
> > diff --git a/include/uapi/drm/drm_fourcc.h
> b/include/uapi/drm/drm_fourcc.h
> > index 82f3278..5eb07a5 100644
> > --- a/include/uapi/drm/drm_fourcc.h
> > +++ b/include/uapi/drm/drm_fourcc.h
> > @@ -24,7 +24,11 @@
> >  #ifndef DRM_FOURCC_H
> >  #define DRM_FOURCC_H
> >
> > +#include "drm_basic_types.h"
> > +
> > +#ifndef DRM_FOURCC_STANDALONE
> >  #include "drm.h"
> > +#endif
> >
> >  #if defined(__cplusplus)
> >  extern "C" {
> >
> > That wouldn't change whether the header is self-sufficient or not,
> > would it?
>
> Exactly this.
>
> This communicates properly that DRM_FOURCC_STANDALONE only affects
> whether drm.h gets pulled in or not, and there are no other effects.
>
> This also makes testing better: when you unconditionally include
> drm_basic_types.h, you are more likely to catch breakage there.
>
> For functionality, it makes no difference. Whether userspace does
>
> #include "drm.h"
> #define DRM_FOURCC_STANDALONE
> #include "drm_fourcc.h"
>
> or
>
> #define DRM_FOURCC_STANDALONE
> #include "drm_fourcc.h"
> #include "drm.h"
>
> the result must always be good.
>
>
> Thanks,
> pq
>
diff mbox series

Patch

diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index 808b48a..53dc3c9 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -48,10 +48,9 @@  typedef unsigned int drm_handle_t;
 #include <asm/ioctl.h>
 typedef unsigned int drm_handle_t;
 
-#else /* One of the BSDs */
+#else
 
 #include <stdint.h>
-#include <sys/ioccom.h>
 #include <sys/types.h>
 typedef int8_t   __s8;
 typedef uint8_t  __u8;
@@ -62,10 +61,16 @@  typedef uint32_t __u32;
 typedef int64_t  __s64;
 typedef uint64_t __u64;
 typedef size_t   __kernel_size_t;
+
+#ifndef _WIN32 /* One of the BSDs */
+
+#include <sys/ioccom.h>
 typedef unsigned long drm_handle_t;
 
 #endif
 
+#endif
+
 #if defined(__cplusplus)
 extern "C" {
 #endif
@@ -128,7 +133,7 @@  struct drm_tex_region {
  * other data stored in the same cache line.
  */
 struct drm_hw_lock {
-	__volatile__ unsigned int lock;		/**< lock variable */
+	volatile unsigned int lock;		/**< lock variable */
 	char padding[60];			/**< Pad to cache line */
 };