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.
James Park (1): drm: Fix drm.h uapi header for Windows
include/uapi/drm/drm.h | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
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(-)
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 */ };
Can you add a Signed-off-by line to your commit message? This means you agree to the Developer Certificate of Origin [1].
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(-)
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 --- include/uapi/drm/drm.h | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
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 */ };
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/648e9281824ddc943c3ea6b34d6d6c154717...
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 |
Libre software enthusiast | Mesa and X developer
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
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 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 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(+)
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 --- include/uapi/drm/drm_fourcc.h | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h index 82f3278..159a9d0 100644 --- a/include/uapi/drm/drm_fourcc.h +++ b/include/uapi/drm/drm_fourcc.h @@ -24,7 +24,13 @@ #ifndef DRM_FOURCC_H #define DRM_FOURCC_H
+#ifdef DRM_FOURCC_STANDALONE +#include <stdint.h> +typedef uint32_t __u32; +typedef uint64_t __u64; +#else #include "drm.h" +#endif
#if defined(__cplusplus) extern "C" {
On Friday, December 4, 2020 5:53 AM, James Park jpark37@lagfreegames.com wrote:
+#ifdef DRM_FOURCC_STANDALONE +#include <stdint.h>
+typedef uint32_t __u32; +typedef uint64_t __u64; +#else #include "drm.h" +#endif
C11 allows duplicate typedefs, but older versions of the standard don't AFAIK. If this is a concern, a solution would be to guard the typedefs.
The typedefs might also conflict on Linux if DRM_FOURCC_STANDALONE is enabled with whatever LInux declared __u32/__u64 as, but I think the implication is that once DRM_FOURCC_STANDALONE has been declared, that's kind of a promise not to include drm.h.
I'm fine with this, but I'm not married to it if someone has a problem where they want to define DRM_FOURCC_STANDALONE, but also can't avoid including drm.h for some reason.
On Fri, Dec 4, 2020 at 12:53 AM Simon Ser contact@emersion.fr wrote:
On Friday, December 4, 2020 5:53 AM, James Park jpark37@lagfreegames.com wrote:
+#ifdef DRM_FOURCC_STANDALONE +#include <stdint.h>
+typedef uint32_t __u32; +typedef uint64_t __u64; +#else #include "drm.h" +#endif
C11 allows duplicate typedefs, but older versions of the standard don't AFAIK. If this is a concern, a solution would be to guard the typedefs.
I suppose I should do this to avoid fighting with <linux/types.h>
#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
I'll wait for more feedback before updating the patch though.
On Fri, Dec 4, 2020 at 1:47 AM James Park james.park@lagfreegames.com wrote:
The typedefs might also conflict on Linux if DRM_FOURCC_STANDALONE is enabled with whatever LInux declared __u32/__u64 as, but I think the implication is that once DRM_FOURCC_STANDALONE has been declared, that's kind of a promise not to include drm.h.
I'm fine with this, but I'm not married to it if someone has a problem where they want to define DRM_FOURCC_STANDALONE, but also can't avoid including drm.h for some reason.
On Fri, Dec 4, 2020 at 12:53 AM Simon Ser contact@emersion.fr wrote:
On Friday, December 4, 2020 5:53 AM, James Park jpark37@lagfreegames.com wrote:
+#ifdef DRM_FOURCC_STANDALONE +#include <stdint.h>
+typedef uint32_t __u32; +typedef uint64_t __u64; +#else #include "drm.h" +#endif
C11 allows duplicate typedefs, but older versions of the standard don't AFAIK. If this is a concern, a solution would be to guard the typedefs.
Hi James,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on drm-intel/for-linux-next] [also build test WARNING on drm-exynos/exynos-drm-next tegra-drm/drm/tegra/for-next drm-tip/drm-tip linus/master drm/drm-next v5.10-rc6 next-20201204] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/James-Park/drm-Allow-drm_fourcc-h-w... base: git://anongit.freedesktop.org/drm-intel for-linux-next config: x86_64-randconfig-c002-20201204 (attached as .config) compiler: gcc-9 (Debian 9.3.0-15) 9.3.0 reproduce (this is a W=1 build): # https://github.com/0day-ci/linux/commit/444ac999e27a36307f741eb0ef60d630b0b8... git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review James-Park/drm-Allow-drm_fourcc-h-without-including-drm-h/20201204-163753 git checkout 444ac999e27a36307f741eb0ef60d630b0b8946a # save the attached .config to linux build tree make W=1 ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot lkp@intel.com
All warnings (new ones prefixed by >>):
usr/include/drm/drm_fourcc.h:29: found __[us]{8,16,32,64} type without #include <linux/types.h>
--- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi James,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on drm-intel/for-linux-next] [also build test WARNING on drm-exynos/exynos-drm-next tegra-drm/drm/tegra/for-next drm-tip/drm-tip linus/master v5.10-rc6 next-20201204] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/James-Park/drm-Allow-drm_fourcc-h-w... base: git://anongit.freedesktop.org/drm-intel for-linux-next config: x86_64-randconfig-a014-20201204 (attached as .config) compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 32c501dd88b62787d3a5ffda7aabcf4650dbe3cd) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # install x86_64 cross compiling tool for clang build # apt-get install binutils-x86-64-linux-gnu # https://github.com/0day-ci/linux/commit/444ac999e27a36307f741eb0ef60d630b0b8... git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review James-Park/drm-Allow-drm_fourcc-h-without-including-drm-h/20201204-163753 git checkout 444ac999e27a36307f741eb0ef60d630b0b8946a # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot lkp@intel.com
All warnings (new ones prefixed by >>):
usr/include/drm/drm_fourcc.h:29: found __[us]{8,16,32,64} type without #include <linux/types.h>
--- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
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
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 --- 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
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index 808b48a..a7f38fc 100644 --- a/include/uapi/drm/drm.h +++ b/include/uapi/drm/drm.h @@ -36,32 +36,22 @@ #ifndef _DRM_H_ #define _DRM_H_
+#include "drm_basic_types.h" + #if defined(__KERNEL__)
-#include <linux/types.h> #include <asm/ioctl.h> typedef unsigned int drm_handle_t;
#elif defined(__linux__)
-#include <linux/types.h> #include <asm/ioctl.h> typedef unsigned int drm_handle_t;
#else /* One of the BSDs */
-#include <stdint.h> #include <sys/ioccom.h> #include <sys/types.h> -typedef int8_t __s8; -typedef uint8_t __u8; -typedef int16_t __s16; -typedef uint16_t __u16; -typedef int32_t __s32; -typedef uint32_t __u32; -typedef int64_t __s64; -typedef uint64_t __u64; -typedef size_t __kernel_size_t; typedef unsigned long drm_handle_t;
#endif diff --git a/include/uapi/drm/drm_basic_types.h b/include/uapi/drm/drm_basic_types.h new file mode 100644 index 0000000..b3c00bb --- /dev/null +++ b/include/uapi/drm/drm_basic_types.h @@ -0,0 +1,52 @@ +/* + * Copyright 1999 Precision Insight, Inc., Cedar Park, Texas. + * Copyright 2000 VA Linux Systems, Inc., Sunnyvale, California. + * All rights reserved. + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice (including the next + * paragraph) shall be included in all copies or substantial portions of the + * Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * VA LINUX SYSTEMS AND/OR ITS SUPPLIERS BE LIABLE FOR ANY CLAIM, DAMAGES OR + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR + * OTHER DEALINGS IN THE SOFTWARE. + */ + +#ifndef _DRM_BASIC_TYPES_H_ +#define _DRM_BASIC_TYPES_H_ + +#if defined(__KERNEL__) + +#include <linux/types.h> + +#elif defined(__linux__) + +#include <linux/types.h> + +#else /* One of the BSDs */ + +#include <stdint.h> +typedef int8_t __s8; +typedef uint8_t __u8; +typedef int16_t __s16; +typedef uint16_t __u16; +typedef int32_t __s32; +typedef uint32_t __u32; +typedef int64_t __s64; +typedef uint64_t __u64; +typedef size_t __kernel_size_t; + +#endif + +#endif 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" {
On Sunday, December 6th, 2020 at 1:39 AM, James Park jpark37@lagfreegames.com wrote:
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 approach looks better to me than the other alternatives.
This will allow Mesa to port code to Windows more easily.
Signed-off-by: James Park jpark37@lagfreegames.com
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
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index 808b48a..a7f38fc 100644 --- a/include/uapi/drm/drm.h +++ b/include/uapi/drm/drm.h @@ -36,32 +36,22 @@ #ifndef _DRM_H_ #define _DRM_H_
+#include "drm_basic_types.h"
#if defined(__KERNEL__)
-#include <linux/types.h> #include <asm/ioctl.h> typedef unsigned int drm_handle_t;
#elif defined(__linux__)
-#include <linux/types.h> #include <asm/ioctl.h> typedef unsigned int drm_handle_t;
#else /* One of the BSDs */
-#include <stdint.h> #include <sys/ioccom.h> #include <sys/types.h> -typedef int8_t __s8; -typedef uint8_t __u8; -typedef int16_t __s16; -typedef uint16_t __u16; -typedef int32_t __s32; -typedef uint32_t __u32; -typedef int64_t __s64; -typedef uint64_t __u64; -typedef size_t __kernel_size_t; typedef unsigned long drm_handle_t;
#endif diff --git a/include/uapi/drm/drm_basic_types.h b/include/uapi/drm/drm_basic_types.h new file mode 100644 index 0000000..b3c00bb --- /dev/null +++ b/include/uapi/drm/drm_basic_types.h @@ -0,0 +1,52 @@ +/*
- Copyright 1999 Precision Insight, Inc., Cedar Park, Texas.
- Copyright 2000 VA Linux Systems, Inc., Sunnyvale, California.
- All rights reserved.
- Permission is hereby granted, free of charge, to any person obtaining a
- copy of this software and associated documentation files (the "Software"),
- to deal in the Software without restriction, including without limitation
- the rights to use, copy, modify, merge, publish, distribute, sublicense,
- and/or sell copies of the Software, and to permit persons to whom the
- Software is furnished to do so, subject to the following conditions:
- The above copyright notice and this permission notice (including the next
- paragraph) shall be included in all copies or substantial portions of the
- Software.
- THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
- VA LINUX SYSTEMS AND/OR ITS SUPPLIERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
- OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
- ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
- OTHER DEALINGS IN THE SOFTWARE.
- */
+#ifndef _DRM_BASIC_TYPES_H_ +#define _DRM_BASIC_TYPES_H_
+#if defined(__KERNEL__)
+#include <linux/types.h>
+#elif defined(__linux__)
Nit: these two #ifs can be combined together.
+#include <linux/types.h>
+#else /* One of the BSDs */
Maybe replace with /* Not Linux */?
+#include <stdint.h> +typedef int8_t __s8; +typedef uint8_t __u8; +typedef int16_t __s16; +typedef uint16_t __u16; +typedef int32_t __s32; +typedef uint32_t __u32; +typedef int64_t __s64; +typedef uint64_t __u64; +typedef size_t __kernel_size_t;
+#endif
+#endif 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" { -- 2.7.4
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
I'd noticed the #if could be combined, but they weren't in drm,h when they could have been, so I didn't want to depart from the existing pattern.
I think "One of the BSDs" is more informative than "Not Linux" if that statement is still true. Given the aversion to making drm.h robust to Windows, I don't think we want to imply compatibility that isn't there.
Thanks, James
On Mon, Dec 7, 2020 at 1:45 AM Simon Ser contact@emersion.fr wrote:
On Sunday, December 6th, 2020 at 1:39 AM, James Park < jpark37@lagfreegames.com> wrote:
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 approach looks better to me than the other alternatives.
This will allow Mesa to port code to Windows more easily.
Signed-off-by: James Park jpark37@lagfreegames.com
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
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index 808b48a..a7f38fc 100644 --- a/include/uapi/drm/drm.h +++ b/include/uapi/drm/drm.h @@ -36,32 +36,22 @@ #ifndef _DRM_H_ #define _DRM_H_
+#include "drm_basic_types.h"
#if defined(__KERNEL__)
-#include <linux/types.h> #include <asm/ioctl.h> typedef unsigned int drm_handle_t;
#elif defined(__linux__)
-#include <linux/types.h> #include <asm/ioctl.h> typedef unsigned int drm_handle_t;
#else /* One of the BSDs */
-#include <stdint.h> #include <sys/ioccom.h> #include <sys/types.h> -typedef int8_t __s8; -typedef uint8_t __u8; -typedef int16_t __s16; -typedef uint16_t __u16; -typedef int32_t __s32; -typedef uint32_t __u32; -typedef int64_t __s64; -typedef uint64_t __u64; -typedef size_t __kernel_size_t; typedef unsigned long drm_handle_t;
#endif diff --git a/include/uapi/drm/drm_basic_types.h
b/include/uapi/drm/drm_basic_types.h
new file mode 100644 index 0000000..b3c00bb --- /dev/null +++ b/include/uapi/drm/drm_basic_types.h @@ -0,0 +1,52 @@ +/*
- Copyright 1999 Precision Insight, Inc., Cedar Park, Texas.
- Copyright 2000 VA Linux Systems, Inc., Sunnyvale, California.
- All rights reserved.
- Permission is hereby granted, free of charge, to any person
obtaining a
- copy of this software and associated documentation files (the
"Software"),
- to deal in the Software without restriction, including without
limitation
- the rights to use, copy, modify, merge, publish, distribute,
sublicense,
- and/or sell copies of the Software, and to permit persons to whom the
- Software is furnished to do so, subject to the following conditions:
- The above copyright notice and this permission notice (including the
next
- paragraph) shall be included in all copies or substantial portions
of the
- Software.
- THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
EXPRESS OR
- IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
MERCHANTABILITY,
- FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT
SHALL
- VA LINUX SYSTEMS AND/OR ITS SUPPLIERS BE LIABLE FOR ANY CLAIM,
DAMAGES OR
- OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
- ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
- OTHER DEALINGS IN THE SOFTWARE.
- */
+#ifndef _DRM_BASIC_TYPES_H_ +#define _DRM_BASIC_TYPES_H_
+#if defined(__KERNEL__)
+#include <linux/types.h>
+#elif defined(__linux__)
Nit: these two #ifs can be combined together.
+#include <linux/types.h>
+#else /* One of the BSDs */
Maybe replace with /* Not Linux */?
+#include <stdint.h> +typedef int8_t __s8; +typedef uint8_t __u8; +typedef int16_t __s16; +typedef uint16_t __u16; +typedef int32_t __s32; +typedef uint32_t __u32; +typedef int64_t __s64; +typedef uint64_t __u64; +typedef size_t __kernel_size_t;
+#endif
+#endif 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" { -- 2.7.4
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Monday, December 7th, 2020 at 10:55 AM, James Park james.park@lagfreegames.com wrote:
I'd noticed the #if could be combined, but they weren't in drm,h when they could have been, so I didn't want to depart from the existing pattern.
I see. The original code really needed the two branches and drm_basic_types.h doesn't. But let's see what others think.
I think "One of the BSDs" is more informative than "Not Linux" if that statement is still true. Given the aversion to making drm.h robust to Windows, I don't think we want to imply compatibility that isn't there.
Well, drm_basic_types.h would be included on Windows as well from drm_fourcc.h, right?
The original code blocks in drm.h look identical to me. I see:
#include <linux/types.h> #include <asm/ioctl.h> typedef unsigned int drm_handle_t;
Good point about drm_basic_types.h. I'll change it to say "Not Linux" after waiting a bit for more feedback.
Thanks, James
On Mon, Dec 7, 2020 at 1:59 AM Simon Ser contact@emersion.fr wrote:
On Monday, December 7th, 2020 at 10:55 AM, James Park < james.park@lagfreegames.com> wrote:
I'd noticed the #if could be combined, but they weren't in drm,h when they could have been, so I didn't want to depart from the existing pattern.
I see. The original code really needed the two branches and drm_basic_types.h doesn't. But let's see what others think.
I think "One of the BSDs" is more informative than "Not Linux" if that statement is still true. Given the aversion to making drm.h robust to Windows, I don't think we want to imply compatibility that isn't there.
Well, drm_basic_types.h would be included on Windows as well from drm_fourcc.h, right?
On Monday, December 7th, 2020 at 11:05 AM, James Park james.park@lagfreegames.com wrote:
The original code blocks in drm.h look identical to me. I see:
#include <linux/types.h> #include <asm/ioctl.h> typedef unsigned int drm_handle_t;
Hmm. Actually you're completely right, it turns out it's necessary to duplicate the branches like this to make `make headers_install` work properly. See 00c9672606f7 ("drm: Untangle __KERNEL__ guards").
Hi James,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on drm-intel/for-linux-next] [also build test WARNING on drm-exynos/exynos-drm-next tegra-drm/drm/tegra/for-next drm-tip/drm-tip linus/master drm/drm-next v5.10-rc7 next-20201207] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/James-Park/drm-drm_basic_types-h-DR... base: git://anongit.freedesktop.org/drm-intel for-linux-next config: x86_64-allyesconfig (attached as .config) compiler: gcc-9 (Debian 9.3.0-15) 9.3.0 reproduce (this is a W=1 build): # https://github.com/0day-ci/linux/commit/c614dbde0c1dc422490fb22281d3e6dcc635... git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review James-Park/drm-drm_basic_types-h-DRM_FOURCC_STANDALONE/20201207-165922 git checkout c614dbde0c1dc422490fb22281d3e6dcc6355f66 # save the attached .config to linux build tree make W=1 ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot lkp@intel.com
All warnings (new ones prefixed by >>):
usr/include/linux/kfd_ioctl.h:37: found __[us]{8,16,32,64} type without #include <linux/types.h>
--- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Create drm_basic_types.h to define types previously defined by drm.h.
Use DRM_FOURCC_STANDALONE to include drm_fourcc.h without drm.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 | 12 ++------- include/uapi/drm/drm_basic_types.h | 52 ++++++++++++++++++++++++++++++++++++++ include/uapi/drm/drm_fourcc.h | 4 +++ 3 files changed, 58 insertions(+), 10 deletions(-) create mode 100644 include/uapi/drm/drm_basic_types.h
Create drm_basic_types.h to define types previously defined by drm.h.
Use DRM_FOURCC_STANDALONE to include drm_fourcc.h without drm.h.
This will allow Mesa to port code to Windows more easily.
Signed-off-by: James Park jpark37@lagfreegames.com --- include/uapi/drm/drm.h | 12 ++------- include/uapi/drm/drm_basic_types.h | 52 ++++++++++++++++++++++++++++++++++++++ include/uapi/drm/drm_fourcc.h | 4 +++ 3 files changed, 58 insertions(+), 10 deletions(-) create mode 100644 include/uapi/drm/drm_basic_types.h
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index 808b48a..d9ba922 100644 --- a/include/uapi/drm/drm.h +++ b/include/uapi/drm/drm.h @@ -36,6 +36,8 @@ #ifndef _DRM_H_ #define _DRM_H_
+#include "drm_basic_types.h" + #if defined(__KERNEL__)
#include <linux/types.h> @@ -50,18 +52,8 @@ typedef unsigned int drm_handle_t;
#else /* One of the BSDs */
-#include <stdint.h> #include <sys/ioccom.h> #include <sys/types.h> -typedef int8_t __s8; -typedef uint8_t __u8; -typedef int16_t __s16; -typedef uint16_t __u16; -typedef int32_t __s32; -typedef uint32_t __u32; -typedef int64_t __s64; -typedef uint64_t __u64; -typedef size_t __kernel_size_t; typedef unsigned long drm_handle_t;
#endif diff --git a/include/uapi/drm/drm_basic_types.h b/include/uapi/drm/drm_basic_types.h new file mode 100644 index 0000000..da1f2c0 --- /dev/null +++ b/include/uapi/drm/drm_basic_types.h @@ -0,0 +1,52 @@ +/* + * Copyright 1999 Precision Insight, Inc., Cedar Park, Texas. + * Copyright 2000 VA Linux Systems, Inc., Sunnyvale, California. + * All rights reserved. + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice (including the next + * paragraph) shall be included in all copies or substantial portions of the + * Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * VA LINUX SYSTEMS AND/OR ITS SUPPLIERS BE LIABLE FOR ANY CLAIM, DAMAGES OR + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR + * OTHER DEALINGS IN THE SOFTWARE. + */ + +#ifndef _DRM_BASIC_TYPES_H_ +#define _DRM_BASIC_TYPES_H_ + +#if defined(__KERNEL__) + +#include <linux/types.h> + +#elif defined(__linux__) + +#include <linux/types.h> + +#else /* Not Linux */ + +#include <stdint.h> +typedef int8_t __s8; +typedef uint8_t __u8; +typedef int16_t __s16; +typedef uint16_t __u16; +typedef int32_t __s32; +typedef uint32_t __u32; +typedef int64_t __s64; +typedef uint64_t __u64; +typedef size_t __kernel_size_t; + +#endif + +#endif diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h index 82f3278..539870f 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" {
May I ask what exactly fails when you drop #include <linux/types.h> from drm.h?
This was the message from kernel test robot.
usr/include/linux/kfd_ioctl.h:37: found __[us]{8,16,32,64} type without
#include <linux/types.h>
On Tue, Dec 8, 2020 at 4:31 AM Simon Ser contact@emersion.fr wrote:
May I ask what exactly fails when you drop #include <linux/types.h> from drm.h?
On Tuesday, December 8th, 2020 at 7:32 PM, James Park james.park@lagfreegames.com wrote:
This was the message from kernel test robot.
usr/include/linux/kfd_ioctl.h:37: found __[us]{8,16,32,64} type without #include <linux/types.h>
Interesting that the warning comes from linux/kfd_ioctl.h. I guess keeping the linux/types.h isn't that of a big deal anyways.
On Monday, December 7th, 2020 at 7:15 PM, James Park jpark37@lagfreegames.com wrote:
Create drm_basic_types.h to define types previously defined by drm.h. Use DRM_FOURCC_STANDALONE to include drm_fourcc.h without drm.h. This will allow Mesa to port code to Windows more easily.
Signed-off-by: James Park jpark37@lagfreegames.com
This version sounds fine to me, thanks.
Acked-by: Simon Ser contact@emersion.fr
Create drm_basic_types.h to define types previously defined by drm.h.
Use DRM_FOURCC_STANDALONE to include drm_fourcc.h without drm.h.
This will allow Mesa to port code to Windows more easily.
Signed-off-by: James Park jpark37@lagfreegames.com Acked-by: Simon Ser contact@emersion.fr
James Park (1): drm: drm_basic_types.h, DRM_FOURCC_STANDALONE
include/uapi/drm/drm.h | 12 ++------- include/uapi/drm/drm_basic_types.h | 52 ++++++++++++++++++++++++++++++++++++++ include/uapi/drm/drm_fourcc.h | 4 +++ 3 files changed, 58 insertions(+), 10 deletions(-) create mode 100644 include/uapi/drm/drm_basic_types.h
Create drm_basic_types.h to define types previously defined by drm.h.
Use DRM_FOURCC_STANDALONE to include drm_fourcc.h without drm.h.
This will allow Mesa to port code to Windows more easily.
Signed-off-by: James Park jpark37@lagfreegames.com Acked-by: Simon Ser contact@emersion.fr --- include/uapi/drm/drm.h | 12 ++------- include/uapi/drm/drm_basic_types.h | 52 ++++++++++++++++++++++++++++++++++++++ include/uapi/drm/drm_fourcc.h | 4 +++ 3 files changed, 58 insertions(+), 10 deletions(-) create mode 100644 include/uapi/drm/drm_basic_types.h
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index 808b48a..d9ba922 100644 --- a/include/uapi/drm/drm.h +++ b/include/uapi/drm/drm.h @@ -36,6 +36,8 @@ #ifndef _DRM_H_ #define _DRM_H_
+#include "drm_basic_types.h" + #if defined(__KERNEL__)
#include <linux/types.h> @@ -50,18 +52,8 @@ typedef unsigned int drm_handle_t;
#else /* One of the BSDs */
-#include <stdint.h> #include <sys/ioccom.h> #include <sys/types.h> -typedef int8_t __s8; -typedef uint8_t __u8; -typedef int16_t __s16; -typedef uint16_t __u16; -typedef int32_t __s32; -typedef uint32_t __u32; -typedef int64_t __s64; -typedef uint64_t __u64; -typedef size_t __kernel_size_t; typedef unsigned long drm_handle_t;
#endif diff --git a/include/uapi/drm/drm_basic_types.h b/include/uapi/drm/drm_basic_types.h new file mode 100644 index 0000000..da1f2c0 --- /dev/null +++ b/include/uapi/drm/drm_basic_types.h @@ -0,0 +1,52 @@ +/* + * Copyright 1999 Precision Insight, Inc., Cedar Park, Texas. + * Copyright 2000 VA Linux Systems, Inc., Sunnyvale, California. + * All rights reserved. + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice (including the next + * paragraph) shall be included in all copies or substantial portions of the + * Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * VA LINUX SYSTEMS AND/OR ITS SUPPLIERS BE LIABLE FOR ANY CLAIM, DAMAGES OR + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR + * OTHER DEALINGS IN THE SOFTWARE. + */ + +#ifndef _DRM_BASIC_TYPES_H_ +#define _DRM_BASIC_TYPES_H_ + +#if defined(__KERNEL__) + +#include <linux/types.h> + +#elif defined(__linux__) + +#include <linux/types.h> + +#else /* Not Linux */ + +#include <stdint.h> +typedef int8_t __s8; +typedef uint8_t __u8; +typedef int16_t __s16; +typedef uint16_t __u16; +typedef int32_t __s32; +typedef uint32_t __u32; +typedef int64_t __s64; +typedef uint64_t __u64; +typedef size_t __kernel_size_t; + +#endif + +#endif diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h index 82f3278..539870f 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" {
On Wed, 9 Dec 2020 03:03:10 -0800 James Park jpark37@lagfreegames.com wrote:
Create drm_basic_types.h to define types previously defined by drm.h.
Use DRM_FOURCC_STANDALONE to include drm_fourcc.h without drm.h.
This will allow Mesa to port code to Windows more easily.
Signed-off-by: James Park jpark37@lagfreegames.com Acked-by: Simon Ser contact@emersion.fr
Looks good to me. Reviewed-by: Pekka Paalanen pekka.paalanen@collabora.com
But with the caveat that I didn't actually test this.
Thanks, pq
include/uapi/drm/drm.h | 12 ++------- include/uapi/drm/drm_basic_types.h | 52 ++++++++++++++++++++++++++++++++++++++ include/uapi/drm/drm_fourcc.h | 4 +++ 3 files changed, 58 insertions(+), 10 deletions(-) create mode 100644 include/uapi/drm/drm_basic_types.h
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index 808b48a..d9ba922 100644 --- a/include/uapi/drm/drm.h +++ b/include/uapi/drm/drm.h @@ -36,6 +36,8 @@ #ifndef _DRM_H_ #define _DRM_H_
+#include "drm_basic_types.h"
#if defined(__KERNEL__)
#include <linux/types.h> @@ -50,18 +52,8 @@ typedef unsigned int drm_handle_t;
#else /* One of the BSDs */
-#include <stdint.h> #include <sys/ioccom.h> #include <sys/types.h> -typedef int8_t __s8; -typedef uint8_t __u8; -typedef int16_t __s16; -typedef uint16_t __u16; -typedef int32_t __s32; -typedef uint32_t __u32; -typedef int64_t __s64; -typedef uint64_t __u64; -typedef size_t __kernel_size_t; typedef unsigned long drm_handle_t;
#endif diff --git a/include/uapi/drm/drm_basic_types.h b/include/uapi/drm/drm_basic_types.h new file mode 100644 index 0000000..da1f2c0 --- /dev/null +++ b/include/uapi/drm/drm_basic_types.h @@ -0,0 +1,52 @@ +/*
- Copyright 1999 Precision Insight, Inc., Cedar Park, Texas.
- Copyright 2000 VA Linux Systems, Inc., Sunnyvale, California.
- All rights reserved.
- Permission is hereby granted, free of charge, to any person obtaining a
- copy of this software and associated documentation files (the "Software"),
- to deal in the Software without restriction, including without limitation
- the rights to use, copy, modify, merge, publish, distribute, sublicense,
- and/or sell copies of the Software, and to permit persons to whom the
- Software is furnished to do so, subject to the following conditions:
- The above copyright notice and this permission notice (including the next
- paragraph) shall be included in all copies or substantial portions of the
- Software.
- THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
- VA LINUX SYSTEMS AND/OR ITS SUPPLIERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
- OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
- ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
- OTHER DEALINGS IN THE SOFTWARE.
- */
+#ifndef _DRM_BASIC_TYPES_H_ +#define _DRM_BASIC_TYPES_H_
+#if defined(__KERNEL__)
+#include <linux/types.h>
+#elif defined(__linux__)
+#include <linux/types.h>
+#else /* Not Linux */
+#include <stdint.h> +typedef int8_t __s8; +typedef uint8_t __u8; +typedef int16_t __s16; +typedef uint16_t __u16; +typedef int32_t __s32; +typedef uint32_t __u32; +typedef int64_t __s64; +typedef uint64_t __u64; +typedef size_t __kernel_size_t;
+#endif
+#endif diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h index 82f3278..539870f 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" {
Create drm_basic_types.h to define types previously defined by drm.h.
Use DRM_FOURCC_STANDALONE to include drm_fourcc.h without drm.h.
This will allow Mesa to port code to Windows more easily.
Signed-off-by: James Park jpark37@lagfreegames.com Acked-by: Simon Ser contact@emersion.fr Reviewed-by: Pekka Paalanen pekka.paalanen@collabora.com
James Park (1): drm: drm_basic_types.h, DRM_FOURCC_STANDALONE
include/uapi/drm/drm.h | 12 ++------- include/uapi/drm/drm_basic_types.h | 52 ++++++++++++++++++++++++++++++++++++++ include/uapi/drm/drm_fourcc.h | 4 +++ 3 files changed, 58 insertions(+), 10 deletions(-) create mode 100644 include/uapi/drm/drm_basic_types.h
Create drm_basic_types.h to define types previously defined by drm.h.
Use DRM_FOURCC_STANDALONE to include drm_fourcc.h without drm.h.
This will allow Mesa to port code to Windows more easily.
Signed-off-by: James Park jpark37@lagfreegames.com Acked-by: Simon Ser contact@emersion.fr Reviewed-by: Pekka Paalanen pekka.paalanen@collabora.com --- include/uapi/drm/drm.h | 12 ++------- include/uapi/drm/drm_basic_types.h | 52 ++++++++++++++++++++++++++++++++++++++ include/uapi/drm/drm_fourcc.h | 4 +++ 3 files changed, 58 insertions(+), 10 deletions(-) create mode 100644 include/uapi/drm/drm_basic_types.h
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index 808b48a..d9ba922 100644 --- a/include/uapi/drm/drm.h +++ b/include/uapi/drm/drm.h @@ -36,6 +36,8 @@ #ifndef _DRM_H_ #define _DRM_H_
+#include "drm_basic_types.h" + #if defined(__KERNEL__)
#include <linux/types.h> @@ -50,18 +52,8 @@ typedef unsigned int drm_handle_t;
#else /* One of the BSDs */
-#include <stdint.h> #include <sys/ioccom.h> #include <sys/types.h> -typedef int8_t __s8; -typedef uint8_t __u8; -typedef int16_t __s16; -typedef uint16_t __u16; -typedef int32_t __s32; -typedef uint32_t __u32; -typedef int64_t __s64; -typedef uint64_t __u64; -typedef size_t __kernel_size_t; typedef unsigned long drm_handle_t;
#endif diff --git a/include/uapi/drm/drm_basic_types.h b/include/uapi/drm/drm_basic_types.h new file mode 100644 index 0000000..da1f2c0 --- /dev/null +++ b/include/uapi/drm/drm_basic_types.h @@ -0,0 +1,52 @@ +/* + * Copyright 1999 Precision Insight, Inc., Cedar Park, Texas. + * Copyright 2000 VA Linux Systems, Inc., Sunnyvale, California. + * All rights reserved. + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice (including the next + * paragraph) shall be included in all copies or substantial portions of the + * Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * VA LINUX SYSTEMS AND/OR ITS SUPPLIERS BE LIABLE FOR ANY CLAIM, DAMAGES OR + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR + * OTHER DEALINGS IN THE SOFTWARE. + */ + +#ifndef _DRM_BASIC_TYPES_H_ +#define _DRM_BASIC_TYPES_H_ + +#if defined(__KERNEL__) + +#include <linux/types.h> + +#elif defined(__linux__) + +#include <linux/types.h> + +#else /* Not Linux */ + +#include <stdint.h> +typedef int8_t __s8; +typedef uint8_t __u8; +typedef int16_t __s16; +typedef uint16_t __u16; +typedef int32_t __s32; +typedef uint32_t __u32; +typedef int64_t __s64; +typedef uint64_t __u64; +typedef size_t __kernel_size_t; + +#endif + +#endif diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h index 82f3278..539870f 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" {
Hello,
Is there something I can do to help move this patch along?
Thanks, James Park
On Thu, Dec 10, 2020 at 1:13 AM James Park jpark37@lagfreegames.com wrote:
Create drm_basic_types.h to define types previously defined by drm.h.
Use DRM_FOURCC_STANDALONE to include drm_fourcc.h without drm.h.
This will allow Mesa to port code to Windows more easily.
Signed-off-by: James Park jpark37@lagfreegames.com Acked-by: Simon Ser contact@emersion.fr Reviewed-by: Pekka Paalanen pekka.paalanen@collabora.com
include/uapi/drm/drm.h | 12 ++------- include/uapi/drm/drm_basic_types.h | 52 ++++++++++++++++++++++++++++++++++++++ include/uapi/drm/drm_fourcc.h | 4 +++ 3 files changed, 58 insertions(+), 10 deletions(-) create mode 100644 include/uapi/drm/drm_basic_types.h
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index 808b48a..d9ba922 100644 --- a/include/uapi/drm/drm.h +++ b/include/uapi/drm/drm.h @@ -36,6 +36,8 @@ #ifndef _DRM_H_ #define _DRM_H_
+#include "drm_basic_types.h"
#if defined(__KERNEL__)
#include <linux/types.h> @@ -50,18 +52,8 @@ typedef unsigned int drm_handle_t;
#else /* One of the BSDs */
-#include <stdint.h> #include <sys/ioccom.h> #include <sys/types.h> -typedef int8_t __s8; -typedef uint8_t __u8; -typedef int16_t __s16; -typedef uint16_t __u16; -typedef int32_t __s32; -typedef uint32_t __u32; -typedef int64_t __s64; -typedef uint64_t __u64; -typedef size_t __kernel_size_t; typedef unsigned long drm_handle_t;
#endif diff --git a/include/uapi/drm/drm_basic_types.h b/include/uapi/drm/drm_basic_types.h new file mode 100644 index 0000000..da1f2c0 --- /dev/null +++ b/include/uapi/drm/drm_basic_types.h @@ -0,0 +1,52 @@ +/*
- Copyright 1999 Precision Insight, Inc., Cedar Park, Texas.
- Copyright 2000 VA Linux Systems, Inc., Sunnyvale, California.
- All rights reserved.
- Permission is hereby granted, free of charge, to any person obtaining a
- copy of this software and associated documentation files (the
"Software"),
- to deal in the Software without restriction, including without
limitation
- the rights to use, copy, modify, merge, publish, distribute,
sublicense,
- and/or sell copies of the Software, and to permit persons to whom the
- Software is furnished to do so, subject to the following conditions:
- The above copyright notice and this permission notice (including the
next
- paragraph) shall be included in all copies or substantial portions of
the
- Software.
- THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
EXPRESS OR
- IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
MERCHANTABILITY,
- FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT
SHALL
- VA LINUX SYSTEMS AND/OR ITS SUPPLIERS BE LIABLE FOR ANY CLAIM, DAMAGES
OR
- OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
- ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
- OTHER DEALINGS IN THE SOFTWARE.
- */
+#ifndef _DRM_BASIC_TYPES_H_ +#define _DRM_BASIC_TYPES_H_
+#if defined(__KERNEL__)
+#include <linux/types.h>
+#elif defined(__linux__)
+#include <linux/types.h>
+#else /* Not Linux */
+#include <stdint.h> +typedef int8_t __s8; +typedef uint8_t __u8; +typedef int16_t __s16; +typedef uint16_t __u16; +typedef int32_t __s32; +typedef uint32_t __u32; +typedef int64_t __s64; +typedef uint64_t __u64; +typedef size_t __kernel_size_t;
+#endif
+#endif diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h index 82f3278..539870f 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" { -- 2.7.4
Hi James,
On Tue, 2 Feb 2021 at 08:27, James Park james.park@lagfreegames.com wrote:
Hello,
Is there something I can do to help move this patch along?
Thanks, James Park
On Thu, Dec 10, 2020 at 1:13 AM James Park jpark37@lagfreegames.com wrote:
Create drm_basic_types.h to define types previously defined by drm.h.
Use DRM_FOURCC_STANDALONE to include drm_fourcc.h without drm.h.
This will allow Mesa to port code to Windows more easily.
Signed-off-by: James Park jpark37@lagfreegames.com Acked-by: Simon Ser contact@emersion.fr Reviewed-by: Pekka Paalanen pekka.paalanen@collabora.com
include/uapi/drm/drm.h | 12 ++------- include/uapi/drm/drm_basic_types.h | 52 ++++++++++++++++++++++++++++++++++++++ include/uapi/drm/drm_fourcc.h | 4 +++ 3 files changed, 58 insertions(+), 10 deletions(-) create mode 100644 include/uapi/drm/drm_basic_types.h
Have you considered the possibility of having the ifdef block inlined within drm_fourcc.h?
Sure some users might need to add an drm.h include in their code. At the same time they also need to explicitly define DRM_FOURCC_STANDALONE, so that is fine. We had all sorts of issues with these headers in the past, so adding another one might end up repeating some of those yet again.
Thanks Emil
I'm not sure what your suggestion is. Move which #ifdef block where?
I don't think everyone is of the opinion that adding drm.h and pulling in unnecessary kernel structures is fine. If I'm not mistaken, the reason people are making me jump through hoops in the first place is to avoid that.
I appreciate the feedback though.
- James
On Tue, Feb 2, 2021 at 9:28 AM Emil Velikov emil.l.velikov@gmail.com wrote:
Hi James,
On Tue, 2 Feb 2021 at 08:27, James Park james.park@lagfreegames.com wrote:
Hello,
Is there something I can do to help move this patch along?
Thanks, James Park
On Thu, Dec 10, 2020 at 1:13 AM James Park jpark37@lagfreegames.com
wrote:
Create drm_basic_types.h to define types previously defined by drm.h.
Use DRM_FOURCC_STANDALONE to include drm_fourcc.h without drm.h.
This will allow Mesa to port code to Windows more easily.
Signed-off-by: James Park jpark37@lagfreegames.com Acked-by: Simon Ser contact@emersion.fr Reviewed-by: Pekka Paalanen pekka.paalanen@collabora.com
include/uapi/drm/drm.h | 12 ++------- include/uapi/drm/drm_basic_types.h | 52
++++++++++++++++++++++++++++++++++++++
include/uapi/drm/drm_fourcc.h | 4 +++ 3 files changed, 58 insertions(+), 10 deletions(-) create mode 100644 include/uapi/drm/drm_basic_types.h
Have you considered the possibility of having the ifdef block inlined within drm_fourcc.h?
Sure some users might need to add an drm.h include in their code. At the same time they also need to explicitly define DRM_FOURCC_STANDALONE, so that is fine. We had all sorts of issues with these headers in the past, so adding another one might end up repeating some of those yet again.
Thanks Emil
Hi james,
On Tue, 2 Feb 2021 at 18:15, James Park james.park@lagfreegames.com wrote:
I'm not sure what your suggestion is. Move which #ifdef block where?
Fair enough. Just sent out a patch which demonstrates what I have in mind.
Thanks Emil
P.S. Please try to avoid top-posting and HTML emails in public discussions.
On Tuesday, February 2nd, 2021 at 11:48 PM, Emil Velikov emil.l.velikov@gmail.com wrote:
P.S. Please try to avoid top-posting and HTML emails in public discussions.
(James: as a side note, here is a guide to configure your e-mail client to use plain-text: https://useplaintext.email/)
On Thursday, December 10th, 2020 at 10:12 AM, James Park jpark37@lagfreegames.com wrote:
+#ifndef _DRM_BASIC_TYPES_H_ +#define _DRM_BASIC_TYPES_H_
Nit: the rest of the kernel doesn't use an underscore prefix for these defines. Note that anything starting with an underscore followed by an upper-case letter is a reserved identifier in C.
On Tuesday, February 2nd, 2021 at 9:51 AM, Simon Ser contact@emersion.fr wrote:
On Thursday, December 10th, 2020 at 10:12 AM, James Park jpark37@lagfreegames.com wrote:
+#ifndef _DRM_BASIC_TYPES_H +#define _DRM_BASIC_TYPES_H
Nit: the rest of the kernel doesn't use an underscore prefix for these defines. Note that anything starting with an underscore followed by an upper-case letter is a reserved identifier in C.
Hm, please disregard, it seems some files still use an underscore.
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/0cc8ae750bfd9eab7e31c7de6aa84f24682f...
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 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/0cc8ae750bfd9eab7e31c7de6aa84f24682f...
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/0cc8ae750bfd9eab7e31c7de6aa84f24682f...
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
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.
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Emil Velikov -
James Park -
James Park -
Jason Ekstrand -
kernel test robot -
Michel Dänzer -
Pekka Paalanen -
Simon Ser -
Ville Syrjälä