[PATCH 1/3] media: imx: add capture compose rectangle

[PATCH 1/3] media: imx: add capture compose rectangle

[Philipp Zabel]

Allowing to compose captured images into larger memory buffers
will let us lift alignment restrictions on CSI crop width.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/staging/media/imx/imx-ic-prpencvf.c   |  3 +-
 drivers/staging/media/imx/imx-media-capture.c | 38 +++++++++++++++++++
 drivers/staging/media/imx/imx-media-csi.c     |  3 +-
 drivers/staging/media/imx/imx-media-vdic.c    |  4 +-
 drivers/staging/media/imx/imx-media.h         |  2 +
 5 files changed, 44 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c b/drivers/staging/media/imx/imx-ic-prpencvf.c
index 28f41caba05d..fe5a77baa592 100644
--- a/drivers/staging/media/imx/imx-ic-prpencvf.c
+++ b/drivers/staging/media/imx/imx-ic-prpencvf.c
@@ -366,8 +366,7 @@ static int prp_setup_channel(struct prp_priv *priv,
 
 	memset(&image, 0, sizeof(image));
 	image.pix = vdev->fmt.fmt.pix;
-	image.rect.width = image.pix.width;
-	image.rect.height = image.pix.height;
+	image.rect = vdev->compose;
 
 	if (rot_swap_width_height) {
 		swap(image.pix.width, image.pix.height);
diff --git a/drivers/staging/media/imx/imx-media-capture.c b/drivers/staging/media/imx/imx-media-capture.c
index b37e1186eb2f..cace8a51aca8 100644
--- a/drivers/staging/media/imx/imx-media-capture.c
+++ b/drivers/staging/media/imx/imx-media-capture.c
@@ -262,6 +262,10 @@ static int capture_s_fmt_vid_cap(struct file *file, void *fh,
 	priv->vdev.fmt.fmt.pix = f->fmt.pix;
 	priv->vdev.cc = imx_media_find_format(f->fmt.pix.pixelformat,
 					      CS_SEL_ANY, true);
+	priv->vdev.compose.left = 0;
+	priv->vdev.compose.top = 0;
+	priv->vdev.compose.width = f->fmt.fmt.pix.width;
+	priv->vdev.compose.height = f->fmt.fmt.pix.height;
 
 	return 0;
 }
@@ -290,6 +294,35 @@ static int capture_s_std(struct file *file, void *fh, v4l2_std_id std)
 	return v4l2_subdev_call(priv->src_sd, video, s_std, std);
 }
 
+static int capture_g_selection(struct file *file, void *fh,
+			       struct v4l2_selection *s)
+{
+	struct capture_priv *priv = video_drvdata(file);
+
+	switch (s->target) {
+	case V4L2_SEL_TGT_CROP:
+	case V4L2_SEL_TGT_CROP_DEFAULT:
+	case V4L2_SEL_TGT_CROP_BOUNDS:
+	case V4L2_SEL_TGT_NATIVE_SIZE:
+	case V4L2_SEL_TGT_COMPOSE:
+	case V4L2_SEL_TGT_COMPOSE_DEFAULT:
+	case V4L2_SEL_TGT_COMPOSE_BOUNDS:
+	case V4L2_SEL_TGT_COMPOSE_PADDED:
+		s->r = priv->vdev.compose;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int capture_s_selection(struct file *file, void *fh,
+			       struct v4l2_selection *s)
+{
+	return capture_g_selection(file, fh, s);
+}
+
 static int capture_g_parm(struct file *file, void *fh,
 			  struct v4l2_streamparm *a)
 {
@@ -350,6 +383,9 @@ static const struct v4l2_ioctl_ops capture_ioctl_ops = {
 	.vidioc_g_std           = capture_g_std,
 	.vidioc_s_std           = capture_s_std,
 
+	.vidioc_g_selection	= capture_g_selection,
+	.vidioc_s_selection	= capture_s_selection,
+
 	.vidioc_g_parm          = capture_g_parm,
 	.vidioc_s_parm          = capture_s_parm,
 
@@ -687,6 +723,8 @@ int imx_media_capture_device_register(struct imx_media_video_dev *vdev)
 	vdev->fmt.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
 	imx_media_mbus_fmt_to_pix_fmt(&vdev->fmt.fmt.pix,
 				      &fmt_src.format, NULL);
+	vdev->compose.width = fmt_src.format.width;
+	vdev->compose.height = fmt_src.format.height;
 	vdev->cc = imx_media_find_format(vdev->fmt.fmt.pix.pixelformat,
 					 CS_SEL_ANY, false);
 
diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
index 4223f8d418ae..c4523afe7b48 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -413,8 +413,7 @@ static int csi_idmac_setup_channel(struct csi_priv *priv)
 
 	memset(&image, 0, sizeof(image));
 	image.pix = vdev->fmt.fmt.pix;
-	image.rect.width = image.pix.width;
-	image.rect.height = image.pix.height;
+	image.rect = vdev->compose;
 
 	csi_idmac_setup_vb2_buf(priv, phys);
 
diff --git a/drivers/staging/media/imx/imx-media-vdic.c b/drivers/staging/media/imx/imx-media-vdic.c
index 482250d47e7c..e08d296cf4eb 100644
--- a/drivers/staging/media/imx/imx-media-vdic.c
+++ b/drivers/staging/media/imx/imx-media-vdic.c
@@ -263,10 +263,10 @@ static int setup_vdi_channel(struct vdic_priv *priv,
 
 	memset(&image, 0, sizeof(image));
 	image.pix = vdev->fmt.fmt.pix;
+	image.rect = vdev->compose;
 	/* one field to VDIC channels */
 	image.pix.height /= 2;
-	image.rect.width = image.pix.width;
-	image.rect.height = image.pix.height;
+	image.rect.height /= 2;
 	image.phys0 = phys0;
 	image.phys1 = phys1;
 
diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h
index bc7feb81937c..7a0e658753f0 100644
--- a/drivers/staging/media/imx/imx-media.h
+++ b/drivers/staging/media/imx/imx-media.h
@@ -80,6 +80,8 @@ struct imx_media_video_dev {
 
 	/* the user format */
 	struct v4l2_format fmt;
+	/* the compose rectangle */
+	struct v4l2_rect compose;
 	const struct imx_media_pixfmt *cc;
 
 	/* links this vdev to master list */
-- 
2.19.1

[PATCH 2/3] media: imx: set compose rectangle to mbus format

[Philipp Zabel]

Prepare for mbus format being smaller than the written rectangle
due to burst size.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/staging/media/imx/imx-media-capture.c | 55 +++++++++++++------
 1 file changed, 38 insertions(+), 17 deletions(-)

diff --git a/drivers/staging/media/imx/imx-media-capture.c b/drivers/staging/media/imx/imx-media-capture.c
index cace8a51aca8..2d49d9573056 100644
--- a/drivers/staging/media/imx/imx-media-capture.c
+++ b/drivers/staging/media/imx/imx-media-capture.c
@@ -203,21 +203,14 @@ static int capture_g_fmt_vid_cap(struct file *file, void *fh,
 	return 0;
 }
 
-static int capture_try_fmt_vid_cap(struct file *file, void *fh,
-				   struct v4l2_format *f)
+static int __capture_try_fmt_vid_cap(struct capture_priv *priv,
+				     struct v4l2_subev_format *fmt_src,
+				     struct v4l2_format *f)
 {
 	struct capture_priv *priv = video_drvdata(file);
-	struct v4l2_subdev_format fmt_src;
 	const struct imx_media_pixfmt *cc, *cc_src;
-	int ret;
-
-	fmt_src.pad = priv->src_sd_pad;
-	fmt_src.which = V4L2_SUBDEV_FORMAT_ACTIVE;
-	ret = v4l2_subdev_call(priv->src_sd, pad, get_fmt, NULL, &fmt_src);
-	if (ret)
-		return ret;
 
-	cc_src = imx_media_find_ipu_format(fmt_src.format.code, CS_SEL_ANY);
+	cc_src = imx_media_find_ipu_format(fmt_src->format.code, CS_SEL_ANY);
 	if (cc_src) {
 		u32 fourcc, cs_sel;
 
@@ -231,7 +224,7 @@ static int capture_try_fmt_vid_cap(struct file *file, void *fh,
 			cc = imx_media_find_format(fourcc, cs_sel, false);
 		}
 	} else {
-		cc_src = imx_media_find_mbus_format(fmt_src.format.code,
+		cc_src = imx_media_find_mbus_format(fmt_src->format.code,
 						    CS_SEL_ANY, true);
 		if (WARN_ON(!cc_src))
 			return -EINVAL;
@@ -239,15 +232,32 @@ static int capture_try_fmt_vid_cap(struct file *file, void *fh,
 		cc = cc_src;
 	}
 
-	imx_media_mbus_fmt_to_pix_fmt(&f->fmt.pix, &fmt_src.format, cc);
+	imx_media_mbus_fmt_to_pix_fmt(&f->fmt.pix, &fmt_src->format, cc);
 
 	return 0;
 }
 
+static int capture_try_fmt_vid_cap(struct file *file, void *fh,
+				   struct v4l2_format *f)
+{
+	struct capture_priv *priv = video_drvdata(file);
+	struct v4l2_subdev_format fmt_src;
+	int ret;
+
+	fmt_src.pad = priv->src_sd_pad;
+	fmt_src.which = V4L2_SUBDEV_FORMAT_ACTIVE;
+	ret = v4l2_subdev_call(priv->src_sd, pad, get_fmt, NULL, &fmt_src);
+	if (ret)
+		return ret;
+
+	return __capture_try_fmt(priv, &fmt_src, f);
+}
+
 static int capture_s_fmt_vid_cap(struct file *file, void *fh,
 				 struct v4l2_format *f)
 {
 	struct capture_priv *priv = video_drvdata(file);
+	struct v4l2_subdev_format fmt_src;
 	int ret;
 
 	if (vb2_is_busy(&priv->q)) {
@@ -255,7 +265,13 @@ static int capture_s_fmt_vid_cap(struct file *file, void *fh,
 		return -EBUSY;
 	}
 
-	ret = capture_try_fmt_vid_cap(file, priv, f);
+	fmt_src.pad = priv->src_sd_pad;
+	fmt_src.which = V4L2_SUBDEV_FORMAT_ACTIVE;
+	ret = v4l2_subdev_call(priv->src_sd, pad, get_fmt, NULL, &fmt_src);
+	if (ret)
+		return ret;
+
+	ret = __capture_try_fmt_vid_cap(priv, &fmt_src, f);
 	if (ret)
 		return ret;
 
@@ -264,8 +280,8 @@ static int capture_s_fmt_vid_cap(struct file *file, void *fh,
 					      CS_SEL_ANY, true);
 	priv->vdev.compose.left = 0;
 	priv->vdev.compose.top = 0;
-	priv->vdev.compose.width = f->fmt.fmt.pix.width;
-	priv->vdev.compose.height = f->fmt.fmt.pix.height;
+	priv->vdev.compose.width = fmt_src.width;
+	priv->vdev.compose.height = fmt_src.height;
 
 	return 0;
 }
@@ -307,9 +323,14 @@ static int capture_g_selection(struct file *file, void *fh,
 	case V4L2_SEL_TGT_COMPOSE:
 	case V4L2_SEL_TGT_COMPOSE_DEFAULT:
 	case V4L2_SEL_TGT_COMPOSE_BOUNDS:
-	case V4L2_SEL_TGT_COMPOSE_PADDED:
 		s->r = priv->vdev.compose;
 		break;
+	case V4L2_SEL_TGT_COMPOSE_PADDED:
+		s->r.left = 0;
+		s->r.top = 0;
+		s->r.width = priv->vdev.fmt.fmt.pix.width;
+		s->r.height = priv->vdev.fmt.fmt.pix.height;
+		break;
 	default:
 		return -EINVAL;
 	}
-- 
2.19.1

[PATCH 3/3] media: imx: lift CSI width alignment restriction

[Philipp Zabel]

The CSI subdevice shouldn't have to care about IDMAC line start
address alignment. With compose rectangle support in the capture
driver, it doesn't have to anymore.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/staging/media/imx/imx-media-capture.c |  9 ++++-----
 drivers/staging/media/imx/imx-media-csi.c     |  2 +-
 drivers/staging/media/imx/imx-media-utils.c   | 15 ++++++++++++---
 3 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/media/imx/imx-media-capture.c b/drivers/staging/media/imx/imx-media-capture.c
index 2d49d9573056..f87d6e8019e5 100644
--- a/drivers/staging/media/imx/imx-media-capture.c
+++ b/drivers/staging/media/imx/imx-media-capture.c
@@ -204,10 +204,9 @@ static int capture_g_fmt_vid_cap(struct file *file, void *fh,
 }
 
 static int __capture_try_fmt_vid_cap(struct capture_priv *priv,
-				     struct v4l2_subev_format *fmt_src,
+				     struct v4l2_subdev_format *fmt_src,
 				     struct v4l2_format *f)
 {
-	struct capture_priv *priv = video_drvdata(file);
 	const struct imx_media_pixfmt *cc, *cc_src;
 
 	cc_src = imx_media_find_ipu_format(fmt_src->format.code, CS_SEL_ANY);
@@ -250,7 +249,7 @@ static int capture_try_fmt_vid_cap(struct file *file, void *fh,
 	if (ret)
 		return ret;
 
-	return __capture_try_fmt(priv, &fmt_src, f);
+	return __capture_try_fmt_vid_cap(priv, &fmt_src, f);
 }
 
 static int capture_s_fmt_vid_cap(struct file *file, void *fh,
@@ -280,8 +279,8 @@ static int capture_s_fmt_vid_cap(struct file *file, void *fh,
 					      CS_SEL_ANY, true);
 	priv->vdev.compose.left = 0;
 	priv->vdev.compose.top = 0;
-	priv->vdev.compose.width = fmt_src.width;
-	priv->vdev.compose.height = fmt_src.height;
+	priv->vdev.compose.width = fmt_src.format.width;
+	priv->vdev.compose.height = fmt_src.format.height;
 
 	return 0;
 }
diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
index c4523afe7b48..d39682192a67 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -41,7 +41,7 @@
 #define MIN_H       144
 #define MAX_W      4096
 #define MAX_H      4096
-#define W_ALIGN    4 /* multiple of 16 pixels */
+#define W_ALIGN    1 /* multiple of 2 pixels */
 #define H_ALIGN    1 /* multiple of 2 lines */
 #define S_ALIGN    1 /* multiple of 2 */
 
diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
index 0eaa353d5cb3..5f110d90a4ef 100644
--- a/drivers/staging/media/imx/imx-media-utils.c
+++ b/drivers/staging/media/imx/imx-media-utils.c
@@ -580,6 +580,7 @@ int imx_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
 				  struct v4l2_mbus_framefmt *mbus,
 				  const struct imx_media_pixfmt *cc)
 {
+	u32 width;
 	u32 stride;
 
 	if (!cc) {
@@ -602,9 +603,16 @@ int imx_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
 		cc = imx_media_find_mbus_format(code, CS_SEL_YUV, false);
 	}
 
-	stride = cc->planar ? mbus->width : (mbus->width * cc->bpp) >> 3;
+	/* Round up width for minimum burst size */
+	width = round_up(mbus->width, 8);
 
-	pix->width = mbus->width;
+	/* Round up stride for IDMAC line start address alignment */
+	if (cc->planar)
+		stride = round_up(width, 16);
+	else
+		stride = round_up((width * cc->bpp) >> 3, 8);
+
+	pix->width = width;
 	pix->height = mbus->height;
 	pix->pixelformat = cc->fourcc;
 	pix->colorspace = mbus->colorspace;
@@ -613,7 +621,8 @@ int imx_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
 	pix->quantization = mbus->quantization;
 	pix->field = mbus->field;
 	pix->bytesperline = stride;
-	pix->sizeimage = (pix->width * pix->height * cc->bpp) >> 3;
+	pix->sizeimage = cc->planar ? ((stride * pix->height * cc->bpp) >> 3) :
+			 stride * pix->height;
 
 	return 0;
 }
-- 
2.19.1

Re: [PATCH 1/3] media: imx: add capture compose rectangle

[Sakari Ailus]

Hi Philipp,

On Mon, Nov 05, 2018 at 04:20:53PM +0100, Philipp Zabel wrote:
> Allowing to compose captured images into larger memory buffers
> will let us lift alignment restrictions on CSI crop width.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  drivers/staging/media/imx/imx-ic-prpencvf.c   |  3 +-
>  drivers/staging/media/imx/imx-media-capture.c | 38 +++++++++++++++++++
>  drivers/staging/media/imx/imx-media-csi.c     |  3 +-
>  drivers/staging/media/imx/imx-media-vdic.c    |  4 +-
>  drivers/staging/media/imx/imx-media.h         |  2 +
>  5 files changed, 44 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c b/drivers/staging/media/imx/imx-ic-prpencvf.c
> index 28f41caba05d..fe5a77baa592 100644
> --- a/drivers/staging/media/imx/imx-ic-prpencvf.c
> +++ b/drivers/staging/media/imx/imx-ic-prpencvf.c
> @@ -366,8 +366,7 @@ static int prp_setup_channel(struct prp_priv *priv,
>  
>  	memset(&image, 0, sizeof(image));
>  	image.pix = vdev->fmt.fmt.pix;
> -	image.rect.width = image.pix.width;
> -	image.rect.height = image.pix.height;
> +	image.rect = vdev->compose;
>  
>  	if (rot_swap_width_height) {
>  		swap(image.pix.width, image.pix.height);
> diff --git a/drivers/staging/media/imx/imx-media-capture.c b/drivers/staging/media/imx/imx-media-capture.c
> index b37e1186eb2f..cace8a51aca8 100644
> --- a/drivers/staging/media/imx/imx-media-capture.c
> +++ b/drivers/staging/media/imx/imx-media-capture.c
> @@ -262,6 +262,10 @@ static int capture_s_fmt_vid_cap(struct file *file, void *fh,
>  	priv->vdev.fmt.fmt.pix = f->fmt.pix;
>  	priv->vdev.cc = imx_media_find_format(f->fmt.pix.pixelformat,
>  					      CS_SEL_ANY, true);
> +	priv->vdev.compose.left = 0;
> +	priv->vdev.compose.top = 0;
> +	priv->vdev.compose.width = f->fmt.fmt.pix.width;
> +	priv->vdev.compose.height = f->fmt.fmt.pix.height;
>  
>  	return 0;
>  }
> @@ -290,6 +294,35 @@ static int capture_s_std(struct file *file, void *fh, v4l2_std_id std)
>  	return v4l2_subdev_call(priv->src_sd, video, s_std, std);
>  }
>  
> +static int capture_g_selection(struct file *file, void *fh,
> +			       struct v4l2_selection *s)
> +{
> +	struct capture_priv *priv = video_drvdata(file);
> +
> +	switch (s->target) {
> +	case V4L2_SEL_TGT_CROP:
> +	case V4L2_SEL_TGT_CROP_DEFAULT:
> +	case V4L2_SEL_TGT_CROP_BOUNDS:
> +	case V4L2_SEL_TGT_NATIVE_SIZE:

The NATIVE_SIZE is for devices such as sensors. It doesn't make sense here.

With that removed,

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

> +	case V4L2_SEL_TGT_COMPOSE:
> +	case V4L2_SEL_TGT_COMPOSE_DEFAULT:
> +	case V4L2_SEL_TGT_COMPOSE_BOUNDS:
> +	case V4L2_SEL_TGT_COMPOSE_PADDED:
> +		s->r = priv->vdev.compose;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
 > +
> +static int capture_s_selection(struct file *file, void *fh,
> +			       struct v4l2_selection *s)
> +{
> +	return capture_g_selection(file, fh, s);
> +}
> +
>  static int capture_g_parm(struct file *file, void *fh,
>  			  struct v4l2_streamparm *a)
>  {
> @@ -350,6 +383,9 @@ static const struct v4l2_ioctl_ops capture_ioctl_ops = {
>  	.vidioc_g_std           = capture_g_std,
>  	.vidioc_s_std           = capture_s_std,
>  
> +	.vidioc_g_selection	= capture_g_selection,
> +	.vidioc_s_selection	= capture_s_selection,
> +
>  	.vidioc_g_parm          = capture_g_parm,
>  	.vidioc_s_parm          = capture_s_parm,
>  
> @@ -687,6 +723,8 @@ int imx_media_capture_device_register(struct imx_media_video_dev *vdev)
>  	vdev->fmt.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>  	imx_media_mbus_fmt_to_pix_fmt(&vdev->fmt.fmt.pix,
>  				      &fmt_src.format, NULL);
> +	vdev->compose.width = fmt_src.format.width;
> +	vdev->compose.height = fmt_src.format.height;
>  	vdev->cc = imx_media_find_format(vdev->fmt.fmt.pix.pixelformat,
>  					 CS_SEL_ANY, false);
>  
> diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
> index 4223f8d418ae..c4523afe7b48 100644
> --- a/drivers/staging/media/imx/imx-media-csi.c
> +++ b/drivers/staging/media/imx/imx-media-csi.c
> @@ -413,8 +413,7 @@ static int csi_idmac_setup_channel(struct csi_priv *priv)
>  
>  	memset(&image, 0, sizeof(image));
>  	image.pix = vdev->fmt.fmt.pix;
> -	image.rect.width = image.pix.width;
> -	image.rect.height = image.pix.height;
> +	image.rect = vdev->compose;
>  
>  	csi_idmac_setup_vb2_buf(priv, phys);
>  
> diff --git a/drivers/staging/media/imx/imx-media-vdic.c b/drivers/staging/media/imx/imx-media-vdic.c
> index 482250d47e7c..e08d296cf4eb 100644
> --- a/drivers/staging/media/imx/imx-media-vdic.c
> +++ b/drivers/staging/media/imx/imx-media-vdic.c
> @@ -263,10 +263,10 @@ static int setup_vdi_channel(struct vdic_priv *priv,
>  
>  	memset(&image, 0, sizeof(image));
>  	image.pix = vdev->fmt.fmt.pix;
> +	image.rect = vdev->compose;
>  	/* one field to VDIC channels */
>  	image.pix.height /= 2;
> -	image.rect.width = image.pix.width;
> -	image.rect.height = image.pix.height;
> +	image.rect.height /= 2;
>  	image.phys0 = phys0;
>  	image.phys1 = phys1;
>  
> diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h
> index bc7feb81937c..7a0e658753f0 100644
> --- a/drivers/staging/media/imx/imx-media.h
> +++ b/drivers/staging/media/imx/imx-media.h
> @@ -80,6 +80,8 @@ struct imx_media_video_dev {
>  
>  	/* the user format */
>  	struct v4l2_format fmt;
> +	/* the compose rectangle */
> +	struct v4l2_rect compose;
>  	const struct imx_media_pixfmt *cc;
>  
>  	/* links this vdev to master list */

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi

Re: [PATCH 1/3] media: imx: add capture compose rectangle

[Philipp Zabel]

Hi Sakari,

On Tue, 2018-11-06 at 16:01 +0200, Sakari Ailus wrote:
[...]
> @@ -290,6 +294,35 @@ static int capture_s_std(struct file *file, void *fh, v4l2_std_id std)
> >  	return v4l2_subdev_call(priv->src_sd, video, s_std, std);
> >  }
> >  
> > +static int capture_g_selection(struct file *file, void *fh,
> > +			       struct v4l2_selection *s)
> > +{
> > +	struct capture_priv *priv = video_drvdata(file);
> > +
> > +	switch (s->target) {
> > +	case V4L2_SEL_TGT_CROP:
> > +	case V4L2_SEL_TGT_CROP_DEFAULT:
> > +	case V4L2_SEL_TGT_CROP_BOUNDS:
> > +	case V4L2_SEL_TGT_NATIVE_SIZE:
> 
> The NATIVE_SIZE is for devices such as sensors. It doesn't make sense here.

Should this be documented in Documentation/media/uapi/v4l/v4l2-
selection-targets.rst ? There it only mentions when to make it
writeable.

> With that removed,
> 
> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Thank you, I'll remove that line.

regards
Philipp

Re: [PATCH 1/3] media: imx: add capture compose rectangle

[Sakari Ailus]

Hi Philipp,

On Tue, Nov 06, 2018 at 03:44:07PM +0100, Philipp Zabel wrote:
> Hi Sakari,
> 
> On Tue, 2018-11-06 at 16:01 +0200, Sakari Ailus wrote:
> [...]
> > @@ -290,6 +294,35 @@ static int capture_s_std(struct file *file, void *fh, v4l2_std_id std)
> > >  	return v4l2_subdev_call(priv->src_sd, video, s_std, std);
> > >  }
> > >  
> > > +static int capture_g_selection(struct file *file, void *fh,
> > > +			       struct v4l2_selection *s)
> > > +{
> > > +	struct capture_priv *priv = video_drvdata(file);
> > > +
> > > +	switch (s->target) {
> > > +	case V4L2_SEL_TGT_CROP:
> > > +	case V4L2_SEL_TGT_CROP_DEFAULT:
> > > +	case V4L2_SEL_TGT_CROP_BOUNDS:
> > > +	case V4L2_SEL_TGT_NATIVE_SIZE:
> > 
> > The NATIVE_SIZE is for devices such as sensors. It doesn't make sense here.
> 
> Should this be documented in Documentation/media/uapi/v4l/v4l2-
> selection-targets.rst ? There it only mentions when to make it
> writeable.

This seems to have originated from the documentation before the ReST
conversion and I had hard time to figure out where the current text (apart
from sensor pixel array) came from. There is also no driver using it in
that meaning, and I doubt if the use is not already been covered by the
compose rectangle.

This indeed requires some follow-up, but that's out of scope of your set.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi

Re: [PATCH 1/3] media: imx: add capture compose rectangle

[Steve Longerbeam]

Hi Philipp,

On 11/5/18 7:20 AM, Philipp Zabel wrote:
> Allowing to compose captured images into larger memory buffers
> will let us lift alignment restrictions on CSI crop width.
>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>   drivers/staging/media/imx/imx-ic-prpencvf.c   |  3 +-
>   drivers/staging/media/imx/imx-media-capture.c | 38 +++++++++++++++++++
>   drivers/staging/media/imx/imx-media-csi.c     |  3 +-
>   drivers/staging/media/imx/imx-media-vdic.c    |  4 +-
>   drivers/staging/media/imx/imx-media.h         |  2 +
>   5 files changed, 44 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c b/drivers/staging/media/imx/imx-ic-prpencvf.c
> index 28f41caba05d..fe5a77baa592 100644
> --- a/drivers/staging/media/imx/imx-ic-prpencvf.c
> +++ b/drivers/staging/media/imx/imx-ic-prpencvf.c
> @@ -366,8 +366,7 @@ static int prp_setup_channel(struct prp_priv *priv,
>   
>   	memset(&image, 0, sizeof(image));
>   	image.pix = vdev->fmt.fmt.pix;
> -	image.rect.width = image.pix.width;
> -	image.rect.height = image.pix.height;
> +	image.rect = vdev->compose;
>   
>   	if (rot_swap_width_height) {
>   		swap(image.pix.width, image.pix.height);
> diff --git a/drivers/staging/media/imx/imx-media-capture.c b/drivers/staging/media/imx/imx-media-capture.c
> index b37e1186eb2f..cace8a51aca8 100644
> --- a/drivers/staging/media/imx/imx-media-capture.c
> +++ b/drivers/staging/media/imx/imx-media-capture.c
> @@ -262,6 +262,10 @@ static int capture_s_fmt_vid_cap(struct file *file, void *fh,
>   	priv->vdev.fmt.fmt.pix = f->fmt.pix;
>   	priv->vdev.cc = imx_media_find_format(f->fmt.pix.pixelformat,
>   					      CS_SEL_ANY, true);
> +	priv->vdev.compose.left = 0;
> +	priv->vdev.compose.top = 0;
> +	priv->vdev.compose.width = f->fmt.fmt.pix.width;
> +	priv->vdev.compose.height = f->fmt.fmt.pix.height;


this should be:

priv->vdev.compose.width = fmt_src.format.width;
priv->vdev.compose.height = fmt_src.format.height;

(corrected in the next patches but needs to be corrected here).

>   
>   	return 0;
>   }
> @@ -290,6 +294,35 @@ static int capture_s_std(struct file *file, void *fh, v4l2_std_id std)
>   	return v4l2_subdev_call(priv->src_sd, video, s_std, std);
>   }
>   
> +static int capture_g_selection(struct file *file, void *fh,
> +			       struct v4l2_selection *s)
> +{
> +	struct capture_priv *priv = video_drvdata(file);
> +
> +	switch (s->target) {
> +	case V4L2_SEL_TGT_CROP:
> +	case V4L2_SEL_TGT_CROP_DEFAULT:
> +	case V4L2_SEL_TGT_CROP_BOUNDS:
> +	case V4L2_SEL_TGT_NATIVE_SIZE:
> +	case V4L2_SEL_TGT_COMPOSE:
> +	case V4L2_SEL_TGT_COMPOSE_DEFAULT:
> +	case V4L2_SEL_TGT_COMPOSE_BOUNDS:
> +	case V4L2_SEL_TGT_COMPOSE_PADDED:
> +		s->r = priv->vdev.compose;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int capture_s_selection(struct file *file, void *fh,
> +			       struct v4l2_selection *s)
> +{
> +	return capture_g_selection(file, fh, s);
> +}
> +
>   static int capture_g_parm(struct file *file, void *fh,
>   			  struct v4l2_streamparm *a)
>   {
> @@ -350,6 +383,9 @@ static const struct v4l2_ioctl_ops capture_ioctl_ops = {
>   	.vidioc_g_std           = capture_g_std,
>   	.vidioc_s_std           = capture_s_std,
>   
> +	.vidioc_g_selection	= capture_g_selection,
> +	.vidioc_s_selection	= capture_s_selection,
> +
>   	.vidioc_g_parm          = capture_g_parm,
>   	.vidioc_s_parm          = capture_s_parm,
>   
> @@ -687,6 +723,8 @@ int imx_media_capture_device_register(struct imx_media_video_dev *vdev)
>   	vdev->fmt.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>   	imx_media_mbus_fmt_to_pix_fmt(&vdev->fmt.fmt.pix,
>   				      &fmt_src.format, NULL);
> +	vdev->compose.width = fmt_src.format.width;
> +	vdev->compose.height = fmt_src.format.height;
>   	vdev->cc = imx_media_find_format(vdev->fmt.fmt.pix.pixelformat,
>   					 CS_SEL_ANY, false);
>   
> diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
> index 4223f8d418ae..c4523afe7b48 100644
> --- a/drivers/staging/media/imx/imx-media-csi.c
> +++ b/drivers/staging/media/imx/imx-media-csi.c
> @@ -413,8 +413,7 @@ static int csi_idmac_setup_channel(struct csi_priv *priv)
>   
>   	memset(&image, 0, sizeof(image));
>   	image.pix = vdev->fmt.fmt.pix;
> -	image.rect.width = image.pix.width;
> -	image.rect.height = image.pix.height;
> +	image.rect = vdev->compose;
>   
>   	csi_idmac_setup_vb2_buf(priv, phys);
>   
> diff --git a/drivers/staging/media/imx/imx-media-vdic.c b/drivers/staging/media/imx/imx-media-vdic.c
> index 482250d47e7c..e08d296cf4eb 100644
> --- a/drivers/staging/media/imx/imx-media-vdic.c
> +++ b/drivers/staging/media/imx/imx-media-vdic.c
> @@ -263,10 +263,10 @@ static int setup_vdi_channel(struct vdic_priv *priv,
>   
>   	memset(&image, 0, sizeof(image));
>   	image.pix = vdev->fmt.fmt.pix;
> +	image.rect = vdev->compose;
>   	/* one field to VDIC channels */
>   	image.pix.height /= 2;
> -	image.rect.width = image.pix.width;
> -	image.rect.height = image.pix.height;
> +	image.rect.height /= 2;
>   	image.phys0 = phys0;
>   	image.phys1 = phys1;
>   
> diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h
> index bc7feb81937c..7a0e658753f0 100644
> --- a/drivers/staging/media/imx/imx-media.h
> +++ b/drivers/staging/media/imx/imx-media.h
> @@ -80,6 +80,8 @@ struct imx_media_video_dev {
>   
>   	/* the user format */
>   	struct v4l2_format fmt;
> +	/* the compose rectangle */
> +	struct v4l2_rect compose;
>   	const struct imx_media_pixfmt *cc;
>   
>   	/* links this vdev to master list */

Re: [PATCH 2/3] media: imx: set compose rectangle to mbus format

[Steve Longerbeam]

Hi Philipp,


On 11/5/18 7:20 AM, Philipp Zabel wrote:
> Prepare for mbus format being smaller than the written rectangle
> due to burst size.
>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>   drivers/staging/media/imx/imx-media-capture.c | 55 +++++++++++++------
>   1 file changed, 38 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/staging/media/imx/imx-media-capture.c b/drivers/staging/media/imx/imx-media-capture.c
> index cace8a51aca8..2d49d9573056 100644
> --- a/drivers/staging/media/imx/imx-media-capture.c
> +++ b/drivers/staging/media/imx/imx-media-capture.c
> @@ -203,21 +203,14 @@ static int capture_g_fmt_vid_cap(struct file *file, void *fh,
>   	return 0;
>   }
>   
> -static int capture_try_fmt_vid_cap(struct file *file, void *fh,
> -				   struct v4l2_format *f)
> +static int __capture_try_fmt_vid_cap(struct capture_priv *priv,
> +				     struct v4l2_subev_format *fmt_src,


typo: struct v4l2_subdev_format *fmt_src,


> +				     struct v4l2_format *f)
>   {
>   	struct capture_priv *priv = video_drvdata(file);
> -	struct v4l2_subdev_format fmt_src;
>   	const struct imx_media_pixfmt *cc, *cc_src;
> -	int ret;
> -
> -	fmt_src.pad = priv->src_sd_pad;
> -	fmt_src.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> -	ret = v4l2_subdev_call(priv->src_sd, pad, get_fmt, NULL, &fmt_src);
> -	if (ret)
> -		return ret;
>   
> -	cc_src = imx_media_find_ipu_format(fmt_src.format.code, CS_SEL_ANY);
> +	cc_src = imx_media_find_ipu_format(fmt_src->format.code, CS_SEL_ANY);
>   	if (cc_src) {
>   		u32 fourcc, cs_sel;
>   
> @@ -231,7 +224,7 @@ static int capture_try_fmt_vid_cap(struct file *file, void *fh,
>   			cc = imx_media_find_format(fourcc, cs_sel, false);
>   		}
>   	} else {
> -		cc_src = imx_media_find_mbus_format(fmt_src.format.code,
> +		cc_src = imx_media_find_mbus_format(fmt_src->format.code,
>   						    CS_SEL_ANY, true);
>   		if (WARN_ON(!cc_src))
>   			return -EINVAL;
> @@ -239,15 +232,32 @@ static int capture_try_fmt_vid_cap(struct file *file, void *fh,
>   		cc = cc_src;
>   	}
>   
> -	imx_media_mbus_fmt_to_pix_fmt(&f->fmt.pix, &fmt_src.format, cc);
> +	imx_media_mbus_fmt_to_pix_fmt(&f->fmt.pix, &fmt_src->format, cc);
>   
>   	return 0;
>   }
>   
> +static int capture_try_fmt_vid_cap(struct file *file, void *fh,
> +				   struct v4l2_format *f)
> +{
> +	struct capture_priv *priv = video_drvdata(file);
> +	struct v4l2_subdev_format fmt_src;
> +	int ret;
> +
> +	fmt_src.pad = priv->src_sd_pad;
> +	fmt_src.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> +	ret = v4l2_subdev_call(priv->src_sd, pad, get_fmt, NULL, &fmt_src);
> +	if (ret)
> +		return ret;
> +
> +	return __capture_try_fmt(priv, &fmt_src, f);


typo: return __capture_try_fmt_vid_cap(priv, &fmt_src, f);


> +}
> +
>   static int capture_s_fmt_vid_cap(struct file *file, void *fh,
>   				 struct v4l2_format *f)
>   {
>   	struct capture_priv *priv = video_drvdata(file);
> +	struct v4l2_subdev_format fmt_src;
>   	int ret;
>   
>   	if (vb2_is_busy(&priv->q)) {
> @@ -255,7 +265,13 @@ static int capture_s_fmt_vid_cap(struct file *file, void *fh,
>   		return -EBUSY;
>   	}
>   
> -	ret = capture_try_fmt_vid_cap(file, priv, f);
> +	fmt_src.pad = priv->src_sd_pad;
> +	fmt_src.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> +	ret = v4l2_subdev_call(priv->src_sd, pad, get_fmt, NULL, &fmt_src);
> +	if (ret)
> +		return ret;
> +
> +	ret = __capture_try_fmt_vid_cap(priv, &fmt_src, f);
>   	if (ret)
>   		return ret;
>   
> @@ -264,8 +280,8 @@ static int capture_s_fmt_vid_cap(struct file *file, void *fh,
>   					      CS_SEL_ANY, true);
>   	priv->vdev.compose.left = 0;
>   	priv->vdev.compose.top = 0;
> -	priv->vdev.compose.width = f->fmt.fmt.pix.width;
> -	priv->vdev.compose.height = f->fmt.fmt.pix.height;
> +	priv->vdev.compose.width = fmt_src.width;
> +	priv->vdev.compose.height = fmt_src.height;
>   
>   	return 0;
>   }
> @@ -307,9 +323,14 @@ static int capture_g_selection(struct file *file, void *fh,
>   	case V4L2_SEL_TGT_COMPOSE:
>   	case V4L2_SEL_TGT_COMPOSE_DEFAULT:
>   	case V4L2_SEL_TGT_COMPOSE_BOUNDS:
> -	case V4L2_SEL_TGT_COMPOSE_PADDED:
>   		s->r = priv->vdev.compose;
>   		break;
> +	case V4L2_SEL_TGT_COMPOSE_PADDED:
> +		s->r.left = 0;
> +		s->r.top = 0;
> +		s->r.width = priv->vdev.fmt.fmt.pix.width;
> +		s->r.height = priv->vdev.fmt.fmt.pix.height;
> +		break;
>   	default:
>   		return -EINVAL;
>   	}

Re: [PATCH 3/3] media: imx: lift CSI width alignment restriction

[Steve Longerbeam]

On 11/5/18 7:20 AM, Philipp Zabel wrote:
> The CSI subdevice shouldn't have to care about IDMAC line start
> address alignment. With compose rectangle support in the capture
> driver, it doesn't have to anymore.
>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>   drivers/staging/media/imx/imx-media-capture.c |  9 ++++-----
>   drivers/staging/media/imx/imx-media-csi.c     |  2 +-
>   drivers/staging/media/imx/imx-media-utils.c   | 15 ++++++++++++---
>   3 files changed, 17 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/staging/media/imx/imx-media-capture.c b/drivers/staging/media/imx/imx-media-capture.c
> index 2d49d9573056..f87d6e8019e5 100644
> --- a/drivers/staging/media/imx/imx-media-capture.c
> +++ b/drivers/staging/media/imx/imx-media-capture.c
> @@ -204,10 +204,9 @@ static int capture_g_fmt_vid_cap(struct file *file, void *fh,
>   }
>   
>   static int __capture_try_fmt_vid_cap(struct capture_priv *priv,
> -				     struct v4l2_subev_format *fmt_src,
> +				     struct v4l2_subdev_format *fmt_src,
>   				     struct v4l2_format *f)
>   {
> -	struct capture_priv *priv = video_drvdata(file);
>   	const struct imx_media_pixfmt *cc, *cc_src;
>   
>   	cc_src = imx_media_find_ipu_format(fmt_src->format.code, CS_SEL_ANY);
> @@ -250,7 +249,7 @@ static int capture_try_fmt_vid_cap(struct file *file, void *fh,
>   	if (ret)
>   		return ret;
>   
> -	return __capture_try_fmt(priv, &fmt_src, f);
> +	return __capture_try_fmt_vid_cap(priv, &fmt_src, f);
>   }
>   
>   static int capture_s_fmt_vid_cap(struct file *file, void *fh,
> @@ -280,8 +279,8 @@ static int capture_s_fmt_vid_cap(struct file *file, void *fh,
>   					      CS_SEL_ANY, true);
>   	priv->vdev.compose.left = 0;
>   	priv->vdev.compose.top = 0;
> -	priv->vdev.compose.width = fmt_src.width;
> -	priv->vdev.compose.height = fmt_src.height;
> +	priv->vdev.compose.width = fmt_src.format.width;
> +	priv->vdev.compose.height = fmt_src.format.height;
>   
>   	return 0;
>   }
> diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
> index c4523afe7b48..d39682192a67 100644
> --- a/drivers/staging/media/imx/imx-media-csi.c
> +++ b/drivers/staging/media/imx/imx-media-csi.c
> @@ -41,7 +41,7 @@
>   #define MIN_H       144
>   #define MAX_W      4096
>   #define MAX_H      4096
> -#define W_ALIGN    4 /* multiple of 16 pixels */
> +#define W_ALIGN    1 /* multiple of 2 pixels */


This works for the IDMAC output pad because the channel's cpmem width 
and stride can be rounded up, but width align at the CSI sink still 
needs to be 8 pixels when directed to the IC via the CSI_SRC_PAD_DIRECT 
pad, in order to support the 8x8 block rotator in the IC PRP, and 
there's no way AFAIK to do the same trick of rounding up width and 
stride for non-IDMAC direct paths through the IPU.

Also, the imx-ic-prpencvf.c W_ALIGN_SRC can be relaxed to 2 pixels as well.

Steve


>   #define H_ALIGN    1 /* multiple of 2 lines */
>   #define S_ALIGN    1 /* multiple of 2 */
>   
> diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
> index 0eaa353d5cb3..5f110d90a4ef 100644
> --- a/drivers/staging/media/imx/imx-media-utils.c
> +++ b/drivers/staging/media/imx/imx-media-utils.c
> @@ -580,6 +580,7 @@ int imx_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
>   				  struct v4l2_mbus_framefmt *mbus,
>   				  const struct imx_media_pixfmt *cc)
>   {
> +	u32 width;
>   	u32 stride;
>   
>   	if (!cc) {
> @@ -602,9 +603,16 @@ int imx_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
>   		cc = imx_media_find_mbus_format(code, CS_SEL_YUV, false);
>   	}
>   
> -	stride = cc->planar ? mbus->width : (mbus->width * cc->bpp) >> 3;
> +	/* Round up width for minimum burst size */
> +	width = round_up(mbus->width, 8);
>   
> -	pix->width = mbus->width;
> +	/* Round up stride for IDMAC line start address alignment */
> +	if (cc->planar)
> +		stride = round_up(width, 16);
> +	else
> +		stride = round_up((width * cc->bpp) >> 3, 8);
> +
> +	pix->width = width;
>   	pix->height = mbus->height;
>   	pix->pixelformat = cc->fourcc;
>   	pix->colorspace = mbus->colorspace;
> @@ -613,7 +621,8 @@ int imx_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
>   	pix->quantization = mbus->quantization;
>   	pix->field = mbus->field;
>   	pix->bytesperline = stride;
> -	pix->sizeimage = (pix->width * pix->height * cc->bpp) >> 3;
> +	pix->sizeimage = cc->planar ? ((stride * pix->height * cc->bpp) >> 3) :
> +			 stride * pix->height;
>   
>   	return 0;
>   }

Re: [PATCH 1/3] media: imx: add capture compose rectangle

[Philipp Zabel]

Hi Steve,

thank you for the review.

On Thu, 2018-11-08 at 21:33 -0800, Steve Longerbeam wrote:
[...]
> > --- a/drivers/staging/media/imx/imx-media-capture.c
> > +++ b/drivers/staging/media/imx/imx-media-capture.c
> > @@ -262,6 +262,10 @@ static int capture_s_fmt_vid_cap(struct file *file, void *fh,
> >   	priv->vdev.fmt.fmt.pix = f->fmt.pix;
> >   	priv->vdev.cc = imx_media_find_format(f->fmt.pix.pixelformat,
> >   					      CS_SEL_ANY, true);
> > +	priv->vdev.compose.left = 0;
> > +	priv->vdev.compose.top = 0;
> > +	priv->vdev.compose.width = f->fmt.fmt.pix.width;
> > +	priv->vdev.compose.height = f->fmt.fmt.pix.height;
> 
> this should be:
> 
> priv->vdev.compose.width = fmt_src.format.width;
> priv->vdev.compose.height = fmt_src.format.height;
> 
> (corrected in the next patches but needs to be corrected here).

Thanks for catching this, it should be

+	priv->vdev.compose.width = f->fmt.pix.width;
+	priv->vdev.compose.height = f->fmt.pix.height;

though, fmt_src is only introduced in patch 2.

regards
Philipp

Re: [PATCH 2/3] media: imx: set compose rectangle to mbus format

[Philipp Zabel]

On Thu, 2018-11-08 at 21:33 -0800, Steve Longerbeam wrote:
> Hi Philipp,
> 
> On 11/5/18 7:20 AM, Philipp Zabel wrote:
> > Prepare for mbus format being smaller than the written rectangle
> > due to burst size.
> > 
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> >   drivers/staging/media/imx/imx-media-capture.c | 55 +++++++++++++------
> >   1 file changed, 38 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/staging/media/imx/imx-media-capture.c b/drivers/staging/media/imx/imx-media-capture.c
> > index cace8a51aca8..2d49d9573056 100644
> > --- a/drivers/staging/media/imx/imx-media-capture.c
> > +++ b/drivers/staging/media/imx/imx-media-capture.c
> > @@ -203,21 +203,14 @@ static int capture_g_fmt_vid_cap(struct file *file, void *fh,
> >   	return 0;
> >   }
> >   
> > -static int capture_try_fmt_vid_cap(struct file *file, void *fh,
> > -				   struct v4l2_format *f)
> > +static int __capture_try_fmt_vid_cap(struct capture_priv *priv,
> > +				     struct v4l2_subev_format *fmt_src,
> 
> 
> typo: struct v4l2_subdev_format *fmt_src,

Fixed, thanks.

[...]
> > 
> > +	return __capture_try_fmt(priv, &fmt_src, f);
> 
> 
> typo: return __capture_try_fmt_vid_cap(priv, &fmt_src, f);

And thanks. Looks like I've misplaced a fixup! patch.

regards
Philipp

Re: [PATCH 3/3] media: imx: lift CSI width alignment restriction

[Philipp Zabel]

Hi Steve,

On Thu, 2018-11-08 at 21:46 -0800, Steve Longerbeam wrote:
> > diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
> > index c4523afe7b48..d39682192a67 100644
> > --- a/drivers/staging/media/imx/imx-media-csi.c
> > +++ b/drivers/staging/media/imx/imx-media-csi.c
> > @@ -41,7 +41,7 @@
> >   #define MIN_H       144
> >   #define MAX_W      4096
> >   #define MAX_H      4096
> > -#define W_ALIGN    4 /* multiple of 16 pixels */
> > +#define W_ALIGN    1 /* multiple of 2 pixels */
> 
> This works for the IDMAC output pad because the channel's cpmem width 
> and stride can be rounded up, but width align at the CSI sink still 
> needs to be 8 pixels when directed to the IC via the CSI_SRC_PAD_DIRECT 
> pad, in order to support the 8x8 block rotator in the IC PRP, and 
> there's no way AFAIK to do the same trick of rounding up width andq 
> stride for non-IDMAC direct paths through the IPU.

Can't we just disallow rotation on prp subdevs if sink format is not
aligned to 2^3? Another possibility would be to align sink pad format
width to 2^3 only if the PAD_DIRECT link is enabled.

> Also, the imx-ic-prpencvf.c W_ALIGN_SRC can be relaxed to 2 pixels as
> well.

True, added for v2.

regards
Philipp

Re: [PATCH 3/3] media: imx: lift CSI width alignment restriction

[Philipp Zabel]

On Thu, 2018-11-08 at 21:46 -0800, Steve Longerbeam wrote:
> On 11/5/18 7:20 AM, Philipp Zabel wrote:
> > The CSI subdevice shouldn't have to care about IDMAC line start
> > address alignment. With compose rectangle support in the capture
> > driver, it doesn't have to anymore.
> > 
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> >   drivers/staging/media/imx/imx-media-capture.c |  9 ++++-----
> >   drivers/staging/media/imx/imx-media-csi.c     |  2 +-
> >   drivers/staging/media/imx/imx-media-utils.c   | 15 ++++++++++++---
> >   3 files changed, 17 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/staging/media/imx/imx-media-capture.c b/drivers/staging/media/imx/imx-media-capture.c
> > index 2d49d9573056..f87d6e8019e5 100644
> > --- a/drivers/staging/media/imx/imx-media-capture.c
> > +++ b/drivers/staging/media/imx/imx-media-capture.c
> > @@ -204,10 +204,9 @@ static int capture_g_fmt_vid_cap(struct file *file, void *fh,
> >   }
> >   
> >   static int __capture_try_fmt_vid_cap(struct capture_priv *priv,
> > -				     struct v4l2_subev_format *fmt_src,
> > +				     struct v4l2_subdev_format *fmt_src,
> >   				     struct v4l2_format *f)
> >   {
> > -	struct capture_priv *priv = video_drvdata(file);
> >   	const struct imx_media_pixfmt *cc, *cc_src;
> >   
> >   	cc_src = imx_media_find_ipu_format(fmt_src->format.code, CS_SEL_ANY);
> > @@ -250,7 +249,7 @@ static int capture_try_fmt_vid_cap(struct file *file, void *fh,
> >   	if (ret)
> >   		return ret;
> >   
> > -	return __capture_try_fmt(priv, &fmt_src, f);
> > +	return __capture_try_fmt_vid_cap(priv, &fmt_src, f);
> >   }
> >   
> >   static int capture_s_fmt_vid_cap(struct file *file, void *fh,
> > @@ -280,8 +279,8 @@ static int capture_s_fmt_vid_cap(struct file *file, void *fh,
> >   					      CS_SEL_ANY, true);
> >   	priv->vdev.compose.left = 0;
> >   	priv->vdev.compose.top = 0;
> > -	priv->vdev.compose.width = fmt_src.width;
> > -	priv->vdev.compose.height = fmt_src.height;
> > +	priv->vdev.compose.width = fmt_src.format.width;
> > +	priv->vdev.compose.height = fmt_src.format.height;
> >   
> >   	return 0;
> >   }
> > diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
> > index c4523afe7b48..d39682192a67 100644
> > --- a/drivers/staging/media/imx/imx-media-csi.c
> > +++ b/drivers/staging/media/imx/imx-media-csi.c
> > @@ -41,7 +41,7 @@
> >   #define MIN_H       144
> >   #define MAX_W      4096
> >   #define MAX_H      4096
> > -#define W_ALIGN    4 /* multiple of 16 pixels */
> > +#define W_ALIGN    1 /* multiple of 2 pixels */
> 
> 
> This works for the IDMAC output pad because the channel's cpmem width 
> and stride can be rounded up, but width align at the CSI sink still 
> needs to be 8 pixels when directed to the IC via the CSI_SRC_PAD_DIRECT 
> pad, in order to support the 8x8 block rotator in the IC PRP, and 
> there's no way AFAIK to do the same trick of rounding up width and 
> stride for non-IDMAC direct paths through the IPU.

Actually, this is not necessary at all. csi_try_crop takes care of this
by setting:
	crop->width &= ~0x7;
Which is then used to set compose rectangle and source pad formats.

So this should be relaxed as well, if the SRC_DIRECT pad is not enabled.
And further, I think there is no reason to align crop->left to multiples
of 4 pixels?

regards
Philipp

Re: [PATCH 3/3] media: imx: lift CSI width alignment restriction

[Steve Longerbeam]

On 11/9/18 6:50 AM, Philipp Zabel wrote:
> On Thu, 2018-11-08 at 21:46 -0800, Steve Longerbeam wrote:
>> On 11/5/18 7:20 AM, Philipp Zabel wrote:
>>> The CSI subdevice shouldn't have to care about IDMAC line start
>>> address alignment. With compose rectangle support in the capture
>>> driver, it doesn't have to anymore.
>>>
>>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
>>> ---
>>>    drivers/staging/media/imx/imx-media-capture.c |  9 ++++-----
>>>    drivers/staging/media/imx/imx-media-csi.c     |  2 +-
>>>    drivers/staging/media/imx/imx-media-utils.c   | 15 ++++++++++++---
>>>    3 files changed, 17 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/staging/media/imx/imx-media-capture.c b/drivers/staging/media/imx/imx-media-capture.c
>>> index 2d49d9573056..f87d6e8019e5 100644
>>> --- a/drivers/staging/media/imx/imx-media-capture.c
>>> +++ b/drivers/staging/media/imx/imx-media-capture.c
>>> @@ -204,10 +204,9 @@ static int capture_g_fmt_vid_cap(struct file *file, void *fh,
>>>    }
>>>    
>>>    static int __capture_try_fmt_vid_cap(struct capture_priv *priv,
>>> -				     struct v4l2_subev_format *fmt_src,
>>> +				     struct v4l2_subdev_format *fmt_src,
>>>    				     struct v4l2_format *f)
>>>    {
>>> -	struct capture_priv *priv = video_drvdata(file);
>>>    	const struct imx_media_pixfmt *cc, *cc_src;
>>>    
>>>    	cc_src = imx_media_find_ipu_format(fmt_src->format.code, CS_SEL_ANY);
>>> @@ -250,7 +249,7 @@ static int capture_try_fmt_vid_cap(struct file *file, void *fh,
>>>    	if (ret)
>>>    		return ret;
>>>    
>>> -	return __capture_try_fmt(priv, &fmt_src, f);
>>> +	return __capture_try_fmt_vid_cap(priv, &fmt_src, f);
>>>    }
>>>    
>>>    static int capture_s_fmt_vid_cap(struct file *file, void *fh,
>>> @@ -280,8 +279,8 @@ static int capture_s_fmt_vid_cap(struct file *file, void *fh,
>>>    					      CS_SEL_ANY, true);
>>>    	priv->vdev.compose.left = 0;
>>>    	priv->vdev.compose.top = 0;
>>> -	priv->vdev.compose.width = fmt_src.width;
>>> -	priv->vdev.compose.height = fmt_src.height;
>>> +	priv->vdev.compose.width = fmt_src.format.width;
>>> +	priv->vdev.compose.height = fmt_src.format.height;
>>>    
>>>    	return 0;
>>>    }
>>> diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
>>> index c4523afe7b48..d39682192a67 100644
>>> --- a/drivers/staging/media/imx/imx-media-csi.c
>>> +++ b/drivers/staging/media/imx/imx-media-csi.c
>>> @@ -41,7 +41,7 @@
>>>    #define MIN_H       144
>>>    #define MAX_W      4096
>>>    #define MAX_H      4096
>>> -#define W_ALIGN    4 /* multiple of 16 pixels */
>>> +#define W_ALIGN    1 /* multiple of 2 pixels */
>>
>> This works for the IDMAC output pad because the channel's cpmem width
>> and stride can be rounded up, but width align at the CSI sink still
>> needs to be 8 pixels when directed to the IC via the CSI_SRC_PAD_DIRECT
>> pad, in order to support the 8x8 block rotator in the IC PRP, and
>> there's no way AFAIK to do the same trick of rounding up width and
>> stride for non-IDMAC direct paths through the IPU.
> Actually, this is not necessary at all. csi_try_crop takes care of this
> by setting:
> 	crop->width &= ~0x7;
> Which is then used to set compose rectangle and source pad formats.

Ah you are right, I had forgotten about that line.

>
> So this should be relaxed as well, if the SRC_DIRECT pad is not enabled.

Agreed, crop->width align can be relaxed but only if SRC_DIRECT pad
not enabled.

> And further, I think there is no reason to align crop->left to multiples
> of 4 pixels?

Honestly, I don't know the reason for that either. Along with crop->top 
this defines the CSI active image frame position (HSC/VSC fields in 
IPU_CSI_OUT_FRM_CTRL register), and I don't see any h/w restrictions on 
HSC in the ref manual. I do remember that this alignment exists in 
FSL/NXP BSP's though, so maybe it is an undocumented restriction.

Steve