v4l: ti-vpe: Some VPE fixes and enhancements

From: Archit Taneja [mailto:[email protected]]
Sent: Monday, March 03, 2014 8:33 AM

The minimum width and height for VPE input/output was kept as 128 pixels. VPE doesn't have a constraint on the image height, it requires the image width to be atleast 16 bytes.

Change the minimum supported dimensions to 32x32. This allows us to de- interlace qcif content. A smaller image size than 32x32 didn't make much sense, so stopped at this.

Signed-off-by Archit Taneja <[email protected]>

---
 drivers/media/platform/ti-vpe/vpe.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/media/platform/ti-vpe/vpe.c +2 -3

index 915029b..3a610a6 100644
@@ -49,8 +49,8 @@
 #define VPE_MODULE_NAME "vpe"
 
 /* minimum and maximum frame sizes */
-#define MIN_W		128
-#define MIN_H		128
+#define MIN_W		32
+#define MIN_H		32
 #define MAX_W		1920
 #define MAX_H		1080
 
--

1.8.3.2

Hi Archit,

Quoted 6 lines

From: Archit Taneja [mailto:[email protected]]
Sent: Monday, March 03, 2014 8:33 AM

The minimum width and height for VPE input/output was kept as 128 pixels. VPE doesn't have a constraint on the image height, it requires the image width to be atleast 16 bytes.

"16 bytes" - shouldn't it be pixels? (also "at least" :) ) I can correct it when applying the patch if the above is the case.

Add crop ioctl ops. For VPE, cropping only makes sense with the input to VPE, or the V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE buffer type.

For the CAPTURE type, a S_CROP ioctl results in setting the crop region as the whole image itself, hence making crop dimensions same as the pix dimensions.

Setting the crop successfully should result in re-configuration of those registers which are affected when either source or destination dimensions change, set_srcdst_params() is called for this purpose.

Some standard crop parameter checks are done in __vpe_try_crop().

Signed-off-by Archit Taneja <[email protected]>

---
 drivers/media/platform/ti-vpe/vpe.c | 96 +++++++++++++++++++++++++++++++++++++
 1 file changed, 96 insertions(+)

drivers/media/platform/ti-vpe/vpe.c +96 -0

index 6acdcd8..c6778f4 100644
@@ -1585,6 +1585,98 @@ static int vpe_s_fmt(struct file *file, void *priv, struct v4l2_format *f)
 	return set_srcdst_params(ctx);
 }
 
+static int __vpe_try_crop(struct vpe_ctx *ctx, struct v4l2_crop *cr)
+{
+	struct vpe_q_data *q_data;
+
+	q_data = get_q_data(ctx, cr->type);
+	if (!q_data)
+		return -EINVAL;
+
+	/* we don't support crop on capture plane */
+	if (cr->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) {
+		cr->c.top = cr->c.left = 0;
+		cr->c.width = q_data->width;
+		cr->c.height = q_data->height;
+		return 0;
+	}
+
+	if (cr->c.top < 0 || cr->c.left < 0) {
+		vpe_err(ctx->dev, "negative values for top and left\n");
+		cr->c.top = cr->c.left = 0;
+	}
+
+	v4l_bound_align_image(&cr->c.width, MIN_W, q_data->width, 1,
+		&cr->c.height, MIN_H, q_data->height, H_ALIGN, S_ALIGN);
+
+	/* adjust left/top if cropping rectangle is out of bounds */
+	if (cr->c.left + cr->c.width > q_data->width)
+		cr->c.left = q_data->width - cr->c.width;
+	if (cr->c.top + cr->c.height > q_data->height)
+		cr->c.top = q_data->height - cr->c.height;
+
+	return 0;
+}
+
+static int vpe_cropcap(struct file *file, void *priv, struct v4l2_cropcap *cr)
+{
+	struct vpe_ctx *ctx = file2ctx(file);
+	struct vpe_q_data *q_data;
+
+	q_data = get_q_data(ctx, cr->type);
+	if (!q_data)
+		return -EINVAL;
+
+	cr->bounds.left = 0;
+	cr->bounds.top = 0;
+	cr->bounds.width = q_data->width;
+	cr->bounds.height = q_data->height;
+	cr->defrect = cr->bounds;
+
+	return 0;
+}
+
+static int vpe_g_crop(struct file *file, void *fh, struct v4l2_crop *cr)
+{
+	struct vpe_ctx *ctx = file2ctx(file);
+	struct vpe_q_data *q_data;
+
+	q_data = get_q_data(ctx, cr->type);
+	if (!q_data)
+		return -EINVAL;
+
+	cr->c = q_data->c_rect;
+
+	return 0;
+}
+
+static int vpe_s_crop(struct file *file, void *priv,
+		const struct v4l2_crop *crop)
+{
+	struct vpe_ctx *ctx = file2ctx(file);
+	struct vpe_q_data *q_data;
+	struct v4l2_crop cr = *crop;
+	int ret;
+
+	ret = __vpe_try_crop(ctx, &cr);
+	if (ret)
+		return ret;
+
+	q_data = get_q_data(ctx, cr.type);
+
+	if ((q_data->c_rect.left == cr.c.left) &&
+			(q_data->c_rect.top == cr.c.top) &&
+			(q_data->c_rect.width == cr.c.width) &&
+			(q_data->c_rect.height == cr.c.height)) {
+		vpe_dbg(ctx->dev, "requested crop values are already set\n");
+		return 0;
+	}
+
+	q_data->c_rect = cr.c;
+
+	return set_srcdst_params(ctx);
+}
+
 static int vpe_reqbufs(struct file *file, void *priv,
 		       struct v4l2_requestbuffers *reqbufs)
 {

@@ -1672,6 +1764,10 @@ static const struct v4l2_ioctl_ops vpe_ioctl_ops = {
 	.vidioc_try_fmt_vid_out_mplane	= vpe_try_fmt,
 	.vidioc_s_fmt_vid_out_mplane	= vpe_s_fmt,
 
+	.vidioc_cropcap			= vpe_cropcap,
+	.vidioc_g_crop			= vpe_g_crop,
+	.vidioc_s_crop			= vpe_s_crop,
+
 	.vidioc_reqbufs		= vpe_reqbufs,
 	.vidioc_querybuf	= vpe_querybuf,
 

Hi Archit!

Quoted 10 lines · Archit Taneja

Add crop ioctl ops. For VPE, cropping only makes sense with the input to VPE, or the V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE buffer type.

For the CAPTURE type, a S_CROP ioctl results in setting the crop region as the whole image itself, hence making crop dimensions same as the pix dimensions.

Setting the crop successfully should result in re-configuration of those registers which are affected when either source or destination dimensions change, set_srcdst_params() is called for this purpose.

Quoted 1 line

Some standard crop parameter checks are done in __vpe_try_crop().

Please use the selection ops instead: if you implement cropping with those then you'll support both the selection API and the old cropping API will be implemented by the v4l2 core using the selection ops. Two for the price of one...

From: Archit Taneja [mailto:[email protected]]
Sent: Monday, March 03, 2014 9:26 AM

Hi,

Quoted 4 lines · Hans Verkuil

Hi Archit!

Quoted 1 line · Archit Taneja

Add crop ioctl ops. For VPE, cropping only makes sense with the

input

Quoted 4 lines

Quoted 4 lines

to VPE, or the V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE buffer type.

For the CAPTURE type, a S_CROP ioctl results in setting the crop region as the whole image itself, hence making crop dimensions same

as the pix dimensions.

Quoted 10 lines

Quoted 6 lines

Setting the crop successfully should result in re-configuration of those registers which are affected when either source or destination dimensions change, set_srcdst_params() is called for this purpose.

Some standard crop parameter checks are done in __vpe_try_crop().

Please use the selection ops instead: if you implement cropping with those then you'll support both the selection API and the old cropping API will be implemented by the v4l2 core using the selection ops. Two

for the price of one...

<snip>

Thanks for the feedback. I'll use selection ops here.

From your reply I understand that you will send a v2 of these patches,

Hi Archit,

Quoted 23 lines

From: Archit Taneja [mailto:[email protected]]
Sent: Monday, March 03, 2014 9:26 AM

Hi,

Quoted 4 lines · Hans Verkuil

Hi Archit!

Quoted 1 line · Archit Taneja

Add crop ioctl ops. For VPE, cropping only makes sense with the

input

Quoted 4 lines

Quoted 4 lines

to VPE, or the V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE buffer type.

For the CAPTURE type, a S_CROP ioctl results in setting the crop region as the whole image itself, hence making crop dimensions same

as the pix dimensions.

Quoted 7 lines

Quoted 6 lines

Setting the crop successfully should result in re-configuration of those registers which are affected when either source or destination dimensions change, set_srcdst_params() is called for this purpose.

Some standard crop parameter checks are done in __vpe_try_crop().

Quoted 8 lines

Quoted 3 lines

Please use the selection ops instead: if you implement cropping with those then you'll support both the selection API and the old cropping API will be implemented by the v4l2 core using the selection ops. Two

for the price of one...

<snip>

Thanks for the feedback. I'll use selection ops here.

From your reply I understand that you will send a v2 of these patches,
right?
If so, please correct the typos I mentioned in the patch 5/7.

Also, it is quite late for v3.15. I did already send a pull request to Mauro,
However I have one patch pending. Could you tell me when are you planning to post v2 of these patches? I want to decide whether should I wait for your patchset or should I send the second pull request with the pending patch as soon as possible.

Hi Archit!

Quoted 10 lines · Archit Taneja

Add crop ioctl ops. For VPE, cropping only makes sense with the input to VPE, or the V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE buffer type.

For the CAPTURE type, a S_CROP ioctl results in setting the crop region as the whole image itself, hence making crop dimensions same as the pix dimensions.

Setting the crop successfully should result in re-configuration of those registers which are affected when either source or destination dimensions change, set_srcdst_params() is called for this purpose.

Quoted 1 line

Some standard crop parameter checks are done in __vpe_try_crop().

Please use the selection ops instead: if you implement cropping with those then you'll support both the selection API and the old cropping API will be implemented by the v4l2 core using the selection ops. Two for the price of one...

Hi Hans,

Quoted 13 lines · Hans Verkuil

Hi Archit!

Quoted 10 lines · Archit Taneja

Add crop ioctl ops. For VPE, cropping only makes sense with the input to VPE, or the V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE buffer type.

For the CAPTURE type, a S_CROP ioctl results in setting the crop region as the whole image itself, hence making crop dimensions same as the pix dimensions.

Setting the crop successfully should result in re-configuration of those registers which are affected when either source or destination dimensions change, set_srcdst_params() is called for this purpose.

Quoted 5 lines

Quoted 1 line

Some standard crop parameter checks are done in __vpe_try_crop().

Please use the selection ops instead: if you implement cropping with those then you'll support both the selection API and the old cropping API will be implemented by the v4l2 core using the selection ops. Two for the price of one...

When using selection API, I was finding issues using the older cropping API. The v4l_s_crop() ioctl func assumes that "crop means compose for output devices". However, for a m2m device. It probably makes sense to provide the following configuration:

for V4L2_BUF_TYPE_VIDEO_OUTPUT (input to the mem to mem HW), use CROP target(to crop the input buffer)

and, for V4L2_BUF_TYPE_VIDEO_CAPTURE(output of the mem to mem HW), use COMPOSE target(to place the HW output into a larger region)

Don't you think forcing OUTPUT devices to 'COMPOSE' for older cropping API is a bit limiting?

Quoted 28 lines · Archit Taneja

Hi Hans,

Quoted 13 lines · Hans Verkuil

Hi Archit!

Quoted 10 lines · Archit Taneja

Add crop ioctl ops. For VPE, cropping only makes sense with the input to VPE, or the V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE buffer type.

For the CAPTURE type, a S_CROP ioctl results in setting the crop region as the whole image itself, hence making crop dimensions same as the pix dimensions.

Setting the crop successfully should result in re-configuration of those registers which are affected when either source or destination dimensions change, set_srcdst_params() is called for this purpose.

Quoted 5 lines

Quoted 1 line

Some standard crop parameter checks are done in __vpe_try_crop().

Please use the selection ops instead: if you implement cropping with those then you'll support both the selection API and the old cropping API will be implemented by the v4l2 core using the selection ops. Two for the price of one...

When using selection API, I was finding issues using the older cropping API. The v4l_s_crop() ioctl func assumes that "crop means compose for output devices". However, for a m2m device. It probably makes sense to provide the following configuration:

Quoted 8 lines

for V4L2_BUF_TYPE_VIDEO_OUTPUT (input to the mem to mem HW), use CROP target(to crop the input buffer)

and, for V4L2_BUF_TYPE_VIDEO_CAPTURE(output of the mem to mem HW), use COMPOSE target(to place the HW output into a larger region)

Don't you think forcing OUTPUT devices to 'COMPOSE' for older cropping API is a bit limiting?

Yes, and that's why the selection API was created to work around that limitation :-)

The old cropping API was insufficiently flexible for modern devices, so we came up with this replacement.

Another reason why you have to implement the selection API: it's the only way to implement your functionality.

Add selection ioctl ops. For VPE, cropping makes sense only for the input to VPE(or V4L2_BUF_TYPE_VIDEO_OUTPUT/MPLANE buffers) and composing makes sense only for the output of VPE(or V4L2_BUF_TYPE_VIDEO_CAPTURE/MPLANE buffers).

For the CAPTURE type, V4L2_SEL_TGT_COMPOSE results in VPE writing the output in a rectangle within the capture buffer. For the OUTPUT type, V4L2_SEL_TGT_CROP results in selecting a rectangle region within the source buffer.

Setting the crop/compose rectangles should successfully result in re-configuration of registers which are affected when either source or destination dimensions change, set_srcdst_params() is called for this purpose.

Signed-off-by Archit Taneja <[email protected]>

---
 drivers/media/platform/ti-vpe/vpe.c | 142 ++++++++++++++++++++++++++++++++++++
 1 file changed, 142 insertions(+)

drivers/media/platform/ti-vpe/vpe.c +0 -0

index 03a6846..b938590 100644

drivers/media/platform/ti-vpe/vpe.c +14 -0

 		return &ctx->q_data[Q_DATA_DST];
 	default:
 		BUG();

@@ -1585,6 +1587,143 @@ static int vpe_s_fmt(struct file *file, void *priv, struct v4l2_format *f)
 	return set_srcdst_params(ctx);
 }
 
+static int __vpe_try_selection(struct vpe_ctx *ctx, struct v4l2_selection *s)
+{
+	struct vpe_q_data *q_data;
+
+	if ((s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
+	    (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT))
+		return -EINVAL;
+
+	q_data = get_q_data(ctx, s->type);
+	if (!q_data)
+		return -EINVAL;
+
+	switch (s->target) {
+	case V4L2_SEL_TGT_COMPOSE:

drivers/media/platform/ti-vpe/vpe.c +5 -0

+			s->r = q_data->c_rect;
+			return 0;
+		}
+
+	case V4L2_SEL_TGT_CROP:

drivers/media/platform/ti-vpe/vpe.c +98 -0

+			s->r = q_data->c_rect;
+			return 0;
+		}
+	default:
+		return -EINVAL;
+	}
+
+	if (s->r.top < 0 || s->r.left < 0) {
+		vpe_err(ctx->dev, "negative values for top and left\n");
+		s->r.top = s->r.left = 0;
+	}
+
+	v4l_bound_align_image(&s->r.width, MIN_W, q_data->width, 1,
+		&s->r.height, MIN_H, q_data->height, H_ALIGN, S_ALIGN);
+
+	/* adjust left/top if cropping rectangle is out of bounds */
+	if (s->r.left + s->r.width > q_data->width)
+		s->r.left = q_data->width - s->r.width;
+	if (s->r.top + s->r.height > q_data->height)
+		s->r.top = q_data->height - s->r.height;
+
+	return 0;
+}
+
+static int vpe_g_selection(struct file *file, void *fh,
+		struct v4l2_selection *s)
+{
+	struct vpe_ctx *ctx = file2ctx(file);
+	struct vpe_q_data *q_data;
+
+	if ((s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
+	    (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT))
+		return -EINVAL;
+
+	q_data = get_q_data(ctx, s->type);
+	if (!q_data)
+		return -EINVAL;
+
+	switch (s->target) {
+	/* return width and height from S_FMT of the respective buffer type */
+	case V4L2_SEL_TGT_COMPOSE_DEFAULT:
+	case V4L2_SEL_TGT_COMPOSE_BOUNDS:
+	case V4L2_SEL_TGT_CROP_BOUNDS:
+	case V4L2_SEL_TGT_CROP_DEFAULT:
+		s->r.left = 0;
+		s->r.top = 0;
+		s->r.width = q_data->width;
+		s->r.height = q_data->height;
+		return 0;
+
+	/*
+	 * CROP target holds for the output buffer type, and COMPOSE target
+	 * holds for the capture buffer type. We still return the c_rect params
+	 * for both the target types.
+	 */
+	case V4L2_SEL_TGT_COMPOSE:
+	case V4L2_SEL_TGT_CROP:
+		s->r.left = q_data->c_rect.left;
+		s->r.top = q_data->c_rect.top;
+		s->r.width = q_data->c_rect.width;
+		s->r.height = q_data->c_rect.height;
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+
+static int vpe_s_selection(struct file *file, void *fh,
+		struct v4l2_selection *s)
+{
+	struct vpe_ctx *ctx = file2ctx(file);
+	struct vpe_q_data *q_data;
+	struct v4l2_selection sel = *s;
+	int ret;
+
+	ret = __vpe_try_selection(ctx, &sel);
+	if (ret)
+		return ret;
+
+	q_data = get_q_data(ctx, sel.type);
+	if (!q_data)
+		return -EINVAL;
+
+	if ((q_data->c_rect.left == sel.r.left) &&
+			(q_data->c_rect.top == sel.r.top) &&
+			(q_data->c_rect.width == sel.r.width) &&
+			(q_data->c_rect.height == sel.r.height)) {
+		vpe_dbg(ctx->dev,
+			"requested crop/compose values are already set\n");
+		return 0;
+	}
+
+	q_data->c_rect = sel.r;
+
+	return set_srcdst_params(ctx);
+}
+
 static int vpe_reqbufs(struct file *file, void *priv,
 		       struct v4l2_requestbuffers *reqbufs)
 {

Hi Archit,

Quoted 22 lines · Archit Taneja

Add selection ioctl ops. For VPE, cropping makes sense only for the input to VPE(or V4L2_BUF_TYPE_VIDEO_OUTPUT/MPLANE buffers) and composing makes sense only for the output of VPE(or V4L2_BUF_TYPE_VIDEO_CAPTURE/MPLANE buffers).

For the CAPTURE type, V4L2_SEL_TGT_COMPOSE results in VPE writing the output in a rectangle within the capture buffer. For the OUTPUT type, V4L2_SEL_TGT_CROP results in selecting a rectangle region within the source buffer.

Setting the crop/compose rectangles should successfully result in re-configuration of registers which are affected when either source or destination dimensions change, set_srcdst_params() is called for this purpose.

Signed-off-by Archit Taneja <[email protected]>

---
  drivers/media/platform/ti-vpe/vpe.c | 142 ++++++++++++++++++++++++++++++++++++
  1 file changed, 142 insertions(+)

drivers/media/platform/ti-vpe/vpe.c +0 -0

index 03a6846..b938590 100644

drivers/media/platform/ti-vpe/vpe.c static struct vpe_q_data *get_q_data(struct vpe_ctx *ctx, · line 416

Quoted 7 lines

drivers/media/platform/ti-vpe/vpe.c +2 -0

@@ -410,8 +410,10 @@ static struct vpe_q_data *get_q_data(struct vpe_ctx *ctx,
  {
  	switch (type) {
  	case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
+	case V4L2_BUF_TYPE_VIDEO_OUTPUT:
  		return &ctx->q_data[Q_DATA_SRC];
  	case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
+	case V4L2_BUF_TYPE_VIDEO_CAPTURE:

I noticed that the querycap implementation is wrong. It reports V4L2_CAP_VIDEO_M2M instead of V4L2_CAP_VIDEO_M2M_MPLANE.

This driver is using the multiplanar formats, so the M2M_MPLANE cap should be set.

This should be a separate patch.

BTW, did you test the driver with the v4l2-compliance tool? The latest version
(http://git.linuxtv.org/v4l-utils.git) has m2m support.

However, if you want to test streaming (the -s option), then you will probably need to base your kernel on this tree:

http://git.linuxtv.org/hverkuil/media_tree.git/shortlog/refs/heads/vb2-part1

That branch contains a pile of fixes for vb2 and without that v4l2-compliance -s will fail a number of tests.

Quoted 21 lines

drivers/media/platform/ti-vpe/vpe.c +14 -0

  		return &ctx->q_data[Q_DATA_DST];
  	default:
  		BUG();

@@ -1585,6 +1587,143 @@ static int vpe_s_fmt(struct file *file, void *priv, struct v4l2_format *f)
  	return set_srcdst_params(ctx);
  }
 
+static int __vpe_try_selection(struct vpe_ctx *ctx, struct v4l2_selection *s)
+{
+	struct vpe_q_data *q_data;
+
+	if ((s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
+	    (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT))
+		return -EINVAL;
+
+	q_data = get_q_data(ctx, s->type);
+	if (!q_data)
+		return -EINVAL;
+
+	switch (s->target) {
+	case V4L2_SEL_TGT_COMPOSE:

drivers/media/platform/ti-vpe/vpe.c static int vpe_s_fmt(struct file *file, void *priv, struct v4l2_format *f) · line 1613

Quoted 10 lines

drivers/media/platform/ti-vpe/vpe.c +10 -0

+	case V4L2_SEL_TGT_COMPOSE_DEFAULT:
+	case V4L2_SEL_TGT_COMPOSE_BOUNDS:
+		/*
+		 * COMPOSE target is only valid for capture buffer type, for
+		 * output buffer type, assign existing crop parameters to the
+		 * selection rectangle
+		 */
+		if (s->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
+			break;
+		} else {

No need for the 'else' keywork here.

Quoted 9 lines

drivers/media/platform/ti-vpe/vpe.c +9 -0

+			s->r = q_data->c_rect;
+			return 0;
+		}
+
+	case V4L2_SEL_TGT_CROP:
+	case V4L2_SEL_TGT_CROP_DEFAULT:
+	case V4L2_SEL_TGT_CROP_BOUNDS:
+		/*
+		 * CROP target is only valid for output buffer type, for capture

drivers/media/platform/ti-vpe/vpe.c static int vpe_s_fmt(struct file *file, void *priv, struct v4l2_format *f) · line 1628

Quoted 6 lines

drivers/media/platform/ti-vpe/vpe.c +6 -0

+		 * buffer type, assign existing compose parameters to the
+		 * selection rectangle
+		 */
+		if (s->type == V4L2_BUF_TYPE_VIDEO_OUTPUT) {
+			break;
+		} else {

Ditto.

I had a minor question about the selection API:

Are the V4L2_SET_TGT_CROP/COMPOSE_DEFAULT and the corresponding 'BOUNDS' targets supposed to be used with VIDIOC_S_SELECTION? If so, what's the expect behaviour?

Quoted 5 lines · Archit Taneja

I had a minor question about the selection API:

Are the V4L2_SET_TGT_CROP/COMPOSE_DEFAULT and the corresponding 'BOUNDS' targets supposed to be used with VIDIOC_S_SELECTION? If so, what's the expect behaviour?

No, those are read only in practice. So only used with G_SELECTION, never with S_SELECTION.

Hi Hans,

Quoted 9 lines · Hans Verkuil

Quoted 5 lines · Archit Taneja

I had a minor question about the selection API:

Are the V4L2_SET_TGT_CROP/COMPOSE_DEFAULT and the corresponding 'BOUNDS' targets supposed to be used with VIDIOC_S_SELECTION? If so, what's the expect behaviour?

No, those are read only in practice. So only used with G_SELECTION, never with S_SELECTION.

<snip>

I tried the v4l2-compliance thing. It's awesome! And a bit annoying too when it comes to fixing little things needed for compliance :). But it's required, and I hope to fix these eventually.

After a few small fixes in the driver, I get the results as below. I am debugging the cause of try_fmt and s_fmt failures. I'm not sure why the streaming test fails with MMAP, the logs of my driver show that a successful mem2mem transaction happened.

I tried this on the 'vb2-part1' branch as you suggested.

Do you think I can go ahead with posting the v3 patch set for 3.15, and work on fixing the compliance issue for the -rc fixes?

Thanks,
Archit

# ./utils/v4l2-compliance/v4l2-compliance  -v --streaming=10
root@localhost:~/source_trees/v4l-utils# Driver Info:
         Driver name   : vpe
         Card type     : vpe
         Bus info      : platform:vpe
         Driver version: 3.14.0
         Capabilities  : 0x84004000
                 Video Memory-to-Memory Multiplanar
                 Streaming
                 Device Capabilities
         Device Caps   : 0x04004000
                 Video Memory-to-Memory Multiplanar
                 Streaming

Compliance test for device /dev/video0 (not using libv4l2):

Required ioctls:
test VIDIOC_QUERYCAP: OK

Allow for multiple opens:
test second video open: OK
test VIDIOC_QUERYCAP: OK
test VIDIOC_G/S_PRIORITY: OK

Debug ioctls:
test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported) test VIDIOC_LOG_STATUS: OK (Not Supported)

Input ioctls:
test VIDIOC_G/S_TUNER: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported) test VIDIOC_ENUMAUDIO: OK (Not Supported) test VIDIOC_G/S/ENUMINPUT: OK (Not Supported) test VIDIOC_G/S_AUDIO: OK (Not Supported)
Inputs: 0 Audio Inputs: 0 Tuners: 0

Output ioctls:
test VIDIOC_G/S_MODULATOR: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_ENUMAUDOUT: OK (Not Supported) test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported) test VIDIOC_G/S_AUDOUT: OK (Not Supported)
Outputs: 0 Audio Outputs: 0 Modulators: 0

Control ioctls:
info: checking v4l2_queryctrl of control 'User
Controls' (0x00980001)
info: checking v4l2_queryctrl of control 'Buffers Per
Transaction' (0x00981950)
info: checking v4l2_queryctrl of control 'Buffers Per
Transaction' (0x08000000)
test VIDIOC_QUERYCTRL/MENU: OK
info: checking control 'User Controls' (0x00980001)
info: checking control 'Buffers Per Transaction'
(0x00981950)
test VIDIOC_G/S_CTRL: OK
info: checking extended control 'User Controls'
(0x00980001)
info: checking extended control 'Buffers Per
Transaction' (0x00981950)
test VIDIOC_G/S/TRY_EXT_CTRLS: OK
info: checking control event 'User Controls' (0x00980001)
info: checking control event 'Buffers Per Transaction'
(0x00981950)
test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK
test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
Standard Controls: 1 Private Controls: 1

Input/Output configuration ioctls:
test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported) test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported) test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)

Format ioctls:
info: found 8 formats for buftype 9
info: found 4 formats for buftype 10
test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK test VIDIOC_G/S_PARM: OK (Not Supported)
test VIDIOC_G_FBUF: OK (Not Supported)
test VIDIOC_G_FMT: OK
fail: v4l2-test-formats.cpp(614): Video Capture
Multiplanar: TRY_FMT(G_FMT) != G_FMT
test VIDIOC_TRY_FMT: FAIL
warn: v4l2-test-formats.cpp(834): S_FMT cannot handle
an invalid pixelformat.
warn: v4l2-test-formats.cpp(835): This may or may not
be a problem. For more information see:

                 warn: v4l2-test-formats.cpp(836): 
http://www.mail-archive.com/[email protected]/msg56550.html
                 fail: v4l2-test-formats.cpp(420): pix_mp.reserved not 

zeroed

                 fail: v4l2-test-formats.cpp(851): Video Capture 

Multiplanar is valid, but no S_FMT was implemented

test VIDIOC_S_FMT: FAIL
test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)

Codec ioctls:
test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported) test VIDIOC_G_ENC_INDEX: OK (Not Supported) test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)

Buffer ioctls:
info: test buftype Video Capture Multiplanar
warn: v4l2-test-buffers.cpp(403): VIDIOC_CREATE_BUFS
not supported
info: test buftype Video Output Multiplanar
warn: v4l2-test-buffers.cpp(403): VIDIOC_CREATE_BUFS
not supported
test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
test VIDIOC_EXPBUF: OK (Not Supported)
test read/write: OK (Not Supported)
Video Capture Multiplanar (polling):
Buffer: 0 Sequence: 0 Field: Top Timestamp: 113.178208s
fail: v4l2-test-buffers.cpp(222): buf.field !=
cur_fmt.fmt.pix.field

                 fail: v4l2-test-buffers.cpp(630): checkQueryBuf(node, 
buf, bufs.type, bufs.memory, buf.index, Dequeued, last_seq)
                 fail: v4l2-test-buffers.cpp(1038): captureBufs(node, 

bufs, m2m_bufs, frame_count, true)
test MMAP: FAIL
test USERPTR: OK (Not Supported)
test DMABUF: Cannot test, specify --expbuf-device

Total: 40, Succeeded: 37, Failed: 3, Warnings: 5

[1]+ Exit 1 ./utils/v4l2-compliance/v4l2-compliance -v
--streaming=10

Quoted 3 lines

Do you think I can go ahead with posting the v3 patch set for 3.15, and work on fixing the compliance issue for the -rc fixes?

It's fine to upstream this in staging, but while not all compliance errors are fixed it can't go to drivers/media. I'm tightening the screws on that since v4l2-compliance is getting to be such a powerful tool for ensuring the driver complies.

Quoted 10 lines

Multiplanar: TRY_FMT(G_FMT) != G_FMT
test VIDIOC_TRY_FMT: FAIL
warn: v4l2-test-formats.cpp(834): S_FMT cannot handle
an invalid pixelformat.
warn: v4l2-test-formats.cpp(835): This may or may not
be a problem. For more information see:

                  warn: v4l2-test-formats.cpp(836):
http://www.mail-archive.com/[email protected]/msg56550.html
                  fail: v4l2-test-formats.cpp(420): pix_mp.reserved not

zeroed

This is easy enough to fix.

Quoted 2 lines

                  fail: v4l2-test-formats.cpp(851): Video Capture

Multiplanar is valid, but no S_FMT was implemented

For the FMT things: run with -T: that gives nice traces. You can also set the debug flag: echo 2 >/sys/class/video4linux/video0/debug to see all ioctls in more detail.

Quoted 9 lines

test VIDIOC_S_FMT: FAIL
test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)

Codec ioctls:
test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported) test VIDIOC_G_ENC_INDEX: OK (Not Supported) test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)

Quoted 14 lines

Buffer ioctls:
info: test buftype Video Capture Multiplanar
warn: v4l2-test-buffers.cpp(403): VIDIOC_CREATE_BUFS
not supported
info: test buftype Video Output Multiplanar
warn: v4l2-test-buffers.cpp(403): VIDIOC_CREATE_BUFS
not supported
test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
test VIDIOC_EXPBUF: OK (Not Supported)
test read/write: OK (Not Supported)
Video Capture Multiplanar (polling):
Buffer: 0 Sequence: 0 Field: Top Timestamp: 113.178208s
fail: v4l2-test-buffers.cpp(222): buf.field !=
cur_fmt.fmt.pix.field

Definitely needs to be fixed, you probably just don't set the field at all.

Hi,

Quoted 4 lines · Hans Verkuil

Quoted 3 lines

Do you think I can go ahead with posting the v3 patch set for 3.15, and work on fixing the compliance issue for the -rc fixes?

Quoted 5 lines

It's fine to upstream this in staging, but while not all compliance errors are fixed it can't go to drivers/media. I'm tightening the screws on that since v4l2-compliance is getting to be such a powerful tool for ensuring the driver complies.

But the vpe driver is already in drivers/media. How do I push these patches if the vpe drivers is not in staging?

<snip>

Quoted 11 lines

Quoted 10 lines

Multiplanar: TRY_FMT(G_FMT) != G_FMT
test VIDIOC_TRY_FMT: FAIL
warn: v4l2-test-formats.cpp(834): S_FMT cannot handle
an invalid pixelformat.
warn: v4l2-test-formats.cpp(835): This may or may not
be a problem. For more information see:

                  warn: v4l2-test-formats.cpp(836):
http://www.mail-archive.com/[email protected]/msg56550.html
                  fail: v4l2-test-formats.cpp(420): pix_mp.reserved not

zeroed

Quoted 8 lines

This is easy enough to fix.

Quoted 2 lines

                  fail: v4l2-test-formats.cpp(851): Video Capture

Multiplanar is valid, but no S_FMT was implemented

For the FMT things: run with -T: that gives nice traces. You can also set the debug flag: echo 2 >/sys/class/video4linux/video0/debug to see all ioctls in more detail.

Thanks for the tip, will try this.

Quoted 24 lines

Quoted 23 lines

test VIDIOC_S_FMT: FAIL
test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)

Codec ioctls:
test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported) test VIDIOC_G_ENC_INDEX: OK (Not Supported) test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)

Buffer ioctls:
info: test buftype Video Capture Multiplanar
warn: v4l2-test-buffers.cpp(403): VIDIOC_CREATE_BUFS
not supported
info: test buftype Video Output Multiplanar
warn: v4l2-test-buffers.cpp(403): VIDIOC_CREATE_BUFS
not supported
test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
test VIDIOC_EXPBUF: OK (Not Supported)
test read/write: OK (Not Supported)
Video Capture Multiplanar (polling):
Buffer: 0 Sequence: 0 Field: Top Timestamp: 113.178208s
fail: v4l2-test-buffers.cpp(222): buf.field !=
cur_fmt.fmt.pix.field

Quoted 1 line

Definitely needs to be fixed, you probably just don't set the field at all.

The VPE output is always progressive. But yes, I should still set the field parameter to something.

Thanks,
Archit

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 13 lines · Archit Taneja

Hi,

Quoted 9 lines · Hans Verkuil

Quoted 3 lines

Do you think I can go ahead with posting the v3 patch set for 3.15, and work on fixing the compliance issue for the -rc fixes?

It's fine to upstream this in staging, but while not all compliance errors are fixed it can't go to drivers/media. I'm tightening the screws on that since v4l2-compliance is getting to be such a powerful tool for ensuring the driver complies.

Quoted 2 lines

But the vpe driver is already in drivers/media. How do I push these patches if the vpe drivers is not in staging?

Oops, sorry. I got confused with Benoit's AM437x ti-vpfe patch :-)

Disregard what I said, it's OK to upstream it. But if you could just spend some hours fixing the problems, that would really be best.

Quoted 50 lines

<snip>

Quoted 11 lines

Quoted 10 lines

Multiplanar: TRY_FMT(G_FMT) != G_FMT
test VIDIOC_TRY_FMT: FAIL
warn: v4l2-test-formats.cpp(834): S_FMT cannot handle
an invalid pixelformat.
warn: v4l2-test-formats.cpp(835): This may or may not
be a problem. For more information see:

                   warn: v4l2-test-formats.cpp(836):
http://www.mail-archive.com/[email protected]/msg56550.html
                   fail: v4l2-test-formats.cpp(420): pix_mp.reserved not

zeroed

Quoted 8 lines

This is easy enough to fix.

Quoted 2 lines

                   fail: v4l2-test-formats.cpp(851): Video Capture

Multiplanar is valid, but no S_FMT was implemented

For the FMT things: run with -T: that gives nice traces. You can also set the debug flag: echo 2 >/sys/class/video4linux/video0/debug to see all ioctls in more detail.

Thanks for the tip, will try this.

Quoted 24 lines

Quoted 23 lines

test VIDIOC_S_FMT: FAIL
test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)

Codec ioctls:
test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported) test VIDIOC_G_ENC_INDEX: OK (Not Supported) test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)

Buffer ioctls:
info: test buftype Video Capture Multiplanar
warn: v4l2-test-buffers.cpp(403): VIDIOC_CREATE_BUFS
not supported
info: test buftype Video Output Multiplanar
warn: v4l2-test-buffers.cpp(403): VIDIOC_CREATE_BUFS
not supported
test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
test VIDIOC_EXPBUF: OK (Not Supported)
test read/write: OK (Not Supported)
Video Capture Multiplanar (polling):
Buffer: 0 Sequence: 0 Field: Top Timestamp: 113.178208s
fail: v4l2-test-buffers.cpp(222): buf.field !=
cur_fmt.fmt.pix.field

Quoted 4 lines

Quoted 1 line

Definitely needs to be fixed, you probably just don't set the field at all.

The VPE output is always progressive. But yes, I should still set the field parameter to something.

V4L2_FIELD_NONE is the correct field setting for that.

Quoted 6 lines · Hans Verkuil

On 03/07/2014 02:22 PM, Archit Taneja wrote:

Disregard what I said, it's OK to upstream it. But if you could just spend
some hours fixing the problems, that would really be best.

Sure, I'll try to fix these issues and then post a v3.

Hi Hans,

Quoted 9 lines · Archit Taneja

Quoted 6 lines · Hans Verkuil

On 03/07/2014 02:22 PM, Archit Taneja wrote:

Disregard what I said, it's OK to upstream it. But if you could just spend
some hours fixing the problems, that would really be best.

Sure, I'll try to fix these issues and then post a v3.

I fixed most of the compliance errors. There were some things I needed to change in .utils/v4l2-compliance/v4l2-test-buffers.cpp'. I added those with some questions in the comments:

utils/v4l2-compliance/v4l2-test-buffers.cpp +0 -0

index 6576d11..532a5b6 100644

utils/v4l2-compliance/v4l2-test-buffers.cpp +3 -0

+			 */
+
+			/* fail_on_test(buf.field == V4L2_FIELD_ANY); */
  			if (cur_fmt.fmt.pix.field == V4L2_FIELD_ALTERNATE) {
  				fail_on_test(buf.field != V4L2_FIELD_BOTTOM &&

  						buf.field != V4L2_FIELD_TOP);

utils/v4l2-compliance/v4l2-test-buffers.cpp +0 -0

@@ -651,9 +657,17 @@ static int captureBufs(struct node *node, const 

struct v4l2_requestbuffers &bufs
} else if (node->is_m2m && timestamp == V4L2_BUF_FLAG_TIMESTAMP_COPY) {
fail_on_test(buffer_info.find(buf.timestamp) == buffer_info.end()); struct v4l2_buffer &orig_buf = buffer_info[buf.timestamp];

utils/v4l2-compliance/v4l2-test-buffers.cpp +0 -1

-				fail_on_test(buf.field != orig_buf.field);

utils/v4l2-compliance/v4l2-test-buffers.cpp +3 -0

+				 */
+				/*fail_on_test((buf.flags & valid_output_flags) !=
+					     (orig_buf.flags & valid_output_flags)); */
  				if (buf.flags & V4L2_BUF_FLAG_TIMECODE)
  					fail_on_test(memcmp(&buf.timecode, &orig_buf.timecode,
  								sizeof(buf.timecode)));

Thanks,
Archit

From: Archit Taneja [mailto:[email protected]]
Sent: Tuesday, March 04, 2014 12:25 PM

Hi,

Quoted 6 lines · Hans Verkuil

Hi Archit,

Quoted 3 lines · Archit Taneja

Add selection ioctl ops. For VPE, cropping makes sense only for the input to VPE(or V4L2_BUF_TYPE_VIDEO_OUTPUT/MPLANE buffers) and composing makes sense only for the output of VPE(or

V4L2_BUF_TYPE_VIDEO_CAPTURE/MPLANE buffers).

Quoted 2 lines

Quoted 2 lines

For the CAPTURE type, V4L2_SEL_TGT_COMPOSE results in VPE writing

the

Quoted 1 line

Quoted 1 line

output in a rectangle within the capture buffer. For the OUTPUT

type,

Quoted 1 line

Quoted 1 line

V4L2_SEL_TGT_CROP results in selecting a rectangle region within the

source buffer.

Quoted 4 lines

Quoted 4 lines

Setting the crop/compose rectangles should successfully result in re-configuration of registers which are affected when either source or destination dimensions change, set_srcdst_params() is called for

this purpose.

Quoted 4 lines

Quoted 4 lines

Signed-off-by Archit Taneja <[email protected]>

---
  drivers/media/platform/ti-vpe/vpe.c | 142

++++++++++++++++++++++++++++++++++++

Quoted 8 lines

drivers/media/platform/ti-vpe/vpe.c static struct vpe_q_data *get_q_data(struct

Quoted 8 lines

  1 file changed, 142 insertions(+)

drivers/media/platform/ti-vpe/vpe.c +0 -0

index 03a6846..b938590 100644
@@ -410,8 +410,10 @@ static struct vpe_q_data *get_q_data(struct

vpe_ctx *ctx,

Quoted 11 lines

drivers/media/platform/ti-vpe/vpe.c static struct vpe_q_data *get_q_data(struct vpe_ctx *ctx, · line 416

Quoted 7 lines

drivers/media/platform/ti-vpe/vpe.c +2 -0

  {
  	switch (type) {
  	case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
+	case V4L2_BUF_TYPE_VIDEO_OUTPUT:
  		return &ctx->q_data[Q_DATA_SRC];
  	case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
+	case V4L2_BUF_TYPE_VIDEO_CAPTURE:

I noticed that the querycap implementation is wrong. It reports V4L2_CAP_VIDEO_M2M instead of V4L2_CAP_VIDEO_M2M_MPLANE.

Quoted 4 lines

This driver is using the multiplanar formats, so the M2M_MPLANE cap should be set.

This should be a separate patch.

Thanks for pointing this out, I'll make a patch for that.

Quoted 2 lines

BTW, did you test the driver with the v4l2-compliance tool? The

latest

Quoted 3 lines

version
(http://git.linuxtv.org/v4l-utils.git) has m2m support.

I haven't tested it with this yet.

Quoted 4 lines

However, if you want to test streaming (the -s option), then you will probably need to base your kernel on this tree:

http://git.linuxtv.org/hverkuil/media_tree.git/shortlog/refs/heads/vb2

Quoted 1 line

-part1

I can give it a try. It'll probably take a bit more time to try this out. I'll need to port some minor DRA7x stuff.

Kamil,

Do you think you have some more time for the m2m pull request?

Quoted 8 lines

That branch contains a pile of fixes for vb2 and without that v4l2-compliance -s will fail a number of tests.

drivers/media/platform/ti-vpe/vpe.c static int vpe_s_fmt(struct file *file, void

Quoted 4 lines

drivers/media/platform/ti-vpe/vpe.c +0 -0

  		return &ctx->q_data[Q_DATA_DST];
  	default:
  		BUG();

@@ -1585,6 +1587,143 @@ static int vpe_s_fmt(struct file *file, void

*priv, struct v4l2_format *f)

Quoted 13 lines

Quoted 13 lines

drivers/media/platform/ti-vpe/vpe.c +10 -0

  	return set_srcdst_params(ctx);
  }
 
+static int __vpe_try_selection(struct vpe_ctx *ctx, struct
+v4l2_selection *s) {
+	struct vpe_q_data *q_data;
+
+	if ((s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
+	    (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT))
+		return -EINVAL;
+
+	q_data = get_q_data(ctx, s->type);
+	if (!q_data)

Quoted 8 lines

drivers/media/platform/ti-vpe/vpe.c static int vpe_s_fmt(struct file *file, void *priv, struct v4l2_format *f)

Quoted 8 lines

drivers/media/platform/ti-vpe/vpe.c +8 -0

+		return -EINVAL;
+
+	switch (s->target) {
+	case V4L2_SEL_TGT_COMPOSE:
+	case V4L2_SEL_TGT_COMPOSE_DEFAULT:
+	case V4L2_SEL_TGT_COMPOSE_BOUNDS:
+		/*
+		 * COMPOSE target is only valid for capture buffer type,

for

Quoted 1 line

drivers/media/platform/ti-vpe/vpe.c static int vpe_s_fmt(struct file *file, void · line 1608

Quoted 1 line

drivers/media/platform/ti-vpe/vpe.c +1 -0

+		 * output buffer type, assign existing crop parameters to

the

Quoted 8 lines

drivers/media/platform/ti-vpe/vpe.c static int vpe_s_fmt(struct file *file, void *priv, struct v4l2_format *f) · line 1613

Quoted 5 lines

drivers/media/platform/ti-vpe/vpe.c +5 -0

+		 * selection rectangle
+		 */
+		if (s->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
+			break;
+		} else {

No need for the 'else' keywork here.

Quoted 9 lines

drivers/media/platform/ti-vpe/vpe.c static int vpe_s_fmt(struct file *file, void *priv, struct v4l2_format *f)

Quoted 9 lines

drivers/media/platform/ti-vpe/vpe.c +9 -0

+			s->r = q_data->c_rect;
+			return 0;
+		}
+
+	case V4L2_SEL_TGT_CROP:
+	case V4L2_SEL_TGT_CROP_DEFAULT:
+	case V4L2_SEL_TGT_CROP_BOUNDS:
+		/*
+		 * CROP target is only valid for output buffer type, for

capture

Quoted 8 lines

drivers/media/platform/ti-vpe/vpe.c static int vpe_s_fmt(struct file *file, void *priv, struct v4l2_format *f) · line 1628

Quoted 6 lines

drivers/media/platform/ti-vpe/vpe.c +6 -0

+		 * buffer type, assign existing compose parameters to the
+		 * selection rectangle
+		 */
+		if (s->type == V4L2_BUF_TYPE_VIDEO_OUTPUT) {
+			break;
+		} else {

Ditto.

Thanks. I'll fix these.

I had a minor question about the selection API:

Are the V4L2_SET_TGT_CROP/COMPOSE_DEFAULT and the corresponding 'BOUNDS'
targets supposed to be used with VIDIOC_S_SELECTION? If so, what's the expect behaviour?

Thanks,
Archit

From: Archit Taneja [mailto:[email protected]]
Sent: Tuesday, March 11, 2014 9:34 AM

VPE has a ctrl parameter which decides how many mem to mem transactions the active job from the job queue can perform.

The driver's job_ready() made sure that the number of ready source buffers are sufficient for the job to execute successfully. But it didn't make sure if there are sufficient ready destination buffers in the capture queue for the VPE output.

If the time taken by VPE to process a single frame is really slow, then it's possible that we don't need to imply such a restriction on the dst queue, but really fast transactions(small resolution, no de-interlacing) may cause us to hit the condition where we don't have any free buffers for the VPE to write on.

Add the extra check in job_ready() to make sure we have the sufficient amount of destination buffers.

Signed-off-by Archit Taneja <[email protected]>

---
 drivers/media/platform/ti-vpe/vpe.c | 3 +++
 1 file changed, 3 insertions(+)

drivers/media/platform/ti-vpe/vpe.c +3 -1

index 7a77a5b..f3143ac 100644
@@ -887,6 +887,9 @@ static int job_ready(void *priv)
 	if (v4l2_m2m_num_src_bufs_ready(ctx->m2m_ctx) < needed)
 		return 0;
 
+	if (v4l2_m2m_num_dst_bufs_ready(ctx->m2m_ctx) < needed)
+		return 0;
+
 	return 1;
 }
 
--

1.8.3.2

From: Archit Taneja [mailto:[email protected]]
Sent: Tuesday, March 11, 2014 9:34 AM

vpe fops(vpe_open in particular) should be called only when VPDMA firmware is loaded. File operations on the video device are possible the moment it is registered.

Currently, we register the video device for VPE at driver probe, after calling a vpdma helper to initialize VPDMA and load firmware. This function is non-blocking(it calls request_firmware_nowait()), and doesn't ensure that the firmware is actually loaded when it returns.

We remove the device registration from vpe probe, and move it to a callback provided by the vpe driver to the vpdma library, through vpdma_create().

The ready field in vpdma_data is no longer needed since we always have firmware loaded before the device is registered.

A minor problem with this approach is that if the video_register_device fails(which doesn't really happen), the vpe platform device would be registered.
however, there won't be any v4l2 device corresponding to it.

Signed-off-by Archit Taneja <[email protected]>

---
 drivers/media/platform/ti-vpe/vpdma.c |  8 +++--
drivers/media/platform/ti-vpe/vpdma.h |  7 +++--
 drivers/media/platform/ti-vpe/vpe.c   | 55 ++++++++++++++++++++-------
--------
 3 files changed, 41 insertions(+), 29 deletions(-)

drivers/media/platform/ti-vpe/vpdma.c +2 -2

index e8175e7..73dd38e 100644
@@ -781,7 +781,7 @@ static void vpdma_firmware_cb(const struct firmware
 *f, void *context)
 	/* already initialized */
 	if (read_field_reg(vpdma, VPDMA_LIST_ATTR, VPDMA_LIST_RDY_MASK,
 			VPDMA_LIST_RDY_SHFT)) {
-		vpdma->ready = true;
+		vpdma->cb(vpdma->pdev);
 		return;
 	}

@@ -811,7 +811,7 @@ static void vpdma_firmware_cb(const struct firmware
 *f, void *context)
 		goto free_buf;
 	}
 
-	vpdma->ready = true;
+	vpdma->cb(vpdma->pdev);
 
 free_buf:

vpdma_unmap_desc_buf(vpdma, &fw_dma_buf); @@ -839,7 +839,8 @@
static int vpdma_load_firmware(struct vpdma_data *vpdma)

drivers/media/platform/ti-vpe/vpdma.c +3 -1

 	return 0;
 }
 
-struct vpdma_data *vpdma_create(struct platform_device *pdev)
+struct vpdma_data *vpdma_create(struct platform_device *pdev,
+		void (*cb)(struct platform_device *pdev))
 {
 	struct resource *res;
 	struct vpdma_data *vpdma;

@@ -854,6 +855,7 @@ struct vpdma_data *vpdma_create(struct
 platform_device *pdev)
 	}
 
 	vpdma->pdev = pdev;
+	vpdma->cb = cb;
 
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "vpdma");

drivers/media/platform/ti-vpe/vpdma.c +0 -0

 	if (res == NULL) {

drivers/media/platform/ti-vpe/vpdma.h +4 -2

index cf40f11..bf5f8bb 100644
@@ -35,8 +35,8 @@ struct vpdma_data {
 
 	struct platform_device	*pdev;
 
-	/* tells whether vpdma firmware is loaded or not */
-	bool ready;
+	/* callback to VPE driver when the firmware is loaded */
+	void (*cb)(struct platform_device *pdev);
 };
 
 enum vpdma_data_format_type {

@@ -208,6 +208,7 @@ void vpdma_set_frame_start_event(struct vpdma_data
 *vpdma,  void vpdma_dump_regs(struct vpdma_data *vpdma);
 
 /* initialize vpdma, passed with VPE's platform device pointer */ -
 struct vpdma_data *vpdma_create(struct platform_device *pdev);
+struct vpdma_data *vpdma_create(struct platform_device *pdev,
+		void (*cb)(struct platform_device *pdev));
 
 #endif

drivers/media/platform/ti-vpe/vpe.c +30 -6

index f3143ac..f1eae67 100644
@@ -1817,11 +1817,6 @@ static int vpe_open(struct file *file)
 
 	vpe_dbg(dev, "vpe_open\n");
 
-	if (!dev->vpdma->ready) {
-		vpe_err(dev, "vpdma firmware not loaded\n");
-		return -ENODEV;
-	}
-
 	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
 	if (!ctx)
 		return -ENOMEM;

@@ -2039,10 +2034,40 @@ static void vpe_runtime_put(struct
 platform_device *pdev)
 	WARN_ON(r < 0 && r != -ENOSYS);
 }
 
+static void vpe_fw_cb(struct platform_device *pdev) {
+	struct vpe_dev *dev = platform_get_drvdata(pdev);
+	struct video_device *vfd;
+	int ret;
+
+	vfd = &dev->vfd;
+	*vfd = vpe_videodev;
+	vfd->lock = &dev->dev_mutex;
+	vfd->v4l2_dev = &dev->v4l2_dev;
+
+	ret = video_register_device(vfd, VFL_TYPE_GRABBER, 0);
+	if (ret) {
+		vpe_err(dev, "Failed to register video device\n");
+
+		vpe_set_clock_enable(dev, 0);
+		vpe_runtime_put(pdev);
+		pm_runtime_disable(&pdev->dev);
+		v4l2_m2m_release(dev->m2m_dev);
+		vb2_dma_contig_cleanup_ctx(dev->alloc_ctx);
+		v4l2_device_unregister(&dev->v4l2_dev);
+
+		return;
+	}
+
+	video_set_drvdata(vfd, dev);
+	snprintf(vfd->name, sizeof(vfd->name), "%s", vpe_videodev.name);
+	dev_info(dev->v4l2_dev.dev, "Device registered as /dev/video%d\n",
+		vfd->num);
+}
+
 static int vpe_probe(struct platform_device *pdev)  {
 	struct vpe_dev *dev;
-	struct video_device *vfd;
 	int ret, irq, func;
 
 	dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL); @@ -

2123,28 +2148,12 @@ static int vpe_probe(struct platform_device *pdev)

drivers/media/platform/ti-vpe/vpe.c +1 -18

 		goto runtime_put;
 	}
 
-	dev->vpdma = vpdma_create(pdev);
+	dev->vpdma = vpdma_create(pdev, vpe_fw_cb);
 	if (IS_ERR(dev->vpdma)) {
 		ret = PTR_ERR(dev->vpdma);
 		goto runtime_put;
 	}
 
-	vfd = &dev->vfd;
-	*vfd = vpe_videodev;
-	vfd->lock = &dev->dev_mutex;
-	vfd->v4l2_dev = &dev->v4l2_dev;
-
-	ret = video_register_device(vfd, VFL_TYPE_GRABBER, 0);
-	if (ret) {
-		vpe_err(dev, "Failed to register video device\n");
-		goto runtime_put;
-	}
-
-	video_set_drvdata(vfd, dev);
-	snprintf(vfd->name, sizeof(vfd->name), "%s", vpe_videodev.name);
-	dev_info(dev->v4l2_dev.dev, "Device registered as /dev/video%d\n",
-		vfd->num);
-
 	return 0;
 
 runtime_put:
--

1.8.3.2

Hi Archit,

Quoted 16 lines

From: Archit Taneja [mailto:[email protected]]
Sent: Tuesday, March 11, 2014 9:34 AM

vpe fops(vpe_open in particular) should be called only when VPDMA firmware is loaded. File operations on the video device are possible the moment it is registered.

Currently, we register the video device for VPE at driver probe, after calling a vpdma helper to initialize VPDMA and load firmware. This function is non-blocking(it calls request_firmware_nowait()), and doesn't ensure that the firmware is actually loaded when it returns.

We remove the device registration from vpe probe, and move it to a callback provided by the vpe driver to the vpdma library, through vpdma_create().

Quoted 7 lines

The ready field in vpdma_data is no longer needed since we always have firmware loaded before the device is registered.

A minor problem with this approach is that if the video_register_device fails(which doesn't really happen), the vpe platform device would be registered.
however, there won't be any v4l2 device corresponding to it.

Could you explain to me one thing. request_firmware cannot be used in probe, thus you are using request_firmware_nowait. Why cannot the firmware be
loaded on open with a regular request_firmware that is waiting?

This patch seems to swap one problem for another. The possibility that open fails (because firmware is not yet loaded) is swapped for a vague possibility
that video_register_device.

From: Archit Taneja [mailto:[email protected]]
Sent: Thursday, March 13, 2014 1:09 PM

Hi Kamil,

Quoted 13 lines · Kamil Debski

Hi Archit,

Quoted 11 lines

From: Archit Taneja [mailto:[email protected]]
Sent: Tuesday, March 11, 2014 9:34 AM

vpe fops(vpe_open in particular) should be called only when VPDMA firmware is loaded. File operations on the video device are possible the moment it is registered.

Currently, we register the video device for VPE at driver probe, after calling a vpdma helper to initialize VPDMA and load firmware. This function is non-blocking(it calls request_firmware_nowait()), and doesn't ensure that the firmware is actually loaded when it

returns.

Quoted 13 lines

Quoted 5 lines

We remove the device registration from vpe probe, and move it to a callback provided by the vpe driver to the vpdma library, through vpdma_create().

Quoted 7 lines

The ready field in vpdma_data is no longer needed since we always have firmware loaded before the device is registered.

A minor problem with this approach is that if the video_register_device fails(which doesn't really happen), the vpe platform device would be registered.
however, there won't be any v4l2 device corresponding to it.

Quoted 4 lines

Could you explain to me one thing. request_firmware cannot be used in probe, thus you are using request_firmware_nowait. Why cannot the firmware be loaded on open with a regular request_firmware that is waiting?

I totally agree with you here. Placing the firmware in open() would probably make more sense.

The reason I didn't place it in open() is because I didn't want to release firmware in a corresponding close(), and be in a situation where the firmware is loaded multiple times in the driver's lifetime.

There are some reasons for doing this. First, it will require more testing with respect to whether the firmware is loaded correctly successive times :), the second is that loading firmware might probably take a bit of time, and we don't want to make applications too slow(I haven't measured the time taken, so I don't have a strong case for this either)

Quoted 2 lines

This patch seems to swap one problem for another. The possibility

that

Quoted 1 line

open fails (because firmware is not yet loaded) is swapped for a

vague

Quoted 1 line

possibility that video_register_device.

The driver will work fine in most cases even without this patch(apart from the possibility mentioned as above).

We could discard this patch from this series, and I can work on a patch which moves firmware loading to the vpe_open() call, and hence solving the issue in the right manner. Does that sound fine?

Thanks,
Archit

Hi,

Quoted 41 lines

From: Archit Taneja [mailto:[email protected]]
Sent: Thursday, March 13, 2014 1:09 PM

Hi Kamil,

Quoted 13 lines · Kamil Debski

Hi Archit,

Quoted 11 lines

From: Archit Taneja [mailto:[email protected]]
Sent: Tuesday, March 11, 2014 9:34 AM

vpe fops(vpe_open in particular) should be called only when VPDMA firmware is loaded. File operations on the video device are possible the moment it is registered.

Currently, we register the video device for VPE at driver probe, after calling a vpdma helper to initialize VPDMA and load firmware. This function is non-blocking(it calls request_firmware_nowait()), and doesn't ensure that the firmware is actually loaded when it

returns.

Quoted 13 lines

Quoted 5 lines

We remove the device registration from vpe probe, and move it to a callback provided by the vpe driver to the vpdma library, through vpdma_create().

Quoted 7 lines

The ready field in vpdma_data is no longer needed since we always have firmware loaded before the device is registered.

A minor problem with this approach is that if the video_register_device fails(which doesn't really happen), the vpe platform device would be registered.
however, there won't be any v4l2 device corresponding to it.

Quoted 4 lines

Could you explain to me one thing. request_firmware cannot be used in probe, thus you are using request_firmware_nowait. Why cannot the firmware be loaded on open with a regular request_firmware that is waiting?

I totally agree with you here. Placing the firmware in open() would probably make more sense.

Quoted 3 lines

The reason I didn't place it in open() is because I didn't want to release firmware in a corresponding close(), and be in a situation where the firmware is loaded multiple times in the driver's lifetime.

Would it be possible to load firmware in open and release it in remove? I know that this would disturb the symmetry between open-release and probe-remove. But this could work.

The video_device struct is currently embedded in the driver data struct vpe_dev. A vpe_dev instance is allocated by the driver, and the memory for the vfd is a part of this struct.

The v4l2 core, however, manages the removal of the vfd region, through the video_device's .release() op, which currently is the helper video_device_release. This causes memory corruption, and leads to issues when we try to re-insert the vpe module.

Use the video_device_release_empty helper function instead

Signed-off-by Archit Taneja <[email protected]>

---
 drivers/media/platform/ti-vpe/vpe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/media/platform/ti-vpe/vpe.c +1 -1

index f1eae67..0363df6 100644
@@ -2000,7 +2000,7 @@ static struct video_device vpe_videodev = {
 	.fops		= &vpe_fops,
 	.ioctl_ops	= &vpe_ioctl_ops,
 	.minor		= -1,
-	.release	= video_device_release,
+	.release	= video_device_release_empty,
 	.vfl_dir	= VFL_DIR_M2M,
 };
 

For OMAP and DRA7x, we generally allocate video and graphics buffers through omapdrm since the corresponding omap-gem driver provides DMM-Tiler backed contiguous buffers. omapdrm is a dma-buf exporter. These buffers are used by other drivers in the video pipeline.

Add VB2_DMABUF flag to the io_modes of the vb2 output and capture queues. This allows the driver to import dma shared buffers.

Signed-off-by Archit Taneja <[email protected]>

---
 drivers/media/platform/ti-vpe/vpe.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/media/platform/ti-vpe/vpe.c +2 -2

index 0363df6..0e7573a 100644
@@ -1770,7 +1770,7 @@ static int queue_init(void *priv, struct vb2_queue *src_vq,
 
 	memset(src_vq, 0, sizeof(*src_vq));
 	src_vq->type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
-	src_vq->io_modes = VB2_MMAP;
+	src_vq->io_modes = VB2_MMAP | VB2_DMABUF;
 	src_vq->drv_priv = ctx;
 	src_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer);
 	src_vq->ops = &vpe_qops;

@@ -1783,7 +1783,7 @@ static int queue_init(void *priv, struct vb2_queue *src_vq,
 
 	memset(dst_vq, 0, sizeof(*dst_vq));
 	dst_vq->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
-	dst_vq->io_modes = VB2_MMAP;
+	dst_vq->io_modes = VB2_MMAP | VB2_DMABUF;
 	dst_vq->drv_priv = ctx;
 	dst_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer);
 	dst_vq->ops = &vpe_qops;

The minimum width and height for VPE input/output was kept as 128 pixels. VPE doesn't have a constraint on the image height, it requires the image width to be at least 16 bytes.

Change the minimum supported dimensions to 32x32. This allows us to de-interlace qcif content. A smaller image size than 32x32 didn't make much sense, so stopped at this.

Signed-off-by Archit Taneja <[email protected]>

---
 drivers/media/platform/ti-vpe/vpe.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/media/platform/ti-vpe/vpe.c +2 -2

index 0e7573a..dbdc338 100644
@@ -49,8 +49,8 @@
 #define VPE_MODULE_NAME "vpe"
 
 /* minimum and maximum frame sizes */
-#define MIN_W		128
-#define MIN_H		128
+#define MIN_W		32
+#define MIN_H		32
 #define MAX_W		1920
 #define MAX_H		1080