From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-11.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,NICE_REPLY_A, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 6685EC43461 for ; Mon, 14 Sep 2020 02:14:24 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 38A7621655 for ; Mon, 14 Sep 2020 02:14:24 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726028AbgINCOX (ORCPT ); Sun, 13 Sep 2020 22:14:23 -0400 Received: from bhuna.collabora.co.uk ([46.235.227.227]:59360 "EHLO bhuna.collabora.co.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725974AbgINCOS (ORCPT ); Sun, 13 Sep 2020 22:14:18 -0400 Received: from [IPv6:2804:14c:483:7989::1000] (unknown [IPv6:2804:14c:483:7989::1000]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) (Authenticated sender: koike) by bhuna.collabora.co.uk (Postfix) with ESMTPSA id 5D90629601A; Mon, 14 Sep 2020 03:14:11 +0100 (BST) Subject: Re: [PATCH v5 1/7] media: v4l2: Extend pixel formats to unify single/multi-planar handling (and more) To: Hans Verkuil , mchehab@kernel.org, laurent.pinchart@ideasonboard.com, sakari.ailus@iki.fi, linux-media@vger.kernel.org Cc: Boris Brezillon , tfiga@chromium.org, hiroh@chromium.org, nicolas@ndufresne.ca, Brian.Starkey@arm.com, kernel@collabora.com, narmstrong@baylibre.com, linux-kernel@vger.kernel.org, frkoenig@chromium.org, mjourdan@baylibre.com, stanimir.varbanov@linaro.org References: <20200804192939.2251988-1-helen.koike@collabora.com> <20200804192939.2251988-2-helen.koike@collabora.com> <767d6287-bd9c-b342-d14c-124e58c143e6@xs4all.nl> From: Helen Koike Message-ID: <9293c7c8-9750-a79c-19ac-954a0629e04d@collabora.com> Date: Sun, 13 Sep 2020 23:14:07 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.12.0 MIME-Version: 1.0 In-Reply-To: <767d6287-bd9c-b342-d14c-124e58c143e6@xs4all.nl> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Hans, On 9/9/20 8:41 AM, Hans Verkuil wrote: > 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 >> Signed-off-by: Helen Koike >> --- >> 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(-) >> > > > >> 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). We discussed this on v4 https://patchwork.linuxtv.org/project/linux-media/cover/20200717115435.2632623-1-helen.koike@collabora.com/ The idea is to replace V4L2_PIX_FMT_FLAG_PREMUL_ALPHA by a modifier, and this API won't need CSC, since the colorspace fields are read-write. Regards, Helen > >> + __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 >