Re: [PATCH v4 1/6] media: v4l2: Extend pixel formats to unify single/multi-planar handling (and more)

[Hans Verkuil <hverkuil@xs4all.nl>]

On 28/07/2020 17:18, Helen Koike wrote:
> Hi Hans,
> 
> On 7/21/20 7:37 AM, Hans Verkuil wrote:
>> On 17/07/2020 13:54, Helen Koike wrote:
>>>  
>>> +/**
>>> + * struct v4l2_plane_ext_pix_format - additional, per-plane format definition
>>> + * @sizeimage:		maximum size in bytes required for data, for which
>>> + *			this plane will be used
>>> + * @bytesperline:	distance in bytes between the leftmost pixels in two
>>> + *			adjacent lines
>>> + * @reserved:		extra space reserved for future fields, must be set to 0
>>> + */
>>> +struct v4l2_plane_ext_pix_format {
>>> +	__u32 sizeimage;
>>> +	__u32 bytesperline;
>>> +	__u32 reserved;
>>> +} __attribute__ ((packed));
>>> +
>>> +/**
>>> + * struct v4l2_ext_pix_format - extended single/multiplanar format definition
>>> + * @type:		type of the data stream; V4L2_BUF_TYPE_VIDEO_CAPTURE or
>>> + *			V4L2_BUF_TYPE_VIDEO_OUTPUT
>>> + * @width:		image width in pixels
>>> + * @height:		image height in pixels
>>> + * @field:		enum v4l2_field; field order (for interlaced video)
>>> + * @pixelformat:	little endian four character code (fourcc)
>>> + * @modifier:		modifier applied to the format (used for tiled formats
>>> + *			and other kind of HW-specific formats, like compressed
>>> + *			formats)
>>> + * @colorspace:		enum v4l2_colorspace; supplemental to pixelformat
>>> + * @plane_fmt:		per-plane information
>>> + * @ycbcr_enc:		enum v4l2_ycbcr_encoding, Y'CbCr encoding
>>> + * @hsv_enc:		enum v4l2_hsv_encoding, HSV encoding
>>> + * @quantization:	enum v4l2_quantization, colorspace quantization
>>> + * @xfer_func:		enum v4l2_xfer_func, colorspace transfer function
>>> + * @reserved:		extra space reserved for future fields, must be set to 0
>>> + */
>>> +struct v4l2_ext_pix_format {
>>> +	__u32 type;
>>> +	__u32 width;
>>> +	__u32 height;
>>> +	__u32 field;
>>> +	__u32 pixelformat;
>>> +	__u64 modifier;
>>> +	__u32 colorspace;
>>
>> This struct has holes and is not the same for 32 and 64 bit architectures.
> 
> This would be true if this struct wasn't packed, but I believe we can remove the
> packed attribute, unless I'm missing something.
> What was the reason for other format structs to have __attribute__ ((packed)) ?

I've never really analyzed it. For new structs you want to avoid messing with that.

> 
>>
>> Moving modifier to before pixelformat will help a lot.
>>
>>> +	struct v4l2_plane_ext_pix_format plane_fmt[VIDEO_MAX_PLANES];
>>> +	union {
>>> +		__u8 ycbcr_enc;
>>> +		__u8 hsv_enc;
>>> +	};
>>> +	__u8 quantization;
>>> +	__u8 xfer_func;
>>
>> I'd change u8 to u32 for these fields for easier alignment.
> 
> Wouldn't it be better to add more reserved fields instead? So we can use this space
> latter in case we need them?
> 
> Without __attribute__ ((packed)), moving modifiers and changing reserved, I have
> from pahole in both 32 and 64 architectures:
> 
> struct v4l2_ext_pix_format {
> 	__u32                      type;                 /*     0     4 */
> 	__u32                      width;                /*     4     4 */
> 	__u32                      height;               /*     8     4 */
> 	__u32                      field;                /*    12     4 */
> 	__u64                      modifier;             /*    16     8 */
> 	__u32                      pixelformat;          /*    24     4 */
> 	__u32                      colorspace;           /*    28     4 */
> 	struct v4l2_plane_ext_pix_format plane_fmt[8];   /*    32    96 */
> 	/* --- cacheline 2 boundary (128 bytes) --- */
> 	union {
> 		__u8               ycbcr_enc;            /*   128     1 */
> 		__u8               hsv_enc;              /*   128     1 */
> 	};                                               /*   128     1 */
> 	__u8                       quantization;         /*   129     1 */
> 	__u8                       xfer_func;            /*   130     1 */
> 	__u8                       _reserved;            /*   131     1 */

This additional _reserved field is just what you want to avoid.

Just stick to u32 as much as possible with a u32 reserved array at the end.

> 	__u32                      reserved[5];          /*   132    20 */

[5] is definitely not enough. But that's something we can ignore until this
struct is finalized.

Regards,

	Hans

> 
> 	/* size: 152, cachelines: 3, members: 13 */
> 	/* last cacheline: 24 bytes */
> };
> 
> 
> What do you think?
> 
> Regards,
> Helen
> 
> 
>>
>> Regards,
>>
>> 	Hans
>>
>>> +	__u32 reserved[4];
>>> +} __attribute__ ((packed));
>>> +
>>>  /**
>>>   * struct v4l2_sdr_format - SDR format definition
>>>   * @pixelformat:	little endian four character code (fourcc)
>>> @@ -2569,6 +2620,10 @@ struct v4l2_create_buffers {
>>>  
>>>  #define VIDIOC_QUERY_EXT_CTRL	_IOWR('V', 103, struct v4l2_query_ext_ctrl)
>>>  
>>> +#define VIDIOC_G_EXT_PIX_FMT	_IOWR('V', 104, struct v4l2_ext_pix_format)
>>> +#define VIDIOC_S_EXT_PIX_FMT	_IOWR('V', 105, struct v4l2_ext_pix_format)
>>> +#define VIDIOC_TRY_EXT_PIX_FMT	_IOWR('V', 106, struct v4l2_ext_pix_format)
>>> +
>>>  /* Reminder: when adding new ioctls please add support for them to
>>>     drivers/media/v4l2-core/v4l2-compat-ioctl32.c as well! */
>>>  
>>>
>>
>>