[RFC v2] [media] v4l2: add V4L2 pixel format array and helper functions

[RFC v2] [media] v4l2: add V4L2 pixel format array and helper functions

[Philipp Zabel]

This patch adds an array of V4L2 pixel formats and descriptions that can be
used by drivers so that each driver doesn't have to provide its own slightly
different format descriptions for VIDIOC_ENUM_FMT.

Each array entry also includes two bits per pixel values (for a single line and
one for the whole image) that can be used to determine the v4l2_pix_format
bytesperline and sizeimage values and whether the format is planar or
compressed.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
Changes since v1:
 - Added use of DIV_ROUND_UP in v4l2_bytesperline
 - Un-inlined v4l2_sizeimage and made it use v4l2_bytesperline
   for non-planar, non-tiled formats
 - Added .planes property to struct v4l2_pixfmt for
   non-contiguous planar formats
 - Fixed Y41P, YUV410, and YVU420M pixelformat values
---
 drivers/media/v4l2-core/v4l2-common.c | 505 ++++++++++++++++++++++++++++++++++
 include/media/v4l2-common.h           |  43 +++
 2 files changed, 548 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
index ccaa38f..63b91a5 100644
--- a/drivers/media/v4l2-core/v4l2-common.c
+++ b/drivers/media/v4l2-core/v4l2-common.c
@@ -533,3 +533,508 @@ void v4l2_get_timestamp(struct timeval *tv)
 	tv->tv_usec = ts.tv_nsec / NSEC_PER_USEC;
 }
 EXPORT_SYMBOL_GPL(v4l2_get_timestamp);
+
+static const struct v4l2_pixfmt v4l2_pixfmts[] = {
+	{
+		.name = "8-bit RGB 3-3-2",
+		.pixelformat = V4L2_PIX_FMT_RGB332,
+		.bpp_line = 8,
+		.bpp_image = 8,
+	}, {
+		.name = "16-bit RGB 4-4-4",
+		.pixelformat = V4L2_PIX_FMT_RGB444,
+		.bpp_line = 16,
+		.bpp_image = 16,
+	}, {
+		.name = "16-bit ARGB 4-4-4-4",
+		.pixelformat = V4L2_PIX_FMT_ARGB444,
+		.bpp_line = 16,
+		.bpp_image = 16,
+	}, {
+		.name = "16-bit XRGB 4-4-4-4",
+		.pixelformat = V4L2_PIX_FMT_XRGB444,
+		.bpp_line = 16,
+		.bpp_image = 16,
+	}, {
+		.name = "16-bit RGB 5-5-5",
+		.pixelformat = V4L2_PIX_FMT_RGB555,
+		.bpp_line = 16,
+		.bpp_image = 16,
+	}, {
+		.name = "16-bit ARGB 1-5-5-5",
+		.pixelformat = V4L2_PIX_FMT_ARGB555,
+		.bpp_line = 16,
+		.bpp_image = 16,
+	}, {
+		.name = "16-bit XRGB 1-5-5-5",
+		.pixelformat = V4L2_PIX_FMT_XRGB555,
+		.bpp_line = 16,
+		.bpp_image = 16,
+	}, {
+		.name = "16-bit RGB 5-6-5",
+		.pixelformat = V4L2_PIX_FMT_RGB565,
+		.bpp_line = 16,
+		.bpp_image = 16,
+	}, {
+		.name = "16-bit RGB 5-5-5 BE",
+		.pixelformat = V4L2_PIX_FMT_RGB555X,
+		.bpp_line = 16,
+		.bpp_image = 16,
+	}, {
+		.name = "16-bit RGB 5-6-5 BE",
+		.pixelformat = V4L2_PIX_FMT_RGB565X,
+		.bpp_line = 16,
+		.bpp_image = 16,
+	}, {
+		.name = "18-bit BGR 6-6-6",
+		.pixelformat = V4L2_PIX_FMT_BGR666,
+		.bpp_line = 18,
+		.bpp_image = 18,
+	}, {
+		.name = "24-bit BGR 8-8-8",
+		.pixelformat = V4L2_PIX_FMT_BGR24,
+		.bpp_line = 24,
+		.bpp_image = 24,
+	}, {
+		.name = "24-bit RGB 8-8-8",
+		.pixelformat = V4L2_PIX_FMT_RGB24,
+		.bpp_line = 24,
+		.bpp_image = 24,
+	}, {
+		.name = "32-bit BGR 8-8-8-8",
+		.pixelformat = V4L2_PIX_FMT_BGR32,
+		.bpp_line = 32,
+		.bpp_image = 32,
+	}, {
+		.name = "32-bit BGRA 8-8-8-8",
+		.pixelformat = V4L2_PIX_FMT_ABGR32,
+		.bpp_line = 32,
+		.bpp_image = 32,
+	}, {
+		.name = "32-bit BGRX 8-8-8-8",
+		.pixelformat = V4L2_PIX_FMT_XBGR32,
+		.bpp_line = 32,
+		.bpp_image = 32,
+	}, {
+		.name = "32-bit RGB 8-8-8-8",
+		.pixelformat = V4L2_PIX_FMT_RGB32,
+		.bpp_line = 32,
+		.bpp_image = 32,
+	}, {
+		.name = "32-bit ARGB 8-8-8-8",
+		.pixelformat = V4L2_PIX_FMT_ARGB32,
+		.bpp_line = 32,
+		.bpp_image = 32,
+	}, {
+		.name = "32-bit XRGB 8-8-8-8",
+		.pixelformat = V4L2_PIX_FMT_XRGB32,
+		.bpp_line = 32,
+		.bpp_image = 32,
+	}, {
+		.name = "8-bit Greyscale",
+		.pixelformat = V4L2_PIX_FMT_GREY,
+		.bpp_line = 8,
+		.bpp_image = 8,
+	}, {
+		.name = "4-bit Greyscale",
+		.pixelformat = V4L2_PIX_FMT_Y4,
+		.bpp_line = 8,
+		.bpp_image = 8,
+	}, {
+		.name = "6-bit Greyscale",
+		.pixelformat = V4L2_PIX_FMT_Y6,
+		.bpp_line = 8,
+		.bpp_image = 8,
+	}, {
+		.name = "10-bit Greyscale",
+		.pixelformat = V4L2_PIX_FMT_Y10,
+		.bpp_line = 16,
+		.bpp_image = 16,
+	}, {
+		.name = "12-bit Greyscale",
+		.pixelformat = V4L2_PIX_FMT_Y12,
+		.bpp_line = 16,
+		.bpp_image = 16,
+	}, {
+		.name = "16-bit Greyscale",
+		.pixelformat = V4L2_PIX_FMT_Y16,
+		.bpp_line = 16,
+		.bpp_image = 16,
+	}, {
+		.name = "10-bit Greyscale (packed)",
+		.pixelformat = V4L2_PIX_FMT_Y10BPACK,
+		.bpp_line = 10,
+		.bpp_image = 10,
+	}, {
+		.name = "8-bit Palette",
+		.pixelformat = V4L2_PIX_FMT_PAL8,
+		.bpp_line = 8,
+		.bpp_image = 8,
+	}, {
+		.name = "8-bit Chrominance UV 4-4",
+		.pixelformat = V4L2_PIX_FMT_UV8,
+		.bpp_line = 8,
+		.bpp_image = 8,
+	}, {
+		.name = "Planar YVU 4:1:0",
+		.pixelformat = V4L2_PIX_FMT_YVU410,
+		.bpp_line = 8,
+		.bpp_image = 9,
+	}, {
+		.name = "Planar YVU 4:2:0",
+		.pixelformat = V4L2_PIX_FMT_YVU420,
+		.bpp_line = 8,
+		.bpp_image = 12,
+	}, {
+		.name = "YUYV 4:2:2",
+		.pixelformat = V4L2_PIX_FMT_YUYV,
+		.bpp_line = 16,
+		.bpp_image = 16,
+	}, {
+		.name = "YYUV 4:2:2",
+		.pixelformat = V4L2_PIX_FMT_YYUV,
+		.bpp_line = 16,
+		.bpp_image = 16,
+	}, {
+		.name = "YVYU 4:2:2",
+		.pixelformat = V4L2_PIX_FMT_YVYU,
+		.bpp_line = 16,
+		.bpp_image = 16,
+	}, {
+		.name = "UYVY 4:2:2",
+		.pixelformat = V4L2_PIX_FMT_UYVY,
+		.bpp_line = 16,
+		.bpp_image = 16,
+	}, {
+		.name = "VYUY 4:2:2",
+		.pixelformat = V4L2_PIX_FMT_VYUY,
+		.bpp_line = 16,
+		.bpp_image = 16,
+	}, {
+		.name = "Planar YVU 4:2:2",
+		.pixelformat = V4L2_PIX_FMT_YUV422P,
+		.bpp_line = 8,
+		.bpp_image = 16,
+	}, {
+		.name = "Planar YUV 4:1:1",
+		.pixelformat = V4L2_PIX_FMT_YUV411P,
+		.bpp_line = 8,
+		.bpp_image = 12,
+	}, {
+		.name = "YUV 4:1:1 (packed)",
+		.pixelformat = V4L2_PIX_FMT_Y41P,
+		.bpp_line = 12,
+		.bpp_image = 12,
+	}, {
+		.name = "16-bit YUV 4-4-4",
+		.pixelformat = V4L2_PIX_FMT_YUV444,
+		.bpp_line = 16,
+		.bpp_image = 16,
+	}, {
+		.name = "16-bit YUV 5-5-5",
+		.pixelformat = V4L2_PIX_FMT_YUV555,
+		.bpp_line = 16,
+		.bpp_image = 16,
+	}, {
+		.name = "16-bit YUV 5-6-5",
+		.pixelformat = V4L2_PIX_FMT_YUV565,
+		.bpp_line = 16,
+		.bpp_image = 16,
+	}, {
+		.name = "32-bit YUV 8-8-8-8",
+		.pixelformat = V4L2_PIX_FMT_YUV32,
+		.bpp_line = 32,
+		.bpp_image = 32,
+	}, {
+		.name = "Planar YUV 4:1:0",
+		.pixelformat = V4L2_PIX_FMT_YUV410,
+		.bpp_line = 8,
+		.bpp_image = 9,
+	}, {
+		.name = "Planar YUV 4:2:0",
+		.pixelformat = V4L2_PIX_FMT_YUV420,
+		.bpp_line = 8,
+		.bpp_image = 12,
+	}, {
+		.name = "8-bit dithered RGB (BTTV)",
+		.pixelformat = V4L2_PIX_FMT_HI240,
+		.bpp_line = 8,
+		.bpp_image = 8,
+	}, {
+		.name = "YUV 4:2:0 (16x16 macroblocks)",
+		.pixelformat = V4L2_PIX_FMT_HM12,
+		/* bpp_line not applicable for macroblock tiled formats */
+		.bpp_image = 12,
+	}, {
+		.name = "YUV 4:2:0 (M420)",
+		.pixelformat = V4L2_PIX_FMT_M420,
+		/* bpp_line not applicable for hybrid line-interleaved planes */
+		.bpp_image = 12,
+	}, {
+		.name = "Y/CbCr 4:2:0",
+		.pixelformat = V4L2_PIX_FMT_NV12,
+		.bpp_line = 8,
+		.bpp_image = 12,
+	}, {
+		.name = "Y/CrCb 4:2:0",
+		.pixelformat = V4L2_PIX_FMT_NV21,
+		.bpp_line = 8,
+		.bpp_image = 12,
+	}, {
+		.name = "Y/CbCr 4:2:2",
+		.pixelformat = V4L2_PIX_FMT_NV16,
+		.bpp_line = 8,
+		.bpp_image = 16,
+	}, {
+		.name = "Y/CrCb 4:2:0",
+		.pixelformat = V4L2_PIX_FMT_NV61,
+		.bpp_line = 8,
+		.bpp_image = 16,
+	}, {
+		.name = "Y/CbCr 4:4:4",
+		.pixelformat = V4L2_PIX_FMT_NV24,
+		.bpp_line = 8,
+		.bpp_image = 24,
+	}, {
+		.name = "Y/CrCb 4:2:0",
+		.pixelformat = V4L2_PIX_FMT_NV42,
+		.bpp_line = 8,
+		.bpp_image = 24,
+	}, {
+		.name = "Y/CbCr 4:2:0 (non-contig.)",
+		.pixelformat = V4L2_PIX_FMT_NV12M,
+		.bpp_line = 8,
+		.bpp_image = 12,
+		.planes = 2,
+	}, {
+		.name = "Y/CrCb 4:2:0 (non-contig.)",
+		.pixelformat = V4L2_PIX_FMT_NV21M,
+		.bpp_line = 8,
+		.bpp_image = 12,
+		.planes = 2,
+	}, {
+		.name = "Y/CbCr 4:2:2 (non-contig.)",
+		.pixelformat = V4L2_PIX_FMT_NV16M,
+		.bpp_line = 8,
+		.bpp_image = 16,
+		.planes = 2,
+	}, {
+		.name = "Y/CrCb 4:2:2 (non-contig.)",
+		.pixelformat = V4L2_PIX_FMT_NV61M,
+		.bpp_line = 8,
+		.bpp_image = 16,
+		.planes = 2,
+	}, {
+		.name = "Y/CbCr 4:2:0 (64x32 MB, non-c.)",
+		.pixelformat = V4L2_PIX_FMT_NV12MT,
+		/* bpp_line not applicable for macroblock tiled formats */
+		.bpp_image = 12,
+	}, {
+		.name = "Y/CbCr 4:2:0 (16x16 MB, non-c.)",
+		.pixelformat = V4L2_PIX_FMT_NV12MT_16X16,
+		/* bpp_line not applicable for macroblock tiled formats */
+		.bpp_image = 12,
+		.planes = 2,
+	}, {
+		.name = "Planar YUV 4:2:0 (non-contig.)",
+		.pixelformat = V4L2_PIX_FMT_YUV420M,
+		.bpp_line = 8,
+		.bpp_image = 12,
+		.planes = 3,
+	}, {
+		.name = "Planar YVU 4:2:0 (non-contig.)",
+		.pixelformat = V4L2_PIX_FMT_YVU420M,
+		.bpp_line = 8,
+		.bpp_image = 12,
+		.planes = 3,
+	}, {
+		.name = "8-bit Bayer BGBG/GRGR",
+		.pixelformat = V4L2_PIX_FMT_SBGGR8,
+		.bpp_line = 8,
+		.bpp_image = 8,
+	}, {
+		.name = "8-bit Bayer GBGB/RGRG",
+		.pixelformat = V4L2_PIX_FMT_SGBRG8,
+		.bpp_line = 8,
+		.bpp_image = 8,
+	}, {
+		.name = "8-bit Bayer GRGR/BGBG",
+		.pixelformat = V4L2_PIX_FMT_SGRBG8,
+		.bpp_line = 8,
+		.bpp_image = 8,
+	}, {
+		.name = "8-bit Bayer RGRG/GBGB",
+		.pixelformat = V4L2_PIX_FMT_SRGGB8,
+		.bpp_line = 8,
+		.bpp_image = 8,
+	}, {
+		.name = "10-bit Bayer BGBG/GRGR",
+		.pixelformat = V4L2_PIX_FMT_SBGGR10,
+		.bpp_line = 16,
+		.bpp_image = 16,
+	}, {
+		.name = "10-bit Bayer GBGB/RGRG",
+		.pixelformat = V4L2_PIX_FMT_SGBRG10,
+		.bpp_line = 16,
+		.bpp_image = 16,
+	}, {
+		.name = "10-bit Bayer GRGR/BGBG",
+		.pixelformat = V4L2_PIX_FMT_SGRBG10,
+		.bpp_line = 16,
+		.bpp_image = 16,
+	}, {
+		.name = "10-bit Bayer RGRG/GBGB",
+		.pixelformat = V4L2_PIX_FMT_SRGGB10,
+		.bpp_line = 16,
+		.bpp_image = 16,
+	}, {
+		.name = "12-bit Bayer BGBG/GRGR",
+		.pixelformat = V4L2_PIX_FMT_SBGGR12,
+		.bpp_line = 16,
+		.bpp_image = 16,
+	}, {
+		.name = "12-bit Bayer GBGB/RGRG",
+		.pixelformat = V4L2_PIX_FMT_SGBRG12,
+		.bpp_line = 16,
+		.bpp_image = 16,
+	}, {
+		.name = "12-bit Bayer GRGR/BGBG",
+		.pixelformat = V4L2_PIX_FMT_SGRBG12,
+		.bpp_line = 16,
+		.bpp_image = 16,
+	}, {
+		.name = "12-bit Bayer RGRG/GBGB",
+		.pixelformat = V4L2_PIX_FMT_SRGGB12,
+		.bpp_line = 16,
+		.bpp_image = 16,
+	}, {
+		.name = "8-bit Bayer BGBG/GRGR (A-law)",
+		.pixelformat = V4L2_PIX_FMT_SBGGR10ALAW8,
+		.bpp_line = 8,
+		.bpp_image = 8,
+	}, {
+		.name = "8-bit Bayer GBGB/RGRG (A-law)",
+		.pixelformat = V4L2_PIX_FMT_SGBRG10ALAW8,
+		.bpp_line = 8,
+		.bpp_image = 8,
+	}, {
+		.name = "8-bit Bayer GRGR/BGBG (A-law)",
+		.pixelformat = V4L2_PIX_FMT_SGRBG10ALAW8,
+		.bpp_line = 8,
+		.bpp_image = 8,
+	}, {
+		.name = "8-bit Bayer RGRG/GBGB (A-law)",
+		.pixelformat = V4L2_PIX_FMT_SRGGB10ALAW8,
+		.bpp_line = 8,
+		.bpp_image = 8,
+	}, {
+
+		.name = "8-bit Bayer BGBG/GRGR (DPCM)",
+		.pixelformat = V4L2_PIX_FMT_SBGGR10DPCM8,
+		.bpp_line = 8,
+		.bpp_image = 8,
+	}, {
+		.name = "8-bit Bayer GBGB/RGRG (DPCM)",
+		.pixelformat = V4L2_PIX_FMT_SGBRG10DPCM8,
+		.bpp_line = 8,
+		.bpp_image = 8,
+	}, {
+		.name = "8-bit Bayer GRGR/BGBG (DPCM)",
+		.pixelformat = V4L2_PIX_FMT_SGRBG10DPCM8,
+		.bpp_line = 8,
+		.bpp_image = 8,
+	}, {
+		.name = "8-bit Bayer RGRG/GBGB (DPCM)",
+		.pixelformat = V4L2_PIX_FMT_SRGGB10DPCM8,
+		.bpp_line = 8,
+		.bpp_image = 8,
+	}, {
+		.name = "16-bit Bayer BGBG/GRGR (exp.)",
+		.pixelformat = V4L2_PIX_FMT_SBGGR16,
+		.bpp_line = 16,
+		.bpp_image = 16,
+	}, {
+		.name = "Motion-JPEG",
+		.pixelformat = V4L2_PIX_FMT_MJPEG,
+	}, {
+		.name = "JFIF JPEG",
+		.pixelformat = V4L2_PIX_FMT_JPEG,
+	}, {
+		.name = "1394",
+		.pixelformat = V4L2_PIX_FMT_DV,
+	}, {
+		.name = "MPEG-1/2/4",
+		.pixelformat = V4L2_PIX_FMT_MPEG,
+	}, {
+		.name = "H.264",
+		.pixelformat = V4L2_PIX_FMT_H264,
+	}, {
+		.name = "H.264 (without start codes)",
+		.pixelformat = V4L2_PIX_FMT_H264_NO_SC,
+	}, {
+		.name = "H.264 MVC",
+		.pixelformat = V4L2_PIX_FMT_H264_MVC,
+	}, {
+		.name = "H.263",
+		.pixelformat = V4L2_PIX_FMT_H263,
+	}, {
+		.name = "MPEG-1 ES",
+		.pixelformat = V4L2_PIX_FMT_MPEG1,
+	}, {
+		.name = "MPEG-2 ES",
+		.pixelformat = V4L2_PIX_FMT_MPEG2,
+	}, {
+		.name = "MPEG-4 part 2 ES",
+		.pixelformat = V4L2_PIX_FMT_MPEG4,
+	}, {
+		.name = "Xvid",
+		.pixelformat = V4L2_PIX_FMT_XVID,
+	}, {
+		.name = "VC-1 (SMPTE 412M Annex G)",
+		.pixelformat = V4L2_PIX_FMT_VC1_ANNEX_G,
+	}, {
+		.name = "VC-1 (SMPTE 412M Annex L)",
+		.pixelformat = V4L2_PIX_FMT_VC1_ANNEX_L,
+	}, {
+		.name = "VP8",
+		.pixelformat = V4L2_PIX_FMT_VP8,
+	},
+};
+
+const struct v4l2_pixfmt *v4l2_pixfmt_by_fourcc(u32 fourcc)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(v4l2_pixfmts); i++) {
+		if (v4l2_pixfmts[i].pixelformat == fourcc)
+			return v4l2_pixfmts + i;
+	}
+
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(v4l2_pixfmt_by_fourcc);
+
+int v4l2_fill_fmtdesc(struct v4l2_fmtdesc *f, u32 fourcc)
+{
+	const struct v4l2_pixfmt *fmt;
+
+	fmt = v4l2_pixfmt_by_fourcc(fourcc);
+	if (!fmt)
+		return -EINVAL;
+
+	strlcpy((char *)f->description, fmt->name, sizeof(f->description));
+	f->pixelformat = fmt->pixelformat;
+	f->flags = (fmt->bpp_image == 0) ? V4L2_FMT_FLAG_COMPRESSED : 0;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(v4l2_fill_fmtdesc);
+
+unsigned int v4l2_sizeimage(const struct v4l2_pixfmt *fmt, unsigned int width,
+			    unsigned int height)
+{
+	if (fmt->bpp_image == fmt->bpp_line)
+		return height * v4l2_bytesperline(fmt, width);
+	else
+		return height * width * fmt->bpp_image / 8;
+}
+EXPORT_SYMBOL_GPL(v4l2_sizeimage);
diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
index 48f9748..dbf7ea4 100644
--- a/include/media/v4l2-common.h
+++ b/include/media/v4l2-common.h
@@ -26,6 +26,7 @@
 #ifndef V4L2_COMMON_H_
 #define V4L2_COMMON_H_
 
+#include <linux/kernel.h>
 #include <media/v4l2-dev.h>
 
 /* Common printk constucts for v4l-i2c drivers. These macros create a unique
@@ -204,4 +205,46 @@ const struct v4l2_frmsize_discrete *v4l2_find_nearest_format(
 
 void v4l2_get_timestamp(struct timeval *tv);
 
+/**
+ * struct v4l2_pixfmt - internal V4L2 pixel format description
+ * @name: format description to be returned by enum_fmt
+ * @pixelformat: v4l2 pixel format fourcc
+ * @bpp_line: bits per pixel, averaged over a line (of the first plane
+ *            for planar formats), used to calculate bytesperline.
+ *            Zero for compressed and macroblock tiled formats.
+ * @bpp_image: bits per pixel, averaged over the whole image. This is used to
+ *             calculate sizeimage for uncompressed formats.
+ *             Zero for compressed formats.
+ * @planes: number of non-contiguous planes for multiplanar formats.
+ *          Zero for contiguous formats.
+ */
+struct v4l2_pixfmt {
+	const char	*name;
+	u32		pixelformat;
+	u8		bpp_line;
+	u8		bpp_image;
+	u8		planes;
+};
+
+const struct v4l2_pixfmt *v4l2_pixfmt_by_fourcc(u32 fourcc);
+int v4l2_fill_fmtdesc(struct v4l2_fmtdesc *f, u32 fourcc);
+unsigned int v4l2_sizeimage(const struct v4l2_pixfmt *fmt, unsigned int width,
+			    unsigned int height);
+
+static inline unsigned int v4l2_bytesperline(const struct v4l2_pixfmt *fmt,
+					     unsigned int width)
+{
+	return DIV_ROUND_UP(width * fmt->bpp_line, 8);
+}
+
+static inline bool v4l2_pixfmt_is_planar(const struct v4l2_pixfmt *fmt)
+{
+	return fmt->bpp_line && (fmt->bpp_line != fmt->bpp_image);
+}
+
+static inline bool v4l2_pixfmt_is_compressed(const struct v4l2_pixfmt *fmt)
+{
+	return fmt->bpp_image == 0;
+}
+
 #endif /* V4L2_COMMON_H_ */
-- 
2.1.0.rc1

Re: [RFC v2] [media] v4l2: add V4L2 pixel format array and helper functions

[Laurent Pinchart]

Hi Philipp,

Thank you for the patch.

On Tuesday 26 August 2014 11:00:54 Philipp Zabel wrote:
> This patch adds an array of V4L2 pixel formats and descriptions that can be
> used by drivers so that each driver doesn't have to provide its own slightly
> different format descriptions for VIDIOC_ENUM_FMT.
> 
> Each array entry also includes two bits per pixel values (for a single line
> and one for the whole image) that can be used to determine the
> v4l2_pix_format bytesperline and sizeimage values and whether the format is
> planar or compressed.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
> Changes since v1:
>  - Added use of DIV_ROUND_UP in v4l2_bytesperline
>  - Un-inlined v4l2_sizeimage and made it use v4l2_bytesperline
>    for non-planar, non-tiled formats
>  - Added .planes property to struct v4l2_pixfmt for
>    non-contiguous planar formats
>  - Fixed Y41P, YUV410, and YVU420M pixelformat values
> ---
>  drivers/media/v4l2-core/v4l2-common.c | 505 +++++++++++++++++++++++++++++++
>  include/media/v4l2-common.h           |  43 +++
>  2 files changed, 548 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-common.c
> b/drivers/media/v4l2-core/v4l2-common.c index ccaa38f..63b91a5 100644
> --- a/drivers/media/v4l2-core/v4l2-common.c
> +++ b/drivers/media/v4l2-core/v4l2-common.c
> @@ -533,3 +533,508 @@ void v4l2_get_timestamp(struct timeval *tv)
>  	tv->tv_usec = ts.tv_nsec / NSEC_PER_USEC;
>  }
>  EXPORT_SYMBOL_GPL(v4l2_get_timestamp);
> +
> +static const struct v4l2_pixfmt v4l2_pixfmts[] = {

[snip]

> +};
> +
> +const struct v4l2_pixfmt *v4l2_pixfmt_by_fourcc(u32 fourcc)
> +{
> +	int i;

The loop counter is always positive, it can be an unsigned int.

> +	for (i = 0; i < ARRAY_SIZE(v4l2_pixfmts); i++) {
> +		if (v4l2_pixfmts[i].pixelformat == fourcc)
> +			return v4l2_pixfmts + i;
> +	}

We currently have 123 pixel formats defined, and that number will keep 
increasing. I wonder if something more efficient than an O(n) array lookup 
would be worth it.

> +
> +	return NULL;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_pixfmt_by_fourcc);
> +
> +int v4l2_fill_fmtdesc(struct v4l2_fmtdesc *f, u32 fourcc)
> +{
> +	const struct v4l2_pixfmt *fmt;
> +
> +	fmt = v4l2_pixfmt_by_fourcc(fourcc);
> +	if (!fmt)
> +		return -EINVAL;
> +
> +	strlcpy((char *)f->description, fmt->name, sizeof(f->description));
> +	f->pixelformat = fmt->pixelformat;
> +	f->flags = (fmt->bpp_image == 0) ? V4L2_FMT_FLAG_COMPRESSED : 0;
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_fill_fmtdesc);
> +
> +unsigned int v4l2_sizeimage(const struct v4l2_pixfmt *fmt, unsigned int
> width,
> +			    unsigned int height)
> +{

A small comment would be useful here to explain why we don't round up in the 
second case.

> +	if (fmt->bpp_image == fmt->bpp_line)
> +		return height * v4l2_bytesperline(fmt, width);
> +	else
> +		return height * width * fmt->bpp_image / 8;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_sizeimage);
> diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
> index 48f9748..dbf7ea4 100644
> --- a/include/media/v4l2-common.h
> +++ b/include/media/v4l2-common.h
> @@ -26,6 +26,7 @@
>  #ifndef V4L2_COMMON_H_
>  #define V4L2_COMMON_H_
> 
> +#include <linux/kernel.h>
>  #include <media/v4l2-dev.h>
> 
>  /* Common printk constucts for v4l-i2c drivers. These macros create a
> unique @@ -204,4 +205,46 @@ const struct v4l2_frmsize_discrete
> *v4l2_find_nearest_format(
> 
>  void v4l2_get_timestamp(struct timeval *tv);
> 
> +/**
> + * struct v4l2_pixfmt - internal V4L2 pixel format description

Maybe struct v4l2_pixfmt_info ?

> + * @name: format description to be returned by enum_fmt
> + * @pixelformat: v4l2 pixel format fourcc
> + * @bpp_line: bits per pixel, averaged over a line (of the first plane
> + *            for planar formats), used to calculate bytesperline.
> + *            Zero for compressed and macroblock tiled formats.
> + * @bpp_image: bits per pixel, averaged over the whole image. This is used
> to
> + *             calculate sizeimage for uncompressed formats.
> + *             Zero for compressed formats.
> + * @planes: number of non-contiguous planes for multiplanar formats.
> + *          Zero for contiguous formats.
> + */
> +struct v4l2_pixfmt {
> +	const char	*name;
> +	u32		pixelformat;
> +	u8		bpp_line;
> +	u8		bpp_image;
> +	u8		planes;
> +};
> +
> +const struct v4l2_pixfmt *v4l2_pixfmt_by_fourcc(u32 fourcc);
> +int v4l2_fill_fmtdesc(struct v4l2_fmtdesc *f, u32 fourcc);
> +unsigned int v4l2_sizeimage(const struct v4l2_pixfmt *fmt, unsigned int
> width,
> +			    unsigned int height);
> +
> +static inline unsigned int v4l2_bytesperline(const struct v4l2_pixfmt *fmt,
> +					     unsigned int width)
> +{
> +	return DIV_ROUND_UP(width * fmt->bpp_line, 8);
> +}
> +
> +static inline bool v4l2_pixfmt_is_planar(const struct v4l2_pixfmt *fmt)
> +{
> +	return fmt->bpp_line && (fmt->bpp_line != fmt->bpp_image);
> +}
> +
> +static inline bool v4l2_pixfmt_is_compressed(const struct v4l2_pixfmt *fmt)
> +{
> +	return fmt->bpp_image == 0;
> +}
> +
>  #endif /* V4L2_COMMON_H_ */

-- 
Regards,

Laurent Pinchart

Re: [RFC v2] [media] v4l2: add V4L2 pixel format array and helper functions

[Philipp Zabel]

Hi Laurent,

thank you for the comments.

Am Dienstag, den 26.08.2014, 12:01 +0200 schrieb Laurent Pinchart:
[...]
> > +};
> > +
> > +const struct v4l2_pixfmt *v4l2_pixfmt_by_fourcc(u32 fourcc)
> > +{
> > +	int i;
> 
> The loop counter is always positive, it can be an unsigned int.

I'll change that.

> > +	for (i = 0; i < ARRAY_SIZE(v4l2_pixfmts); i++) {
> > +		if (v4l2_pixfmts[i].pixelformat == fourcc)
> > +			return v4l2_pixfmts + i;
> > +	}
> 
> We currently have 123 pixel formats defined, and that number will keep 
> increasing. I wonder if something more efficient than an O(n) array lookup 
> would be worth it.

How about a function similar to soc_mbus_find_fmtdesc that uses an array
provided by the driver:

const struct v4l2_pixfmt_info *v4l2_find_pixfmt(u32 pixelformat,
		const struct v4l2_pixfmt_info *array, unsigned int len)
{
	unsigned int i;

	for (i = 0; i < len; i++) {
		if (pixelformat == array[i].pixelformat)
			return array + i;
	}

	return NULL;
}

And a function to fill this driver specific array from the global array
once:

void v4l2_init_pixfmt_array(struct v4l2_pixfmt_info *array, int len)
{
	unsigned int i;

	for (i = 0; i < len; i++)
		array[i] = *v4l2_pixfmt_by_fourcc(array[i].pixelformat);
}

A driver could then do the following:

static struct v4l2_pixfmt_info driver_formats[] = {
	{ .pixelformat = V4L2_PIX_FMT_YUYV },
	{ .pixelformat = V4L2_PIX_FMT_YUV420 },
};

int driver_probe(...)
{
	...
	v4l2_init_pixfmt_array(driver_formats,
			ARRAY_SIZE(driver_formats));
	...
}

[...]
> > +unsigned int v4l2_sizeimage(const struct v4l2_pixfmt *fmt, unsigned int
> > width,
> > +			    unsigned int height)
> > +{
> 
> A small comment would be useful here to explain why we don't round up in the 
> second case.

Agreed, I think the YUV410 case is a good example for this.

[...]
> > +/**
> > + * struct v4l2_pixfmt - internal V4L2 pixel format description
> 
> Maybe struct v4l2_pixfmt_info ?

That's fine with me.

regards
Philipp

Re: [RFC v2] [media] v4l2: add V4L2 pixel format array and helper functions

[Laurent Pinchart]

Hi Philipp,

On Wednesday 27 August 2014 11:30:14 Philipp Zabel wrote:
> Am Dienstag, den 26.08.2014, 12:01 +0200 schrieb Laurent Pinchart:
> [...]
> 
> > > +};
> > > +
> > > +const struct v4l2_pixfmt *v4l2_pixfmt_by_fourcc(u32 fourcc)
> > > +{
> > > +	int i;
> > 
> > The loop counter is always positive, it can be an unsigned int.
> 
> I'll change that.
> 
> > > +	for (i = 0; i < ARRAY_SIZE(v4l2_pixfmts); i++) {
> > > +		if (v4l2_pixfmts[i].pixelformat == fourcc)
> > > +			return v4l2_pixfmts + i;
> > > +	}
> > 
> > We currently have 123 pixel formats defined, and that number will keep
> > increasing. I wonder if something more efficient than an O(n) array lookup
> > would be worth it.
> 
> How about a function similar to soc_mbus_find_fmtdesc that uses an array
> provided by the driver:
> 
> const struct v4l2_pixfmt_info *v4l2_find_pixfmt(u32 pixelformat,
> 		const struct v4l2_pixfmt_info *array, unsigned int len)
> {
> 	unsigned int i;
> 
> 	for (i = 0; i < len; i++) {
> 		if (pixelformat == array[i].pixelformat)
> 			return array + i;
> 	}
> 
> 	return NULL;
> }
> 
> And a function to fill this driver specific array from the global array
> once:
> 
> void v4l2_init_pixfmt_array(struct v4l2_pixfmt_info *array, int len)
> {
> 	unsigned int i;
> 
> 	for (i = 0; i < len; i++)
> 		array[i] = *v4l2_pixfmt_by_fourcc(array[i].pixelformat);
> }
> 
> A driver could then do the following:
> 
> static struct v4l2_pixfmt_info driver_formats[] = {
> 	{ .pixelformat = V4L2_PIX_FMT_YUYV },
> 	{ .pixelformat = V4L2_PIX_FMT_YUV420 },
> };
> 
> int driver_probe(...)
> {
> 	...
> 	v4l2_init_pixfmt_array(driver_formats,
> 			ARRAY_SIZE(driver_formats));
> 	...
> }

Good question. This option consumes more memory, and prevents the driver-
specific format info arrays to be const, which bothers me a bit. On the other 
hand it allows drivers to override some of the default values for odd cases. I 
won't nack this approach, but I'm wondering whether a better solution wouldn't 
be possible. Hans, Mauro, Guennadi, any opinion ?

> [...]
> 
> > > +unsigned int v4l2_sizeimage(const struct v4l2_pixfmt *fmt, unsigned int
> > > width,
> > > +			    unsigned int height)
> > > +{
> > 
> > A small comment would be useful here to explain why we don't round up in
> > the second case.
> 
> Agreed, I think the YUV410 case is a good example for this.
> 
> [...]
> 
> > > +/**
> > > + * struct v4l2_pixfmt - internal V4L2 pixel format description
> > 
> > Maybe struct v4l2_pixfmt_info ?
> 
> That's fine with me.

-- 
Regards,

Laurent Pinchart

Re: [RFC v2] [media] v4l2: add V4L2 pixel format array and helper functions

[Philipp Zabel]

Hi Laurent,

Am Donnerstag, den 28.08.2014, 14:24 +0200 schrieb Laurent Pinchart:
> > A driver could then do the following:
> > 
> > static struct v4l2_pixfmt_info driver_formats[] = {
> > 	{ .pixelformat = V4L2_PIX_FMT_YUYV },
> > 	{ .pixelformat = V4L2_PIX_FMT_YUV420 },
> > };
> > 
> > int driver_probe(...)
> > {
> > 	...
> > 	v4l2_init_pixfmt_array(driver_formats,
> > 			ARRAY_SIZE(driver_formats));
> > 	...
> > }
> 
> Good question. This option consumes more memory, and prevents the driver-
> specific format info arrays to be const, which bothers me a bit.

Also, this wouldn't help drivers that don't want to take these
additional steps, which probably includes a lot of camera drivers with
only a few formats.

> On the other hand it allows drivers to override some of the default
> values for odd cases.

Hm, but those cases don't have to use the v4l2_pixfmt_info at all.

> I won't nack this approach, but I'm wondering whether a better
> solution wouldn't be possible. Hans, Mauro, Guennadi, any opinion ?

We could keep the global v4l2_pixfmt_info array sorted by fourcc value
and do a binary search (would have to be kept in mind when adding new
formats) or build a hash table (more complicated code, consumes memory).

regards
Philipp

Re: [RFC v2] [media] v4l2: add V4L2 pixel format array and helper functions

[Laurent Pinchart]

Hi Philipp,

On Thursday 28 August 2014 18:09:35 Philipp Zabel wrote:
> Am Donnerstag, den 28.08.2014, 14:24 +0200 schrieb Laurent Pinchart:
> > > A driver could then do the following:
> > > 
> > > static struct v4l2_pixfmt_info driver_formats[] = {
> > > 
> > > 	{ .pixelformat = V4L2_PIX_FMT_YUYV },
> > > 	{ .pixelformat = V4L2_PIX_FMT_YUV420 },
> > > 
> > > };
> > > 
> > > int driver_probe(...)
> > > {
> > > 
> > > 	...
> > > 	v4l2_init_pixfmt_array(driver_formats,
> > > 	
> > > 			ARRAY_SIZE(driver_formats));
> > > 	
> > > 	...
> > > 
> > > }
> > 
> > Good question. This option consumes more memory, and prevents the driver-
> > specific format info arrays to be const, which bothers me a bit.
> 
> Also, this wouldn't help drivers that don't want to take these
> additional steps, which probably includes a lot of camera drivers with
> only a few formats.
> 
> > On the other hand it allows drivers to override some of the default
> > values for odd cases.
> 
> Hm, but those cases don't have to use the v4l2_pixfmt_info at all.
> 
> > I won't nack this approach, but I'm wondering whether a better
> > solution wouldn't be possible. Hans, Mauro, Guennadi, any opinion ?
> 
> We could keep the global v4l2_pixfmt_info array sorted by fourcc value
> and do a binary search (would have to be kept in mind when adding new
> formats)

I like that option, provided we can ensure that the array is sorted. This can 
get a bit tricky, and Hans might wear his "don't over-optimize" hat :-)

> or build a hash table (more complicated code, consumes memory).

-- 
Regards,

Laurent Pinchart

Re: [RFC v2] [media] v4l2: add V4L2 pixel format array and helper functions

[Hans Verkuil]

On 08/28/2014 06:25 PM, Laurent Pinchart wrote:
> Hi Philipp,
> 
> On Thursday 28 August 2014 18:09:35 Philipp Zabel wrote:
>> Am Donnerstag, den 28.08.2014, 14:24 +0200 schrieb Laurent Pinchart:
>>>> A driver could then do the following:
>>>>
>>>> static struct v4l2_pixfmt_info driver_formats[] = {
>>>>
>>>> 	{ .pixelformat = V4L2_PIX_FMT_YUYV },
>>>> 	{ .pixelformat = V4L2_PIX_FMT_YUV420 },
>>>>
>>>> };
>>>>
>>>> int driver_probe(...)
>>>> {
>>>>
>>>> 	...
>>>> 	v4l2_init_pixfmt_array(driver_formats,
>>>> 	
>>>> 			ARRAY_SIZE(driver_formats));
>>>> 	
>>>> 	...
>>>>
>>>> }
>>>
>>> Good question. This option consumes more memory, and prevents the driver-
>>> specific format info arrays to be const, which bothers me a bit.
>>
>> Also, this wouldn't help drivers that don't want to take these
>> additional steps, which probably includes a lot of camera drivers with
>> only a few formats.
>>
>>> On the other hand it allows drivers to override some of the default
>>> values for odd cases.
>>
>> Hm, but those cases don't have to use the v4l2_pixfmt_info at all.
>>
>>> I won't nack this approach, but I'm wondering whether a better
>>> solution wouldn't be possible. Hans, Mauro, Guennadi, any opinion ?
>>
>> We could keep the global v4l2_pixfmt_info array sorted by fourcc value
>> and do a binary search (would have to be kept in mind when adding new
>> formats)
> 
> I like that option, provided we can ensure that the array is sorted. This can 
> get a bit tricky, and Hans might wear his "don't over-optimize" hat :-)

Well, for small sets of data (which this is) a binary search may well be
slower than a simple search. So yes, you should do some performance tests
before going with the more complex option.

By placing the commonly used pixel formats at the beginning of the list I
suspect a simple search is the fastest lookup method, and very easy to
implement as well.

Regards,

	Hans

> 
>> or build a hash table (more complicated code, consumes memory).
> 

Re: [RFC v2] [media] v4l2: add V4L2 pixel format array and helper functions

[Mauro Carvalho Chehab]

Em Thu, 28 Aug 2014 18:40:53 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> On 08/28/2014 06:25 PM, Laurent Pinchart wrote:
> > Hi Philipp,
> > 
> > On Thursday 28 August 2014 18:09:35 Philipp Zabel wrote:
> >> Am Donnerstag, den 28.08.2014, 14:24 +0200 schrieb Laurent Pinchart:
> >>>> A driver could then do the following:
> >>>>
> >>>> static struct v4l2_pixfmt_info driver_formats[] = {
> >>>>
> >>>> 	{ .pixelformat = V4L2_PIX_FMT_YUYV },
> >>>> 	{ .pixelformat = V4L2_PIX_FMT_YUV420 },
> >>>>
> >>>> };
> >>>>
> >>>> int driver_probe(...)
> >>>> {
> >>>>
> >>>> 	...
> >>>> 	v4l2_init_pixfmt_array(driver_formats,
> >>>> 	
> >>>> 			ARRAY_SIZE(driver_formats));
> >>>> 	
> >>>> 	...
> >>>>
> >>>> }
> >>>
> >>> Good question. This option consumes more memory, and prevents the driver-
> >>> specific format info arrays to be const, which bothers me a bit.
> >>
> >> Also, this wouldn't help drivers that don't want to take these
> >> additional steps, which probably includes a lot of camera drivers with
> >> only a few formats.
> >>
> >>> On the other hand it allows drivers to override some of the default
> >>> values for odd cases.
> >>
> >> Hm, but those cases don't have to use the v4l2_pixfmt_info at all.
> >>
> >>> I won't nack this approach, but I'm wondering whether a better
> >>> solution wouldn't be possible. Hans, Mauro, Guennadi, any opinion ?
> >>
> >> We could keep the global v4l2_pixfmt_info array sorted by fourcc value
> >> and do a binary search (would have to be kept in mind when adding new
> >> formats)
> > 
> > I like that option, provided we can ensure that the array is sorted. This can 
> > get a bit tricky, and Hans might wear his "don't over-optimize" hat :-)

The big issue is that, afaikt, there's no way to make gcc to order it,
so the order would need to be manually ensured. This is challenging, and
makes the review process complex if done right.

I really don't see any gain on applying such patch. If the concern is
just about properly naming the pixel formats, it is a way easier to use
some defines for the names, and use the defines. Btw, that's how we solved
this issue at rc core:
	http://git.linuxtv.org/cgit.cgi/media_tree.git/tree/include/media/rc-map.h

Also, that means a less footprint for tiny Kernels.

> Well, for small sets of data (which this is) a binary search may well be
> slower than a simple search. So yes, you should do some performance tests
> before going with the more complex option.

with 128 pixformats, a binary search takes 8 ifs against 128. So, it
should be faster.

Yet, even on a very slow machine, seeking for 128 formats is still
likely fast enough to not affect performance of a media application.

> By placing the commonly used pixel formats at the beginning of the list I
> suspect a simple search is the fastest lookup method, and very easy to
> implement as well.

IMHO, we shouldn't apply this approach, as we're just growing the
Kernel size without any real benefit.

Regards,
Mauro

Re: [RFC v2] [media] v4l2: add V4L2 pixel format array and helper functions

[Hans Verkuil]

On 08/28/2014 07:18 PM, Mauro Carvalho Chehab wrote:
> Em Thu, 28 Aug 2014 18:40:53 +0200
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> 
>> On 08/28/2014 06:25 PM, Laurent Pinchart wrote:
>>> Hi Philipp,
>>>
>>> On Thursday 28 August 2014 18:09:35 Philipp Zabel wrote:
>>>> Am Donnerstag, den 28.08.2014, 14:24 +0200 schrieb Laurent Pinchart:
>>>>>> A driver could then do the following:
>>>>>>
>>>>>> static struct v4l2_pixfmt_info driver_formats[] = {
>>>>>>
>>>>>> 	{ .pixelformat = V4L2_PIX_FMT_YUYV },
>>>>>> 	{ .pixelformat = V4L2_PIX_FMT_YUV420 },
>>>>>>
>>>>>> };
>>>>>>
>>>>>> int driver_probe(...)
>>>>>> {
>>>>>>
>>>>>> 	...
>>>>>> 	v4l2_init_pixfmt_array(driver_formats,
>>>>>> 	
>>>>>> 			ARRAY_SIZE(driver_formats));
>>>>>> 	
>>>>>> 	...
>>>>>>
>>>>>> }
>>>>>
>>>>> Good question. This option consumes more memory, and prevents the driver-
>>>>> specific format info arrays to be const, which bothers me a bit.
>>>>
>>>> Also, this wouldn't help drivers that don't want to take these
>>>> additional steps, which probably includes a lot of camera drivers with
>>>> only a few formats.
>>>>
>>>>> On the other hand it allows drivers to override some of the default
>>>>> values for odd cases.
>>>>
>>>> Hm, but those cases don't have to use the v4l2_pixfmt_info at all.
>>>>
>>>>> I won't nack this approach, but I'm wondering whether a better
>>>>> solution wouldn't be possible. Hans, Mauro, Guennadi, any opinion ?
>>>>
>>>> We could keep the global v4l2_pixfmt_info array sorted by fourcc value
>>>> and do a binary search (would have to be kept in mind when adding new
>>>> formats)
>>>
>>> I like that option, provided we can ensure that the array is sorted. This can 
>>> get a bit tricky, and Hans might wear his "don't over-optimize" hat :-)
> 
> The big issue is that, afaikt, there's no way to make gcc to order it,
> so the order would need to be manually ensured. This is challenging, and
> makes the review process complex if done right.
> 
> I really don't see any gain on applying such patch. If the concern is
> just about properly naming the pixel formats, it is a way easier to use
> some defines for the names, and use the defines.

It's not just the names, also the bit depth etc. Most drivers need that information
and having it in a central place simplifies driver design. Yes, it slightly
increases the amount of memory, but that is insignificant compared to the huge
amount of memory necessary for video buffers. And reducing driver complexity is
always good since that has always been the main problem with drivers, not memory
or code performance.

> Btw, that's how we solved
> this issue at rc core:
> 	http://git.linuxtv.org/cgit.cgi/media_tree.git/tree/include/media/rc-map.h
> 
> Also, that means a less footprint for tiny Kernels.
> 
>> Well, for small sets of data (which this is) a binary search may well be
>> slower than a simple search. So yes, you should do some performance tests
>> before going with the more complex option.
> 
> with 128 pixformats, a binary search takes 8 ifs against 128.

Actually, that's 64 on average. Even less if you know that some formats will
be searched for a lot more frequently than others and you can order your data
accordingly.

> So, it
> should be faster.

Binary search has a lot more overhead than a simple array traversal. I did
experiments with this when I worked on the control framework, and it was
very surprising how slow binary search was compared to a simple linked list
traversal. I think I needed well over 100 elements before the binary search
became faster. You really need to test things like this if you know the data
set is relatively small.

> 
> Yet, even on a very slow machine, seeking for 128 formats is still
> likely fast enough to not affect performance of a media application.

I agree with that.
 
>> By placing the commonly used pixel formats at the beginning of the list I
>> suspect a simple search is the fastest lookup method, and very easy to
>> implement as well.
> 
> IMHO, we shouldn't apply this approach, as we're just growing the
> Kernel size without any real benefit.

Simplifying drivers is the real benefit.

Regards,

	Hans

Re: [RFC v2] [media] v4l2: add V4L2 pixel format array and helper functions

[Hans Verkuil]

On 08/28/2014 07:32 PM, Hans Verkuil wrote:
> On 08/28/2014 07:18 PM, Mauro Carvalho Chehab wrote:
>> Em Thu, 28 Aug 2014 18:40:53 +0200
>> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
>>
>>> On 08/28/2014 06:25 PM, Laurent Pinchart wrote:
>>>> Hi Philipp,
>>>>
>>>> On Thursday 28 August 2014 18:09:35 Philipp Zabel wrote:
>>>>> Am Donnerstag, den 28.08.2014, 14:24 +0200 schrieb Laurent Pinchart:
>>>>>>> A driver could then do the following:
>>>>>>>
>>>>>>> static struct v4l2_pixfmt_info driver_formats[] = {
>>>>>>>
>>>>>>> 	{ .pixelformat = V4L2_PIX_FMT_YUYV },
>>>>>>> 	{ .pixelformat = V4L2_PIX_FMT_YUV420 },
>>>>>>>
>>>>>>> };
>>>>>>>
>>>>>>> int driver_probe(...)
>>>>>>> {
>>>>>>>
>>>>>>> 	...
>>>>>>> 	v4l2_init_pixfmt_array(driver_formats,
>>>>>>> 	
>>>>>>> 			ARRAY_SIZE(driver_formats));
>>>>>>> 	
>>>>>>> 	...
>>>>>>>
>>>>>>> }
>>>>>>
>>>>>> Good question. This option consumes more memory, and prevents the driver-
>>>>>> specific format info arrays to be const, which bothers me a bit.
>>>>>
>>>>> Also, this wouldn't help drivers that don't want to take these
>>>>> additional steps, which probably includes a lot of camera drivers with
>>>>> only a few formats.
>>>>>
>>>>>> On the other hand it allows drivers to override some of the default
>>>>>> values for odd cases.
>>>>>
>>>>> Hm, but those cases don't have to use the v4l2_pixfmt_info at all.
>>>>>
>>>>>> I won't nack this approach, but I'm wondering whether a better
>>>>>> solution wouldn't be possible. Hans, Mauro, Guennadi, any opinion ?
>>>>>
>>>>> We could keep the global v4l2_pixfmt_info array sorted by fourcc value
>>>>> and do a binary search (would have to be kept in mind when adding new
>>>>> formats)
>>>>
>>>> I like that option, provided we can ensure that the array is sorted. This can 
>>>> get a bit tricky, and Hans might wear his "don't over-optimize" hat :-)
>>
>> The big issue is that, afaikt, there's no way to make gcc to order it,
>> so the order would need to be manually ensured. This is challenging, and
>> makes the review process complex if done right.
>>
>> I really don't see any gain on applying such patch. If the concern is
>> just about properly naming the pixel formats, it is a way easier to use
>> some defines for the names, and use the defines.
> 
> It's not just the names, also the bit depth etc. Most drivers need that information
> and having it in a central place simplifies driver design. Yes, it slightly
> increases the amount of memory, but that is insignificant compared to the huge
> amount of memory necessary for video buffers. And reducing driver complexity is
> always good since that has always been the main problem with drivers, not memory
> or code performance.

I just want to add that we should try out any core solution with an existing driver
(e.g. saa7134) to see if whatever solution we come up with actually makes drivers
less complex. The saa7134 is from what I've seen fairly representative of most in
that is has additional fields besides the name, fourcc and depth that are driver
specific. So how will that be handled...

Regards,

	Hans

Re: [RFC v2] [media] v4l2: add V4L2 pixel format array and helper functions

[Philipp Zabel]

Hi Hans,

Am Donnerstag, den 28.08.2014, 19:41 +0200 schrieb Hans Verkuil:
> On 08/28/2014 07:32 PM, Hans Verkuil wrote:
> > On 08/28/2014 07:18 PM, Mauro Carvalho Chehab wrote:
> >> I really don't see any gain on applying such patch. If the concern is
> >> just about properly naming the pixel formats, it is a way easier to use
> >> some defines for the names, and use the defines.
> > 
> > It's not just the names, also the bit depth etc. Most drivers need that information
> > and having it in a central place simplifies driver design. Yes, it slightly
> > increases the amount of memory, but that is insignificant compared to the huge
> > amount of memory necessary for video buffers. And reducing driver complexity is
> > always good since that has always been the main problem with drivers, not memory
> > or code performance.
> 
> I just want to add that we should try out any core solution with an existing driver
> (e.g. saa7134) to see if whatever solution we come up with actually makes drivers
> less complex. The saa7134 is from what I've seen fairly representative of most in
> that is has additional fields besides the name, fourcc and depth that are driver
> specific. So how will that be handled...

my main motivation was unification of the format description strings
(well, not wanting to come up with possibly new ones for every driver),
but reducing driver complexity is a nice side effect.

I think saa7134 won't benefit a whole lot. It still needs to keep that
list for the custom saa7134_format fields, but at least the common ones
can be moved into a struct v4l2_pixfmt_info.

The global array mostly has the potential to remove a little boilerplate
from a lot of the simple cases that are happy with the per-format info
already provided by the array.

regards
Philipp