| Message ID | 20230206043308.28365-1-ayaka@soulik.info |
|---|---|
| Headers | show |
| Series | media: v4l2: Add extended fmt and buffer | expand |
Hi, please avoid HTML replies on the mailing list. Le mardi 27 juin 2023 à 14:42 +0800, Hsia-Jun Li a écrit : > > +/** > > + * struct v4l2_ext_pix_format - extended multiplanar format definition > > + * @type: enum v4l2_buf_type; type of the data stream > > + * @width: image width in pixels > > + * @height: image height in pixels > > + * @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) as defined in drm_fourcc.h > > + * @field: enum v4l2_field; field order (for interlaced video) > > + * @colorspace: enum v4l2_colorspace; supplemental to pixelformat > > + * @plane_fmt: per-plane information > > + * @flags: format flags (V4L2_PIX_FMT_FLAG_*) > > + * @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: drivers and applications must zero this array > > + */ > > +struct v4l2_ext_pix_format { > > + __u32 type; > > + __u32 width; > > + __u32 height; > > + __u32 pixelformat; > > + __u64 modifier; > > + __u32 field; > > + __u32 colorspace; > > + > > + struct v4l2_plane_pix_format plane_fmt[VIDEO_MAX_PLANES]; > > + __u8 flags; > > + union { > > + __u8 ycbcr_enc; > > + __u8 hsv_enc; > > + }; > > + __u8 quantization; > > + __u8 xfer_func; > > > > > > > > > > > > I heard that a suggestion that we could remove colorimetry fields > here. > Although those are useless for codec M2M drivers if no pixel > formats translation invoked. > Even HDMI(DRM) cares about colorspace. > For example if a downsink(TV) shows RGB formats, with an YUV > input frame buffer, colorimetry would be important or the wrong > EOTF would be used. If YUV is MPEG range(linear EOTF) while a > non-linear EOFT (full range) is used, you would found the black is > not black enough while the white looks a gray. Also color bias > would happen. > This problem may not happen to a ultra high resolution TV while > only YUV type color formats are supported due to HDMI bandwidth > limitation. > The problem I want to raise is the time cost for enumeration. > Each pixel format with a colorimetry setting would invoke a > ioctl(). For the application likes Gstreamer would enum all the > possible colorimetries. > It would be better we could have something like DRM blob id that > application could copy the data from a non-DMA buffer from the > kernel. This is a good topic. Colorimetry could indeed be moved away from the format, considering they cannot be enumerated. It remains that this information needs to be passed around, and the format of a blob in media space is not has restricted as with display HW. I think keeping an "exploded version" of the colorimetry remains needed. Remember though that for stateful decoder, were the information could be stored in the bitstream, the decoder is responsible for returning that information. Currently its passed through the G_FMT call, it would need to be replaced with a control, similar to the HDR10 static metadata. If the colorimetry is no longer static in the future, and may change while streaming, one option would be RO request. This was foreseen for HDR10+ and Dolby Vision metadata notably, though other options exists. There exist known decoders that can do YUV to RGB conversion using an inline post procesor (VC8000D and newer is an example), and for these to return correct colors, the colorimetry information needs to be passed. So its not strictly useless. In short, if we drop colorimetry from format, we also need to go ahead and design a replacement for it, that allow for the application to detect changes. regards, Nicolas
On 6/29/23 00:27, Nicolas Dufresne wrote: > CAUTION: Email originated externally, do not click links or open attachments unless you recognize the sender and know the content is safe. > > > Hi, > > please avoid HTML replies on the mailing list. I need to blame outlook. > Le mardi 27 juin 2023 à 14:42 +0800, Hsia-Jun Li a écrit : >>> +/** >>> + * struct v4l2_ext_pix_format - extended multiplanar format definition >>> + * @type: enum v4l2_buf_type; type of the data stream >>> + * @width: image width in pixels >>> + * @height: image height in pixels >>> + * @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) as defined in drm_fourcc.h >>> + * @field: enum v4l2_field; field order (for interlaced video) >>> + * @colorspace: enum v4l2_colorspace; supplemental to pixelformat >>> + * @plane_fmt: per-plane information >>> + * @flags: format flags (V4L2_PIX_FMT_FLAG_*) >>> + * @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: drivers and applications must zero this array >>> + */ >>> +struct v4l2_ext_pix_format { >>> + __u32 type; >>> + __u32 width; >>> + __u32 height; >>> + __u32 pixelformat; >>> + __u64 modifier; >>> + __u32 field; >>> + __u32 colorspace; >>> + >>> + struct v4l2_plane_pix_format plane_fmt[VIDEO_MAX_PLANES]; >>> + __u8 flags; >>> + union { >>> + __u8 ycbcr_enc; >>> + __u8 hsv_enc; >>> + }; >>> + __u8 quantization; >>> + __u8 xfer_func; >>> >>> >>> >>> >>> >> I heard that a suggestion that we could remove colorimetry fields >> here. >> Although those are useless for codec M2M drivers if no pixel >> formats translation invoked. >> Even HDMI(DRM) cares about colorspace. >> For example if a downsink(TV) shows RGB formats, with an YUV >> input frame buffer, colorimetry would be important or the wrong >> EOTF would be used. If YUV is MPEG range(linear EOTF) while a >> non-linear EOFT (full range) is used, you would found the black is >> not black enough while the white looks a gray. Also color bias >> would happen. >> This problem may not happen to a ultra high resolution TV while >> only YUV type color formats are supported due to HDMI bandwidth >> limitation. >> The problem I want to raise is the time cost for enumeration. >> Each pixel format with a colorimetry setting would invoke a >> ioctl(). For the application likes Gstreamer would enum all the >> possible colorimetries. >> It would be better we could have something like DRM blob id that >> application could copy the data from a non-DMA buffer from the >> kernel. > This is a good topic. Colorimetry could indeed be moved away from the format, > considering they cannot be enumerated. It remains that this information needs to > be passed around, and the format of a blob in media space is not has restricted > as with display HW. I think keeping an "exploded version" of the colorimetry > remains needed. > > Remember though that for stateful decoder, were the information could be stored > in the bitstream, the decoder is responsible for returning that information. > Currently its passed through the G_FMT call, it would need to be replaced with a > control, similar to the HDR10 static metadata. If the colorimetry is no longer > static in the future, and may change while streaming, one option would be RO > request. This was foreseen for HDR10+ and Dolby Vision metadata notably, though > other options exists. That is why I want to promote importing a whole buffer(framebuffer id) from drm. We can't pass such DMA metadate with the main data(graphics here) in a queue. > > There exist known decoders that can do YUV to RGB conversion using an inline > post procesor (VC8000D and newer is an example), and for these to return correct > colors, the colorimetry information needs to be passed. So its not strictly > useless. > > In short, if we drop colorimetry from format, we also need to go ahead and I am worrying when people setup up a UVC device(usb camera), they may ignore this step. Camera could be in a BT.601 colorspace than BT.709 which has a slight difference. And BT.601 has a full range variant. > design a replacement for it, that allow for the application to detect changes. > > regards, > Nicolas >
On Mon, Feb 06, 2023 at 12:33:00PM +0800, ayaka wrote: > From: Randy Li <ayaka@soulik.info> > > 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_pix_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> > Signed-off-by: Randy Li <ayaka@soulik.info> > --- > > Changes in v7: > - use the resevered fields in v4l2_fmtdesc to store the modifier for format > enumerate. > - Dropping the old ioctl() calling new interfaces adapter. Why? We need backwards compatibility with the legacy API, but requiring drivers to implement both will add even more boiler plate code to the drivers and potential for each driver to have a buggy implementation in its own way. (From my experience, every driver already handles the TRY_FMT/S_FMT operations in its own slightly buggy way.) What we probably don't need is core code for making old drivers automatically support the new API (i.e. the other way around). We can have the drivers modified to the new API whenever one needs to use the new features with the old hardware. (And possibly other modernization of the driver code could happen at the same time.) > - Refresh the patch > Changes in v6: > The main change here was fixing the conversion, so planes reflects color planes, > and to implement this properly I made major refactors compared to the previous > version. > - struct v4l2_plane_ext_pix_format removed, using struct v4l2_plane_pix_format instead (Tomasz) > - refer to drm_fourcc.h in struct v4l2_ext_pix_format docs (Hans) > - reorder colorimetry fields in struct v4l2_ext_pix_format (Hans) > - do not set Ext ioctls as valid for vid_out_overlay (Tomasz) > - refactor conversion functions, so planes are color planes (Tomasz) > - Don't explicitly check for e->modifier != 0 in v4l2_ext_pix_format_to_format() (Tomasz) > - Use "ef" for extended formats in the framework for consistency (Tomasz) > - Handle xfer_func field in conversions (Tomasz) > - Zero reserved fields in v4l_s_ext_pix_fmt() and v4l_try_ext_pix_fmt() (Tomasz) > - Refactor format functions to use v4l_fmt_ioctl_via_ext() > - Several fixes/refactoring/changes > - Remove EXT API for touch devices > > 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 | 13 + > drivers/media/v4l2-core/v4l2-ioctl.c | 357 ++++++++++++++++++++++++++- > include/media/v4l2-ioctl.h | 28 +++ > include/uapi/linux/videodev2.h | 46 +++- > 4 files changed, 438 insertions(+), 6 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c > index 397d553177fa..cfe90bfd47f1 100644 > --- a/drivers/media/v4l2-core/v4l2-dev.c > +++ b/drivers/media/v4l2-core/v4l2-dev.c > @@ -615,6 +615,10 @@ static void determine_valid_ioctls(struct video_device *vdev) > ops->vidioc_g_fmt_vid_out_mplane || > ops->vidioc_g_fmt_vid_out_overlay))) > __set_bit(_IOC_NR(VIDIOC_G_FMT), valid_ioctls); > + if ((is_rx && ops->vidioc_g_ext_pix_fmt_vid_cap) || > + (is_tx && ops->vidioc_g_ext_pix_fmt_vid_out)) { > + __set_bit(_IOC_NR(VIDIOC_G_EXT_PIX_FMT), valid_ioctls); > + } > if ((is_rx && (ops->vidioc_s_fmt_vid_cap || > ops->vidioc_s_fmt_vid_cap_mplane || > ops->vidioc_s_fmt_vid_overlay)) || > @@ -622,6 +626,11 @@ static void determine_valid_ioctls(struct video_device *vdev) > ops->vidioc_s_fmt_vid_out_mplane || > ops->vidioc_s_fmt_vid_out_overlay))) > __set_bit(_IOC_NR(VIDIOC_S_FMT), valid_ioctls); > + > + if ((is_rx && ops->vidioc_s_ext_pix_fmt_vid_cap) || > + (is_tx && ops->vidioc_s_ext_pix_fmt_vid_out)) { > + __set_bit(_IOC_NR(VIDIOC_S_EXT_PIX_FMT), valid_ioctls); > + } > if ((is_rx && (ops->vidioc_try_fmt_vid_cap || > ops->vidioc_try_fmt_vid_cap_mplane || > ops->vidioc_try_fmt_vid_overlay)) || > @@ -629,6 +638,10 @@ static void determine_valid_ioctls(struct video_device *vdev) > ops->vidioc_try_fmt_vid_out_mplane || > ops->vidioc_try_fmt_vid_out_overlay))) > __set_bit(_IOC_NR(VIDIOC_TRY_FMT), valid_ioctls); > + if ((is_rx && ops->vidioc_try_ext_pix_fmt_vid_cap) || > + (is_tx && ops->vidioc_try_ext_pix_fmt_vid_out)) { > + __set_bit(_IOC_NR(VIDIOC_TRY_EXT_PIX_FMT), valid_ioctls); > + } > SET_VALID_IOCTL(ops, VIDIOC_OVERLAY, vidioc_overlay); > SET_VALID_IOCTL(ops, VIDIOC_G_FBUF, vidioc_g_fbuf); > SET_VALID_IOCTL(ops, VIDIOC_S_FBUF, vidioc_s_fbuf); > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c > index 87f163a89c80..52b77a968bb3 100644 > --- a/drivers/media/v4l2-core/v4l2-ioctl.c > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c > @@ -18,8 +18,10 @@ > > #include <linux/v4l2-subdev.h> > #include <linux/videodev2.h> > +#include <drm/drm_fourcc.h> > > #include <media/media-device.h> /* for media_set_bus_info() */ > + > #include <media/v4l2-common.h> > #include <media/v4l2-ioctl.h> > #include <media/v4l2-ctrls.h> > @@ -34,6 +36,11 @@ > > #define is_valid_ioctl(vfd, cmd) test_bit(_IOC_NR(cmd), (vfd)->valid_ioctls) > > +#define V4L2_IS_CAP_MULTIPLANAR(vdev) (vdev->device_caps & \ > + (V4L2_CAP_VIDEO_CAPTURE_MPLANE | \ > + V4L2_CAP_VIDEO_OUTPUT_MPLANE | \ > + V4L2_CAP_VIDEO_M2M_MPLANE)) > + > struct std_descr { > v4l2_std_id std; > const char *descr; > @@ -353,6 +360,27 @@ static void v4l_print_format(const void *arg, bool write_only) > } > } > > +static void v4l_print_ext_pix_format(const void *arg, bool write_only) > +{ > + const struct v4l2_ext_pix_format *ef = arg; > + unsigned int i; > + > + pr_cont("type=%s, width=%u, height=%u, format=%c%c%c%c, modifier %llx, field=%s, colorspace=%d, ycbcr_enc=%u, quantization=%u, xfer_func=%u\n", > + prt_names(ef->type, v4l2_type_names), > + ef->width, ef->height, > + (ef->pixelformat & 0xff), > + (ef->pixelformat >> 8) & 0xff, > + (ef->pixelformat >> 16) & 0xff, > + (ef->pixelformat >> 24) & 0xff, > + ef->modifier, prt_names(ef->field, v4l2_field_names), > + ef->colorspace, ef->ycbcr_enc, > + ef->quantization, ef->xfer_func); > + for (i = 0; i < VIDEO_MAX_PLANES && ef->plane_fmt[i].sizeimage; i++) > + pr_debug("plane %u: bytesperline=%u sizeimage=%u\n", > + i, ef->plane_fmt[i].bytesperline, > + ef->plane_fmt[i].sizeimage); > +} > + > static void v4l_print_framebuffer(const void *arg, bool write_only) > { > const struct v4l2_framebuffer *p = arg; > @@ -940,7 +968,9 @@ static int check_fmt(struct file *file, enum v4l2_buf_type type) > switch (type) { > case V4L2_BUF_TYPE_VIDEO_CAPTURE: > if ((is_vid || is_tch) && is_rx && > - (ops->vidioc_g_fmt_vid_cap || ops->vidioc_g_fmt_vid_cap_mplane)) > + (ops->vidioc_g_fmt_vid_cap || > + ops->vidioc_g_ext_pix_fmt_vid_cap || > + ops->vidioc_g_fmt_vid_cap_mplane)) > return 0; > break; > case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE: > @@ -953,7 +983,9 @@ static int check_fmt(struct file *file, enum v4l2_buf_type type) > break; > case V4L2_BUF_TYPE_VIDEO_OUTPUT: > if (is_vid && is_tx && > - (ops->vidioc_g_fmt_vid_out || ops->vidioc_g_fmt_vid_out_mplane)) > + (ops->vidioc_g_fmt_vid_out || > + ops->vidioc_g_ext_pix_fmt_vid_out || > + ops->vidioc_g_fmt_vid_out_mplane)) > return 0; > break; > case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE: > @@ -1082,6 +1114,203 @@ static void v4l_sanitize_format(struct v4l2_format *fmt) > } > } > > +static void > +v4l2_ext_pix_format_to_pix_format(const struct v4l2_ext_pix_format *ef, > + struct v4l2_pix_format *pix) > +{ > + unsigned int i; > + > + pix->width = ef->width; > + pix->height = ef->height; > + pix->field = ef->field; > + pix->flags = V4L2_PIX_FMT_FLAG_SET_CSC; > + pix->colorspace = ef->colorspace; > + pix->ycbcr_enc = ef->ycbcr_enc; > + pix->priv = V4L2_PIX_FMT_PRIV_MAGIC; > + pix->quantization = ef->quantization; > + pix->pixelformat = ef->pixelformat; > + pix->bytesperline = ef->plane_fmt[0].bytesperline; > + pix->sizeimage = ef->plane_fmt[0].sizeimage; > + for (i = 1; i < VIDEO_MAX_PLANES && ef->plane_fmt[i].sizeimage; i++) > + pix->sizeimage += ef->plane_fmt[i].sizeimage; > +} > + > +static void > +v4l2_ext_pix_format_to_pix_mp_format(const struct v4l2_ext_pix_format *ef, > + struct v4l2_pix_format_mplane *pix_mp) > +{ > + const struct v4l2_format_info *info = > + v4l2_format_info(ef->pixelformat); > + unsigned int i; > + > + pix_mp->width = ef->width; > + pix_mp->height = ef->height; > + pix_mp->field = ef->field; > + pix_mp->flags = V4L2_PIX_FMT_FLAG_SET_CSC; > + pix_mp->colorspace = ef->colorspace; > + pix_mp->ycbcr_enc = ef->ycbcr_enc; > + pix_mp->quantization = ef->quantization; > + pix_mp->pixelformat = ef->pixelformat; > + > + /* This is true when converting to non-M-variant */ > + if (info && info->mem_planes == 1) { > + pix_mp->plane_fmt[0] = ef->plane_fmt[0]; > + pix_mp->num_planes = 1; > + for (i = 1; i < VIDEO_MAX_PLANES && ef->plane_fmt[i].sizeimage; i++) > + pix_mp->plane_fmt[0].sizeimage += ef->plane_fmt[i].sizeimage; > + > + return; > + } > + > + for (i = 0; i < VIDEO_MAX_PLANES && ef->plane_fmt[i].sizeimage; i++) > + pix_mp->plane_fmt[i] = ef->plane_fmt[i]; > + pix_mp->num_planes = i; > +} > + > +/* > + * v4l2_ext_pix_format_to_format - convert to v4l2_ext_pix_format to v4l2_format > + * > + * @ef: A pointer to struct struct v4l2_ext_pix_format to be converted. > + * @f: A pointer to struct v4l2_format to be filled. > + * @is_mplane: Bool indicating if multiplanar API should be used in @f. > + * > + * If pixelformat should be converted to M-variant, change ef->pixelformat > + * to the M-variant before calling this function. > + */ > +static void v4l2_ext_pix_format_to_format(const struct v4l2_ext_pix_format *ef, > + struct v4l2_format *f, bool is_mplane) > +{ > + memset(f, 0, sizeof(*f)); > + > + if (ef->modifier != DRM_FORMAT_MOD_LINEAR && > + ef->modifier != DRM_FORMAT_MOD_INVALID) > + pr_warn("Modifiers are not supported in v4l2_format, ignoring %llx\n", > + ef->modifier); > + > + if (!is_mplane) { > + f->type = ef->type; > + v4l2_ext_pix_format_to_pix_format(ef, &f->fmt.pix); > + return; > + } > + > + if (ef->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) > + f->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE; > + else > + f->type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE; > + > + v4l2_ext_pix_format_to_pix_mp_format(ef, &f->fmt.pix_mp); > +} > + > +static void > +v4l2_pix_format_to_ext_pix_format(const struct v4l2_pix_format *pix, > + struct v4l2_ext_pix_format *ef) > +{ > + const struct v4l2_format_info *info = > + v4l2_format_info(pix->pixelformat); > + unsigned int i; > + > + ef->width = pix->width; > + ef->height = pix->height; > + ef->field = pix->field; > + ef->colorspace = pix->colorspace; > + ef->ycbcr_enc = pix->ycbcr_enc; > + ef->quantization = pix->quantization; > + ef->xfer_func = pix->xfer_func; > + if (pix->flags) > + pr_warn("Ignoring pixelformat flags 0x%x\n", pix->flags); > + > + /* We assume M-variants won't be used in this function */ > + ef->pixelformat = pix->pixelformat; > + > + ef->plane_fmt[0].bytesperline = pix->bytesperline; > + ef->plane_fmt[0].sizeimage = pix->sizeimage; > + > + if (!info) > + return; Should we print a warning in case of !info? > + > + for (i = 1; i < info->comp_planes; i++) { > + ef->plane_fmt[i].bytesperline = pix->bytesperline / info->hdiv; > + ef->plane_fmt[i].sizeimage = ef->plane_fmt[i].bytesperline * > + ef->height / info->vdiv; > + ef->plane_fmt[0].sizeimage -= ef->plane_fmt[i].sizeimage; > + } > +} > + > +static void > +v4l2_pix_mp_format_to_ext_pix_format(const struct v4l2_pix_format_mplane *pix_mp, > + struct v4l2_ext_pix_format *ef) > +{ > + const struct v4l2_format_info *info = > + v4l2_format_info(pix_mp->pixelformat); > + unsigned int i; > + > + ef->width = pix_mp->width; > + ef->height = pix_mp->height; > + ef->field = pix_mp->field; > + ef->colorspace = pix_mp->colorspace; > + ef->ycbcr_enc = pix_mp->ycbcr_enc; > + ef->quantization = pix_mp->quantization; > + ef->xfer_func = pix_mp->xfer_func; > + if (pix_mp->flags) > + pr_warn("Ignoring pixelformat flags 0x%x\n", pix_mp->flags); > + > + if (!info) > + return; If we return here, the returned ef will not make any sense, because it would have uninitialized plane_fmt[]. Should we print a warning here too? > + > + ef->pixelformat = pix_mp->pixelformat; > + > + if (info->comp_planes == info->mem_planes) { > + for (i = 0; i < pix_mp->num_planes && i < VIDEO_MAX_PLANES; i++) > + ef->plane_fmt[i] = pix_mp->plane_fmt[i]; > + > + return; > + } > + > + /* case where mem_planes is 1 and comp_planes > 1 */ > + ef->plane_fmt[0] = pix_mp->plane_fmt[0]; > + for (i = 1; i < info->comp_planes; i++) { > + ef->plane_fmt[i].bytesperline = > + pix_mp->plane_fmt[0].bytesperline / info->hdiv; > + ef->plane_fmt[i].sizeimage = > + ef->plane_fmt[i].bytesperline * ef->height / info->vdiv; > + ef->plane_fmt[0].sizeimage -= ef->plane_fmt[i].sizeimage; > + } > +} > + > +/* > + * v4l2_format_to_ext_pix_format - convert to v4l2_format to v4l2_ext_pix_format > + * > + * @f: A pointer to struct v4l2_format to be converted. > + * @ef: A pointer to struct struct v4l2_ext_pix_format to be filled. > + * > + * This method normalize the pixelformat to non-M variant. > + */ > +static void v4l2_format_to_ext_pix_format(const struct v4l2_format *f, > + struct v4l2_ext_pix_format *ef) > +{ > + memset(ef, 0, sizeof(*ef)); > + > + switch (f->type) { > + case V4L2_BUF_TYPE_VIDEO_CAPTURE: > + case V4L2_BUF_TYPE_VIDEO_OUTPUT: > + ef->type = f->type; > + v4l2_pix_format_to_ext_pix_format(&f->fmt.pix, ef); > + break; > + case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE: > + ef->type = V4L2_BUF_TYPE_VIDEO_CAPTURE; > + v4l2_pix_mp_format_to_ext_pix_format(&f->fmt.pix_mp, ef); > + break; > + case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE: > + ef->type = V4L2_BUF_TYPE_VIDEO_OUTPUT; > + v4l2_pix_mp_format_to_ext_pix_format(&f->fmt.pix_mp, ef); > + break; > + default: > + WARN("Converting to Ext Pix Format with wrong buffer type %s\n", > + prt_names(f->type, v4l2_type_names)); > + break; > + } > +} > + > static int v4l_querycap(const struct v4l2_ioctl_ops *ops, > struct file *file, void *fh, void *arg) > { > @@ -1654,7 +1883,9 @@ static int v4l_g_fmt(const struct v4l2_ioctl_ops *ops, > v4l_pix_format_touch(&p->fmt.pix); > return ret; > case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE: > - return ops->vidioc_g_fmt_vid_cap_mplane(file, fh, arg); > + if (ops->vidioc_g_fmt_vid_cap_mplane) > + return ops->vidioc_g_fmt_vid_cap_mplane(file, fh, arg); What is this change needed for? > + break; > case V4L2_BUF_TYPE_VIDEO_OVERLAY: > return ops->vidioc_g_fmt_vid_overlay(file, fh, arg); > case V4L2_BUF_TYPE_VBI_CAPTURE: > @@ -1662,7 +1893,8 @@ static int v4l_g_fmt(const struct v4l2_ioctl_ops *ops, > case V4L2_BUF_TYPE_SLICED_VBI_CAPTURE: > return ops->vidioc_g_fmt_sliced_vbi_cap(file, fh, arg); > case V4L2_BUF_TYPE_VIDEO_OUTPUT: > - if (unlikely(!ops->vidioc_g_fmt_vid_out)) > + if (unlikely(!ops->vidioc_g_fmt_vid_out && > + !ops->vidioc_g_ext_pix_fmt_vid_out)) Ditto. > break; > p->fmt.pix.priv = V4L2_PIX_FMT_PRIV_MAGIC; > ret = ops->vidioc_g_fmt_vid_out(file, fh, arg); > @@ -1670,7 +1902,9 @@ static int v4l_g_fmt(const struct v4l2_ioctl_ops *ops, > p->fmt.pix.priv = V4L2_PIX_FMT_PRIV_MAGIC; > return ret; > case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE: > - return ops->vidioc_g_fmt_vid_out_mplane(file, fh, arg); > + if (ops->vidioc_g_fmt_vid_out_mplane) > + return ops->vidioc_g_fmt_vid_out_mplane(file, fh, arg); > + break; Ditto. > case V4L2_BUF_TYPE_VIDEO_OUTPUT_OVERLAY: > return ops->vidioc_g_fmt_vid_out_overlay(file, fh, arg); > case V4L2_BUF_TYPE_VBI_OUTPUT: > @@ -1689,6 +1923,42 @@ static int v4l_g_fmt(const struct v4l2_ioctl_ops *ops, > return -EINVAL; > } > > +static int v4l_g_ext_pix_fmt(const struct v4l2_ioctl_ops *ops, > + struct file *file, void *fh, void *arg) > +{ > + struct v4l2_ext_pix_format *ef = arg; > + struct v4l2_format f = { > + .type = ef->type, > + }; > + int ret = check_fmt(file, ef->type); > + > + if (ret) > + return ret; > + > + memset(ef, 0, sizeof(*ef)); > + ef->type = f.type; > + > + switch (f.type) { > + case V4L2_BUF_TYPE_VIDEO_CAPTURE: > + if (ops->vidioc_g_ext_pix_fmt_vid_cap) > + return ops->vidioc_g_ext_pix_fmt_vid_cap(file, fh, ef); > + break; > + case V4L2_BUF_TYPE_VIDEO_OUTPUT: > + if (ops->vidioc_g_ext_pix_fmt_vid_out) > + return ops->vidioc_g_ext_pix_fmt_vid_out(file, fh, ef); > + break; > + default: > + return -EINVAL; > + } > + Given that determine_valid_ioctls() will only set the valid ioctl bit if the right operation is provided by the driver, when would the code below be run? > + ret = v4l_g_fmt(ops, file, fh, &f); > + if (ret) > + return ret; > + > + v4l2_format_to_ext_pix_format(&f, ef); > + return 0; > +} > + > static int v4l_s_fmt(const struct v4l2_ioctl_ops *ops, > struct file *file, void *fh, void *arg) > { > @@ -1794,6 +2064,42 @@ static int v4l_s_fmt(const struct v4l2_ioctl_ops *ops, > return -EINVAL; > } > > +static int v4l_s_ext_pix_fmt(const struct v4l2_ioctl_ops *ops,() > + struct file *file, void *fh, void *arg) > +{ > + struct video_device *vfd = video_devdata(file); > + struct v4l2_ext_pix_format *ef = arg; > + struct v4l2_format f; > + int ret = check_fmt(file, ef->type); > + > + if (ret) > + return ret; > + > + memset(ef->reserved, 0, sizeof(ef->reserved)); > + > + switch (ef->type) { > + case V4L2_BUF_TYPE_VIDEO_CAPTURE: > + if (ops->vidioc_s_ext_pix_fmt_vid_cap) > + return ops->vidioc_s_ext_pix_fmt_vid_cap(file, fh, ef); > + break; > + case V4L2_BUF_TYPE_VIDEO_OUTPUT: > + if (ops->vidioc_s_ext_pix_fmt_vid_out) > + return ops->vidioc_s_ext_pix_fmt_vid_out(file, fh, ef); > + break; > + default: > + return -EINVAL; > + } > + Ditto. > + v4l2_ext_pix_format_to_format(ef, &f, V4L2_IS_CAP_MULTIPLANAR(vfd)); > + > + ret = v4l_s_fmt(ops, file, fh, &f); > + if (ret) > + return ret; > + > + v4l2_format_to_ext_pix_format(&f, ef); > + return 0; > +} > + > static int v4l_try_fmt(const struct v4l2_ioctl_ops *ops, > struct file *file, void *fh, void *arg) > { > @@ -1896,6 +2202,44 @@ static int v4l_try_fmt(const struct v4l2_ioctl_ops *ops, > return -EINVAL; > } > > +static int v4l_try_ext_pix_fmt(const struct v4l2_ioctl_ops *ops, > + struct file *file, void *fh, void *arg) > +{ > + struct video_device *vfd = video_devdata(file); > + struct v4l2_ext_pix_format *ef = arg; > + struct v4l2_format f; > + int ret = check_fmt(file, ef->type); > + > + if (ret) > + return ret; > + > + memset(ef->reserved, 0, sizeof(ef->reserved)); > + > + switch (ef->type) { > + case V4L2_BUF_TYPE_VIDEO_CAPTURE: > + if (ops->vidioc_try_ext_pix_fmt_vid_cap) > + return ops->vidioc_try_ext_pix_fmt_vid_cap(file, fh, > + ef); > + break; > + case V4L2_BUF_TYPE_VIDEO_OUTPUT: > + if (ops->vidioc_try_ext_pix_fmt_vid_out) > + return ops->vidioc_try_ext_pix_fmt_vid_out(file, fh, > + ef); > + break; > + default: > + return -EINVAL; > + } Ditto. > + > + v4l2_ext_pix_format_to_format(ef, &f, V4L2_IS_CAP_MULTIPLANAR(vfd)); > + > + ret = v4l_try_fmt(ops, file, fh, &f); > + if (ret) > + return ret; > + > + v4l2_format_to_ext_pix_format(&f, ef); > + return 0; > +} > + > static int v4l_streamon(const struct v4l2_ioctl_ops *ops, > struct file *file, void *fh, void *arg) > { > @@ -2902,6 +3246,9 @@ static const struct v4l2_ioctl_info v4l2_ioctls[] = { > IOCTL_INFO(VIDIOC_ENUM_FREQ_BANDS, v4l_enum_freq_bands, v4l_print_freq_band, 0), > IOCTL_INFO(VIDIOC_DBG_G_CHIP_INFO, v4l_dbg_g_chip_info, v4l_print_dbg_chip_info, INFO_FL_CLEAR(v4l2_dbg_chip_info, match)), > IOCTL_INFO(VIDIOC_QUERY_EXT_CTRL, v4l_query_ext_ctrl, v4l_print_query_ext_ctrl, INFO_FL_CTRL | INFO_FL_CLEAR(v4l2_query_ext_ctrl, id)), > + IOCTL_INFO(VIDIOC_G_EXT_PIX_FMT, v4l_g_ext_pix_fmt, v4l_print_ext_pix_format, 0), > + IOCTL_INFO(VIDIOC_S_EXT_PIX_FMT, v4l_s_ext_pix_fmt, v4l_print_ext_pix_format, INFO_FL_PRIO), > + IOCTL_INFO(VIDIOC_TRY_EXT_PIX_FMT, v4l_try_ext_pix_fmt, v4l_print_ext_pix_format, 0), > }; > #define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls) > > diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h > index edb733f21604..c44708dc9355 100644 > --- a/include/media/v4l2-ioctl.h > +++ b/include/media/v4l2-ioctl.h > @@ -48,11 +48,17 @@ struct v4l2_fh; > * @vidioc_g_fmt_vid_cap: pointer to the function that implements > * :ref:`VIDIOC_G_FMT <vidioc_g_fmt>` ioctl logic for video capture > * in single plane mode > + * @vidioc_g_ext_pix_fmt_vid_cap: pointer to the function that implements > + * :ref:`VIDIOC_G_EXT_PIX_FMT <vidioc_g_ext_pix_fmt>` ioctl logic for video > + * capture > * @vidioc_g_fmt_vid_overlay: pointer to the function that implements > * :ref:`VIDIOC_G_FMT <vidioc_g_fmt>` ioctl logic for video overlay > * @vidioc_g_fmt_vid_out: pointer to the function that implements > * :ref:`VIDIOC_G_FMT <vidioc_g_fmt>` ioctl logic for video out > * in single plane mode > + * @vidioc_g_ext_pix_fmt_vid_out: pointer to the function that implements > + * :ref:`VIDIOC_G_EXT_PIX_FMT <vidioc_g_ext_pix_fmt>` ioctl logic for video > + * out > * @vidioc_g_fmt_vid_out_overlay: pointer to the function that implements > * :ref:`VIDIOC_G_FMT <vidioc_g_fmt>` ioctl logic for video overlay output > * @vidioc_g_fmt_vbi_cap: pointer to the function that implements > @@ -82,11 +88,16 @@ struct v4l2_fh; > * @vidioc_s_fmt_vid_cap: pointer to the function that implements > * :ref:`VIDIOC_S_FMT <vidioc_g_fmt>` ioctl logic for video capture > * in single plane mode > + * @vidioc_s_ext_pix_fmt_vid_cap: pointer to the function that implements > + * :ref:`VIDIOC_S_EXT_PIX_FMT <vidioc_g_ext_pix_fmt>` ioctl logic for video > + * capture > * @vidioc_s_fmt_vid_overlay: pointer to the function that implements > * :ref:`VIDIOC_S_FMT <vidioc_g_fmt>` ioctl logic for video overlay > * @vidioc_s_fmt_vid_out: pointer to the function that implements > * :ref:`VIDIOC_S_FMT <vidioc_g_fmt>` ioctl logic for video out > * in single plane mode > + * @vidioc_s_ext_pix_fmt_vid_out: pointer to the function that implements > + * :ref:`VIDIOC_S_EXT_PIX_FMT <vidioc_g_fmt>` ioctl logic for video out > * @vidioc_s_fmt_vid_out_overlay: pointer to the function that implements > * :ref:`VIDIOC_S_FMT <vidioc_g_fmt>` ioctl logic for video overlay output > * @vidioc_s_fmt_vbi_cap: pointer to the function that implements > @@ -116,11 +127,16 @@ struct v4l2_fh; > * @vidioc_try_fmt_vid_cap: pointer to the function that implements > * :ref:`VIDIOC_TRY_FMT <vidioc_g_fmt>` ioctl logic for video capture > * in single plane mode > + * @vidioc_try_ext_pix_fmt_vid_cap: pointer to the function that implements > + * :ref:`VIDIOC_TRY_EXT_PIX_FMT <vidioc_g_ext_pix_fmt>` ioctl logic for > + video capture > * @vidioc_try_fmt_vid_overlay: pointer to the function that implements > * :ref:`VIDIOC_TRY_FMT <vidioc_g_fmt>` ioctl logic for video overlay > * @vidioc_try_fmt_vid_out: pointer to the function that implements > * :ref:`VIDIOC_TRY_FMT <vidioc_g_fmt>` ioctl logic for video out > * in single plane mode > + * @vidioc_try_ext_pix_fmt_vid_out: pointer to the function that implements > + * :ref:`VIDIOC_TRY_EXT_PIX_FMT <vidioc_g_fmt>` ioctl logic for video out > * @vidioc_try_fmt_vid_out_overlay: pointer to the function that implements > * :ref:`VIDIOC_TRY_FMT <vidioc_g_fmt>` ioctl logic for video overlay > * output > @@ -319,10 +335,14 @@ struct v4l2_ioctl_ops { > /* VIDIOC_G_FMT handlers */ > int (*vidioc_g_fmt_vid_cap)(struct file *file, void *fh, > struct v4l2_format *f); > + int (*vidioc_g_ext_pix_fmt_vid_cap)(struct file *file, void *fh, > + struct v4l2_ext_pix_format *ef); > int (*vidioc_g_fmt_vid_overlay)(struct file *file, void *fh, > struct v4l2_format *f); > int (*vidioc_g_fmt_vid_out)(struct file *file, void *fh, > struct v4l2_format *f); > + int (*vidioc_g_ext_pix_fmt_vid_out)(struct file *file, void *fh, > + struct v4l2_ext_pix_format *ef); > int (*vidioc_g_fmt_vid_out_overlay)(struct file *file, void *fh, > struct v4l2_format *f); > int (*vidioc_g_fmt_vbi_cap)(struct file *file, void *fh, > @@ -349,10 +369,14 @@ struct v4l2_ioctl_ops { > /* VIDIOC_S_FMT handlers */ > int (*vidioc_s_fmt_vid_cap)(struct file *file, void *fh, > struct v4l2_format *f); > + int (*vidioc_s_ext_pix_fmt_vid_cap)(struct file *file, void *fh, > + struct v4l2_ext_pix_format *ef); > int (*vidioc_s_fmt_vid_overlay)(struct file *file, void *fh, > struct v4l2_format *f); > int (*vidioc_s_fmt_vid_out)(struct file *file, void *fh, > struct v4l2_format *f); > + int (*vidioc_s_ext_pix_fmt_vid_out)(struct file *file, void *fh, > + struct v4l2_ext_pix_format *ef); > int (*vidioc_s_fmt_vid_out_overlay)(struct file *file, void *fh, > struct v4l2_format *f); > int (*vidioc_s_fmt_vbi_cap)(struct file *file, void *fh, > @@ -379,10 +403,14 @@ struct v4l2_ioctl_ops { > /* VIDIOC_TRY_FMT handlers */ > int (*vidioc_try_fmt_vid_cap)(struct file *file, void *fh, > struct v4l2_format *f); > + int (*vidioc_try_ext_pix_fmt_vid_cap)(struct file *file, void *fh, > + struct v4l2_ext_pix_format *ef); > int (*vidioc_try_fmt_vid_overlay)(struct file *file, void *fh, > struct v4l2_format *f); > int (*vidioc_try_fmt_vid_out)(struct file *file, void *fh, > struct v4l2_format *f); > + int (*vidioc_try_ext_pix_fmt_vid_out)(struct file *file, void *fh, > + struct v4l2_ext_pix_format *ef); > int (*vidioc_try_fmt_vid_out_overlay)(struct file *file, void *fh, > struct v4l2_format *f); > int (*vidioc_try_fmt_vbi_cap)(struct file *file, void *fh, > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h > index 17a9b975177a..74a8dd7f7637 100644 > --- a/include/uapi/linux/videodev2.h > +++ b/include/uapi/linux/videodev2.h > @@ -840,7 +840,8 @@ struct v4l2_fmtdesc { > __u8 description[32]; /* Description string */ > __u32 pixelformat; /* Format fourcc */ > __u32 mbus_code; /* Media bus code */ > - __u32 reserved[3]; > + __u64 modifier; /* Format modifier */ How would that work? Would the driver expose multiple entries for the same pixelformat, just with different modifier? > + __u32 reserved; > }; > > #define V4L2_FMT_FLAG_COMPRESSED 0x0001 > @@ -2417,6 +2418,45 @@ struct v4l2_format { > } fmt; > }; > > +/** > + * struct v4l2_ext_pix_format - extended multiplanar format definition > + * @type: enum v4l2_buf_type; type of the data stream > + * @width: image width in pixels > + * @height: image height in pixels > + * @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) as defined in drm_fourcc.h > + * @field: enum v4l2_field; field order (for interlaced video) > + * @colorspace: enum v4l2_colorspace; supplemental to pixelformat > + * @plane_fmt: per-plane information > + * @flags: format flags (V4L2_PIX_FMT_FLAG_*) > + * @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: drivers and applications must zero this array > + */ > +struct v4l2_ext_pix_format { > + __u32 type; > + __u32 width; > + __u32 height; > + __u32 pixelformat; > + __u64 modifier; > + __u32 field; > + __u32 colorspace; > + > + struct v4l2_plane_pix_format plane_fmt[VIDEO_MAX_PLANES]; > + __u8 flags; > + union { nit: Spurious space before "union". Best regards, Tomasz > + __u8 ycbcr_enc; > + __u8 hsv_enc; > + }; > + __u8 quantization; > + __u8 xfer_func; > + __u32 reserved[10]; > +} __attribute__ ((packed)); > + > /* Stream type-dependent parameters > */ > struct v4l2_streamparm { > @@ -2690,6 +2730,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! */ > > -- > 2.17.1 >
On 7/13/23 18:26, Tomasz Figa wrote: > CAUTION: Email originated externally, do not click links or open attachments unless you recognize the sender and know the content is safe. > > > On Mon, Feb 06, 2023 at 12:33:00PM +0800, ayaka wrote: >> From: Randy Li <ayaka@soulik.info> >> >> 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_pix_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> >> Signed-off-by: Randy Li <ayaka@soulik.info> >> --- >> >> Changes in v7: >> - use the resevered fields in v4l2_fmtdesc to store the modifier for format >> enumerate. >> - Dropping the old ioctl() calling new interfaces adapter. > > Why? > > We need backwards compatibility with the legacy API, but requiring > drivers to implement both will add even more boiler plate code to the > drivers and potential for each driver to have a buggy implementation in > its own way. (From my experience, every driver already handles the > TRY_FMT/S_FMT operations in its own slightly buggy way.) Then we would continues on dirtying the fourcc namespace. I think the whole point of this extend API is to trying not squeeze those tiling formats into a small 32bits namespace. > > What we probably don't need is core code for making old drivers > automatically support the new API (i.e. the other way around). Make this wrapper layer in kernel to save application from keepping the legacy code. I really want people dropping a v4l2 fourcc pixel format, using drm pixel formats instead. The rest fixup would be considered in the new version of this series. We can > have the drivers modified to the new API whenever one needs to use the > new features with the old hardware. (And possibly other modernization of > the driver code could happen at the same time.) > >> - Refresh the patch >> Changes in v6: >> The main change here was fixing the conversion, so planes reflects color planes, >> and to implement this properly I made major refactors compared to the previous >> version. >> - struct v4l2_plane_ext_pix_format removed, using struct v4l2_plane_pix_format instead (Tomasz) >> - refer to drm_fourcc.h in struct v4l2_ext_pix_format docs (Hans) >> - reorder colorimetry fields in struct v4l2_ext_pix_format (Hans) >> - do not set Ext ioctls as valid for vid_out_overlay (Tomasz) >> - refactor conversion functions, so planes are color planes (Tomasz) >> - Don't explicitly check for e->modifier != 0 in v4l2_ext_pix_format_to_format() (Tomasz) >> - Use "ef" for extended formats in the framework for consistency (Tomasz) >> - Handle xfer_func field in conversions (Tomasz) >> - Zero reserved fields in v4l_s_ext_pix_fmt() and v4l_try_ext_pix_fmt() (Tomasz) >> - Refactor format functions to use v4l_fmt_ioctl_via_ext() >> - Several fixes/refactoring/changes >> - Remove EXT API for touch devices >> >> 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 | 13 + >> drivers/media/v4l2-core/v4l2-ioctl.c | 357 ++++++++++++++++++++++++++- >> include/media/v4l2-ioctl.h | 28 +++ >> include/uapi/linux/videodev2.h | 46 +++- >> 4 files changed, 438 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c >> index 397d553177fa..cfe90bfd47f1 100644 >> --- a/drivers/media/v4l2-core/v4l2-dev.c >> +++ b/drivers/media/v4l2-core/v4l2-dev.c >> @@ -615,6 +615,10 @@ static void determine_valid_ioctls(struct video_device *vdev) >> ops->vidioc_g_fmt_vid_out_mplane || >> ops->vidioc_g_fmt_vid_out_overlay))) >> __set_bit(_IOC_NR(VIDIOC_G_FMT), valid_ioctls); >> + if ((is_rx && ops->vidioc_g_ext_pix_fmt_vid_cap) || >> + (is_tx && ops->vidioc_g_ext_pix_fmt_vid_out)) { >> + __set_bit(_IOC_NR(VIDIOC_G_EXT_PIX_FMT), valid_ioctls); >> + } >> if ((is_rx && (ops->vidioc_s_fmt_vid_cap || >> ops->vidioc_s_fmt_vid_cap_mplane || >> ops->vidioc_s_fmt_vid_overlay)) || >> @@ -622,6 +626,11 @@ static void determine_valid_ioctls(struct video_device *vdev) >> ops->vidioc_s_fmt_vid_out_mplane || >> ops->vidioc_s_fmt_vid_out_overlay))) >> __set_bit(_IOC_NR(VIDIOC_S_FMT), valid_ioctls); >> + >> + if ((is_rx && ops->vidioc_s_ext_pix_fmt_vid_cap) || >> + (is_tx && ops->vidioc_s_ext_pix_fmt_vid_out)) { >> + __set_bit(_IOC_NR(VIDIOC_S_EXT_PIX_FMT), valid_ioctls); >> + } >> if ((is_rx && (ops->vidioc_try_fmt_vid_cap || >> ops->vidioc_try_fmt_vid_cap_mplane || >> ops->vidioc_try_fmt_vid_overlay)) || >> @@ -629,6 +638,10 @@ static void determine_valid_ioctls(struct video_device *vdev) >> ops->vidioc_try_fmt_vid_out_mplane || >> ops->vidioc_try_fmt_vid_out_overlay))) >> __set_bit(_IOC_NR(VIDIOC_TRY_FMT), valid_ioctls); >> + if ((is_rx && ops->vidioc_try_ext_pix_fmt_vid_cap) || >> + (is_tx && ops->vidioc_try_ext_pix_fmt_vid_out)) { >> + __set_bit(_IOC_NR(VIDIOC_TRY_EXT_PIX_FMT), valid_ioctls); >> + } >> SET_VALID_IOCTL(ops, VIDIOC_OVERLAY, vidioc_overlay); >> SET_VALID_IOCTL(ops, VIDIOC_G_FBUF, vidioc_g_fbuf); >> SET_VALID_IOCTL(ops, VIDIOC_S_FBUF, vidioc_s_fbuf); >> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c >> index 87f163a89c80..52b77a968bb3 100644 >> --- a/drivers/media/v4l2-core/v4l2-ioctl.c >> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c >> @@ -18,8 +18,10 @@ >> >> #include <linux/v4l2-subdev.h> >> #include <linux/videodev2.h> >> +#include <drm/drm_fourcc.h> >> >> #include <media/media-device.h> /* for media_set_bus_info() */ >> + >> #include <media/v4l2-common.h> >> #include <media/v4l2-ioctl.h> >> #include <media/v4l2-ctrls.h> >> @@ -34,6 +36,11 @@ >> >> #define is_valid_ioctl(vfd, cmd) test_bit(_IOC_NR(cmd), (vfd)->valid_ioctls) >> >> +#define V4L2_IS_CAP_MULTIPLANAR(vdev) (vdev->device_caps & \ >> + (V4L2_CAP_VIDEO_CAPTURE_MPLANE | \ >> + V4L2_CAP_VIDEO_OUTPUT_MPLANE | \ >> + V4L2_CAP_VIDEO_M2M_MPLANE)) >> + >> struct std_descr { >> v4l2_std_id std; >> const char *descr; >> @@ -353,6 +360,27 @@ static void v4l_print_format(const void *arg, bool write_only) >> } >> } >> >> +static void v4l_print_ext_pix_format(const void *arg, bool write_only) >> +{ >> + const struct v4l2_ext_pix_format *ef = arg; >> + unsigned int i; >> + >> + pr_cont("type=%s, width=%u, height=%u, format=%c%c%c%c, modifier %llx, field=%s, colorspace=%d, ycbcr_enc=%u, quantization=%u, xfer_func=%u\n", >> + prt_names(ef->type, v4l2_type_names), >> + ef->width, ef->height, >> + (ef->pixelformat & 0xff), >> + (ef->pixelformat >> 8) & 0xff, >> + (ef->pixelformat >> 16) & 0xff, >> + (ef->pixelformat >> 24) & 0xff, >> + ef->modifier, prt_names(ef->field, v4l2_field_names), >> + ef->colorspace, ef->ycbcr_enc, >> + ef->quantization, ef->xfer_func); >> + for (i = 0; i < VIDEO_MAX_PLANES && ef->plane_fmt[i].sizeimage; i++) >> + pr_debug("plane %u: bytesperline=%u sizeimage=%u\n", >> + i, ef->plane_fmt[i].bytesperline, >> + ef->plane_fmt[i].sizeimage); >> +} >> + >> static void v4l_print_framebuffer(const void *arg, bool write_only) >> { >> const struct v4l2_framebuffer *p = arg; >> @@ -940,7 +968,9 @@ static int check_fmt(struct file *file, enum v4l2_buf_type type) >> switch (type) { >> case V4L2_BUF_TYPE_VIDEO_CAPTURE: >> if ((is_vid || is_tch) && is_rx && >> - (ops->vidioc_g_fmt_vid_cap || ops->vidioc_g_fmt_vid_cap_mplane)) >> + (ops->vidioc_g_fmt_vid_cap || >> + ops->vidioc_g_ext_pix_fmt_vid_cap || >> + ops->vidioc_g_fmt_vid_cap_mplane)) >> return 0; >> break; >> case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE: >> @@ -953,7 +983,9 @@ static int check_fmt(struct file *file, enum v4l2_buf_type type) >> break; >> case V4L2_BUF_TYPE_VIDEO_OUTPUT: >> if (is_vid && is_tx && >> - (ops->vidioc_g_fmt_vid_out || ops->vidioc_g_fmt_vid_out_mplane)) >> + (ops->vidioc_g_fmt_vid_out || >> + ops->vidioc_g_ext_pix_fmt_vid_out || >> + ops->vidioc_g_fmt_vid_out_mplane)) >> return 0; >> break; >> case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE: >> @@ -1082,6 +1114,203 @@ static void v4l_sanitize_format(struct v4l2_format *fmt) >> } >> } >> >> +static void >> +v4l2_ext_pix_format_to_pix_format(const struct v4l2_ext_pix_format *ef, >> + struct v4l2_pix_format *pix) >> +{ >> + unsigned int i; >> + >> + pix->width = ef->width; >> + pix->height = ef->height; >> + pix->field = ef->field; >> + pix->flags = V4L2_PIX_FMT_FLAG_SET_CSC; >> + pix->colorspace = ef->colorspace; >> + pix->ycbcr_enc = ef->ycbcr_enc; >> + pix->priv = V4L2_PIX_FMT_PRIV_MAGIC; >> + pix->quantization = ef->quantization; >> + pix->pixelformat = ef->pixelformat; >> + pix->bytesperline = ef->plane_fmt[0].bytesperline; >> + pix->sizeimage = ef->plane_fmt[0].sizeimage; >> + for (i = 1; i < VIDEO_MAX_PLANES && ef->plane_fmt[i].sizeimage; i++) >> + pix->sizeimage += ef->plane_fmt[i].sizeimage; >> +} >> + >> +static void >> +v4l2_ext_pix_format_to_pix_mp_format(const struct v4l2_ext_pix_format *ef, >> + struct v4l2_pix_format_mplane *pix_mp) >> +{ >> + const struct v4l2_format_info *info = >> + v4l2_format_info(ef->pixelformat); >> + unsigned int i; >> + >> + pix_mp->width = ef->width; >> + pix_mp->height = ef->height; >> + pix_mp->field = ef->field; >> + pix_mp->flags = V4L2_PIX_FMT_FLAG_SET_CSC; >> + pix_mp->colorspace = ef->colorspace; >> + pix_mp->ycbcr_enc = ef->ycbcr_enc; >> + pix_mp->quantization = ef->quantization; >> + pix_mp->pixelformat = ef->pixelformat; >> + >> + /* This is true when converting to non-M-variant */ >> + if (info && info->mem_planes == 1) { >> + pix_mp->plane_fmt[0] = ef->plane_fmt[0]; >> + pix_mp->num_planes = 1; >> + for (i = 1; i < VIDEO_MAX_PLANES && ef->plane_fmt[i].sizeimage; i++) >> + pix_mp->plane_fmt[0].sizeimage += ef->plane_fmt[i].sizeimage; >> + >> + return; >> + } >> + >> + for (i = 0; i < VIDEO_MAX_PLANES && ef->plane_fmt[i].sizeimage; i++) >> + pix_mp->plane_fmt[i] = ef->plane_fmt[i]; >> + pix_mp->num_planes = i; >> +} >> + >> +/* >> + * v4l2_ext_pix_format_to_format - convert to v4l2_ext_pix_format to v4l2_format >> + * >> + * @ef: A pointer to struct struct v4l2_ext_pix_format to be converted. >> + * @f: A pointer to struct v4l2_format to be filled. >> + * @is_mplane: Bool indicating if multiplanar API should be used in @f. >> + * >> + * If pixelformat should be converted to M-variant, change ef->pixelformat >> + * to the M-variant before calling this function. >> + */ >> +static void v4l2_ext_pix_format_to_format(const struct v4l2_ext_pix_format *ef, >> + struct v4l2_format *f, bool is_mplane) >> +{ >> + memset(f, 0, sizeof(*f)); >> + >> + if (ef->modifier != DRM_FORMAT_MOD_LINEAR && >> + ef->modifier != DRM_FORMAT_MOD_INVALID) >> + pr_warn("Modifiers are not supported in v4l2_format, ignoring %llx\n", >> + ef->modifier); >> + >> + if (!is_mplane) { >> + f->type = ef->type; >> + v4l2_ext_pix_format_to_pix_format(ef, &f->fmt.pix); >> + return; >> + } >> + >> + if (ef->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) >> + f->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE; >> + else >> + f->type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE; >> + >> + v4l2_ext_pix_format_to_pix_mp_format(ef, &f->fmt.pix_mp); >> +} >> + >> +static void >> +v4l2_pix_format_to_ext_pix_format(const struct v4l2_pix_format *pix, >> + struct v4l2_ext_pix_format *ef) >> +{ >> + const struct v4l2_format_info *info = >> + v4l2_format_info(pix->pixelformat); >> + unsigned int i; >> + >> + ef->width = pix->width; >> + ef->height = pix->height; >> + ef->field = pix->field; >> + ef->colorspace = pix->colorspace; >> + ef->ycbcr_enc = pix->ycbcr_enc; >> + ef->quantization = pix->quantization; >> + ef->xfer_func = pix->xfer_func; >> + if (pix->flags) >> + pr_warn("Ignoring pixelformat flags 0x%x\n", pix->flags); >> + >> + /* We assume M-variants won't be used in this function */ >> + ef->pixelformat = pix->pixelformat; >> + >> + ef->plane_fmt[0].bytesperline = pix->bytesperline; >> + ef->plane_fmt[0].sizeimage = pix->sizeimage; >> + >> + if (!info) >> + return; > > Should we print a warning in case of !info? > >> + >> + for (i = 1; i < info->comp_planes; i++) { >> + ef->plane_fmt[i].bytesperline = pix->bytesperline / info->hdiv; >> + ef->plane_fmt[i].sizeimage = ef->plane_fmt[i].bytesperline * >> + ef->height / info->vdiv; >> + ef->plane_fmt[0].sizeimage -= ef->plane_fmt[i].sizeimage; >> + } >> +} >> + >> +static void >> +v4l2_pix_mp_format_to_ext_pix_format(const struct v4l2_pix_format_mplane *pix_mp, >> + struct v4l2_ext_pix_format *ef) >> +{ >> + const struct v4l2_format_info *info = >> + v4l2_format_info(pix_mp->pixelformat); >> + unsigned int i; >> + >> + ef->width = pix_mp->width; >> + ef->height = pix_mp->height; >> + ef->field = pix_mp->field; >> + ef->colorspace = pix_mp->colorspace; >> + ef->ycbcr_enc = pix_mp->ycbcr_enc; >> + ef->quantization = pix_mp->quantization; >> + ef->xfer_func = pix_mp->xfer_func; >> + if (pix_mp->flags) >> + pr_warn("Ignoring pixelformat flags 0x%x\n", pix_mp->flags); >> + >> + if (!info) >> + return; > > If we return here, the returned ef will not make any sense, because it > would have uninitialized plane_fmt[]. Should we print a warning here too? > >> + >> + ef->pixelformat = pix_mp->pixelformat; >> + >> + if (info->comp_planes == info->mem_planes) { >> + for (i = 0; i < pix_mp->num_planes && i < VIDEO_MAX_PLANES; i++) >> + ef->plane_fmt[i] = pix_mp->plane_fmt[i]; >> + >> + return; >> + } >> + >> + /* case where mem_planes is 1 and comp_planes > 1 */ >> + ef->plane_fmt[0] = pix_mp->plane_fmt[0]; >> + for (i = 1; i < info->comp_planes; i++) { >> + ef->plane_fmt[i].bytesperline = >> + pix_mp->plane_fmt[0].bytesperline / info->hdiv; >> + ef->plane_fmt[i].sizeimage = >> + ef->plane_fmt[i].bytesperline * ef->height / info->vdiv; >> + ef->plane_fmt[0].sizeimage -= ef->plane_fmt[i].sizeimage; >> + } >> +} >> + >> +/* >> + * v4l2_format_to_ext_pix_format - convert to v4l2_format to v4l2_ext_pix_format >> + * >> + * @f: A pointer to struct v4l2_format to be converted. >> + * @ef: A pointer to struct struct v4l2_ext_pix_format to be filled. >> + * >> + * This method normalize the pixelformat to non-M variant. >> + */ >> +static void v4l2_format_to_ext_pix_format(const struct v4l2_format *f, >> + struct v4l2_ext_pix_format *ef) >> +{ >> + memset(ef, 0, sizeof(*ef)); >> + >> + switch (f->type) { >> + case V4L2_BUF_TYPE_VIDEO_CAPTURE: >> + case V4L2_BUF_TYPE_VIDEO_OUTPUT: >> + ef->type = f->type; >> + v4l2_pix_format_to_ext_pix_format(&f->fmt.pix, ef); >> + break; >> + case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE: >> + ef->type = V4L2_BUF_TYPE_VIDEO_CAPTURE; >> + v4l2_pix_mp_format_to_ext_pix_format(&f->fmt.pix_mp, ef); >> + break; >> + case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE: >> + ef->type = V4L2_BUF_TYPE_VIDEO_OUTPUT; >> + v4l2_pix_mp_format_to_ext_pix_format(&f->fmt.pix_mp, ef); >> + break; >> + default: >> + WARN("Converting to Ext Pix Format with wrong buffer type %s\n", >> + prt_names(f->type, v4l2_type_names)); >> + break; >> + } >> +} >> + >> static int v4l_querycap(const struct v4l2_ioctl_ops *ops, >> struct file *file, void *fh, void *arg) >> { >> @@ -1654,7 +1883,9 @@ static int v4l_g_fmt(const struct v4l2_ioctl_ops *ops, >> v4l_pix_format_touch(&p->fmt.pix); >> return ret; >> case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE: >> - return ops->vidioc_g_fmt_vid_cap_mplane(file, fh, arg); >> + if (ops->vidioc_g_fmt_vid_cap_mplane) >> + return ops->vidioc_g_fmt_vid_cap_mplane(file, fh, arg); > > What is this change needed for? > >> + break; >> case V4L2_BUF_TYPE_VIDEO_OVERLAY: >> return ops->vidioc_g_fmt_vid_overlay(file, fh, arg); >> case V4L2_BUF_TYPE_VBI_CAPTURE: >> @@ -1662,7 +1893,8 @@ static int v4l_g_fmt(const struct v4l2_ioctl_ops *ops, >> case V4L2_BUF_TYPE_SLICED_VBI_CAPTURE: >> return ops->vidioc_g_fmt_sliced_vbi_cap(file, fh, arg); >> case V4L2_BUF_TYPE_VIDEO_OUTPUT: >> - if (unlikely(!ops->vidioc_g_fmt_vid_out)) >> + if (unlikely(!ops->vidioc_g_fmt_vid_out && >> + !ops->vidioc_g_ext_pix_fmt_vid_out)) > > Ditto. > >> break; >> p->fmt.pix.priv = V4L2_PIX_FMT_PRIV_MAGIC; >> ret = ops->vidioc_g_fmt_vid_out(file, fh, arg); >> @@ -1670,7 +1902,9 @@ static int v4l_g_fmt(const struct v4l2_ioctl_ops *ops, >> p->fmt.pix.priv = V4L2_PIX_FMT_PRIV_MAGIC; >> return ret; >> case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE: >> - return ops->vidioc_g_fmt_vid_out_mplane(file, fh, arg); >> + if (ops->vidioc_g_fmt_vid_out_mplane) >> + return ops->vidioc_g_fmt_vid_out_mplane(file, fh, arg); >> + break; > > Ditto. > >> case V4L2_BUF_TYPE_VIDEO_OUTPUT_OVERLAY: >> return ops->vidioc_g_fmt_vid_out_overlay(file, fh, arg); >> case V4L2_BUF_TYPE_VBI_OUTPUT: >> @@ -1689,6 +1923,42 @@ static int v4l_g_fmt(const struct v4l2_ioctl_ops *ops, >> return -EINVAL; >> } >> >> +static int v4l_g_ext_pix_fmt(const struct v4l2_ioctl_ops *ops, >> + struct file *file, void *fh, void *arg) >> +{ >> + struct v4l2_ext_pix_format *ef = arg; >> + struct v4l2_format f = { >> + .type = ef->type, >> + }; >> + int ret = check_fmt(file, ef->type); >> + >> + if (ret) >> + return ret; >> + >> + memset(ef, 0, sizeof(*ef)); >> + ef->type = f.type; >> + >> + switch (f.type) { >> + case V4L2_BUF_TYPE_VIDEO_CAPTURE: >> + if (ops->vidioc_g_ext_pix_fmt_vid_cap) >> + return ops->vidioc_g_ext_pix_fmt_vid_cap(file, fh, ef); >> + break; >> + case V4L2_BUF_TYPE_VIDEO_OUTPUT: >> + if (ops->vidioc_g_ext_pix_fmt_vid_out) >> + return ops->vidioc_g_ext_pix_fmt_vid_out(file, fh, ef); >> + break; >> + default: >> + return -EINVAL; >> + } >> + > > Given that determine_valid_ioctls() will only set the valid ioctl bit if > the right operation is provided by the driver, when would the code below > be run? > >> + ret = v4l_g_fmt(ops, file, fh, &f); >> + if (ret) >> + return ret; >> + >> + v4l2_format_to_ext_pix_format(&f, ef); >> + return 0; >> +} >> + >> static int v4l_s_fmt(const struct v4l2_ioctl_ops *ops, >> struct file *file, void *fh, void *arg) >> { >> @@ -1794,6 +2064,42 @@ static int v4l_s_fmt(const struct v4l2_ioctl_ops *ops, >> return -EINVAL; >> } >> >> +static int v4l_s_ext_pix_fmt(const struct v4l2_ioctl_ops *ops,() >> + struct file *file, void *fh, void *arg) >> +{ >> + struct video_device *vfd = video_devdata(file); >> + struct v4l2_ext_pix_format *ef = arg; >> + struct v4l2_format f; >> + int ret = check_fmt(file, ef->type); >> + >> + if (ret) >> + return ret; >> + >> + memset(ef->reserved, 0, sizeof(ef->reserved)); >> + >> + switch (ef->type) { >> + case V4L2_BUF_TYPE_VIDEO_CAPTURE: >> + if (ops->vidioc_s_ext_pix_fmt_vid_cap) >> + return ops->vidioc_s_ext_pix_fmt_vid_cap(file, fh, ef); >> + break; >> + case V4L2_BUF_TYPE_VIDEO_OUTPUT: >> + if (ops->vidioc_s_ext_pix_fmt_vid_out) >> + return ops->vidioc_s_ext_pix_fmt_vid_out(file, fh, ef); >> + break; >> + default: >> + return -EINVAL; >> + } >> + > > Ditto. > >> + v4l2_ext_pix_format_to_format(ef, &f, V4L2_IS_CAP_MULTIPLANAR(vfd)); >> + >> + ret = v4l_s_fmt(ops, file, fh, &f); >> + if (ret) >> + return ret; >> + >> + v4l2_format_to_ext_pix_format(&f, ef); >> + return 0; >> +} >> + >> static int v4l_try_fmt(const struct v4l2_ioctl_ops *ops, >> struct file *file, void *fh, void *arg) >> { >> @@ -1896,6 +2202,44 @@ static int v4l_try_fmt(const struct v4l2_ioctl_ops *ops, >> return -EINVAL; >> } >> >> +static int v4l_try_ext_pix_fmt(const struct v4l2_ioctl_ops *ops, >> + struct file *file, void *fh, void *arg) >> +{ >> + struct video_device *vfd = video_devdata(file); >> + struct v4l2_ext_pix_format *ef = arg; >> + struct v4l2_format f; >> + int ret = check_fmt(file, ef->type); >> + >> + if (ret) >> + return ret; >> + >> + memset(ef->reserved, 0, sizeof(ef->reserved)); >> + >> + switch (ef->type) { >> + case V4L2_BUF_TYPE_VIDEO_CAPTURE: >> + if (ops->vidioc_try_ext_pix_fmt_vid_cap) >> + return ops->vidioc_try_ext_pix_fmt_vid_cap(file, fh, >> + ef); >> + break; >> + case V4L2_BUF_TYPE_VIDEO_OUTPUT: >> + if (ops->vidioc_try_ext_pix_fmt_vid_out) >> + return ops->vidioc_try_ext_pix_fmt_vid_out(file, fh, >> + ef); >> + break; >> + default: >> + return -EINVAL; >> + } > > Ditto. > >> + >> + v4l2_ext_pix_format_to_format(ef, &f, V4L2_IS_CAP_MULTIPLANAR(vfd)); >> + >> + ret = v4l_try_fmt(ops, file, fh, &f); >> + if (ret) >> + return ret; >> + >> + v4l2_format_to_ext_pix_format(&f, ef); >> + return 0; >> +} >> + >> static int v4l_streamon(const struct v4l2_ioctl_ops *ops, >> struct file *file, void *fh, void *arg) >> { >> @@ -2902,6 +3246,9 @@ static const struct v4l2_ioctl_info v4l2_ioctls[] = { >> IOCTL_INFO(VIDIOC_ENUM_FREQ_BANDS, v4l_enum_freq_bands, v4l_print_freq_band, 0), >> IOCTL_INFO(VIDIOC_DBG_G_CHIP_INFO, v4l_dbg_g_chip_info, v4l_print_dbg_chip_info, INFO_FL_CLEAR(v4l2_dbg_chip_info, match)), >> IOCTL_INFO(VIDIOC_QUERY_EXT_CTRL, v4l_query_ext_ctrl, v4l_print_query_ext_ctrl, INFO_FL_CTRL | INFO_FL_CLEAR(v4l2_query_ext_ctrl, id)), >> + IOCTL_INFO(VIDIOC_G_EXT_PIX_FMT, v4l_g_ext_pix_fmt, v4l_print_ext_pix_format, 0), >> + IOCTL_INFO(VIDIOC_S_EXT_PIX_FMT, v4l_s_ext_pix_fmt, v4l_print_ext_pix_format, INFO_FL_PRIO), >> + IOCTL_INFO(VIDIOC_TRY_EXT_PIX_FMT, v4l_try_ext_pix_fmt, v4l_print_ext_pix_format, 0), >> }; >> #define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls) >> >> diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h >> index edb733f21604..c44708dc9355 100644 >> --- a/include/media/v4l2-ioctl.h >> +++ b/include/media/v4l2-ioctl.h >> @@ -48,11 +48,17 @@ struct v4l2_fh; >> * @vidioc_g_fmt_vid_cap: pointer to the function that implements >> * :ref:`VIDIOC_G_FMT <vidioc_g_fmt>` ioctl logic for video capture >> * in single plane mode >> + * @vidioc_g_ext_pix_fmt_vid_cap: pointer to the function that implements >> + * :ref:`VIDIOC_G_EXT_PIX_FMT <vidioc_g_ext_pix_fmt>` ioctl logic for video >> + * capture >> * @vidioc_g_fmt_vid_overlay: pointer to the function that implements >> * :ref:`VIDIOC_G_FMT <vidioc_g_fmt>` ioctl logic for video overlay >> * @vidioc_g_fmt_vid_out: pointer to the function that implements >> * :ref:`VIDIOC_G_FMT <vidioc_g_fmt>` ioctl logic for video out >> * in single plane mode >> + * @vidioc_g_ext_pix_fmt_vid_out: pointer to the function that implements >> + * :ref:`VIDIOC_G_EXT_PIX_FMT <vidioc_g_ext_pix_fmt>` ioctl logic for video >> + * out >> * @vidioc_g_fmt_vid_out_overlay: pointer to the function that implements >> * :ref:`VIDIOC_G_FMT <vidioc_g_fmt>` ioctl logic for video overlay output >> * @vidioc_g_fmt_vbi_cap: pointer to the function that implements >> @@ -82,11 +88,16 @@ struct v4l2_fh; >> * @vidioc_s_fmt_vid_cap: pointer to the function that implements >> * :ref:`VIDIOC_S_FMT <vidioc_g_fmt>` ioctl logic for video capture >> * in single plane mode >> + * @vidioc_s_ext_pix_fmt_vid_cap: pointer to the function that implements >> + * :ref:`VIDIOC_S_EXT_PIX_FMT <vidioc_g_ext_pix_fmt>` ioctl logic for video >> + * capture >> * @vidioc_s_fmt_vid_overlay: pointer to the function that implements >> * :ref:`VIDIOC_S_FMT <vidioc_g_fmt>` ioctl logic for video overlay >> * @vidioc_s_fmt_vid_out: pointer to the function that implements >> * :ref:`VIDIOC_S_FMT <vidioc_g_fmt>` ioctl logic for video out >> * in single plane mode >> + * @vidioc_s_ext_pix_fmt_vid_out: pointer to the function that implements >> + * :ref:`VIDIOC_S_EXT_PIX_FMT <vidioc_g_fmt>` ioctl logic for video out >> * @vidioc_s_fmt_vid_out_overlay: pointer to the function that implements >> * :ref:`VIDIOC_S_FMT <vidioc_g_fmt>` ioctl logic for video overlay output >> * @vidioc_s_fmt_vbi_cap: pointer to the function that implements >> @@ -116,11 +127,16 @@ struct v4l2_fh; >> * @vidioc_try_fmt_vid_cap: pointer to the function that implements >> * :ref:`VIDIOC_TRY_FMT <vidioc_g_fmt>` ioctl logic for video capture >> * in single plane mode >> + * @vidioc_try_ext_pix_fmt_vid_cap: pointer to the function that implements >> + * :ref:`VIDIOC_TRY_EXT_PIX_FMT <vidioc_g_ext_pix_fmt>` ioctl logic for >> + video capture >> * @vidioc_try_fmt_vid_overlay: pointer to the function that implements >> * :ref:`VIDIOC_TRY_FMT <vidioc_g_fmt>` ioctl logic for video overlay >> * @vidioc_try_fmt_vid_out: pointer to the function that implements >> * :ref:`VIDIOC_TRY_FMT <vidioc_g_fmt>` ioctl logic for video out >> * in single plane mode >> + * @vidioc_try_ext_pix_fmt_vid_out: pointer to the function that implements >> + * :ref:`VIDIOC_TRY_EXT_PIX_FMT <vidioc_g_fmt>` ioctl logic for video out >> * @vidioc_try_fmt_vid_out_overlay: pointer to the function that implements >> * :ref:`VIDIOC_TRY_FMT <vidioc_g_fmt>` ioctl logic for video overlay >> * output >> @@ -319,10 +335,14 @@ struct v4l2_ioctl_ops { >> /* VIDIOC_G_FMT handlers */ >> int (*vidioc_g_fmt_vid_cap)(struct file *file, void *fh, >> struct v4l2_format *f); >> + int (*vidioc_g_ext_pix_fmt_vid_cap)(struct file *file, void *fh, >> + struct v4l2_ext_pix_format *ef); >> int (*vidioc_g_fmt_vid_overlay)(struct file *file, void *fh, >> struct v4l2_format *f); >> int (*vidioc_g_fmt_vid_out)(struct file *file, void *fh, >> struct v4l2_format *f); >> + int (*vidioc_g_ext_pix_fmt_vid_out)(struct file *file, void *fh, >> + struct v4l2_ext_pix_format *ef); >> int (*vidioc_g_fmt_vid_out_overlay)(struct file *file, void *fh, >> struct v4l2_format *f); >> int (*vidioc_g_fmt_vbi_cap)(struct file *file, void *fh, >> @@ -349,10 +369,14 @@ struct v4l2_ioctl_ops { >> /* VIDIOC_S_FMT handlers */ >> int (*vidioc_s_fmt_vid_cap)(struct file *file, void *fh, >> struct v4l2_format *f); >> + int (*vidioc_s_ext_pix_fmt_vid_cap)(struct file *file, void *fh, >> + struct v4l2_ext_pix_format *ef); >> int (*vidioc_s_fmt_vid_overlay)(struct file *file, void *fh, >> struct v4l2_format *f); >> int (*vidioc_s_fmt_vid_out)(struct file *file, void *fh, >> struct v4l2_format *f); >> + int (*vidioc_s_ext_pix_fmt_vid_out)(struct file *file, void *fh, >> + struct v4l2_ext_pix_format *ef); >> int (*vidioc_s_fmt_vid_out_overlay)(struct file *file, void *fh, >> struct v4l2_format *f); >> int (*vidioc_s_fmt_vbi_cap)(struct file *file, void *fh, >> @@ -379,10 +403,14 @@ struct v4l2_ioctl_ops { >> /* VIDIOC_TRY_FMT handlers */ >> int (*vidioc_try_fmt_vid_cap)(struct file *file, void *fh, >> struct v4l2_format *f); >> + int (*vidioc_try_ext_pix_fmt_vid_cap)(struct file *file, void *fh, >> + struct v4l2_ext_pix_format *ef); >> int (*vidioc_try_fmt_vid_overlay)(struct file *file, void *fh, >> struct v4l2_format *f); >> int (*vidioc_try_fmt_vid_out)(struct file *file, void *fh, >> struct v4l2_format *f); >> + int (*vidioc_try_ext_pix_fmt_vid_out)(struct file *file, void *fh, >> + struct v4l2_ext_pix_format *ef); >> int (*vidioc_try_fmt_vid_out_overlay)(struct file *file, void *fh, >> struct v4l2_format *f); >> int (*vidioc_try_fmt_vbi_cap)(struct file *file, void *fh, >> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h >> index 17a9b975177a..74a8dd7f7637 100644 >> --- a/include/uapi/linux/videodev2.h >> +++ b/include/uapi/linux/videodev2.h >> @@ -840,7 +840,8 @@ struct v4l2_fmtdesc { >> __u8 description[32]; /* Description string */ >> __u32 pixelformat; /* Format fourcc */ >> __u32 mbus_code; /* Media bus code */ >> - __u32 reserved[3]; >> + __u64 modifier; /* Format modifier */ > > How would that work? Would the driver expose multiple entries for the > same pixelformat, just with different modifier? > >> + __u32 reserved; >> }; >> >> #define V4L2_FMT_FLAG_COMPRESSED 0x0001 >> @@ -2417,6 +2418,45 @@ struct v4l2_format { >> } fmt; >> }; >> >> +/** >> + * struct v4l2_ext_pix_format - extended multiplanar format definition >> + * @type: enum v4l2_buf_type; type of the data stream >> + * @width: image width in pixels >> + * @height: image height in pixels >> + * @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) as defined in drm_fourcc.h >> + * @field: enum v4l2_field; field order (for interlaced video) >> + * @colorspace: enum v4l2_colorspace; supplemental to pixelformat >> + * @plane_fmt: per-plane information >> + * @flags: format flags (V4L2_PIX_FMT_FLAG_*) >> + * @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: drivers and applications must zero this array >> + */ >> +struct v4l2_ext_pix_format { >> + __u32 type; >> + __u32 width; >> + __u32 height; >> + __u32 pixelformat; >> + __u64 modifier; >> + __u32 field; >> + __u32 colorspace; >> + >> + struct v4l2_plane_pix_format plane_fmt[VIDEO_MAX_PLANES]; >> + __u8 flags; >> + union { > > nit: Spurious space before "union". > > Best regards, > Tomasz > >> + __u8 ycbcr_enc; >> + __u8 hsv_enc; >> + }; >> + __u8 quantization; >> + __u8 xfer_func; >> + __u32 reserved[10]; >> +} __attribute__ ((packed)); >> + >> /* Stream type-dependent parameters >> */ >> struct v4l2_streamparm { >> @@ -2690,6 +2730,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! */ >> >> -- >> 2.17.1 >>
From: "Randy Li" <ayaka@soulik.info> This is the v7 refresh of the previous v6. Except the ext fmt part, I drop backward feedback feature. I didn't changes those ext_buf related patches unless it conflicts during the refresh. v6: https://patchwork.linuxtv.org/project/linux-media/cover/20210114180738.1758707-1-helen.koike@collabora.com/ v5: https://patchwork.linuxtv.org/project/linux-media/cover/20200804192939.2251988-1-helen.koike@collabora.com/ v4: https://patchwork.linuxtv.org/project/linux-media/cover/20200717115435.2632623-1-helen.koike@collabora.com/ v3: https://patchwork.linuxtv.org/cover/59345/ v2: https://patchwork.kernel.org/project/linux-media/list/?series=101153 v1: https://patchwork.kernel.org/project/linux-media/list/?series=93707 Helen Koike (8): media: vivid: Convert to v4l2_ext_pix_format media: v4l2: Add extended buffer (de)queue operations for video types media: videobuf2-v4l2: reorganize flags handling media: videobuf2: Expose helpers for Ext qbuf/dqbuf media: vivid: use vb2_ioctls_ext_{d}qbuf hooks media: vimc: use vb2_ioctls_ext_{d}qbuf hooks media: mediabus: Add helpers to convert a ext_pix format to/from a mbus_fmt media: vimc: Convert to v4l2_ext_pix_format Randy Li (1): media: v4l2: Extend pixel formats to unify single/multi-planar handling (and more) .../media/common/videobuf2/videobuf2-core.c | 46 +- .../media/common/videobuf2/videobuf2-v4l2.c | 499 ++++++++++++---- .../media/test-drivers/vimc/vimc-capture.c | 119 ++-- drivers/media/test-drivers/vimc/vimc-common.c | 6 +- drivers/media/test-drivers/vimc/vimc-common.h | 2 +- drivers/media/test-drivers/vivid/vivid-core.c | 106 +--- .../media/test-drivers/vivid/vivid-vid-cap.c | 203 +++---- .../media/test-drivers/vivid/vivid-vid-cap.h | 15 +- .../media/test-drivers/vivid/vivid-vid-out.c | 198 +++---- .../media/test-drivers/vivid/vivid-vid-out.h | 15 +- drivers/media/v4l2-core/v4l2-dev.c | 17 + drivers/media/v4l2-core/v4l2-ioctl.c | 541 +++++++++++++++++- include/media/v4l2-ioctl.h | 36 ++ include/media/v4l2-mediabus.h | 42 ++ include/media/videobuf2-core.h | 33 +- include/media/videobuf2-v4l2.h | 8 +- include/uapi/linux/videodev2.h | 101 +++- 17 files changed, 1459 insertions(+), 528 deletions(-)