Hi Helen,
Some review comments, concentrating on the uAPI.
On 04/08/2020 21:29, Helen Koike wrote:
> This is part of the multiplanar and singleplanar unification process.
> v4l2_ext_pix_format is supposed to work for both cases.
>
> We also add the concept of modifiers already employed in DRM to expose
> HW-specific formats (like tiled or compressed formats) and allow
> exchanging this information with the DRM subsystem in a consistent way.
>
> Note that only V4L2_BUF_TYPE_VIDEO_[OUTPUT,CAPTURE] are accepted in
> v4l2_ext_format, other types will be rejected if you use the
> {G,S,TRY}_EXT_PIX_FMT ioctls.
>
> New hooks have been added to v4l2_ioctl_ops to support those new ioctls
> in drivers, but, in the meantime, the core takes care of converting
> {S,G,TRY}_EXT_PIX_FMT requests into {S,G,TRY}_FMT so that old drivers can
> still work if the userspace app/lib uses the new ioctls.
> The conversion is also done the other around to allow userspace
> apps/libs using {S,G,TRY}_FMT to work with drivers implementing the
> _ext_ hooks.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>
> Signed-off-by: Helen Koike <helen.koike@xxxxxxxxxxxxx>
> ---
> Changes in v5:
> - change sizes and reorder fields to avoid holes in the struct and make
> it the same for 32 and 64 bits
> - removed __attribute__ ((packed)) from uapi structs
> - Fix doc warning from make htmldocs
> - Updated commit message with EXT_PIX prefix for the ioctls.
>
> Changes in v4:
> - Use v4l2_ext_pix_format directly in the ioctl, drop v4l2_ext_format,
> making V4L2_BUF_TYPE_VIDEO_[OUTPUT,CAPTURE] the only valid types.
> - Add reserved fields
> - Removed num_planes from struct v4l2_ext_pix_format
> - Removed flag field from struct v4l2_ext_pix_format, since the only
> defined value is V4L2_PIX_FMT_FLAG_PREMUL_ALPHA only used by vsp1,
> where we can use modifiers, or add it back later through the reserved
> bits.
> - In v4l2_ext_format_to_format(), check if modifier is != MOD_LINEAR &&
> != MOD_INVALID
> - Fix type assignment in v4l_g_fmt_ext_pix()
> - Rebased on top of media/master (post 5.8-rc1)
>
> Changes in v3:
> - Rebased on top of media/master (post 5.4-rc1)
>
> Changes in v2:
> - Move the modifier in v4l2_ext_format (was formerly placed in
> v4l2_ext_plane)
> - Fix a few bugs in the converters and add a strict parameter to
> allow conversion of uninitialized/mis-initialized objects
> ---
> drivers/media/v4l2-core/v4l2-dev.c | 21 +-
> drivers/media/v4l2-core/v4l2-ioctl.c | 585 +++++++++++++++++++++++----
> include/media/v4l2-ioctl.h | 34 ++
> include/uapi/linux/videodev2.h | 56 +++
> 4 files changed, 615 insertions(+), 81 deletions(-)
>
<snip>
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index c7b70ff53bc1d..7123c6a4d9569 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -2254,6 +2254,58 @@ struct v4l2_pix_format_mplane {
> __u8 reserved[7];
> } __attribute__ ((packed));
>
> +/**
> + * 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.
> + * Should be set to zero for unused planes.
> + * @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;
I'd use reserved[4] here.
> +};
> +
> +/**
> + * 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)
This should refer to the drm.h header since we're reusing the DRM modifiers.
> + * @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;
> + __u64 modifier;
> + __u32 pixelformat;
> + __u32 colorspace;
> + struct v4l2_plane_ext_pix_format plane_fmt[VIDEO_MAX_PLANES];
> + union {
> + __u32 ycbcr_enc;
> + __u32 hsv_enc;
> + };
> + __u32 quantization;
> + __u32 xfer_func;
I'd reorder this:
struct v4l2_plane_ext_pix_format plane_fmt[VIDEO_MAX_PLANES];
__u32 pixelformat;
__u32 colorspace;
__u32 xfer_func;
union {
__u32 ycbcr_enc;
__u32 hsv_enc;
};
__u32 quantization;
The reason for reordering is that I like to keep the colorimetry fields in
that order since that is how these fields are processed mathematically: you
apply the colorspace matrix first, then the transfer function, then optionally
convert to Y'CbCr or HSV and finally you quantize the result.
There is also a __u32 flags; field missing (needed for V4L2_PIX_FMT_FLAG_PREMUL_ALPHA
and for the upcoming CSC support).
> + __u32 reserved[9];
> +};
> +
> /**
> * struct v4l2_sdr_format - SDR format definition
> * @pixelformat: little endian four character code (fourcc)
> @@ -2571,6 +2623,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! */
>
>
Regards,
Hans
