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

[PATCH 00/16] 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 (single/multi)
and favoring code reuse.

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.

Thanks,
	André

André Almeida (16):
  media: Move sp2mp functions to v4l2-common
  media: vimc: Remove unnecessary stream check
  media: vimc: Check if the stream is on using ved.stream
  media: vimc: cap: Change vimc_cap_device.format type
  media: vimc: Create multiplanar parameter
  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: 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: cap: Dynamically define device caps

 drivers/media/platform/vimc/vimc-capture.c    | 310 +++++++++++++++---
 drivers/media/platform/vimc/vimc-common.c     |  37 +++
 drivers/media/platform/vimc/vimc-common.h     |  50 ++-
 drivers/media/platform/vimc/vimc-core.c       |   8 +
 drivers/media/platform/vimc/vimc-debayer.c    |  38 +--
 drivers/media/platform/vimc/vimc-scaler.c     | 125 ++++---
 drivers/media/platform/vimc/vimc-sensor.c     |  62 ++--
 drivers/media/platform/vimc/vimc-streamer.c   |   2 +-
 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                   |  31 ++
 14 files changed, 580 insertions(+), 225 deletions(-)

-- 
2.21.0

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

[André Almeida]

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

Signed-off-by: André Almeida <andrealmeid@collabora.com>
---
 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                   | 31 ++++++++++
 6 files changed, 99 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 e61b91b414f9..c42ba5ade6cf 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..d118f8f34d32 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..d106f36ebaf4 100644
--- a/include/media/v4l2-common.h
+++ b/include/media/v4l2-common.h
@@ -424,4 +424,35 @@ 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 - type to be used by v4l2_fmt_sp2mp_func to pass the generic
+ * mp function as argument
+ * @file: device's descriptor file
+ * @priv: private data pointer
+ * @f: format that holds a mp pixel format
+ */
+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 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.
+ * @file: device's descriptor file
+ * @priv: private data pointer
+ * @f: format that holds a sp pixel format
+ * @func: generic mp function
+ */
+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 01/16] media: Move sp2mp functions to v4l2-common

[Helen Koike]

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

s/cote/code

> 
> Signed-off-by: André Almeida <andrealmeid@collabora.com>
> ---
>  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                   | 31 ++++++++++
>  6 files changed, 99 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 e61b91b414f9..c42ba5ade6cf 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..d118f8f34d32 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..d106f36ebaf4 100644
> --- a/include/media/v4l2-common.h
> +++ b/include/media/v4l2-common.h
> @@ -424,4 +424,35 @@ 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 - type to be used by v4l2_fmt_sp2mp_func to pass the generic
> + * mp function as argument

You can use a shorter title, e.g.
Function pointer type for v4l2_fmt_sp2mp()

And you can add a better description below,
e.g.https://git.linuxtv.org/media_tree.git/tree/include/media/v4l2-common.h#n287

this also applies to other docs.

> + * @file: device's descriptor file
> + * @priv: private data pointer
> + * @f: format that holds a mp pixel format
> + */
> +typedef int (*v4l2_fmtfunc)(struct file *file, void *priv,
> +		struct v4l2_format *f);

It would be nice if you could align the second line with the arguments
of the function in the first line.

> +
> +/**
> + * 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);

same here regarding alignment.

> +
> +/**
> + * v4l2_fmt_sp2mp_func - 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.
> + * @file: device's descriptor file
> + * @priv: private data pointer
> + * @f: format that holds a sp pixel format
> + * @func: generic mp function
> + */
> +int v4l2_fmt_sp2mp_func(struct file *file, void *priv,
> +		struct v4l2_format *f, v4l2_fmtfunc func);

same here regarding alignment.

Regards,
Helen

> +
>  #endif /* V4L2_COMMON_H_ */
> 

[PATCH 02/16] media: vimc: Remove unnecessary stream check

[André Almeida]

Since V4L2 already checks if the stream is running (and avoid starting
it twice), remove the check at subdevices.

Signed-off-by: André Almeida <andrealmeid@collabora.com>
---
 drivers/media/platform/vimc/vimc-debayer.c | 3 ---
 drivers/media/platform/vimc/vimc-scaler.c  | 3 ---
 drivers/media/platform/vimc/vimc-sensor.c  | 4 ----
 3 files changed, 10 deletions(-)

diff --git a/drivers/media/platform/vimc/vimc-debayer.c b/drivers/media/platform/vimc/vimc-debayer.c
index fc3babdb3a1c..5f84cb38f0f9 100644
--- a/drivers/media/platform/vimc/vimc-debayer.c
+++ b/drivers/media/platform/vimc/vimc-debayer.c
@@ -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 d613297a75b8..3102febefd63 100644
--- a/drivers/media/platform/vimc/vimc-scaler.c
+++ b/drivers/media/platform/vimc/vimc-scaler.c
@@ -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));
diff --git a/drivers/media/platform/vimc/vimc-sensor.c b/drivers/media/platform/vimc/vimc-sensor.c
index cfa72dd39748..44a75099ce7f 100644
--- a/drivers/media/platform/vimc/vimc-sensor.c
+++ b/drivers/media/platform/vimc/vimc-sensor.c
@@ -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

[PATCH 03/16] media: vimc: Check if the stream is on using ved.stream

[André Almeida]

Change the way that the subdevices check if the stream is running in set
format functions. It uses the *stream in vimc_deb_device, the more
appropriate pointer. This also makes easier to get rid of the void* and u8*
in the subdevices structs.

Signed-off-by: André Almeida <andrealmeid@collabora.com>
---
 drivers/media/platform/vimc/vimc-debayer.c | 2 +-
 drivers/media/platform/vimc/vimc-scaler.c  | 4 ++--
 drivers/media/platform/vimc/vimc-sensor.c  | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/vimc/vimc-debayer.c b/drivers/media/platform/vimc/vimc-debayer.c
index 5f84cb38f0f9..f72f888ba5a6 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;
diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c
index 3102febefd63..6e88328dca5c 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;
@@ -334,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 44a75099ce7f..e60f1985edb0 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;
-- 
2.21.0

Re: [PATCH 03/16] media: vimc: Check if the stream is on using ved.stream

[Helen Koike]

On 3/15/19 1:43 PM, André Almeida wrote:
> Change the way that the subdevices check if the stream is running in set
> format functions. It uses the *stream in vimc_deb_device, the more
> appropriate pointer. This also makes easier to get rid of the void* and u8*
> in the subdevices structs.

I would just reword this a bit, how about:

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.

I also think you can squash this patch with the previous one, and just
mention in the patch you are removing unnecessary checks.

Regards,
Helen

> 
> Signed-off-by: André Almeida <andrealmeid@collabora.com>
> ---
>  drivers/media/platform/vimc/vimc-debayer.c | 2 +-
>  drivers/media/platform/vimc/vimc-scaler.c  | 4 ++--
>  drivers/media/platform/vimc/vimc-sensor.c  | 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/platform/vimc/vimc-debayer.c b/drivers/media/platform/vimc/vimc-debayer.c
> index 5f84cb38f0f9..f72f888ba5a6 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;
> diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c
> index 3102febefd63..6e88328dca5c 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;
> @@ -334,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 44a75099ce7f..e60f1985edb0 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;
> 

[PATCH 04/16] 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>
---
 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 c955feea4508..c7ec55c4fe13 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",
@@ -405,15 +409,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;
 }
@@ -477,8 +481,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

[PATCH 05/16] 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.

Signed-off-by: André Almeida <andrealmeid@collabora.com>
---
 drivers/media/platform/vimc/vimc-common.h | 2 ++
 drivers/media/platform/vimc/vimc-core.c   | 8 ++++++++
 2 files changed, 10 insertions(+)

diff --git a/drivers/media/platform/vimc/vimc-common.h b/drivers/media/platform/vimc/vimc-common.h
index 7ceb9ea937e2..25e47c8691dd 100644
--- a/drivers/media/platform/vimc/vimc-common.h
+++ b/drivers/media/platform/vimc/vimc-common.h
@@ -26,6 +26,8 @@
 
 #define VIMC_PDEV_NAME "vimc"
 
+extern unsigned int multiplanar;
+
 /* VIMC-specific controls */
 #define VIMC_CID_VIMC_BASE		(0x00f00000 | 0xf000)
 #define VIMC_CID_VIMC_CLASS		(0x00f00000 | 1)
diff --git a/drivers/media/platform/vimc/vimc-core.c b/drivers/media/platform/vimc/vimc-core.c
index 0fbb7914098f..34ca90fa6e79 100644
--- a/drivers/media/platform/vimc/vimc-core.c
+++ b/drivers/media/platform/vimc/vimc-core.c
@@ -26,6 +26,11 @@
 
 #define VIMC_MDEV_MODEL_NAME "VIMC MDEV"
 
+unsigned int multiplanar;
+module_param(multiplanar, uint, 0000);
+MODULE_PARM_DESC(sca_mult, "0 (default) creates a single planar device, 1 creates a multiplanar device.");
+
+
 #define VIMC_ENT_LINK(src, srcpad, sink, sinkpad, link_flags) {	\
 	.src_ent = src,						\
 	.src_pad = srcpad,					\
@@ -388,6 +393,9 @@ static int __init vimc_init(void)
 		return ret;
 	}
 
+	dev_dbg(&vimc_dev.pdev.dev, "vimc: multiplanar mode is %s\n",
+		multiplanar ? "ON" : "OFF");
+
 	return 0;
 }
 
-- 
2.21.0

Re: [PATCH 05/16] media: vimc: Create multiplanar parameter

[Helen Koike]

On 3/15/19 1:43 PM, André Almeida wrote:
> Create multiplanar kernel module parameter to define if
> the driver is running in single planar or in multiplanar mode.
> 
> Signed-off-by: André Almeida <andrealmeid@collabora.com>
> ---
>  drivers/media/platform/vimc/vimc-common.h | 2 ++
>  drivers/media/platform/vimc/vimc-core.c   | 8 ++++++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/drivers/media/platform/vimc/vimc-common.h b/drivers/media/platform/vimc/vimc-common.h
> index 7ceb9ea937e2..25e47c8691dd 100644
> --- a/drivers/media/platform/vimc/vimc-common.h
> +++ b/drivers/media/platform/vimc/vimc-common.h
> @@ -26,6 +26,8 @@
>  
>  #define VIMC_PDEV_NAME "vimc"
>  
> +extern unsigned int multiplanar;
> +
>  /* VIMC-specific controls */
>  #define VIMC_CID_VIMC_BASE		(0x00f00000 | 0xf000)
>  #define VIMC_CID_VIMC_CLASS		(0x00f00000 | 1)
> diff --git a/drivers/media/platform/vimc/vimc-core.c b/drivers/media/platform/vimc/vimc-core.c
> index 0fbb7914098f..34ca90fa6e79 100644
> --- a/drivers/media/platform/vimc/vimc-core.c
> +++ b/drivers/media/platform/vimc/vimc-core.c
> @@ -26,6 +26,11 @@
>  
>  #define VIMC_MDEV_MODEL_NAME "VIMC MDEV"
>  
> +unsigned int multiplanar;
> +module_param(multiplanar, uint, 0000);
> +MODULE_PARM_DESC(sca_mult, "0 (default) creates a single planar device, 1 creates a multiplanar device.");
> +
> +
>  #define VIMC_ENT_LINK(src, srcpad, sink, sinkpad, link_flags) {	\
>  	.src_ent = src,						\
>  	.src_pad = srcpad,					\
> @@ -388,6 +393,9 @@ static int __init vimc_init(void)
>  		return ret;
>  	}
>  
> +	dev_dbg(&vimc_dev.pdev.dev, "vimc: multiplanar mode is %s\n",
> +		multiplanar ? "ON" : "OFF");
> +
>  	return 0;
>  }
>  
> 

It just came to me that instead of using the multiplanar variable
everywhere, you can just check the vcap->vdev.device_caps to see if
multiplanar is supported. This way, you can add this multiplanar
variable together with the capability in the [PATCH 16/16] media: vimc:
cap: Dynamically define device caps.

Like this, we won't have the weird state where user can set
multiplanar=1 but the capabilities says it doesn't really support
multiplanar.


Regards,
Helen

[PATCH 06/16] 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>
---
 drivers/media/platform/vimc/vimc-capture.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
index c7ec55c4fe13..a93241d53953 100644
--- a/drivers/media/platform/vimc/vimc-capture.c
+++ b/drivers/media/platform/vimc/vimc-capture.c
@@ -282,7 +282,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 (vcap->format.type == V4L2_CAP_VIDEO_CAPTURE_MPLANE)
+		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

[PATCH 07/16] 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>
---
 drivers/media/platform/vimc/vimc-capture.c | 63 +++++++++++++++++-----
 1 file changed, 50 insertions(+), 13 deletions(-)

diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
index a93241d53953..47acf50f1ad2 100644
--- a/drivers/media/platform/vimc/vimc-capture.c
+++ b/drivers/media/platform/vimc/vimc-capture.c
@@ -127,7 +127,7 @@ static int vimc_cap_g_fmt_vid_cap(struct file *file, void *priv,
 static int vimc_cap_try_fmt_vid_cap(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;
@@ -145,20 +145,58 @@ 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,
+	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,
+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];
+	strncpy(f->description, v4l2_get_fourcc_name(f->pixelformat), 4);
+	f->description[4] = '\0';
+
+	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)
+{
+	if (multiplanar)
+		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)
+{
+	if (multiplanar)
+		return -EINVAL;
+
+	return v4l2_fmt_sp2mp_func(file, priv, f, vimc_cap_try_fmt_vid_cap);
+}
+
+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 (multiplanar)
+		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);
 
 	dev_dbg(vcap->dev, "%s: format update: "
 		"old:%dx%d (0x%x, %d, %d, %d, %d) "
@@ -181,15 +219,13 @@ 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,
+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))
+	if (multiplanar)
 		return -EINVAL;
 
-	f->pixelformat = vimc_cap_supported_pixfmt[f->index];
-
-	return 0;
+	return vimc_cap_enum_fmt_vid_cap(file, priv, f);
 }
 
 static bool vimc_cap_is_pixfmt_supported(u32 pixelformat)
@@ -235,10 +271,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 07/16] media: vimc: cap: Add handler for singleplanar fmt ioctls

[Helen Koike]

On 3/15/19 1:43 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().
> 
> Signed-off-by: André Almeida <andrealmeid@collabora.com>
> ---
>  drivers/media/platform/vimc/vimc-capture.c | 63 +++++++++++++++++-----
>  1 file changed, 50 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
> index a93241d53953..47acf50f1ad2 100644
> --- a/drivers/media/platform/vimc/vimc-capture.c
> +++ b/drivers/media/platform/vimc/vimc-capture.c
> @@ -127,7 +127,7 @@ static int vimc_cap_g_fmt_vid_cap(struct file *file, void *priv,
>  static int vimc_cap_try_fmt_vid_cap(struct file *file, void *priv,
>  				    struct v4l2_format *f)

I think you can add the _mp suffix to vimc_cap_try_fmt_vid_cap, so it
makes it explicit that it deals with mp formats.

But as you also adds a vimc_cap_try_fmt_vid_cap_mp in the next commit,
you can use a _vimc_cap_try_fmt_vid_cap_mp to have both functions.

Example:
https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/vivid/vivid-osd.c#n129
https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/vivid/vivid-osd.c#n169

Regards,
Helen

>  {
> -	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;
> @@ -145,20 +145,58 @@ 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,
> +	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,
> +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];
> +	strncpy(f->description, v4l2_get_fourcc_name(f->pixelformat), 4);
> +	f->description[4] = '\0';
> +
> +	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)
> +{
> +	if (multiplanar)
> +		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)
> +{
> +	if (multiplanar)
> +		return -EINVAL;
> +
> +	return v4l2_fmt_sp2mp_func(file, priv, f, vimc_cap_try_fmt_vid_cap);
> +}
> +
> +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 (multiplanar)
> +		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);
>  
>  	dev_dbg(vcap->dev, "%s: format update: "
>  		"old:%dx%d (0x%x, %d, %d, %d, %d) "
> @@ -181,15 +219,13 @@ 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,
> +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))
> +	if (multiplanar)
>  		return -EINVAL;
>  
> -	f->pixelformat = vimc_cap_supported_pixfmt[f->index];
> -
> -	return 0;
> +	return vimc_cap_enum_fmt_vid_cap(file, priv, f);
>  }
>  
>  static bool vimc_cap_is_pixfmt_supported(u32 pixelformat)
> @@ -235,10 +271,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 08/16] 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>
---
 drivers/media/platform/vimc/vimc-capture.c | 79 ++++++++++++++++++++++
 1 file changed, 79 insertions(+)

diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
index 47acf50f1ad2..09a8fd618b12 100644
--- a/drivers/media/platform/vimc/vimc-capture.c
+++ b/drivers/media/platform/vimc/vimc-capture.c
@@ -114,6 +114,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)
 {
@@ -237,6 +241,71 @@ 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)
+{
+	if (!multiplanar)
+		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)
+{
+	if (!multiplanar)
+		return -EINVAL;
+
+	return vimc_cap_try_fmt_vid_cap(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 (!multiplanar)
+		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);
+
+	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)
+{
+	if (!multiplanar)
+		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)
@@ -268,14 +337,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

Re: [PATCH 08/16] media: vimc: cap: Add handler for multiplanar fmt ioctls

[Helen Koike]

On 3/15/19 1:43 PM, André Almeida wrote:
> Add functions to handle multiplanar format ioctls, calling
> the generic format ioctls functions when possible.
> 
> Signed-off-by: André Almeida <andrealmeid@collabora.com>
> ---
>  drivers/media/platform/vimc/vimc-capture.c | 79 ++++++++++++++++++++++
>  1 file changed, 79 insertions(+)
> 
> diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
> index 47acf50f1ad2..09a8fd618b12 100644
> --- a/drivers/media/platform/vimc/vimc-capture.c
> +++ b/drivers/media/platform/vimc/vimc-capture.c
> @@ -114,6 +114,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)
>  {
> @@ -237,6 +241,71 @@ 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)

Please align this second line with the parameters of the function (this
also applies to other places).

Regards,
Helen

> +{
> +	if (!multiplanar)
> +		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)
> +{
> +	if (!multiplanar)
> +		return -EINVAL;
> +
> +	return vimc_cap_try_fmt_vid_cap(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 (!multiplanar)
> +		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);
> +
> +	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)
> +{
> +	if (!multiplanar)
> +		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)
> @@ -268,14 +337,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,
> 

[PATCH 09/16] 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>
---
 drivers/media/platform/vimc/vimc-capture.c | 30 ++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
index 09a8fd618b12..2d668012e9e9 100644
--- a/drivers/media/platform/vimc/vimc-capture.c
+++ b/drivers/media/platform/vimc/vimc-capture.c
@@ -54,6 +54,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;
@@ -153,13 +166,26 @@ static int vimc_cap_try_fmt_vid_cap(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))
+	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 + (multiplanar ? 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];
+
 	strncpy(f->description, v4l2_get_fourcc_name(f->pixelformat), 4);
 	f->description[4] = '\0';
 
-- 
2.21.0

Re: [PATCH 09/16] media: vimc: cap: Add multiplanar formats

[Helen Koike]

On 3/15/19 1:43 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>
> ---
>  drivers/media/platform/vimc/vimc-capture.c | 30 ++++++++++++++++++++--
>  1 file changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
> index 09a8fd618b12..2d668012e9e9 100644
> --- a/drivers/media/platform/vimc/vimc-capture.c
> +++ b/drivers/media/platform/vimc/vimc-capture.c
> @@ -54,6 +54,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;
> @@ -153,13 +166,26 @@ static int vimc_cap_try_fmt_vid_cap(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))
> +	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 + (multiplanar ? 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];

I think it would be better if you break the line like this:

		f->pixelformat =
			vimc_cap_supported_pixfmt_mp[f->index - sp_size];

but this is a matter of style.

Regards,
Helen

> +	else
> +		f->pixelformat = vimc_cap_supported_pixfmt[f->index];
> +
>  	strncpy(f->description, v4l2_get_fourcc_name(f->pixelformat), 4);
>  	f->description[4] = '\0';
>  
> 

[PATCH 10/16] 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 perfom those
same actions.

Signed-off-by: André Almeida <andrealmeid@collabora.com>
---
 drivers/media/platform/vimc/vimc-capture.c | 31 +++++++++++++++++-----
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
index 2d668012e9e9..24052f15c4cf 100644
--- a/drivers/media/platform/vimc/vimc-capture.c
+++ b/drivers/media/platform/vimc/vimc-capture.c
@@ -96,6 +96,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
@@ -160,7 +169,9 @@ static int vimc_cap_try_fmt_vid_cap(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 = multiplanar ?
+				fmt_default_mp.fmt.pix_mp.pixelformat :
+				fmt_default.fmt.pix.pixelformat;
 
 	return v4l2_fill_pixfmt_mp(format, format->pixelformat,
 				format->width, format->height);
@@ -627,11 +638,19 @@ static int vimc_cap_comp_bind(struct device *comp, struct device *master,
 	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);
+	if (multiplanar) {
+		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);
+	}
 
 	/* Fill the vimc_ent_device struct */
 	vcap->ved.ent = &vcap->vdev.entity;
-- 
2.21.0

[PATCH 11/16] 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>
---
 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 24052f15c4cf..83196b8c31b5 100644
--- a/drivers/media/platform/vimc/vimc-capture.c
+++ b/drivers/media/platform/vimc/vimc-capture.c
@@ -486,12 +486,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 (multiplanar) {
+		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 (multiplanar) {
+		*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;
 }
@@ -499,9 +515,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 (multiplanar) {
+		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

[PATCH 12/16] 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>
---
 drivers/media/platform/vimc/vimc-capture.c  |  6 +--
 drivers/media/platform/vimc/vimc-common.c   | 37 ++++++++++++++++
 drivers/media/platform/vimc/vimc-common.h   | 48 +++++++++++++++++++--
 drivers/media/platform/vimc/vimc-debayer.c  | 33 +++++++-------
 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, 126 insertions(+), 44 deletions(-)

diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
index 83196b8c31b5..bb982761562e 100644
--- a/drivers/media/platform/vimc/vimc-capture.c
+++ b/drivers/media/platform/vimc/vimc-capture.c
@@ -571,8 +571,8 @@ static void vimc_cap_comp_unbind(struct device *comp, struct device *master,
 	kfree(vcap);
 }
 
-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);
@@ -601,7 +601,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 f664f23ee0ca..96247302f6c9 100644
--- a/drivers/media/platform/vimc/vimc-common.c
+++ b/drivers/media/platform/vimc/vimc-common.c
@@ -378,6 +378,43 @@ 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)
+{
+	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->pixelformat = pixelformat;
+		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;
+	}
+}
+
 void vimc_ent_sd_unregister(struct vimc_ent_device *ved, struct v4l2_subdev *sd)
 {
 	v4l2_device_unregister_subdev(sd);
diff --git a/drivers/media/platform/vimc/vimc-common.h b/drivers/media/platform/vimc/vimc-common.h
index 25e47c8691dd..c891701e95a5 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"
 
@@ -81,6 +82,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;
+	u8 *plane_addr[TPG_MAX_PLANES];
+	u8 num_planes;
+	u32 sizeimage[TPG_MAX_PLANES];
+	u32 bytesperline[TPG_MAX_PLANES];
+	u8 bpp[TPG_MAX_PLANES];
+};
+
 /**
  * struct vimc_ent_device - core struct that represents a node in the topology
  *
@@ -103,10 +129,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,20 @@ 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
+ *
+ * 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);
+
 #endif
diff --git a/drivers/media/platform/vimc/vimc-debayer.c b/drivers/media/platform/vimc/vimc-debayer.c
index f72f888ba5a6..19668de9a4d5 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,8 @@ 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);
 
 		/* Get bpp from the sink */
 		pix_info = v4l2_format_info(vdeb->sink_pix_map->pixelformat);
@@ -366,16 +364,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 +487,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 +496,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 6e88328dca5c..65519495ecca 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_comp_unbind(struct device *comp, struct device *master,
diff --git a/drivers/media/platform/vimc/vimc-sensor.c b/drivers/media/platform/vimc/vimc-sensor.c
index e60f1985edb0..020651320ac9 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 c19093b6c787..efbc6adc34be 100644
--- a/drivers/media/platform/vimc/vimc-streamer.c
+++ b/drivers/media/platform/vimc/vimc-streamer.c
@@ -124,7 +124,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 12/16] media: vimc: Add and use new struct vimc_frame

[Helen Koike]

On 3/15/19 1:43 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>
> ---
>  drivers/media/platform/vimc/vimc-capture.c  |  6 +--
>  drivers/media/platform/vimc/vimc-common.c   | 37 ++++++++++++++++
>  drivers/media/platform/vimc/vimc-common.h   | 48 +++++++++++++++++++--
>  drivers/media/platform/vimc/vimc-debayer.c  | 33 +++++++-------
>  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, 126 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
> index 83196b8c31b5..bb982761562e 100644
> --- a/drivers/media/platform/vimc/vimc-capture.c
> +++ b/drivers/media/platform/vimc/vimc-capture.c
> @@ -571,8 +571,8 @@ static void vimc_cap_comp_unbind(struct device *comp, struct device *master,
>  	kfree(vcap);
>  }
>  
> -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);
> @@ -601,7 +601,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 f664f23ee0ca..96247302f6c9 100644
> --- a/drivers/media/platform/vimc/vimc-common.c
> +++ b/drivers/media/platform/vimc/vimc-common.c
> @@ -378,6 +378,43 @@ 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)
> +{
> +	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->pixelformat = pixelformat;

This assigned was already done outside the if block

> +		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;

You can use a single line here, it will fix 80 chars exact :)

> +			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;
> +	}
> +}
> +
>  void vimc_ent_sd_unregister(struct vimc_ent_device *ved, struct v4l2_subdev *sd)
>  {
>  	v4l2_device_unregister_subdev(sd);
> diff --git a/drivers/media/platform/vimc/vimc-common.h b/drivers/media/platform/vimc/vimc-common.h
> index 25e47c8691dd..c891701e95a5 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"
>  
> @@ -81,6 +82,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;
> +	u8 *plane_addr[TPG_MAX_PLANES];
> +	u8 num_planes;

please move u8 to the end to avoid weird padding in the struct.

> +	u32 sizeimage[TPG_MAX_PLANES];
> +	u32 bytesperline[TPG_MAX_PLANES];
> +	u8 bpp[TPG_MAX_PLANES];
> +};
> +
>  /**
>   * struct vimc_ent_device - core struct that represents a node in the topology
>   *
> @@ -103,10 +129,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,20 @@ 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
> + *
> + * 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);
> +
>  #endif
> diff --git a/drivers/media/platform/vimc/vimc-debayer.c b/drivers/media/platform/vimc/vimc-debayer.c
> index f72f888ba5a6..19668de9a4d5 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,8 @@ 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);
>  
>  		/* Get bpp from the sink */
>  		pix_info = v4l2_format_info(vdeb->sink_pix_map->pixelformat);
> @@ -366,16 +364,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 +487,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 +496,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);

please align

Regards,
Helen

>  			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 6e88328dca5c..65519495ecca 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_comp_unbind(struct device *comp, struct device *master,
> diff --git a/drivers/media/platform/vimc/vimc-sensor.c b/drivers/media/platform/vimc/vimc-sensor.c
> index e60f1985edb0..020651320ac9 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 c19093b6c787..efbc6adc34be 100644
> --- a/drivers/media/platform/vimc/vimc-streamer.c
> +++ b/drivers/media/platform/vimc/vimc-streamer.c
> @@ -124,7 +124,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 13/16] 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>
---
 drivers/media/platform/vimc/vimc-sensor.c | 48 +++++++++++++----------
 1 file changed, 28 insertions(+), 20 deletions(-)

diff --git a/drivers/media/platform/vimc/vimc-sensor.c b/drivers/media/platform/vimc/vimc-sensor.c
index 020651320ac9..33cbe2cd42ee 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_s_fourcc(&vsen->tpg, pixelformat);
 	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,32 +195,36 @@ 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;
 
 	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);
 		/*
 		 * 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]) {
+				for (i -= 1; i >= 0; i--)
+					vfree(vsen->frame.plane_addr[i]);
+				return -ENOMEM;
+			}
+		}
 
 		/* configure the test pattern generator */
 		vimc_sen_tpg_s_format(vsen);
 
 	} else {
+		for (i = 0; i < vsen->frame.num_planes; i++) {
+			vfree(vsen->frame.plane_addr[i]);
+			vsen->frame.plane_addr[i] = NULL;
+		}
 
-		vfree(vsen->frame.plane_addr[0]);
-		vsen->frame.plane_addr[0] = NULL;
 		return 0;
 	}
 
-- 
2.21.0

Re: [PATCH 13/16] media: vimc: sen: Add support for multiplanar formats

[Helen Koike]

On 3/15/19 1:43 PM, André Almeida wrote:
> 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>
> ---
>  drivers/media/platform/vimc/vimc-sensor.c | 48 +++++++++++++----------
>  1 file changed, 28 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/media/platform/vimc/vimc-sensor.c b/drivers/media/platform/vimc/vimc-sensor.c
> index 020651320ac9..33cbe2cd42ee 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_s_fourcc(&vsen->tpg, pixelformat);
>  	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);

You don't need to move this line to the top.

> +
> +	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]);

alignment

>  
> -	tpg_fill_plane_buffer(&vsen->tpg, 0, 0, vsen->frame.plane_addr[0]);
>  	return &vsen->frame;
>  }
>  
> @@ -191,32 +195,36 @@ 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;
>  
>  	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);
>  		/*
>  		 * 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]) {
> +				for (i -= 1; i >= 0; i--)
> +					vfree(vsen->frame.plane_addr[i]);
> +				return -ENOMEM;

This code is really simiar to the else block below.
I think you can use a goto here, e.g.:

			if (!vsen->frame.plane_addr[i]) {
				ret = -ENOMEM
				goto free_planes
			}
  		/* configure the test pattern generator */
  		vimc_sen_tpg_s_format(vsen);
		return 0;
	}

	i = vsen->frame.num_planes; // this can be initialized when i is
declared too

free_planes:
	for (i = 0; i < vsen->frame.num_planes; i++) {
		vfree(vsen->frame.plane_addr[i]);
		vsen->frame.plane_addr[i] = NULL;
	}
	return ret;
}


what do you think?
this also applies to the scaler.

Regards,
Helen

> +			}
> +		}
>  
>  		/* configure the test pattern generator */
>  		vimc_sen_tpg_s_format(vsen);
>  
>  	} else {
> +		for (i = 0; i < vsen->frame.num_planes; i++) {
> +			vfree(vsen->frame.plane_addr[i]);
> +			vsen->frame.plane_addr[i] = NULL;
> +		}
>  
> -		vfree(vsen->frame.plane_addr[0]);
> -		vsen->frame.plane_addr[0] = NULL;
>  		return 0;
>  	}
>  
> 

[PATCH 14/16] 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>
---
 drivers/media/platform/vimc/vimc-scaler.c | 110 ++++++++++++++--------
 1 file changed, 69 insertions(+), 41 deletions(-)

diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c
index 65519495ecca..15fbb0914056 100644
--- a/drivers/media/platform/vimc/vimc-scaler.c
+++ b/drivers/media/platform/vimc/vimc-scaler.c
@@ -35,10 +35,14 @@ MODULE_PARM_DESC(sca_mult, " the image size multiplier");
 #define IS_SRC(pad)	(pad)
 #define MAX_ZOOM	8
 
+/* TODO: enum only scalable formats */
 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 +55,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 +211,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;
 
 	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,32 +223,41 @@ 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);
+
+		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]) {
+				for (i -= 1; i >= 0; i--)
+					vfree(vsca->src_frame.plane_addr[i]);
+				return -ENOMEM;
+			}
+			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;
+		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;
@@ -270,18 +283,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 +305,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 +315,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 +367,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

Re: [PATCH 14/16] media: vimc: sca: Add support for multiplanar formats

[Helen Koike]

On 3/15/19 1:43 PM, André Almeida wrote:
> 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>
> ---
>  drivers/media/platform/vimc/vimc-scaler.c | 110 ++++++++++++++--------
>  1 file changed, 69 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c
> index 65519495ecca..15fbb0914056 100644
> --- a/drivers/media/platform/vimc/vimc-scaler.c
> +++ b/drivers/media/platform/vimc/vimc-scaler.c
> @@ -35,10 +35,14 @@ MODULE_PARM_DESC(sca_mult, " the image size multiplier");
>  #define IS_SRC(pad)	(pad)
>  #define MAX_ZOOM	8
>  
> +/* TODO: enum only scalable formats */

why is this a TODO? Isn't what it is doing already?

>  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 +55,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 +211,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;
>  
>  	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,32 +223,41 @@ 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);
> +
> +		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
> +			 */

I know this comment was already like this, but could you please also
correct the style of this comment? It should start with
/*
 * Allocate ...

Regards,
Helen

> +			vsca->src_frame.plane_addr[i] = vmalloc(frame_size);
> +			if (!vsca->src_frame.plane_addr[i]) {
> +				for (i -= 1; i >= 0; i--)
> +					vfree(vsca->src_frame.plane_addr[i]);
> +				return -ENOMEM;
> +			}
> +			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;
> +		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;
> @@ -270,18 +283,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 +305,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 +315,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 +367,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;
>  };
> 

[PATCH 15/16] 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>
---
 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 bb982761562e..50f7e71f23cd 100644
--- a/drivers/media/platform/vimc/vimc-capture.c
+++ b/drivers/media/platform/vimc/vimc-capture.c
@@ -577,6 +577,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);
@@ -599,13 +601,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 16/16] media: vimc: cap: Dynamically define device caps

[André Almeida]

The device capabilities are defined 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>
---
 drivers/media/platform/vimc/vimc-capture.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
index 50f7e71f23cd..c30a2887db33 100644
--- a/drivers/media/platform/vimc/vimc-capture.c
+++ b/drivers/media/platform/vimc/vimc-capture.c
@@ -652,7 +652,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);
@@ -697,7 +699,9 @@ static int vimc_cap_comp_bind(struct device *comp, struct device *master,
 
 	/* 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 = video_device_release_empty;
 	vdev->fops = &vimc_cap_fops;
-- 
2.21.0

Re: [PATCH 00/16] media: vimc: Add support for multiplanar formats

[André Almeida]

On 3/15/19 1:43 PM, André Almeida wrote:
> 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 (single/multi)
> and favoring code reuse.
> 
> 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.
> 

Forgot to mention that this patch series depends on this one:

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

> Thanks,
> 	André
> 
> André Almeida (16):
>    media: Move sp2mp functions to v4l2-common
>    media: vimc: Remove unnecessary stream check
>    media: vimc: Check if the stream is on using ved.stream
>    media: vimc: cap: Change vimc_cap_device.format type
>    media: vimc: Create multiplanar parameter
>    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: 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: cap: Dynamically define device caps
> 
>   drivers/media/platform/vimc/vimc-capture.c    | 310 +++++++++++++++---
>   drivers/media/platform/vimc/vimc-common.c     |  37 +++
>   drivers/media/platform/vimc/vimc-common.h     |  50 ++-
>   drivers/media/platform/vimc/vimc-core.c       |   8 +
>   drivers/media/platform/vimc/vimc-debayer.c    |  38 +--
>   drivers/media/platform/vimc/vimc-scaler.c     | 125 ++++---
>   drivers/media/platform/vimc/vimc-sensor.c     |  62 ++--
>   drivers/media/platform/vimc/vimc-streamer.c   |   2 +-
>   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                   |  31 ++
>   14 files changed, 580 insertions(+), 225 deletions(-)
> 

Re: [PATCH 00/16] media: vimc: Add support for multiplanar formats

[Helen Koike]

Hi Andre,

Thanks for you patch. I didn't do a full review, but I did some minor
comments, please check.

Thanks
Helen

On 3/15/19 2:29 PM, André Almeida wrote:
> 
> 
> On 3/15/19 1:43 PM, André Almeida wrote:
>> 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 (single/multi)
>> and favoring code reuse.
>>
>> 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.
>>
> 
> Forgot to mention that this patch series depends on this one:
> 
> "[PATCH] media: vimc: propagate pixel format in the stream"
> 
>> Thanks,
>>     André
>>
>> André Almeida (16):
>>    media: Move sp2mp functions to v4l2-common
>>    media: vimc: Remove unnecessary stream check
>>    media: vimc: Check if the stream is on using ved.stream
>>    media: vimc: cap: Change vimc_cap_device.format type
>>    media: vimc: Create multiplanar parameter
>>    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: 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: cap: Dynamically define device caps
>>
>>   drivers/media/platform/vimc/vimc-capture.c    | 310 +++++++++++++++---
>>   drivers/media/platform/vimc/vimc-common.c     |  37 +++
>>   drivers/media/platform/vimc/vimc-common.h     |  50 ++-
>>   drivers/media/platform/vimc/vimc-core.c       |   8 +
>>   drivers/media/platform/vimc/vimc-debayer.c    |  38 +--
>>   drivers/media/platform/vimc/vimc-scaler.c     | 125 ++++---
>>   drivers/media/platform/vimc/vimc-sensor.c     |  62 ++--
>>   drivers/media/platform/vimc/vimc-streamer.c   |   2 +-
>>   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                   |  31 ++
>>   14 files changed, 580 insertions(+), 225 deletions(-)
>>