linux-media.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 1/3] media: imx: add capture compose rectangle
@ 2019-01-11 11:10 Philipp Zabel
  2019-01-11 11:10 ` [PATCH v3 2/3] media: imx: set compose rectangle to mbus format Philipp Zabel
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Philipp Zabel @ 2019-01-11 11:10 UTC (permalink / raw)
  To: linux-media; +Cc: Hans Verkuil, Steve Longerbeam, Sakari Ailus, kernel

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>
Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Steve Longerbeam <slongerbeam@gmail.com>
---
 drivers/staging/media/imx/imx-ic-prpencvf.c   |  3 +-
 drivers/staging/media/imx/imx-media-capture.c | 37 +++++++++++++++++++
 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, 43 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..fb985e68f9ab 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.pix.width;
+	priv->vdev.compose.height = f->fmt.pix.height;
 
 	return 0;
 }
@@ -290,6 +294,34 @@ 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_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 +382,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 +722,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.20.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v3 2/3] media: imx: set compose rectangle to mbus format
  2019-01-11 11:10 [PATCH v3 1/3] media: imx: add capture compose rectangle Philipp Zabel
@ 2019-01-11 11:10 ` Philipp Zabel
  2019-01-16 15:28   ` Hans Verkuil
  2019-01-11 11:10 ` [PATCH v3 3/3] media: imx: lift CSI and PRP ENC/VF width alignment restriction Philipp Zabel
  2019-01-16 15:29 ` [PATCH v3 1/3] media: imx: add capture compose rectangle Hans Verkuil
  2 siblings, 1 reply; 9+ messages in thread
From: Philipp Zabel @ 2019-01-11 11:10 UTC (permalink / raw)
  To: linux-media; +Cc: Hans Verkuil, Steve Longerbeam, Sakari Ailus, kernel

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

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
Reviewed-by: Steve Longerbeam <slongerbeam@gmail.com>
---
 drivers/staging/media/imx/imx-media-capture.c | 56 +++++++++++++------
 1 file changed, 38 insertions(+), 18 deletions(-)

diff --git a/drivers/staging/media/imx/imx-media-capture.c b/drivers/staging/media/imx/imx-media-capture.c
index fb985e68f9ab..614e335fb61c 100644
--- a/drivers/staging/media/imx/imx-media-capture.c
+++ b/drivers/staging/media/imx/imx-media-capture.c
@@ -203,21 +203,13 @@ 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_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 +223,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 +231,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_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 +264,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 +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 = f->fmt.pix.width;
-	priv->vdev.compose.height = f->fmt.pix.height;
+	priv->vdev.compose.width = fmt_src.format.width;
+	priv->vdev.compose.height = fmt_src.format.height;
 
 	return 0;
 }
@@ -306,9 +321,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.20.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v3 3/3] media: imx: lift CSI and PRP ENC/VF width alignment restriction
  2019-01-11 11:10 [PATCH v3 1/3] media: imx: add capture compose rectangle Philipp Zabel
  2019-01-11 11:10 ` [PATCH v3 2/3] media: imx: set compose rectangle to mbus format Philipp Zabel
@ 2019-01-11 11:10 ` Philipp Zabel
  2019-01-11 18:13   ` Steve Longerbeam
  2019-01-16 15:29 ` [PATCH v3 1/3] media: imx: add capture compose rectangle Hans Verkuil
  2 siblings, 1 reply; 9+ messages in thread
From: Philipp Zabel @ 2019-01-11 11:10 UTC (permalink / raw)
  To: linux-media; +Cc: Hans Verkuil, Steve Longerbeam, Sakari Ailus, kernel

The CSI, PRP ENC, and PRP VF subdevices shouldn't have to care about
IDMAC line start address alignment. With compose rectangle support in
the capture driver, they don't have to anymore.
If the direct CSI -> IC path is enabled, the CSI output width must
still be aligned to 8 pixels (IC burst length).

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
Changes since v2:
 - drop csi_src_pad_enabled(), check active_output_pad instead
 - initialize active_output_pad and set it to CSI_SRC_PAD_IDMAC
   while source pad links are disabled
---
 drivers/staging/media/imx/imx-ic-prpencvf.c |  2 +-
 drivers/staging/media/imx/imx-media-csi.c   | 11 +++++++++--
 drivers/staging/media/imx/imx-media-utils.c | 15 ++++++++++++---
 3 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c b/drivers/staging/media/imx/imx-ic-prpencvf.c
index fe5a77baa592..7bb754cb703e 100644
--- a/drivers/staging/media/imx/imx-ic-prpencvf.c
+++ b/drivers/staging/media/imx/imx-ic-prpencvf.c
@@ -48,7 +48,7 @@
 
 #define MAX_W_SRC  1024
 #define MAX_H_SRC  1024
-#define W_ALIGN_SRC   4 /* multiple of 16 pixels */
+#define W_ALIGN_SRC   1 /* multiple of 2 pixels */
 #define H_ALIGN_SRC   1 /* multiple of 2 lines */
 
 #define S_ALIGN       1 /* multiple of 2 */
diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
index c4523afe7b48..390beb61aa9b 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 */
 
@@ -1000,6 +1000,8 @@ static int csi_link_setup(struct media_entity *entity,
 		v4l2_ctrl_handler_free(&priv->ctrl_hdlr);
 		v4l2_ctrl_handler_init(&priv->ctrl_hdlr, 0);
 		priv->sink = NULL;
+		/* do not apply IC burst alignment in csi_try_crop */
+		priv->active_output_pad = CSI_SRC_PAD_IDMAC;
 		goto out;
 	}
 
@@ -1141,7 +1143,10 @@ static void csi_try_crop(struct csi_priv *priv,
 		crop->left = infmt->width - crop->width;
 	/* adjust crop left/width to h/w alignment restrictions */
 	crop->left &= ~0x3;
-	crop->width &= ~0x7;
+	if (priv->active_output_pad == CSI_SRC_PAD_DIRECT)
+		crop->width &= ~0x7; /* multiple of 8 pixels (IC burst) */
+	else
+		crop->width &= ~0x1; /* multiple of 2 pixels */
 
 	/*
 	 * FIXME: not sure why yet, but on interlaced bt.656,
@@ -1863,6 +1868,8 @@ static int imx_csi_probe(struct platform_device *pdev)
 	priv->csi_id = pdata->csi;
 	priv->smfc_id = (priv->csi_id == 0) ? 0 : 2;
 
+	priv->active_output_pad = CSI_SRC_PAD_IDMAC;
+
 	timer_setup(&priv->eof_timeout_timer, csi_idmac_eof_timeout, 0);
 	spin_lock_init(&priv->irqlock);
 
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.20.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 3/3] media: imx: lift CSI and PRP ENC/VF width alignment restriction
  2019-01-11 11:10 ` [PATCH v3 3/3] media: imx: lift CSI and PRP ENC/VF width alignment restriction Philipp Zabel
@ 2019-01-11 18:13   ` Steve Longerbeam
  0 siblings, 0 replies; 9+ messages in thread
From: Steve Longerbeam @ 2019-01-11 18:13 UTC (permalink / raw)
  To: Philipp Zabel, linux-media; +Cc: Hans Verkuil, Sakari Ailus, kernel

Reviewed-by: Steve Longerbeam <slongerbeam@gmail.com>

On 1/11/19 3:10 AM, Philipp Zabel wrote:
> The CSI, PRP ENC, and PRP VF subdevices shouldn't have to care about
> IDMAC line start address alignment. With compose rectangle support in
> the capture driver, they don't have to anymore.
> If the direct CSI -> IC path is enabled, the CSI output width must
> still be aligned to 8 pixels (IC burst length).
>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
> Changes since v2:
>   - drop csi_src_pad_enabled(), check active_output_pad instead
>   - initialize active_output_pad and set it to CSI_SRC_PAD_IDMAC
>     while source pad links are disabled
> ---
>   drivers/staging/media/imx/imx-ic-prpencvf.c |  2 +-
>   drivers/staging/media/imx/imx-media-csi.c   | 11 +++++++++--
>   drivers/staging/media/imx/imx-media-utils.c | 15 ++++++++++++---
>   3 files changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c b/drivers/staging/media/imx/imx-ic-prpencvf.c
> index fe5a77baa592..7bb754cb703e 100644
> --- a/drivers/staging/media/imx/imx-ic-prpencvf.c
> +++ b/drivers/staging/media/imx/imx-ic-prpencvf.c
> @@ -48,7 +48,7 @@
>   
>   #define MAX_W_SRC  1024
>   #define MAX_H_SRC  1024
> -#define W_ALIGN_SRC   4 /* multiple of 16 pixels */
> +#define W_ALIGN_SRC   1 /* multiple of 2 pixels */
>   #define H_ALIGN_SRC   1 /* multiple of 2 lines */
>   
>   #define S_ALIGN       1 /* multiple of 2 */
> diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
> index c4523afe7b48..390beb61aa9b 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 */
>   
> @@ -1000,6 +1000,8 @@ static int csi_link_setup(struct media_entity *entity,
>   		v4l2_ctrl_handler_free(&priv->ctrl_hdlr);
>   		v4l2_ctrl_handler_init(&priv->ctrl_hdlr, 0);
>   		priv->sink = NULL;
> +		/* do not apply IC burst alignment in csi_try_crop */
> +		priv->active_output_pad = CSI_SRC_PAD_IDMAC;
>   		goto out;
>   	}
>   
> @@ -1141,7 +1143,10 @@ static void csi_try_crop(struct csi_priv *priv,
>   		crop->left = infmt->width - crop->width;
>   	/* adjust crop left/width to h/w alignment restrictions */
>   	crop->left &= ~0x3;
> -	crop->width &= ~0x7;
> +	if (priv->active_output_pad == CSI_SRC_PAD_DIRECT)
> +		crop->width &= ~0x7; /* multiple of 8 pixels (IC burst) */
> +	else
> +		crop->width &= ~0x1; /* multiple of 2 pixels */
>   
>   	/*
>   	 * FIXME: not sure why yet, but on interlaced bt.656,
> @@ -1863,6 +1868,8 @@ static int imx_csi_probe(struct platform_device *pdev)
>   	priv->csi_id = pdata->csi;
>   	priv->smfc_id = (priv->csi_id == 0) ? 0 : 2;
>   
> +	priv->active_output_pad = CSI_SRC_PAD_IDMAC;
> +
>   	timer_setup(&priv->eof_timeout_timer, csi_idmac_eof_timeout, 0);
>   	spin_lock_init(&priv->irqlock);
>   
> 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;
>   }


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 2/3] media: imx: set compose rectangle to mbus format
  2019-01-11 11:10 ` [PATCH v3 2/3] media: imx: set compose rectangle to mbus format Philipp Zabel
@ 2019-01-16 15:28   ` Hans Verkuil
  2019-01-17 13:01     ` Philipp Zabel
  0 siblings, 1 reply; 9+ messages in thread
From: Hans Verkuil @ 2019-01-16 15:28 UTC (permalink / raw)
  To: Philipp Zabel, linux-media
  Cc: Hans Verkuil, Steve Longerbeam, Sakari Ailus, kernel

On 1/11/19 12:10 PM, 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>
> Reviewed-by: Steve Longerbeam <slongerbeam@gmail.com>
> ---
>  drivers/staging/media/imx/imx-media-capture.c | 56 +++++++++++++------
>  1 file changed, 38 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/staging/media/imx/imx-media-capture.c b/drivers/staging/media/imx/imx-media-capture.c
> index fb985e68f9ab..614e335fb61c 100644
> --- a/drivers/staging/media/imx/imx-media-capture.c
> +++ b/drivers/staging/media/imx/imx-media-capture.c
> @@ -203,21 +203,13 @@ 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_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 +223,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 +231,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_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 +264,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 +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 = f->fmt.pix.width;
> -	priv->vdev.compose.height = f->fmt.pix.height;
> +	priv->vdev.compose.width = fmt_src.format.width;
> +	priv->vdev.compose.height = fmt_src.format.height;
>  
>  	return 0;
>  }
> @@ -306,9 +321,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:

Shouldn't this be for COMPOSE_BOUNDS as well?

Do you need _PADDED at all? That only makes sense if the DMA writes beyond
the COMPOSE rectangle due to padding requirements. I'm not aware that that's
the case for imx. I may be wrong, this would be correct if the DMA indeed
writes the full buffer, even if the actual image is smaller.

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

I see that the image is always DMAed to the top-left corner of the buffer.

Is there a need to implement s_selection so you can change the top-left
corner of the compose rectangle within the buffer?

Regards,

	Hans

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 1/3] media: imx: add capture compose rectangle
  2019-01-11 11:10 [PATCH v3 1/3] media: imx: add capture compose rectangle Philipp Zabel
  2019-01-11 11:10 ` [PATCH v3 2/3] media: imx: set compose rectangle to mbus format Philipp Zabel
  2019-01-11 11:10 ` [PATCH v3 3/3] media: imx: lift CSI and PRP ENC/VF width alignment restriction Philipp Zabel
@ 2019-01-16 15:29 ` Hans Verkuil
  2019-01-17 12:46   ` Philipp Zabel
  2 siblings, 1 reply; 9+ messages in thread
From: Hans Verkuil @ 2019-01-16 15:29 UTC (permalink / raw)
  To: Philipp Zabel, linux-media
  Cc: Hans Verkuil, Steve Longerbeam, Sakari Ailus, kernel

On 1/11/19 12:10 PM, 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>
> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Reviewed-by: Steve Longerbeam <slongerbeam@gmail.com>
> ---
>  drivers/staging/media/imx/imx-ic-prpencvf.c   |  3 +-
>  drivers/staging/media/imx/imx-media-capture.c | 37 +++++++++++++++++++
>  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, 43 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..fb985e68f9ab 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.pix.width;
> +	priv->vdev.compose.height = f->fmt.pix.height;
>  
>  	return 0;
>  }
> @@ -290,6 +294,34 @@ 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:

The crop rectangle is equal to the compose rectangle? That can't be right.
Does the hardware support cropping at all? Probably not, and in that case
you shouldn't support the crop target at all.

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

Don't implement s_selection unless you can actually change the selection
rectangle(s).

> +
>  static int capture_g_parm(struct file *file, void *fh,
>  			  struct v4l2_streamparm *a)
>  {
> @@ -350,6 +382,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 +722,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,

	Hans

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 1/3] media: imx: add capture compose rectangle
  2019-01-16 15:29 ` [PATCH v3 1/3] media: imx: add capture compose rectangle Hans Verkuil
@ 2019-01-17 12:46   ` Philipp Zabel
  0 siblings, 0 replies; 9+ messages in thread
From: Philipp Zabel @ 2019-01-17 12:46 UTC (permalink / raw)
  To: Hans Verkuil, linux-media
  Cc: Sakari Ailus, Hans Verkuil, kernel, Steve Longerbeam

Hi Hans,

thank you for the review.

On Wed, 2019-01-16 at 16:29 +0100, Hans Verkuil wrote:
[...]
> @@ -290,6 +294,34 @@ 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:
> 
> The crop rectangle is equal to the compose rectangle? That can't be right.
> Does the hardware support cropping at all? Probably not, and in that case
> you shouldn't support the crop target at all.

Indeed the hardware does not support cropping at the DMA controller,
that has to be done in the CSI subdev already. I'll drop the CROP
targets.

> > +	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);
> > +}
> 
> Don't implement s_selection unless you can actually change the selection
> rectangle(s).

Ok, I'll only support g_selection for now. The main purpose of this
series is to allow capturing non-burstsize aligned widths at the CSI,
which are written padded to burst size alignement, and to report the
valid pixels to userspace via COMPOSE_DEFAULT rectangle.

regards
Philipp

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 2/3] media: imx: set compose rectangle to mbus format
  2019-01-16 15:28   ` Hans Verkuil
@ 2019-01-17 13:01     ` Philipp Zabel
  2019-01-17 13:54       ` Hans Verkuil
  0 siblings, 1 reply; 9+ messages in thread
From: Philipp Zabel @ 2019-01-17 13:01 UTC (permalink / raw)
  To: Hans Verkuil, linux-media
  Cc: Sakari Ailus, Hans Verkuil, kernel, Steve Longerbeam

On Wed, 2019-01-16 at 16:28 +0100, Hans Verkuil wrote:
> On 1/11/19 12:10 PM, 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>
> > Reviewed-by: Steve Longerbeam <slongerbeam@gmail.com>
> > ---
> >  drivers/staging/media/imx/imx-media-capture.c | 56 +++++++++++++------
> >  1 file changed, 38 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/staging/media/imx/imx-media-capture.c b/drivers/staging/media/imx/imx-media-capture.c
> > index fb985e68f9ab..614e335fb61c 100644
> > --- a/drivers/staging/media/imx/imx-media-capture.c
> > +++ b/drivers/staging/media/imx/imx-media-capture.c
> > @@ -203,21 +203,13 @@ 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_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 +223,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 +231,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_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 +264,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 +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 = f->fmt.pix.width;
> > -	priv->vdev.compose.height = f->fmt.pix.height;
> > +	priv->vdev.compose.width = fmt_src.format.width;
> > +	priv->vdev.compose.height = fmt_src.format.height;
> >  
> >  	return 0;
> >  }
> > @@ -306,9 +321,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:
> 
> Shouldn't this be for COMPOSE_BOUNDS as well?

COMPOSE_BOUNDS specifies the bounds in which COMPOSE can be set. Since
we don't allow changing COMPOSE at all, COMPOSE/BOUNDS/DEFAULT should
all be the same.
COMPOSE_PADDED is larger than the fixed COMPOSE rectangle on the right
side to align to DMA burst size.

> Do you need _PADDED at all? That only makes sense if the DMA writes beyond
> the COMPOSE rectangle due to padding requirements. I'm not aware that that's
> the case for imx.
> I may be wrong, this would be correct if the DMA indeed
> writes the full buffer, even if the actual image is smaller.

That's exactly what happens, the hardware writes with a fixed burst size
and doesn't support partial bursts as far as I am aware.
If the video input signal width is not a multiple of DMA burst size, the
last written burst of each line does contain some invalid padding pixels
at the end.

> > +		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;
> >  	}
> > 
> 
> I see that the image is always DMAed to the top-left corner of the buffer.
> 
> Is there a need to implement s_selection so you can change the top-left
> corner of the compose rectangle within the buffer?

I haven't seen a use case for this, but I'm curious how that is supposed
to work. Currently we are limiting buffer width/height in S_FMT to the
connected subdevice's mbus format width / height.
After this we'd have to allow any width / height larger than mbus format
and limit compose rectangle width/height to the mbus format?

regards
Philipp

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 2/3] media: imx: set compose rectangle to mbus format
  2019-01-17 13:01     ` Philipp Zabel
@ 2019-01-17 13:54       ` Hans Verkuil
  0 siblings, 0 replies; 9+ messages in thread
From: Hans Verkuil @ 2019-01-17 13:54 UTC (permalink / raw)
  To: Philipp Zabel, linux-media
  Cc: Sakari Ailus, Hans Verkuil, kernel, Steve Longerbeam

On 1/17/19 2:01 PM, Philipp Zabel wrote:
> On Wed, 2019-01-16 at 16:28 +0100, Hans Verkuil wrote:
>> On 1/11/19 12:10 PM, 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>
>>> Reviewed-by: Steve Longerbeam <slongerbeam@gmail.com>
>>> ---
>>>  drivers/staging/media/imx/imx-media-capture.c | 56 +++++++++++++------
>>>  1 file changed, 38 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/drivers/staging/media/imx/imx-media-capture.c b/drivers/staging/media/imx/imx-media-capture.c
>>> index fb985e68f9ab..614e335fb61c 100644
>>> --- a/drivers/staging/media/imx/imx-media-capture.c
>>> +++ b/drivers/staging/media/imx/imx-media-capture.c
>>> @@ -203,21 +203,13 @@ 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_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 +223,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 +231,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_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 +264,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 +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 = f->fmt.pix.width;
>>> -	priv->vdev.compose.height = f->fmt.pix.height;
>>> +	priv->vdev.compose.width = fmt_src.format.width;
>>> +	priv->vdev.compose.height = fmt_src.format.height;
>>>  
>>>  	return 0;
>>>  }
>>> @@ -306,9 +321,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:
>>
>> Shouldn't this be for COMPOSE_BOUNDS as well?
> 
> COMPOSE_BOUNDS specifies the bounds in which COMPOSE can be set. Since
> we don't allow changing COMPOSE at all, COMPOSE/BOUNDS/DEFAULT should
> all be the same.
> COMPOSE_PADDED is larger than the fixed COMPOSE rectangle on the right
> side to align to DMA burst size.
> 
>> Do you need _PADDED at all? That only makes sense if the DMA writes beyond
>> the COMPOSE rectangle due to padding requirements. I'm not aware that that's
>> the case for imx.
>> I may be wrong, this would be correct if the DMA indeed
>> writes the full buffer, even if the actual image is smaller.
> 
> That's exactly what happens, the hardware writes with a fixed burst size
> and doesn't support partial bursts as far as I am aware.
> If the video input signal width is not a multiple of DMA burst size, the
> last written burst of each line does contain some invalid padding pixels
> at the end.

Ah, OK. Can you add a comment to clarify this?

> 
>>> +		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;
>>>  	}
>>>
>>
>> I see that the image is always DMAed to the top-left corner of the buffer.
>>
>> Is there a need to implement s_selection so you can change the top-left
>> corner of the compose rectangle within the buffer?
> 
> I haven't seen a use case for this, but I'm curious how that is supposed
> to work. Currently we are limiting buffer width/height in S_FMT to the
> connected subdevice's mbus format width / height.
> After this we'd have to allow any width / height larger than mbus format
> and limit compose rectangle width/height to the mbus format?

Given the fact that the DMA doesn't support partial bursts (aka scatter-gather
DMA), there is no point in doing this.

The use-case I was thinking of is that e.g. you make a buffer that's twice the
height and width of the actual stream and compose it into one quarter of the
buffer. That way you can compose multiple streams into its own 'window' inside
the buffer. But that only works with gather-scatter DMA and so is not relevant
for this hardware.

So just add a comment and this patch is fine.

Regards,

	Hans

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2019-01-17 13:54 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-11 11:10 [PATCH v3 1/3] media: imx: add capture compose rectangle Philipp Zabel
2019-01-11 11:10 ` [PATCH v3 2/3] media: imx: set compose rectangle to mbus format Philipp Zabel
2019-01-16 15:28   ` Hans Verkuil
2019-01-17 13:01     ` Philipp Zabel
2019-01-17 13:54       ` Hans Verkuil
2019-01-11 11:10 ` [PATCH v3 3/3] media: imx: lift CSI and PRP ENC/VF width alignment restriction Philipp Zabel
2019-01-11 18:13   ` Steve Longerbeam
2019-01-16 15:29 ` [PATCH v3 1/3] media: imx: add capture compose rectangle Hans Verkuil
2019-01-17 12:46   ` Philipp Zabel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).