Re: [PATCH 1/1] v4l2: use __u32 rather than enums in ioctl() structs
On Sat May 5 2012 12:25:45 Sakari Ailus wrote: From: Rémi Denis-Courmont r...@remlab.net V4L2 uses the enum type in IOCTL arguments in IOCTLs that were defined until the use of enum was considered less than ideal. Recently Rémi Denis-Courmont brought up the issue by proposing a patch to convert the enums to unsigned: URL:http://www.spinics.net/lists/linux-media/msg46167.html This sparked a long discussion where another solution to the issue was proposed: two sets of IOCTL structures, one with __u32 and the other with enums, and conversion code between the two: URL:http://www.spinics.net/lists/linux-media/msg47168.html Both approaches implement a complete solution that resolves the problem. The first one is simple but requires assuming enums and __u32 are the same in size (so we won't break the ABI) while the second one is more complex and less clean but does not require making that assumption. The issue boils down to whether enums are fundamentally different from __u32 or not, and can the former be substituted by the latter. During the discussion it was concluded that the __u32 has the same size as enums on all archs Linux is supported: it has not been shown that replacing those enums in IOCTL arguments would break neither source or binary compatibility. If no such reason is found, just replacing the enums with __u32s is the way to go. This is what this patch does. This patch is slightly different from Remi's first RFC (link above): it uses __u32 instead of unsigned and also changes the arguments of VIDIOC_G_PRIORITY and VIDIOC_S_PRIORITY. Signed-off-by: Rémi Denis-Courmont r...@remlab.net Signed-off-by: Sakari Ailus sakari.ai...@iki.fi --- include/linux/videodev2.h | 46 ++-- 1 files changed, 23 insertions(+), 23 deletions(-) diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h index 5a09ac3..585e4b4 100644 --- a/include/linux/videodev2.h +++ b/include/linux/videodev2.h @@ -292,10 +292,10 @@ struct v4l2_pix_format { __u32 width; __u32 height; __u32 pixelformat; - enum v4l2_field field; + __u32 field; One suggestion: add a comment like this: __u32 field; /* refer to enum v4l2_field */ This keeps the link between the u32 and the enum values. Note that the DocBook documentation also has to be updated. Looks good to me otherwise. Regards, Hans __u32 bytesperline; /* for padding, zero if unused */ __u32 sizeimage; - enum v4l2_colorspacecolorspace; + __u32 colorspace; __u32 priv; /* private data, depends on pixelformat */ }; @@ -432,7 +432,7 @@ struct v4l2_pix_format { */ struct v4l2_fmtdesc { __u32 index; /* Format number */ - enum v4l2_buf_type type; /* buffer type*/ + __u32 type; /* buffer type*/ __u32 flags; __u8description[32]; /* Description string */ __u32 pixelformat; /* Format fourcc */ @@ -573,8 +573,8 @@ struct v4l2_jpegcompression { */ struct v4l2_requestbuffers { __u32 count; - enum v4l2_buf_type type; - enum v4l2_memorymemory; + __u32 type; + __u32 memory; __u32 reserved[2]; }; @@ -636,16 +636,16 @@ struct v4l2_plane { */ struct v4l2_buffer { __u32 index; - enum v4l2_buf_type type; + __u32 type; __u32 bytesused; __u32 flags; - enum v4l2_field field; + __u32 field; struct timeval timestamp; struct v4l2_timecodetimecode; __u32 sequence; /* memory location */ - enum v4l2_memorymemory; + __u32 memory; union { __u32 offset; unsigned long userptr; @@ -708,7 +708,7 @@ struct v4l2_clip { struct v4l2_window { struct v4l2_rectw; - enum v4l2_field field; + __u32 field; __u32 chromakey; struct v4l2_clip__user *clips; __u32 clipcount; @@ -745,14 +745,14 @@ struct v4l2_outputparm { * I N P U T I M A G E C R O P P I N G */ struct v4l2_cropcap { - enum v4l2_buf_type type; + __u32 type; struct v4l2_rectbounds; struct v4l2_rectdefrect; struct v4l2_fract pixelaspect; }; struct v4l2_crop { - enum v4l2_buf_type
Re: [PATCH 1/1] v4l2: use __u32 rather than enums in ioctl() structs
Hi Hans, On Sat, May 05, 2012 at 12:55:01PM +0200, Hans Verkuil wrote: On Sat May 5 2012 12:25:45 Sakari Ailus wrote: From: Rémi Denis-Courmont r...@remlab.net V4L2 uses the enum type in IOCTL arguments in IOCTLs that were defined until the use of enum was considered less than ideal. Recently Rémi Denis-Courmont brought up the issue by proposing a patch to convert the enums to unsigned: URL:http://www.spinics.net/lists/linux-media/msg46167.html This sparked a long discussion where another solution to the issue was proposed: two sets of IOCTL structures, one with __u32 and the other with enums, and conversion code between the two: URL:http://www.spinics.net/lists/linux-media/msg47168.html Both approaches implement a complete solution that resolves the problem. The first one is simple but requires assuming enums and __u32 are the same in size (so we won't break the ABI) while the second one is more complex and less clean but does not require making that assumption. The issue boils down to whether enums are fundamentally different from __u32 or not, and can the former be substituted by the latter. During the discussion it was concluded that the __u32 has the same size as enums on all archs Linux is supported: it has not been shown that replacing those enums in IOCTL arguments would break neither source or binary compatibility. If no such reason is found, just replacing the enums with __u32s is the way to go. This is what this patch does. This patch is slightly different from Remi's first RFC (link above): it uses __u32 instead of unsigned and also changes the arguments of VIDIOC_G_PRIORITY and VIDIOC_S_PRIORITY. Signed-off-by: Rémi Denis-Courmont r...@remlab.net Signed-off-by: Sakari Ailus sakari.ai...@iki.fi --- include/linux/videodev2.h | 46 ++-- 1 files changed, 23 insertions(+), 23 deletions(-) diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h index 5a09ac3..585e4b4 100644 --- a/include/linux/videodev2.h +++ b/include/linux/videodev2.h @@ -292,10 +292,10 @@ struct v4l2_pix_format { __u32 width; __u32 height; __u32 pixelformat; - enum v4l2_field field; + __u32 field; One suggestion: add a comment like this: __u32 field; /* refer to enum v4l2_field */ This keeps the link between the u32 and the enum values. Note that the DocBook documentation also has to be updated. Looks good to me otherwise. Thanks for the comments. I'll make the above changes. Cheers, -- Sakari Ailus e-mail: sakari.ai...@iki.fi jabber/XMPP/Gmail: sai...@retiisi.org.uk -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] v4l2: use __u32 rather than enums in ioctl() structs
On Sat May 5 2012 13:14:10 Sakari Ailus wrote: Hi Hans, On Sat, May 05, 2012 at 12:55:01PM +0200, Hans Verkuil wrote: On Sat May 5 2012 12:25:45 Sakari Ailus wrote: From: Rémi Denis-Courmont r...@remlab.net V4L2 uses the enum type in IOCTL arguments in IOCTLs that were defined until the use of enum was considered less than ideal. Recently Rémi Denis-Courmont brought up the issue by proposing a patch to convert the enums to unsigned: URL:http://www.spinics.net/lists/linux-media/msg46167.html This sparked a long discussion where another solution to the issue was proposed: two sets of IOCTL structures, one with __u32 and the other with enums, and conversion code between the two: URL:http://www.spinics.net/lists/linux-media/msg47168.html Both approaches implement a complete solution that resolves the problem. The first one is simple but requires assuming enums and __u32 are the same in size (so we won't break the ABI) while the second one is more complex and less clean but does not require making that assumption. The issue boils down to whether enums are fundamentally different from __u32 or not, and can the former be substituted by the latter. During the discussion it was concluded that the __u32 has the same size as enums on all archs Linux is supported: it has not been shown that replacing those enums in IOCTL arguments would break neither source or binary compatibility. If no such reason is found, just replacing the enums with __u32s is the way to go. This is what this patch does. This patch is slightly different from Remi's first RFC (link above): it uses __u32 instead of unsigned and also changes the arguments of VIDIOC_G_PRIORITY and VIDIOC_S_PRIORITY. Signed-off-by: Rémi Denis-Courmont r...@remlab.net Signed-off-by: Sakari Ailus sakari.ai...@iki.fi --- include/linux/videodev2.h | 46 ++-- 1 files changed, 23 insertions(+), 23 deletions(-) diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h index 5a09ac3..585e4b4 100644 --- a/include/linux/videodev2.h +++ b/include/linux/videodev2.h @@ -292,10 +292,10 @@ struct v4l2_pix_format { __u32 width; __u32 height; __u32 pixelformat; - enum v4l2_field field; + __u32 field; One suggestion: add a comment like this: __u32 field; /* refer to enum v4l2_field */ This keeps the link between the u32 and the enum values. Note that the DocBook documentation also has to be updated. And v4l2-compat-ioctl32 as well! :-) That too has enums in the compat structs. Regards, Hans Looks good to me otherwise. Thanks for the comments. I'll make the above changes. Cheers, -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html