[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2 1/1] v4l2: use __u32 rather than enums in ioctl() structs



On Mon May 7 2012 16:27:49 Sakari Ailus wrote:
> From: Rémi Denis-Courmont <remi@xxxxxxxxxx>
> 
> 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 <remi@xxxxxxxxxx>
> Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxx>
> ---
> Changes since v1:
> 
> - Fixes according to comments by Hans Verkuil:
>   - Update documentation
>   - Also remove enums in compat32 code
> 
>  Documentation/DocBook/media/v4l/io.xml             |   12 +++--
>  .../DocBook/media/v4l/vidioc-create-bufs.xml       |   10 +++--
>  Documentation/DocBook/media/v4l/vidioc-cropcap.xml |    4 +-
>  .../DocBook/media/v4l/vidioc-enum-fmt.xml          |    4 +-
>  Documentation/DocBook/media/v4l/vidioc-g-crop.xml  |    4 +-
>  Documentation/DocBook/media/v4l/vidioc-g-fmt.xml   |    2 +-
>  .../DocBook/media/v4l/vidioc-g-frequency.xml       |    6 +-
>  Documentation/DocBook/media/v4l/vidioc-g-parm.xml  |    5 +-
>  .../DocBook/media/v4l/vidioc-g-sliced-vbi-cap.xml  |    2 +-
>  Documentation/DocBook/media/v4l/vidioc-g-tuner.xml |    2 +-
>  .../DocBook/media/v4l/vidioc-queryctrl.xml         |    2 +-
>  Documentation/DocBook/media/v4l/vidioc-reqbufs.xml |    7 ++-
>  .../DocBook/media/v4l/vidioc-s-hw-freq-seek.xml    |    5 +-
>  drivers/media/video/v4l2-compat-ioctl32.c          |   12 +++---
>  include/linux/videodev2.h                          |   46 ++++++++++----------
>  15 files changed, 65 insertions(+), 58 deletions(-)
> 
> diff --git a/Documentation/DocBook/media/v4l/io.xml b/Documentation/DocBook/media/v4l/io.xml
> index b815929..fd6aca2 100644
> --- a/Documentation/DocBook/media/v4l/io.xml
> +++ b/Documentation/DocBook/media/v4l/io.xml
> @@ -543,12 +543,13 @@ and can range from zero to the number of buffers allocated
>  with the &VIDIOC-REQBUFS; ioctl (&v4l2-requestbuffers; <structfield>count</structfield>) minus one.</entry>
>  	  </row>
>  	  <row>
> -	    <entry>&v4l2-buf-type;</entry>
> +	    <entry>__u32</entry>

The problem with replacing &v4l2-buf-type; by __u32 is that you loose the link
to the v4l2-buf-type enum.

>  	    <entry><structfield>type</structfield></entry>
>  	    <entry></entry>
>  	    <entry>Type of the buffer, same as &v4l2-format;

I would change this to something like:

Type of the buffer (see enum &v4l2-buf-type;), same as...

Same for all the other similar cases. Annoying, I know, but I believe it is
important to have these links available.

... cut ...

> 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;

Same here: you need a comment like "/* see enum v4l2_field */" to keep the
association between the field and the possible value.

Regards,

	Hans

>  	__u32            	bytesperline;	/* for padding, zero if unused */
>  	__u32          		sizeimage;
> -	enum v4l2_colorspace	colorspace;
> +	__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;
>  	__u8		    description[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_memory        memory;
> +	__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_timecode	timecode;
>  	__u32			sequence;
>  
>  	/* memory location */
> -	enum v4l2_memory        memory;
> +	__u32			memory;
>  	union {
>  		__u32           offset;
>  		unsigned long   userptr;
> @@ -708,7 +708,7 @@ struct v4l2_clip {
>  
>  struct v4l2_window {
>  	struct v4l2_rect        w;
> -	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_rect        bounds;
>  	struct v4l2_rect        defrect;
>  	struct v4l2_fract       pixelaspect;
>  };
>  
>  struct v4l2_crop {
> -	enum v4l2_buf_type      type;
> +	__u32			type;
>  	struct v4l2_rect        c;
>  };
>  
> @@ -1157,7 +1157,7 @@ enum v4l2_ctrl_type {
>  /*  Used in the VIDIOC_QUERYCTRL ioctl for querying controls */
>  struct v4l2_queryctrl {
>  	__u32		     id;
> -	enum v4l2_ctrl_type  type;
> +	__u32		     type;
>  	__u8		     name[32];	/* Whatever */
>  	__s32		     minimum;	/* Note signedness */
>  	__s32		     maximum;
> @@ -1792,7 +1792,7 @@ enum v4l2_jpeg_chroma_subsampling {
>  struct v4l2_tuner {
>  	__u32                   index;
>  	__u8			name[32];
> -	enum v4l2_tuner_type    type;
> +	__u32			type;
>  	__u32			capability;
>  	__u32			rangelow;
>  	__u32			rangehigh;
> @@ -1842,14 +1842,14 @@ struct v4l2_modulator {
>  
>  struct v4l2_frequency {
>  	__u32		      tuner;
> -	enum v4l2_tuner_type  type;
> +	__u32		      type;
>  	__u32		      frequency;
>  	__u32		      reserved[8];
>  };
>  
>  struct v4l2_hw_freq_seek {
>  	__u32		      tuner;
> -	enum v4l2_tuner_type  type;
> +	__u32		      type;
>  	__u32		      seek_upward;
>  	__u32		      wrap_around;
>  	__u32		      spacing;
> @@ -2060,7 +2060,7 @@ struct v4l2_sliced_vbi_cap {
>  				 (equals frame lines 313-336 for 625 line video
>  				  standards, 263-286 for 525 line standards) */
>  	__u16   service_lines[2][24];
> -	enum v4l2_buf_type type;
> +	__u32	type;
>  	__u32   reserved[3];    /* must be 0 */
>  };
>  
> @@ -2150,8 +2150,8 @@ struct v4l2_pix_format_mplane {
>  	__u32				width;
>  	__u32				height;
>  	__u32				pixelformat;
> -	enum v4l2_field			field;
> -	enum v4l2_colorspace		colorspace;
> +	__u32				field;
> +	__u32				colorspace;
>  
>  	struct v4l2_plane_pix_format	plane_fmt[VIDEO_MAX_PLANES];
>  	__u8				num_planes;
> @@ -2169,7 +2169,7 @@ struct v4l2_pix_format_mplane {
>   * @raw_data:	placeholder for future extensions and custom formats
>   */
>  struct v4l2_format {
> -	enum v4l2_buf_type type;
> +	__u32	 type;
>  	union {
>  		struct v4l2_pix_format		pix;     /* V4L2_BUF_TYPE_VIDEO_CAPTURE */
>  		struct v4l2_pix_format_mplane	pix_mp;  /* V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE */
> @@ -2183,7 +2183,7 @@ struct v4l2_format {
>  /*	Stream type-dependent parameters
>   */
>  struct v4l2_streamparm {
> -	enum v4l2_buf_type type;
> +	__u32	 type;
>  	union {
>  		struct v4l2_captureparm	capture;
>  		struct v4l2_outputparm	output;
> @@ -2303,7 +2303,7 @@ struct v4l2_dbg_chip_ident {
>  struct v4l2_create_buffers {
>  	__u32			index;
>  	__u32			count;
> -	enum v4l2_memory        memory;
> +	__u32			memory;
>  	struct v4l2_format	format;
>  	__u32			reserved[8];
>  };
> @@ -2360,8 +2360,8 @@ struct v4l2_create_buffers {
>  #define VIDIOC_TRY_FMT      	_IOWR('V', 64, struct v4l2_format)
>  #define VIDIOC_ENUMAUDIO	_IOWR('V', 65, struct v4l2_audio)
>  #define VIDIOC_ENUMAUDOUT	_IOWR('V', 66, struct v4l2_audioout)
> -#define VIDIOC_G_PRIORITY        _IOR('V', 67, enum v4l2_priority)
> -#define VIDIOC_S_PRIORITY        _IOW('V', 68, enum v4l2_priority)
> +#define VIDIOC_G_PRIORITY	 _IOR('V', 67, __u32)
> +#define VIDIOC_S_PRIORITY	 _IOW('V', 68, __u32)
>  #define VIDIOC_G_SLICED_VBI_CAP _IOWR('V', 69, struct v4l2_sliced_vbi_cap)
>  #define VIDIOC_LOG_STATUS         _IO('V', 70)
>  #define VIDIOC_G_EXT_CTRLS	_IOWR('V', 71, struct v4l2_ext_controls)
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Linux Input]     [Video for Linux]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Photos]     [Yosemite Photos]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Devices]     [Yosemite Backpacking]

Add to Google Powered by Linux