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! */
>>>
>>>
>>
>>
