From: Helen Koike <helen.koike@collabora.com>
To: Hans Verkuil <hverkuil@xs4all.nl>,
mchehab@kernel.org, laurent.pinchart@ideasonboard.com,
sakari.ailus@iki.fi, linux-media@vger.kernel.org
Cc: Boris Brezillon <boris.brezillon@collabora.com>,
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
Subject: Re: [PATCH v5 1/7] media: v4l2: Extend pixel formats to unify single/multi-planar handling (and more)
Date: Sun, 13 Sep 2020 23:14:07 -0300 [thread overview]
Message-ID: <9293c7c8-9750-a79c-19ac-954a0629e04d@collabora.com> (raw)
In-Reply-To: <767d6287-bd9c-b342-d14c-124e58c143e6@xs4all.nl>
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 <boris.brezillon@collabora.com>
>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
>> ---
>> 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).
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
>
next prev parent reply other threads:[~2020-09-14 2:14 UTC|newest]
Thread overview: 51+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-04 19:29 [PATCH v5 0/7] media: v4l2: Add extended fmt and buffer ioctls Helen Koike
2020-08-04 19:29 ` [PATCH v5 1/7] media: v4l2: Extend pixel formats to unify single/multi-planar handling (and more) Helen Koike
2020-08-14 7:49 ` Alexandre Courbot
2020-11-19 16:23 ` Helen Koike
2020-09-09 11:41 ` Hans Verkuil
2020-09-14 2:14 ` Helen Koike [this message]
2020-10-02 19:49 ` Tomasz Figa
2020-11-14 14:21 ` Helen Koike
2020-11-19 5:45 ` Tomasz Figa
2020-11-19 10:08 ` Helen Koike
2020-11-19 13:43 ` Helen Koike
2020-12-14 10:19 ` Tomasz Figa
2020-08-04 19:29 ` [PATCH v5 2/7] media: v4l2: Add extended buffer operations Helen Koike
2020-08-14 7:49 ` Alexandre Courbot
2020-09-09 12:27 ` Hans Verkuil
2020-11-23 15:08 ` Helen Koike
2020-11-23 15:46 ` Tomasz Figa
2020-11-23 17:40 ` Helen Koike
2020-12-03 15:11 ` Hans Verkuil
2020-12-03 19:52 ` Helen Koike
2020-12-14 10:46 ` Tomasz Figa
2020-12-15 14:36 ` Helen Koike
2020-12-16 3:13 ` Tomasz Figa
2020-12-17 13:19 ` Helen Koike
2020-12-21 3:13 ` Tomasz Figa
2020-12-23 12:04 ` Helen Koike
2021-01-08 10:00 ` Tomasz Figa
2020-12-14 10:38 ` Tomasz Figa
2020-11-20 11:14 ` Tomasz Figa
2020-11-23 20:33 ` Helen Koike
2020-12-14 10:36 ` Tomasz Figa
2020-12-14 13:23 ` Helen Koike
2020-12-15 9:03 ` Tomasz Figa
2020-08-04 19:29 ` [PATCH v5 3/7] media: videobuf2: Expose helpers to implement the _ext_fmt and _ext_buf hooks Helen Koike
2020-12-14 8:52 ` Tomasz Figa
2020-12-14 12:29 ` Helen Koike
2020-08-04 19:29 ` [PATCH v5 4/7] media: mediabus: Add helpers to convert a ext_pix format to/from a mbus_fmt Helen Koike
2020-08-14 7:49 ` Alexandre Courbot
2020-08-04 19:29 ` [PATCH v5 5/7] media: vivid: Convert the capture and output drivers to EXT_FMT/EXT_BUF Helen Koike
2020-08-04 19:29 ` [PATCH v5 6/7] media: vimc: Implement the ext_fmt and ext_buf hooks Helen Koike
2020-08-04 19:29 ` [PATCH v5 7/7] media: docs: add documentation for the Extended API Helen Koike
2020-08-14 7:49 ` Alexandre Courbot
2020-11-19 10:28 ` Helen Koike
2020-11-20 11:06 ` Tomasz Figa
2020-11-20 12:24 ` Hans Verkuil
2020-11-20 12:40 ` Tomasz Figa
2020-11-20 13:20 ` Hans Verkuil
2021-01-14 18:04 ` Helen Koike
2020-08-04 19:34 ` [PATCH v5 0/7] media: v4l2: Add extended fmt and buffer ioctls Helen Koike
2020-08-14 7:49 ` Alexandre Courbot
2020-11-27 15:06 ` Helen Koike
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=9293c7c8-9750-a79c-19ac-954a0629e04d@collabora.com \
--to=helen.koike@collabora.com \
--cc=Brian.Starkey@arm.com \
--cc=boris.brezillon@collabora.com \
--cc=frkoenig@chromium.org \
--cc=hiroh@chromium.org \
--cc=hverkuil@xs4all.nl \
--cc=kernel@collabora.com \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=mchehab@kernel.org \
--cc=mjourdan@baylibre.com \
--cc=narmstrong@baylibre.com \
--cc=nicolas@ndufresne.ca \
--cc=sakari.ailus@iki.fi \
--cc=stanimir.varbanov@linaro.org \
--cc=tfiga@chromium.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).