| Message ID | 20190205202417.16555-2-ezequiel@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add MPEG-2 decoding to Rockchip VPU | expand |
Hi Ezequiel, A quick review below. This looks really useful, BTW. On 2/5/19 9:24 PM, Ezequiel Garcia wrote: > Add two new API helpers, v4l2_fill_pixfmt and v4l2_fill_pixfmt_mp, > to be used by drivers to calculate plane sizes and bytes per lines. > > Note that driver-specific paddig and alignment are not paddig -> padding > taken into account, and must be done by drivers using this API. > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > --- > drivers/media/v4l2-core/Makefile | 2 +- > drivers/media/v4l2-core/v4l2-common.c | 71 +++++++++++++++++ > drivers/media/v4l2-core/v4l2-fourcc.c | 109 ++++++++++++++++++++++++++ > include/media/v4l2-common.h | 5 ++ > include/media/v4l2-fourcc.h | 53 +++++++++++++ > 5 files changed, 239 insertions(+), 1 deletion(-) > create mode 100644 drivers/media/v4l2-core/v4l2-fourcc.c > create mode 100644 include/media/v4l2-fourcc.h > > diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile > index 9ee57e1efefe..bc23c3407c17 100644 > --- a/drivers/media/v4l2-core/Makefile > +++ b/drivers/media/v4l2-core/Makefile > @@ -7,7 +7,7 @@ tuner-objs := tuner-core.o > > videodev-objs := v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o \ > v4l2-event.o v4l2-ctrls.o v4l2-subdev.o v4l2-clk.o \ > - v4l2-async.o > + v4l2-async.o v4l2-fourcc.o > ifeq ($(CONFIG_COMPAT),y) > videodev-objs += v4l2-compat-ioctl32.o > endif > diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c > index 50763fb42a1b..39d86a389cae 100644 > --- a/drivers/media/v4l2-core/v4l2-common.c > +++ b/drivers/media/v4l2-core/v4l2-common.c > @@ -61,6 +61,7 @@ > #include <media/v4l2-common.h> > #include <media/v4l2-device.h> > #include <media/v4l2-ctrls.h> > +#include <media/v4l2-fourcc.h> Either create a v4l2-fourcc.c source using this header, or move the contents of v4l2-fourcc.h to v4l2-common.h. Creating a new header but not a new source is a bit weird. > > #include <linux/videodev2.h> > > @@ -455,3 +456,73 @@ int v4l2_s_parm_cap(struct video_device *vdev, > return ret; > } > EXPORT_SYMBOL_GPL(v4l2_s_parm_cap); > + > +void v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt, > + int pixelformat, int width, int height) > +{ > + const struct v4l2_format_info *info; > + struct v4l2_plane_pix_format *plane; > + int i; > + > + info = v4l2_format_info(pixelformat); > + if (!info) > + return; You should return a bool or something to indicate whether or not the pixelformat was known. > + > + pixfmt->width = width; > + pixfmt->height = height; > + pixfmt->pixelformat = pixelformat; > + > + if (!info->multiplanar) { It would make much more sense if multiplanar contained the number of planes to use (i.e. equal to pixfmt->num_planes). See more about this below. > + pixfmt->num_planes = 1; > + plane = &pixfmt->plane_fmt[0]; > + plane->bytesperline = width * info->cpp[0]; > + plane->sizeimage = 0; > + for (i = 0; i < info->num_planes; i++) { > + unsigned int hsub = (i == 0) ? 1 : info->hsub; > + unsigned int vsub = (i == 0) ? 1 : info->vsub; > + > + plane->sizeimage += info->cpp[i] * > + DIV_ROUND_UP(width, hsub) * > + DIV_ROUND_UP(height, vsub); > + } > + } else { > + pixfmt->num_planes = info->num_planes; > + for (i = 0; i < info->num_planes; i++) { > + unsigned int hsub = (i == 0) ? 1 : info->hsub; > + unsigned int vsub = (i == 0) ? 1 : info->vsub; > + > + plane = &pixfmt->plane_fmt[i]; > + plane->bytesperline = > + info->cpp[i] * DIV_ROUND_UP(width, hsub); > + plane->sizeimage = > + plane->bytesperline * DIV_ROUND_UP(height, vsub); > + } > + } > +} > +EXPORT_SYMBOL_GPL(v4l2_fill_pixfmt_mp); > + > +void v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, int pixelformat, int width, int height) > +{ > + const struct v4l2_format_info *info; > + int i; > + > + info = v4l2_format_info(pixelformat); > + if (!info) > + return; You have to check if pixelformat was a multiplanar format and reject it. > + > + pixfmt->width = width; > + pixfmt->height = height; > + pixfmt->pixelformat = pixelformat; > + pixfmt->bytesperline = width * info->cpp[0]; > + pixfmt->sizeimage = 0; > + > + for (i = 0; i < info->num_planes; i++) { > + unsigned int hsub = (i == 0) ? 1 : info->hsub; > + unsigned int vsub = (i == 0) ? 1 : info->vsub; > + > + pixfmt->sizeimage += info->cpp[i] * > + DIV_ROUND_UP(width, hsub) * > + DIV_ROUND_UP(height, vsub); > + } > +} > +EXPORT_SYMBOL_GPL(v4l2_fill_pixfmt); > diff --git a/drivers/media/v4l2-core/v4l2-fourcc.c b/drivers/media/v4l2-core/v4l2-fourcc.c > new file mode 100644 > index 000000000000..982c0ffa1a66 > --- /dev/null > +++ b/drivers/media/v4l2-core/v4l2-fourcc.c > @@ -0,0 +1,109 @@ > +/* > + * Copyright (c) 2018 Collabora, Ltd. > + * > + * Based on drm-fourcc: > + * Copyright (c) 2016 Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + * > + * Permission to use, copy, modify, distribute, and sell this software and its > + * documentation for any purpose is hereby granted without fee, provided that > + * the above copyright notice appear in all copies and that both that copyright > + * notice and this permission notice appear in supporting documentation, and > + * that the name of the copyright holders not be used in advertising or > + * publicity pertaining to distribution of the software without specific, > + * written prior permission. The copyright holders make no representations > + * about the suitability of this software for any purpose. It is provided "as > + * is" without express or implied warranty. > + * > + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS SOFTWARE, > + * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO > + * EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY SPECIAL, INDIRECT OR > + * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE, > + * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER > + * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE > + * OF THIS SOFTWARE. > + */ > + > +#include <linux/ctype.h> > +#include <linux/videodev2.h> > +#include <media/v4l2-fourcc.h> > + > +static char printable_char(int c) > +{ > + return isascii(c) && isprint(c) ? c : '?'; > +} > + > +const char *v4l2_get_format_name(uint32_t format) This should be called v4l2_get_fourcc_name. The format name is what ENUMFMT returns. > +{ > + static char buf[4]; > + > + snprintf(buf, 4, > + "%c%c%c%c", > + printable_char(format & 0xff), > + printable_char((format >> 8) & 0xff), > + printable_char((format >> 16) & 0xff), > + printable_char((format >> 24) & 0x7f)); If bit 31 is set, then add a '-BE' suffix to indicate that this is a big endian variant of the same pixelformat with bit 31 set to 0. See also v4l_fill_fmtdesc() in v4l2-ioctl.c. > + > + return buf; > +} > +EXPORT_SYMBOL(v4l2_get_format_name); I remember that Sakari tried to make a macro for this in a public header, but it was either rejected or fizzled out: https://www.mail-archive.com/linux-media@vger.kernel.org/msg128702.html > + > +const struct v4l2_format_info *v4l2_format_info(u32 format) > +{ > + static const struct v4l2_format_info formats[] = { > + /* RGB formats */ > + { .format = V4L2_PIX_FMT_BGR24, .num_planes = 1, .cpp = { 3, 0, 0 }, .hsub = 1, .vsub = 1 }, > + { .format = V4L2_PIX_FMT_RGB24, .num_planes = 1, .cpp = { 3, 0, 0 }, .hsub = 1, .vsub = 1 }, > + { .format = V4L2_PIX_FMT_HSV24, .num_planes = 1, .cpp = { 3, 0, 0 }, .hsub = 1, .vsub = 1 }, > + { .format = V4L2_PIX_FMT_BGR32, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 }, > + { .format = V4L2_PIX_FMT_XBGR32, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 }, > + { .format = V4L2_PIX_FMT_RGB32, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 }, > + { .format = V4L2_PIX_FMT_XRGB32, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 }, > + { .format = V4L2_PIX_FMT_HSV32, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 }, > + { .format = V4L2_PIX_FMT_ARGB32, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 }, > + { .format = V4L2_PIX_FMT_ABGR32, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 }, > + { .format = V4L2_PIX_FMT_GREY, .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1 }, > + > + /* YUV formats */ > + { .format = V4L2_PIX_FMT_YUYV, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1 }, > + { .format = V4L2_PIX_FMT_YVYU, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1 }, > + { .format = V4L2_PIX_FMT_UYVY, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1 }, > + { .format = V4L2_PIX_FMT_VYUY, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1 }, > + > + { .format = V4L2_PIX_FMT_NV12, .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 2 }, > + { .format = V4L2_PIX_FMT_NV21, .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 2 }, > + { .format = V4L2_PIX_FMT_NV16, .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 1 }, > + { .format = V4L2_PIX_FMT_NV61, .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 1 }, > + { .format = V4L2_PIX_FMT_NV24, .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 1, .vsub = 1 }, > + { .format = V4L2_PIX_FMT_NV42, .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 1, .vsub = 1 }, > + > + { .format = V4L2_PIX_FMT_YUV410, .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 4, .vsub = 4 }, > + { .format = V4L2_PIX_FMT_YVU410, .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 4, .vsub = 4 }, > + { .format = V4L2_PIX_FMT_YUV411P, .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 4, .vsub = 1 }, > + { .format = V4L2_PIX_FMT_YUV420, .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 2 }, > + { .format = V4L2_PIX_FMT_YVU420, .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 2 }, > + { .format = V4L2_PIX_FMT_YUV422P, .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 1 }, > + > + { .format = V4L2_PIX_FMT_YUV420M, .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 2, .multiplanar = 1 }, > + { .format = V4L2_PIX_FMT_YVU420M, .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 2, .multiplanar = 1 }, > + { .format = V4L2_PIX_FMT_YUV422M, .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 1, .multiplanar = 1 }, > + { .format = V4L2_PIX_FMT_YVU422M, .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 1, .multiplanar = 1 }, > + { .format = V4L2_PIX_FMT_YUV444M, .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 1, .vsub = 1, .multiplanar = 1 }, > + { .format = V4L2_PIX_FMT_YVU444M, .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 1, .vsub = 1, .multiplanar = 1 }, > + > + { .format = V4L2_PIX_FMT_NV12M, .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 2, .multiplanar = 1 }, > + { .format = V4L2_PIX_FMT_NV21M, .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 2, .multiplanar = 1 }, > + { .format = V4L2_PIX_FMT_NV16M, .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 1, .multiplanar = 1 }, > + { .format = V4L2_PIX_FMT_NV61M, .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 1, .multiplanar = 1 }, > + > + }; > + unsigned int i; > + > + for (i = 0; i < ARRAY_SIZE(formats); ++i) { > + if (formats[i].format == format) > + return &formats[i]; > + } No need for {} > + > + pr_warn("Unsupported V4L 4CC format %s (%08x)\n", v4l2_get_format_name(format), format); Make this a pr_dev or remove it altogether. I prefer the latter. > + return NULL; > +} > +EXPORT_SYMBOL(v4l2_format_info); > diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h > index 0c511ed8ffb0..6461ce747d90 100644 > --- a/include/media/v4l2-common.h > +++ b/include/media/v4l2-common.h > @@ -327,6 +327,11 @@ void v4l_bound_align_image(unsigned int *width, unsigned int wmin, > unsigned int hmax, unsigned int halign, > unsigned int salign); > > +void v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, int pixelformat, > + int width, int height); > +void v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt, int pixelformat, > + int width, int height); > + > /** > * v4l2_find_nearest_size - Find the nearest size among a discrete > * set of resolutions contained in an array of a driver specific struct. > diff --git a/include/media/v4l2-fourcc.h b/include/media/v4l2-fourcc.h > new file mode 100644 > index 000000000000..3d24f442aaf5 > --- /dev/null > +++ b/include/media/v4l2-fourcc.h > @@ -0,0 +1,53 @@ > +/* > + * Copyright (c) 2018 Collabora, Ltd. > + * > + * Based on drm-fourcc: > + * Copyright (c) 2016 Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + * > + * Permission to use, copy, modify, distribute, and sell this software and its > + * documentation for any purpose is hereby granted without fee, provided that > + * the above copyright notice appear in all copies and that both that copyright > + * notice and this permission notice appear in supporting documentation, and > + * that the name of the copyright holders not be used in advertising or > + * publicity pertaining to distribution of the software without specific, > + * written prior permission. The copyright holders make no representations > + * about the suitability of this software for any purpose. It is provided "as > + * is" without express or implied warranty. > + * > + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS SOFTWARE, > + * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO > + * EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY SPECIAL, INDIRECT OR > + * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE, > + * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER > + * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE > + * OF THIS SOFTWARE. Should be an SPDX ID. > + */ > +#ifndef __V4L2_FOURCC_H__ > +#define __V4L2_FOURCC_H__ > + > +#include <linux/types.h> > + > +/** > + * struct v4l2_format_info - information about a V4L2 format > + * @format: 4CC format identifier (V4L2_PIX_FMT_*) > + * @header_size: Size of header, optional and used by compressed formats > + * @num_planes: Number of planes (1 to 3) This is actually 1-4 since there may be an alpha channel as well. Not that we have such formats since the only formats with an alpha channel are interleaved formats, but it is possible. So this value is 2 for both NV12 and NV12M. > + * @cpp: Number of bytes per pixel (per plane) cpp? Shouldn't that be bpp? Note that this can differ per plane (see e.g. NV24). > + * @hsub: Horizontal chroma subsampling factor > + * @vsub: Vertical chroma subsampling factor A bit too cryptic IMHO. I would prefer hdiv or hsubsampling. 'hsub' suggests subtraction :-) > + * @multiplanar: Is it a multiplanar variant format? (e.g. NV12M) This should, I think, be renamed to num_non_contig_planes to indicate how many non-contiguous planes there are in the format. So this value is 1 for NV12 and 2 for NV12M. For V4L2_PIX_FMT_YUV444M it is 3. You can stick this value directly into pixfmt_mp->num_planes. As an aside: perhaps we should start calling the 'multiplanar API' the 'multiple non-contiguous planes API', at least in the documentation. It's the first time that I found a description that actually covers the real meaning. > + */ > +struct v4l2_format_info { > + u32 format; > + u32 header_size; > + u8 num_planes; > + u8 cpp[3]; > + u8 hsub; > + u8 vsub; > + u8 multiplanar; > +}; > + > +const struct v4l2_format_info *v4l2_format_info(u32 format); > +const char *v4l2_get_format_name(u32 format); > + > +#endif > Regards, Hans
On Wed, 2019-02-06 at 11:43 +0100, Hans Verkuil wrote: > Hi Ezequiel, > > A quick review below. This looks really useful, BTW. > > On 2/5/19 9:24 PM, Ezequiel Garcia wrote: > > Add two new API helpers, v4l2_fill_pixfmt and v4l2_fill_pixfmt_mp, > > to be used by drivers to calculate plane sizes and bytes per lines. > > > > Note that driver-specific paddig and alignment are not > > paddig -> padding > > > taken into account, and must be done by drivers using this API. > > > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > > --- > > drivers/media/v4l2-core/Makefile | 2 +- > > drivers/media/v4l2-core/v4l2-common.c | 71 +++++++++++++++++ > > drivers/media/v4l2-core/v4l2-fourcc.c | 109 ++++++++++++++++++++++++++ > > include/media/v4l2-common.h | 5 ++ > > include/media/v4l2-fourcc.h | 53 +++++++++++++ > > 5 files changed, 239 insertions(+), 1 deletion(-) > > create mode 100644 drivers/media/v4l2-core/v4l2-fourcc.c > > create mode 100644 include/media/v4l2-fourcc.h > > > > diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile > > index 9ee57e1efefe..bc23c3407c17 100644 > > --- a/drivers/media/v4l2-core/Makefile > > +++ b/drivers/media/v4l2-core/Makefile > > @@ -7,7 +7,7 @@ tuner-objs := tuner-core.o > > > > videodev-objs := v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o \ > > v4l2-event.o v4l2-ctrls.o v4l2-subdev.o v4l2-clk.o \ > > - v4l2-async.o > > + v4l2-async.o v4l2-fourcc.o > > ifeq ($(CONFIG_COMPAT),y) > > videodev-objs += v4l2-compat-ioctl32.o > > endif > > diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c > > index 50763fb42a1b..39d86a389cae 100644 > > --- a/drivers/media/v4l2-core/v4l2-common.c > > +++ b/drivers/media/v4l2-core/v4l2-common.c > > @@ -61,6 +61,7 @@ > > #include <media/v4l2-common.h> > > #include <media/v4l2-device.h> > > #include <media/v4l2-ctrls.h> > > +#include <media/v4l2-fourcc.h> > > Either create a v4l2-fourcc.c source using this header, or move the > contents of v4l2-fourcc.h to v4l2-common.h. > > Creating a new header but not a new source is a bit weird. > This patch adds v4l2-fourcc.c and v4l2-fourcc.h, containing only the pixel format array and the fourcc name helper. > > > > #include <linux/videodev2.h> > > > > @@ -455,3 +456,73 @@ int v4l2_s_parm_cap(struct video_device *vdev, > > return ret; > > } > > EXPORT_SYMBOL_GPL(v4l2_s_parm_cap); > > + > > +void v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt, > > + int pixelformat, int width, int height) > > +{ > > + const struct v4l2_format_info *info; > > + struct v4l2_plane_pix_format *plane; > > + int i; > > + > > + info = v4l2_format_info(pixelformat); > > + if (!info) > > + return; > > You should return a bool or something to indicate whether or not > the pixelformat was known. > Got it. > > + > > + pixfmt->width = width; > > + pixfmt->height = height; > > + pixfmt->pixelformat = pixelformat; > > + > > + if (!info->multiplanar) { > > It would make much more sense if multiplanar contained the number of > planes to use (i.e. equal to pixfmt->num_planes). > > See more about this below. > > > + pixfmt->num_planes = 1; > > + plane = &pixfmt->plane_fmt[0]; > > + plane->bytesperline = width * info->cpp[0]; > > + plane->sizeimage = 0; > > + for (i = 0; i < info->num_planes; i++) { > > + unsigned int hsub = (i == 0) ? 1 : info->hsub; > > + unsigned int vsub = (i == 0) ? 1 : info->vsub; > > + > > + plane->sizeimage += info->cpp[i] * > > + DIV_ROUND_UP(width, hsub) * > > + DIV_ROUND_UP(height, vsub); > > + } > > + } else { > > + pixfmt->num_planes = info->num_planes; > > + for (i = 0; i < info->num_planes; i++) { > > + unsigned int hsub = (i == 0) ? 1 : info->hsub; > > + unsigned int vsub = (i == 0) ? 1 : info->vsub; > > + > > + plane = &pixfmt->plane_fmt[i]; > > + plane->bytesperline = > > + info->cpp[i] * DIV_ROUND_UP(width, hsub); > > + plane->sizeimage = > > + plane->bytesperline * DIV_ROUND_UP(height, vsub); > > + } > > + } > > +} > > +EXPORT_SYMBOL_GPL(v4l2_fill_pixfmt_mp); > > + > > +void v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, int pixelformat, int width, int height) > > +{ > > + const struct v4l2_format_info *info; > > + int i; > > + > > + info = v4l2_format_info(pixelformat); > > + if (!info) > > + return; > > You have to check if pixelformat was a multiplanar format and reject it. > OK. > > + > > + pixfmt->width = width; > > + pixfmt->height = height; > > + pixfmt->pixelformat = pixelformat; > > + pixfmt->bytesperline = width * info->cpp[0]; > > + pixfmt->sizeimage = 0; > > + > > + for (i = 0; i < info->num_planes; i++) { > > + unsigned int hsub = (i == 0) ? 1 : info->hsub; > > + unsigned int vsub = (i == 0) ? 1 : info->vsub; > > + > > + pixfmt->sizeimage += info->cpp[i] * > > + DIV_ROUND_UP(width, hsub) * > > + DIV_ROUND_UP(height, vsub); > > + } > > +} > > +EXPORT_SYMBOL_GPL(v4l2_fill_pixfmt); > > diff --git a/drivers/media/v4l2-core/v4l2-fourcc.c b/drivers/media/v4l2-core/v4l2-fourcc.c > > new file mode 100644 > > index 000000000000..982c0ffa1a66 > > --- /dev/null > > +++ b/drivers/media/v4l2-core/v4l2-fourcc.c > > @@ -0,0 +1,109 @@ > > +/* > > + * Copyright (c) 2018 Collabora, Ltd. > > + * > > + * Based on drm-fourcc: > > + * Copyright (c) 2016 Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > + * > > + * Permission to use, copy, modify, distribute, and sell this software and its > > + * documentation for any purpose is hereby granted without fee, provided that > > + * the above copyright notice appear in all copies and that both that copyright > > + * notice and this permission notice appear in supporting documentation, and > > + * that the name of the copyright holders not be used in advertising or > > + * publicity pertaining to distribution of the software without specific, > > + * written prior permission. The copyright holders make no representations > > + * about the suitability of this software for any purpose. It is provided "as > > + * is" without express or implied warranty. > > + * > > + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS SOFTWARE, > > + * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO > > + * EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY SPECIAL, INDIRECT OR > > + * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE, > > + * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER > > + * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE > > + * OF THIS SOFTWARE. > > + */ > > + > > +#include <linux/ctype.h> > > +#include <linux/videodev2.h> > > +#include <media/v4l2-fourcc.h> > > + > > +static char printable_char(int c) > > +{ > > + return isascii(c) && isprint(c) ? c : '?'; > > +} > > + > > +const char *v4l2_get_format_name(uint32_t format) > > This should be called v4l2_get_fourcc_name. The format name is what ENUMFMT returns. > Got it. > > +{ > > + static char buf[4]; > > + > > + snprintf(buf, 4, > > + "%c%c%c%c", > > + printable_char(format & 0xff), > > + printable_char((format >> 8) & 0xff), > > + printable_char((format >> 16) & 0xff), > > + printable_char((format >> 24) & 0x7f)); > > If bit 31 is set, then add a '-BE' suffix to indicate that this is a > big endian variant of the same pixelformat with bit 31 set to 0. > > See also v4l_fill_fmtdesc() in v4l2-ioctl.c. > I omitted endianness and alpha because they weren't used by V4L2, but let me add them to make the code generic and support out-of-tree stuff. > > + > > + return buf; > > +} > > +EXPORT_SYMBOL(v4l2_get_format_name); > > I remember that Sakari tried to make a macro for this in a public header, but > it was either rejected or fizzled out: > > https://www.mail-archive.com/linux-media@vger.kernel.org/msg128702.html > I see. This time we are adding the fourcc name helper to the private API, which shouldn't cause too much debate. Adding Sakari to the loop. > > + > > +const struct v4l2_format_info *v4l2_format_info(u32 format) > > +{ > > + static const struct v4l2_format_info formats[] = { > > + /* RGB formats */ > > + { .format = V4L2_PIX_FMT_BGR24, .num_planes = 1, .cpp = { 3, 0, 0 }, .hsub = 1, .vsub = 1 }, > > + { .format = V4L2_PIX_FMT_RGB24, .num_planes = 1, .cpp = { 3, 0, 0 }, .hsub = 1, .vsub = 1 }, > > + { .format = V4L2_PIX_FMT_HSV24, .num_planes = 1, .cpp = { 3, 0, 0 }, .hsub = 1, .vsub = 1 }, > > + { .format = V4L2_PIX_FMT_BGR32, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 }, > > + { .format = V4L2_PIX_FMT_XBGR32, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 }, > > + { .format = V4L2_PIX_FMT_RGB32, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 }, > > + { .format = V4L2_PIX_FMT_XRGB32, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 }, > > + { .format = V4L2_PIX_FMT_HSV32, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 }, > > + { .format = V4L2_PIX_FMT_ARGB32, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 }, > > + { .format = V4L2_PIX_FMT_ABGR32, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 }, > > + { .format = V4L2_PIX_FMT_GREY, .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1 }, > > + > > + /* YUV formats */ > > + { .format = V4L2_PIX_FMT_YUYV, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1 }, > > + { .format = V4L2_PIX_FMT_YVYU, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1 }, > > + { .format = V4L2_PIX_FMT_UYVY, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1 }, > > + { .format = V4L2_PIX_FMT_VYUY, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1 }, > > + > > + { .format = V4L2_PIX_FMT_NV12, .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 2 }, > > + { .format = V4L2_PIX_FMT_NV21, .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 2 }, > > + { .format = V4L2_PIX_FMT_NV16, .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 1 }, > > + { .format = V4L2_PIX_FMT_NV61, .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 1 }, > > + { .format = V4L2_PIX_FMT_NV24, .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 1, .vsub = 1 }, > > + { .format = V4L2_PIX_FMT_NV42, .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 1, .vsub = 1 }, > > + > > + { .format = V4L2_PIX_FMT_YUV410, .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 4, .vsub = 4 }, > > + { .format = V4L2_PIX_FMT_YVU410, .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 4, .vsub = 4 }, > > + { .format = V4L2_PIX_FMT_YUV411P, .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 4, .vsub = 1 }, > > + { .format = V4L2_PIX_FMT_YUV420, .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 2 }, > > + { .format = V4L2_PIX_FMT_YVU420, .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 2 }, > > + { .format = V4L2_PIX_FMT_YUV422P, .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 1 }, > > + > > + { .format = V4L2_PIX_FMT_YUV420M, .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 2, .multiplanar = 1 }, > > + { .format = V4L2_PIX_FMT_YVU420M, .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 2, .multiplanar = 1 }, > > + { .format = V4L2_PIX_FMT_YUV422M, .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 1, .multiplanar = 1 }, > > + { .format = V4L2_PIX_FMT_YVU422M, .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 1, .multiplanar = 1 }, > > + { .format = V4L2_PIX_FMT_YUV444M, .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 1, .vsub = 1, .multiplanar = 1 }, > > + { .format = V4L2_PIX_FMT_YVU444M, .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 1, .vsub = 1, .multiplanar = 1 }, > > + > > + { .format = V4L2_PIX_FMT_NV12M, .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 2, .multiplanar = 1 }, > > + { .format = V4L2_PIX_FMT_NV21M, .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 2, .multiplanar = 1 }, > > + { .format = V4L2_PIX_FMT_NV16M, .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 1, .multiplanar = 1 }, > > + { .format = V4L2_PIX_FMT_NV61M, .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 1, .multiplanar = 1 }, > > + > > + }; > > + unsigned int i; > > + > > + for (i = 0; i < ARRAY_SIZE(formats); ++i) { > > + if (formats[i].format == format) > > + return &formats[i]; > > + } > > No need for {} > OK > > + > > + pr_warn("Unsupported V4L 4CC format %s (%08x)\n", v4l2_get_format_name(format), format); > > Make this a pr_dev or remove it altogether. I prefer the latter. > Right. If the function returns an error condition, we can drop it. > > + return NULL; > > +} > > +EXPORT_SYMBOL(v4l2_format_info); > > diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h > > index 0c511ed8ffb0..6461ce747d90 100644 > > --- a/include/media/v4l2-common.h > > +++ b/include/media/v4l2-common.h > > @@ -327,6 +327,11 @@ void v4l_bound_align_image(unsigned int *width, unsigned int wmin, > > unsigned int hmax, unsigned int halign, > > unsigned int salign); > > > > +void v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, int pixelformat, > > + int width, int height); > > +void v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt, int pixelformat, > > + int width, int height); > > + > > /** > > * v4l2_find_nearest_size - Find the nearest size among a discrete > > * set of resolutions contained in an array of a driver specific struct. > > diff --git a/include/media/v4l2-fourcc.h b/include/media/v4l2-fourcc.h > > new file mode 100644 > > index 000000000000..3d24f442aaf5 > > --- /dev/null > > +++ b/include/media/v4l2-fourcc.h > > @@ -0,0 +1,53 @@ > > +/* > > + * Copyright (c) 2018 Collabora, Ltd. > > + * > > + * Based on drm-fourcc: > > + * Copyright (c) 2016 Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > + * > > + * Permission to use, copy, modify, distribute, and sell this software and its > > + * documentation for any purpose is hereby granted without fee, provided that > > + * the above copyright notice appear in all copies and that both that copyright > > + * notice and this permission notice appear in supporting documentation, and > > + * that the name of the copyright holders not be used in advertising or > > + * publicity pertaining to distribution of the software without specific, > > + * written prior permission. The copyright holders make no representations > > + * about the suitability of this software for any purpose. It is provided "as > > + * is" without express or implied warranty. > > + * > > + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS SOFTWARE, > > + * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO > > + * EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY SPECIAL, INDIRECT OR > > + * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE, > > + * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER > > + * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE > > + * OF THIS SOFTWARE. > > Should be an SPDX ID. > > > + */ > > +#ifndef __V4L2_FOURCC_H__ > > +#define __V4L2_FOURCC_H__ > > + > > +#include <linux/types.h> > > + > > +/** > > + * struct v4l2_format_info - information about a V4L2 format > > + * @format: 4CC format identifier (V4L2_PIX_FMT_*) > > + * @header_size: Size of header, optional and used by compressed formats > > + * @num_planes: Number of planes (1 to 3) > > This is actually 1-4 since there may be an alpha channel as well. Not that we have > such formats since the only formats with an alpha channel are interleaved formats, > but it is possible. > > So this value is 2 for both NV12 and NV12M. > > > + * @cpp: Number of bytes per pixel (per plane) > > cpp? Shouldn't that be bpp? > > Note that this can differ per plane (see e.g. NV24). > Yes, the comment should specify that it's an array of bytes per pixel. And the naming follows drm-fourcc. cpp stands for char-per-pixel. It may be confusing but at the same time, I really like to have consistent naming across the board (think for instance in the DMA coherent/consistent aliases). Also, note that drm-fourcc deprecates cpp, to support tile formats. Hopefully we don't need that here? > > + * @hsub: Horizontal chroma subsampling factor > > + * @vsub: Vertical chroma subsampling factor > > A bit too cryptic IMHO. I would prefer hdiv or hsubsampling. 'hsub' suggests > subtraction :-) > Ditto, this name follows drm-fourcc. I'm fine either way. > > + * @multiplanar: Is it a multiplanar variant format? (e.g. NV12M) > > This should, I think, be renamed to num_non_contig_planes to indicate how many > non-contiguous planes there are in the format. > > So this value is 1 for NV12 and 2 for NV12M. For V4L2_PIX_FMT_YUV444M it is 3. > > You can stick this value directly into pixfmt_mp->num_planes. > Fine by me, but I have to admit I don't see the value of adding the number of non-contiguous planes. For multiplanar non-contiguous formats the number of planes is equal to the number of planes. Although maybe it will be clear this way for readers? > As an aside: perhaps we should start calling the 'multiplanar API' the > 'multiple non-contiguous planes API', at least in the documentation. It's the > first time that I found a description that actually covers the real meaning. > Yes, indeed. In fact, my first version of this code had something like "is_noncontiguous" instead of the "multiplanar" field. > > + */ > > +struct v4l2_format_info { > > + u32 format; > > + u32 header_size; > > + u8 num_planes; > > + u8 cpp[3]; > > + u8 hsub; > > + u8 vsub; > > + u8 multiplanar; > > +}; > > + > > +const struct v4l2_format_info *v4l2_format_info(u32 format); > > +const char *v4l2_get_format_name(u32 format); > > + > > +#endif > > > Thanks a lot for the thorough review! > Regards, > > Hans
On 2/6/19 5:22 PM, Ezequiel Garcia wrote: > On Wed, 2019-02-06 at 11:43 +0100, Hans Verkuil wrote: >> Hi Ezequiel, >> >> A quick review below. This looks really useful, BTW. >> >> On 2/5/19 9:24 PM, Ezequiel Garcia wrote: >>> Add two new API helpers, v4l2_fill_pixfmt and v4l2_fill_pixfmt_mp, >>> to be used by drivers to calculate plane sizes and bytes per lines. >>> >>> Note that driver-specific paddig and alignment are not >> >> paddig -> padding >> >>> taken into account, and must be done by drivers using this API. >>> >>> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> >>> --- >>> drivers/media/v4l2-core/Makefile | 2 +- >>> drivers/media/v4l2-core/v4l2-common.c | 71 +++++++++++++++++ >>> drivers/media/v4l2-core/v4l2-fourcc.c | 109 ++++++++++++++++++++++++++ >>> include/media/v4l2-common.h | 5 ++ >>> include/media/v4l2-fourcc.h | 53 +++++++++++++ >>> 5 files changed, 239 insertions(+), 1 deletion(-) >>> create mode 100644 drivers/media/v4l2-core/v4l2-fourcc.c >>> create mode 100644 include/media/v4l2-fourcc.h >>> >>> diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile >>> index 9ee57e1efefe..bc23c3407c17 100644 >>> --- a/drivers/media/v4l2-core/Makefile >>> +++ b/drivers/media/v4l2-core/Makefile >>> @@ -7,7 +7,7 @@ tuner-objs := tuner-core.o >>> >>> videodev-objs := v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o \ >>> v4l2-event.o v4l2-ctrls.o v4l2-subdev.o v4l2-clk.o \ >>> - v4l2-async.o >>> + v4l2-async.o v4l2-fourcc.o >>> ifeq ($(CONFIG_COMPAT),y) >>> videodev-objs += v4l2-compat-ioctl32.o >>> endif >>> diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c >>> index 50763fb42a1b..39d86a389cae 100644 >>> --- a/drivers/media/v4l2-core/v4l2-common.c >>> +++ b/drivers/media/v4l2-core/v4l2-common.c >>> @@ -61,6 +61,7 @@ >>> #include <media/v4l2-common.h> >>> #include <media/v4l2-device.h> >>> #include <media/v4l2-ctrls.h> >>> +#include <media/v4l2-fourcc.h> >> >> Either create a v4l2-fourcc.c source using this header, or move the >> contents of v4l2-fourcc.h to v4l2-common.h. >> >> Creating a new header but not a new source is a bit weird. >> > > This patch adds v4l2-fourcc.c and v4l2-fourcc.h, containing > only the pixel format array and the fourcc name helper. Ah, I now see that I got confused because some functions were added to v4l2-fourcc.c and others to v4l2-common.c. I still stand by my comment: either everything goes to v4l2-common, or to v4l2-fourcc. But it makes no sense IMHO to split it over two sources. > >>> >>> #include <linux/videodev2.h> >>> >>> @@ -455,3 +456,73 @@ int v4l2_s_parm_cap(struct video_device *vdev, >>> return ret; >>> } >>> EXPORT_SYMBOL_GPL(v4l2_s_parm_cap); >>> + >>> +void v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt, >>> + int pixelformat, int width, int height) >>> +{ >>> + const struct v4l2_format_info *info; >>> + struct v4l2_plane_pix_format *plane; >>> + int i; >>> + >>> + info = v4l2_format_info(pixelformat); >>> + if (!info) >>> + return; >> >> You should return a bool or something to indicate whether or not >> the pixelformat was known. >> > > Got it. > >>> + >>> + pixfmt->width = width; >>> + pixfmt->height = height; >>> + pixfmt->pixelformat = pixelformat; >>> + >>> + if (!info->multiplanar) { >> >> It would make much more sense if multiplanar contained the number of >> planes to use (i.e. equal to pixfmt->num_planes). >> >> See more about this below. >> >>> + pixfmt->num_planes = 1; >>> + plane = &pixfmt->plane_fmt[0]; >>> + plane->bytesperline = width * info->cpp[0]; >>> + plane->sizeimage = 0; >>> + for (i = 0; i < info->num_planes; i++) { >>> + unsigned int hsub = (i == 0) ? 1 : info->hsub; >>> + unsigned int vsub = (i == 0) ? 1 : info->vsub; >>> + >>> + plane->sizeimage += info->cpp[i] * >>> + DIV_ROUND_UP(width, hsub) * >>> + DIV_ROUND_UP(height, vsub); >>> + } >>> + } else { >>> + pixfmt->num_planes = info->num_planes; >>> + for (i = 0; i < info->num_planes; i++) { >>> + unsigned int hsub = (i == 0) ? 1 : info->hsub; >>> + unsigned int vsub = (i == 0) ? 1 : info->vsub; >>> + >>> + plane = &pixfmt->plane_fmt[i]; >>> + plane->bytesperline = >>> + info->cpp[i] * DIV_ROUND_UP(width, hsub); >>> + plane->sizeimage = >>> + plane->bytesperline * DIV_ROUND_UP(height, vsub); >>> + } >>> + } >>> +} >>> +EXPORT_SYMBOL_GPL(v4l2_fill_pixfmt_mp); >>> + >>> +void v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, int pixelformat, int width, int height) >>> +{ >>> + const struct v4l2_format_info *info; >>> + int i; >>> + >>> + info = v4l2_format_info(pixelformat); >>> + if (!info) >>> + return; >> >> You have to check if pixelformat was a multiplanar format and reject it. >> > > OK. > >>> + >>> + pixfmt->width = width; >>> + pixfmt->height = height; >>> + pixfmt->pixelformat = pixelformat; >>> + pixfmt->bytesperline = width * info->cpp[0]; >>> + pixfmt->sizeimage = 0; >>> + >>> + for (i = 0; i < info->num_planes; i++) { >>> + unsigned int hsub = (i == 0) ? 1 : info->hsub; >>> + unsigned int vsub = (i == 0) ? 1 : info->vsub; >>> + >>> + pixfmt->sizeimage += info->cpp[i] * >>> + DIV_ROUND_UP(width, hsub) * >>> + DIV_ROUND_UP(height, vsub); >>> + } >>> +} >>> +EXPORT_SYMBOL_GPL(v4l2_fill_pixfmt); >>> diff --git a/drivers/media/v4l2-core/v4l2-fourcc.c b/drivers/media/v4l2-core/v4l2-fourcc.c >>> new file mode 100644 >>> index 000000000000..982c0ffa1a66 >>> --- /dev/null >>> +++ b/drivers/media/v4l2-core/v4l2-fourcc.c >>> @@ -0,0 +1,109 @@ >>> +/* >>> + * Copyright (c) 2018 Collabora, Ltd. >>> + * >>> + * Based on drm-fourcc: >>> + * Copyright (c) 2016 Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>> + * >>> + * Permission to use, copy, modify, distribute, and sell this software and its >>> + * documentation for any purpose is hereby granted without fee, provided that >>> + * the above copyright notice appear in all copies and that both that copyright >>> + * notice and this permission notice appear in supporting documentation, and >>> + * that the name of the copyright holders not be used in advertising or >>> + * publicity pertaining to distribution of the software without specific, >>> + * written prior permission. The copyright holders make no representations >>> + * about the suitability of this software for any purpose. It is provided "as >>> + * is" without express or implied warranty. >>> + * >>> + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS SOFTWARE, >>> + * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO >>> + * EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY SPECIAL, INDIRECT OR >>> + * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE, >>> + * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER >>> + * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE >>> + * OF THIS SOFTWARE. >>> + */ >>> + >>> +#include <linux/ctype.h> >>> +#include <linux/videodev2.h> >>> +#include <media/v4l2-fourcc.h> >>> + >>> +static char printable_char(int c) >>> +{ >>> + return isascii(c) && isprint(c) ? c : '?'; >>> +} >>> + >>> +const char *v4l2_get_format_name(uint32_t format) >> >> This should be called v4l2_get_fourcc_name. The format name is what ENUMFMT returns. >> > > Got it. > >>> +{ >>> + static char buf[4]; >>> + >>> + snprintf(buf, 4, >>> + "%c%c%c%c", >>> + printable_char(format & 0xff), >>> + printable_char((format >> 8) & 0xff), >>> + printable_char((format >> 16) & 0xff), >>> + printable_char((format >> 24) & 0x7f)); >> >> If bit 31 is set, then add a '-BE' suffix to indicate that this is a >> big endian variant of the same pixelformat with bit 31 set to 0. >> >> See also v4l_fill_fmtdesc() in v4l2-ioctl.c. >> > > I omitted endianness and alpha because they weren't used > by V4L2, but let me add them to make the code generic > and support out-of-tree stuff. > >>> + >>> + return buf; >>> +} >>> +EXPORT_SYMBOL(v4l2_get_format_name); >> >> I remember that Sakari tried to make a macro for this in a public header, but >> it was either rejected or fizzled out: >> >> https://www.mail-archive.com/linux-media@vger.kernel.org/msg128702.html >> > > I see. This time we are adding the fourcc name helper to the private API, > which shouldn't cause too much debate. Adding Sakari to the loop. > >>> + >>> +const struct v4l2_format_info *v4l2_format_info(u32 format) >>> +{ >>> + static const struct v4l2_format_info formats[] = { >>> + /* RGB formats */ >>> + { .format = V4L2_PIX_FMT_BGR24, .num_planes = 1, .cpp = { 3, 0, 0 }, .hsub = 1, .vsub = 1 }, >>> + { .format = V4L2_PIX_FMT_RGB24, .num_planes = 1, .cpp = { 3, 0, 0 }, .hsub = 1, .vsub = 1 }, >>> + { .format = V4L2_PIX_FMT_HSV24, .num_planes = 1, .cpp = { 3, 0, 0 }, .hsub = 1, .vsub = 1 }, >>> + { .format = V4L2_PIX_FMT_BGR32, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 }, >>> + { .format = V4L2_PIX_FMT_XBGR32, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 }, >>> + { .format = V4L2_PIX_FMT_RGB32, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 }, >>> + { .format = V4L2_PIX_FMT_XRGB32, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 }, >>> + { .format = V4L2_PIX_FMT_HSV32, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 }, >>> + { .format = V4L2_PIX_FMT_ARGB32, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 }, >>> + { .format = V4L2_PIX_FMT_ABGR32, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 }, >>> + { .format = V4L2_PIX_FMT_GREY, .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1 }, >>> + >>> + /* YUV formats */ >>> + { .format = V4L2_PIX_FMT_YUYV, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1 }, >>> + { .format = V4L2_PIX_FMT_YVYU, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1 }, >>> + { .format = V4L2_PIX_FMT_UYVY, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1 }, >>> + { .format = V4L2_PIX_FMT_VYUY, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1 }, >>> + >>> + { .format = V4L2_PIX_FMT_NV12, .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 2 }, >>> + { .format = V4L2_PIX_FMT_NV21, .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 2 }, >>> + { .format = V4L2_PIX_FMT_NV16, .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 1 }, >>> + { .format = V4L2_PIX_FMT_NV61, .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 1 }, >>> + { .format = V4L2_PIX_FMT_NV24, .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 1, .vsub = 1 }, >>> + { .format = V4L2_PIX_FMT_NV42, .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 1, .vsub = 1 }, >>> + >>> + { .format = V4L2_PIX_FMT_YUV410, .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 4, .vsub = 4 }, >>> + { .format = V4L2_PIX_FMT_YVU410, .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 4, .vsub = 4 }, >>> + { .format = V4L2_PIX_FMT_YUV411P, .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 4, .vsub = 1 }, >>> + { .format = V4L2_PIX_FMT_YUV420, .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 2 }, >>> + { .format = V4L2_PIX_FMT_YVU420, .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 2 }, >>> + { .format = V4L2_PIX_FMT_YUV422P, .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 1 }, >>> + >>> + { .format = V4L2_PIX_FMT_YUV420M, .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 2, .multiplanar = 1 }, >>> + { .format = V4L2_PIX_FMT_YVU420M, .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 2, .multiplanar = 1 }, >>> + { .format = V4L2_PIX_FMT_YUV422M, .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 1, .multiplanar = 1 }, >>> + { .format = V4L2_PIX_FMT_YVU422M, .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 1, .multiplanar = 1 }, >>> + { .format = V4L2_PIX_FMT_YUV444M, .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 1, .vsub = 1, .multiplanar = 1 }, >>> + { .format = V4L2_PIX_FMT_YVU444M, .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 1, .vsub = 1, .multiplanar = 1 }, >>> + >>> + { .format = V4L2_PIX_FMT_NV12M, .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 2, .multiplanar = 1 }, >>> + { .format = V4L2_PIX_FMT_NV21M, .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 2, .multiplanar = 1 }, >>> + { .format = V4L2_PIX_FMT_NV16M, .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 1, .multiplanar = 1 }, >>> + { .format = V4L2_PIX_FMT_NV61M, .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 1, .multiplanar = 1 }, >>> + >>> + }; >>> + unsigned int i; >>> + >>> + for (i = 0; i < ARRAY_SIZE(formats); ++i) { >>> + if (formats[i].format == format) >>> + return &formats[i]; >>> + } >> >> No need for {} >> > > OK > >>> + >>> + pr_warn("Unsupported V4L 4CC format %s (%08x)\n", v4l2_get_format_name(format), format); >> >> Make this a pr_dev or remove it altogether. I prefer the latter. >> > > Right. If the function returns an error condition, we can drop it. > >>> + return NULL; >>> +} >>> +EXPORT_SYMBOL(v4l2_format_info); >>> diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h >>> index 0c511ed8ffb0..6461ce747d90 100644 >>> --- a/include/media/v4l2-common.h >>> +++ b/include/media/v4l2-common.h >>> @@ -327,6 +327,11 @@ void v4l_bound_align_image(unsigned int *width, unsigned int wmin, >>> unsigned int hmax, unsigned int halign, >>> unsigned int salign); >>> >>> +void v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, int pixelformat, >>> + int width, int height); >>> +void v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt, int pixelformat, >>> + int width, int height); >>> + >>> /** >>> * v4l2_find_nearest_size - Find the nearest size among a discrete >>> * set of resolutions contained in an array of a driver specific struct. >>> diff --git a/include/media/v4l2-fourcc.h b/include/media/v4l2-fourcc.h >>> new file mode 100644 >>> index 000000000000..3d24f442aaf5 >>> --- /dev/null >>> +++ b/include/media/v4l2-fourcc.h >>> @@ -0,0 +1,53 @@ >>> +/* >>> + * Copyright (c) 2018 Collabora, Ltd. >>> + * >>> + * Based on drm-fourcc: >>> + * Copyright (c) 2016 Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>> + * >>> + * Permission to use, copy, modify, distribute, and sell this software and its >>> + * documentation for any purpose is hereby granted without fee, provided that >>> + * the above copyright notice appear in all copies and that both that copyright >>> + * notice and this permission notice appear in supporting documentation, and >>> + * that the name of the copyright holders not be used in advertising or >>> + * publicity pertaining to distribution of the software without specific, >>> + * written prior permission. The copyright holders make no representations >>> + * about the suitability of this software for any purpose. It is provided "as >>> + * is" without express or implied warranty. >>> + * >>> + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS SOFTWARE, >>> + * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO >>> + * EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY SPECIAL, INDIRECT OR >>> + * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE, >>> + * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER >>> + * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE >>> + * OF THIS SOFTWARE. >> >> Should be an SPDX ID. >> >>> + */ >>> +#ifndef __V4L2_FOURCC_H__ >>> +#define __V4L2_FOURCC_H__ >>> + >>> +#include <linux/types.h> >>> + >>> +/** >>> + * struct v4l2_format_info - information about a V4L2 format >>> + * @format: 4CC format identifier (V4L2_PIX_FMT_*) >>> + * @header_size: Size of header, optional and used by compressed formats >>> + * @num_planes: Number of planes (1 to 3) >> >> This is actually 1-4 since there may be an alpha channel as well. Not that we have >> such formats since the only formats with an alpha channel are interleaved formats, >> but it is possible. >> >> So this value is 2 for both NV12 and NV12M. >> >>> + * @cpp: Number of bytes per pixel (per plane) >> >> cpp? Shouldn't that be bpp? >> >> Note that this can differ per plane (see e.g. NV24). >> > > Yes, the comment should specify that it's an array of bytes per pixel. > > And the naming follows drm-fourcc. cpp stands for char-per-pixel. > It may be confusing but at the same time, I really like to have > consistent naming across the board (think for instance in > the DMA coherent/consistent aliases). We never use cpp in the media subsystem, it's always bpp. Please keep that name. And the v4l2 fourccs are unrelated to the drm fourccs. Historically they developed along separate paths and I actually think it can be confusing to suggest that they are related. Now whether there should be an effort to unify the two is an entirely different question. > > Also, note that drm-fourcc deprecates cpp, to support tile formats. > Hopefully we don't need that here? We do have tile formats (V4L2_PIX_FMT_NV12MT_16X16), but it is up to the driver to align width/height accordingly. > >>> + * @hsub: Horizontal chroma subsampling factor >>> + * @vsub: Vertical chroma subsampling factor >> >> A bit too cryptic IMHO. I would prefer hdiv or hsubsampling. 'hsub' suggests >> subtraction :-) >> > > Ditto, this name follows drm-fourcc. I'm fine either way. > >>> + * @multiplanar: Is it a multiplanar variant format? (e.g. NV12M) >> >> This should, I think, be renamed to num_non_contig_planes to indicate how many >> non-contiguous planes there are in the format. >> >> So this value is 1 for NV12 and 2 for NV12M. For V4L2_PIX_FMT_YUV444M it is 3. >> >> You can stick this value directly into pixfmt_mp->num_planes. >> > > Fine by me, but I have to admit I don't see the value of adding the > number of non-contiguous planes. For multiplanar non-contiguous formats > the number of planes is equal to the number of planes. Hmm, that's true. Choose whatever gives you the shortest code :-) > > Although maybe it will be clear this way for readers? > >> As an aside: perhaps we should start calling the 'multiplanar API' the >> 'multiple non-contiguous planes API', at least in the documentation. It's the >> first time that I found a description that actually covers the real meaning. >> > > Yes, indeed. In fact, my first version of this code had something like > "is_noncontiguous" instead of the "multiplanar" field. I'm fine with that. Add a comment after it like: /* aka multiplanar */ > >>> + */ >>> +struct v4l2_format_info { >>> + u32 format; >>> + u32 header_size; >>> + u8 num_planes; >>> + u8 cpp[3]; >>> + u8 hsub; >>> + u8 vsub; >>> + u8 multiplanar; >>> +}; >>> + >>> +const struct v4l2_format_info *v4l2_format_info(u32 format); >>> +const char *v4l2_get_format_name(u32 format); >>> + >>> +#endif >>> >> > > Thanks a lot for the thorough review! > >> Regards, >> >> Hans > > > Regards, Hans
On Thu, Feb 7, 2019 at 1:36 AM Hans Verkuil <hverkuil@xs4all.nl> wrote: > > On 2/6/19 5:22 PM, Ezequiel Garcia wrote: > > On Wed, 2019-02-06 at 11:43 +0100, Hans Verkuil wrote: > >> Hi Ezequiel, > >> > >> A quick review below. This looks really useful, BTW. > >> > >> On 2/5/19 9:24 PM, Ezequiel Garcia wrote: [snip] > >>> +/** > >>> + * struct v4l2_format_info - information about a V4L2 format > >>> + * @format: 4CC format identifier (V4L2_PIX_FMT_*) > >>> + * @header_size: Size of header, optional and used by compressed formats > >>> + * @num_planes: Number of planes (1 to 3) > >> > >> This is actually 1-4 since there may be an alpha channel as well. Not that we have > >> such formats since the only formats with an alpha channel are interleaved formats, > >> but it is possible. How about 1 to VIDEO_MAX_PLANES to be a bit more consistent? Tbh. I'm not sure why we have that defined to 8, but if we have such constant already, it could make sense to use it here as well. [snip] > > > > Also, note that drm-fourcc deprecates cpp, to support tile formats. > > Hopefully we don't need that here? > > We do have tile formats (V4L2_PIX_FMT_NV12MT_16X16), but it is up to the > driver to align width/height accordingly. > I'd still make these helpers align to the constraints defined by the format itself (e.g. 16x16), since it doesn't cost us anything, and have the driver do any further alignment only if they need so. > > > >>> + * @hsub: Horizontal chroma subsampling factor > >>> + * @vsub: Vertical chroma subsampling factor > >> > >> A bit too cryptic IMHO. I would prefer hdiv or hsubsampling. 'hsub' suggests > >> subtraction :-) > >> > > > > Ditto, this name follows drm-fourcc. I'm fine either way. > > I personally like hsub and vsub too, but maybe I just spent too much time with DRM code. *subsampling would make the initializers super wide, so if we decide that we don't like *sub, I'd go with *div. > >>> + * @multiplanar: Is it a multiplanar variant format? (e.g. NV12M) > >> > >> This should, I think, be renamed to num_non_contig_planes to indicate how many > >> non-contiguous planes there are in the format. > >> > >> So this value is 1 for NV12 and 2 for NV12M. For V4L2_PIX_FMT_YUV444M it is 3. > >> > >> You can stick this value directly into pixfmt_mp->num_planes. > >> > > > > Fine by me, but I have to admit I don't see the value of adding the > > number of non-contiguous planes. For multiplanar non-contiguous formats > > the number of planes is equal to the number of planes. > > Hmm, that's true. Choose whatever gives you the shortest code :-) > > > > > Although maybe it will be clear this way for readers? > > > >> As an aside: perhaps we should start calling the 'multiplanar API' the > >> 'multiple non-contiguous planes API', at least in the documentation. It's the To me, "multiple non-contiguous planes API" would suggest that the planes themselves are non-contiguous. Many drivers (especially Samsung ones) have a distinction between "color planes" and "memory planes" internally, so maybe "Multiple memory planes API" could make sense? > >> first time that I found a description that actually covers the real meaning. > >> > > > > Yes, indeed. In fact, my first version of this code had something like > > "is_noncontiguous" instead of the "multiplanar" field. > > I'm fine with that. Add a comment after it like: /* aka multiplanar */ > FWIW, some of the drivers have .num_cplanes and .num_mplanes in their format descriptors. Best regards, Tomasz
On 2/20/19 7:53 AM, Tomasz Figa wrote: > On Thu, Feb 7, 2019 at 1:36 AM Hans Verkuil <hverkuil@xs4all.nl> wrote: >> >> On 2/6/19 5:22 PM, Ezequiel Garcia wrote: >>> On Wed, 2019-02-06 at 11:43 +0100, Hans Verkuil wrote: >>>> Hi Ezequiel, >>>> >>>> A quick review below. This looks really useful, BTW. >>>> >>>> On 2/5/19 9:24 PM, Ezequiel Garcia wrote: > > [snip] >>>>> +/** >>>>> + * struct v4l2_format_info - information about a V4L2 format >>>>> + * @format: 4CC format identifier (V4L2_PIX_FMT_*) >>>>> + * @header_size: Size of header, optional and used by compressed formats >>>>> + * @num_planes: Number of planes (1 to 3) >>>> >>>> This is actually 1-4 since there may be an alpha channel as well. Not that we have >>>> such formats since the only formats with an alpha channel are interleaved formats, >>>> but it is possible. > > How about 1 to VIDEO_MAX_PLANES to be a bit more consistent? > Tbh. I'm not sure why we have that defined to 8, but if we have such > constant already, it could make sense to use it here as well. We didn't know at the time how many planes we would need. I think we chose 8 because 1) that fit inside struct v4l2_format and 2) it allowed room for planes carrying meta data. In hindsight we probably should have chosen 4 instead of 8. In any case, since this is an internal API I think chosing MAX_PLANES here would waste unnecessary memory. > > [snip] >>> >>> Also, note that drm-fourcc deprecates cpp, to support tile formats. >>> Hopefully we don't need that here? >> >> We do have tile formats (V4L2_PIX_FMT_NV12MT_16X16), but it is up to the >> driver to align width/height accordingly. >> > > I'd still make these helpers align to the constraints defined by the > format itself (e.g. 16x16), since it doesn't cost us anything, and > have the driver do any further alignment only if they need so. Yes, sorry, I should have said that: for tiled pixel formats this struct should give the alignments. But those alignments differ from hsub/vsub: those values restrict the resolution, but the 'tiled' alignments are on top of that. > >>> >>>>> + * @hsub: Horizontal chroma subsampling factor >>>>> + * @vsub: Vertical chroma subsampling factor >>>> >>>> A bit too cryptic IMHO. I would prefer hdiv or hsubsampling. 'hsub' suggests >>>> subtraction :-) >>>> >>> >>> Ditto, this name follows drm-fourcc. I'm fine either way. >>> > > I personally like hsub and vsub too, but maybe I just spent too much > time with DRM code. *subsampling would make the initializers super > wide, so if we decide that we don't like *sub, I'd go with *div. > >>>>> + * @multiplanar: Is it a multiplanar variant format? (e.g. NV12M) >>>> >>>> This should, I think, be renamed to num_non_contig_planes to indicate how many >>>> non-contiguous planes there are in the format. >>>> >>>> So this value is 1 for NV12 and 2 for NV12M. For V4L2_PIX_FMT_YUV444M it is 3. >>>> >>>> You can stick this value directly into pixfmt_mp->num_planes. >>>> >>> >>> Fine by me, but I have to admit I don't see the value of adding the >>> number of non-contiguous planes. For multiplanar non-contiguous formats >>> the number of planes is equal to the number of planes. >> >> Hmm, that's true. Choose whatever gives you the shortest code :-) >> >>> >>> Although maybe it will be clear this way for readers? >>> >>>> As an aside: perhaps we should start calling the 'multiplanar API' the >>>> 'multiple non-contiguous planes API', at least in the documentation. It's the > > To me, "multiple non-contiguous planes API" would suggest that the > planes themselves are non-contiguous. > > Many drivers (especially Samsung ones) have a distinction between > "color planes" and "memory planes" internally, so maybe "Multiple > memory planes API" could make sense? Huh, that's an idea. So _MPLANE should have been _MMPLANE? > >>>> first time that I found a description that actually covers the real meaning. >>>> >>> >>> Yes, indeed. In fact, my first version of this code had something like >>> "is_noncontiguous" instead of the "multiplanar" field. >> >> I'm fine with that. Add a comment after it like: /* aka multiplanar */ >> > > FWIW, some of the drivers have .num_cplanes and .num_mplanes in their > format descriptors. I think that makes sense. Good suggestion. Regards, Hans
On Wed, 2019-02-20 at 09:39 +0100, Hans Verkuil wrote: > On 2/20/19 7:53 AM, Tomasz Figa wrote: > > On Thu, Feb 7, 2019 at 1:36 AM Hans Verkuil <hverkuil@xs4all.nl> wrote: > > > On 2/6/19 5:22 PM, Ezequiel Garcia wrote: > > > > On Wed, 2019-02-06 at 11:43 +0100, Hans Verkuil wrote: > > > > > Hi Ezequiel, > > > > > > > > > > A quick review below. This looks really useful, BTW. > > > > > > > > > > On 2/5/19 9:24 PM, Ezequiel Garcia wrote: > > > > [snip] > > > > > > +/** > > > > > > + * struct v4l2_format_info - information about a V4L2 format > > > > > > + * @format: 4CC format identifier (V4L2_PIX_FMT_*) > > > > > > + * @header_size: Size of header, optional and used by compressed formats > > > > > > + * @num_planes: Number of planes (1 to 3) > > > > > > > > > > This is actually 1-4 since there may be an alpha channel as well. Not that we have > > > > > such formats since the only formats with an alpha channel are interleaved formats, > > > > > but it is possible. > > > > How about 1 to VIDEO_MAX_PLANES to be a bit more consistent? > > Tbh. I'm not sure why we have that defined to 8, but if we have such > > constant already, it could make sense to use it here as well. > > We didn't know at the time how many planes we would need. I think we > chose 8 because 1) that fit inside struct v4l2_format and 2) it allowed > room for planes carrying meta data. > > In hindsight we probably should have chosen 4 instead of 8. > > In any case, since this is an internal API I think chosing MAX_PLANES > here would waste unnecessary memory. > 4 it is then. > > [snip] > > > > Also, note that drm-fourcc deprecates cpp, to support tile formats. > > > > Hopefully we don't need that here? > > > > > > We do have tile formats (V4L2_PIX_FMT_NV12MT_16X16), but it is up to the > > > driver to align width/height accordingly. > > > > > > > I'd still make these helpers align to the constraints defined by the > > format itself (e.g. 16x16), since it doesn't cost us anything, and > > have the driver do any further alignment only if they need so. > > Yes, sorry, I should have said that: for tiled pixel formats this > struct should give the alignments. > > But those alignments differ from hsub/vsub: those values restrict the > resolution, but the 'tiled' alignments are on top of that. > Hm, OK. So the idea is support tiling formats on this helpers? Might need to change the way we describe formats, as DRM does, to incorporate some notion of pixel blocks. > > > > > > + * @hsub: Horizontal chroma subsampling factor > > > > > > + * @vsub: Vertical chroma subsampling factor > > > > > > > > > > A bit too cryptic IMHO. I would prefer hdiv or hsubsampling. 'hsub' suggests > > > > > subtraction :-) > > > > > > > > > > > > > Ditto, this name follows drm-fourcc. I'm fine either way. > > > > > > > > I personally like hsub and vsub too, but maybe I just spent too much > > time with DRM code. *subsampling would make the initializers super > > wide, so if we decide that we don't like *sub, I'd go with *div. > > Note that there's a v2 patch, where I went with *div :-) > > > > > > + * @multiplanar: Is it a multiplanar variant format? (e.g. NV12M) > > > > > > > > > > This should, I think, be renamed to num_non_contig_planes to indicate how many > > > > > non-contiguous planes there are in the format. > > > > > > > > > > So this value is 1 for NV12 and 2 for NV12M. For V4L2_PIX_FMT_YUV444M it is 3. > > > > > > > > > > You can stick this value directly into pixfmt_mp->num_planes. > > > > > > > > > > > > > Fine by me, but I have to admit I don't see the value of adding the > > > > number of non-contiguous planes. For multiplanar non-contiguous formats > > > > the number of planes is equal to the number of planes. > > > > > > Hmm, that's true. Choose whatever gives you the shortest code :-) > > > > > > > Although maybe it will be clear this way for readers? > > > > > > > > > As an aside: perhaps we should start calling the 'multiplanar API' the > > > > > 'multiple non-contiguous planes API', at least in the documentation. It's the > > > > To me, "multiple non-contiguous planes API" would suggest that the > > planes themselves are non-contiguous. > > > > Many drivers (especially Samsung ones) have a distinction between > > "color planes" and "memory planes" internally, so maybe "Multiple > > memory planes API" could make sense? > > Huh, that's an idea. So _MPLANE should have been _MMPLANE? > > > > > > first time that I found a description that actually covers the real meaning. > > > > > > > > > > > > > Yes, indeed. In fact, my first version of this code had something like > > > > "is_noncontiguous" instead of the "multiplanar" field. > > > > > > I'm fine with that. Add a comment after it like: /* aka multiplanar */ > > > > > > > FWIW, some of the drivers have .num_cplanes and .num_mplanes in their > > format descriptors. > > This sounds nice. To be clear, this means that instead of a boolean is_noncontigous, we describe the number of (non-contiguous) memory planes and the number of component planes. I really dislike the name "color planes", because it could be misleading as chroma planes, so I think "component planes" makes it more clear. Thanks for reviewing! Eze
diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile index 9ee57e1efefe..bc23c3407c17 100644 --- a/drivers/media/v4l2-core/Makefile +++ b/drivers/media/v4l2-core/Makefile @@ -7,7 +7,7 @@ tuner-objs := tuner-core.o videodev-objs := v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o \ v4l2-event.o v4l2-ctrls.o v4l2-subdev.o v4l2-clk.o \ - v4l2-async.o + v4l2-async.o v4l2-fourcc.o ifeq ($(CONFIG_COMPAT),y) videodev-objs += v4l2-compat-ioctl32.o endif diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c index 50763fb42a1b..39d86a389cae 100644 --- a/drivers/media/v4l2-core/v4l2-common.c +++ b/drivers/media/v4l2-core/v4l2-common.c @@ -61,6 +61,7 @@ #include <media/v4l2-common.h> #include <media/v4l2-device.h> #include <media/v4l2-ctrls.h> +#include <media/v4l2-fourcc.h> #include <linux/videodev2.h> @@ -455,3 +456,73 @@ int v4l2_s_parm_cap(struct video_device *vdev, return ret; } EXPORT_SYMBOL_GPL(v4l2_s_parm_cap); + +void v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt, + int pixelformat, int width, int height) +{ + const struct v4l2_format_info *info; + struct v4l2_plane_pix_format *plane; + int i; + + info = v4l2_format_info(pixelformat); + if (!info) + return; + + pixfmt->width = width; + pixfmt->height = height; + pixfmt->pixelformat = pixelformat; + + if (!info->multiplanar) { + pixfmt->num_planes = 1; + plane = &pixfmt->plane_fmt[0]; + plane->bytesperline = width * info->cpp[0]; + plane->sizeimage = 0; + for (i = 0; i < info->num_planes; i++) { + unsigned int hsub = (i == 0) ? 1 : info->hsub; + unsigned int vsub = (i == 0) ? 1 : info->vsub; + + plane->sizeimage += info->cpp[i] * + DIV_ROUND_UP(width, hsub) * + DIV_ROUND_UP(height, vsub); + } + } else { + pixfmt->num_planes = info->num_planes; + for (i = 0; i < info->num_planes; i++) { + unsigned int hsub = (i == 0) ? 1 : info->hsub; + unsigned int vsub = (i == 0) ? 1 : info->vsub; + + plane = &pixfmt->plane_fmt[i]; + plane->bytesperline = + info->cpp[i] * DIV_ROUND_UP(width, hsub); + plane->sizeimage = + plane->bytesperline * DIV_ROUND_UP(height, vsub); + } + } +} +EXPORT_SYMBOL_GPL(v4l2_fill_pixfmt_mp); + +void v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, int pixelformat, int width, int height) +{ + const struct v4l2_format_info *info; + int i; + + info = v4l2_format_info(pixelformat); + if (!info) + return; + + pixfmt->width = width; + pixfmt->height = height; + pixfmt->pixelformat = pixelformat; + pixfmt->bytesperline = width * info->cpp[0]; + pixfmt->sizeimage = 0; + + for (i = 0; i < info->num_planes; i++) { + unsigned int hsub = (i == 0) ? 1 : info->hsub; + unsigned int vsub = (i == 0) ? 1 : info->vsub; + + pixfmt->sizeimage += info->cpp[i] * + DIV_ROUND_UP(width, hsub) * + DIV_ROUND_UP(height, vsub); + } +} +EXPORT_SYMBOL_GPL(v4l2_fill_pixfmt); diff --git a/drivers/media/v4l2-core/v4l2-fourcc.c b/drivers/media/v4l2-core/v4l2-fourcc.c new file mode 100644 index 000000000000..982c0ffa1a66 --- /dev/null +++ b/drivers/media/v4l2-core/v4l2-fourcc.c @@ -0,0 +1,109 @@ +/* + * Copyright (c) 2018 Collabora, Ltd. + * + * Based on drm-fourcc: + * Copyright (c) 2016 Laurent Pinchart <laurent.pinchart@ideasonboard.com> + * + * Permission to use, copy, modify, distribute, and sell this software and its + * documentation for any purpose is hereby granted without fee, provided that + * the above copyright notice appear in all copies and that both that copyright + * notice and this permission notice appear in supporting documentation, and + * that the name of the copyright holders not be used in advertising or + * publicity pertaining to distribution of the software without specific, + * written prior permission. The copyright holders make no representations + * about the suitability of this software for any purpose. It is provided "as + * is" without express or implied warranty. + * + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS SOFTWARE, + * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO + * EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY SPECIAL, INDIRECT OR + * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE, + * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER + * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE + * OF THIS SOFTWARE. + */ + +#include <linux/ctype.h> +#include <linux/videodev2.h> +#include <media/v4l2-fourcc.h> + +static char printable_char(int c) +{ + return isascii(c) && isprint(c) ? c : '?'; +} + +const char *v4l2_get_format_name(uint32_t format) +{ + static char buf[4]; + + snprintf(buf, 4, + "%c%c%c%c", + printable_char(format & 0xff), + printable_char((format >> 8) & 0xff), + printable_char((format >> 16) & 0xff), + printable_char((format >> 24) & 0x7f)); + + return buf; +} +EXPORT_SYMBOL(v4l2_get_format_name); + +const struct v4l2_format_info *v4l2_format_info(u32 format) +{ + static const struct v4l2_format_info formats[] = { + /* RGB formats */ + { .format = V4L2_PIX_FMT_BGR24, .num_planes = 1, .cpp = { 3, 0, 0 }, .hsub = 1, .vsub = 1 }, + { .format = V4L2_PIX_FMT_RGB24, .num_planes = 1, .cpp = { 3, 0, 0 }, .hsub = 1, .vsub = 1 }, + { .format = V4L2_PIX_FMT_HSV24, .num_planes = 1, .cpp = { 3, 0, 0 }, .hsub = 1, .vsub = 1 }, + { .format = V4L2_PIX_FMT_BGR32, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 }, + { .format = V4L2_PIX_FMT_XBGR32, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 }, + { .format = V4L2_PIX_FMT_RGB32, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 }, + { .format = V4L2_PIX_FMT_XRGB32, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 }, + { .format = V4L2_PIX_FMT_HSV32, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 }, + { .format = V4L2_PIX_FMT_ARGB32, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 }, + { .format = V4L2_PIX_FMT_ABGR32, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 }, + { .format = V4L2_PIX_FMT_GREY, .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1 }, + + /* YUV formats */ + { .format = V4L2_PIX_FMT_YUYV, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1 }, + { .format = V4L2_PIX_FMT_YVYU, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1 }, + { .format = V4L2_PIX_FMT_UYVY, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1 }, + { .format = V4L2_PIX_FMT_VYUY, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1 }, + + { .format = V4L2_PIX_FMT_NV12, .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 2 }, + { .format = V4L2_PIX_FMT_NV21, .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 2 }, + { .format = V4L2_PIX_FMT_NV16, .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 1 }, + { .format = V4L2_PIX_FMT_NV61, .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 1 }, + { .format = V4L2_PIX_FMT_NV24, .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 1, .vsub = 1 }, + { .format = V4L2_PIX_FMT_NV42, .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 1, .vsub = 1 }, + + { .format = V4L2_PIX_FMT_YUV410, .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 4, .vsub = 4 }, + { .format = V4L2_PIX_FMT_YVU410, .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 4, .vsub = 4 }, + { .format = V4L2_PIX_FMT_YUV411P, .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 4, .vsub = 1 }, + { .format = V4L2_PIX_FMT_YUV420, .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 2 }, + { .format = V4L2_PIX_FMT_YVU420, .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 2 }, + { .format = V4L2_PIX_FMT_YUV422P, .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 1 }, + + { .format = V4L2_PIX_FMT_YUV420M, .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 2, .multiplanar = 1 }, + { .format = V4L2_PIX_FMT_YVU420M, .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 2, .multiplanar = 1 }, + { .format = V4L2_PIX_FMT_YUV422M, .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 1, .multiplanar = 1 }, + { .format = V4L2_PIX_FMT_YVU422M, .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 1, .multiplanar = 1 }, + { .format = V4L2_PIX_FMT_YUV444M, .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 1, .vsub = 1, .multiplanar = 1 }, + { .format = V4L2_PIX_FMT_YVU444M, .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 1, .vsub = 1, .multiplanar = 1 }, + + { .format = V4L2_PIX_FMT_NV12M, .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 2, .multiplanar = 1 }, + { .format = V4L2_PIX_FMT_NV21M, .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 2, .multiplanar = 1 }, + { .format = V4L2_PIX_FMT_NV16M, .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 1, .multiplanar = 1 }, + { .format = V4L2_PIX_FMT_NV61M, .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 1, .multiplanar = 1 }, + + }; + unsigned int i; + + for (i = 0; i < ARRAY_SIZE(formats); ++i) { + if (formats[i].format == format) + return &formats[i]; + } + + pr_warn("Unsupported V4L 4CC format %s (%08x)\n", v4l2_get_format_name(format), format); + return NULL; +} +EXPORT_SYMBOL(v4l2_format_info); diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h index 0c511ed8ffb0..6461ce747d90 100644 --- a/include/media/v4l2-common.h +++ b/include/media/v4l2-common.h @@ -327,6 +327,11 @@ void v4l_bound_align_image(unsigned int *width, unsigned int wmin, unsigned int hmax, unsigned int halign, unsigned int salign); +void v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, int pixelformat, + int width, int height); +void v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt, int pixelformat, + int width, int height); + /** * v4l2_find_nearest_size - Find the nearest size among a discrete * set of resolutions contained in an array of a driver specific struct. diff --git a/include/media/v4l2-fourcc.h b/include/media/v4l2-fourcc.h new file mode 100644 index 000000000000..3d24f442aaf5 --- /dev/null +++ b/include/media/v4l2-fourcc.h @@ -0,0 +1,53 @@ +/* + * Copyright (c) 2018 Collabora, Ltd. + * + * Based on drm-fourcc: + * Copyright (c) 2016 Laurent Pinchart <laurent.pinchart@ideasonboard.com> + * + * Permission to use, copy, modify, distribute, and sell this software and its + * documentation for any purpose is hereby granted without fee, provided that + * the above copyright notice appear in all copies and that both that copyright + * notice and this permission notice appear in supporting documentation, and + * that the name of the copyright holders not be used in advertising or + * publicity pertaining to distribution of the software without specific, + * written prior permission. The copyright holders make no representations + * about the suitability of this software for any purpose. It is provided "as + * is" without express or implied warranty. + * + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS SOFTWARE, + * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO + * EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY SPECIAL, INDIRECT OR + * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE, + * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER + * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE + * OF THIS SOFTWARE. + */ +#ifndef __V4L2_FOURCC_H__ +#define __V4L2_FOURCC_H__ + +#include <linux/types.h> + +/** + * struct v4l2_format_info - information about a V4L2 format + * @format: 4CC format identifier (V4L2_PIX_FMT_*) + * @header_size: Size of header, optional and used by compressed formats + * @num_planes: Number of planes (1 to 3) + * @cpp: Number of bytes per pixel (per plane) + * @hsub: Horizontal chroma subsampling factor + * @vsub: Vertical chroma subsampling factor + * @multiplanar: Is it a multiplanar variant format? (e.g. NV12M) + */ +struct v4l2_format_info { + u32 format; + u32 header_size; + u8 num_planes; + u8 cpp[3]; + u8 hsub; + u8 vsub; + u8 multiplanar; +}; + +const struct v4l2_format_info *v4l2_format_info(u32 format); +const char *v4l2_get_format_name(u32 format); + +#endif
Add two new API helpers, v4l2_fill_pixfmt and v4l2_fill_pixfmt_mp, to be used by drivers to calculate plane sizes and bytes per lines. Note that driver-specific paddig and alignment are not taken into account, and must be done by drivers using this API. Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> --- drivers/media/v4l2-core/Makefile | 2 +- drivers/media/v4l2-core/v4l2-common.c | 71 +++++++++++++++++ drivers/media/v4l2-core/v4l2-fourcc.c | 109 ++++++++++++++++++++++++++ include/media/v4l2-common.h | 5 ++ include/media/v4l2-fourcc.h | 53 +++++++++++++ 5 files changed, 239 insertions(+), 1 deletion(-) create mode 100644 drivers/media/v4l2-core/v4l2-fourcc.c create mode 100644 include/media/v4l2-fourcc.h