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 <ezequ...@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.pinch...@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.pinch...@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
