| 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 |
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/
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(-)
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.
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
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)
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 >
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
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 >
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.
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
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?
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.
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
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 >
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
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(+)
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
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
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 >
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
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 > >
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 > > > > >
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.
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. >
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.
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 > > > > > > > >
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
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.
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. >
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?
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? >
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
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 >
I'd really like this for Mesa so we can pull drm_fourcc.h into common WSI code. Why has it stalled? --Jason On Tue, Dec 8, 2020 at 2:33 AM James Park <james.park@lagfreegames.com> wrote: > > 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 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
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 */ };