[PATCH v2 00/15] media: vimc: Add support for multiplanar formats

[PATCH v2 00/15] media: vimc: Add support for multiplanar formats

[André Almeida]

Hello,

This series implements support for multiplane pixel formats at Vimc.
A lot of changes were required since vimc support for singleplane
was "hardcoded". The code has been adapted in order to support both
formats. When was possible, the functions were written generically,
avoiding functions for just one type of pixel format.

The debayer subdevice is the only one that currently doesn't supports
multiplanar formats. Documentation to each device will be made in a
future patch. In hardcoded topology, the exposed capture device
`RGB/YUV Capture` have a debayer in the pipeline, so it will fail when
tested with multiplanar formats.

The last commit of this series was tested using Hans' virtme.sh[1]
script here are the summary of results:

Grand Total for vivid device /dev/media0: 631, Succeeded: 631, Failed: 0, Warnings: 6
Grand Total for vivid device /dev/media1: 631, Succeeded: 631, Failed: 0, Warnings: 6
Grand Total for vim2m device /dev/media3: 61, Succeeded: 61, Failed: 0, Warnings: 0
Grand Total for vimc device /dev/media3: 478, Succeeded: 478, Failed: 0, Warnings: 0
Final Summary: 1801, Succeeded: 1801, Failed: 0, Warnings: 12

This patch depends on this one:

"[PATCH] media: vimc: propagate pixel format in the stream"

Thanks,
	André

[1] https://hverkuil.home.xs4all.nl/virtme/virtme.sh

Changes in v2:
- Fix typos
- Fix indentations
- Enhance v4l2_fmt_* documentation
- Change the order of commits, now the multiplanar parameter is the last
one with the commit to set the device capabilities
- Squash "unnecessary checks" commits together
- In v1, the whole media device was in singleplanar or in multiplanar
format. Now, each stream/pipeline can be in a format
- Check the capture capabilities to get if the stream is in
singleplanar/multiplanar mode, instead of checking the module parameter.
- Change `if (multiplanar)` to `if (IS_MULTIPLANAR(vcap))`
- Add a new commit to propagate in the stream if the capture device is
in multiplanar or singleplanar mode


André Almeida (15):
  media: Move sp2mp functions to v4l2-common
  media: vimc: Remove unnecessary stream checks
  media: vimc: cap: Change vimc_cap_device.format type
  media: vimc: cap: Dynamically define stream pixelformat
  media: vimc: cap: Add handler for singleplanar fmt ioctls
  media: vimc: cap: Add handler for multiplanar fmt ioctls
  media: vimc: cap: Add multiplanar formats
  media: vimc: cap: Add multiplanar default format
  media: vimc: cap: Allocate and verify mplanar buffers
  media: vimc: Propagate multiplanar state in the stream
  media: vimc: Add and use new struct vimc_frame
  media: vimc: sen: Add support for multiplanar formats
  media: vimc: sca: Add support for multiplanar formats
  media: vimc: cap: Add support for multiplanar formats
  media: vimc: Create multiplanar parameter

 drivers/media/platform/vimc/vimc-capture.c    | 351 +++++++++++++++---
 drivers/media/platform/vimc/vimc-common.c     |  36 ++
 drivers/media/platform/vimc/vimc-common.h     |  49 ++-
 drivers/media/platform/vimc/vimc-debayer.c    |  39 +-
 drivers/media/platform/vimc/vimc-scaler.c     | 128 ++++---
 drivers/media/platform/vimc/vimc-sensor.c     |  65 ++--
 drivers/media/platform/vimc/vimc-streamer.c   |   2 +-
 drivers/media/platform/vimc/vimc-streamer.h   |   3 +
 drivers/media/platform/vivid/vivid-vid-cap.c  |   6 +-
 .../media/platform/vivid/vivid-vid-common.c   |  59 ---
 .../media/platform/vivid/vivid-vid-common.h   |   9 -
 drivers/media/platform/vivid/vivid-vid-out.c  |   6 +-
 drivers/media/v4l2-core/v4l2-common.c         |  62 ++++
 include/media/v4l2-common.h                   |  37 ++
 14 files changed, 618 insertions(+), 234 deletions(-)

--
2.21.0

[PATCH v2 01/15] media: Move sp2mp functions to v4l2-common

[André Almeida]

Move sp2mp functions from vivid code to v4l2-common as it will be reused
by vimc driver for multiplanar support.

Signed-off-by: André Almeida <andrealmeid@collabora.com>
---
Changes in v2:
- Fix indentation
- Change the style of comments. Functions now have a shorter title and a
better description bellow

 drivers/media/platform/vivid/vivid-vid-cap.c  |  6 +-
 .../media/platform/vivid/vivid-vid-common.c   | 59 ------------------
 .../media/platform/vivid/vivid-vid-common.h   |  9 ---
 drivers/media/platform/vivid/vivid-vid-out.c  |  6 +-
 drivers/media/v4l2-core/v4l2-common.c         | 62 +++++++++++++++++++
 include/media/v4l2-common.h                   | 37 +++++++++++
 6 files changed, 105 insertions(+), 74 deletions(-)

diff --git a/drivers/media/platform/vivid/vivid-vid-cap.c b/drivers/media/platform/vivid/vivid-vid-cap.c
index 52eeda624d7e..b5ad71bbf7bf 100644
--- a/drivers/media/platform/vivid/vivid-vid-cap.c
+++ b/drivers/media/platform/vivid/vivid-vid-cap.c
@@ -815,7 +815,7 @@ int vidioc_g_fmt_vid_cap(struct file *file, void *priv,
 
 	if (dev->multiplanar)
 		return -ENOTTY;
-	return fmt_sp2mp_func(file, priv, f, vivid_g_fmt_vid_cap);
+	return v4l2_fmt_sp2mp_func(file, priv, f, vivid_g_fmt_vid_cap);
 }
 
 int vidioc_try_fmt_vid_cap(struct file *file, void *priv,
@@ -825,7 +825,7 @@ int vidioc_try_fmt_vid_cap(struct file *file, void *priv,
 
 	if (dev->multiplanar)
 		return -ENOTTY;
-	return fmt_sp2mp_func(file, priv, f, vivid_try_fmt_vid_cap);
+	return v4l2_fmt_sp2mp_func(file, priv, f, vivid_try_fmt_vid_cap);
 }
 
 int vidioc_s_fmt_vid_cap(struct file *file, void *priv,
@@ -835,7 +835,7 @@ int vidioc_s_fmt_vid_cap(struct file *file, void *priv,
 
 	if (dev->multiplanar)
 		return -ENOTTY;
-	return fmt_sp2mp_func(file, priv, f, vivid_s_fmt_vid_cap);
+	return v4l2_fmt_sp2mp_func(file, priv, f, vivid_s_fmt_vid_cap);
 }
 
 int vivid_vid_cap_g_selection(struct file *file, void *priv,
diff --git a/drivers/media/platform/vivid/vivid-vid-common.c b/drivers/media/platform/vivid/vivid-vid-common.c
index 74b83bcc6119..3dd3a05d2e67 100644
--- a/drivers/media/platform/vivid/vivid-vid-common.c
+++ b/drivers/media/platform/vivid/vivid-vid-common.c
@@ -674,65 +674,6 @@ void vivid_send_source_change(struct vivid_dev *dev, unsigned type)
 	}
 }
 
-/*
- * Conversion function that converts a single-planar format to a
- * single-plane multiplanar format.
- */
-void fmt_sp2mp(const struct v4l2_format *sp_fmt, struct v4l2_format *mp_fmt)
-{
-	struct v4l2_pix_format_mplane *mp = &mp_fmt->fmt.pix_mp;
-	struct v4l2_plane_pix_format *ppix = &mp->plane_fmt[0];
-	const struct v4l2_pix_format *pix = &sp_fmt->fmt.pix;
-	bool is_out = sp_fmt->type == V4L2_BUF_TYPE_VIDEO_OUTPUT;
-
-	memset(mp->reserved, 0, sizeof(mp->reserved));
-	mp_fmt->type = is_out ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE :
-			   V4L2_CAP_VIDEO_CAPTURE_MPLANE;
-	mp->width = pix->width;
-	mp->height = pix->height;
-	mp->pixelformat = pix->pixelformat;
-	mp->field = pix->field;
-	mp->colorspace = pix->colorspace;
-	mp->xfer_func = pix->xfer_func;
-	/* Also copies hsv_enc */
-	mp->ycbcr_enc = pix->ycbcr_enc;
-	mp->quantization = pix->quantization;
-	mp->num_planes = 1;
-	mp->flags = pix->flags;
-	ppix->sizeimage = pix->sizeimage;
-	ppix->bytesperline = pix->bytesperline;
-	memset(ppix->reserved, 0, sizeof(ppix->reserved));
-}
-
-int fmt_sp2mp_func(struct file *file, void *priv,
-		struct v4l2_format *f, fmtfunc func)
-{
-	struct v4l2_format fmt;
-	struct v4l2_pix_format_mplane *mp = &fmt.fmt.pix_mp;
-	struct v4l2_plane_pix_format *ppix = &mp->plane_fmt[0];
-	struct v4l2_pix_format *pix = &f->fmt.pix;
-	int ret;
-
-	/* Converts to a mplane format */
-	fmt_sp2mp(f, &fmt);
-	/* Passes it to the generic mplane format function */
-	ret = func(file, priv, &fmt);
-	/* Copies back the mplane data to the single plane format */
-	pix->width = mp->width;
-	pix->height = mp->height;
-	pix->pixelformat = mp->pixelformat;
-	pix->field = mp->field;
-	pix->colorspace = mp->colorspace;
-	pix->xfer_func = mp->xfer_func;
-	/* Also copies hsv_enc */
-	pix->ycbcr_enc = mp->ycbcr_enc;
-	pix->quantization = mp->quantization;
-	pix->sizeimage = ppix->sizeimage;
-	pix->bytesperline = ppix->bytesperline;
-	pix->flags = mp->flags;
-	return ret;
-}
-
 int vivid_vid_adjust_sel(unsigned flags, struct v4l2_rect *r)
 {
 	unsigned w = r->width;
diff --git a/drivers/media/platform/vivid/vivid-vid-common.h b/drivers/media/platform/vivid/vivid-vid-common.h
index 29b6c0b40a1b..13adea56baa0 100644
--- a/drivers/media/platform/vivid/vivid-vid-common.h
+++ b/drivers/media/platform/vivid/vivid-vid-common.h
@@ -8,15 +8,6 @@
 #ifndef _VIVID_VID_COMMON_H_
 #define _VIVID_VID_COMMON_H_
 
-typedef int (*fmtfunc)(struct file *file, void *priv, struct v4l2_format *f);
-
-/*
- * Conversion function that converts a single-planar format to a
- * single-plane multiplanar format.
- */
-void fmt_sp2mp(const struct v4l2_format *sp_fmt, struct v4l2_format *mp_fmt);
-int fmt_sp2mp_func(struct file *file, void *priv,
-		struct v4l2_format *f, fmtfunc func);
 
 extern const struct v4l2_dv_timings_cap vivid_dv_timings_cap;
 
diff --git a/drivers/media/platform/vivid/vivid-vid-out.c b/drivers/media/platform/vivid/vivid-vid-out.c
index 9350ca65dd91..e9d68ccbcc5d 100644
--- a/drivers/media/platform/vivid/vivid-vid-out.c
+++ b/drivers/media/platform/vivid/vivid-vid-out.c
@@ -612,7 +612,7 @@ int vidioc_g_fmt_vid_out(struct file *file, void *priv,
 
 	if (dev->multiplanar)
 		return -ENOTTY;
-	return fmt_sp2mp_func(file, priv, f, vivid_g_fmt_vid_out);
+	return v4l2_fmt_sp2mp_func(file, priv, f, vivid_g_fmt_vid_out);
 }
 
 int vidioc_try_fmt_vid_out(struct file *file, void *priv,
@@ -622,7 +622,7 @@ int vidioc_try_fmt_vid_out(struct file *file, void *priv,
 
 	if (dev->multiplanar)
 		return -ENOTTY;
-	return fmt_sp2mp_func(file, priv, f, vivid_try_fmt_vid_out);
+	return v4l2_fmt_sp2mp_func(file, priv, f, vivid_try_fmt_vid_out);
 }
 
 int vidioc_s_fmt_vid_out(struct file *file, void *priv,
@@ -632,7 +632,7 @@ int vidioc_s_fmt_vid_out(struct file *file, void *priv,
 
 	if (dev->multiplanar)
 		return -ENOTTY;
-	return fmt_sp2mp_func(file, priv, f, vivid_s_fmt_vid_out);
+	return v4l2_fmt_sp2mp_func(file, priv, f, vivid_s_fmt_vid_out);
 }
 
 int vivid_vid_out_g_selection(struct file *file, void *priv,
diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
index 779e44d6db43..da294b812424 100644
--- a/drivers/media/v4l2-core/v4l2-common.c
+++ b/drivers/media/v4l2-core/v4l2-common.c
@@ -653,3 +653,65 @@ int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, int pixelformat, int width,
 	return 0;
 }
 EXPORT_SYMBOL_GPL(v4l2_fill_pixfmt);
+
+/*
+ * Conversion functions that convert a single-planar format to a
+ * multi-planar format.
+ */
+void v4l2_fmt_sp2mp(const struct v4l2_format *sp_fmt,
+		    struct v4l2_format *mp_fmt)
+{
+	struct v4l2_pix_format_mplane *mp = &mp_fmt->fmt.pix_mp;
+	struct v4l2_plane_pix_format *ppix = &mp->plane_fmt[0];
+	const struct v4l2_pix_format *pix = &sp_fmt->fmt.pix;
+	bool is_out = sp_fmt->type == V4L2_BUF_TYPE_VIDEO_OUTPUT;
+
+	memset(mp->reserved, 0, sizeof(mp->reserved));
+	mp_fmt->type = is_out ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE :
+			   V4L2_CAP_VIDEO_CAPTURE_MPLANE;
+	mp->width = pix->width;
+	mp->height = pix->height;
+	mp->pixelformat = pix->pixelformat;
+	mp->field = pix->field;
+	mp->colorspace = pix->colorspace;
+	mp->xfer_func = pix->xfer_func;
+	/* Also copies hsv_enc */
+	mp->ycbcr_enc = pix->ycbcr_enc;
+	mp->quantization = pix->quantization;
+	mp->num_planes = 1;
+	mp->flags = pix->flags;
+	ppix->sizeimage = pix->sizeimage;
+	ppix->bytesperline = pix->bytesperline;
+	memset(ppix->reserved, 0, sizeof(ppix->reserved));
+}
+EXPORT_SYMBOL_GPL(v4l2_fmt_sp2mp);
+
+int v4l2_fmt_sp2mp_func(struct file *file, void *priv, struct v4l2_format *f,
+			v4l2_fmtfunc func)
+{
+	struct v4l2_format fmt;
+	struct v4l2_pix_format_mplane *mp = &fmt.fmt.pix_mp;
+	struct v4l2_plane_pix_format *ppix = &mp->plane_fmt[0];
+	struct v4l2_pix_format *pix = &f->fmt.pix;
+	int ret;
+
+	/* Converts to a mplane format */
+	v4l2_fmt_sp2mp(f, &fmt);
+	/* Passes it to the generic mplane format function */
+	ret = func(file, priv, &fmt);
+	/* Copies back the mplane data to the single plane format */
+	pix->width = mp->width;
+	pix->height = mp->height;
+	pix->pixelformat = mp->pixelformat;
+	pix->field = mp->field;
+	pix->colorspace = mp->colorspace;
+	pix->xfer_func = mp->xfer_func;
+	/* Also copies hsv_enc */
+	pix->ycbcr_enc = mp->ycbcr_enc;
+	pix->quantization = mp->quantization;
+	pix->sizeimage = ppix->sizeimage;
+	pix->bytesperline = ppix->bytesperline;
+	pix->flags = mp->flags;
+	return ret;
+}
+EXPORT_SYMBOL_GPL(v4l2_fmt_sp2mp_func);
diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
index 937b74a946cd..c5be976ce49e 100644
--- a/include/media/v4l2-common.h
+++ b/include/media/v4l2-common.h
@@ -424,4 +424,41 @@ int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, int pixelformat,
 int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt, int pixelformat,
 			int width, int height);
 
+/**
+ * v4l2_fmtfunc - function pointer for v4l2_fmt_sp2mp()
+ *
+ * @file: device's descriptor file
+ * @priv: private data pointer
+ * @f: format that holds a mp pixel format
+ *
+ * Type to be passed as argument of v4l2_fmt_sp2mp() to be used by
+ * v4l2_fmt_sp2mp_func to pass the generic multi-planar function as argument.
+ */
+typedef int (*v4l2_fmtfunc)(struct file *file, void *priv,
+			    struct v4l2_format *f);
+
+/**
+ * v4l2_fmt_sp2mp - transforms a single-planar format struct into a multi-planar
+ * struct
+ *
+ * @sp_fmt: pointer to the single-planar format struct (in)
+ * @mp_fmt: pointer to the multi-planar format struct (out)
+ */
+void v4l2_fmt_sp2mp(const struct v4l2_format *sp_fmt,
+		    struct v4l2_format *mp_fmt);
+
+/**
+ * v4l2_fmt_sp2mp_func - handler for single-planar format functions
+ * @file: device's descriptor file
+ * @priv: private data pointer
+ * @f: format that holds a sp pixel format
+ * @func: generic mp function
+ *
+ * Handler to call a generic multi-planar format function using single-planar
+ * format. It converts the sp to a mp, calls the function and converts mp back
+ * to sp.
+ */
+int v4l2_fmt_sp2mp_func(struct file *file, void *priv, struct v4l2_format *f,
+			v4l2_fmtfunc func);
+
 #endif /* V4L2_COMMON_H_ */
-- 
2.21.0

Re: [PATCH v2 01/15] media: Move sp2mp functions to v4l2-common

[Helen Koike]

Hi Andre,

Thanks for the patch, just some small comments.

On 3/27/19 12:17 PM, André Almeida wrote:
> Move sp2mp functions from vivid code to v4l2-common as it will be reused
> by vimc driver for multiplanar support.

Just a detail (not a big deal):
I would re-order this patch to be just before patch "media: vimc: cap:
Add handler for singleplanar fmt ioctls", as it is only required there,
so it is easier to see why this modification was needed.

> 
> Signed-off-by: André Almeida <andrealmeid@collabora.com>
> ---
> Changes in v2:
> - Fix indentation
> - Change the style of comments. Functions now have a shorter title and a
> better description bellow
> 
>  drivers/media/platform/vivid/vivid-vid-cap.c  |  6 +-
>  .../media/platform/vivid/vivid-vid-common.c   | 59 ------------------
>  .../media/platform/vivid/vivid-vid-common.h   |  9 ---
>  drivers/media/platform/vivid/vivid-vid-out.c  |  6 +-
>  drivers/media/v4l2-core/v4l2-common.c         | 62 +++++++++++++++++++
>  include/media/v4l2-common.h                   | 37 +++++++++++
>  6 files changed, 105 insertions(+), 74 deletions(-)
> 
> diff --git a/drivers/media/platform/vivid/vivid-vid-cap.c b/drivers/media/platform/vivid/vivid-vid-cap.c
> index 52eeda624d7e..b5ad71bbf7bf 100644
> --- a/drivers/media/platform/vivid/vivid-vid-cap.c
> +++ b/drivers/media/platform/vivid/vivid-vid-cap.c
> @@ -815,7 +815,7 @@ int vidioc_g_fmt_vid_cap(struct file *file, void *priv,
>  
>  	if (dev->multiplanar)
>  		return -ENOTTY;
> -	return fmt_sp2mp_func(file, priv, f, vivid_g_fmt_vid_cap);
> +	return v4l2_fmt_sp2mp_func(file, priv, f, vivid_g_fmt_vid_cap);
>  }
>  
>  int vidioc_try_fmt_vid_cap(struct file *file, void *priv,
> @@ -825,7 +825,7 @@ int vidioc_try_fmt_vid_cap(struct file *file, void *priv,
>  
>  	if (dev->multiplanar)
>  		return -ENOTTY;
> -	return fmt_sp2mp_func(file, priv, f, vivid_try_fmt_vid_cap);
> +	return v4l2_fmt_sp2mp_func(file, priv, f, vivid_try_fmt_vid_cap);
>  }
>  
>  int vidioc_s_fmt_vid_cap(struct file *file, void *priv,
> @@ -835,7 +835,7 @@ int vidioc_s_fmt_vid_cap(struct file *file, void *priv,
>  
>  	if (dev->multiplanar)
>  		return -ENOTTY;
> -	return fmt_sp2mp_func(file, priv, f, vivid_s_fmt_vid_cap);
> +	return v4l2_fmt_sp2mp_func(file, priv, f, vivid_s_fmt_vid_cap);
>  }
>  
>  int vivid_vid_cap_g_selection(struct file *file, void *priv,
> diff --git a/drivers/media/platform/vivid/vivid-vid-common.c b/drivers/media/platform/vivid/vivid-vid-common.c
> index 74b83bcc6119..3dd3a05d2e67 100644
> --- a/drivers/media/platform/vivid/vivid-vid-common.c
> +++ b/drivers/media/platform/vivid/vivid-vid-common.c
> @@ -674,65 +674,6 @@ void vivid_send_source_change(struct vivid_dev *dev, unsigned type)
>  	}
>  }
>  
> -/*
> - * Conversion function that converts a single-planar format to a
> - * single-plane multiplanar format.
> - */
> -void fmt_sp2mp(const struct v4l2_format *sp_fmt, struct v4l2_format *mp_fmt)
> -{
> -	struct v4l2_pix_format_mplane *mp = &mp_fmt->fmt.pix_mp;
> -	struct v4l2_plane_pix_format *ppix = &mp->plane_fmt[0];
> -	const struct v4l2_pix_format *pix = &sp_fmt->fmt.pix;
> -	bool is_out = sp_fmt->type == V4L2_BUF_TYPE_VIDEO_OUTPUT;
> -
> -	memset(mp->reserved, 0, sizeof(mp->reserved));
> -	mp_fmt->type = is_out ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE :
> -			   V4L2_CAP_VIDEO_CAPTURE_MPLANE;
> -	mp->width = pix->width;
> -	mp->height = pix->height;
> -	mp->pixelformat = pix->pixelformat;
> -	mp->field = pix->field;
> -	mp->colorspace = pix->colorspace;
> -	mp->xfer_func = pix->xfer_func;
> -	/* Also copies hsv_enc */
> -	mp->ycbcr_enc = pix->ycbcr_enc;
> -	mp->quantization = pix->quantization;
> -	mp->num_planes = 1;
> -	mp->flags = pix->flags;
> -	ppix->sizeimage = pix->sizeimage;
> -	ppix->bytesperline = pix->bytesperline;
> -	memset(ppix->reserved, 0, sizeof(ppix->reserved));
> -}
> -
> -int fmt_sp2mp_func(struct file *file, void *priv,
> -		struct v4l2_format *f, fmtfunc func)
> -{
> -	struct v4l2_format fmt;
> -	struct v4l2_pix_format_mplane *mp = &fmt.fmt.pix_mp;
> -	struct v4l2_plane_pix_format *ppix = &mp->plane_fmt[0];
> -	struct v4l2_pix_format *pix = &f->fmt.pix;
> -	int ret;
> -
> -	/* Converts to a mplane format */
> -	fmt_sp2mp(f, &fmt);
> -	/* Passes it to the generic mplane format function */
> -	ret = func(file, priv, &fmt);
> -	/* Copies back the mplane data to the single plane format */
> -	pix->width = mp->width;
> -	pix->height = mp->height;
> -	pix->pixelformat = mp->pixelformat;
> -	pix->field = mp->field;
> -	pix->colorspace = mp->colorspace;
> -	pix->xfer_func = mp->xfer_func;
> -	/* Also copies hsv_enc */
> -	pix->ycbcr_enc = mp->ycbcr_enc;
> -	pix->quantization = mp->quantization;
> -	pix->sizeimage = ppix->sizeimage;
> -	pix->bytesperline = ppix->bytesperline;
> -	pix->flags = mp->flags;
> -	return ret;
> -}
> -
>  int vivid_vid_adjust_sel(unsigned flags, struct v4l2_rect *r)
>  {
>  	unsigned w = r->width;
> diff --git a/drivers/media/platform/vivid/vivid-vid-common.h b/drivers/media/platform/vivid/vivid-vid-common.h
> index 29b6c0b40a1b..13adea56baa0 100644
> --- a/drivers/media/platform/vivid/vivid-vid-common.h
> +++ b/drivers/media/platform/vivid/vivid-vid-common.h
> @@ -8,15 +8,6 @@
>  #ifndef _VIVID_VID_COMMON_H_
>  #define _VIVID_VID_COMMON_H_
>  
> -typedef int (*fmtfunc)(struct file *file, void *priv, struct v4l2_format *f);
> -
> -/*
> - * Conversion function that converts a single-planar format to a
> - * single-plane multiplanar format.
> - */
> -void fmt_sp2mp(const struct v4l2_format *sp_fmt, struct v4l2_format *mp_fmt);
> -int fmt_sp2mp_func(struct file *file, void *priv,
> -		struct v4l2_format *f, fmtfunc func);
>  
>  extern const struct v4l2_dv_timings_cap vivid_dv_timings_cap;
>  
> diff --git a/drivers/media/platform/vivid/vivid-vid-out.c b/drivers/media/platform/vivid/vivid-vid-out.c
> index 9350ca65dd91..e9d68ccbcc5d 100644
> --- a/drivers/media/platform/vivid/vivid-vid-out.c
> +++ b/drivers/media/platform/vivid/vivid-vid-out.c
> @@ -612,7 +612,7 @@ int vidioc_g_fmt_vid_out(struct file *file, void *priv,
>  
>  	if (dev->multiplanar)
>  		return -ENOTTY;
> -	return fmt_sp2mp_func(file, priv, f, vivid_g_fmt_vid_out);
> +	return v4l2_fmt_sp2mp_func(file, priv, f, vivid_g_fmt_vid_out);
>  }
>  
>  int vidioc_try_fmt_vid_out(struct file *file, void *priv,
> @@ -622,7 +622,7 @@ int vidioc_try_fmt_vid_out(struct file *file, void *priv,
>  
>  	if (dev->multiplanar)
>  		return -ENOTTY;
> -	return fmt_sp2mp_func(file, priv, f, vivid_try_fmt_vid_out);
> +	return v4l2_fmt_sp2mp_func(file, priv, f, vivid_try_fmt_vid_out);
>  }
>  
>  int vidioc_s_fmt_vid_out(struct file *file, void *priv,
> @@ -632,7 +632,7 @@ int vidioc_s_fmt_vid_out(struct file *file, void *priv,
>  
>  	if (dev->multiplanar)
>  		return -ENOTTY;
> -	return fmt_sp2mp_func(file, priv, f, vivid_s_fmt_vid_out);
> +	return v4l2_fmt_sp2mp_func(file, priv, f, vivid_s_fmt_vid_out);
>  }
>  
>  int vivid_vid_out_g_selection(struct file *file, void *priv,
> diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> index 779e44d6db43..da294b812424 100644
> --- a/drivers/media/v4l2-core/v4l2-common.c
> +++ b/drivers/media/v4l2-core/v4l2-common.c
> @@ -653,3 +653,65 @@ int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, int pixelformat, int width,
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(v4l2_fill_pixfmt);
> +
> +/*
> + * Conversion functions that convert a single-planar format to a
> + * multi-planar format.
> + */
> +void v4l2_fmt_sp2mp(const struct v4l2_format *sp_fmt,
> +		    struct v4l2_format *mp_fmt)
> +{
> +	struct v4l2_pix_format_mplane *mp = &mp_fmt->fmt.pix_mp;
> +	struct v4l2_plane_pix_format *ppix = &mp->plane_fmt[0];
> +	const struct v4l2_pix_format *pix = &sp_fmt->fmt.pix;
> +	bool is_out = sp_fmt->type == V4L2_BUF_TYPE_VIDEO_OUTPUT;
> +
> +	memset(mp->reserved, 0, sizeof(mp->reserved));
> +	mp_fmt->type = is_out ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE :
> +			   V4L2_CAP_VIDEO_CAPTURE_MPLANE;
> +	mp->width = pix->width;
> +	mp->height = pix->height;
> +	mp->pixelformat = pix->pixelformat;
> +	mp->field = pix->field;
> +	mp->colorspace = pix->colorspace;
> +	mp->xfer_func = pix->xfer_func;
> +	/* Also copies hsv_enc */
> +	mp->ycbcr_enc = pix->ycbcr_enc;
> +	mp->quantization = pix->quantization;
> +	mp->num_planes = 1;
> +	mp->flags = pix->flags;
> +	ppix->sizeimage = pix->sizeimage;
> +	ppix->bytesperline = pix->bytesperline;
> +	memset(ppix->reserved, 0, sizeof(ppix->reserved));
> +}
> +EXPORT_SYMBOL_GPL(v4l2_fmt_sp2mp);
> +
> +int v4l2_fmt_sp2mp_func(struct file *file, void *priv, struct v4l2_format *f,
> +			v4l2_fmtfunc func)
> +{
> +	struct v4l2_format fmt;
> +	struct v4l2_pix_format_mplane *mp = &fmt.fmt.pix_mp;
> +	struct v4l2_plane_pix_format *ppix = &mp->plane_fmt[0];
> +	struct v4l2_pix_format *pix = &f->fmt.pix;
> +	int ret;
> +
> +	/* Converts to a mplane format */
> +	v4l2_fmt_sp2mp(f, &fmt);
> +	/* Passes it to the generic mplane format function */
> +	ret = func(file, priv, &fmt);
> +	/* Copies back the mplane data to the single plane format */
> +	pix->width = mp->width;
> +	pix->height = mp->height;
> +	pix->pixelformat = mp->pixelformat;
> +	pix->field = mp->field;
> +	pix->colorspace = mp->colorspace;
> +	pix->xfer_func = mp->xfer_func;
> +	/* Also copies hsv_enc */
> +	pix->ycbcr_enc = mp->ycbcr_enc;
> +	pix->quantization = mp->quantization;
> +	pix->sizeimage = ppix->sizeimage;
> +	pix->bytesperline = ppix->bytesperline;
> +	pix->flags = mp->flags;
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_fmt_sp2mp_func);
> diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
> index 937b74a946cd..c5be976ce49e 100644
> --- a/include/media/v4l2-common.h
> +++ b/include/media/v4l2-common.h
> @@ -424,4 +424,41 @@ int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, int pixelformat,
>  int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt, int pixelformat,
>  			int width, int height);
>  
> +/**
> + * v4l2_fmtfunc - function pointer for v4l2_fmt_sp2mp()
> + *
> + * @file: device's descriptor file
> + * @priv: private data pointer
> + * @f: format that holds a mp pixel format
> + *
> + * Type to be passed as argument of v4l2_fmt_sp2mp() to be used by
> + * v4l2_fmt_sp2mp_func to pass the generic multi-planar function as argument.

I would re-word this a bit:

* Function type used by v4l2_fmt_sp2mp(). Please, see v4l2_fmt_sp2mp for
* more details.

And in the docs of function v4l2_fmt_sp2mp() you can explain better how
it is used.

Helen

> + */
> +typedef int (*v4l2_fmtfunc)(struct file *file, void *priv,
> +			    struct v4l2_format *f);
> +
> +/**
> + * v4l2_fmt_sp2mp - transforms a single-planar format struct into a multi-planar
> + * struct
> + *
> + * @sp_fmt: pointer to the single-planar format struct (in)
> + * @mp_fmt: pointer to the multi-planar format struct (out)
> + */
> +void v4l2_fmt_sp2mp(const struct v4l2_format *sp_fmt,
> +		    struct v4l2_format *mp_fmt);
> +
> +/**
> + * v4l2_fmt_sp2mp_func - handler for single-planar format functions
> + * @file: device's descriptor file
> + * @priv: private data pointer
> + * @f: format that holds a sp pixel format
> + * @func: generic mp function
> + *
> + * Handler to call a generic multi-planar format function using single-planar
> + * format. It converts the sp to a mp, calls the function and converts mp back
> + * to sp.
> + */
> +int v4l2_fmt_sp2mp_func(struct file *file, void *priv, struct v4l2_format *f,
> +			v4l2_fmtfunc func);
> +
>  #endif /* V4L2_COMMON_H_ */
> 

[PATCH v2 02/15] media: vimc: Remove unnecessary stream checks

[André Almeida]

Change the way subdevices check if the stream is running. Verify the stream
pointer instead of src_frame. This makes easier to get rid of the void* and
u8* in the subdevices structs. Also, remove redundant checks for stream.

Signed-off-by: André Almeida <andrealmeid@collabora.com>
---
Changes in v2:
- Squashed commits that remove unnecessary checks together
- Reworded commit message

 drivers/media/platform/vimc/vimc-debayer.c | 5 +----
 drivers/media/platform/vimc/vimc-scaler.c  | 7 ++-----
 drivers/media/platform/vimc/vimc-sensor.c  | 6 +-----
 3 files changed, 4 insertions(+), 14 deletions(-)

diff --git a/drivers/media/platform/vimc/vimc-debayer.c b/drivers/media/platform/vimc/vimc-debayer.c
index 419ad57a4f5c..613b7abbe06e 100644
--- a/drivers/media/platform/vimc/vimc-debayer.c
+++ b/drivers/media/platform/vimc/vimc-debayer.c
@@ -270,7 +270,7 @@ static int vimc_deb_set_fmt(struct v4l2_subdev *sd,
 
 	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
 		/* Do not change the format while stream is on */
-		if (vdeb->src_frame)
+		if (vdeb->ved.stream)
 			return -EBUSY;
 
 		sink_fmt = &vdeb->sink_fmt;
@@ -337,9 +337,6 @@ static int vimc_deb_s_stream(struct v4l2_subdev *sd, int enable)
 		const struct v4l2_format_info *pix_info;
 		unsigned int frame_size;
 
-		if (vdeb->src_frame)
-			return 0;
-
 		/* We only support translating bayer to RGB24 */
 		if (src_pixelformat != V4L2_PIX_FMT_RGB24) {
 			dev_err(vdeb->dev,
diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c
index f15542920bdc..8544c745c6e6 100644
--- a/drivers/media/platform/vimc/vimc-scaler.c
+++ b/drivers/media/platform/vimc/vimc-scaler.c
@@ -158,7 +158,7 @@ static int vimc_sca_set_fmt(struct v4l2_subdev *sd,
 
 	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
 		/* Do not change the format while stream is on */
-		if (vsca->src_frame)
+		if (vsca->ved.stream)
 			return -EBUSY;
 
 		sink_fmt = &vsca->sink_fmt;
@@ -213,9 +213,6 @@ static int vimc_sca_s_stream(struct v4l2_subdev *sd, int enable)
 		const struct v4l2_format_info *pix_info;
 		unsigned int frame_size;
 
-		if (vsca->src_frame)
-			return 0;
-
 		if (!vimc_sca_is_pixfmt_supported(pixelformat)) {
 			dev_err(vsca->dev, "pixfmt (%s) is not supported\n",
 				v4l2_get_fourcc_name(pixelformat));
@@ -337,7 +334,7 @@ static void *vimc_sca_process_frame(struct vimc_ent_device *ved,
 						    ved);
 
 	/* If the stream in this node is not active, just return */
-	if (!vsca->src_frame)
+	if (!ved->stream)
 		return ERR_PTR(-EINVAL);
 
 	vimc_sca_fill_src_frame(vsca, sink_frame);
diff --git a/drivers/media/platform/vimc/vimc-sensor.c b/drivers/media/platform/vimc/vimc-sensor.c
index 081e54204c9f..1b2b75b27952 100644
--- a/drivers/media/platform/vimc/vimc-sensor.c
+++ b/drivers/media/platform/vimc/vimc-sensor.c
@@ -141,7 +141,7 @@ static int vimc_sen_set_fmt(struct v4l2_subdev *sd,
 
 	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
 		/* Do not change the format while stream is on */
-		if (vsen->frame)
+		if (vsen->ved.stream)
 			return -EBUSY;
 
 		mf = &vsen->mbus_format;
@@ -197,10 +197,6 @@ static int vimc_sen_s_stream(struct v4l2_subdev *sd, int enable)
 		const struct v4l2_format_info *pix_info;
 		unsigned int frame_size;
 
-		if (vsen->kthread_sen)
-			/* tpg is already executing */
-			return 0;
-
 		/* Calculate the frame size */
 		pix_info = v4l2_format_info(pixelformat);
 		frame_size = vsen->mbus_format.width * pix_info->bpp[0] *
-- 
2.21.0

Re: [PATCH v2 02/15] media: vimc: Remove unnecessary stream checks

[Helen Koike]

On 3/27/19 12:17 PM, André Almeida wrote:
> Change the way subdevices check if the stream is running. Verify the stream
> pointer instead of src_frame. This makes easier to get rid of the void* and
> u8* in the subdevices structs. Also, remove redundant checks for stream.
> 
> Signed-off-by: André Almeida <andrealmeid@collabora.com>

Acked-by: Helen Koike <helen.koike@collabora.com>

> ---
> Changes in v2:
> - Squashed commits that remove unnecessary checks together
> - Reworded commit message
> 
>  drivers/media/platform/vimc/vimc-debayer.c | 5 +----
>  drivers/media/platform/vimc/vimc-scaler.c  | 7 ++-----
>  drivers/media/platform/vimc/vimc-sensor.c  | 6 +-----
>  3 files changed, 4 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/media/platform/vimc/vimc-debayer.c b/drivers/media/platform/vimc/vimc-debayer.c
> index 419ad57a4f5c..613b7abbe06e 100644
> --- a/drivers/media/platform/vimc/vimc-debayer.c
> +++ b/drivers/media/platform/vimc/vimc-debayer.c
> @@ -270,7 +270,7 @@ static int vimc_deb_set_fmt(struct v4l2_subdev *sd,
>  
>  	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
>  		/* Do not change the format while stream is on */
> -		if (vdeb->src_frame)
> +		if (vdeb->ved.stream)
>  			return -EBUSY;
>  
>  		sink_fmt = &vdeb->sink_fmt;
> @@ -337,9 +337,6 @@ static int vimc_deb_s_stream(struct v4l2_subdev *sd, int enable)
>  		const struct v4l2_format_info *pix_info;
>  		unsigned int frame_size;
>  
> -		if (vdeb->src_frame)
> -			return 0;
> -
>  		/* We only support translating bayer to RGB24 */
>  		if (src_pixelformat != V4L2_PIX_FMT_RGB24) {
>  			dev_err(vdeb->dev,
> diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c
> index f15542920bdc..8544c745c6e6 100644
> --- a/drivers/media/platform/vimc/vimc-scaler.c
> +++ b/drivers/media/platform/vimc/vimc-scaler.c
> @@ -158,7 +158,7 @@ static int vimc_sca_set_fmt(struct v4l2_subdev *sd,
>  
>  	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
>  		/* Do not change the format while stream is on */
> -		if (vsca->src_frame)
> +		if (vsca->ved.stream)
>  			return -EBUSY;
>  
>  		sink_fmt = &vsca->sink_fmt;
> @@ -213,9 +213,6 @@ static int vimc_sca_s_stream(struct v4l2_subdev *sd, int enable)
>  		const struct v4l2_format_info *pix_info;
>  		unsigned int frame_size;
>  
> -		if (vsca->src_frame)
> -			return 0;
> -
>  		if (!vimc_sca_is_pixfmt_supported(pixelformat)) {
>  			dev_err(vsca->dev, "pixfmt (%s) is not supported\n",
>  				v4l2_get_fourcc_name(pixelformat));
> @@ -337,7 +334,7 @@ static void *vimc_sca_process_frame(struct vimc_ent_device *ved,
>  						    ved);
>  
>  	/* If the stream in this node is not active, just return */
> -	if (!vsca->src_frame)
> +	if (!ved->stream)
>  		return ERR_PTR(-EINVAL);
>  
>  	vimc_sca_fill_src_frame(vsca, sink_frame);
> diff --git a/drivers/media/platform/vimc/vimc-sensor.c b/drivers/media/platform/vimc/vimc-sensor.c
> index 081e54204c9f..1b2b75b27952 100644
> --- a/drivers/media/platform/vimc/vimc-sensor.c
> +++ b/drivers/media/platform/vimc/vimc-sensor.c
> @@ -141,7 +141,7 @@ static int vimc_sen_set_fmt(struct v4l2_subdev *sd,
>  
>  	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
>  		/* Do not change the format while stream is on */
> -		if (vsen->frame)
> +		if (vsen->ved.stream)
>  			return -EBUSY;
>  
>  		mf = &vsen->mbus_format;
> @@ -197,10 +197,6 @@ static int vimc_sen_s_stream(struct v4l2_subdev *sd, int enable)
>  		const struct v4l2_format_info *pix_info;
>  		unsigned int frame_size;
>  
> -		if (vsen->kthread_sen)
> -			/* tpg is already executing */
> -			return 0;
> -
>  		/* Calculate the frame size */
>  		pix_info = v4l2_format_info(pixelformat);
>  		frame_size = vsen->mbus_format.width * pix_info->bpp[0] *
> 

[PATCH v2 03/15] media: vimc: cap: Change vimc_cap_device.format type

[André Almeida]

vimc_cap_device.format field was defined as v4l2_pix_format,
a struct to handle single planar pixel formats. It turns out that
if v4l2_format is used, we can reuse functions for both
v4l2_pix_format and v4l2_pix_format_mplane. This change will
help at future commits implementing multiplanar pixel
format support.

Signed-off-by: André Almeida <andrealmeid@collabora.com>
---
Change in v2: none

 drivers/media/platform/vimc/vimc-capture.c | 56 ++++++++++++----------
 1 file changed, 31 insertions(+), 25 deletions(-)

diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
index e7d0fc2228a6..de52f20b5c85 100644
--- a/drivers/media/platform/vimc/vimc-capture.c
+++ b/drivers/media/platform/vimc/vimc-capture.c
@@ -58,7 +58,7 @@ struct vimc_cap_device {
 	struct vimc_ent_device ved;
 	struct video_device vdev;
 	struct device *dev;
-	struct v4l2_pix_format format;
+	struct v4l2_format format;
 	struct vb2_queue queue;
 	struct list_head buf_list;
 	/*
@@ -74,12 +74,13 @@ struct vimc_cap_device {
 	struct vimc_stream stream;
 };
 
-static const struct v4l2_pix_format fmt_default = {
-	.width = 640,
-	.height = 480,
-	.pixelformat = V4L2_PIX_FMT_RGB24,
-	.field = V4L2_FIELD_NONE,
-	.colorspace = V4L2_COLORSPACE_DEFAULT,
+static const struct v4l2_format fmt_default = {
+	.type = V4L2_BUF_TYPE_VIDEO_CAPTURE,
+	.fmt.pix.width = 640,
+	.fmt.pix.height = 480,
+	.fmt.pix.pixelformat = V4L2_PIX_FMT_RGB24,
+	.fmt.pix.field = V4L2_FIELD_NONE,
+	.fmt.pix.colorspace = V4L2_COLORSPACE_DEFAULT,
 };
 
 struct vimc_cap_buffer {
@@ -110,7 +111,7 @@ static void vimc_cap_get_format(struct vimc_ent_device *ved,
 	struct vimc_cap_device *vcap = container_of(ved, struct vimc_cap_device,
 						    ved);
 
-	*fmt = vcap->format;
+	*fmt = vcap->format.fmt.pix;
 }
 
 static int vimc_cap_g_fmt_vid_cap(struct file *file, void *priv,
@@ -118,7 +119,7 @@ static int vimc_cap_g_fmt_vid_cap(struct file *file, void *priv,
 {
 	struct vimc_cap_device *vcap = video_drvdata(file);
 
-	f->fmt.pix = vcap->format;
+	*f = vcap->format;
 
 	return 0;
 }
@@ -136,13 +137,13 @@ static int vimc_cap_try_fmt_vid_cap(struct file *file, void *priv,
 	vimc_colorimetry_clamp(format);
 
 	if (format->field == V4L2_FIELD_ANY)
-		format->field = fmt_default.field;
+		format->field = fmt_default.fmt.pix.field;
 
 	/* TODO: Add support for custom bytesperline values */
 
 	/* Don't accept a pixelformat that is not on the table */
 	if (!v4l2_format_info(format->pixelformat))
-		format->pixelformat = fmt_default.pixelformat;
+		format->pixelformat = fmt_default.fmt.pix.pixelformat;
 
 	return v4l2_fill_pixfmt(format, format->pixelformat,
 				format->width, format->height);
@@ -163,17 +164,19 @@ static int vimc_cap_s_fmt_vid_cap(struct file *file, void *priv,
 		"old:%dx%d (0x%x, %d, %d, %d, %d) "
 		"new:%dx%d (0x%x, %d, %d, %d, %d)\n", vcap->vdev.name,
 		/* old */
-		vcap->format.width, vcap->format.height,
-		vcap->format.pixelformat, vcap->format.colorspace,
-		vcap->format.quantization, vcap->format.xfer_func,
-		vcap->format.ycbcr_enc,
+		vcap->format.fmt.pix.width, vcap->format.fmt.pix.height,
+		vcap->format.fmt.pix.pixelformat,
+		vcap->format.fmt.pix.colorspace,
+		vcap->format.fmt.pix.quantization,
+		vcap->format.fmt.pix.xfer_func,
+		vcap->format.fmt.pix.ycbcr_enc,
 		/* new */
 		f->fmt.pix.width, f->fmt.pix.height,
 		f->fmt.pix.pixelformat,	f->fmt.pix.colorspace,
 		f->fmt.pix.quantization, f->fmt.pix.xfer_func,
 		f->fmt.pix.ycbcr_enc);
 
-	vcap->format = f->fmt.pix;
+	vcap->format = *f;
 
 	return 0;
 }
@@ -279,7 +282,8 @@ static int vimc_cap_start_streaming(struct vb2_queue *vq, unsigned int count)
 		return ret;
 	}
 
-	vcap->stream.producer_pixfmt = vcap->format.pixelformat;
+	vcap->stream.producer_pixfmt = vcap->format.fmt.pix.pixelformat;
+
 	ret = vimc_streamer_s_stream(&vcap->stream, &vcap->ved, 1);
 	if (ret) {
 		media_pipeline_stop(entity);
@@ -326,10 +330,10 @@ static int vimc_cap_queue_setup(struct vb2_queue *vq, unsigned int *nbuffers,
 	struct vimc_cap_device *vcap = vb2_get_drv_priv(vq);
 
 	if (*nplanes)
-		return sizes[0] < vcap->format.sizeimage ? -EINVAL : 0;
+		return sizes[0] < vcap->format.fmt.pix.sizeimage ? -EINVAL : 0;
 	/* We don't support multiplanes for now */
 	*nplanes = 1;
-	sizes[0] = vcap->format.sizeimage;
+	sizes[0] = vcap->format.fmt.pix.sizeimage;
 
 	return 0;
 }
@@ -337,7 +341,7 @@ static int vimc_cap_queue_setup(struct vb2_queue *vq, unsigned int *nbuffers,
 static int vimc_cap_buffer_prepare(struct vb2_buffer *vb)
 {
 	struct vimc_cap_device *vcap = vb2_get_drv_priv(vb->vb2_queue);
-	unsigned long size = vcap->format.sizeimage;
+	unsigned long size = vcap->format.fmt.pix.sizeimage;
 
 	if (vb2_plane_size(vb, 0) < size) {
 		dev_err(vcap->dev, "%s: buffer too small (%lu < %lu)\n",
@@ -412,15 +416,15 @@ static void *vimc_cap_process_frame(struct vimc_ent_device *ved,
 	/* Fill the buffer */
 	vimc_buf->vb2.vb2_buf.timestamp = ktime_get_ns();
 	vimc_buf->vb2.sequence = vcap->sequence++;
-	vimc_buf->vb2.field = vcap->format.field;
+	vimc_buf->vb2.field = vcap->format.fmt.pix.field;
 
 	vbuf = vb2_plane_vaddr(&vimc_buf->vb2.vb2_buf, 0);
 
-	memcpy(vbuf, frame, vcap->format.sizeimage);
+	memcpy(vbuf, frame, vcap->format.fmt.pix.sizeimage);
 
 	/* Set it as ready */
 	vb2_set_plane_payload(&vimc_buf->vb2.vb2_buf, 0,
-			      vcap->format.sizeimage);
+			      vcap->format.fmt.pix.sizeimage);
 	vb2_buffer_done(&vimc_buf->vb2.vb2_buf, VB2_BUF_STATE_DONE);
 	return NULL;
 }
@@ -484,8 +488,10 @@ static int vimc_cap_comp_bind(struct device *comp, struct device *master,
 
 	/* Set default frame format */
 	vcap->format = fmt_default;
-	v4l2_fill_pixfmt(&vcap->format, vcap->format.pixelformat,
-			 vcap->format.width, vcap->format.height);
+	v4l2_fill_pixfmt(&vcap->format.fmt.pix,
+			 vcap->format.fmt.pix.pixelformat,
+			 vcap->format.fmt.pix.width,
+			 vcap->format.fmt.pix.height);
 
 	/* Fill the vimc_ent_device struct */
 	vcap->ved.ent = &vcap->vdev.entity;
-- 
2.21.0

Re: [PATCH v2 03/15] media: vimc: cap: Change vimc_cap_device.format type

[Helen Koike]

On 3/27/19 12:17 PM, André Almeida wrote:
> vimc_cap_device.format field was defined as v4l2_pix_format,
> a struct to handle single planar pixel formats. It turns out that
> if v4l2_format is used, we can reuse functions for both
> v4l2_pix_format and v4l2_pix_format_mplane. This change will
> help at future commits implementing multiplanar pixel
> format support.
> 
> Signed-off-by: André Almeida <andrealmeid@collabora.com>

Acked-by: Helen Koike <helen.koike@collabora.com>

> ---
> Change in v2: none
> 
>  drivers/media/platform/vimc/vimc-capture.c | 56 ++++++++++++----------
>  1 file changed, 31 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
> index e7d0fc2228a6..de52f20b5c85 100644
> --- a/drivers/media/platform/vimc/vimc-capture.c
> +++ b/drivers/media/platform/vimc/vimc-capture.c
> @@ -58,7 +58,7 @@ struct vimc_cap_device {
>  	struct vimc_ent_device ved;
>  	struct video_device vdev;
>  	struct device *dev;
> -	struct v4l2_pix_format format;
> +	struct v4l2_format format;
>  	struct vb2_queue queue;
>  	struct list_head buf_list;
>  	/*
> @@ -74,12 +74,13 @@ struct vimc_cap_device {
>  	struct vimc_stream stream;
>  };
>  
> -static const struct v4l2_pix_format fmt_default = {
> -	.width = 640,
> -	.height = 480,
> -	.pixelformat = V4L2_PIX_FMT_RGB24,
> -	.field = V4L2_FIELD_NONE,
> -	.colorspace = V4L2_COLORSPACE_DEFAULT,
> +static const struct v4l2_format fmt_default = {
> +	.type = V4L2_BUF_TYPE_VIDEO_CAPTURE,
> +	.fmt.pix.width = 640,
> +	.fmt.pix.height = 480,
> +	.fmt.pix.pixelformat = V4L2_PIX_FMT_RGB24,
> +	.fmt.pix.field = V4L2_FIELD_NONE,
> +	.fmt.pix.colorspace = V4L2_COLORSPACE_DEFAULT,
>  };
>  
>  struct vimc_cap_buffer {
> @@ -110,7 +111,7 @@ static void vimc_cap_get_format(struct vimc_ent_device *ved,
>  	struct vimc_cap_device *vcap = container_of(ved, struct vimc_cap_device,
>  						    ved);
>  
> -	*fmt = vcap->format;
> +	*fmt = vcap->format.fmt.pix;
>  }
>  
>  static int vimc_cap_g_fmt_vid_cap(struct file *file, void *priv,
> @@ -118,7 +119,7 @@ static int vimc_cap_g_fmt_vid_cap(struct file *file, void *priv,
>  {
>  	struct vimc_cap_device *vcap = video_drvdata(file);
>  
> -	f->fmt.pix = vcap->format;
> +	*f = vcap->format;
>  
>  	return 0;
>  }
> @@ -136,13 +137,13 @@ static int vimc_cap_try_fmt_vid_cap(struct file *file, void *priv,
>  	vimc_colorimetry_clamp(format);
>  
>  	if (format->field == V4L2_FIELD_ANY)
> -		format->field = fmt_default.field;
> +		format->field = fmt_default.fmt.pix.field;
>  
>  	/* TODO: Add support for custom bytesperline values */
>  
>  	/* Don't accept a pixelformat that is not on the table */
>  	if (!v4l2_format_info(format->pixelformat))
> -		format->pixelformat = fmt_default.pixelformat;
> +		format->pixelformat = fmt_default.fmt.pix.pixelformat;
>  
>  	return v4l2_fill_pixfmt(format, format->pixelformat,
>  				format->width, format->height);
> @@ -163,17 +164,19 @@ static int vimc_cap_s_fmt_vid_cap(struct file *file, void *priv,
>  		"old:%dx%d (0x%x, %d, %d, %d, %d) "
>  		"new:%dx%d (0x%x, %d, %d, %d, %d)\n", vcap->vdev.name,
>  		/* old */
> -		vcap->format.width, vcap->format.height,
> -		vcap->format.pixelformat, vcap->format.colorspace,
> -		vcap->format.quantization, vcap->format.xfer_func,
> -		vcap->format.ycbcr_enc,
> +		vcap->format.fmt.pix.width, vcap->format.fmt.pix.height,
> +		vcap->format.fmt.pix.pixelformat,
> +		vcap->format.fmt.pix.colorspace,
> +		vcap->format.fmt.pix.quantization,
> +		vcap->format.fmt.pix.xfer_func,
> +		vcap->format.fmt.pix.ycbcr_enc,
>  		/* new */
>  		f->fmt.pix.width, f->fmt.pix.height,
>  		f->fmt.pix.pixelformat,	f->fmt.pix.colorspace,
>  		f->fmt.pix.quantization, f->fmt.pix.xfer_func,
>  		f->fmt.pix.ycbcr_enc);
>  
> -	vcap->format = f->fmt.pix;
> +	vcap->format = *f;
>  
>  	return 0;
>  }
> @@ -279,7 +282,8 @@ static int vimc_cap_start_streaming(struct vb2_queue *vq, unsigned int count)
>  		return ret;
>  	}
>  
> -	vcap->stream.producer_pixfmt = vcap->format.pixelformat;
> +	vcap->stream.producer_pixfmt = vcap->format.fmt.pix.pixelformat;
> +
>  	ret = vimc_streamer_s_stream(&vcap->stream, &vcap->ved, 1);
>  	if (ret) {
>  		media_pipeline_stop(entity);
> @@ -326,10 +330,10 @@ static int vimc_cap_queue_setup(struct vb2_queue *vq, unsigned int *nbuffers,
>  	struct vimc_cap_device *vcap = vb2_get_drv_priv(vq);
>  
>  	if (*nplanes)
> -		return sizes[0] < vcap->format.sizeimage ? -EINVAL : 0;
> +		return sizes[0] < vcap->format.fmt.pix.sizeimage ? -EINVAL : 0;
>  	/* We don't support multiplanes for now */
>  	*nplanes = 1;
> -	sizes[0] = vcap->format.sizeimage;
> +	sizes[0] = vcap->format.fmt.pix.sizeimage;
>  
>  	return 0;
>  }
> @@ -337,7 +341,7 @@ static int vimc_cap_queue_setup(struct vb2_queue *vq, unsigned int *nbuffers,
>  static int vimc_cap_buffer_prepare(struct vb2_buffer *vb)
>  {
>  	struct vimc_cap_device *vcap = vb2_get_drv_priv(vb->vb2_queue);
> -	unsigned long size = vcap->format.sizeimage;
> +	unsigned long size = vcap->format.fmt.pix.sizeimage;
>  
>  	if (vb2_plane_size(vb, 0) < size) {
>  		dev_err(vcap->dev, "%s: buffer too small (%lu < %lu)\n",
> @@ -412,15 +416,15 @@ static void *vimc_cap_process_frame(struct vimc_ent_device *ved,
>  	/* Fill the buffer */
>  	vimc_buf->vb2.vb2_buf.timestamp = ktime_get_ns();
>  	vimc_buf->vb2.sequence = vcap->sequence++;
> -	vimc_buf->vb2.field = vcap->format.field;
> +	vimc_buf->vb2.field = vcap->format.fmt.pix.field;
>  
>  	vbuf = vb2_plane_vaddr(&vimc_buf->vb2.vb2_buf, 0);
>  
> -	memcpy(vbuf, frame, vcap->format.sizeimage);
> +	memcpy(vbuf, frame, vcap->format.fmt.pix.sizeimage);
>  
>  	/* Set it as ready */
>  	vb2_set_plane_payload(&vimc_buf->vb2.vb2_buf, 0,
> -			      vcap->format.sizeimage);
> +			      vcap->format.fmt.pix.sizeimage);
>  	vb2_buffer_done(&vimc_buf->vb2.vb2_buf, VB2_BUF_STATE_DONE);
>  	return NULL;
>  }
> @@ -484,8 +488,10 @@ static int vimc_cap_comp_bind(struct device *comp, struct device *master,
>  
>  	/* Set default frame format */
>  	vcap->format = fmt_default;
> -	v4l2_fill_pixfmt(&vcap->format, vcap->format.pixelformat,
> -			 vcap->format.width, vcap->format.height);
> +	v4l2_fill_pixfmt(&vcap->format.fmt.pix,
> +			 vcap->format.fmt.pix.pixelformat,
> +			 vcap->format.fmt.pix.width,
> +			 vcap->format.fmt.pix.height);
>  
>  	/* Fill the vimc_ent_device struct */
>  	vcap->ved.ent = &vcap->vdev.entity;
> 

[PATCH v2 04/15] media: vimc: cap: Dynamically define stream pixelformat

[André Almeida]

Define the pixelformat of the streamer depending to the
mode (single/multiplanar).

Signed-off-by: André Almeida <andrealmeid@collabora.com>
---
Change in v2:
- Add a macro to check if device is in multiplanar mode. Now, this check
is done using the capture device capabilities, rather than using the
multiplanar module parameter

 drivers/media/platform/vimc/vimc-capture.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
index de52f20b5c85..add6df565fa9 100644
--- a/drivers/media/platform/vimc/vimc-capture.c
+++ b/drivers/media/platform/vimc/vimc-capture.c
@@ -28,6 +28,10 @@
 
 #define VIMC_CAP_DRV_NAME "vimc-capture"
 
+/* Checks if the device supports multiplanar capture */
+#define IS_MULTIPLANAR(vcap) \
+	(vcap->vdev.device_caps & V4L2_CAP_VIDEO_CAPTURE_MPLANE)
+
 static const u32 vimc_cap_supported_pixfmt[] = {
 	V4L2_PIX_FMT_BGR24,
 	V4L2_PIX_FMT_RGB24,
@@ -282,7 +286,12 @@ static int vimc_cap_start_streaming(struct vb2_queue *vq, unsigned int count)
 		return ret;
 	}
 
-	vcap->stream.producer_pixfmt = vcap->format.fmt.pix.pixelformat;
+	if (IS_MULTIPLANAR(vcap))
+		vcap->stream.producer_pixfmt =
+			vcap->format.fmt.pix_mp.pixelformat;
+	else
+		vcap->stream.producer_pixfmt =
+			vcap->format.fmt.pix.pixelformat;
 
 	ret = vimc_streamer_s_stream(&vcap->stream, &vcap->ved, 1);
 	if (ret) {
-- 
2.21.0

Re: [PATCH v2 04/15] media: vimc: cap: Dynamically define stream pixelformat

[Helen Koike]

On 3/27/19 12:17 PM, André Almeida wrote:
> Define the pixelformat of the streamer depending to the
> mode (single/multiplanar).
> 
> Signed-off-by: André Almeida <andrealmeid@collabora.com>

Acked-by: Helen Koike <helen.koike@collabora.com>

> ---
> Change in v2:
> - Add a macro to check if device is in multiplanar mode. Now, this check
> is done using the capture device capabilities, rather than using the
> multiplanar module parameter
> 
>  drivers/media/platform/vimc/vimc-capture.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
> index de52f20b5c85..add6df565fa9 100644
> --- a/drivers/media/platform/vimc/vimc-capture.c
> +++ b/drivers/media/platform/vimc/vimc-capture.c
> @@ -28,6 +28,10 @@
>  
>  #define VIMC_CAP_DRV_NAME "vimc-capture"
>  
> +/* Checks if the device supports multiplanar capture */
> +#define IS_MULTIPLANAR(vcap) \
> +	(vcap->vdev.device_caps & V4L2_CAP_VIDEO_CAPTURE_MPLANE)
> +
>  static const u32 vimc_cap_supported_pixfmt[] = {
>  	V4L2_PIX_FMT_BGR24,
>  	V4L2_PIX_FMT_RGB24,
> @@ -282,7 +286,12 @@ static int vimc_cap_start_streaming(struct vb2_queue *vq, unsigned int count)
>  		return ret;
>  	}
>  
> -	vcap->stream.producer_pixfmt = vcap->format.fmt.pix.pixelformat;
> +	if (IS_MULTIPLANAR(vcap))
> +		vcap->stream.producer_pixfmt =
> +			vcap->format.fmt.pix_mp.pixelformat;
> +	else
> +		vcap->stream.producer_pixfmt =
> +			vcap->format.fmt.pix.pixelformat;
>  
>  	ret = vimc_streamer_s_stream(&vcap->stream, &vcap->ved, 1);
>  	if (ret) {
> 

[PATCH v2 05/15] media: vimc: cap: Add handler for singleplanar fmt ioctls

[André Almeida]

Since multiplanar is a superset of single planar formats, instead of
having different implementations for them, treat all formats as
multiplanar. If we need to work with single planar formats, convert them to
multiplanar (with num_planes = 1), re-use the multiplanar code, and
transform them back to single planar. This is implemented with
v4l2_fmt_sp2mp_func().

Signed-off-by: André Almeida <andrealmeid@collabora.com>
---
Changes in v2:
- Make more clear that the generic function _vimc_cap_try_fmt_vid_cap_mp
expects a multiplanar format as input 

 drivers/media/platform/vimc/vimc-capture.c | 75 +++++++++++++++++-----
 1 file changed, 58 insertions(+), 17 deletions(-)

diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
index add6df565fa9..24d70c2c9db7 100644
--- a/drivers/media/platform/vimc/vimc-capture.c
+++ b/drivers/media/platform/vimc/vimc-capture.c
@@ -128,10 +128,10 @@ static int vimc_cap_g_fmt_vid_cap(struct file *file, void *priv,
 	return 0;
 }
 
-static int vimc_cap_try_fmt_vid_cap(struct file *file, void *priv,
+static int _vimc_cap_try_fmt_vid_cap_mp(struct file *file, void *priv,
 				    struct v4l2_format *f)
 {
-	struct v4l2_pix_format *format = &f->fmt.pix;
+	struct v4l2_pix_format_mplane *format = &f->fmt.pix_mp;
 
 	format->width = clamp_t(u32, format->width, VIMC_FRAME_MIN_WIDTH,
 				VIMC_FRAME_MAX_WIDTH) & ~1;
@@ -149,20 +149,60 @@ static int vimc_cap_try_fmt_vid_cap(struct file *file, void *priv,
 	if (!v4l2_format_info(format->pixelformat))
 		format->pixelformat = fmt_default.fmt.pix.pixelformat;
 
-	return v4l2_fill_pixfmt(format, format->pixelformat,
-				format->width, format->height);
+	return v4l2_fill_pixfmt_mp(format, format->pixelformat,
+				   format->width, format->height);
 }
 
-static int vimc_cap_s_fmt_vid_cap(struct file *file, void *priv,
-				  struct v4l2_format *f)
+static int vimc_cap_enum_fmt_vid_cap(struct file *file, void *priv,
+				     struct v4l2_fmtdesc *f)
+{
+	if (f->index >= ARRAY_SIZE(vimc_cap_supported_pixfmt))
+		return -EINVAL;
+
+	f->pixelformat = vimc_cap_supported_pixfmt[f->index];
+
+	return 0;
+}
+
+/*
+ * VIDIOC FMT handlers for single-planar
+ */
+
+static int vimc_cap_g_fmt_vid_cap_sp(struct file *file, void *priv,
+				     struct v4l2_format *f)
+{
+	struct vimc_cap_device *vcap = video_drvdata(file);
+
+	if (IS_MULTIPLANAR(vcap))
+		return -EINVAL;
+
+	return vimc_cap_g_fmt_vid_cap(file, priv, f);
+}
+
+static int vimc_cap_try_fmt_vid_cap_sp(struct file *file, void *priv,
+				       struct v4l2_format *f)
 {
 	struct vimc_cap_device *vcap = video_drvdata(file);
 
+	if (IS_MULTIPLANAR(vcap))
+		return -EINVAL;
+
+	return v4l2_fmt_sp2mp_func(file, priv, f, _vimc_cap_try_fmt_vid_cap_mp);
+}
+
+static int vimc_cap_s_fmt_vid_cap_sp(struct file *file, void *priv,
+				     struct v4l2_format *f)
+{
+	struct vimc_cap_device *vcap = video_drvdata(file);
+
+	if (IS_MULTIPLANAR(vcap))
+		return -EINVAL;
+
 	/* Do not change the format while stream is on */
 	if (vb2_is_busy(&vcap->queue))
 		return -EBUSY;
 
-	vimc_cap_try_fmt_vid_cap(file, priv, f);
+	v4l2_fmt_sp2mp_func(file, priv, f, _vimc_cap_try_fmt_vid_cap_mp);
 
 	dev_dbg(vcap->dev, "%s: format update: "
 		"old:%dx%d (0x%x, %d, %d, %d, %d) "
@@ -185,15 +225,15 @@ static int vimc_cap_s_fmt_vid_cap(struct file *file, void *priv,
 	return 0;
 }
 
-static int vimc_cap_enum_fmt_vid_cap(struct file *file, void *priv,
-				     struct v4l2_fmtdesc *f)
+static int vimc_cap_enum_fmt_vid_cap_sp(struct file *file, void *priv,
+					struct v4l2_fmtdesc *f)
 {
-	if (f->index >= ARRAY_SIZE(vimc_cap_supported_pixfmt))
-		return -EINVAL;
+	struct vimc_cap_device *vcap = video_drvdata(file);
 
-	f->pixelformat = vimc_cap_supported_pixfmt[f->index];
+	if (IS_MULTIPLANAR(vcap))
+		return -EINVAL;
 
-	return 0;
+	return vimc_cap_enum_fmt_vid_cap(file, priv, f);
 }
 
 static bool vimc_cap_is_pixfmt_supported(u32 pixelformat)
@@ -239,10 +279,11 @@ static const struct v4l2_file_operations vimc_cap_fops = {
 static const struct v4l2_ioctl_ops vimc_cap_ioctl_ops = {
 	.vidioc_querycap = vimc_cap_querycap,
 
-	.vidioc_g_fmt_vid_cap = vimc_cap_g_fmt_vid_cap,
-	.vidioc_s_fmt_vid_cap = vimc_cap_s_fmt_vid_cap,
-	.vidioc_try_fmt_vid_cap = vimc_cap_try_fmt_vid_cap,
-	.vidioc_enum_fmt_vid_cap = vimc_cap_enum_fmt_vid_cap,
+	.vidioc_g_fmt_vid_cap = vimc_cap_g_fmt_vid_cap_sp,
+	.vidioc_s_fmt_vid_cap = vimc_cap_s_fmt_vid_cap_sp,
+	.vidioc_try_fmt_vid_cap = vimc_cap_try_fmt_vid_cap_sp,
+	.vidioc_enum_fmt_vid_cap = vimc_cap_enum_fmt_vid_cap_sp,
+
 	.vidioc_enum_framesizes = vimc_cap_enum_framesizes,
 
 	.vidioc_reqbufs = vb2_ioctl_reqbufs,
-- 
2.21.0

Re: [PATCH v2 05/15] media: vimc: cap: Add handler for singleplanar fmt ioctls

[Helen Koike]

Hi Andre,

On 3/27/19 12:17 PM, André Almeida wrote:
> Since multiplanar is a superset of single planar formats, instead of
> having different implementations for them, treat all formats as
> multiplanar. If we need to work with single planar formats, convert them to
> multiplanar (with num_planes = 1), re-use the multiplanar code, and
> transform them back to single planar. This is implemented with
> v4l2_fmt_sp2mp_func().

I would rename the commit title as you are not really adding handlers,
the handlers are already there, you are refactoring to make it easier to
add multiplanar handlers in the next commit.

Maybe:
media: vimc: cap: refactor singleplanar as a subset of multiplanar

> 
> Signed-off-by: André Almeida <andrealmeid@collabora.com>
> ---
> Changes in v2:
> - Make more clear that the generic function _vimc_cap_try_fmt_vid_cap_mp
> expects a multiplanar format as input 
> 
>  drivers/media/platform/vimc/vimc-capture.c | 75 +++++++++++++++++-----
>  1 file changed, 58 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
> index add6df565fa9..24d70c2c9db7 100644
> --- a/drivers/media/platform/vimc/vimc-capture.c
> +++ b/drivers/media/platform/vimc/vimc-capture.c
> @@ -128,10 +128,10 @@ static int vimc_cap_g_fmt_vid_cap(struct file *file, void *priv,
>  	return 0;
>  }
>  
> -static int vimc_cap_try_fmt_vid_cap(struct file *file, void *priv,
> +static int _vimc_cap_try_fmt_vid_cap_mp(struct file *file, void *priv,
>  				    struct v4l2_format *f)

Please align the argument.

>  {
> -	struct v4l2_pix_format *format = &f->fmt.pix;
> +	struct v4l2_pix_format_mplane *format = &f->fmt.pix_mp;
>  
>  	format->width = clamp_t(u32, format->width, VIMC_FRAME_MIN_WIDTH,
>  				VIMC_FRAME_MAX_WIDTH) & ~1;
> @@ -149,20 +149,60 @@ static int vimc_cap_try_fmt_vid_cap(struct file *file, void *priv,
>  	if (!v4l2_format_info(format->pixelformat))
>  		format->pixelformat = fmt_default.fmt.pix.pixelformat;
>  
> -	return v4l2_fill_pixfmt(format, format->pixelformat,
> -				format->width, format->height);
> +	return v4l2_fill_pixfmt_mp(format, format->pixelformat,
> +				   format->width, format->height);
>  }
>  
> -static int vimc_cap_s_fmt_vid_cap(struct file *file, void *priv,
> -				  struct v4l2_format *f)
> +static int vimc_cap_enum_fmt_vid_cap(struct file *file, void *priv,
> +				     struct v4l2_fmtdesc *f)
> +{
> +	if (f->index >= ARRAY_SIZE(vimc_cap_supported_pixfmt))
> +		return -EINVAL;
> +
> +	f->pixelformat = vimc_cap_supported_pixfmt[f->index];
> +
> +	return 0;
> +}
> +
> +/*
> + * VIDIOC FMT handlers for single-planar
> + */
> +
> +static int vimc_cap_g_fmt_vid_cap_sp(struct file *file, void *priv,
> +				     struct v4l2_format *f)
> +{
> +	struct vimc_cap_device *vcap = video_drvdata(file);
> +
> +	if (IS_MULTIPLANAR(vcap))
> +		return -EINVAL;

I think you can get rid of these checks, you just need to make sure only
sp callbacks are registered in vimc_cap_fops if you are in sp mode; And
only mp callbacks are registered in vimc_cap_fops if you are in mp mode
(as we don't support both at the same time atm.

You could check the planar mode in the bind function, then you can alter
fops before calling video_register_device(), .

This would clean your code a bit, and you won't need these two functions
in your next patch:
_vimc_cap_try_fmt_vid_cap_mp
vimc_cap_try_fmt_vid_cap_mp
as the first just performs this check and then calls the second, you can
eliminate the check and remove _vimc_cap_try_fmt_vid_cap_mp.

Helen


> +
> +	return vimc_cap_g_fmt_vid_cap(file, priv, f);
> +}
> +
> +static int vimc_cap_try_fmt_vid_cap_sp(struct file *file, void *priv,
> +				       struct v4l2_format *f)
>  {
>  	struct vimc_cap_device *vcap = video_drvdata(file);
>  
> +	if (IS_MULTIPLANAR(vcap))
> +		return -EINVAL;
> +
> +	return v4l2_fmt_sp2mp_func(file, priv, f, _vimc_cap_try_fmt_vid_cap_mp);
> +}
> +
> +static int vimc_cap_s_fmt_vid_cap_sp(struct file *file, void *priv,
> +				     struct v4l2_format *f)
> +{
> +	struct vimc_cap_device *vcap = video_drvdata(file);
> +
> +	if (IS_MULTIPLANAR(vcap))
> +		return -EINVAL;
> +
>  	/* Do not change the format while stream is on */
>  	if (vb2_is_busy(&vcap->queue))
>  		return -EBUSY;
>  
> -	vimc_cap_try_fmt_vid_cap(file, priv, f);
> +	v4l2_fmt_sp2mp_func(file, priv, f, _vimc_cap_try_fmt_vid_cap_mp);
>  
>  	dev_dbg(vcap->dev, "%s: format update: "
>  		"old:%dx%d (0x%x, %d, %d, %d, %d) "
> @@ -185,15 +225,15 @@ static int vimc_cap_s_fmt_vid_cap(struct file *file, void *priv,
>  	return 0;
>  }
>  
> -static int vimc_cap_enum_fmt_vid_cap(struct file *file, void *priv,
> -				     struct v4l2_fmtdesc *f)
> +static int vimc_cap_enum_fmt_vid_cap_sp(struct file *file, void *priv,
> +					struct v4l2_fmtdesc *f)
>  {
> -	if (f->index >= ARRAY_SIZE(vimc_cap_supported_pixfmt))
> -		return -EINVAL;
> +	struct vimc_cap_device *vcap = video_drvdata(file);
>  
> -	f->pixelformat = vimc_cap_supported_pixfmt[f->index];
> +	if (IS_MULTIPLANAR(vcap))
> +		return -EINVAL;
>  
> -	return 0;
> +	return vimc_cap_enum_fmt_vid_cap(file, priv, f);
>  }
>  
>  static bool vimc_cap_is_pixfmt_supported(u32 pixelformat)
> @@ -239,10 +279,11 @@ static const struct v4l2_file_operations vimc_cap_fops = {
>  static const struct v4l2_ioctl_ops vimc_cap_ioctl_ops = {
>  	.vidioc_querycap = vimc_cap_querycap,
>  
> -	.vidioc_g_fmt_vid_cap = vimc_cap_g_fmt_vid_cap,
> -	.vidioc_s_fmt_vid_cap = vimc_cap_s_fmt_vid_cap,
> -	.vidioc_try_fmt_vid_cap = vimc_cap_try_fmt_vid_cap,
> -	.vidioc_enum_fmt_vid_cap = vimc_cap_enum_fmt_vid_cap,
> +	.vidioc_g_fmt_vid_cap = vimc_cap_g_fmt_vid_cap_sp,
> +	.vidioc_s_fmt_vid_cap = vimc_cap_s_fmt_vid_cap_sp,
> +	.vidioc_try_fmt_vid_cap = vimc_cap_try_fmt_vid_cap_sp,
> +	.vidioc_enum_fmt_vid_cap = vimc_cap_enum_fmt_vid_cap_sp,
> +
>  	.vidioc_enum_framesizes = vimc_cap_enum_framesizes,
>  
>  	.vidioc_reqbufs = vb2_ioctl_reqbufs,
> 

[PATCH v2 06/15] media: vimc: cap: Add handler for multiplanar fmt ioctls

[André Almeida]

Add functions to handle multiplanar format ioctls, calling
the generic format ioctls functions when possible.

Signed-off-by: André Almeida <andrealmeid@collabora.com>
---
Change in v2:
- Fix alignment

 drivers/media/platform/vimc/vimc-capture.c | 85 ++++++++++++++++++++++
 1 file changed, 85 insertions(+)

diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
index 24d70c2c9db7..5e13c6580aff 100644
--- a/drivers/media/platform/vimc/vimc-capture.c
+++ b/drivers/media/platform/vimc/vimc-capture.c
@@ -118,6 +118,10 @@ static void vimc_cap_get_format(struct vimc_ent_device *ved,
 	*fmt = vcap->format.fmt.pix;
 }
 
+/*
+ * Functions to handle both single- and multi-planar VIDIOC FMT
+ */
+
 static int vimc_cap_g_fmt_vid_cap(struct file *file, void *priv,
 				  struct v4l2_format *f)
 {
@@ -245,6 +249,77 @@ static bool vimc_cap_is_pixfmt_supported(u32 pixelformat)
 			return true;
 	return false;
 }
+/*
+ * VIDIOC handlers for multi-planar formats
+ */
+
+static int vimc_cap_g_fmt_vid_cap_mp(struct file *file, void *priv,
+				     struct v4l2_format *f)
+{
+	struct vimc_cap_device *vcap = video_drvdata(file);
+
+	if (!IS_MULTIPLANAR(vcap))
+		return -EINVAL;
+
+	return vimc_cap_g_fmt_vid_cap(file, priv, f);
+}
+
+static int vimc_cap_try_fmt_vid_cap_mp(struct file *file, void *priv,
+				       struct v4l2_format *f)
+{
+	struct vimc_cap_device *vcap = video_drvdata(file);
+
+	if (!IS_MULTIPLANAR(vcap))
+		return -EINVAL;
+
+	return _vimc_cap_try_fmt_vid_cap_mp(file, priv, f);
+}
+
+static int vimc_cap_s_fmt_vid_cap_mp(struct file *file, void *priv,
+				     struct v4l2_format *f)
+{
+	struct vimc_cap_device *vcap = video_drvdata(file);
+
+	if (!IS_MULTIPLANAR(vcap))
+		return -EINVAL;
+
+	/* Do not change the format while stream is on */
+	if (vb2_is_busy(&vcap->queue))
+		return -EBUSY;
+
+	_vimc_cap_try_fmt_vid_cap_mp(file, priv, f);
+
+	dev_dbg(vcap->dev, "%s: format update: "
+		"old:%dx%d (0x%x, %d, %d, %d, %d) "
+		"new:%dx%d (0x%x, %d, %d, %d, %d)\n", vcap->vdev.name,
+		/* old */
+		vcap->format.fmt.pix_mp.width, vcap->format.fmt.pix_mp.height,
+		vcap->format.fmt.pix_mp.pixelformat,
+		vcap->format.fmt.pix_mp.colorspace,
+		vcap->format.fmt.pix_mp.quantization,
+		vcap->format.fmt.pix_mp.xfer_func,
+		vcap->format.fmt.pix_mp.ycbcr_enc,
+		/* new */
+		f->fmt.pix_mp.width, f->fmt.pix_mp.height,
+		f->fmt.pix_mp.pixelformat, f->fmt.pix_mp.colorspace,
+		f->fmt.pix_mp.quantization, f->fmt.pix_mp.xfer_func,
+		f->fmt.pix_mp.ycbcr_enc);
+
+	vcap->format = *f;
+
+	return 0;
+}
+
+static int vimc_cap_enum_fmt_vid_cap_mp(struct file *file, void *priv,
+					struct v4l2_fmtdesc *f)
+{
+	struct vimc_cap_device *vcap = video_drvdata(file);
+
+	if (!IS_MULTIPLANAR(vcap))
+		return -EINVAL;
+
+	return vimc_cap_enum_fmt_vid_cap(file, priv, f);
+}
 
 static int vimc_cap_enum_framesizes(struct file *file, void *fh,
 				    struct v4l2_frmsizeenum *fsize)
@@ -276,14 +351,24 @@ static const struct v4l2_file_operations vimc_cap_fops = {
 	.mmap           = vb2_fop_mmap,
 };
 
+
 static const struct v4l2_ioctl_ops vimc_cap_ioctl_ops = {
 	.vidioc_querycap = vimc_cap_querycap,
 
+	/**
+	 * The vidioc_*_vid_cap* functions acts as a front end to
+	 * vimc_*_vid_cap, dealing with the single- and multi-planar
+	 */
 	.vidioc_g_fmt_vid_cap = vimc_cap_g_fmt_vid_cap_sp,
 	.vidioc_s_fmt_vid_cap = vimc_cap_s_fmt_vid_cap_sp,
 	.vidioc_try_fmt_vid_cap = vimc_cap_try_fmt_vid_cap_sp,
 	.vidioc_enum_fmt_vid_cap = vimc_cap_enum_fmt_vid_cap_sp,
 
+	.vidioc_g_fmt_vid_cap_mplane = vimc_cap_g_fmt_vid_cap_mp,
+	.vidioc_s_fmt_vid_cap_mplane = vimc_cap_s_fmt_vid_cap_mp,
+	.vidioc_try_fmt_vid_cap_mplane = vimc_cap_try_fmt_vid_cap_mp,
+	.vidioc_enum_fmt_vid_cap_mplane = vimc_cap_enum_fmt_vid_cap_mp,
+
 	.vidioc_enum_framesizes = vimc_cap_enum_framesizes,
 
 	.vidioc_reqbufs = vb2_ioctl_reqbufs,
-- 
2.21.0

[PATCH v2 07/15] media: vimc: cap: Add multiplanar formats

[André Almeida]

Add multiplanar formats to be exposed to the userspace as
supported formats. Since we don't want to support multiplanar
formats when the driver is in singleplanar mode, we only access
the multiplanar formats array if the multiplanar mode is enabled.

Signed-off-by: André Almeida <andrealmeid@collabora.com>
---
Changes in v2:
- Change line break style
- Add TODO comment

 drivers/media/platform/vimc/vimc-capture.c | 34 ++++++++++++++++++++--
 1 file changed, 32 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
index 5e13c6580aff..a65611be63bb 100644
--- a/drivers/media/platform/vimc/vimc-capture.c
+++ b/drivers/media/platform/vimc/vimc-capture.c
@@ -32,6 +32,10 @@
 #define IS_MULTIPLANAR(vcap) \
 	(vcap->vdev.device_caps & V4L2_CAP_VIDEO_CAPTURE_MPLANE)
 
+/**
+ * TODO: capture device should only enum formats that all subdevices on
+ * topology accepts
+ */
 static const u32 vimc_cap_supported_pixfmt[] = {
 	V4L2_PIX_FMT_BGR24,
 	V4L2_PIX_FMT_RGB24,
@@ -58,6 +62,19 @@ static const u32 vimc_cap_supported_pixfmt[] = {
 	V4L2_PIX_FMT_SRGGB12,
 };
 
+static const u32 vimc_cap_supported_pixfmt_mp[] = {
+	V4L2_PIX_FMT_YUV420M,
+	V4L2_PIX_FMT_YVU420M,
+	V4L2_PIX_FMT_YUV422M,
+	V4L2_PIX_FMT_YVU422M,
+	V4L2_PIX_FMT_YUV444M,
+	V4L2_PIX_FMT_YVU444M,
+	V4L2_PIX_FMT_NV12M,
+	V4L2_PIX_FMT_NV21M,
+	V4L2_PIX_FMT_NV16M,
+	V4L2_PIX_FMT_NV61M,
+};
+
 struct vimc_cap_device {
 	struct vimc_ent_device ved;
 	struct video_device vdev;
@@ -157,13 +174,26 @@ static int _vimc_cap_try_fmt_vid_cap_mp(struct file *file, void *priv,
 				   format->width, format->height);
 }
 
+/**
+ * When multiplanar is true, consider that the vimc_cap_enum_fmt_vid_cap_mp
+ * is concantenate in the vimc_cap_enum_fmt_vid_cap array. Otherwise, just
+ * consider the single-planar array
+ */
 static int vimc_cap_enum_fmt_vid_cap(struct file *file, void *priv,
 				     struct v4l2_fmtdesc *f)
 {
-	if (f->index >= ARRAY_SIZE(vimc_cap_supported_pixfmt))
+	struct vimc_cap_device *vcap = video_drvdata(file);
+	const unsigned int sp_size = ARRAY_SIZE(vimc_cap_supported_pixfmt);
+	const unsigned int mp_size = ARRAY_SIZE(vimc_cap_supported_pixfmt_mp);
+
+	if (f->index >= sp_size + (IS_MULTIPLANAR(vcap) ? mp_size : 0))
 		return -EINVAL;
 
-	f->pixelformat = vimc_cap_supported_pixfmt[f->index];
+	if (f->index >= sp_size)
+		f->pixelformat =
+			vimc_cap_supported_pixfmt_mp[f->index - sp_size];
+	else
+		f->pixelformat = vimc_cap_supported_pixfmt[f->index];
 
 	return 0;
 }
-- 
2.21.0

Re: [PATCH v2 07/15] media: vimc: cap: Add multiplanar formats

[Helen Koike]

On 3/27/19 12:17 PM, André Almeida wrote:
> Add multiplanar formats to be exposed to the userspace as
> supported formats. Since we don't want to support multiplanar
> formats when the driver is in singleplanar mode, we only access
> the multiplanar formats array if the multiplanar mode is enabled.
> 
> Signed-off-by: André Almeida <andrealmeid@collabora.com>

Acked-by: Helen Koike <helen.koike@collabora.com>

> ---
> Changes in v2:
> - Change line break style
> - Add TODO comment
> 
>  drivers/media/platform/vimc/vimc-capture.c | 34 ++++++++++++++++++++--
>  1 file changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
> index 5e13c6580aff..a65611be63bb 100644
> --- a/drivers/media/platform/vimc/vimc-capture.c
> +++ b/drivers/media/platform/vimc/vimc-capture.c
> @@ -32,6 +32,10 @@
>  #define IS_MULTIPLANAR(vcap) \
>  	(vcap->vdev.device_caps & V4L2_CAP_VIDEO_CAPTURE_MPLANE)
>  
> +/**
> + * TODO: capture device should only enum formats that all subdevices on
> + * topology accepts
> + */
>  static const u32 vimc_cap_supported_pixfmt[] = {
>  	V4L2_PIX_FMT_BGR24,
>  	V4L2_PIX_FMT_RGB24,
> @@ -58,6 +62,19 @@ static const u32 vimc_cap_supported_pixfmt[] = {
>  	V4L2_PIX_FMT_SRGGB12,
>  };
>  
> +static const u32 vimc_cap_supported_pixfmt_mp[] = {
> +	V4L2_PIX_FMT_YUV420M,
> +	V4L2_PIX_FMT_YVU420M,
> +	V4L2_PIX_FMT_YUV422M,
> +	V4L2_PIX_FMT_YVU422M,
> +	V4L2_PIX_FMT_YUV444M,
> +	V4L2_PIX_FMT_YVU444M,
> +	V4L2_PIX_FMT_NV12M,
> +	V4L2_PIX_FMT_NV21M,
> +	V4L2_PIX_FMT_NV16M,
> +	V4L2_PIX_FMT_NV61M,
> +};
> +
>  struct vimc_cap_device {
>  	struct vimc_ent_device ved;
>  	struct video_device vdev;
> @@ -157,13 +174,26 @@ static int _vimc_cap_try_fmt_vid_cap_mp(struct file *file, void *priv,
>  				   format->width, format->height);
>  }
>  
> +/**
> + * When multiplanar is true, consider that the vimc_cap_enum_fmt_vid_cap_mp
> + * is concantenate in the vimc_cap_enum_fmt_vid_cap array. Otherwise, just
> + * consider the single-planar array
> + */
>  static int vimc_cap_enum_fmt_vid_cap(struct file *file, void *priv,
>  				     struct v4l2_fmtdesc *f)
>  {
> -	if (f->index >= ARRAY_SIZE(vimc_cap_supported_pixfmt))
> +	struct vimc_cap_device *vcap = video_drvdata(file);
> +	const unsigned int sp_size = ARRAY_SIZE(vimc_cap_supported_pixfmt);
> +	const unsigned int mp_size = ARRAY_SIZE(vimc_cap_supported_pixfmt_mp);
> +
> +	if (f->index >= sp_size + (IS_MULTIPLANAR(vcap) ? mp_size : 0))
>  		return -EINVAL;
>  
> -	f->pixelformat = vimc_cap_supported_pixfmt[f->index];
> +	if (f->index >= sp_size)
> +		f->pixelformat =
> +			vimc_cap_supported_pixfmt_mp[f->index - sp_size];
> +	else
> +		f->pixelformat = vimc_cap_supported_pixfmt[f->index];
>  
>  	return 0;
>  }
> 

[PATCH v2 08/15] media: vimc: cap: Add multiplanar default format

[André Almeida]

vimc already have a default single planar default format.
Add a multiplanar default pixel format to perform those
same actions. Change where the default pixelformat is set to make sure
that the vcap is with the right capabilities.

Signed-off-by: André Almeida <andrealmeid@collabora.com>
---
Change in v2:
- Move here the default format is set to verify if the device have a
multiplanar capability before assign the default format

 drivers/media/platform/vimc/vimc-capture.c | 38 +++++++++++++++++-----
 1 file changed, 30 insertions(+), 8 deletions(-)

diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
index a65611be63bb..c344d04ed8ea 100644
--- a/drivers/media/platform/vimc/vimc-capture.c
+++ b/drivers/media/platform/vimc/vimc-capture.c
@@ -104,6 +104,15 @@ static const struct v4l2_format fmt_default = {
 	.fmt.pix.colorspace = V4L2_COLORSPACE_DEFAULT,
 };
 
+static const struct v4l2_format fmt_default_mp = {
+	.type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE,
+	.fmt.pix_mp.width = 640,
+	.fmt.pix_mp.height = 480,
+	.fmt.pix_mp.pixelformat = V4L2_PIX_FMT_YVU420M,
+	.fmt.pix_mp.field = V4L2_FIELD_NONE,
+	.fmt.pix_mp.colorspace = V4L2_COLORSPACE_DEFAULT,
+};
+
 struct vimc_cap_buffer {
 	/*
 	 * struct vb2_v4l2_buffer must be the first element
@@ -153,6 +162,7 @@ static int _vimc_cap_try_fmt_vid_cap_mp(struct file *file, void *priv,
 				    struct v4l2_format *f)
 {
 	struct v4l2_pix_format_mplane *format = &f->fmt.pix_mp;
+	struct vimc_cap_device *vcap = video_drvdata(file);
 
 	format->width = clamp_t(u32, format->width, VIMC_FRAME_MIN_WIDTH,
 				VIMC_FRAME_MAX_WIDTH) & ~1;
@@ -168,7 +178,9 @@ static int _vimc_cap_try_fmt_vid_cap_mp(struct file *file, void *priv,
 
 	/* Don't accept a pixelformat that is not on the table */
 	if (!v4l2_format_info(format->pixelformat))
-		format->pixelformat = fmt_default.fmt.pix.pixelformat;
+		format->pixelformat = IS_MULTIPLANAR(vcap) ?
+				fmt_default_mp.fmt.pix_mp.pixelformat :
+				fmt_default.fmt.pix.pixelformat;
 
 	return v4l2_fill_pixfmt_mp(format, format->pixelformat,
 				   format->width, format->height);
@@ -651,13 +663,6 @@ static int vimc_cap_comp_bind(struct device *comp, struct device *master,
 	INIT_LIST_HEAD(&vcap->buf_list);
 	spin_lock_init(&vcap->qlock);
 
-	/* Set default frame format */
-	vcap->format = fmt_default;
-	v4l2_fill_pixfmt(&vcap->format.fmt.pix,
-			 vcap->format.fmt.pix.pixelformat,
-			 vcap->format.fmt.pix.width,
-			 vcap->format.fmt.pix.height);
-
 	/* Fill the vimc_ent_device struct */
 	vcap->ved.ent = &vcap->vdev.entity;
 	vcap->ved.process_frame = vimc_cap_process_frame;
@@ -679,6 +684,23 @@ static int vimc_cap_comp_bind(struct device *comp, struct device *master,
 	strscpy(vdev->name, pdata->entity_name, sizeof(vdev->name));
 	video_set_drvdata(vdev, &vcap->ved);
 
+	dev_dbg(vcap->dev, "vimc-capture is in %s mode", IS_MULTIPLANAR(vcap) ?
+		"multiplanar" : "singleplanar");
+
+	if (IS_MULTIPLANAR(vcap)) {
+		vcap->format = fmt_default_mp;
+		v4l2_fill_pixfmt_mp(&vcap->format.fmt.pix_mp,
+				vcap->format.fmt.pix_mp.pixelformat,
+				vcap->format.fmt.pix_mp.width,
+				vcap->format.fmt.pix_mp.height);
+	} else {
+		vcap->format = fmt_default;
+		v4l2_fill_pixfmt(&vcap->format.fmt.pix,
+				vcap->format.fmt.pix.pixelformat,
+				vcap->format.fmt.pix.width,
+				vcap->format.fmt.pix.height);
+	}
+
 	/* Register the video_device with the v4l2 and the media framework */
 	ret = video_register_device(vdev, VFL_TYPE_GRABBER, -1);
 	if (ret) {
-- 
2.21.0

Re: [PATCH v2 08/15] media: vimc: cap: Add multiplanar default format

[Helen Koike]

On 3/27/19 12:17 PM, André Almeida wrote:
> vimc already have a default single planar default format.

The word "default" is repeated twice.

> Add a multiplanar default pixel format to perform those
> same actions. Change where the default pixelformat is set to make sure
> that the vcap is with the right capabilities.
> 
> Signed-off-by: André Almeida <andrealmeid@collabora.com>
> ---
> Change in v2:
> - Move here the default format is set to verify if the device have a
> multiplanar capability before assign the default format
> 
>  drivers/media/platform/vimc/vimc-capture.c | 38 +++++++++++++++++-----
>  1 file changed, 30 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
> index a65611be63bb..c344d04ed8ea 100644
> --- a/drivers/media/platform/vimc/vimc-capture.c
> +++ b/drivers/media/platform/vimc/vimc-capture.c
> @@ -104,6 +104,15 @@ static const struct v4l2_format fmt_default = {
>  	.fmt.pix.colorspace = V4L2_COLORSPACE_DEFAULT,
>  };
>  
> +static const struct v4l2_format fmt_default_mp = {

I would just add a "_sp" sufix to the sp version to make it consistent
with previous patches.

> +	.type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE,
> +	.fmt.pix_mp.width = 640,
> +	.fmt.pix_mp.height = 480,
> +	.fmt.pix_mp.pixelformat = V4L2_PIX_FMT_YVU420M,
> +	.fmt.pix_mp.field = V4L2_FIELD_NONE,
> +	.fmt.pix_mp.colorspace = V4L2_COLORSPACE_DEFAULT,
> +};
> +
>  struct vimc_cap_buffer {
>  	/*
>  	 * struct vb2_v4l2_buffer must be the first element
> @@ -153,6 +162,7 @@ static int _vimc_cap_try_fmt_vid_cap_mp(struct file *file, void *priv,
>  				    struct v4l2_format *f)
>  {
>  	struct v4l2_pix_format_mplane *format = &f->fmt.pix_mp;
> +	struct vimc_cap_device *vcap = video_drvdata(file);
>  
>  	format->width = clamp_t(u32, format->width, VIMC_FRAME_MIN_WIDTH,
>  				VIMC_FRAME_MAX_WIDTH) & ~1;
> @@ -168,7 +178,9 @@ static int _vimc_cap_try_fmt_vid_cap_mp(struct file *file, void *priv,
>  
>  	/* Don't accept a pixelformat that is not on the table */
>  	if (!v4l2_format_info(format->pixelformat))
> -		format->pixelformat = fmt_default.fmt.pix.pixelformat;
> +		format->pixelformat = IS_MULTIPLANAR(vcap) ?
> +				fmt_default_mp.fmt.pix_mp.pixelformat :
> +				fmt_default.fmt.pix.pixelformat;
>  
>  	return v4l2_fill_pixfmt_mp(format, format->pixelformat,
>  				   format->width, format->height);
> @@ -651,13 +663,6 @@ static int vimc_cap_comp_bind(struct device *comp, struct device *master,
>  	INIT_LIST_HEAD(&vcap->buf_list);
>  	spin_lock_init(&vcap->qlock);
>  
> -	/* Set default frame format */
> -	vcap->format = fmt_default;
> -	v4l2_fill_pixfmt(&vcap->format.fmt.pix,
> -			 vcap->format.fmt.pix.pixelformat,
> -			 vcap->format.fmt.pix.width,
> -			 vcap->format.fmt.pix.height);
> -
>  	/* Fill the vimc_ent_device struct */
>  	vcap->ved.ent = &vcap->vdev.entity;
>  	vcap->ved.process_frame = vimc_cap_process_frame;
> @@ -679,6 +684,23 @@ static int vimc_cap_comp_bind(struct device *comp, struct device *master,
>  	strscpy(vdev->name, pdata->entity_name, sizeof(vdev->name));
>  	video_set_drvdata(vdev, &vcap->ved);
>  
> +	dev_dbg(vcap->dev, "vimc-capture is in %s mode", IS_MULTIPLANAR(vcap) ?
> +		"multiplanar" : "singleplanar");

I think if you put the check in the second line it would be better:

IS_MULTIPLANAR(vcap) ? "multiplanar" : "singleplanar"

(this is just a suggestion, feel free to take it or not).

Helen

> +
> +	if (IS_MULTIPLANAR(vcap)) {
> +		vcap->format = fmt_default_mp;
> +		v4l2_fill_pixfmt_mp(&vcap->format.fmt.pix_mp,
> +				vcap->format.fmt.pix_mp.pixelformat,
> +				vcap->format.fmt.pix_mp.width,
> +				vcap->format.fmt.pix_mp.height);
> +	} else {
> +		vcap->format = fmt_default;
> +		v4l2_fill_pixfmt(&vcap->format.fmt.pix,
> +				vcap->format.fmt.pix.pixelformat,
> +				vcap->format.fmt.pix.width,
> +				vcap->format.fmt.pix.height);
> +	}
> +
>  	/* Register the video_device with the v4l2 and the media framework */
>  	ret = video_register_device(vdev, VFL_TYPE_GRABBER, -1);
>  	if (ret) {
> 

[PATCH v2 09/15] media: vimc: cap: Allocate and verify mplanar buffers

[André Almeida]

If the driver is in multiplanar mode, fill the vb2 structures
with the planes sizes and verify it the sizes allocated to the
planes are enough.

Signed-off-by: André Almeida <andrealmeid@collabora.com>
---
Change in v2:
- Use IS_MULTIPLANAR macro

 drivers/media/platform/vimc/vimc-capture.c | 42 ++++++++++++++++++----
 1 file changed, 36 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
index c344d04ed8ea..57bc2b64b093 100644
--- a/drivers/media/platform/vimc/vimc-capture.c
+++ b/drivers/media/platform/vimc/vimc-capture.c
@@ -505,12 +505,28 @@ static int vimc_cap_queue_setup(struct vb2_queue *vq, unsigned int *nbuffers,
 				struct device *alloc_devs[])
 {
 	struct vimc_cap_device *vcap = vb2_get_drv_priv(vq);
+	const struct v4l2_plane_pix_format *plane_fmt =
+		vcap->format.fmt.pix_mp.plane_fmt;
+	unsigned int i;
+
+	if (IS_MULTIPLANAR(vcap)) {
+		for (i = 0; i < *nplanes; i++)
+			if (sizes[i] < plane_fmt[i].sizeimage)
+				return -EINVAL;
+	} else if (*nplanes && sizes[0] < vcap->format.fmt.pix.sizeimage)
+		return -EINVAL;
 
 	if (*nplanes)
-		return sizes[0] < vcap->format.fmt.pix.sizeimage ? -EINVAL : 0;
-	/* We don't support multiplanes for now */
-	*nplanes = 1;
-	sizes[0] = vcap->format.fmt.pix.sizeimage;
+		return 0;
+
+	if (IS_MULTIPLANAR(vcap)) {
+		*nplanes = vcap->format.fmt.pix_mp.num_planes;
+		for (i = 0; i < *nplanes; i++)
+			sizes[i] = plane_fmt[i].sizeimage;
+	} else {
+		*nplanes = 1;
+		sizes[0] = vcap->format.fmt.pix.sizeimage;
+	}
 
 	return 0;
 }
@@ -518,9 +534,23 @@ static int vimc_cap_queue_setup(struct vb2_queue *vq, unsigned int *nbuffers,
 static int vimc_cap_buffer_prepare(struct vb2_buffer *vb)
 {
 	struct vimc_cap_device *vcap = vb2_get_drv_priv(vb->vb2_queue);
-	unsigned long size = vcap->format.fmt.pix.sizeimage;
+	unsigned long size;
+	unsigned int i;
 
-	if (vb2_plane_size(vb, 0) < size) {
+	if (IS_MULTIPLANAR(vcap)) {
+		for (i = 0; i < vb->num_planes; i++) {
+			size = vcap->format.fmt.pix_mp.plane_fmt[i].sizeimage;
+			if (vb2_plane_size(vb, i) < size) {
+				dev_err(vcap->dev,
+					"%s: buffer too small (%lu < %lu)\n",
+					vcap->vdev.name, vb2_plane_size(vb, i),
+					size);
+
+				return -EINVAL;
+			}
+		}
+	} else if (vb2_plane_size(vb, 0) < vcap->format.fmt.pix.sizeimage) {
+		size = vcap->format.fmt.pix.sizeimage;
 		dev_err(vcap->dev, "%s: buffer too small (%lu < %lu)\n",
 			vcap->vdev.name, vb2_plane_size(vb, 0), size);
 		return -EINVAL;
-- 
2.21.0

Re: [PATCH v2 09/15] media: vimc: cap: Allocate and verify mplanar buffers

[Helen Koike]

On 3/27/19 12:17 PM, André Almeida wrote:
> If the driver is in multiplanar mode, fill the vb2 structures
> with the planes sizes and verify it the sizes allocated to the
> planes are enough.
> 
> Signed-off-by: André Almeida <andrealmeid@collabora.com>

Acked-by: Helen Koike <helen.koike@collabora.com>

> ---
> Change in v2:
> - Use IS_MULTIPLANAR macro
> 
>  drivers/media/platform/vimc/vimc-capture.c | 42 ++++++++++++++++++----
>  1 file changed, 36 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
> index c344d04ed8ea..57bc2b64b093 100644
> --- a/drivers/media/platform/vimc/vimc-capture.c
> +++ b/drivers/media/platform/vimc/vimc-capture.c
> @@ -505,12 +505,28 @@ static int vimc_cap_queue_setup(struct vb2_queue *vq, unsigned int *nbuffers,
>  				struct device *alloc_devs[])
>  {
>  	struct vimc_cap_device *vcap = vb2_get_drv_priv(vq);
> +	const struct v4l2_plane_pix_format *plane_fmt =
> +		vcap->format.fmt.pix_mp.plane_fmt;
> +	unsigned int i;
> +
> +	if (IS_MULTIPLANAR(vcap)) {
> +		for (i = 0; i < *nplanes; i++)
> +			if (sizes[i] < plane_fmt[i].sizeimage)
> +				return -EINVAL;
> +	} else if (*nplanes && sizes[0] < vcap->format.fmt.pix.sizeimage)
> +		return -EINVAL;

I thought checkpatch should complain about the unbalanced braces here
but it didn't.

Helen

>  
>  	if (*nplanes)
> -		return sizes[0] < vcap->format.fmt.pix.sizeimage ? -EINVAL : 0;
> -	/* We don't support multiplanes for now */
> -	*nplanes = 1;
> -	sizes[0] = vcap->format.fmt.pix.sizeimage;
> +		return 0;
> +
> +	if (IS_MULTIPLANAR(vcap)) {
> +		*nplanes = vcap->format.fmt.pix_mp.num_planes;
> +		for (i = 0; i < *nplanes; i++)
> +			sizes[i] = plane_fmt[i].sizeimage;
> +	} else {
> +		*nplanes = 1;
> +		sizes[0] = vcap->format.fmt.pix.sizeimage;
> +	}
>  
>  	return 0;
>  }
> @@ -518,9 +534,23 @@ static int vimc_cap_queue_setup(struct vb2_queue *vq, unsigned int *nbuffers,
>  static int vimc_cap_buffer_prepare(struct vb2_buffer *vb)
>  {
>  	struct vimc_cap_device *vcap = vb2_get_drv_priv(vb->vb2_queue);
> -	unsigned long size = vcap->format.fmt.pix.sizeimage;
> +	unsigned long size;
> +	unsigned int i;
>  
> -	if (vb2_plane_size(vb, 0) < size) {
> +	if (IS_MULTIPLANAR(vcap)) {
> +		for (i = 0; i < vb->num_planes; i++) {
> +			size = vcap->format.fmt.pix_mp.plane_fmt[i].sizeimage;
> +			if (vb2_plane_size(vb, i) < size) {
> +				dev_err(vcap->dev,
> +					"%s: buffer too small (%lu < %lu)\n",
> +					vcap->vdev.name, vb2_plane_size(vb, i),
> +					size);
> +
> +				return -EINVAL;
> +			}
> +		}
> +	} else if (vb2_plane_size(vb, 0) < vcap->format.fmt.pix.sizeimage) {
> +		size = vcap->format.fmt.pix.sizeimage;
>  		dev_err(vcap->dev, "%s: buffer too small (%lu < %lu)\n",
>  			vcap->vdev.name, vb2_plane_size(vb, 0), size);
>  		return -EINVAL;
> 

[PATCH v2 10/15] media: vimc: Propagate multiplanar state in the stream

[André Almeida]

Add a multiplanar flag in vimc_stream in order to propagate the
state (singleplanar/multiplanar) to subdevices at the stream.

Signed-off-by: André Almeida <andrealmeid@collabora.com>
---
Change in v2:
- New commit

 drivers/media/platform/vimc/vimc-capture.c  | 7 +++++--
 drivers/media/platform/vimc/vimc-streamer.h | 3 +++
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
index 57bc2b64b093..3c93fbd51629 100644
--- a/drivers/media/platform/vimc/vimc-capture.c
+++ b/drivers/media/platform/vimc/vimc-capture.c
@@ -454,12 +454,15 @@ static int vimc_cap_start_streaming(struct vb2_queue *vq, unsigned int count)
 		return ret;
 	}
 
-	if (IS_MULTIPLANAR(vcap))
+	if (IS_MULTIPLANAR(vcap)) {
 		vcap->stream.producer_pixfmt =
 			vcap->format.fmt.pix_mp.pixelformat;
-	else
+		vcap->stream.multiplanar = true;
+	} else {
 		vcap->stream.producer_pixfmt =
 			vcap->format.fmt.pix.pixelformat;
+		vcap->stream.multiplanar = false;
+	}
 
 	ret = vimc_streamer_s_stream(&vcap->stream, &vcap->ved, 1);
 	if (ret) {
diff --git a/drivers/media/platform/vimc/vimc-streamer.h b/drivers/media/platform/vimc/vimc-streamer.h
index 2b3667408794..4878e0b72ea7 100644
--- a/drivers/media/platform/vimc/vimc-streamer.h
+++ b/drivers/media/platform/vimc/vimc-streamer.h
@@ -25,6 +25,8 @@
  * processed in the pipeline.
  * @pipe_size:		size of @ved_pipeline
  * @kthread:		thread that generates the frames of the stream.
+ * @multiplanar:	sets if the stream is running in multiplanar or
+ * singleplanar format mode
  * @producer_pixfmt:	the pixel format requested from the pipeline. This must
  * be set just before calling vimc_streamer_s_stream(ent, 1). This value is
  * propagated up to the source of the base image (usually a sensor node) and
@@ -40,6 +42,7 @@ struct vimc_stream {
 	struct vimc_ent_device *ved_pipeline[VIMC_STREAMER_PIPELINE_MAX_SIZE];
 	unsigned int pipe_size;
 	struct task_struct *kthread;
+	bool multiplanar;
 	u32 producer_pixfmt;
 };
 
-- 
2.21.0

Re: [PATCH v2 10/15] media: vimc: Propagate multiplanar state in the stream

[Helen Koike]

On 3/27/19 12:17 PM, André Almeida wrote:
> Add a multiplanar flag in vimc_stream in order to propagate the
> state (singleplanar/multiplanar) to subdevices at the stream.
> 
> Signed-off-by: André Almeida <andrealmeid@collabora.com>
> ---
> Change in v2:
> - New commit
> 
>  drivers/media/platform/vimc/vimc-capture.c  | 7 +++++--
>  drivers/media/platform/vimc/vimc-streamer.h | 3 +++
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
> index 57bc2b64b093..3c93fbd51629 100644
> --- a/drivers/media/platform/vimc/vimc-capture.c
> +++ b/drivers/media/platform/vimc/vimc-capture.c
> @@ -454,12 +454,15 @@ static int vimc_cap_start_streaming(struct vb2_queue *vq, unsigned int count)
>  		return ret;
>  	}
>  
> -	if (IS_MULTIPLANAR(vcap))
> +	if (IS_MULTIPLANAR(vcap)) {
>  		vcap->stream.producer_pixfmt =
>  			vcap->format.fmt.pix_mp.pixelformat;
> -	else
> +		vcap->stream.multiplanar = true;
> +	} else {
>  		vcap->stream.producer_pixfmt =
>  			vcap->format.fmt.pix.pixelformat;
> +		vcap->stream.multiplanar = false;
> +	}

I believe you could have just:

vcap->stream.multiplanar = IS_MULTIPLANAR(vcap);

>  
>  	ret = vimc_streamer_s_stream(&vcap->stream, &vcap->ved, 1);
>  	if (ret) {
> diff --git a/drivers/media/platform/vimc/vimc-streamer.h b/drivers/media/platform/vimc/vimc-streamer.h
> index 2b3667408794..4878e0b72ea7 100644
> --- a/drivers/media/platform/vimc/vimc-streamer.h
> +++ b/drivers/media/platform/vimc/vimc-streamer.h
> @@ -25,6 +25,8 @@
>   * processed in the pipeline.
>   * @pipe_size:		size of @ved_pipeline
>   * @kthread:		thread that generates the frames of the stream.
> + * @multiplanar:	sets if the stream is running in multiplanar or
> + * singleplanar format mode
>   * @producer_pixfmt:	the pixel format requested from the pipeline. This must
>   * be set just before calling vimc_streamer_s_stream(ent, 1). This value is
>   * propagated up to the source of the base image (usually a sensor node) and
> @@ -40,6 +42,7 @@ struct vimc_stream {
>  	struct vimc_ent_device *ved_pipeline[VIMC_STREAMER_PIPELINE_MAX_SIZE];
>  	unsigned int pipe_size;
>  	struct task_struct *kthread;
> +	bool multiplanar;
>  	u32 producer_pixfmt;
>  };
>  
> 

[PATCH v2 11/15] media: vimc: Add and use new struct vimc_frame

[André Almeida]

Struct vimc_frame is intended to hold metadata about a frame,
such as memory address of a plane, number of planes and size
of each plane, to better integrated with the multiplanar operations.
The struct can be also used with singleplanar formats, making the
implementation of frame manipulation generic for both type of
formats.

vimc_fill_frame function fills a vimc_frame structure given a
pixelformat, height and width. This is done once to avoid recalculations
and provide enough information to subdevices work with
the frame.

Change the return and argument type of process_frame from void* to
vimc_frame*. Change the frame in subdevices structs from u8* to vimc_frame.

Signed-off-by: André Almeida <andrealmeid@collabora.com>
---
Change in v2:
- Fix alignment issues

 drivers/media/platform/vimc/vimc-capture.c  |  6 +--
 drivers/media/platform/vimc/vimc-common.c   | 36 +++++++++++++++
 drivers/media/platform/vimc/vimc-common.h   | 49 +++++++++++++++++++--
 drivers/media/platform/vimc/vimc-debayer.c  | 34 +++++++-------
 drivers/media/platform/vimc/vimc-scaler.c   | 26 ++++++-----
 drivers/media/platform/vimc/vimc-sensor.c   | 18 ++++----
 drivers/media/platform/vimc/vimc-streamer.c |  2 +-
 7 files changed, 127 insertions(+), 44 deletions(-)

diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
index 3c93fbd51629..fae42b778711 100644
--- a/drivers/media/platform/vimc/vimc-capture.c
+++ b/drivers/media/platform/vimc/vimc-capture.c
@@ -600,8 +600,8 @@ static void vimc_cap_comp_unbind(struct device *comp, struct device *master,
 	video_unregister_device(&vcap->vdev);
 }
 
-static void *vimc_cap_process_frame(struct vimc_ent_device *ved,
-				    const void *frame)
+static struct vimc_frame *vimc_cap_process_frame(struct vimc_ent_device *ved,
+						 const struct vimc_frame *frame)
 {
 	struct vimc_cap_device *vcap = container_of(ved, struct vimc_cap_device,
 						    ved);
@@ -630,7 +630,7 @@ static void *vimc_cap_process_frame(struct vimc_ent_device *ved,
 
 	vbuf = vb2_plane_vaddr(&vimc_buf->vb2.vb2_buf, 0);
 
-	memcpy(vbuf, frame, vcap->format.fmt.pix.sizeimage);
+	memcpy(vbuf, frame->plane_addr[0], vcap->format.fmt.pix.sizeimage);
 
 	/* Set it as ready */
 	vb2_set_plane_payload(&vimc_buf->vb2.vb2_buf, 0,
diff --git a/drivers/media/platform/vimc/vimc-common.c b/drivers/media/platform/vimc/vimc-common.c
index 8d314b5dbb73..26868a753c53 100644
--- a/drivers/media/platform/vimc/vimc-common.c
+++ b/drivers/media/platform/vimc/vimc-common.c
@@ -380,6 +380,42 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved,
 }
 EXPORT_SYMBOL_GPL(vimc_ent_sd_register);
 
+void vimc_fill_frame(struct vimc_frame *frame, u32 pixelformat, u32 width,
+		     u32 height, bool multiplanar)
+{
+	unsigned int i;
+	const struct v4l2_format_info *pix_info;
+
+	pix_info = v4l2_format_info(pixelformat);
+	frame->pixelformat = pixelformat;
+
+	if (multiplanar) {
+		struct v4l2_pix_format_mplane pix_fmt_mp;
+
+		v4l2_fill_pixfmt_mp(&pix_fmt_mp, pixelformat, width, height);
+
+		frame->num_planes = pix_fmt_mp.num_planes;
+		for (i = 0; i < pix_fmt_mp.num_planes; i++) {
+			frame->sizeimage[i] = pix_fmt_mp.plane_fmt[i].sizeimage;
+			frame->bytesperline[i] =
+				pix_fmt_mp.plane_fmt[i].bytesperline;
+			frame->bpp[i] = pix_info->bpp[i];
+			frame->plane_addr[i] = NULL;
+		}
+	} else {
+		struct v4l2_pix_format pix_fmt;
+
+		v4l2_fill_pixfmt(&pix_fmt, pixelformat, width, height);
+
+		frame->num_planes = 1;
+		frame->sizeimage[0] = pix_fmt.sizeimage;
+		frame->bytesperline[0] = pix_fmt.bytesperline;
+		frame->bpp[0] = pix_info->bpp[0];
+		frame->plane_addr[0] = NULL;
+	}
+}
+EXPORT_SYMBOL_GPL(vimc_fill_frame);
+
 void vimc_ent_sd_unregister(struct vimc_ent_device *ved, struct v4l2_subdev *sd)
 {
 	media_entity_cleanup(ved->ent);
diff --git a/drivers/media/platform/vimc/vimc-common.h b/drivers/media/platform/vimc/vimc-common.h
index 142ddfa69b3d..7d167dd696ca 100644
--- a/drivers/media/platform/vimc/vimc-common.h
+++ b/drivers/media/platform/vimc/vimc-common.h
@@ -21,6 +21,7 @@
 #include <linux/slab.h>
 #include <media/media-device.h>
 #include <media/v4l2-device.h>
+#include <media/tpg/v4l2-tpg.h>
 
 #include "vimc-streamer.h"
 
@@ -79,6 +80,31 @@ struct vimc_platform_data {
 	char entity_name[32];
 };
 
+/**
+ * struct vimc_frame - metadata about frame components
+ *
+ * @pixelformat:	fourcc pixelformat code
+ * @plane_addr:		pointer to kernel address of the plane
+ * @num_planes:		number of valid planes on a frame
+ * @sizeimage:		size in bytes of a plane
+ * @bytesperline:	number of bytes per line of a plane
+ * @bpp:		number of bytes per pixel of a plane
+ *
+ * This struct helps subdevices to get information about the frame on
+ * multiplanar formats. If a singleplanar format is used, only the first
+ * index of each array is used and num_planes is set to 1, so the
+ * implementation is generic and the code will work for both formats.
+ */
+
+struct vimc_frame {
+	u32 pixelformat;
+	u32 sizeimage[TPG_MAX_PLANES];
+	u32 bytesperline[TPG_MAX_PLANES];
+	u8 *plane_addr[TPG_MAX_PLANES];
+	u8 bpp[TPG_MAX_PLANES];
+	u8 num_planes;
+};
+
 /**
  * struct vimc_ent_device - core struct that represents a node in the topology
  *
@@ -101,10 +127,10 @@ struct vimc_ent_device {
 	struct media_entity *ent;
 	struct media_pad *pads;
 	struct vimc_stream *stream;
-	void * (*process_frame)(struct vimc_ent_device *ved,
-				const void *frame);
+	struct vimc_frame * (*process_frame)(struct vimc_ent_device *ved,
+				const struct vimc_frame *frame);
 	void (*vdev_get_format)(struct vimc_ent_device *ved,
-			      struct v4l2_pix_format *fmt);
+				struct v4l2_pix_format *fmt);
 };
 
 /**
@@ -206,4 +232,21 @@ void vimc_ent_sd_unregister(struct vimc_ent_device *ved,
  */
 int vimc_link_validate(struct media_link *link);
 
+/**
+ * vimc_fill_frame - fills struct vimc_frame
+ *
+ * @frame: pointer to the frame to be filled
+ * @pixelformat: pixelformat fourcc code
+ * @width: width of the image
+ * @height: height of the image
+ * @multiplanar: flag to define if the stream is in singleplanar/multiplanar
+ * mode
+ *
+ * This function fills the fields of vimc_frame in order to subdevs have
+ * information about the frame being processed, works both for single
+ * and multiplanar pixel formats.
+ */
+void vimc_fill_frame(struct vimc_frame *frame, u32 pixelformat, u32 width,
+		     u32 height, bool multiplanar);
+
 #endif
diff --git a/drivers/media/platform/vimc/vimc-debayer.c b/drivers/media/platform/vimc/vimc-debayer.c
index 613b7abbe06e..c2fb1fab7fe9 100644
--- a/drivers/media/platform/vimc/vimc-debayer.c
+++ b/drivers/media/platform/vimc/vimc-debayer.c
@@ -62,7 +62,7 @@ struct vimc_deb_device {
 	void (*set_rgb_src)(struct vimc_deb_device *vdeb, unsigned int lin,
 			    unsigned int col, unsigned int rgb[3]);
 	/* Values calculated when the stream starts */
-	u8 *src_frame;
+	struct vimc_frame src_frame;
 	const struct vimc_deb_pix_map *sink_pix_map;
 	unsigned int sink_bpp;
 };
@@ -325,7 +325,7 @@ static void vimc_deb_set_rgb_pix_rgb24(struct vimc_deb_device *vdeb,
 
 	index = VIMC_FRAME_INDEX(lin, col, vdeb->sink_fmt.width, 3);
 	for (i = 0; i < 3; i++)
-		vdeb->src_frame[index + i] = rgb[i];
+		vdeb->src_frame.plane_addr[0][index + i] = rgb[i];
 }
 
 static int vimc_deb_s_stream(struct v4l2_subdev *sd, int enable)
@@ -335,7 +335,6 @@ static int vimc_deb_s_stream(struct v4l2_subdev *sd, int enable)
 	if (enable) {
 		u32 src_pixelformat = vdeb->ved.stream->producer_pixfmt;
 		const struct v4l2_format_info *pix_info;
-		unsigned int frame_size;
 
 		/* We only support translating bayer to RGB24 */
 		if (src_pixelformat != V4L2_PIX_FMT_RGB24) {
@@ -354,9 +353,9 @@ static int vimc_deb_s_stream(struct v4l2_subdev *sd, int enable)
 			vdeb->sink_pix_map->pixelformat;
 
 		/* Calculate frame_size of the source */
-		pix_info = v4l2_format_info(src_pixelformat);
-		frame_size = vdeb->sink_fmt.width * vdeb->sink_fmt.height *
-			     pix_info->bpp[0];
+		vimc_fill_frame(&vdeb->src_frame, src_pixelformat,
+				vdeb->sink_fmt.width, vdeb->sink_fmt.height,
+				vdeb->ved.stream->producer_pixfmt);
 
 		/* Get bpp from the sink */
 		pix_info = v4l2_format_info(vdeb->sink_pix_map->pixelformat);
@@ -366,16 +365,18 @@ static int vimc_deb_s_stream(struct v4l2_subdev *sd, int enable)
 		 * Allocate the frame buffer. Use vmalloc to be able to
 		 * allocate a large amount of memory
 		 */
-		vdeb->src_frame = vmalloc(frame_size);
-		if (!vdeb->src_frame)
+		vdeb->src_frame.plane_addr[0] =
+					vmalloc(vdeb->src_frame.sizeimage[0]);
+		if (!vdeb->src_frame.plane_addr[0])
 			return -ENOMEM;
 
+
 	} else {
-		if (!vdeb->src_frame)
+		if (!vdeb->src_frame.plane_addr[0])
 			return 0;
 
-		vfree(vdeb->src_frame);
-		vdeb->src_frame = NULL;
+		vfree(vdeb->src_frame.plane_addr[0]);
+		vdeb->src_frame.plane_addr[0] = NULL;
 	}
 
 	return 0;
@@ -487,8 +488,8 @@ static void vimc_deb_calc_rgb_sink(struct vimc_deb_device *vdeb,
 	}
 }
 
-static void *vimc_deb_process_frame(struct vimc_ent_device *ved,
-				    const void *sink_frame)
+static struct vimc_frame *vimc_deb_process_frame(struct vimc_ent_device *ved,
+					const struct vimc_frame *sink_frame)
 {
 	struct vimc_deb_device *vdeb = container_of(ved, struct vimc_deb_device,
 						    ved);
@@ -496,16 +497,17 @@ static void *vimc_deb_process_frame(struct vimc_ent_device *ved,
 	unsigned int i, j;
 
 	/* If the stream in this node is not active, just return */
-	if (!vdeb->src_frame)
+	if (!vdeb->src_frame.plane_addr[0])
 		return ERR_PTR(-EINVAL);
 
 	for (i = 0; i < vdeb->sink_fmt.height; i++)
 		for (j = 0; j < vdeb->sink_fmt.width; j++) {
-			vimc_deb_calc_rgb_sink(vdeb, sink_frame, i, j, rgb);
+			vimc_deb_calc_rgb_sink(vdeb, sink_frame->plane_addr[0],
+					       i, j, rgb);
 			vdeb->set_rgb_src(vdeb, i, j, rgb);
 		}
 
-	return vdeb->src_frame;
+	return &vdeb->src_frame;
 
 }
 
diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c
index 8544c745c6e6..92c639f1817e 100644
--- a/drivers/media/platform/vimc/vimc-scaler.c
+++ b/drivers/media/platform/vimc/vimc-scaler.c
@@ -50,7 +50,7 @@ struct vimc_sca_device {
 	 */
 	struct v4l2_mbus_framefmt sink_fmt;
 	/* Values calculated when the stream starts */
-	u8 *src_frame;
+	struct vimc_frame src_frame;
 	unsigned int src_line_size;
 	unsigned int bpp;
 };
@@ -234,16 +234,17 @@ static int vimc_sca_s_stream(struct v4l2_subdev *sd, int enable)
 		/* Allocate the frame buffer. Use vmalloc to be able to
 		 * allocate a large amount of memory
 		 */
-		vsca->src_frame = vmalloc(frame_size);
-		if (!vsca->src_frame)
+		vsca->src_frame.plane_addr[0] = vmalloc(frame_size);
+		vsca->src_frame.sizeimage[0] = frame_size;
+		if (!vsca->src_frame.plane_addr[0])
 			return -ENOMEM;
 
 	} else {
-		if (!vsca->src_frame)
+		if (!vsca->src_frame.plane_addr[0])
 			return 0;
 
-		vfree(vsca->src_frame);
-		vsca->src_frame = NULL;
+		vfree(vsca->src_frame.plane_addr[0]);
+		vsca->src_frame.plane_addr[0] = NULL;
 	}
 
 	return 0;
@@ -306,8 +307,9 @@ static void vimc_sca_scale_pix(const struct vimc_sca_device *const vsca,
 				vsca->sd.name, index + j);
 
 			/* copy the pixel to the position index + j */
-			vimc_sca_fill_pix(&vsca->src_frame[index + j],
-					  pixel, vsca->bpp);
+			vimc_sca_fill_pix(
+				&vsca->src_frame.plane_addr[0][index + j],
+				pixel, vsca->bpp);
 		}
 
 		/* move the index to the next line */
@@ -327,8 +329,8 @@ static void vimc_sca_fill_src_frame(const struct vimc_sca_device *const vsca,
 			vimc_sca_scale_pix(vsca, i, j, sink_frame);
 }
 
-static void *vimc_sca_process_frame(struct vimc_ent_device *ved,
-				    const void *sink_frame)
+static struct vimc_frame *vimc_sca_process_frame(struct vimc_ent_device *ved,
+				    const struct vimc_frame *sink_frame)
 {
 	struct vimc_sca_device *vsca = container_of(ved, struct vimc_sca_device,
 						    ved);
@@ -337,9 +339,9 @@ static void *vimc_sca_process_frame(struct vimc_ent_device *ved,
 	if (!ved->stream)
 		return ERR_PTR(-EINVAL);
 
-	vimc_sca_fill_src_frame(vsca, sink_frame);
+	vimc_sca_fill_src_frame(vsca, sink_frame->plane_addr[0]);
 
-	return vsca->src_frame;
+	return &vsca->src_frame;
 };
 
 static void vimc_sca_release(struct v4l2_subdev *sd)
diff --git a/drivers/media/platform/vimc/vimc-sensor.c b/drivers/media/platform/vimc/vimc-sensor.c
index 1b2b75b27952..3495a3f3dd60 100644
--- a/drivers/media/platform/vimc/vimc-sensor.c
+++ b/drivers/media/platform/vimc/vimc-sensor.c
@@ -36,7 +36,7 @@ struct vimc_sen_device {
 	struct device *dev;
 	struct tpg_data tpg;
 	struct task_struct *kthread_sen;
-	u8 *frame;
+	struct vimc_frame frame;
 	/* The active format */
 	struct v4l2_mbus_framefmt mbus_format;
 	struct v4l2_ctrl_handler hdl;
@@ -177,14 +177,14 @@ static const struct v4l2_subdev_pad_ops vimc_sen_pad_ops = {
 	.set_fmt		= vimc_sen_set_fmt,
 };
 
-static void *vimc_sen_process_frame(struct vimc_ent_device *ved,
-				    const void *sink_frame)
+static struct vimc_frame *vimc_sen_process_frame(struct vimc_ent_device *ved,
+				    const struct vimc_frame *sink_frame)
 {
 	struct vimc_sen_device *vsen = container_of(ved, struct vimc_sen_device,
 						    ved);
 
-	tpg_fill_plane_buffer(&vsen->tpg, 0, 0, vsen->frame);
-	return vsen->frame;
+	tpg_fill_plane_buffer(&vsen->tpg, 0, 0, vsen->frame.plane_addr[0]);
+	return &vsen->frame;
 }
 
 static int vimc_sen_s_stream(struct v4l2_subdev *sd, int enable)
@@ -206,8 +206,8 @@ static int vimc_sen_s_stream(struct v4l2_subdev *sd, int enable)
 		 * Allocate the frame buffer. Use vmalloc to be able to
 		 * allocate a large amount of memory
 		 */
-		vsen->frame = vmalloc(frame_size);
-		if (!vsen->frame)
+		vsen->frame.plane_addr[0] = vmalloc(frame_size);
+		if (!vsen->frame.plane_addr[0])
 			return -ENOMEM;
 
 		/* configure the test pattern generator */
@@ -215,8 +215,8 @@ static int vimc_sen_s_stream(struct v4l2_subdev *sd, int enable)
 
 	} else {
 
-		vfree(vsen->frame);
-		vsen->frame = NULL;
+		vfree(vsen->frame.plane_addr[0]);
+		vsen->frame.plane_addr[0] = NULL;
 		return 0;
 	}
 
diff --git a/drivers/media/platform/vimc/vimc-streamer.c b/drivers/media/platform/vimc/vimc-streamer.c
index 26b674259489..216ac8a83ba5 100644
--- a/drivers/media/platform/vimc/vimc-streamer.c
+++ b/drivers/media/platform/vimc/vimc-streamer.c
@@ -125,7 +125,7 @@ static int vimc_streamer_pipeline_init(struct vimc_stream *stream,
 static int vimc_streamer_thread(void *data)
 {
 	struct vimc_stream *stream = data;
-	u8 *frame = NULL;
+	struct vimc_frame *frame = NULL;
 	int i;
 
 	set_freezable();
-- 
2.21.0

Re: [PATCH v2 11/15] media: vimc: Add and use new struct vimc_frame

[Helen Koike]

Hi André,

Thanks for the patch, just some comments bellow.

On 3/27/19 12:17 PM, André Almeida wrote:
> Struct vimc_frame is intended to hold metadata about a frame,
> such as memory address of a plane, number of planes and size
> of each plane, to better integrated with the multiplanar operations.
> The struct can be also used with singleplanar formats, making the
> implementation of frame manipulation generic for both type of
> formats.
> 
> vimc_fill_frame function fills a vimc_frame structure given a
> pixelformat, height and width. This is done once to avoid recalculations
> and provide enough information to subdevices work with
> the frame.
> 
> Change the return and argument type of process_frame from void* to
> vimc_frame*. Change the frame in subdevices structs from u8* to vimc_frame.
> 
> Signed-off-by: André Almeida <andrealmeid@collabora.com>
> ---
> Change in v2:
> - Fix alignment issues
> 
>  drivers/media/platform/vimc/vimc-capture.c  |  6 +--
>  drivers/media/platform/vimc/vimc-common.c   | 36 +++++++++++++++
>  drivers/media/platform/vimc/vimc-common.h   | 49 +++++++++++++++++++--
>  drivers/media/platform/vimc/vimc-debayer.c  | 34 +++++++-------
>  drivers/media/platform/vimc/vimc-scaler.c   | 26 ++++++-----
>  drivers/media/platform/vimc/vimc-sensor.c   | 18 ++++----
>  drivers/media/platform/vimc/vimc-streamer.c |  2 +-
>  7 files changed, 127 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
> index 3c93fbd51629..fae42b778711 100644
> --- a/drivers/media/platform/vimc/vimc-capture.c
> +++ b/drivers/media/platform/vimc/vimc-capture.c
> @@ -600,8 +600,8 @@ static void vimc_cap_comp_unbind(struct device *comp, struct device *master,
>  	video_unregister_device(&vcap->vdev);
>  }
>  
> -static void *vimc_cap_process_frame(struct vimc_ent_device *ved,
> -				    const void *frame)
> +static struct vimc_frame *vimc_cap_process_frame(struct vimc_ent_device *ved,
> +						 const struct vimc_frame *frame)
>  {
>  	struct vimc_cap_device *vcap = container_of(ved, struct vimc_cap_device,
>  						    ved);
> @@ -630,7 +630,7 @@ static void *vimc_cap_process_frame(struct vimc_ent_device *ved,
>  
>  	vbuf = vb2_plane_vaddr(&vimc_buf->vb2.vb2_buf, 0);
>  
> -	memcpy(vbuf, frame, vcap->format.fmt.pix.sizeimage);
> +	memcpy(vbuf, frame->plane_addr[0], vcap->format.fmt.pix.sizeimage);
>  
>  	/* Set it as ready */
>  	vb2_set_plane_payload(&vimc_buf->vb2.vb2_buf, 0,
> diff --git a/drivers/media/platform/vimc/vimc-common.c b/drivers/media/platform/vimc/vimc-common.c
> index 8d314b5dbb73..26868a753c53 100644
> --- a/drivers/media/platform/vimc/vimc-common.c
> +++ b/drivers/media/platform/vimc/vimc-common.c
> @@ -380,6 +380,42 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved,
>  }
>  EXPORT_SYMBOL_GPL(vimc_ent_sd_register);
>  
> +void vimc_fill_frame(struct vimc_frame *frame, u32 pixelformat, u32 width,
> +		     u32 height, bool multiplanar)
> +{
> +	unsigned int i;
> +	const struct v4l2_format_info *pix_info;
> +
> +	pix_info = v4l2_format_info(pixelformat);
> +	frame->pixelformat = pixelformat;
> +
> +	if (multiplanar) {
> +		struct v4l2_pix_format_mplane pix_fmt_mp;
> +
> +		v4l2_fill_pixfmt_mp(&pix_fmt_mp, pixelformat, width, height);
> +
> +		frame->num_planes = pix_fmt_mp.num_planes;
> +		for (i = 0; i < pix_fmt_mp.num_planes; i++) {
> +			frame->sizeimage[i] = pix_fmt_mp.plane_fmt[i].sizeimage;
> +			frame->bytesperline[i] =
> +				pix_fmt_mp.plane_fmt[i].bytesperline;
> +			frame->bpp[i] = pix_info->bpp[i];
> +			frame->plane_addr[i] = NULL;
> +		}
> +	} else {
> +		struct v4l2_pix_format pix_fmt;
> +
> +		v4l2_fill_pixfmt(&pix_fmt, pixelformat, width, height);
> +
> +		frame->num_planes = 1;
> +		frame->sizeimage[0] = pix_fmt.sizeimage;
> +		frame->bytesperline[0] = pix_fmt.bytesperline;
> +		frame->bpp[0] = pix_info->bpp[0];
> +		frame->plane_addr[0] = NULL;
> +	}
> +}
> +EXPORT_SYMBOL_GPL(vimc_fill_frame);
> +
>  void vimc_ent_sd_unregister(struct vimc_ent_device *ved, struct v4l2_subdev *sd)
>  {
>  	media_entity_cleanup(ved->ent);
> diff --git a/drivers/media/platform/vimc/vimc-common.h b/drivers/media/platform/vimc/vimc-common.h
> index 142ddfa69b3d..7d167dd696ca 100644
> --- a/drivers/media/platform/vimc/vimc-common.h
> +++ b/drivers/media/platform/vimc/vimc-common.h
> @@ -21,6 +21,7 @@
>  #include <linux/slab.h>
>  #include <media/media-device.h>
>  #include <media/v4l2-device.h>
> +#include <media/tpg/v4l2-tpg.h>
>  
>  #include "vimc-streamer.h"
>  
> @@ -79,6 +80,31 @@ struct vimc_platform_data {
>  	char entity_name[32];
>  };
>  
> +/**
> + * struct vimc_frame - metadata about frame components
> + *
> + * @pixelformat:	fourcc pixelformat code
> + * @plane_addr:		pointer to kernel address of the plane
> + * @num_planes:		number of valid planes on a frame
> + * @sizeimage:		size in bytes of a plane
> + * @bytesperline:	number of bytes per line of a plane
> + * @bpp:		number of bytes per pixel of a plane

Please, re-order the variable docs in the same order declared in the struct.

> + *
> + * This struct helps subdevices to get information about the frame on
> + * multiplanar formats. If a singleplanar format is used, only the first
> + * index of each array is used and num_planes is set to 1, so the
> + * implementation is generic and the code will work for both formats.
> + */
> +
> +struct vimc_frame {
> +	u32 pixelformat;
> +	u32 sizeimage[TPG_MAX_PLANES];
> +	u32 bytesperline[TPG_MAX_PLANES];
> +	u8 *plane_addr[TPG_MAX_PLANES];
> +	u8 bpp[TPG_MAX_PLANES];

any particular reason not to use unsigned int here?

> +	u8 num_planes;

same here.

Usually we use u8, u16, u32, s8, s16, etc, if we have some specific
requirement to keep the data size (because of some API, protocol, or if
the hardware requires it).

The main reason, afaik, is that int is more efficient for the processor.
In some architectures the cpu doesn't know how to deal with a byte
directly, just with a word, so if you use u8, it means the processor
will read a word and then do a mask, making it less efficient (and
sometimes making the code bigger if done by the compiler).

Another thing that just occurred to me, instead of adding those fields
in vimc_frame, what do you think of having just:

struct vimc_frame {
	struct v4l2_format fmt;
	u8 *plane_addr[TPG_MAX_PLANES];
};

So you wouldn't need the vimc_fil_frame.

You could also just work with multiplanar, and convert to single planar
in the capture:

struct vimc_frame {
	struct v4l2_pix_format_mplane fmt;
	u8 *plane_addr[TPG_MAX_PLANES];
};

So the subdevices can just work with multiplanar (they doesn't need to
know how to deal with single planar, just multiplanar with num_plane=1),
and the capture can make the conversion if required.

Helen

> +};
> +
>  /**
>   * struct vimc_ent_device - core struct that represents a node in the topology
>   *
> @@ -101,10 +127,10 @@ struct vimc_ent_device {
>  	struct media_entity *ent;
>  	struct media_pad *pads;
>  	struct vimc_stream *stream;
> -	void * (*process_frame)(struct vimc_ent_device *ved,
> -				const void *frame);
> +	struct vimc_frame * (*process_frame)(struct vimc_ent_device *ved,
> +				const struct vimc_frame *frame);
>  	void (*vdev_get_format)(struct vimc_ent_device *ved,
> -			      struct v4l2_pix_format *fmt);
> +				struct v4l2_pix_format *fmt);
>  };
>  
>  /**
> @@ -206,4 +232,21 @@ void vimc_ent_sd_unregister(struct vimc_ent_device *ved,
>   */
>  int vimc_link_validate(struct media_link *link);
>  
> +/**
> + * vimc_fill_frame - fills struct vimc_frame
> + *
> + * @frame: pointer to the frame to be filled
> + * @pixelformat: pixelformat fourcc code
> + * @width: width of the image
> + * @height: height of the image
> + * @multiplanar: flag to define if the stream is in singleplanar/multiplanar
> + * mode
> + *
> + * This function fills the fields of vimc_frame in order to subdevs have
> + * information about the frame being processed, works both for single
> + * and multiplanar pixel formats.
> + */
> +void vimc_fill_frame(struct vimc_frame *frame, u32 pixelformat, u32 width,
> +		     u32 height, bool multiplanar);
> +
>  #endif
> diff --git a/drivers/media/platform/vimc/vimc-debayer.c b/drivers/media/platform/vimc/vimc-debayer.c
> index 613b7abbe06e..c2fb1fab7fe9 100644
> --- a/drivers/media/platform/vimc/vimc-debayer.c
> +++ b/drivers/media/platform/vimc/vimc-debayer.c
> @@ -62,7 +62,7 @@ struct vimc_deb_device {
>  	void (*set_rgb_src)(struct vimc_deb_device *vdeb, unsigned int lin,
>  			    unsigned int col, unsigned int rgb[3]);
>  	/* Values calculated when the stream starts */
> -	u8 *src_frame;
> +	struct vimc_frame src_frame;
>  	const struct vimc_deb_pix_map *sink_pix_map;
>  	unsigned int sink_bpp;
>  };
> @@ -325,7 +325,7 @@ static void vimc_deb_set_rgb_pix_rgb24(struct vimc_deb_device *vdeb,
>  
>  	index = VIMC_FRAME_INDEX(lin, col, vdeb->sink_fmt.width, 3);
>  	for (i = 0; i < 3; i++)
> -		vdeb->src_frame[index + i] = rgb[i];
> +		vdeb->src_frame.plane_addr[0][index + i] = rgb[i];
>  }
>  
>  static int vimc_deb_s_stream(struct v4l2_subdev *sd, int enable)
> @@ -335,7 +335,6 @@ static int vimc_deb_s_stream(struct v4l2_subdev *sd, int enable)
>  	if (enable) {
>  		u32 src_pixelformat = vdeb->ved.stream->producer_pixfmt;
>  		const struct v4l2_format_info *pix_info;
> -		unsigned int frame_size;
>  
>  		/* We only support translating bayer to RGB24 */
>  		if (src_pixelformat != V4L2_PIX_FMT_RGB24) {
> @@ -354,9 +353,9 @@ static int vimc_deb_s_stream(struct v4l2_subdev *sd, int enable)
>  			vdeb->sink_pix_map->pixelformat;
>  
>  		/* Calculate frame_size of the source */
> -		pix_info = v4l2_format_info(src_pixelformat);
> -		frame_size = vdeb->sink_fmt.width * vdeb->sink_fmt.height *
> -			     pix_info->bpp[0];
> +		vimc_fill_frame(&vdeb->src_frame, src_pixelformat,
> +				vdeb->sink_fmt.width, vdeb->sink_fmt.height,
> +				vdeb->ved.stream->producer_pixfmt);
>  
>  		/* Get bpp from the sink */
>  		pix_info = v4l2_format_info(vdeb->sink_pix_map->pixelformat);
> @@ -366,16 +365,18 @@ static int vimc_deb_s_stream(struct v4l2_subdev *sd, int enable)
>  		 * Allocate the frame buffer. Use vmalloc to be able to
>  		 * allocate a large amount of memory
>  		 */
> -		vdeb->src_frame = vmalloc(frame_size);
> -		if (!vdeb->src_frame)
> +		vdeb->src_frame.plane_addr[0] =
> +					vmalloc(vdeb->src_frame.sizeimage[0]);
> +		if (!vdeb->src_frame.plane_addr[0])
>  			return -ENOMEM;
>  
> +
>  	} else {
> -		if (!vdeb->src_frame)
> +		if (!vdeb->src_frame.plane_addr[0])
>  			return 0;
>  
> -		vfree(vdeb->src_frame);
> -		vdeb->src_frame = NULL;
> +		vfree(vdeb->src_frame.plane_addr[0]);
> +		vdeb->src_frame.plane_addr[0] = NULL;
>  	}
>  
>  	return 0;
> @@ -487,8 +488,8 @@ static void vimc_deb_calc_rgb_sink(struct vimc_deb_device *vdeb,
>  	}
>  }
>  
> -static void *vimc_deb_process_frame(struct vimc_ent_device *ved,
> -				    const void *sink_frame)
> +static struct vimc_frame *vimc_deb_process_frame(struct vimc_ent_device *ved,
> +					const struct vimc_frame *sink_frame)
>  {
>  	struct vimc_deb_device *vdeb = container_of(ved, struct vimc_deb_device,
>  						    ved);
> @@ -496,16 +497,17 @@ static void *vimc_deb_process_frame(struct vimc_ent_device *ved,
>  	unsigned int i, j;
>  
>  	/* If the stream in this node is not active, just return */
> -	if (!vdeb->src_frame)
> +	if (!vdeb->src_frame.plane_addr[0])
>  		return ERR_PTR(-EINVAL);
>  
>  	for (i = 0; i < vdeb->sink_fmt.height; i++)
>  		for (j = 0; j < vdeb->sink_fmt.width; j++) {
> -			vimc_deb_calc_rgb_sink(vdeb, sink_frame, i, j, rgb);
> +			vimc_deb_calc_rgb_sink(vdeb, sink_frame->plane_addr[0],
> +					       i, j, rgb);
>  			vdeb->set_rgb_src(vdeb, i, j, rgb);
>  		}
>  
> -	return vdeb->src_frame;
> +	return &vdeb->src_frame;
>  
>  }
>  
> diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c
> index 8544c745c6e6..92c639f1817e 100644
> --- a/drivers/media/platform/vimc/vimc-scaler.c
> +++ b/drivers/media/platform/vimc/vimc-scaler.c
> @@ -50,7 +50,7 @@ struct vimc_sca_device {
>  	 */
>  	struct v4l2_mbus_framefmt sink_fmt;
>  	/* Values calculated when the stream starts */
> -	u8 *src_frame;
> +	struct vimc_frame src_frame;
>  	unsigned int src_line_size;
>  	unsigned int bpp;
>  };
> @@ -234,16 +234,17 @@ static int vimc_sca_s_stream(struct v4l2_subdev *sd, int enable)
>  		/* Allocate the frame buffer. Use vmalloc to be able to
>  		 * allocate a large amount of memory
>  		 */
> -		vsca->src_frame = vmalloc(frame_size);
> -		if (!vsca->src_frame)
> +		vsca->src_frame.plane_addr[0] = vmalloc(frame_size);
> +		vsca->src_frame.sizeimage[0] = frame_size;
> +		if (!vsca->src_frame.plane_addr[0])
>  			return -ENOMEM;
>  
>  	} else {
> -		if (!vsca->src_frame)
> +		if (!vsca->src_frame.plane_addr[0])
>  			return 0;
>  
> -		vfree(vsca->src_frame);
> -		vsca->src_frame = NULL;
> +		vfree(vsca->src_frame.plane_addr[0]);
> +		vsca->src_frame.plane_addr[0] = NULL;
>  	}
>  
>  	return 0;
> @@ -306,8 +307,9 @@ static void vimc_sca_scale_pix(const struct vimc_sca_device *const vsca,
>  				vsca->sd.name, index + j);
>  
>  			/* copy the pixel to the position index + j */
> -			vimc_sca_fill_pix(&vsca->src_frame[index + j],
> -					  pixel, vsca->bpp);
> +			vimc_sca_fill_pix(
> +				&vsca->src_frame.plane_addr[0][index + j],
> +				pixel, vsca->bpp);
>  		}
>  
>  		/* move the index to the next line */
> @@ -327,8 +329,8 @@ static void vimc_sca_fill_src_frame(const struct vimc_sca_device *const vsca,
>  			vimc_sca_scale_pix(vsca, i, j, sink_frame);
>  }
>  
> -static void *vimc_sca_process_frame(struct vimc_ent_device *ved,
> -				    const void *sink_frame)
> +static struct vimc_frame *vimc_sca_process_frame(struct vimc_ent_device *ved,
> +				    const struct vimc_frame *sink_frame)
>  {
>  	struct vimc_sca_device *vsca = container_of(ved, struct vimc_sca_device,
>  						    ved);
> @@ -337,9 +339,9 @@ static void *vimc_sca_process_frame(struct vimc_ent_device *ved,
>  	if (!ved->stream)
>  		return ERR_PTR(-EINVAL);
>  
> -	vimc_sca_fill_src_frame(vsca, sink_frame);
> +	vimc_sca_fill_src_frame(vsca, sink_frame->plane_addr[0]);
>  
> -	return vsca->src_frame;
> +	return &vsca->src_frame;
>  };
>  
>  static void vimc_sca_release(struct v4l2_subdev *sd)
> diff --git a/drivers/media/platform/vimc/vimc-sensor.c b/drivers/media/platform/vimc/vimc-sensor.c
> index 1b2b75b27952..3495a3f3dd60 100644
> --- a/drivers/media/platform/vimc/vimc-sensor.c
> +++ b/drivers/media/platform/vimc/vimc-sensor.c
> @@ -36,7 +36,7 @@ struct vimc_sen_device {
>  	struct device *dev;
>  	struct tpg_data tpg;
>  	struct task_struct *kthread_sen;
> -	u8 *frame;
> +	struct vimc_frame frame;
>  	/* The active format */
>  	struct v4l2_mbus_framefmt mbus_format;
>  	struct v4l2_ctrl_handler hdl;
> @@ -177,14 +177,14 @@ static const struct v4l2_subdev_pad_ops vimc_sen_pad_ops = {
>  	.set_fmt		= vimc_sen_set_fmt,
>  };
>  
> -static void *vimc_sen_process_frame(struct vimc_ent_device *ved,
> -				    const void *sink_frame)
> +static struct vimc_frame *vimc_sen_process_frame(struct vimc_ent_device *ved,
> +				    const struct vimc_frame *sink_frame)
>  {
>  	struct vimc_sen_device *vsen = container_of(ved, struct vimc_sen_device,
>  						    ved);
>  
> -	tpg_fill_plane_buffer(&vsen->tpg, 0, 0, vsen->frame);
> -	return vsen->frame;
> +	tpg_fill_plane_buffer(&vsen->tpg, 0, 0, vsen->frame.plane_addr[0]);
> +	return &vsen->frame;
>  }
>  
>  static int vimc_sen_s_stream(struct v4l2_subdev *sd, int enable)
> @@ -206,8 +206,8 @@ static int vimc_sen_s_stream(struct v4l2_subdev *sd, int enable)
>  		 * Allocate the frame buffer. Use vmalloc to be able to
>  		 * allocate a large amount of memory
>  		 */
> -		vsen->frame = vmalloc(frame_size);
> -		if (!vsen->frame)
> +		vsen->frame.plane_addr[0] = vmalloc(frame_size);
> +		if (!vsen->frame.plane_addr[0])
>  			return -ENOMEM;
>  
>  		/* configure the test pattern generator */
> @@ -215,8 +215,8 @@ static int vimc_sen_s_stream(struct v4l2_subdev *sd, int enable)
>  
>  	} else {
>  
> -		vfree(vsen->frame);
> -		vsen->frame = NULL;
> +		vfree(vsen->frame.plane_addr[0]);
> +		vsen->frame.plane_addr[0] = NULL;
>  		return 0;
>  	}
>  
> diff --git a/drivers/media/platform/vimc/vimc-streamer.c b/drivers/media/platform/vimc/vimc-streamer.c
> index 26b674259489..216ac8a83ba5 100644
> --- a/drivers/media/platform/vimc/vimc-streamer.c
> +++ b/drivers/media/platform/vimc/vimc-streamer.c
> @@ -125,7 +125,7 @@ static int vimc_streamer_pipeline_init(struct vimc_stream *stream,
>  static int vimc_streamer_thread(void *data)
>  {
>  	struct vimc_stream *stream = data;
> -	u8 *frame = NULL;
> +	struct vimc_frame *frame = NULL;
>  	int i;
>  
>  	set_freezable();
> 

[PATCH v2 12/15] media: vimc: sen: Add support for multiplanar formats

[André Almeida]

This commit adapts vimc-sensor to handle multiplanar pixel formats,
adapting the memory allocation and TPG configuration.

Signed-off-by: André Almeida <andrealmeid@collabora.com>
---
Changes in v2:
- Fix alignment issues
- Remove unnecessary variable assignment
- Reuse and share the code to free the memory of planes with a goto

 drivers/media/platform/vimc/vimc-sensor.c | 51 +++++++++++++----------
 1 file changed, 29 insertions(+), 22 deletions(-)

diff --git a/drivers/media/platform/vimc/vimc-sensor.c b/drivers/media/platform/vimc/vimc-sensor.c
index 3495a3f3dd60..217822b2aeb8 100644
--- a/drivers/media/platform/vimc/vimc-sensor.c
+++ b/drivers/media/platform/vimc/vimc-sensor.c
@@ -97,16 +97,16 @@ static int vimc_sen_get_fmt(struct v4l2_subdev *sd,
 static void vimc_sen_tpg_s_format(struct vimc_sen_device *vsen)
 {
 	u32 pixelformat = vsen->ved.stream->producer_pixfmt;
-	const struct v4l2_format_info *pix_info;
-
-	pix_info = v4l2_format_info(pixelformat);
+	unsigned int i;
 
 	tpg_reset_source(&vsen->tpg, vsen->mbus_format.width,
 			 vsen->mbus_format.height, vsen->mbus_format.field);
-	tpg_s_bytesperline(&vsen->tpg, 0,
-			   vsen->mbus_format.width * pix_info->bpp[0]);
 	tpg_s_buf_height(&vsen->tpg, vsen->mbus_format.height);
 	tpg_s_fourcc(&vsen->tpg, pixelformat);
+
+	for (i = 0; i < tpg_g_planes(&vsen->tpg); i++)
+		tpg_s_bytesperline(&vsen->tpg, i, vsen->frame.bytesperline[i]);
+
 	/* TODO: add support for V4L2_FIELD_ALTERNATE */
 	tpg_s_field(&vsen->tpg, vsen->mbus_format.field, false);
 	tpg_s_colorspace(&vsen->tpg, vsen->mbus_format.colorspace);
@@ -182,8 +182,12 @@ static struct vimc_frame *vimc_sen_process_frame(struct vimc_ent_device *ved,
 {
 	struct vimc_sen_device *vsen = container_of(ved, struct vimc_sen_device,
 						    ved);
+	unsigned int i;
+
+	for (i = 0; i < tpg_g_planes(&vsen->tpg); i++)
+		tpg_fill_plane_buffer(&vsen->tpg, 0, i,
+				      vsen->frame.plane_addr[i]);
 
-	tpg_fill_plane_buffer(&vsen->tpg, 0, 0, vsen->frame.plane_addr[0]);
 	return &vsen->frame;
 }
 
@@ -191,36 +195,39 @@ static int vimc_sen_s_stream(struct v4l2_subdev *sd, int enable)
 {
 	struct vimc_sen_device *vsen =
 				container_of(sd, struct vimc_sen_device, sd);
+	unsigned int i, ret = 0;
 
 	if (enable) {
-		u32 pixelformat = vsen->ved.stream->producer_pixfmt;
-		const struct v4l2_format_info *pix_info;
-		unsigned int frame_size;
-
 		/* Calculate the frame size */
-		pix_info = v4l2_format_info(pixelformat);
-		frame_size = vsen->mbus_format.width * pix_info->bpp[0] *
-			     vsen->mbus_format.height;
-
+		vimc_fill_frame(&vsen->frame, vsen->ved.stream->producer_pixfmt,
+				vsen->mbus_format.width,
+				vsen->mbus_format.height,
+				vsen->ved.stream->multiplanar);
 		/*
 		 * Allocate the frame buffer. Use vmalloc to be able to
 		 * allocate a large amount of memory
 		 */
-		vsen->frame.plane_addr[0] = vmalloc(frame_size);
-		if (!vsen->frame.plane_addr[0])
-			return -ENOMEM;
+		for (i = 0; i < vsen->frame.num_planes; i++) {
+			vsen->frame.plane_addr[i] =
+				vmalloc(vsen->frame.sizeimage[i]);
+			if (!vsen->frame.plane_addr[i]) {
+				ret = -ENOMEM;
+				goto free_planes;
+			}
+		}
 
 		/* configure the test pattern generator */
 		vimc_sen_tpg_s_format(vsen);
 
 	} else {
-
-		vfree(vsen->frame.plane_addr[0]);
-		vsen->frame.plane_addr[0] = NULL;
-		return 0;
+free_planes:
+		for (i = 0; i < vsen->frame.num_planes; i++) {
+			vfree(vsen->frame.plane_addr[i]);
+			vsen->frame.plane_addr[i] = NULL;
+		}
 	}
 
-	return 0;
+	return ret;
 }
 
 static const struct v4l2_subdev_core_ops vimc_sen_core_ops = {
-- 
2.21.0

[PATCH v2 13/15] media: vimc: sca: Add support for multiplanar formats

[André Almeida]

Change the scaling functions in order to scale planes. This change makes
easier to support multiplanar pixel formats.

Signed-off-by: André Almeida <andrealmeid@collabora.com>
---
Changes in v2:
- Move the TODO comment to vimc-capture
- Reuse and share the code to free the memory of planes with a goto
- Fix comment block style 

 drivers/media/platform/vimc/vimc-scaler.c | 113 ++++++++++++++--------
 1 file changed, 71 insertions(+), 42 deletions(-)

diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c
index 92c639f1817e..46fb4a98f731 100644
--- a/drivers/media/platform/vimc/vimc-scaler.c
+++ b/drivers/media/platform/vimc/vimc-scaler.c
@@ -39,6 +39,9 @@ static const u32 vimc_sca_supported_pixfmt[] = {
 	V4L2_PIX_FMT_BGR24,
 	V4L2_PIX_FMT_RGB24,
 	V4L2_PIX_FMT_ARGB32,
+	V4L2_PIX_FMT_YUV420,
+	V4L2_PIX_FMT_YUV420M,
+	V4L2_PIX_FMT_NV12M,
 };
 
 struct vimc_sca_device {
@@ -51,8 +54,8 @@ struct vimc_sca_device {
 	struct v4l2_mbus_framefmt sink_fmt;
 	/* Values calculated when the stream starts */
 	struct vimc_frame src_frame;
-	unsigned int src_line_size;
-	unsigned int bpp;
+	unsigned int src_line_size[TPG_MAX_PLANES];
+	unsigned int bpp[TPG_MAX_PLANES];
 };
 
 static const struct v4l2_mbus_framefmt sink_fmt_default = {
@@ -207,10 +210,10 @@ static const struct v4l2_subdev_pad_ops vimc_sca_pad_ops = {
 static int vimc_sca_s_stream(struct v4l2_subdev *sd, int enable)
 {
 	struct vimc_sca_device *vsca = v4l2_get_subdevdata(sd);
+	unsigned int i, ret = 0;
 
 	if (enable) {
 		u32 pixelformat = vsca->ved.stream->producer_pixfmt;
-		const struct v4l2_format_info *pix_info;
 		unsigned int frame_size;
 
 		if (!vimc_sca_is_pixfmt_supported(pixelformat)) {
@@ -219,35 +222,46 @@ static int vimc_sca_s_stream(struct v4l2_subdev *sd, int enable)
 			return -EINVAL;
 		}
 
-		/* Save the bytes per pixel of the sink */
-		pix_info = v4l2_format_info(pixelformat);
-		vsca->bpp = pix_info->bpp[0];
-
-		/* Calculate the width in bytes of the src frame */
-		vsca->src_line_size = vsca->sink_fmt.width *
-				      sca_mult * vsca->bpp;
-
-		/* Calculate the frame size of the source pad */
-		frame_size = vsca->src_line_size * vsca->sink_fmt.height *
-			     sca_mult;
-
-		/* Allocate the frame buffer. Use vmalloc to be able to
-		 * allocate a large amount of memory
-		 */
-		vsca->src_frame.plane_addr[0] = vmalloc(frame_size);
-		vsca->src_frame.sizeimage[0] = frame_size;
-		if (!vsca->src_frame.plane_addr[0])
-			return -ENOMEM;
+		vimc_fill_frame(&vsca->src_frame, pixelformat,
+				vsca->sink_fmt.width, vsca->sink_fmt.height,
+				vsca->ved.stream->multiplanar);
+
+		for (i = 0; i < vsca->src_frame.num_planes; i++) {
+			/* Save the bytes per pixel of the sink */
+			vsca->bpp[i] = vsca->src_frame.bpp[i];
+
+			/* Calculate the width in bytes of the src frame */
+			vsca->src_line_size[i] =
+				vsca->src_frame.bytesperline[i] * sca_mult;
+
+			/* Calculate the frame size of the source pad */
+			frame_size = vsca->src_frame.sizeimage[i] *
+			     sca_mult * sca_mult;
+
+			/**
+			 * Allocate the frame buffer. Use vmalloc to be able to
+			 * allocate a large amount of memory
+			 */
+			vsca->src_frame.plane_addr[i] = vmalloc(frame_size);
+			if (!vsca->src_frame.plane_addr[i]) {
+				ret = -ENOMEM;
+				goto free_planes;
+			}
+			vsca->src_frame.sizeimage[i] = frame_size;
+		}
 
 	} else {
 		if (!vsca->src_frame.plane_addr[0])
 			return 0;
 
-		vfree(vsca->src_frame.plane_addr[0]);
-		vsca->src_frame.plane_addr[0] = NULL;
+free_planes:
+		for (i = 0; i < vsca->src_frame.num_planes; i++) {
+			vfree(vsca->src_frame.plane_addr[i]);
+			vsca->src_frame.plane_addr[i] = NULL;
+		}
 	}
 
-	return 0;
+	return ret;
 }
 
 static const struct v4l2_subdev_video_ops vimc_sca_video_ops = {
@@ -270,18 +284,19 @@ static void vimc_sca_fill_pix(u8 *const ptr,
 		ptr[i] = pixel[i];
 }
 
-static void vimc_sca_scale_pix(const struct vimc_sca_device *const vsca,
+/* TODO: parallelize this function */
+static void vimc_sca_scale_plane(const struct vimc_sca_device *const vsca,
 			       const unsigned int lin, const unsigned int col,
-			       const u8 *const sink_frame)
+			       const struct vimc_frame *sink_frame,
+			       u8 num_plane, u32 width)
+
 {
 	unsigned int i, j, index;
 	const u8 *pixel;
 
 	/* Point to the pixel value in position (lin, col) in the sink frame */
-	index = VIMC_FRAME_INDEX(lin, col,
-				 vsca->sink_fmt.width,
-				 vsca->bpp);
-	pixel = &sink_frame[index];
+	index = VIMC_FRAME_INDEX(lin, col, width, vsca->bpp[num_plane]);
+	pixel = &sink_frame->plane_addr[num_plane][index];
 
 	dev_dbg(vsca->dev,
 		"sca: %s: --- scale_pix sink pos %dx%d, index %d ---\n",
@@ -291,7 +306,7 @@ static void vimc_sca_scale_pix(const struct vimc_sca_device *const vsca,
 	 * in the scaled src frame
 	 */
 	index = VIMC_FRAME_INDEX(lin * sca_mult, col * sca_mult,
-				 vsca->sink_fmt.width * sca_mult, vsca->bpp);
+				 width * sca_mult, vsca->bpp[num_plane]);
 
 	dev_dbg(vsca->dev, "sca: %s: scale_pix src pos %dx%d, index %d\n",
 		vsca->sd.name, lin * sca_mult, col * sca_mult, index);
@@ -301,32 +316,46 @@ static void vimc_sca_scale_pix(const struct vimc_sca_device *const vsca,
 		/* Iterate through each beginning of a
 		 * pixel repetition in a line
 		 */
-		for (j = 0; j < sca_mult * vsca->bpp; j += vsca->bpp) {
+		unsigned int bpp = vsca->bpp[num_plane];
+
+		for (j = 0; j < sca_mult * bpp; j += bpp) {
 			dev_dbg(vsca->dev,
 				"sca: %s: sca: scale_pix src pos %d\n",
 				vsca->sd.name, index + j);
 
 			/* copy the pixel to the position index + j */
 			vimc_sca_fill_pix(
-				&vsca->src_frame.plane_addr[0][index + j],
-				pixel, vsca->bpp);
+				&vsca->src_frame.plane_addr[num_plane][index+j],
+				pixel, bpp);
 		}
 
 		/* move the index to the next line */
-		index += vsca->src_line_size;
+		index += vsca->src_line_size[num_plane];
 	}
 }
 
 static void vimc_sca_fill_src_frame(const struct vimc_sca_device *const vsca,
-				    const u8 *const sink_frame)
+				    const struct vimc_frame *sink_frame)
 {
-	unsigned int i, j;
+	u32 i, j, width, height;
+	unsigned int num_plane;
+	const struct v4l2_format_info *info;
+
+	info = v4l2_format_info(sink_frame->pixelformat);
 
 	/* Scale each pixel from the original sink frame */
 	/* TODO: implement scale down, only scale up is supported for now */
-	for (i = 0; i < vsca->sink_fmt.height; i++)
-		for (j = 0; j < vsca->sink_fmt.width; j++)
-			vimc_sca_scale_pix(vsca, i, j, sink_frame);
+	for (num_plane = 0; num_plane < info->comp_planes; num_plane++) {
+		width = vsca->sink_fmt.width /
+					((num_plane == 0) ? 1 : info->vdiv);
+		height = vsca->sink_fmt.height /
+					((num_plane == 0) ? 1 : info->hdiv);
+
+		for (i = 0; i < height; i++)
+			for (j = 0; j < width; j++)
+				vimc_sca_scale_plane(vsca, i, j, sink_frame,
+						     num_plane, width);
+	}
 }
 
 static struct vimc_frame *vimc_sca_process_frame(struct vimc_ent_device *ved,
@@ -339,7 +368,7 @@ static struct vimc_frame *vimc_sca_process_frame(struct vimc_ent_device *ved,
 	if (!ved->stream)
 		return ERR_PTR(-EINVAL);
 
-	vimc_sca_fill_src_frame(vsca, sink_frame->plane_addr[0]);
+	vimc_sca_fill_src_frame(vsca, sink_frame);
 
 	return &vsca->src_frame;
 };
-- 
2.21.0

[PATCH v2 14/15] media: vimc: cap: Add support for multiplanar formats

[André Almeida]

Adapt vimc-capture to support multiplanar formats, copying
each plane to the correct buffer.

Signed-off-by: André Almeida <andrealmeid@collabora.com>
---
Change in v2: none

 drivers/media/platform/vimc/vimc-capture.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
index fae42b778711..f5bfb36a145c 100644
--- a/drivers/media/platform/vimc/vimc-capture.c
+++ b/drivers/media/platform/vimc/vimc-capture.c
@@ -606,6 +606,8 @@ static struct vimc_frame *vimc_cap_process_frame(struct vimc_ent_device *ved,
 	struct vimc_cap_device *vcap = container_of(ved, struct vimc_cap_device,
 						    ved);
 	struct vimc_cap_buffer *vimc_buf;
+	unsigned long plane_size;
+	unsigned int i;
 	void *vbuf;
 
 	spin_lock(&vcap->qlock);
@@ -628,13 +630,17 @@ static struct vimc_frame *vimc_cap_process_frame(struct vimc_ent_device *ved,
 	vimc_buf->vb2.sequence = vcap->sequence++;
 	vimc_buf->vb2.field = vcap->format.fmt.pix.field;
 
-	vbuf = vb2_plane_vaddr(&vimc_buf->vb2.vb2_buf, 0);
+	/* For each plane, copy the pixels */
+	for (i = 0; i < vimc_buf->vb2.vb2_buf.num_planes; i++) {
+		vbuf = vb2_plane_vaddr(&vimc_buf->vb2.vb2_buf, i);
+		plane_size = frame->sizeimage[i];
+
+		memcpy(vbuf, frame->plane_addr[i], plane_size);
 
-	memcpy(vbuf, frame->plane_addr[0], vcap->format.fmt.pix.sizeimage);
+		/* Set it as ready */
+		vb2_set_plane_payload(&vimc_buf->vb2.vb2_buf, i, plane_size);
+	}
 
-	/* Set it as ready */
-	vb2_set_plane_payload(&vimc_buf->vb2.vb2_buf, 0,
-			      vcap->format.fmt.pix.sizeimage);
 	vb2_buffer_done(&vimc_buf->vb2.vb2_buf, VB2_BUF_STATE_DONE);
 	return NULL;
 }
-- 
2.21.0

[PATCH v2 15/15] media: vimc: Create multiplanar parameter

[André Almeida]

Create multiplanar kernel module parameter to define if the driver is
running in single planar or in multiplanar mode. Define the device
capabilities according to the multiplanar kernel parameter. A device can't
support both CAP_VIDEO_CAPTURE and CAP_VIDEO_CAPTURE_MPLANAR at the
same time.

Signed-off-by: André Almeida <andrealmeid@collabora.com>
---
Change in v2:
- Squash commits to create multiplanar module parameter and to define
the device capabilities
- Move the creation of the multiplanar parameter to the end of
history, so it's only added when all required changes are applied

 drivers/media/platform/vimc/vimc-capture.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
index f5bfb36a145c..3666fcca6812 100644
--- a/drivers/media/platform/vimc/vimc-capture.c
+++ b/drivers/media/platform/vimc/vimc-capture.c
@@ -28,6 +28,10 @@
 
 #define VIMC_CAP_DRV_NAME "vimc-capture"
 
+static unsigned int multiplanar;
+module_param(multiplanar, uint, 0000);
+MODULE_PARM_DESC(multiplanar, "0 (default) creates a single planar device, 1 creates a multiplanar device.");
+
 /* Checks if the device supports multiplanar capture */
 #define IS_MULTIPLANAR(vcap) \
 	(vcap->vdev.device_caps & V4L2_CAP_VIDEO_CAPTURE_MPLANE)
@@ -681,7 +685,9 @@ static int vimc_cap_comp_bind(struct device *comp, struct device *master,
 
 	/* Initialize the vb2 queue */
 	q = &vcap->queue;
-	q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+	q->type = multiplanar ?
+		V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE :
+		V4L2_BUF_TYPE_VIDEO_CAPTURE;
 	q->io_modes = VB2_MMAP | VB2_DMABUF | VB2_USERPTR;
 	q->drv_priv = vcap;
 	q->buf_struct_size = sizeof(struct vimc_cap_buffer);
@@ -709,9 +715,12 @@ static int vimc_cap_comp_bind(struct device *comp, struct device *master,
 	dev_set_drvdata(comp, &vcap->ved);
 	vcap->dev = comp;
 
+
 	/* Initialize the video_device struct */
 	vdev = &vcap->vdev;
-	vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
+	vdev->device_caps = (multiplanar ?
+			V4L2_CAP_VIDEO_CAPTURE_MPLANE :
+			V4L2_BUF_TYPE_VIDEO_CAPTURE) | V4L2_CAP_STREAMING;
 	vdev->entity.ops = &vimc_cap_mops;
 	vdev->release = vimc_cap_release;
 	vdev->fops = &vimc_cap_fops;
-- 
2.21.0

Re: [PATCH v2 15/15] media: vimc: Create multiplanar parameter

[Helen Koike]

On 3/27/19 12:17 PM, André Almeida wrote:
> Create multiplanar kernel module parameter to define if the driver is
> running in single planar or in multiplanar mode. Define the device
> capabilities according to the multiplanar kernel parameter. A device can't
> support both CAP_VIDEO_CAPTURE and CAP_VIDEO_CAPTURE_MPLANAR at the
> same time.
> 
> Signed-off-by: André Almeida <andrealmeid@collabora.com>
> ---
> Change in v2:
> - Squash commits to create multiplanar module parameter and to define
> the device capabilities
> - Move the creation of the multiplanar parameter to the end of
> history, so it's only added when all required changes are applied
> 
>  drivers/media/platform/vimc/vimc-capture.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
> index f5bfb36a145c..3666fcca6812 100644
> --- a/drivers/media/platform/vimc/vimc-capture.c
> +++ b/drivers/media/platform/vimc/vimc-capture.c
> @@ -28,6 +28,10 @@
>  
>  #define VIMC_CAP_DRV_NAME "vimc-capture"
>  
> +static unsigned int multiplanar;
> +module_param(multiplanar, uint, 0000);
> +MODULE_PARM_DESC(multiplanar, "0 (default) creates a single planar device, 1 creates a multiplanar device.");
> +
>  /* Checks if the device supports multiplanar capture */
>  #define IS_MULTIPLANAR(vcap) \
>  	(vcap->vdev.device_caps & V4L2_CAP_VIDEO_CAPTURE_MPLANE)
> @@ -681,7 +685,9 @@ static int vimc_cap_comp_bind(struct device *comp, struct device *master,
>  
>  	/* Initialize the vb2 queue */
>  	q = &vcap->queue;
> -	q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> +	q->type = multiplanar ?
> +		V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE :
> +		V4L2_BUF_TYPE_VIDEO_CAPTURE;
>  	q->io_modes = VB2_MMAP | VB2_DMABUF | VB2_USERPTR;
>  	q->drv_priv = vcap;
>  	q->buf_struct_size = sizeof(struct vimc_cap_buffer);
> @@ -709,9 +715,12 @@ static int vimc_cap_comp_bind(struct device *comp, struct device *master,
>  	dev_set_drvdata(comp, &vcap->ved);
>  	vcap->dev = comp;
>  
> +
>  	/* Initialize the video_device struct */
>  	vdev = &vcap->vdev;
> -	vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
> +	vdev->device_caps = (multiplanar ?
> +			V4L2_CAP_VIDEO_CAPTURE_MPLANE :
> +			V4L2_BUF_TYPE_VIDEO_CAPTURE) | V4L2_CAP_STREAMING;

s/V4L2_BUF_TYPE_VIDEO_CAPTURE/V4L2_CAP_VIDEO_CAPTURE

>  	vdev->entity.ops = &vimc_cap_mops;
>  	vdev->release = vimc_cap_release;
>  	vdev->fops = &vimc_cap_fops;
>