[PATCH 0/2] media: Introduce pixel format helpers

[PATCH 0/2] media: Introduce pixel format helpers

[Ezequiel Garcia]

Following the "Add MPEG-2 decoding to Rockchip VPU"[1], and given we want to start
using the pixel format helpers rather soon, I've decided to submit them separatedly.

Aside from documenting the block_w and block_h, as suggested by Emil Velikov, these
two patches are unmodified compared to v2 [1].

This series contains the minimum patches to introduce the pixel format
helpers, it currently will have no users, but we expect users to start
popping-up soon.
very shortly [2].

To avoid confusing the patches here with those on v2 [1], I'm marking them as v3.

Thanks!

[1] https://patchwork.kernel.org/cover/10838327/
[2] https://patchwork.kernel.org/patch/10851639/

Ezequiel Garcia (2):
  rockchip/vpu: Rename pixel format helpers
  media: Introduce helpers to fill pixel format structs

 drivers/media/v4l2-core/v4l2-common.c         | 164 ++++++++++++++++++
 .../media/rockchip/vpu/rockchip_vpu_enc.c     |  12 +-
 include/media/v4l2-common.h                   |  33 ++++
 3 files changed, 203 insertions(+), 6 deletions(-)

-- 
2.20.1

[PATCH v3 1/2] rockchip/vpu: Rename pixel format helpers

[Ezequiel Garcia]

The rockchip VPU driver uses generic names for its pixel format
helpers. We want to use the same names for generic versions
of these helpers, so rename the rockchip ones.

The driver will be switched to the generic helpers later.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 .../staging/media/rockchip/vpu/rockchip_vpu_enc.c    | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/media/rockchip/vpu/rockchip_vpu_enc.c b/drivers/staging/media/rockchip/vpu/rockchip_vpu_enc.c
index ab0fb2053620..fb5e36aedd8c 100644
--- a/drivers/staging/media/rockchip/vpu/rockchip_vpu_enc.c
+++ b/drivers/staging/media/rockchip/vpu/rockchip_vpu_enc.c
@@ -41,7 +41,7 @@
  * @is_compressed: Is it a compressed format?
  * @multiplanar: Is it a multiplanar variant format? (e.g. NV12M)
  */
-struct v4l2_format_info {
+struct rockchip_vpu_v4l2_format_info {
 	u32 format;
 	u32 header_size;
 	u8 num_planes;
@@ -52,10 +52,10 @@ struct v4l2_format_info {
 	u8 multiplanar;
 };
 
-static const struct v4l2_format_info *
-v4l2_format_info(u32 format)
+static const struct rockchip_vpu_v4l2_format_info *
+rockchip_vpu_v4l2_format_info(u32 format)
 {
-	static const struct v4l2_format_info formats[] = {
+	static const struct rockchip_vpu_v4l2_format_info formats[] = {
 		{ .format = V4L2_PIX_FMT_YUV420M,	.num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 2, .multiplanar = 1 },
 		{ .format = V4L2_PIX_FMT_NV12M,		.num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 2, .multiplanar = 1 },
 		{ .format = V4L2_PIX_FMT_YUYV,		.num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1 },
@@ -76,11 +76,11 @@ static void
 fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt,
 	       int pixelformat, int width, int height)
 {
-	const struct v4l2_format_info *info;
+	const struct rockchip_vpu_v4l2_format_info *info;
 	struct v4l2_plane_pix_format *plane;
 	int i;
 
-	info = v4l2_format_info(pixelformat);
+	info = rockchip_vpu_v4l2_format_info(pixelformat);
 	if (!info)
 		return;
 
-- 
2.20.1

[PATCH v3 2/2] media: Introduce helpers to fill pixel format structs

[Ezequiel Garcia]

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 padding 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/v4l2-common.c | 164 ++++++++++++++++++++++++++
 include/media/v4l2-common.h           |  33 ++++++
 2 files changed, 197 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
index 663730f088cd..d869a2910435 100644
--- a/drivers/media/v4l2-core/v4l2-common.c
+++ b/drivers/media/v4l2-core/v4l2-common.c
@@ -445,3 +445,167 @@ int v4l2_s_parm_cap(struct video_device *vdev,
 	return ret;
 }
 EXPORT_SYMBOL_GPL(v4l2_s_parm_cap);
+
+const struct v4l2_format_info *v4l2_format_info(u32 format)
+{
+	static const struct v4l2_format_info formats[] = {
+		/* RGB formats */
+		{ .format = V4L2_PIX_FMT_BGR24,   .mem_planes = 1, .comp_planes = 1, .bpp = { 3, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 },
+		{ .format = V4L2_PIX_FMT_RGB24,   .mem_planes = 1, .comp_planes = 1, .bpp = { 3, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 },
+		{ .format = V4L2_PIX_FMT_HSV24,   .mem_planes = 1, .comp_planes = 1, .bpp = { 3, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 },
+		{ .format = V4L2_PIX_FMT_BGR32,   .mem_planes = 1, .comp_planes = 1, .bpp = { 4, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 },
+		{ .format = V4L2_PIX_FMT_XBGR32,  .mem_planes = 1, .comp_planes = 1, .bpp = { 4, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 },
+		{ .format = V4L2_PIX_FMT_RGB32,   .mem_planes = 1, .comp_planes = 1, .bpp = { 4, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 },
+		{ .format = V4L2_PIX_FMT_XRGB32,  .mem_planes = 1, .comp_planes = 1, .bpp = { 4, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 },
+		{ .format = V4L2_PIX_FMT_HSV32,   .mem_planes = 1, .comp_planes = 1, .bpp = { 4, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 },
+		{ .format = V4L2_PIX_FMT_ARGB32,  .mem_planes = 1, .comp_planes = 1, .bpp = { 4, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 },
+		{ .format = V4L2_PIX_FMT_ABGR32,  .mem_planes = 1, .comp_planes = 1, .bpp = { 4, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 },
+		{ .format = V4L2_PIX_FMT_GREY,    .mem_planes = 1, .comp_planes = 1, .bpp = { 1, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 },
+
+		/* YUV packed formats */
+		{ .format = V4L2_PIX_FMT_YUYV,    .mem_planes = 1, .comp_planes = 1, .bpp = { 2, 0, 0, 0 }, .hdiv = 2, .vdiv = 1 },
+		{ .format = V4L2_PIX_FMT_YVYU,    .mem_planes = 1, .comp_planes = 1, .bpp = { 2, 0, 0, 0 }, .hdiv = 2, .vdiv = 1 },
+		{ .format = V4L2_PIX_FMT_UYVY,    .mem_planes = 1, .comp_planes = 1, .bpp = { 2, 0, 0, 0 }, .hdiv = 2, .vdiv = 1 },
+		{ .format = V4L2_PIX_FMT_VYUY,    .mem_planes = 1, .comp_planes = 1, .bpp = { 2, 0, 0, 0 }, .hdiv = 2, .vdiv = 1 },
+
+		/* YUV planar formats */
+		{ .format = V4L2_PIX_FMT_NV12,    .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .hdiv = 2, .vdiv = 2 },
+		{ .format = V4L2_PIX_FMT_NV21,    .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .hdiv = 2, .vdiv = 2 },
+		{ .format = V4L2_PIX_FMT_NV16,    .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .hdiv = 2, .vdiv = 1 },
+		{ .format = V4L2_PIX_FMT_NV61,    .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .hdiv = 2, .vdiv = 1 },
+		{ .format = V4L2_PIX_FMT_NV24,    .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .hdiv = 1, .vdiv = 1 },
+		{ .format = V4L2_PIX_FMT_NV42,    .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .hdiv = 1, .vdiv = 1 },
+
+		{ .format = V4L2_PIX_FMT_YUV410,  .mem_planes = 1, .comp_planes = 3, .bpp = { 1, 1, 1, 0 }, .hdiv = 4, .vdiv = 4 },
+		{ .format = V4L2_PIX_FMT_YVU410,  .mem_planes = 1, .comp_planes = 3, .bpp = { 1, 1, 1, 0 }, .hdiv = 4, .vdiv = 4 },
+		{ .format = V4L2_PIX_FMT_YUV411P, .mem_planes = 1, .comp_planes = 3, .bpp = { 1, 1, 1, 0 }, .hdiv = 4, .vdiv = 1 },
+		{ .format = V4L2_PIX_FMT_YUV420,  .mem_planes = 1, .comp_planes = 3, .bpp = { 1, 1, 1, 0 }, .hdiv = 2, .vdiv = 2 },
+		{ .format = V4L2_PIX_FMT_YVU420,  .mem_planes = 1, .comp_planes = 3, .bpp = { 1, 1, 1, 0 }, .hdiv = 2, .vdiv = 2 },
+		{ .format = V4L2_PIX_FMT_YUV422P, .mem_planes = 1, .comp_planes = 3, .bpp = { 1, 1, 1, 0 }, .hdiv = 2, .vdiv = 1 },
+
+		/* YUV planar formats, non contiguous variant */
+		{ .format = V4L2_PIX_FMT_YUV420M, .mem_planes = 3, .comp_planes = 3, .bpp = { 1, 1, 1, 0 }, .hdiv = 2, .vdiv = 2 },
+		{ .format = V4L2_PIX_FMT_YVU420M, .mem_planes = 3, .comp_planes = 3, .bpp = { 1, 1, 1, 0 }, .hdiv = 2, .vdiv = 2 },
+		{ .format = V4L2_PIX_FMT_YUV422M, .mem_planes = 3, .comp_planes = 3, .bpp = { 1, 1, 1, 0 }, .hdiv = 2, .vdiv = 1 },
+		{ .format = V4L2_PIX_FMT_YVU422M, .mem_planes = 3, .comp_planes = 3, .bpp = { 1, 1, 1, 0 }, .hdiv = 2, .vdiv = 1 },
+		{ .format = V4L2_PIX_FMT_YUV444M, .mem_planes = 3, .comp_planes = 3, .bpp = { 1, 1, 1, 0 }, .hdiv = 1, .vdiv = 1 },
+		{ .format = V4L2_PIX_FMT_YVU444M, .mem_planes = 3, .comp_planes = 3, .bpp = { 1, 1, 1, 0 }, .hdiv = 1, .vdiv = 1 },
+
+		{ .format = V4L2_PIX_FMT_NV12M,   .mem_planes = 2, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .hdiv = 2, .vdiv = 2 },
+		{ .format = V4L2_PIX_FMT_NV21M,   .mem_planes = 2, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .hdiv = 2, .vdiv = 2 },
+		{ .format = V4L2_PIX_FMT_NV16M,   .mem_planes = 2, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .hdiv = 2, .vdiv = 1 },
+		{ .format = V4L2_PIX_FMT_NV61M,   .mem_planes = 2, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .hdiv = 2, .vdiv = 1 },
+	};
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(formats); ++i)
+		if (formats[i].format == format)
+			return &formats[i];
+	return NULL;
+}
+EXPORT_SYMBOL(v4l2_format_info);
+
+static inline unsigned int v4l2_format_block_width(const struct v4l2_format_info *info, int plane)
+{
+	if (!info->block_w[plane])
+		return 1;
+	return info->block_w[plane];
+}
+
+static inline unsigned int v4l2_format_block_height(const struct v4l2_format_info *info, int plane)
+{
+	if (!info->block_h[plane])
+		return 1;
+	return info->block_h[plane];
+}
+
+int 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 -EINVAL;
+
+	pixfmt->width = width;
+	pixfmt->height = height;
+	pixfmt->pixelformat = pixelformat;
+	pixfmt->num_planes = info->mem_planes;
+
+	if (info->mem_planes == 1) {
+		plane = &pixfmt->plane_fmt[0];
+		plane->bytesperline = ALIGN(width, v4l2_format_block_width(info, 0)) * info->bpp[0];
+		plane->sizeimage = 0;
+
+		for (i = 0; i < info->comp_planes; i++) {
+			unsigned int hdiv = (i == 0) ? 1 : info->hdiv;
+			unsigned int vdiv = (i == 0) ? 1 : info->vdiv;
+			unsigned int aligned_width;
+			unsigned int aligned_height;
+
+			aligned_width = ALIGN(width, v4l2_format_block_width(info, i));
+			aligned_height = ALIGN(height, v4l2_format_block_height(info, i));
+
+			plane->sizeimage += info->bpp[i] *
+				DIV_ROUND_UP(aligned_width, hdiv) *
+				DIV_ROUND_UP(aligned_height, vdiv);
+		}
+	} else {
+		for (i = 0; i < info->comp_planes; i++) {
+			unsigned int hdiv = (i == 0) ? 1 : info->hdiv;
+			unsigned int vdiv = (i == 0) ? 1 : info->vdiv;
+			unsigned int aligned_width;
+			unsigned int aligned_height;
+
+			aligned_width = ALIGN(width, v4l2_format_block_width(info, i));
+			aligned_height = ALIGN(height, v4l2_format_block_height(info, i));
+
+			plane = &pixfmt->plane_fmt[i];
+			plane->bytesperline =
+				info->bpp[i] * DIV_ROUND_UP(aligned_width, hdiv);
+			plane->sizeimage =
+				plane->bytesperline * DIV_ROUND_UP(aligned_height, vdiv);
+		}
+	}
+	return 0;
+}
+EXPORT_SYMBOL_GPL(v4l2_fill_pixfmt_mp);
+
+int 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 -EINVAL;
+
+	/* Single planar API cannot be used for multi plane formats. */
+	if (info->mem_planes > 1)
+		return -EINVAL;
+
+	pixfmt->width = width;
+	pixfmt->height = height;
+	pixfmt->pixelformat = pixelformat;
+	pixfmt->bytesperline = ALIGN(width, v4l2_format_block_width(info, 0)) * info->bpp[0];
+	pixfmt->sizeimage = 0;
+
+	for (i = 0; i < info->comp_planes; i++) {
+		unsigned int hdiv = (i == 0) ? 1 : info->hdiv;
+		unsigned int vdiv = (i == 0) ? 1 : info->vdiv;
+		unsigned int aligned_width;
+		unsigned int aligned_height;
+
+		aligned_width = ALIGN(width, v4l2_format_block_width(info, i));
+		aligned_height = ALIGN(height, v4l2_format_block_height(info, i));
+
+		pixfmt->sizeimage += info->bpp[i] *
+			DIV_ROUND_UP(aligned_width, hdiv) *
+			DIV_ROUND_UP(aligned_height, vdiv);
+	}
+	return 0;
+}
+EXPORT_SYMBOL_GPL(v4l2_fill_pixfmt);
diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
index 2b93cb281fa5..0a41bbecf3d3 100644
--- a/include/media/v4l2-common.h
+++ b/include/media/v4l2-common.h
@@ -392,4 +392,37 @@ int v4l2_s_parm_cap(struct video_device *vdev,
 	((u64)(a).numerator * (b).denominator OP	\
 	(u64)(b).numerator * (a).denominator)
 
+/* ------------------------------------------------------------------------- */
+
+/* Pixel format and FourCC helpers */
+
+/**
+ * struct v4l2_format_info - information about a V4L2 format
+ * @format: 4CC format identifier (V4L2_PIX_FMT_*)
+ * @mem_planes: Number of memory planes, which includes the alpha plane (1 to 4).
+ * @comp_planes: Number of component planes, which includes the alpha plane (1 to 4).
+ * @bpp: Array of per-plane bytes per pixel
+ * @hdiv: Horizontal chroma subsampling factor
+ * @vdiv: Vertical chroma subsampling factor
+ * @block_w: Per-plane macroblock pixel width (optional)
+ * @block_h: Per-plane macroblock pixel height (optional)
+ */
+struct v4l2_format_info {
+	u32 format;
+	u8 mem_planes;
+	u8 comp_planes;
+	u8 bpp[4];
+	u8 hdiv;
+	u8 vdiv;
+	u8 block_w[4];
+	u8 block_h[4];
+};
+
+const struct v4l2_format_info *v4l2_format_info(u32 format);
+
+int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, int pixelformat,
+		     int width, int height);
+int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt, int pixelformat,
+			int width, int height);
+
 #endif /* V4L2_COMMON_H_ */
-- 
2.20.1

Re: [PATCH v3 2/2] media: Introduce helpers to fill pixel format structs

[Jacopo Mondi]

[-- Attachment #1: Type: text/plain, Size: 13233 bytes --]

Hi Ezequiel,
   this is very nice, thank you!

On Thu, Mar 28, 2019 at 03:07:04PM -0300, 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 padding 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/v4l2-common.c | 164 ++++++++++++++++++++++++++
>  include/media/v4l2-common.h           |  33 ++++++
>  2 files changed, 197 insertions(+)
>
> diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> index 663730f088cd..d869a2910435 100644
> --- a/drivers/media/v4l2-core/v4l2-common.c
> +++ b/drivers/media/v4l2-core/v4l2-common.c
> @@ -445,3 +445,167 @@ int v4l2_s_parm_cap(struct video_device *vdev,
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(v4l2_s_parm_cap);
> +
> +const struct v4l2_format_info *v4l2_format_info(u32 format)
> +{
> +	static const struct v4l2_format_info formats[] = {
> +		/* RGB formats */
> +		{ .format = V4L2_PIX_FMT_BGR24,   .mem_planes = 1, .comp_planes = 1, .bpp = { 3, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 },

How would you feel using macros here, this would help sticking to
80-or-so columns, as otherwise this table is pretty wide.

I've sketched out something quick:

#define V4L2_FMT_INFO_BPP(p1_, p2_, p3_, p4_) { p1_, p2_, p3_, p4_ }
#define V4L2_FMT_INFO(pf_, mpl_, cpl_, bpp_, hd_, vd_)			\
		{ 							\
		  .format       = V4L2_PIX_FMT_##pf_, 			\
		  .mem_planes   = mpl_,					\
		  .comp_planes  = cpl_,					\
		  .bpp          = bpp_, 				\
		  .hdiv         = hd_,					\
		  .vdiv         = vd_ 					\
		}

const struct v4l2_format_info *v4l2_format_info(u32 format)
{
	static const struct v4l2_format_info formats[] = {
		/* RGB formats */
       		V4L2_FMT_INFO(BGR24, 1, 1, V4L2_FMT_INFO_BPP(3, 0, 0, 0), 1, 1),
                ...
}

Not sure you might like it or not.

> +		{ .format = V4L2_PIX_FMT_RGB24,   .mem_planes = 1, .comp_planes = 1, .bpp = { 3, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 },
> +		{ .format = V4L2_PIX_FMT_HSV24,   .mem_planes = 1, .comp_planes = 1, .bpp = { 3, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 },
> +		{ .format = V4L2_PIX_FMT_BGR32,   .mem_planes = 1, .comp_planes = 1, .bpp = { 4, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 },
> +		{ .format = V4L2_PIX_FMT_XBGR32,  .mem_planes = 1, .comp_planes = 1, .bpp = { 4, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 },
> +		{ .format = V4L2_PIX_FMT_RGB32,   .mem_planes = 1, .comp_planes = 1, .bpp = { 4, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 },
> +		{ .format = V4L2_PIX_FMT_XRGB32,  .mem_planes = 1, .comp_planes = 1, .bpp = { 4, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 },
> +		{ .format = V4L2_PIX_FMT_HSV32,   .mem_planes = 1, .comp_planes = 1, .bpp = { 4, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 },
> +		{ .format = V4L2_PIX_FMT_ARGB32,  .mem_planes = 1, .comp_planes = 1, .bpp = { 4, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 },
> +		{ .format = V4L2_PIX_FMT_ABGR32,  .mem_planes = 1, .comp_planes = 1, .bpp = { 4, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 },
> +		{ .format = V4L2_PIX_FMT_GREY,    .mem_planes = 1, .comp_planes = 1, .bpp = { 1, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 },
> +
> +		/* YUV packed formats */
> +		{ .format = V4L2_PIX_FMT_YUYV,    .mem_planes = 1, .comp_planes = 1, .bpp = { 2, 0, 0, 0 }, .hdiv = 2, .vdiv = 1 },
> +		{ .format = V4L2_PIX_FMT_YVYU,    .mem_planes = 1, .comp_planes = 1, .bpp = { 2, 0, 0, 0 }, .hdiv = 2, .vdiv = 1 },
> +		{ .format = V4L2_PIX_FMT_UYVY,    .mem_planes = 1, .comp_planes = 1, .bpp = { 2, 0, 0, 0 }, .hdiv = 2, .vdiv = 1 },
> +		{ .format = V4L2_PIX_FMT_VYUY,    .mem_planes = 1, .comp_planes = 1, .bpp = { 2, 0, 0, 0 }, .hdiv = 2, .vdiv = 1 },
> +
> +		/* YUV planar formats */
> +		{ .format = V4L2_PIX_FMT_NV12,    .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .hdiv = 2, .vdiv = 2 },
> +		{ .format = V4L2_PIX_FMT_NV21,    .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .hdiv = 2, .vdiv = 2 },
> +		{ .format = V4L2_PIX_FMT_NV16,    .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .hdiv = 2, .vdiv = 1 },
> +		{ .format = V4L2_PIX_FMT_NV61,    .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .hdiv = 2, .vdiv = 1 },
> +		{ .format = V4L2_PIX_FMT_NV24,    .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .hdiv = 1, .vdiv = 1 },
> +		{ .format = V4L2_PIX_FMT_NV42,    .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .hdiv = 1, .vdiv = 1 },
> +
> +		{ .format = V4L2_PIX_FMT_YUV410,  .mem_planes = 1, .comp_planes = 3, .bpp = { 1, 1, 1, 0 }, .hdiv = 4, .vdiv = 4 },
> +		{ .format = V4L2_PIX_FMT_YVU410,  .mem_planes = 1, .comp_planes = 3, .bpp = { 1, 1, 1, 0 }, .hdiv = 4, .vdiv = 4 },
> +		{ .format = V4L2_PIX_FMT_YUV411P, .mem_planes = 1, .comp_planes = 3, .bpp = { 1, 1, 1, 0 }, .hdiv = 4, .vdiv = 1 },
> +		{ .format = V4L2_PIX_FMT_YUV420,  .mem_planes = 1, .comp_planes = 3, .bpp = { 1, 1, 1, 0 }, .hdiv = 2, .vdiv = 2 },
> +		{ .format = V4L2_PIX_FMT_YVU420,  .mem_planes = 1, .comp_planes = 3, .bpp = { 1, 1, 1, 0 }, .hdiv = 2, .vdiv = 2 },
> +		{ .format = V4L2_PIX_FMT_YUV422P, .mem_planes = 1, .comp_planes = 3, .bpp = { 1, 1, 1, 0 }, .hdiv = 2, .vdiv = 1 },
> +
> +		/* YUV planar formats, non contiguous variant */
> +		{ .format = V4L2_PIX_FMT_YUV420M, .mem_planes = 3, .comp_planes = 3, .bpp = { 1, 1, 1, 0 }, .hdiv = 2, .vdiv = 2 },
> +		{ .format = V4L2_PIX_FMT_YVU420M, .mem_planes = 3, .comp_planes = 3, .bpp = { 1, 1, 1, 0 }, .hdiv = 2, .vdiv = 2 },
> +		{ .format = V4L2_PIX_FMT_YUV422M, .mem_planes = 3, .comp_planes = 3, .bpp = { 1, 1, 1, 0 }, .hdiv = 2, .vdiv = 1 },
> +		{ .format = V4L2_PIX_FMT_YVU422M, .mem_planes = 3, .comp_planes = 3, .bpp = { 1, 1, 1, 0 }, .hdiv = 2, .vdiv = 1 },
> +		{ .format = V4L2_PIX_FMT_YUV444M, .mem_planes = 3, .comp_planes = 3, .bpp = { 1, 1, 1, 0 }, .hdiv = 1, .vdiv = 1 },
> +		{ .format = V4L2_PIX_FMT_YVU444M, .mem_planes = 3, .comp_planes = 3, .bpp = { 1, 1, 1, 0 }, .hdiv = 1, .vdiv = 1 },
> +
> +		{ .format = V4L2_PIX_FMT_NV12M,   .mem_planes = 2, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .hdiv = 2, .vdiv = 2 },
> +		{ .format = V4L2_PIX_FMT_NV21M,   .mem_planes = 2, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .hdiv = 2, .vdiv = 2 },
> +		{ .format = V4L2_PIX_FMT_NV16M,   .mem_planes = 2, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .hdiv = 2, .vdiv = 1 },
> +		{ .format = V4L2_PIX_FMT_NV61M,   .mem_planes = 2, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .hdiv = 2, .vdiv = 1 },
> +	};

Can I ask why you define the array inside this function and not as a static
array with file scope? Is this something trivial I am missing? It
will be placed in the same data segment anyhow, is this to reduce the
probability of an unlikely name clash within the file? Just curious..

> +	unsigned int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(formats); ++i)
> +		if (formats[i].format == format)
> +			return &formats[i];

Nit: empty line maybe

> +	return NULL;
> +}
> +EXPORT_SYMBOL(v4l2_format_info);
> +
> +static inline unsigned int v4l2_format_block_width(const struct v4l2_format_info *info, int plane)
> +{
> +	if (!info->block_w[plane])
> +		return 1;

Nit: empty line, or just
        return info->block_w[plane] ? info->block_w[plane] : 1;

> +	return info->block_w[plane];


> +}
> +
> +static inline unsigned int v4l2_format_block_height(const struct v4l2_format_info *info, int plane)

You don't like 80 columns that much, right :p ?

> +{
> +	if (!info->block_h[plane])
> +		return 1;

Same here

> +	return info->block_h[plane];
> +}
> +
> +int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt,
> +			 int pixelformat, int width, int height)

Bad alignment to open parenthesis

> +{
> +	const struct v4l2_format_info *info;
> +	struct v4l2_plane_pix_format *plane;
> +	int i;

unsigned int i; ?

> +
> +	info = v4l2_format_info(pixelformat);
> +	if (!info)
> +		return -EINVAL;
> +
> +	pixfmt->width = width;
> +	pixfmt->height = height;
> +	pixfmt->pixelformat = pixelformat;
> +	pixfmt->num_planes = info->mem_planes;
> +
> +	if (info->mem_planes == 1) {
> +		plane = &pixfmt->plane_fmt[0];
> +		plane->bytesperline = ALIGN(width, v4l2_format_block_width(info, 0)) * info->bpp[0];

80 cols :'(

> +		plane->sizeimage = 0;
> +
> +		for (i = 0; i < info->comp_planes; i++) {
> +			unsigned int hdiv = (i == 0) ? 1 : info->hdiv;
> +			unsigned int vdiv = (i == 0) ? 1 : info->vdiv;
> +			unsigned int aligned_width;
> +			unsigned int aligned_height;
> +
> +			aligned_width = ALIGN(width, v4l2_format_block_width(info, i));
> +			aligned_height = ALIGN(height, v4l2_format_block_height(info, i));
> +
> +			plane->sizeimage += info->bpp[i] *
> +				DIV_ROUND_UP(aligned_width, hdiv) *
> +				DIV_ROUND_UP(aligned_height, vdiv);
> +		}

I was initially puzzled by some of your definitions, say NV12, where
you mark the hdiv factor for the second plane to 2, while it is
actually only vertically subsampled. But now I see what you've done for NV12
and this works for all the other formats I have verified (NV12, NV16, YUYV
packed, and YVV4xx). Nice :)

Nit: you can save the below else branch indentation if you return
here.

> +	} else {
> +		for (i = 0; i < info->comp_planes; i++) {
> +			unsigned int hdiv = (i == 0) ? 1 : info->hdiv;
> +			unsigned int vdiv = (i == 0) ? 1 : info->vdiv;
> +			unsigned int aligned_width;
> +			unsigned int aligned_height;
> +
> +			aligned_width = ALIGN(width, v4l2_format_block_width(info, i));
> +			aligned_height = ALIGN(height, v4l2_format_block_height(info, i));
> +
> +			plane = &pixfmt->plane_fmt[i];
> +			plane->bytesperline =
> +				info->bpp[i] * DIV_ROUND_UP(aligned_width, hdiv);
> +			plane->sizeimage =
> +				plane->bytesperline * DIV_ROUND_UP(aligned_height, vdiv);
> +		}
> +	}

Empty line maybe?

> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_fill_pixfmt_mp);
> +
> +int 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 -EINVAL;
> +
> +	/* Single planar API cannot be used for multi plane formats. */
> +	if (info->mem_planes > 1)
> +		return -EINVAL;
> +
> +	pixfmt->width = width;
> +	pixfmt->height = height;
> +	pixfmt->pixelformat = pixelformat;
> +	pixfmt->bytesperline = ALIGN(width, v4l2_format_block_width(info, 0)) * info->bpp[0];
> +	pixfmt->sizeimage = 0;
> +
> +	for (i = 0; i < info->comp_planes; i++) {
> +		unsigned int hdiv = (i == 0) ? 1 : info->hdiv;
> +		unsigned int vdiv = (i == 0) ? 1 : info->vdiv;
> +		unsigned int aligned_width;
> +		unsigned int aligned_height;
> +
> +		aligned_width = ALIGN(width, v4l2_format_block_width(info, i));
> +		aligned_height = ALIGN(height, v4l2_format_block_height(info, i));
> +
> +		pixfmt->sizeimage += info->bpp[i] *
> +			DIV_ROUND_UP(aligned_width, hdiv) *
> +			DIV_ROUND_UP(aligned_height, vdiv);
> +	}
> +	return 0;
> +}

Same comments as per the multi planar version.

> +EXPORT_SYMBOL_GPL(v4l2_fill_pixfmt);
> diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
> index 2b93cb281fa5..0a41bbecf3d3 100644
> --- a/include/media/v4l2-common.h
> +++ b/include/media/v4l2-common.h
> @@ -392,4 +392,37 @@ int v4l2_s_parm_cap(struct video_device *vdev,
>  	((u64)(a).numerator * (b).denominator OP	\
>  	(u64)(b).numerator * (a).denominator)
>
> +/* ------------------------------------------------------------------------- */
> +
> +/* Pixel format and FourCC helpers */
> +
> +/**
> + * struct v4l2_format_info - information about a V4L2 format
> + * @format: 4CC format identifier (V4L2_PIX_FMT_*)
> + * @mem_planes: Number of memory planes, which includes the alpha plane (1 to 4).
> + * @comp_planes: Number of component planes, which includes the alpha plane (1 to 4).

Is alpha channel present in other formats that are not packed RGB?
Because if that's not the case, they are packed, and all the ones you
have defined here have 1 memory and 1 component plane only.
What formats would you expect to be multi planar and have an alpha channel?

> + * @bpp: Array of per-plane bytes per pixel
> + * @hdiv: Horizontal chroma subsampling factor
> + * @vdiv: Vertical chroma subsampling factor
> + * @block_w: Per-plane macroblock pixel width (optional)
> + * @block_h: Per-plane macroblock pixel height (optional)
> + */
> +struct v4l2_format_info {
> +	u32 format;
> +	u8 mem_planes;
> +	u8 comp_planes;
> +	u8 bpp[4];
> +	u8 hdiv;
> +	u8 vdiv;
> +	u8 block_w[4];
> +	u8 block_h[4];

I might have missed how a user of these fill functions is supposed to set
block_w and block_h. If these are alignment requirements that might be
platform dependent, shouldn't they come as parameters to v4l2_fill_pixfmt()
and v4l2_fill_pixfmt_mp()? Or assume drivers already provide aligned sizes
maybe...

Thank you, sorry for the many questions.
   j

> +};
> +
> +const struct v4l2_format_info *v4l2_format_info(u32 format);
> +
> +int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, int pixelformat,
> +		     int width, int height);
> +int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt, int pixelformat,
> +			int width, int height);
> +
>  #endif /* V4L2_COMMON_H_ */
> --
> 2.20.1
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/2] media: Introduce pixel format helpers

[Alexandre Courbot]

On Fri, Mar 29, 2019 at 3:08 AM Ezequiel Garcia <ezequiel@collabora.com> wrote:
>
> Following the "Add MPEG-2 decoding to Rockchip VPU"[1], and given we want to start
> using the pixel format helpers rather soon, I've decided to submit them separatedly.
>
> Aside from documenting the block_w and block_h, as suggested by Emil Velikov, these
> two patches are unmodified compared to v2 [1].
>
> This series contains the minimum patches to introduce the pixel format
> helpers, it currently will have no users, but we expect users to start
> popping-up soon.
> very shortly [2].

Is there anything that prevents you from moving the Rockchip drivers
to using these right after patch 2/2? IIRC that's what your MPEG-2
series did initially, and that would introduce a user immediately.

Re: [PATCH v3 2/2] media: Introduce helpers to fill pixel format structs

[Ezequiel Garcia]

On Thu, 2019-03-28 at 21:05 +0100, Jacopo Mondi wrote:
> Hi Ezequiel,
>    this is very nice, thank you!
> 

Hi Jacopo,

> On Thu, Mar 28, 2019 at 03:07:04PM -0300, 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 padding 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/v4l2-common.c | 164 ++++++++++++++++++++++++++
> >  include/media/v4l2-common.h           |  33 ++++++
> >  2 files changed, 197 insertions(+)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> > index 663730f088cd..d869a2910435 100644
> > --- a/drivers/media/v4l2-core/v4l2-common.c
> > +++ b/drivers/media/v4l2-core/v4l2-common.c
> > @@ -445,3 +445,167 @@ int v4l2_s_parm_cap(struct video_device *vdev,
> >  	return ret;
> >  }
> >  EXPORT_SYMBOL_GPL(v4l2_s_parm_cap);
> > +
> > +const struct v4l2_format_info *v4l2_format_info(u32 format)
> > +{
> > +	static const struct v4l2_format_info formats[] = {
> > +		/* RGB formats */
> > +		{ .format = V4L2_PIX_FMT_BGR24,   .mem_planes = 1, .comp_planes = 1, .bpp = { 3, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 },
> 
> How would you feel using macros here, this would help sticking to
> 80-or-so columns, as otherwise this table is pretty wide.
> 
> I've sketched out something quick:
> 
> #define V4L2_FMT_INFO_BPP(p1_, p2_, p3_, p4_) { p1_, p2_, p3_, p4_ }
> #define V4L2_FMT_INFO(pf_, mpl_, cpl_, bpp_, hd_, vd_)			\
> 		{ 							\
> 		  .format       = V4L2_PIX_FMT_##pf_, 			\
> 		  .mem_planes   = mpl_,					\
> 		  .comp_planes  = cpl_,					\
> 		  .bpp          = bpp_, 				\
> 		  .hdiv         = hd_,					\
> 		  .vdiv         = vd_ 					\
> 		}
> 
> const struct v4l2_format_info *v4l2_format_info(u32 format)
> {
> 	static const struct v4l2_format_info formats[] = {
> 		/* RGB formats */
>        		V4L2_FMT_INFO(BGR24, 1, 1, V4L2_FMT_INFO_BPP(3, 0, 0, 0), 1, 1),
>                 ...
> }
> 
> Not sure you might like it or not.
> 

I'm not entirely sure this improves readability. Hans has
just sent a pull-request for this. But see below, my replies
to some of your questions.

> > +		{ .format = V4L2_PIX_FMT_RGB24,   .mem_planes = 1, .comp_planes = 1, .bpp = { 3, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 },
> > +		{ .format = V4L2_PIX_FMT_HSV24,   .mem_planes = 1, .comp_planes = 1, .bpp = { 3, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 },
> > +		{ .format = V4L2_PIX_FMT_BGR32,   .mem_planes = 1, .comp_planes = 1, .bpp = { 4, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 },
> > +		{ .format = V4L2_PIX_FMT_XBGR32,  .mem_planes = 1, .comp_planes = 1, .bpp = { 4, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 },
> > +		{ .format = V4L2_PIX_FMT_RGB32,   .mem_planes = 1, .comp_planes = 1, .bpp = { 4, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 },
> > +		{ .format = V4L2_PIX_FMT_XRGB32,  .mem_planes = 1, .comp_planes = 1, .bpp = { 4, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 },
> > +		{ .format = V4L2_PIX_FMT_HSV32,   .mem_planes = 1, .comp_planes = 1, .bpp = { 4, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 },
> > +		{ .format = V4L2_PIX_FMT_ARGB32,  .mem_planes = 1, .comp_planes = 1, .bpp = { 4, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 },
> > +		{ .format = V4L2_PIX_FMT_ABGR32,  .mem_planes = 1, .comp_planes = 1, .bpp = { 4, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 },
> > +		{ .format = V4L2_PIX_FMT_GREY,    .mem_planes = 1, .comp_planes = 1, .bpp = { 1, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 },
> > +
> > +		/* YUV packed formats */
> > +		{ .format = V4L2_PIX_FMT_YUYV,    .mem_planes = 1, .comp_planes = 1, .bpp = { 2, 0, 0, 0 }, .hdiv = 2, .vdiv = 1 },
> > +		{ .format = V4L2_PIX_FMT_YVYU,    .mem_planes = 1, .comp_planes = 1, .bpp = { 2, 0, 0, 0 }, .hdiv = 2, .vdiv = 1 },
> > +		{ .format = V4L2_PIX_FMT_UYVY,    .mem_planes = 1, .comp_planes = 1, .bpp = { 2, 0, 0, 0 }, .hdiv = 2, .vdiv = 1 },
> > +		{ .format = V4L2_PIX_FMT_VYUY,    .mem_planes = 1, .comp_planes = 1, .bpp = { 2, 0, 0, 0 }, .hdiv = 2, .vdiv = 1 },
> > +
> > +		/* YUV planar formats */
> > +		{ .format = V4L2_PIX_FMT_NV12,    .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .hdiv = 2, .vdiv = 2 },
> > +		{ .format = V4L2_PIX_FMT_NV21,    .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .hdiv = 2, .vdiv = 2 },
> > +		{ .format = V4L2_PIX_FMT_NV16,    .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .hdiv = 2, .vdiv = 1 },
> > +		{ .format = V4L2_PIX_FMT_NV61,    .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .hdiv = 2, .vdiv = 1 },
> > +		{ .format = V4L2_PIX_FMT_NV24,    .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .hdiv = 1, .vdiv = 1 },
> > +		{ .format = V4L2_PIX_FMT_NV42,    .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .hdiv = 1, .vdiv = 1 },
> > +
> > +		{ .format = V4L2_PIX_FMT_YUV410,  .mem_planes = 1, .comp_planes = 3, .bpp = { 1, 1, 1, 0 }, .hdiv = 4, .vdiv = 4 },
> > +		{ .format = V4L2_PIX_FMT_YVU410,  .mem_planes = 1, .comp_planes = 3, .bpp = { 1, 1, 1, 0 }, .hdiv = 4, .vdiv = 4 },
> > +		{ .format = V4L2_PIX_FMT_YUV411P, .mem_planes = 1, .comp_planes = 3, .bpp = { 1, 1, 1, 0 }, .hdiv = 4, .vdiv = 1 },
> > +		{ .format = V4L2_PIX_FMT_YUV420,  .mem_planes = 1, .comp_planes = 3, .bpp = { 1, 1, 1, 0 }, .hdiv = 2, .vdiv = 2 },
> > +		{ .format = V4L2_PIX_FMT_YVU420,  .mem_planes = 1, .comp_planes = 3, .bpp = { 1, 1, 1, 0 }, .hdiv = 2, .vdiv = 2 },
> > +		{ .format = V4L2_PIX_FMT_YUV422P, .mem_planes = 1, .comp_planes = 3, .bpp = { 1, 1, 1, 0 }, .hdiv = 2, .vdiv = 1 },
> > +
> > +		/* YUV planar formats, non contiguous variant */
> > +		{ .format = V4L2_PIX_FMT_YUV420M, .mem_planes = 3, .comp_planes = 3, .bpp = { 1, 1, 1, 0 }, .hdiv = 2, .vdiv = 2 },
> > +		{ .format = V4L2_PIX_FMT_YVU420M, .mem_planes = 3, .comp_planes = 3, .bpp = { 1, 1, 1, 0 }, .hdiv = 2, .vdiv = 2 },
> > +		{ .format = V4L2_PIX_FMT_YUV422M, .mem_planes = 3, .comp_planes = 3, .bpp = { 1, 1, 1, 0 }, .hdiv = 2, .vdiv = 1 },
> > +		{ .format = V4L2_PIX_FMT_YVU422M, .mem_planes = 3, .comp_planes = 3, .bpp = { 1, 1, 1, 0 }, .hdiv = 2, .vdiv = 1 },
> > +		{ .format = V4L2_PIX_FMT_YUV444M, .mem_planes = 3, .comp_planes = 3, .bpp = { 1, 1, 1, 0 }, .hdiv = 1, .vdiv = 1 },
> > +		{ .format = V4L2_PIX_FMT_YVU444M, .mem_planes = 3, .comp_planes = 3, .bpp = { 1, 1, 1, 0 }, .hdiv = 1, .vdiv = 1 },
> > +
> > +		{ .format = V4L2_PIX_FMT_NV12M,   .mem_planes = 2, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .hdiv = 2, .vdiv = 2 },
> > +		{ .format = V4L2_PIX_FMT_NV21M,   .mem_planes = 2, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .hdiv = 2, .vdiv = 2 },
> > +		{ .format = V4L2_PIX_FMT_NV16M,   .mem_planes = 2, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .hdiv = 2, .vdiv = 1 },
> > +		{ .format = V4L2_PIX_FMT_NV61M,   .mem_planes = 2, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .hdiv = 2, .vdiv = 1 },
> > +	};
> 
> Can I ask why you define the array inside this function and not as a static
> array with file scope? Is this something trivial I am missing? It
> will be placed in the same data segment anyhow, is this to reduce the
> probability of an unlikely name clash within the file? Just curious..
> 

I followed drm-fourcc.c style. I like this pattern because it keeps the
data and the function together, and because it forces all users to get
the pixel format via v4l2_format_info().

> > +	unsigned int i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(formats); ++i)
> > +		if (formats[i].format == format)
> > +			return &formats[i];
> 
> Nit: empty line maybe
> 
> > +	return NULL;
> > +}
> > +EXPORT_SYMBOL(v4l2_format_info);
> > +
> > +static inline unsigned int v4l2_format_block_width(const struct v4l2_format_info *info, int plane)
> > +{
> > +	if (!info->block_w[plane])
> > +		return 1;
> 
> Nit: empty line, or just
>         return info->block_w[plane] ? info->block_w[plane] : 1;
> 
> > +	return info->block_w[plane];
> > +}
> > +
> > +static inline unsigned int v4l2_format_block_height(const struct v4l2_format_info *info, int plane)
> 
> You don't like 80 columns that much, right :p ?
> 

Guess I'm not a fan :-)

> > +{
> > +	if (!info->block_h[plane])
> > +		return 1;
> 
> Same here
> 
> > +	return info->block_h[plane];
> > +}
> > +
> > +int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt,
> > +			 int pixelformat, int width, int height)
> 
> Bad alignment to open parenthesis
> 
> > +{
> > +	const struct v4l2_format_info *info;
> > +	struct v4l2_plane_pix_format *plane;
> > +	int i;
> 
> unsigned int i; ?
> 
> > +
> > +	info = v4l2_format_info(pixelformat);
> > +	if (!info)
> > +		return -EINVAL;
> > +
> > +	pixfmt->width = width;
> > +	pixfmt->height = height;
> > +	pixfmt->pixelformat = pixelformat;
> > +	pixfmt->num_planes = info->mem_planes;
> > +
> > +	if (info->mem_planes == 1) {
> > +		plane = &pixfmt->plane_fmt[0];
> > +		plane->bytesperline = ALIGN(width, v4l2_format_block_width(info, 0)) * info->bpp[0];
> 
> 80 cols :'(
> 
> > +		plane->sizeimage = 0;
> > +
> > +		for (i = 0; i < info->comp_planes; i++) {
> > +			unsigned int hdiv = (i == 0) ? 1 : info->hdiv;
> > +			unsigned int vdiv = (i == 0) ? 1 : info->vdiv;
> > +			unsigned int aligned_width;
> > +			unsigned int aligned_height;
> > +
> > +			aligned_width = ALIGN(width, v4l2_format_block_width(info, i));
> > +			aligned_height = ALIGN(height, v4l2_format_block_height(info, i));
> > +
> > +			plane->sizeimage += info->bpp[i] *
> > +				DIV_ROUND_UP(aligned_width, hdiv) *
> > +				DIV_ROUND_UP(aligned_height, vdiv);
> > +		}
> 
> I was initially puzzled by some of your definitions, say NV12, where
> you mark the hdiv factor for the second plane to 2, while it is
> actually only vertically subsampled. But now I see what you've done for NV12
> and this works for all the other formats I have verified (NV12, NV16, YUYV
> packed, and YVV4xx). Nice :)
> 

NV12 is 2x2 subsampled, that is why the format is:

{ .format = V4L2_PIX_FMT_NV12,    .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .hdiv = 2, .vdiv = 2 },

It has two planes: one luma plane and one plane with the
two chroma components interleaved (hence bytes-per-pixel being 2).

The chroma components are subsampled in 2x2, and that is why hdiv and vdiv is 2.

> Nit: you can save the below else branch indentation if you return
> here.
> 
> > +	} else {
> > +		for (i = 0; i < info->comp_planes; i++) {
> > +			unsigned int hdiv = (i == 0) ? 1 : info->hdiv;
> > +			unsigned int vdiv = (i == 0) ? 1 : info->vdiv;
> > +			unsigned int aligned_width;
> > +			unsigned int aligned_height;
> > +
> > +			aligned_width = ALIGN(width, v4l2_format_block_width(info, i));
> > +			aligned_height = ALIGN(height, v4l2_format_block_height(info, i));
> > +
> > +			plane = &pixfmt->plane_fmt[i];
> > +			plane->bytesperline =
> > +				info->bpp[i] * DIV_ROUND_UP(aligned_width, hdiv);
> > +			plane->sizeimage =
> > +				plane->bytesperline * DIV_ROUND_UP(aligned_height, vdiv);
> > +		}
> > +	}
> 
> Empty line maybe?
> 
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(v4l2_fill_pixfmt_mp);
> > +
> > +int 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 -EINVAL;
> > +
> > +	/* Single planar API cannot be used for multi plane formats. */
> > +	if (info->mem_planes > 1)
> > +		return -EINVAL;
> > +
> > +	pixfmt->width = width;
> > +	pixfmt->height = height;
> > +	pixfmt->pixelformat = pixelformat;
> > +	pixfmt->bytesperline = ALIGN(width, v4l2_format_block_width(info, 0)) * info->bpp[0];
> > +	pixfmt->sizeimage = 0;
> > +
> > +	for (i = 0; i < info->comp_planes; i++) {
> > +		unsigned int hdiv = (i == 0) ? 1 : info->hdiv;
> > +		unsigned int vdiv = (i == 0) ? 1 : info->vdiv;
> > +		unsigned int aligned_width;
> > +		unsigned int aligned_height;
> > +
> > +		aligned_width = ALIGN(width, v4l2_format_block_width(info, i));
> > +		aligned_height = ALIGN(height, v4l2_format_block_height(info, i));
> > +
> > +		pixfmt->sizeimage += info->bpp[i] *
> > +			DIV_ROUND_UP(aligned_width, hdiv) *
> > +			DIV_ROUND_UP(aligned_height, vdiv);
> > +	}
> > +	return 0;
> > +}
> 
> Same comments as per the multi planar version.
> 
> > +EXPORT_SYMBOL_GPL(v4l2_fill_pixfmt);
> > diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
> > index 2b93cb281fa5..0a41bbecf3d3 100644
> > --- a/include/media/v4l2-common.h
> > +++ b/include/media/v4l2-common.h
> > @@ -392,4 +392,37 @@ int v4l2_s_parm_cap(struct video_device *vdev,
> >  	((u64)(a).numerator * (b).denominator OP	\
> >  	(u64)(b).numerator * (a).denominator)
> > 
> > +/* ------------------------------------------------------------------------- */
> > +
> > +/* Pixel format and FourCC helpers */
> > +
> > +/**
> > + * struct v4l2_format_info - information about a V4L2 format
> > + * @format: 4CC format identifier (V4L2_PIX_FMT_*)
> > + * @mem_planes: Number of memory planes, which includes the alpha plane (1 to 4).
> > + * @comp_planes: Number of component planes, which includes the alpha plane (1 to 4).
> 
> Is alpha channel present in other formats that are not packed RGB?
> Because if that's not the case, they are packed, and all the ones you
> have defined here have 1 memory and 1 component plane only.
> What formats would you expect to be multi planar and have an alpha channel?
> 

There's a format called AYUV https://www.fourcc.org/pixel-format/yuv-ayuv/.

It's not currently supported by the media subsystem, but our intention is to
try to get this API as generic as possible.

> > + * @bpp: Array of per-plane bytes per pixel
> > + * @hdiv: Horizontal chroma subsampling factor
> > + * @vdiv: Vertical chroma subsampling factor
> > + * @block_w: Per-plane macroblock pixel width (optional)
> > + * @block_h: Per-plane macroblock pixel height (optional)
> > + */
> > +struct v4l2_format_info {
> > +	u32 format;
> > +	u8 mem_planes;
> > +	u8 comp_planes;
> > +	u8 bpp[4];
> > +	u8 hdiv;
> > +	u8 vdiv;
> > +	u8 block_w[4];
> > +	u8 block_h[4];
> 
> I might have missed how a user of these fill functions is supposed to set
> block_w and block_h. If these are alignment requirements that might be
> platform dependent, shouldn't they come as parameters to v4l2_fill_pixfmt()
> and v4l2_fill_pixfmt_mp()? Or assume drivers already provide aligned sizes
> maybe...
> 

These will need a contributor to introduce a pixel format. They are meant
for tiled pixel formats. I left the introduction of them to the developers
of the respective drivers.


> Thank you, sorry for the many questions.
> 

No problem.

Thanks!
Eze

Re: [PATCH 0/2] media: Introduce pixel format helpers

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 943 bytes --]

Hi Ezequiel,

On Thu, Mar 28, 2019 at 03:07:02PM -0300, Ezequiel Garcia wrote:
> Following the "Add MPEG-2 decoding to Rockchip VPU"[1], and given we want to start
> using the pixel format helpers rather soon, I've decided to submit them separatedly.
>
> Aside from documenting the block_w and block_h, as suggested by Emil Velikov, these
> two patches are unmodified compared to v2 [1].
>
> This series contains the minimum patches to introduce the pixel format
> helpers, it currently will have no users, but we expect users to start
> popping-up soon.
> very shortly [2].
>
> To avoid confusing the patches here with those on v2 [1], I'm marking them as v3.

You might have missed it, but have you looked at the RFC "drm: Split
out the formats API and move it to a common place" I sent a couple of
weeks ago? It seems to cover the same use cases.

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3 2/2] media: Introduce helpers to fill pixel format structs

[Nicolas Dufresne]

Le lundi 01 avril 2019 à 13:23 -0300, Ezequiel Garcia a écrit :
> On Thu, 2019-03-28 at 21:05 +0100, Jacopo Mondi wrote:
> > Hi Ezequiel,
> >    this is very nice, thank you!
> > 
> 
> Hi Jacopo,
> 
> > On Thu, Mar 28, 2019 at 03:07:04PM -0300, 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 padding 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/v4l2-common.c | 164 ++++++++++++++++++++++++++
> > >  include/media/v4l2-common.h           |  33 ++++++
> > >  2 files changed, 197 insertions(+)
> > > 
> > > diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> > > index 663730f088cd..d869a2910435 100644
> > > --- a/drivers/media/v4l2-core/v4l2-common.c
> > > +++ b/drivers/media/v4l2-core/v4l2-common.c
> > > @@ -445,3 +445,167 @@ int v4l2_s_parm_cap(struct video_device *vdev,
> > >  	return ret;
> > >  }
> > >  EXPORT_SYMBOL_GPL(v4l2_s_parm_cap);
> > > +
> > > +const struct v4l2_format_info *v4l2_format_info(u32 format)
> > > +{
> > > +	static const struct v4l2_format_info formats[] = {
> > > +		/* RGB formats */
> > > +		{ .format = V4L2_PIX_FMT_BGR24,   .mem_planes = 1, .comp_planes = 1, .bpp = { 3, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 },
> > 
> > How would you feel using macros here, this would help sticking to
> > 80-or-so columns, as otherwise this table is pretty wide.
> > 
> > I've sketched out something quick:
> > 
> > #define V4L2_FMT_INFO_BPP(p1_, p2_, p3_, p4_) { p1_, p2_, p3_, p4_ }
> > #define V4L2_FMT_INFO(pf_, mpl_, cpl_, bpp_, hd_, vd_)			\
> > 		{ 							\
> > 		  .format       = V4L2_PIX_FMT_##pf_, 			\
> > 		  .mem_planes   = mpl_,					\
> > 		  .comp_planes  = cpl_,					\
> > 		  .bpp          = bpp_, 				\
> > 		  .hdiv         = hd_,					\
> > 		  .vdiv         = vd_ 					\
> > 		}
> > 
> > const struct v4l2_format_info *v4l2_format_info(u32 format)
> > {
> > 	static const struct v4l2_format_info formats[] = {
> > 		/* RGB formats */
> >        		V4L2_FMT_INFO(BGR24, 1, 1, V4L2_FMT_INFO_BPP(3, 0, 0, 0), 1, 1),
> >                 ...
> > }
> > 
> > Not sure you might like it or not.
> > 
> 
> I'm not entirely sure this improves readability. Hans has
> just sent a pull-request for this. But see below, my replies
> to some of your questions.
> 
> > > +		{ .format = V4L2_PIX_FMT_RGB24,   .mem_planes = 1, .comp_planes = 1, .bpp = { 3, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 },
> > > +		{ .format = V4L2_PIX_FMT_HSV24,   .mem_planes = 1, .comp_planes = 1, .bpp = { 3, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 },
> > > +		{ .format = V4L2_PIX_FMT_BGR32,   .mem_planes = 1, .comp_planes = 1, .bpp = { 4, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 },
> > > +		{ .format = V4L2_PIX_FMT_XBGR32,  .mem_planes = 1, .comp_planes = 1, .bpp = { 4, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 },
> > > +		{ .format = V4L2_PIX_FMT_RGB32,   .mem_planes = 1, .comp_planes = 1, .bpp = { 4, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 },
> > > +		{ .format = V4L2_PIX_FMT_XRGB32,  .mem_planes = 1, .comp_planes = 1, .bpp = { 4, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 },
> > > +		{ .format = V4L2_PIX_FMT_HSV32,   .mem_planes = 1, .comp_planes = 1, .bpp = { 4, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 },
> > > +		{ .format = V4L2_PIX_FMT_ARGB32,  .mem_planes = 1, .comp_planes = 1, .bpp = { 4, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 },
> > > +		{ .format = V4L2_PIX_FMT_ABGR32,  .mem_planes = 1, .comp_planes = 1, .bpp = { 4, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 },
> > > +		{ .format = V4L2_PIX_FMT_GREY,    .mem_planes = 1, .comp_planes = 1, .bpp = { 1, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 },
> > > +
> > > +		/* YUV packed formats */
> > > +		{ .format = V4L2_PIX_FMT_YUYV,    .mem_planes = 1, .comp_planes = 1, .bpp = { 2, 0, 0, 0 }, .hdiv = 2, .vdiv = 1 },
> > > +		{ .format = V4L2_PIX_FMT_YVYU,    .mem_planes = 1, .comp_planes = 1, .bpp = { 2, 0, 0, 0 }, .hdiv = 2, .vdiv = 1 },
> > > +		{ .format = V4L2_PIX_FMT_UYVY,    .mem_planes = 1, .comp_planes = 1, .bpp = { 2, 0, 0, 0 }, .hdiv = 2, .vdiv = 1 },
> > > +		{ .format = V4L2_PIX_FMT_VYUY,    .mem_planes = 1, .comp_planes = 1, .bpp = { 2, 0, 0, 0 }, .hdiv = 2, .vdiv = 1 },
> > > +
> > > +		/* YUV planar formats */
> > > +		{ .format = V4L2_PIX_FMT_NV12,    .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .hdiv = 2, .vdiv = 2 },
> > > +		{ .format = V4L2_PIX_FMT_NV21,    .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .hdiv = 2, .vdiv = 2 },
> > > +		{ .format = V4L2_PIX_FMT_NV16,    .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .hdiv = 2, .vdiv = 1 },
> > > +		{ .format = V4L2_PIX_FMT_NV61,    .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .hdiv = 2, .vdiv = 1 },
> > > +		{ .format = V4L2_PIX_FMT_NV24,    .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .hdiv = 1, .vdiv = 1 },
> > > +		{ .format = V4L2_PIX_FMT_NV42,    .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .hdiv = 1, .vdiv = 1 },
> > > +
> > > +		{ .format = V4L2_PIX_FMT_YUV410,  .mem_planes = 1, .comp_planes = 3, .bpp = { 1, 1, 1, 0 }, .hdiv = 4, .vdiv = 4 },
> > > +		{ .format = V4L2_PIX_FMT_YVU410,  .mem_planes = 1, .comp_planes = 3, .bpp = { 1, 1, 1, 0 }, .hdiv = 4, .vdiv = 4 },
> > > +		{ .format = V4L2_PIX_FMT_YUV411P, .mem_planes = 1, .comp_planes = 3, .bpp = { 1, 1, 1, 0 }, .hdiv = 4, .vdiv = 1 },
> > > +		{ .format = V4L2_PIX_FMT_YUV420,  .mem_planes = 1, .comp_planes = 3, .bpp = { 1, 1, 1, 0 }, .hdiv = 2, .vdiv = 2 },
> > > +		{ .format = V4L2_PIX_FMT_YVU420,  .mem_planes = 1, .comp_planes = 3, .bpp = { 1, 1, 1, 0 }, .hdiv = 2, .vdiv = 2 },
> > > +		{ .format = V4L2_PIX_FMT_YUV422P, .mem_planes = 1, .comp_planes = 3, .bpp = { 1, 1, 1, 0 }, .hdiv = 2, .vdiv = 1 },
> > > +
> > > +		/* YUV planar formats, non contiguous variant */
> > > +		{ .format = V4L2_PIX_FMT_YUV420M, .mem_planes = 3, .comp_planes = 3, .bpp = { 1, 1, 1, 0 }, .hdiv = 2, .vdiv = 2 },
> > > +		{ .format = V4L2_PIX_FMT_YVU420M, .mem_planes = 3, .comp_planes = 3, .bpp = { 1, 1, 1, 0 }, .hdiv = 2, .vdiv = 2 },
> > > +		{ .format = V4L2_PIX_FMT_YUV422M, .mem_planes = 3, .comp_planes = 3, .bpp = { 1, 1, 1, 0 }, .hdiv = 2, .vdiv = 1 },
> > > +		{ .format = V4L2_PIX_FMT_YVU422M, .mem_planes = 3, .comp_planes = 3, .bpp = { 1, 1, 1, 0 }, .hdiv = 2, .vdiv = 1 },
> > > +		{ .format = V4L2_PIX_FMT_YUV444M, .mem_planes = 3, .comp_planes = 3, .bpp = { 1, 1, 1, 0 }, .hdiv = 1, .vdiv = 1 },
> > > +		{ .format = V4L2_PIX_FMT_YVU444M, .mem_planes = 3, .comp_planes = 3, .bpp = { 1, 1, 1, 0 }, .hdiv = 1, .vdiv = 1 },
> > > +
> > > +		{ .format = V4L2_PIX_FMT_NV12M,   .mem_planes = 2, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .hdiv = 2, .vdiv = 2 },
> > > +		{ .format = V4L2_PIX_FMT_NV21M,   .mem_planes = 2, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .hdiv = 2, .vdiv = 2 },
> > > +		{ .format = V4L2_PIX_FMT_NV16M,   .mem_planes = 2, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .hdiv = 2, .vdiv = 1 },
> > > +		{ .format = V4L2_PIX_FMT_NV61M,   .mem_planes = 2, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .hdiv = 2, .vdiv = 1 },
> > > +	};
> > 
> > Can I ask why you define the array inside this function and not as a static
> > array with file scope? Is this something trivial I am missing? It
> > will be placed in the same data segment anyhow, is this to reduce the
> > probability of an unlikely name clash within the file? Just curious..
> > 
> 
> I followed drm-fourcc.c style. I like this pattern because it keeps the
> data and the function together, and because it forces all users to get
> the pixel format via v4l2_format_info().
> 
> > > +	unsigned int i;
> > > +
> > > +	for (i = 0; i < ARRAY_SIZE(formats); ++i)
> > > +		if (formats[i].format == format)
> > > +			return &formats[i];
> > 
> > Nit: empty line maybe
> > 
> > > +	return NULL;
> > > +}
> > > +EXPORT_SYMBOL(v4l2_format_info);
> > > +
> > > +static inline unsigned int v4l2_format_block_width(const struct v4l2_format_info *info, int plane)
> > > +{
> > > +	if (!info->block_w[plane])
> > > +		return 1;
> > 
> > Nit: empty line, or just
> >         return info->block_w[plane] ? info->block_w[plane] : 1;
> > 
> > > +	return info->block_w[plane];
> > > +}
> > > +
> > > +static inline unsigned int v4l2_format_block_height(const struct v4l2_format_info *info, int plane)
> > 
> > You don't like 80 columns that much, right :p ?
> > 
> 
> Guess I'm not a fan :-)
> 
> > > +{
> > > +	if (!info->block_h[plane])
> > > +		return 1;
> > 
> > Same here
> > 
> > > +	return info->block_h[plane];
> > > +}
> > > +
> > > +int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt,
> > > +			 int pixelformat, int width, int height)
> > 
> > Bad alignment to open parenthesis
> > 
> > > +{
> > > +	const struct v4l2_format_info *info;
> > > +	struct v4l2_plane_pix_format *plane;
> > > +	int i;
> > 
> > unsigned int i; ?
> > 
> > > +
> > > +	info = v4l2_format_info(pixelformat);
> > > +	if (!info)
> > > +		return -EINVAL;
> > > +
> > > +	pixfmt->width = width;
> > > +	pixfmt->height = height;
> > > +	pixfmt->pixelformat = pixelformat;
> > > +	pixfmt->num_planes = info->mem_planes;
> > > +
> > > +	if (info->mem_planes == 1) {
> > > +		plane = &pixfmt->plane_fmt[0];
> > > +		plane->bytesperline = ALIGN(width, v4l2_format_block_width(info, 0)) * info->bpp[0];
> > 
> > 80 cols :'(
> > 
> > > +		plane->sizeimage = 0;
> > > +
> > > +		for (i = 0; i < info->comp_planes; i++) {
> > > +			unsigned int hdiv = (i == 0) ? 1 : info->hdiv;
> > > +			unsigned int vdiv = (i == 0) ? 1 : info->vdiv;
> > > +			unsigned int aligned_width;
> > > +			unsigned int aligned_height;
> > > +
> > > +			aligned_width = ALIGN(width, v4l2_format_block_width(info, i));
> > > +			aligned_height = ALIGN(height, v4l2_format_block_height(info, i));
> > > +
> > > +			plane->sizeimage += info->bpp[i] *
> > > +				DIV_ROUND_UP(aligned_width, hdiv) *
> > > +				DIV_ROUND_UP(aligned_height, vdiv);
> > > +		}
> > 
> > I was initially puzzled by some of your definitions, say NV12, where
> > you mark the hdiv factor for the second plane to 2, while it is
> > actually only vertically subsampled. But now I see what you've done for NV12
> > and this works for all the other formats I have verified (NV12, NV16, YUYV
> > packed, and YVV4xx). Nice :)
> > 
> 
> NV12 is 2x2 subsampled, that is why the format is:
> 
> { .format = V4L2_PIX_FMT_NV12,    .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .hdiv = 2, .vdiv = 2 },
> 
> It has two planes: one luma plane and one plane with the
> two chroma components interleaved (hence bytes-per-pixel being 2).
> 
> The chroma components are subsampled in 2x2, and that is why hdiv and vdiv is 2.
> 
> > Nit: you can save the below else branch indentation if you return
> > here.
> > 
> > > +	} else {
> > > +		for (i = 0; i < info->comp_planes; i++) {
> > > +			unsigned int hdiv = (i == 0) ? 1 : info->hdiv;
> > > +			unsigned int vdiv = (i == 0) ? 1 : info->vdiv;
> > > +			unsigned int aligned_width;
> > > +			unsigned int aligned_height;
> > > +
> > > +			aligned_width = ALIGN(width, v4l2_format_block_width(info, i));
> > > +			aligned_height = ALIGN(height, v4l2_format_block_height(info, i));
> > > +
> > > +			plane = &pixfmt->plane_fmt[i];
> > > +			plane->bytesperline =
> > > +				info->bpp[i] * DIV_ROUND_UP(aligned_width, hdiv);
> > > +			plane->sizeimage =
> > > +				plane->bytesperline * DIV_ROUND_UP(aligned_height, vdiv);
> > > +		}
> > > +	}
> > 
> > Empty line maybe?
> > 
> > > +	return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(v4l2_fill_pixfmt_mp);
> > > +
> > > +int 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 -EINVAL;
> > > +
> > > +	/* Single planar API cannot be used for multi plane formats. */
> > > +	if (info->mem_planes > 1)
> > > +		return -EINVAL;
> > > +
> > > +	pixfmt->width = width;
> > > +	pixfmt->height = height;
> > > +	pixfmt->pixelformat = pixelformat;
> > > +	pixfmt->bytesperline = ALIGN(width, v4l2_format_block_width(info, 0)) * info->bpp[0];
> > > +	pixfmt->sizeimage = 0;
> > > +
> > > +	for (i = 0; i < info->comp_planes; i++) {
> > > +		unsigned int hdiv = (i == 0) ? 1 : info->hdiv;
> > > +		unsigned int vdiv = (i == 0) ? 1 : info->vdiv;
> > > +		unsigned int aligned_width;
> > > +		unsigned int aligned_height;
> > > +
> > > +		aligned_width = ALIGN(width, v4l2_format_block_width(info, i));
> > > +		aligned_height = ALIGN(height, v4l2_format_block_height(info, i));
> > > +
> > > +		pixfmt->sizeimage += info->bpp[i] *
> > > +			DIV_ROUND_UP(aligned_width, hdiv) *
> > > +			DIV_ROUND_UP(aligned_height, vdiv);
> > > +	}
> > > +	return 0;
> > > +}
> > 
> > Same comments as per the multi planar version.
> > 
> > > +EXPORT_SYMBOL_GPL(v4l2_fill_pixfmt);
> > > diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
> > > index 2b93cb281fa5..0a41bbecf3d3 100644
> > > --- a/include/media/v4l2-common.h
> > > +++ b/include/media/v4l2-common.h
> > > @@ -392,4 +392,37 @@ int v4l2_s_parm_cap(struct video_device *vdev,
> > >  	((u64)(a).numerator * (b).denominator OP	\
> > >  	(u64)(b).numerator * (a).denominator)
> > > 
> > > +/* ------------------------------------------------------------------------- */
> > > +
> > > +/* Pixel format and FourCC helpers */
> > > +
> > > +/**
> > > + * struct v4l2_format_info - information about a V4L2 format
> > > + * @format: 4CC format identifier (V4L2_PIX_FMT_*)
> > > + * @mem_planes: Number of memory planes, which includes the alpha plane (1 to 4).
> > > + * @comp_planes: Number of component planes, which includes the alpha plane (1 to 4).
> > 
> > Is alpha channel present in other formats that are not packed RGB?
> > Because if that's not the case, they are packed, and all the ones you
> > have defined here have 1 memory and 1 component plane only.
> > What formats would you expect to be multi planar and have an alpha channel?
> > 
> 
> There's a format called AYUV https://www.fourcc.org/pixel-format/yuv-ayuv/.
> 
> It's not currently supported by the media subsystem, but our intention is to
> try to get this API as generic as possible.
> 
> > > + * @bpp: Array of per-plane bytes per pixel
> > > + * @hdiv: Horizontal chroma subsampling factor
> > > + * @vdiv: Vertical chroma subsampling factor
> > > + * @block_w: Per-plane macroblock pixel width (optional)
> > > + * @block_h: Per-plane macroblock pixel height (optional)
> > > + */
> > > +struct v4l2_format_info {
> > > +	u32 format;
> > > +	u8 mem_planes;
> > > +	u8 comp_planes;
> > > +	u8 bpp[4];
> > > +	u8 hdiv;
> > > +	u8 vdiv;
> > > +	u8 block_w[4];
> > > +	u8 block_h[4];
> > 
> > I might have missed how a user of these fill functions is supposed to set
> > block_w and block_h. If these are alignment requirements that might be
> > platform dependent, shouldn't they come as parameters to v4l2_fill_pixfmt()
> > and v4l2_fill_pixfmt_mp()? Or assume drivers already provide aligned sizes
> > maybe...
> > 
> 
> These will need a contributor to introduce a pixel format. They are meant
> for tiled pixel formats. I left the introduction of them to the developers
> of the respective drivers.

I suspect we already have a format, see:

https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/pixfmt-nv12mt.html
https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/pixfmt-nv12m.html
(the 16x16 variant)

They were respectively added for Exynos 4 and Exynos 5 support. So for
the second, I suspsect block_w/h is 16. But for NV12MT I'm not very
clear if it would be block_w of 64 (the actual tile width) or 128 which
is the required width alignment.

> 
> 
> > Thank you, sorry for the many questions.
> > 
> 
> No problem.
> 
> Thanks!
> Eze
>