[PATCH v3 0/4] VPE improvements

[PATCH v3 0/4] VPE improvements

[Nikhil Devshatwar]

This patchset adds following improvements for the ti-vpe driver.
* Support SEQ_TB format for interlaced buffers
	Some of the video decoders generate interlaced content in SEQ_TB format
	Y top, T bottom in one plane and UV top, UV bottom in another
* Improve multi instance latency
	Improve m2m job scheduling in multi instance use cases
	Start processing even if all buffers aren't present
* N frame de-interlace support
	For N input fields, generate N progressive frames

Archit Taneja (1):
  media: ti-vpe: Use line average de-interlacing for first 2 frames

Nikhil Devshatwar (3):
  media: ti-vpe: Use data offset for getting dma_addr for a plane
  media: ti-vpe: Do not perform job transaction atomically
  media: ti-vpe: Add support for SEQ_TB buffers

 drivers/media/platform/ti-vpe/vpe.c |  191 ++++++++++++++++++++++++++++-------
 1 file changed, 154 insertions(+), 37 deletions(-)

-- 
1.7.9.5

[PATCH v3 1/4] media: ti-vpe: Use data offset for getting dma_addr for a plane

[Nikhil Devshatwar]

The data_offset in v4l2_planes structure will help us point to the start of
data content for that particular plane. This may be useful when a single
buffer contains the data for different planes e.g. Y planes of two fields in
the same buffer. With this, user space can pass queue top field and
bottom field with same dmafd and different data_offsets.

Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
---
Changes from v2:
 * Use data_offset only for OUTPUT stream buffers

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

diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
index 9a081c2..ba26b83 100644
--- a/drivers/media/platform/ti-vpe/vpe.c
+++ b/drivers/media/platform/ti-vpe/vpe.c
@@ -496,6 +496,14 @@ struct vpe_mmr_adb {
 
 #define VPE_SET_MMR_ADB_HDR(ctx, hdr, regs, offset_a)	\
 	VPDMA_SET_MMR_ADB_HDR(ctx->mmr_adb, vpe_mmr_adb, hdr, regs, offset_a)
+
+static inline dma_addr_t vb2_dma_addr_plus_data_offset(struct vb2_buffer *vb,
+	unsigned int plane_no)
+{
+	return vb2_dma_contig_plane_dma_addr(vb, plane_no) +
+		vb->v4l2_planes[plane_no].data_offset;
+}
+
 /*
  * Set the headers for all of the address/data block structures.
  */
@@ -1043,7 +1051,7 @@ static void add_in_dtd(struct vpe_ctx *ctx, int port)
 
 		vpdma_fmt = fmt->vpdma_fmt[plane];
 
-		dma_addr = vb2_dma_contig_plane_dma_addr(vb, plane);
+		dma_addr = vb2_dma_addr_plus_data_offset(vb, plane);
 		if (!dma_addr) {
 			vpe_err(ctx->dev,
 				"acquiring input buffer(%d) dma_addr failed\n",
-- 
1.7.9.5

[PATCH v3 2/4] media: ti-vpe: Use line average de-interlacing for first 2 frames

[Nikhil Devshatwar]

From: Archit Taneja <archit@ti.com>

For n input fields, the VPE de-interlacer creates n - 2 progressive frames.

To support this, we use line average mode of de-interlacer for the first 2
input fields to generate 2 progressive frames. We then revert back to the
preferred EDI method, and create n - 2 frames, creating a sum of n frames.

Signed-off-by: Archit Taneja <archit@ti.com>
Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
---
 drivers/media/platform/ti-vpe/vpe.c |   29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
index ba26b83..26e44a1 100644
--- a/drivers/media/platform/ti-vpe/vpe.c
+++ b/drivers/media/platform/ti-vpe/vpe.c
@@ -807,6 +807,23 @@ static void set_dei_shadow_registers(struct vpe_ctx *ctx)
 	ctx->load_mmrs = true;
 }
 
+static void config_edi_input_mode(struct vpe_ctx *ctx, int mode)
+{
+	struct vpe_mmr_adb *mmr_adb = ctx->mmr_adb.addr;
+	u32 *edi_config_reg = &mmr_adb->dei_regs[3];
+
+	if (mode & 0x2)
+		write_field(edi_config_reg, 1, 1, 2);	/* EDI_ENABLE_3D */
+
+	if (mode & 0x3)
+		write_field(edi_config_reg, 1, 1, 3);	/* EDI_CHROMA_3D  */
+
+	write_field(edi_config_reg, mode, VPE_EDI_INP_MODE_MASK,
+		VPE_EDI_INP_MODE_SHIFT);
+
+	ctx->load_mmrs = true;
+}
+
 /*
  * Set the shadow registers whose values are modified when either the
  * source or destination format is changed.
@@ -1119,6 +1136,15 @@ static void device_run(void *priv)
 	ctx->dst_vb = v4l2_m2m_dst_buf_remove(ctx->m2m_ctx);
 	WARN_ON(ctx->dst_vb == NULL);
 
+	if (ctx->deinterlacing) {
+		/*
+		 * we have output the first 2 frames through line average, we
+		 * now switch to EDI de-interlacer
+		 */
+		if (ctx->sequence == 2)
+			config_edi_input_mode(ctx, 0x3); /* EDI (Y + UV) */
+	}
+
 	/* config descriptors */
 	if (ctx->dev->loaded_mmrs != ctx->mmr_adb.dma_addr || ctx->load_mmrs) {
 		vpdma_map_desc_buf(ctx->dev->vpdma, &ctx->mmr_adb);
@@ -1780,6 +1806,9 @@ static int vpe_streamon(struct file *file, void *priv, enum v4l2_buf_type type)
 {
 	struct vpe_ctx *ctx = file2ctx(file);
 
+	if (ctx->deinterlacing)
+		config_edi_input_mode(ctx, 0x0);
+
 	return v4l2_m2m_streamon(file, ctx->m2m_ctx, type);
 }
 
-- 
1.7.9.5

[PATCH v3 3/4] media: ti-vpe: Do not perform job transaction atomically

[Nikhil Devshatwar]

Current VPE driver does not start the job untill all the buffers for
a transaction are not queued. When running in multiple context, this might
increase the processing latency.

Alternate solution would be to try to continue the same context as long as
buffers for the transaction are ready; else switch the conext. This may
increase number of context switches but it reduces latency significantly.

In this approach, the job_ready always succeeds as long as there are buffers
on the CAPTURE and OUTPUT stream. Processing may start immediately as the
first 2 iterations don't need extra source buffers. Shift all the source buffers
after each iteration and remove the oldest buffer.

Also, with this removes the constraint of pre buffering 3 buffers before call
to STREAMON in case of deinterlacing.

Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
---
 drivers/media/platform/ti-vpe/vpe.c |   32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
index 26e44a1..6a96bbf 100644
--- a/drivers/media/platform/ti-vpe/vpe.c
+++ b/drivers/media/platform/ti-vpe/vpe.c
@@ -907,15 +907,14 @@ static struct vpe_ctx *file2ctx(struct file *file)
 static int job_ready(void *priv)
 {
 	struct vpe_ctx *ctx = priv;
-	int needed = ctx->bufs_per_job;
 
-	if (ctx->deinterlacing && ctx->src_vbs[2] == NULL)
-		needed += 2;	/* need additional two most recent fields */
-
-	if (v4l2_m2m_num_src_bufs_ready(ctx->m2m_ctx) < needed)
-		return 0;
-
-	if (v4l2_m2m_num_dst_bufs_ready(ctx->m2m_ctx) < needed)
+	/*
+	 * This check is needed as this might be called directly from driver
+	 * When called by m2m framework, this will always satisy, but when
+	 * called from vpe_irq, this might fail. (src stream with zero buffers)
+	 */
+	if (v4l2_m2m_num_src_bufs_ready(ctx->m2m_ctx) <= 0 ||
+		v4l2_m2m_num_dst_bufs_ready(ctx->m2m_ctx) <= 0)
 		return 0;
 
 	return 1;
@@ -1124,19 +1123,20 @@ static void device_run(void *priv)
 	struct sc_data *sc = ctx->dev->sc;
 	struct vpe_q_data *d_q_data = &ctx->q_data[Q_DATA_DST];
 
-	if (ctx->deinterlacing && ctx->src_vbs[2] == NULL) {
-		ctx->src_vbs[2] = v4l2_m2m_src_buf_remove(ctx->m2m_ctx);
-		WARN_ON(ctx->src_vbs[2] == NULL);
-		ctx->src_vbs[1] = v4l2_m2m_src_buf_remove(ctx->m2m_ctx);
-		WARN_ON(ctx->src_vbs[1] == NULL);
-	}
-
 	ctx->src_vbs[0] = v4l2_m2m_src_buf_remove(ctx->m2m_ctx);
 	WARN_ON(ctx->src_vbs[0] == NULL);
 	ctx->dst_vb = v4l2_m2m_dst_buf_remove(ctx->m2m_ctx);
 	WARN_ON(ctx->dst_vb == NULL);
 
 	if (ctx->deinterlacing) {
+
+		if (ctx->src_vbs[2] == NULL) {
+			ctx->src_vbs[2] = ctx->src_vbs[0];
+			WARN_ON(ctx->src_vbs[2] == NULL);
+			ctx->src_vbs[1] = ctx->src_vbs[0];
+			WARN_ON(ctx->src_vbs[1] == NULL);
+		}
+
 		/*
 		 * we have output the first 2 frames through line average, we
 		 * now switch to EDI de-interlacer
@@ -1360,7 +1360,7 @@ static irqreturn_t vpe_irq(int irq_vpe, void *data)
 	}
 
 	ctx->bufs_completed++;
-	if (ctx->bufs_completed < ctx->bufs_per_job) {
+	if (ctx->bufs_completed < ctx->bufs_per_job && job_ready(ctx)) {
 		device_run(ctx);
 		goto handled;
 	}
-- 
1.7.9.5

[PATCH v3 4/4] media: ti-vpe: Add support for SEQ_TB buffers

[Nikhil Devshatwar]

The video source can generate the data in the SEQ_TB buffer format.
In the case of TI SoC, the IVA_HD can generate the interlaced content in
the SEQ_TB buffer format. This is the format where the top and bottom field
data can be contained in a single buffer. For example, for NV12, interlaced
format, the data in Y buffer will be arranged as Y-top followed by Y-bottom.
And likewise for UV plane.

Also, queueing one buffer of SEQ_TB is euivalent to queueing two different
buffers for top and bottom fields. Driver needs to take care of this when
handling source buffer lists.

Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
---
Changes from v1:
 * Add check for valid field in qbuf ioctl
 * Fix issue with swapped fields

 drivers/media/platform/ti-vpe/vpe.c |  124 ++++++++++++++++++++++++++++-------
 1 file changed, 102 insertions(+), 22 deletions(-)

diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
index 6a96bbf..b53aea5 100644
--- a/drivers/media/platform/ti-vpe/vpe.c
+++ b/drivers/media/platform/ti-vpe/vpe.c
@@ -319,9 +319,13 @@ struct vpe_q_data {
 };
 
 /* vpe_q_data flag bits */
-#define	Q_DATA_FRAME_1D		(1 << 0)
-#define	Q_DATA_MODE_TILED	(1 << 1)
-#define	Q_DATA_INTERLACED	(1 << 2)
+#define	Q_DATA_FRAME_1D			(1 << 0)
+#define	Q_DATA_MODE_TILED		(1 << 1)
+#define	Q_DATA_INTERLACED_ALTERNATE	(1 << 2)
+#define	Q_DATA_INTERLACED_SEQ_TB	(1 << 3)
+
+#define Q_IS_INTERLACED		(Q_DATA_INTERLACED_ALTERNATE | \
+				Q_DATA_INTERLACED_SEQ_TB)
 
 enum {
 	Q_DATA_SRC = 0,
@@ -647,7 +651,7 @@ static void set_us_coefficients(struct vpe_ctx *ctx)
 
 	cp = &us_coeffs[0].anchor_fid0_c0;
 
-	if (s_q_data->flags & Q_DATA_INTERLACED)	/* interlaced */
+	if (s_q_data->flags & Q_IS_INTERLACED)		/* interlaced */
 		cp += sizeof(us_coeffs[0]) / sizeof(*cp);
 
 	end_cp = cp + sizeof(us_coeffs[0]) / sizeof(*cp);
@@ -774,8 +778,7 @@ static void set_dei_regs(struct vpe_ctx *ctx)
 	 * for both progressive and interlace content in interlace bypass mode.
 	 * It has been recommended not to use progressive bypass mode.
 	 */
-	if ((!ctx->deinterlacing && (s_q_data->flags & Q_DATA_INTERLACED)) ||
-			!(s_q_data->flags & Q_DATA_INTERLACED)) {
+	if (!(s_q_data->flags & Q_IS_INTERLACED) || !ctx->deinterlacing) {
 		deinterlace = false;
 		val = VPE_DEI_INTERLACE_BYPASS;
 	}
@@ -843,8 +846,8 @@ static int set_srcdst_params(struct vpe_ctx *ctx)
 	ctx->sequence = 0;
 	ctx->field = V4L2_FIELD_TOP;
 
-	if ((s_q_data->flags & Q_DATA_INTERLACED) &&
-			!(d_q_data->flags & Q_DATA_INTERLACED)) {
+	if ((s_q_data->flags & Q_IS_INTERLACED) &&
+			!(d_q_data->flags & Q_IS_INTERLACED)) {
 		int bytes_per_line;
 		const struct vpdma_data_format *mv =
 			&vpdma_misc_fmts[VPDMA_DATA_FMT_MV];
@@ -1068,6 +1071,27 @@ static void add_in_dtd(struct vpe_ctx *ctx, int port)
 		vpdma_fmt = fmt->vpdma_fmt[plane];
 
 		dma_addr = vb2_dma_addr_plus_data_offset(vb, plane);
+
+		if (q_data->flags & Q_DATA_INTERLACED_SEQ_TB) {
+			/*
+			 * Use top or bottom field from same vb alternately
+			 * f,f-1,f-2 = TBT when seq is even
+			 * f,f-1,f-2 = BTB when seq is odd
+			 */
+			field = (p_data->vb_index + (ctx->sequence % 2)) % 2;
+
+			if (field) {
+				/* bottom field of a SEQ_TB buffer
+				 * Skip the top field data by */
+				int height = q_data->height / 2;
+				int bpp = fmt->fourcc == V4L2_PIX_FMT_NV12 ?
+						1 : (vpdma_fmt->depth >> 3);
+				if (plane)
+					height /= 2;
+				dma_addr += q_data->width * height * bpp;
+			}
+		}
+
 		if (!dma_addr) {
 			vpe_err(ctx->dev,
 				"acquiring input buffer(%d) dma_addr failed\n",
@@ -1122,9 +1146,22 @@ static void device_run(void *priv)
 	struct vpe_ctx *ctx = priv;
 	struct sc_data *sc = ctx->dev->sc;
 	struct vpe_q_data *d_q_data = &ctx->q_data[Q_DATA_DST];
+	struct vpe_q_data *s_q_data = &ctx->q_data[Q_DATA_SRC];
+
+	if (ctx->deinterlacing && s_q_data->flags & Q_DATA_INTERLACED_SEQ_TB &&
+		ctx->sequence % 2 == 0) {
+		/* When using SEQ_TB buffers, When using it first time,
+		 * No need to remove the buffer as the next field is present
+		 * in the same buffer. (so that job_ready won't fail)
+		 * It will be removed when using bottom field
+		 */
+		ctx->src_vbs[0] = v4l2_m2m_next_src_buf(ctx->m2m_ctx);
+		WARN_ON(ctx->src_vbs[0] == NULL);
+	} else {
+		ctx->src_vbs[0] = v4l2_m2m_src_buf_remove(ctx->m2m_ctx);
+		WARN_ON(ctx->src_vbs[0] == NULL);
+	}
 
-	ctx->src_vbs[0] = v4l2_m2m_src_buf_remove(ctx->m2m_ctx);
-	WARN_ON(ctx->src_vbs[0] == NULL);
 	ctx->dst_vb = v4l2_m2m_dst_buf_remove(ctx->m2m_ctx);
 	WARN_ON(ctx->dst_vb == NULL);
 
@@ -1256,6 +1293,7 @@ static irqreturn_t vpe_irq(int irq_vpe, void *data)
 	struct vpe_dev *dev = (struct vpe_dev *)data;
 	struct vpe_ctx *ctx;
 	struct vpe_q_data *d_q_data;
+	struct vpe_q_data *s_q_data;
 	struct vb2_buffer *s_vb, *d_vb;
 	struct v4l2_buffer *s_buf, *d_buf;
 	unsigned long flags;
@@ -1332,7 +1370,7 @@ static irqreturn_t vpe_irq(int irq_vpe, void *data)
 	d_buf->sequence = ctx->sequence;
 
 	d_q_data = &ctx->q_data[Q_DATA_DST];
-	if (d_q_data->flags & Q_DATA_INTERLACED) {
+	if (d_q_data->flags & Q_IS_INTERLACED) {
 		d_buf->field = ctx->field;
 		if (ctx->field == V4L2_FIELD_BOTTOM) {
 			ctx->sequence++;
@@ -1346,12 +1384,28 @@ static irqreturn_t vpe_irq(int irq_vpe, void *data)
 		ctx->sequence++;
 	}
 
-	if (ctx->deinterlacing)
-		s_vb = ctx->src_vbs[2];
+	s_q_data = &ctx->q_data[Q_DATA_SRC];
+
+	if (ctx->deinterlacing) {
+		/* Allow source buffer to be dequeued only if it won't be used
+		 * in the next iteration. All vbs are initialized to first
+		 * buffer and we are shifting buffers every iteration, for the
+		 * first two iterations, no buffer will be dequeued.
+		 * This ensures that driver will keep (n-2)th (n-1)th and (n)th
+		 * field when deinterlacing is enabled */
+		if (ctx->src_vbs[2] != ctx->src_vbs[1])
+			s_vb = ctx->src_vbs[2];
+		else
+			s_vb = NULL;
+	}
 
 	spin_lock_irqsave(&dev->lock, flags);
-	v4l2_m2m_buf_done(s_vb, VB2_BUF_STATE_DONE);
+
+	if (s_vb)
+		v4l2_m2m_buf_done(s_vb, VB2_BUF_STATE_DONE);
+
 	v4l2_m2m_buf_done(d_vb, VB2_BUF_STATE_DONE);
+
 	spin_unlock_irqrestore(&dev->lock, flags);
 
 	if (ctx->deinterlacing) {
@@ -1467,7 +1521,7 @@ static int __vpe_try_fmt(struct vpe_ctx *ctx, struct v4l2_format *f,
 	struct v4l2_pix_format_mplane *pix = &f->fmt.pix_mp;
 	struct v4l2_plane_pix_format *plane_fmt;
 	unsigned int w_align;
-	int i, depth, depth_bytes;
+	int i, depth, depth_bytes, height;
 
 	if (!fmt || !(fmt->types & type)) {
 		vpe_err(ctx->dev, "Fourcc format (0x%08x) invalid.\n",
@@ -1475,7 +1529,8 @@ static int __vpe_try_fmt(struct vpe_ctx *ctx, struct v4l2_format *f,
 		return -EINVAL;
 	}
 
-	if (pix->field != V4L2_FIELD_NONE && pix->field != V4L2_FIELD_ALTERNATE)
+	if (pix->field != V4L2_FIELD_NONE && pix->field != V4L2_FIELD_ALTERNATE
+			&& pix->field != V4L2_FIELD_SEQ_TB)
 		pix->field = V4L2_FIELD_NONE;
 
 	depth = fmt->vpdma_fmt[VPE_LUMA]->depth;
@@ -1509,6 +1564,14 @@ static int __vpe_try_fmt(struct vpe_ctx *ctx, struct v4l2_format *f,
 	pix->num_planes = fmt->coplanar ? 2 : 1;
 	pix->pixelformat = fmt->fourcc;
 
+	/* for the actual image parameters, we need to consider the field height
+	 * of the image for SEQ_TB buffers.
+	 */
+	if (pix->field == V4L2_FIELD_SEQ_TB)
+		height = pix->height / 2;
+	else
+		height = pix->height;
+
 	if (!pix->colorspace) {
 		if (fmt->fourcc == V4L2_PIX_FMT_RGB24 ||
 				fmt->fourcc == V4L2_PIX_FMT_BGR24 ||
@@ -1516,7 +1579,7 @@ static int __vpe_try_fmt(struct vpe_ctx *ctx, struct v4l2_format *f,
 				fmt->fourcc == V4L2_PIX_FMT_BGR32) {
 			pix->colorspace = V4L2_COLORSPACE_SRGB;
 		} else {
-			if (pix->height > 1280)	/* HD */
+			if (height > 1280)	/* HD */
 				pix->colorspace = V4L2_COLORSPACE_REC709;
 			else			/* SD */
 				pix->colorspace = V4L2_COLORSPACE_SMPTE170M;
@@ -1593,9 +1656,15 @@ static int __vpe_s_fmt(struct vpe_ctx *ctx, struct v4l2_format *f)
 	q_data->c_rect.height	= q_data->height;
 
 	if (q_data->field == V4L2_FIELD_ALTERNATE)
-		q_data->flags |= Q_DATA_INTERLACED;
+		q_data->flags |= Q_DATA_INTERLACED_ALTERNATE;
+	else if (q_data->field == V4L2_FIELD_SEQ_TB)
+		q_data->flags |= Q_DATA_INTERLACED_SEQ_TB;
 	else
-		q_data->flags &= ~Q_DATA_INTERLACED;
+		q_data->flags &= ~Q_IS_INTERLACED;
+
+	/* the crop height is halved for the case of SEQ_TB buffers */
+	if (q_data->flags & Q_DATA_INTERLACED_SEQ_TB)
+		q_data->c_rect.height /= 2;
 
 	vpe_dbg(ctx->dev, "Setting format for type %d, wxh: %dx%d, fmt: %d bpl_y %d",
 		f->type, q_data->width, q_data->height, q_data->fmt->fourcc,
@@ -1631,6 +1700,7 @@ static int vpe_s_fmt(struct file *file, void *priv, struct v4l2_format *f)
 static int __vpe_try_selection(struct vpe_ctx *ctx, struct v4l2_selection *s)
 {
 	struct vpe_q_data *q_data;
+	int height;
 
 	if ((s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
 	    (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT))
@@ -1665,13 +1735,22 @@ static int __vpe_try_selection(struct vpe_ctx *ctx, struct v4l2_selection *s)
 		return -EINVAL;
 	}
 
+	/*
+	* For SEQ_TB buffers, crop height should be less than the height of
+	* the field height, not the buffer height
+	*/
+	if (q_data->flags & Q_DATA_INTERLACED_SEQ_TB)
+		height = q_data->height / 2;
+	else
+		height = q_data->height;
+
 	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);
+		&s->r.height, MIN_H, 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)
@@ -1919,11 +1998,12 @@ static int vpe_buf_prepare(struct vb2_buffer *vb)
 	num_planes = q_data->fmt->coplanar ? 2 : 1;
 
 	if (vb->vb2_queue->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
-		if (!(q_data->flags & Q_DATA_INTERLACED)) {
+		if (!(q_data->flags & Q_IS_INTERLACED)) {
 			vb->v4l2_buf.field = V4L2_FIELD_NONE;
 		} else {
 			if (vb->v4l2_buf.field != V4L2_FIELD_TOP &&
-					vb->v4l2_buf.field != V4L2_FIELD_BOTTOM)
+			vb->v4l2_buf.field != V4L2_FIELD_BOTTOM &&
+			vb->v4l2_buf.field != V4L2_FIELD_SEQ_TB)
 				return -EINVAL;
 		}
 	}
-- 
1.7.9.5

Re: [PATCH v3 1/4] media: ti-vpe: Use data offset for getting dma_addr for a plane

[Hans Verkuil]

On 11/29/2014 11:27 AM, Nikhil Devshatwar wrote:
> The data_offset in v4l2_planes structure will help us point to the start of
> data content for that particular plane. This may be useful when a single
> buffer contains the data for different planes e.g. Y planes of two fields in
> the same buffer. With this, user space can pass queue top field and
> bottom field with same dmafd and different data_offsets.
> 
> Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
> ---
> Changes from v2:
>  * Use data_offset only for OUTPUT stream buffers
> 
>  drivers/media/platform/ti-vpe/vpe.c |   10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
> index 9a081c2..ba26b83 100644
> --- a/drivers/media/platform/ti-vpe/vpe.c
> +++ b/drivers/media/platform/ti-vpe/vpe.c
> @@ -496,6 +496,14 @@ struct vpe_mmr_adb {
>  
>  #define VPE_SET_MMR_ADB_HDR(ctx, hdr, regs, offset_a)	\
>  	VPDMA_SET_MMR_ADB_HDR(ctx->mmr_adb, vpe_mmr_adb, hdr, regs, offset_a)
> +
> +static inline dma_addr_t vb2_dma_addr_plus_data_offset(struct vb2_buffer *vb,
> +	unsigned int plane_no)
> +{
> +	return vb2_dma_contig_plane_dma_addr(vb, plane_no) +
> +		vb->v4l2_planes[plane_no].data_offset;
> +}
> +
>  /*
>   * Set the headers for all of the address/data block structures.
>   */
> @@ -1043,7 +1051,7 @@ static void add_in_dtd(struct vpe_ctx *ctx, int port)
>  
>  		vpdma_fmt = fmt->vpdma_fmt[plane];
>  
> -		dma_addr = vb2_dma_contig_plane_dma_addr(vb, plane);
> +		dma_addr = vb2_dma_addr_plus_data_offset(vb, plane);
>  		if (!dma_addr) {
>  			vpe_err(ctx->dev,
>  				"acquiring input buffer(%d) dma_addr failed\n",
> 

Should there be a check somewhere that verifies that vb2_get_plane_payload() -
vb->v4l2_planes[plane_no].data_offset is still large enough for the image you
want to copy?

Regards,

	Hans

Re: [PATCH v3 4/4] media: ti-vpe: Add support for SEQ_TB buffers

[Hans Verkuil]

On 11/29/2014 11:27 AM, Nikhil Devshatwar wrote:
> The video source can generate the data in the SEQ_TB buffer format.
> In the case of TI SoC, the IVA_HD can generate the interlaced content in
> the SEQ_TB buffer format. This is the format where the top and bottom field
> data can be contained in a single buffer. For example, for NV12, interlaced
> format, the data in Y buffer will be arranged as Y-top followed by Y-bottom.
> And likewise for UV plane.
> 
> Also, queueing one buffer of SEQ_TB is euivalent to queueing two different

queueing -> queuing 
euivalent -> equivalent

> buffers for top and bottom fields. Driver needs to take care of this when
> handling source buffer lists.
> 
> Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>

Currently you are supporting SEQ_TB. Should SEQ_BT also be supported? I don't
know if you are aware but NTSC transmits the bottom field before the top field,
while PAL/SECAM transmit top before bottom.

So for NTSC support having SEQ_BT might be useful. Up to you, though.

Regards,

	Hans

> ---
> Changes from v1:
>  * Add check for valid field in qbuf ioctl
>  * Fix issue with swapped fields
> 
>  drivers/media/platform/ti-vpe/vpe.c |  124 ++++++++++++++++++++++++++++-------
>  1 file changed, 102 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
> index 6a96bbf..b53aea5 100644
> --- a/drivers/media/platform/ti-vpe/vpe.c
> +++ b/drivers/media/platform/ti-vpe/vpe.c
> @@ -319,9 +319,13 @@ struct vpe_q_data {
>  };
>  
>  /* vpe_q_data flag bits */
> -#define	Q_DATA_FRAME_1D		(1 << 0)
> -#define	Q_DATA_MODE_TILED	(1 << 1)
> -#define	Q_DATA_INTERLACED	(1 << 2)
> +#define	Q_DATA_FRAME_1D			(1 << 0)
> +#define	Q_DATA_MODE_TILED		(1 << 1)
> +#define	Q_DATA_INTERLACED_ALTERNATE	(1 << 2)
> +#define	Q_DATA_INTERLACED_SEQ_TB	(1 << 3)
> +
> +#define Q_IS_INTERLACED		(Q_DATA_INTERLACED_ALTERNATE | \
> +				Q_DATA_INTERLACED_SEQ_TB)
>  
>  enum {
>  	Q_DATA_SRC = 0,
> @@ -647,7 +651,7 @@ static void set_us_coefficients(struct vpe_ctx *ctx)
>  
>  	cp = &us_coeffs[0].anchor_fid0_c0;
>  
> -	if (s_q_data->flags & Q_DATA_INTERLACED)	/* interlaced */
> +	if (s_q_data->flags & Q_IS_INTERLACED)		/* interlaced */
>  		cp += sizeof(us_coeffs[0]) / sizeof(*cp);
>  
>  	end_cp = cp + sizeof(us_coeffs[0]) / sizeof(*cp);
> @@ -774,8 +778,7 @@ static void set_dei_regs(struct vpe_ctx *ctx)
>  	 * for both progressive and interlace content in interlace bypass mode.
>  	 * It has been recommended not to use progressive bypass mode.
>  	 */
> -	if ((!ctx->deinterlacing && (s_q_data->flags & Q_DATA_INTERLACED)) ||
> -			!(s_q_data->flags & Q_DATA_INTERLACED)) {
> +	if (!(s_q_data->flags & Q_IS_INTERLACED) || !ctx->deinterlacing) {
>  		deinterlace = false;
>  		val = VPE_DEI_INTERLACE_BYPASS;
>  	}
> @@ -843,8 +846,8 @@ static int set_srcdst_params(struct vpe_ctx *ctx)
>  	ctx->sequence = 0;
>  	ctx->field = V4L2_FIELD_TOP;
>  
> -	if ((s_q_data->flags & Q_DATA_INTERLACED) &&
> -			!(d_q_data->flags & Q_DATA_INTERLACED)) {
> +	if ((s_q_data->flags & Q_IS_INTERLACED) &&
> +			!(d_q_data->flags & Q_IS_INTERLACED)) {
>  		int bytes_per_line;
>  		const struct vpdma_data_format *mv =
>  			&vpdma_misc_fmts[VPDMA_DATA_FMT_MV];
> @@ -1068,6 +1071,27 @@ static void add_in_dtd(struct vpe_ctx *ctx, int port)
>  		vpdma_fmt = fmt->vpdma_fmt[plane];
>  
>  		dma_addr = vb2_dma_addr_plus_data_offset(vb, plane);
> +
> +		if (q_data->flags & Q_DATA_INTERLACED_SEQ_TB) {
> +			/*
> +			 * Use top or bottom field from same vb alternately
> +			 * f,f-1,f-2 = TBT when seq is even
> +			 * f,f-1,f-2 = BTB when seq is odd
> +			 */
> +			field = (p_data->vb_index + (ctx->sequence % 2)) % 2;
> +
> +			if (field) {
> +				/* bottom field of a SEQ_TB buffer
> +				 * Skip the top field data by */
> +				int height = q_data->height / 2;
> +				int bpp = fmt->fourcc == V4L2_PIX_FMT_NV12 ?
> +						1 : (vpdma_fmt->depth >> 3);
> +				if (plane)
> +					height /= 2;
> +				dma_addr += q_data->width * height * bpp;
> +			}
> +		}
> +
>  		if (!dma_addr) {
>  			vpe_err(ctx->dev,
>  				"acquiring input buffer(%d) dma_addr failed\n",
> @@ -1122,9 +1146,22 @@ static void device_run(void *priv)
>  	struct vpe_ctx *ctx = priv;
>  	struct sc_data *sc = ctx->dev->sc;
>  	struct vpe_q_data *d_q_data = &ctx->q_data[Q_DATA_DST];
> +	struct vpe_q_data *s_q_data = &ctx->q_data[Q_DATA_SRC];
> +
> +	if (ctx->deinterlacing && s_q_data->flags & Q_DATA_INTERLACED_SEQ_TB &&
> +		ctx->sequence % 2 == 0) {
> +		/* When using SEQ_TB buffers, When using it first time,
> +		 * No need to remove the buffer as the next field is present
> +		 * in the same buffer. (so that job_ready won't fail)
> +		 * It will be removed when using bottom field
> +		 */
> +		ctx->src_vbs[0] = v4l2_m2m_next_src_buf(ctx->m2m_ctx);
> +		WARN_ON(ctx->src_vbs[0] == NULL);
> +	} else {
> +		ctx->src_vbs[0] = v4l2_m2m_src_buf_remove(ctx->m2m_ctx);
> +		WARN_ON(ctx->src_vbs[0] == NULL);
> +	}
>  
> -	ctx->src_vbs[0] = v4l2_m2m_src_buf_remove(ctx->m2m_ctx);
> -	WARN_ON(ctx->src_vbs[0] == NULL);
>  	ctx->dst_vb = v4l2_m2m_dst_buf_remove(ctx->m2m_ctx);
>  	WARN_ON(ctx->dst_vb == NULL);
>  
> @@ -1256,6 +1293,7 @@ static irqreturn_t vpe_irq(int irq_vpe, void *data)
>  	struct vpe_dev *dev = (struct vpe_dev *)data;
>  	struct vpe_ctx *ctx;
>  	struct vpe_q_data *d_q_data;
> +	struct vpe_q_data *s_q_data;
>  	struct vb2_buffer *s_vb, *d_vb;
>  	struct v4l2_buffer *s_buf, *d_buf;
>  	unsigned long flags;
> @@ -1332,7 +1370,7 @@ static irqreturn_t vpe_irq(int irq_vpe, void *data)
>  	d_buf->sequence = ctx->sequence;
>  
>  	d_q_data = &ctx->q_data[Q_DATA_DST];
> -	if (d_q_data->flags & Q_DATA_INTERLACED) {
> +	if (d_q_data->flags & Q_IS_INTERLACED) {
>  		d_buf->field = ctx->field;
>  		if (ctx->field == V4L2_FIELD_BOTTOM) {
>  			ctx->sequence++;
> @@ -1346,12 +1384,28 @@ static irqreturn_t vpe_irq(int irq_vpe, void *data)
>  		ctx->sequence++;
>  	}
>  
> -	if (ctx->deinterlacing)
> -		s_vb = ctx->src_vbs[2];
> +	s_q_data = &ctx->q_data[Q_DATA_SRC];
> +
> +	if (ctx->deinterlacing) {
> +		/* Allow source buffer to be dequeued only if it won't be used
> +		 * in the next iteration. All vbs are initialized to first
> +		 * buffer and we are shifting buffers every iteration, for the
> +		 * first two iterations, no buffer will be dequeued.
> +		 * This ensures that driver will keep (n-2)th (n-1)th and (n)th
> +		 * field when deinterlacing is enabled */
> +		if (ctx->src_vbs[2] != ctx->src_vbs[1])
> +			s_vb = ctx->src_vbs[2];
> +		else
> +			s_vb = NULL;
> +	}
>  
>  	spin_lock_irqsave(&dev->lock, flags);
> -	v4l2_m2m_buf_done(s_vb, VB2_BUF_STATE_DONE);
> +
> +	if (s_vb)
> +		v4l2_m2m_buf_done(s_vb, VB2_BUF_STATE_DONE);
> +
>  	v4l2_m2m_buf_done(d_vb, VB2_BUF_STATE_DONE);
> +
>  	spin_unlock_irqrestore(&dev->lock, flags);
>  
>  	if (ctx->deinterlacing) {
> @@ -1467,7 +1521,7 @@ static int __vpe_try_fmt(struct vpe_ctx *ctx, struct v4l2_format *f,
>  	struct v4l2_pix_format_mplane *pix = &f->fmt.pix_mp;
>  	struct v4l2_plane_pix_format *plane_fmt;
>  	unsigned int w_align;
> -	int i, depth, depth_bytes;
> +	int i, depth, depth_bytes, height;
>  
>  	if (!fmt || !(fmt->types & type)) {
>  		vpe_err(ctx->dev, "Fourcc format (0x%08x) invalid.\n",
> @@ -1475,7 +1529,8 @@ static int __vpe_try_fmt(struct vpe_ctx *ctx, struct v4l2_format *f,
>  		return -EINVAL;
>  	}
>  
> -	if (pix->field != V4L2_FIELD_NONE && pix->field != V4L2_FIELD_ALTERNATE)
> +	if (pix->field != V4L2_FIELD_NONE && pix->field != V4L2_FIELD_ALTERNATE
> +			&& pix->field != V4L2_FIELD_SEQ_TB)
>  		pix->field = V4L2_FIELD_NONE;
>  
>  	depth = fmt->vpdma_fmt[VPE_LUMA]->depth;
> @@ -1509,6 +1564,14 @@ static int __vpe_try_fmt(struct vpe_ctx *ctx, struct v4l2_format *f,
>  	pix->num_planes = fmt->coplanar ? 2 : 1;
>  	pix->pixelformat = fmt->fourcc;
>  
> +	/* for the actual image parameters, we need to consider the field height
> +	 * of the image for SEQ_TB buffers.
> +	 */
> +	if (pix->field == V4L2_FIELD_SEQ_TB)
> +		height = pix->height / 2;
> +	else
> +		height = pix->height;
> +
>  	if (!pix->colorspace) {
>  		if (fmt->fourcc == V4L2_PIX_FMT_RGB24 ||
>  				fmt->fourcc == V4L2_PIX_FMT_BGR24 ||
> @@ -1516,7 +1579,7 @@ static int __vpe_try_fmt(struct vpe_ctx *ctx, struct v4l2_format *f,
>  				fmt->fourcc == V4L2_PIX_FMT_BGR32) {
>  			pix->colorspace = V4L2_COLORSPACE_SRGB;
>  		} else {
> -			if (pix->height > 1280)	/* HD */
> +			if (height > 1280)	/* HD */
>  				pix->colorspace = V4L2_COLORSPACE_REC709;
>  			else			/* SD */
>  				pix->colorspace = V4L2_COLORSPACE_SMPTE170M;
> @@ -1593,9 +1656,15 @@ static int __vpe_s_fmt(struct vpe_ctx *ctx, struct v4l2_format *f)
>  	q_data->c_rect.height	= q_data->height;
>  
>  	if (q_data->field == V4L2_FIELD_ALTERNATE)
> -		q_data->flags |= Q_DATA_INTERLACED;
> +		q_data->flags |= Q_DATA_INTERLACED_ALTERNATE;
> +	else if (q_data->field == V4L2_FIELD_SEQ_TB)
> +		q_data->flags |= Q_DATA_INTERLACED_SEQ_TB;
>  	else
> -		q_data->flags &= ~Q_DATA_INTERLACED;
> +		q_data->flags &= ~Q_IS_INTERLACED;
> +
> +	/* the crop height is halved for the case of SEQ_TB buffers */
> +	if (q_data->flags & Q_DATA_INTERLACED_SEQ_TB)
> +		q_data->c_rect.height /= 2;
>  
>  	vpe_dbg(ctx->dev, "Setting format for type %d, wxh: %dx%d, fmt: %d bpl_y %d",
>  		f->type, q_data->width, q_data->height, q_data->fmt->fourcc,
> @@ -1631,6 +1700,7 @@ static int vpe_s_fmt(struct file *file, void *priv, struct v4l2_format *f)
>  static int __vpe_try_selection(struct vpe_ctx *ctx, struct v4l2_selection *s)
>  {
>  	struct vpe_q_data *q_data;
> +	int height;
>  
>  	if ((s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
>  	    (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT))
> @@ -1665,13 +1735,22 @@ static int __vpe_try_selection(struct vpe_ctx *ctx, struct v4l2_selection *s)
>  		return -EINVAL;
>  	}
>  
> +	/*
> +	* For SEQ_TB buffers, crop height should be less than the height of
> +	* the field height, not the buffer height
> +	*/
> +	if (q_data->flags & Q_DATA_INTERLACED_SEQ_TB)
> +		height = q_data->height / 2;
> +	else
> +		height = q_data->height;
> +
>  	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);
> +		&s->r.height, MIN_H, 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)
> @@ -1919,11 +1998,12 @@ static int vpe_buf_prepare(struct vb2_buffer *vb)
>  	num_planes = q_data->fmt->coplanar ? 2 : 1;
>  
>  	if (vb->vb2_queue->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
> -		if (!(q_data->flags & Q_DATA_INTERLACED)) {
> +		if (!(q_data->flags & Q_IS_INTERLACED)) {
>  			vb->v4l2_buf.field = V4L2_FIELD_NONE;
>  		} else {
>  			if (vb->v4l2_buf.field != V4L2_FIELD_TOP &&
> -					vb->v4l2_buf.field != V4L2_FIELD_BOTTOM)
> +			vb->v4l2_buf.field != V4L2_FIELD_BOTTOM &&
> +			vb->v4l2_buf.field != V4L2_FIELD_SEQ_TB)
>  				return -EINVAL;
>  		}
>  	}
> 

RE: [PATCH v3 1/4] media: ti-vpe: Use data offset for getting dma_addr for a plane

[Devshatwar, Nikhil]

> -----Original Message-----
> From: Hans Verkuil [mailto:hverkuil@xs4all.nl]
> Sent: Monday, December 01, 2014 4:18 PM
> To: Devshatwar, Nikhil; linux-media@vger.kernel.org
> Subject: Re: [PATCH v3 1/4] media: ti-vpe: Use data offset for getting
> dma_addr for a plane
> 
> On 11/29/2014 11:27 AM, Nikhil Devshatwar wrote:
> > The data_offset in v4l2_planes structure will help us point to the
> > start of data content for that particular plane. This may be useful
> > when a single buffer contains the data for different planes e.g. Y
> > planes of two fields in the same buffer. With this, user space can
> > pass queue top field and bottom field with same dmafd and different
> data_offsets.
> >
> > Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
> > ---
> > Changes from v2:
> >  * Use data_offset only for OUTPUT stream buffers
> >
> >  drivers/media/platform/ti-vpe/vpe.c |   10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/platform/ti-vpe/vpe.c
> > b/drivers/media/platform/ti-vpe/vpe.c
> > index 9a081c2..ba26b83 100644
> > --- a/drivers/media/platform/ti-vpe/vpe.c
> > +++ b/drivers/media/platform/ti-vpe/vpe.c
> > @@ -496,6 +496,14 @@ struct vpe_mmr_adb {
> >
> >  #define VPE_SET_MMR_ADB_HDR(ctx, hdr, regs, offset_a)	\
> >  	VPDMA_SET_MMR_ADB_HDR(ctx->mmr_adb, vpe_mmr_adb, hdr, regs,
> > offset_a)
> > +
> > +static inline dma_addr_t vb2_dma_addr_plus_data_offset(struct
> vb2_buffer *vb,
> > +	unsigned int plane_no)
> > +{
> > +	return vb2_dma_contig_plane_dma_addr(vb, plane_no) +
> > +		vb->v4l2_planes[plane_no].data_offset;
> > +}
> > +
> >  /*
> >   * Set the headers for all of the address/data block structures.
> >   */
> > @@ -1043,7 +1051,7 @@ static void add_in_dtd(struct vpe_ctx *ctx, int
> > port)
> >
> >  		vpdma_fmt = fmt->vpdma_fmt[plane];
> >
> > -		dma_addr = vb2_dma_contig_plane_dma_addr(vb, plane);
> > +		dma_addr = vb2_dma_addr_plus_data_offset(vb, plane);
> >  		if (!dma_addr) {
> >  			vpe_err(ctx->dev,
> >  				"acquiring input buffer(%d) dma_addr failed\n",
> >
> 
> Should there be a check somewhere that verifies that
> vb2_get_plane_payload() -
> vb->v4l2_planes[plane_no].data_offset is still large enough for the
> vb->image you
> want to copy?

Yes, it is done as part of the vb2_qbuf -> __buf_prepare -> __verify_length function
If the data_offset is high enough that it goes out of the length of that plane,
The qbuf ioctl should have failed, So we can safely assume the validity of data_offset here

> 
> Regards,
> 
> 	Hans

Re: [PATCH v3 1/4] media: ti-vpe: Use data offset for getting dma_addr for a plane

[Hans Verkuil]

On 12/01/2014 12:00 PM, Devshatwar, Nikhil wrote:
>> -----Original Message-----
>> From: Hans Verkuil [mailto:hverkuil@xs4all.nl]
>> Sent: Monday, December 01, 2014 4:18 PM
>> To: Devshatwar, Nikhil; linux-media@vger.kernel.org
>> Subject: Re: [PATCH v3 1/4] media: ti-vpe: Use data offset for getting
>> dma_addr for a plane
>>
>> On 11/29/2014 11:27 AM, Nikhil Devshatwar wrote:
>>> The data_offset in v4l2_planes structure will help us point to the
>>> start of data content for that particular plane. This may be useful
>>> when a single buffer contains the data for different planes e.g. Y
>>> planes of two fields in the same buffer. With this, user space can
>>> pass queue top field and bottom field with same dmafd and different
>> data_offsets.
>>>
>>> Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
>>> ---
>>> Changes from v2:
>>>  * Use data_offset only for OUTPUT stream buffers
>>>
>>>  drivers/media/platform/ti-vpe/vpe.c |   10 +++++++++-
>>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/media/platform/ti-vpe/vpe.c
>>> b/drivers/media/platform/ti-vpe/vpe.c
>>> index 9a081c2..ba26b83 100644
>>> --- a/drivers/media/platform/ti-vpe/vpe.c
>>> +++ b/drivers/media/platform/ti-vpe/vpe.c
>>> @@ -496,6 +496,14 @@ struct vpe_mmr_adb {
>>>
>>>  #define VPE_SET_MMR_ADB_HDR(ctx, hdr, regs, offset_a)	\
>>>  	VPDMA_SET_MMR_ADB_HDR(ctx->mmr_adb, vpe_mmr_adb, hdr, regs,
>>> offset_a)
>>> +
>>> +static inline dma_addr_t vb2_dma_addr_plus_data_offset(struct
>> vb2_buffer *vb,
>>> +	unsigned int plane_no)
>>> +{
>>> +	return vb2_dma_contig_plane_dma_addr(vb, plane_no) +
>>> +		vb->v4l2_planes[plane_no].data_offset;
>>> +}
>>> +
>>>  /*
>>>   * Set the headers for all of the address/data block structures.
>>>   */
>>> @@ -1043,7 +1051,7 @@ static void add_in_dtd(struct vpe_ctx *ctx, int
>>> port)
>>>
>>>  		vpdma_fmt = fmt->vpdma_fmt[plane];
>>>
>>> -		dma_addr = vb2_dma_contig_plane_dma_addr(vb, plane);
>>> +		dma_addr = vb2_dma_addr_plus_data_offset(vb, plane);
>>>  		if (!dma_addr) {
>>>  			vpe_err(ctx->dev,
>>>  				"acquiring input buffer(%d) dma_addr failed\n",
>>>
>>
>> Should there be a check somewhere that verifies that
>> vb2_get_plane_payload() -
>> vb->v4l2_planes[plane_no].data_offset is still large enough for the
>> vb->image you
>> want to copy?
> 
> Yes, it is done as part of the vb2_qbuf -> __buf_prepare -> __verify_length function
> If the data_offset is high enough that it goes out of the length of that plane,
> The qbuf ioctl should have failed, So we can safely assume the validity of data_offset here

data_offset, yes. But the plane payload as well? Remember that the actual payload
if data_offset > 0 is bytesused - data_offset.

We probably need helper functions for that in videobuf2-core.h. They aren't there
yet because data_offset is used so rarely.

Regards,

	Hans

> 
>>
>> Regards,
>>
>> 	Hans
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

RE: [PATCH v3 1/4] media: ti-vpe: Use data offset for getting dma_addr for a plane

[Devshatwar, Nikhil]

> -----Original Message-----
> From: Hans Verkuil [mailto:hverkuil@xs4all.nl]
> Sent: Monday, December 01, 2014 4:34 PM
> To: Devshatwar, Nikhil; linux-media@vger.kernel.org
> Subject: Re: [PATCH v3 1/4] media: ti-vpe: Use data offset for getting
> dma_addr for a plane
> 
> On 12/01/2014 12:00 PM, Devshatwar, Nikhil wrote:
> >> -----Original Message-----
> >> From: Hans Verkuil [mailto:hverkuil@xs4all.nl]
> >> Sent: Monday, December 01, 2014 4:18 PM
> >> To: Devshatwar, Nikhil; linux-media@vger.kernel.org
> >> Subject: Re: [PATCH v3 1/4] media: ti-vpe: Use data offset for
> >> getting dma_addr for a plane
> >>
> >> On 11/29/2014 11:27 AM, Nikhil Devshatwar wrote:
> >>> The data_offset in v4l2_planes structure will help us point to the
> >>> start of data content for that particular plane. This may be useful
> >>> when a single buffer contains the data for different planes e.g. Y
> >>> planes of two fields in the same buffer. With this, user space can
> >>> pass queue top field and bottom field with same dmafd and different
> >> data_offsets.
> >>>
> >>> Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
> >>> ---
> >>> Changes from v2:
> >>>  * Use data_offset only for OUTPUT stream buffers
> >>>
> >>>  drivers/media/platform/ti-vpe/vpe.c |   10 +++++++++-
> >>>  1 file changed, 9 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/media/platform/ti-vpe/vpe.c
> >>> b/drivers/media/platform/ti-vpe/vpe.c
> >>> index 9a081c2..ba26b83 100644
> >>> --- a/drivers/media/platform/ti-vpe/vpe.c
> >>> +++ b/drivers/media/platform/ti-vpe/vpe.c
> >>> @@ -496,6 +496,14 @@ struct vpe_mmr_adb {
> >>>
> >>>  #define VPE_SET_MMR_ADB_HDR(ctx, hdr, regs, offset_a)	\
> >>>  	VPDMA_SET_MMR_ADB_HDR(ctx->mmr_adb, vpe_mmr_adb, hdr, regs,
> >>> offset_a)
> >>> +
> >>> +static inline dma_addr_t vb2_dma_addr_plus_data_offset(struct
> >> vb2_buffer *vb,
> >>> +	unsigned int plane_no)
> >>> +{
> >>> +	return vb2_dma_contig_plane_dma_addr(vb, plane_no) +
> >>> +		vb->v4l2_planes[plane_no].data_offset;
> >>> +}
> >>> +
> >>>  /*
> >>>   * Set the headers for all of the address/data block structures.
> >>>   */
> >>> @@ -1043,7 +1051,7 @@ static void add_in_dtd(struct vpe_ctx *ctx,
> >>> int
> >>> port)
> >>>
> >>>  		vpdma_fmt = fmt->vpdma_fmt[plane];
> >>>
> >>> -		dma_addr = vb2_dma_contig_plane_dma_addr(vb, plane);
> >>> +		dma_addr = vb2_dma_addr_plus_data_offset(vb, plane);
> >>>  		if (!dma_addr) {
> >>>  			vpe_err(ctx->dev,
> >>>  				"acquiring input buffer(%d) dma_addr failed\n",
> >>>
> >>
> >> Should there be a check somewhere that verifies that
> >> vb2_get_plane_payload() -
> >> vb->v4l2_planes[plane_no].data_offset is still large enough for the
> >> vb->image you
> >> want to copy?
> >
> > Yes, it is done as part of the vb2_qbuf -> __buf_prepare ->
> > __verify_length function If the data_offset is high enough that it
> > goes out of the length of that plane, The qbuf ioctl should have
> > failed, So we can safely assume the validity of data_offset here
> 
> data_offset, yes. But the plane payload as well? Remember that the
> actual payload if data_offset > 0 is bytesused - data_offset.
> 
> We probably need helper functions for that in videobuf2-core.h. They
> aren't there yet because data_offset is used so rarely.
> 

Ohh OK, Now I got what you mean
For this, I think http://lxr.free-electrons.com/source/drivers/media/platform/ti-vpe/vpe.c#L1872
Should be the correct place.
I need to have vb2_plane_offset function
So that we can check if size < plane_offset + the image size (which is qdata_sizeimage)
Also, when calling set_plae_payload, it should be plane_offset + image size
Am I correct?

> Regards,
> 
> 	Hans
> 
> >
> >>
> >> Regards,
> >>
> >> 	Hans
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-
> media"
> > in the body of a message to majordomo@vger.kernel.org More majordomo
> > info at  http://vger.kernel.org/majordomo-info.html
> >

RE: [PATCH v3 4/4] media: ti-vpe: Add support for SEQ_TB buffers

[Devshatwar, Nikhil]

> -----Original Message-----
> From: Hans Verkuil [mailto:hverkuil@xs4all.nl]
> Sent: Monday, December 01, 2014 4:22 PM
> To: Devshatwar, Nikhil; linux-media@vger.kernel.org
> Subject: Re: [PATCH v3 4/4] media: ti-vpe: Add support for SEQ_TB
> buffers
> 
> On 11/29/2014 11:27 AM, Nikhil Devshatwar wrote:
> > The video source can generate the data in the SEQ_TB buffer format.
> > In the case of TI SoC, the IVA_HD can generate the interlaced content
> > in the SEQ_TB buffer format. This is the format where the top and
> > bottom field data can be contained in a single buffer. For example,
> > for NV12, interlaced format, the data in Y buffer will be arranged as
> Y-top followed by Y-bottom.
> > And likewise for UV plane.
> >
> > Also, queueing one buffer of SEQ_TB is euivalent to queueing two
> > different
> 
> queueing -> queuing
> euivalent -> equivalent
> 
> > buffers for top and bottom fields. Driver needs to take care of this
> > when handling source buffer lists.
> >
> > Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
> 
> Currently you are supporting SEQ_TB. Should SEQ_BT also be supported? I
> don't know if you are aware but NTSC transmits the bottom field before
> the top field, while PAL/SECAM transmit top before bottom.
> 
> So for NTSC support having SEQ_BT might be useful. Up to you, though.
> 
OK no problem
I think that should be easy to add
I will add this support and repost the patchset
> Regards,
> 
> 	Hans
> 
> > ---
> > Changes from v1:
> >  * Add check for valid field in qbuf ioctl
> >  * Fix issue with swapped fields
> >
> >  drivers/media/platform/ti-vpe/vpe.c |  124
> > ++++++++++++++++++++++++++++-------
> >  1 file changed, 102 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/media/platform/ti-vpe/vpe.c
> > b/drivers/media/platform/ti-vpe/vpe.c
> > index 6a96bbf..b53aea5 100644
> > --- a/drivers/media/platform/ti-vpe/vpe.c
> > +++ b/drivers/media/platform/ti-vpe/vpe.c
> > @@ -319,9 +319,13 @@ struct vpe_q_data {  };
> >
> >  /* vpe_q_data flag bits */
> > -#define	Q_DATA_FRAME_1D		(1 << 0)
> > -#define	Q_DATA_MODE_TILED	(1 << 1)
> > -#define	Q_DATA_INTERLACED	(1 << 2)
> > +#define	Q_DATA_FRAME_1D			(1 << 0)
> > +#define	Q_DATA_MODE_TILED		(1 << 1)
> > +#define	Q_DATA_INTERLACED_ALTERNATE	(1 << 2)
> > +#define	Q_DATA_INTERLACED_SEQ_TB	(1 << 3)
> > +
> > +#define Q_IS_INTERLACED		(Q_DATA_INTERLACED_ALTERNATE | \
> > +				Q_DATA_INTERLACED_SEQ_TB)
> >
> >  enum {
> >  	Q_DATA_SRC = 0,
> > @@ -647,7 +651,7 @@ static void set_us_coefficients(struct vpe_ctx
> > *ctx)
> >
> >  	cp = &us_coeffs[0].anchor_fid0_c0;
> >
> > -	if (s_q_data->flags & Q_DATA_INTERLACED)	/* interlaced */
> > +	if (s_q_data->flags & Q_IS_INTERLACED)		/* interlaced */
> >  		cp += sizeof(us_coeffs[0]) / sizeof(*cp);
> >
> >  	end_cp = cp + sizeof(us_coeffs[0]) / sizeof(*cp); @@ -774,8
> +778,7
> > @@ static void set_dei_regs(struct vpe_ctx *ctx)
> >  	 * for both progressive and interlace content in interlace bypass
> mode.
> >  	 * It has been recommended not to use progressive bypass mode.
> >  	 */
> > -	if ((!ctx->deinterlacing && (s_q_data->flags &
> Q_DATA_INTERLACED)) ||
> > -			!(s_q_data->flags & Q_DATA_INTERLACED)) {
> > +	if (!(s_q_data->flags & Q_IS_INTERLACED) || !ctx->deinterlacing)
> {
> >  		deinterlace = false;
> >  		val = VPE_DEI_INTERLACE_BYPASS;
> >  	}
> > @@ -843,8 +846,8 @@ static int set_srcdst_params(struct vpe_ctx *ctx)
> >  	ctx->sequence = 0;
> >  	ctx->field = V4L2_FIELD_TOP;
> >
> > -	if ((s_q_data->flags & Q_DATA_INTERLACED) &&
> > -			!(d_q_data->flags & Q_DATA_INTERLACED)) {
> > +	if ((s_q_data->flags & Q_IS_INTERLACED) &&
> > +			!(d_q_data->flags & Q_IS_INTERLACED)) {
> >  		int bytes_per_line;
> >  		const struct vpdma_data_format *mv =
> >  			&vpdma_misc_fmts[VPDMA_DATA_FMT_MV];
> > @@ -1068,6 +1071,27 @@ static void add_in_dtd(struct vpe_ctx *ctx,
> int port)
> >  		vpdma_fmt = fmt->vpdma_fmt[plane];
> >
> >  		dma_addr = vb2_dma_addr_plus_data_offset(vb, plane);
> > +
> > +		if (q_data->flags & Q_DATA_INTERLACED_SEQ_TB) {
> > +			/*
> > +			 * Use top or bottom field from same vb alternately
> > +			 * f,f-1,f-2 = TBT when seq is even
> > +			 * f,f-1,f-2 = BTB when seq is odd
> > +			 */
> > +			field = (p_data->vb_index + (ctx->sequence % 2)) % 2;
> > +
> > +			if (field) {
> > +				/* bottom field of a SEQ_TB buffer
> > +				 * Skip the top field data by */
> > +				int height = q_data->height / 2;
> > +				int bpp = fmt->fourcc == V4L2_PIX_FMT_NV12 ?
> > +						1 : (vpdma_fmt->depth >> 3);
> > +				if (plane)
> > +					height /= 2;
> > +				dma_addr += q_data->width * height * bpp;
> > +			}
> > +		}
> > +
> >  		if (!dma_addr) {
> >  			vpe_err(ctx->dev,
> >  				"acquiring input buffer(%d) dma_addr failed\n",
> @@ -1122,9
> > +1146,22 @@ static void device_run(void *priv)
> >  	struct vpe_ctx *ctx = priv;
> >  	struct sc_data *sc = ctx->dev->sc;
> >  	struct vpe_q_data *d_q_data = &ctx->q_data[Q_DATA_DST];
> > +	struct vpe_q_data *s_q_data = &ctx->q_data[Q_DATA_SRC];
> > +
> > +	if (ctx->deinterlacing && s_q_data->flags &
> Q_DATA_INTERLACED_SEQ_TB &&
> > +		ctx->sequence % 2 == 0) {
> > +		/* When using SEQ_TB buffers, When using it first time,
> > +		 * No need to remove the buffer as the next field is
> present
> > +		 * in the same buffer. (so that job_ready won't fail)
> > +		 * It will be removed when using bottom field
> > +		 */
> > +		ctx->src_vbs[0] = v4l2_m2m_next_src_buf(ctx->m2m_ctx);
> > +		WARN_ON(ctx->src_vbs[0] == NULL);
> > +	} else {
> > +		ctx->src_vbs[0] = v4l2_m2m_src_buf_remove(ctx->m2m_ctx);
> > +		WARN_ON(ctx->src_vbs[0] == NULL);
> > +	}
> >
> > -	ctx->src_vbs[0] = v4l2_m2m_src_buf_remove(ctx->m2m_ctx);
> > -	WARN_ON(ctx->src_vbs[0] == NULL);
> >  	ctx->dst_vb = v4l2_m2m_dst_buf_remove(ctx->m2m_ctx);
> >  	WARN_ON(ctx->dst_vb == NULL);
> >
> > @@ -1256,6 +1293,7 @@ static irqreturn_t vpe_irq(int irq_vpe, void
> *data)
> >  	struct vpe_dev *dev = (struct vpe_dev *)data;
> >  	struct vpe_ctx *ctx;
> >  	struct vpe_q_data *d_q_data;
> > +	struct vpe_q_data *s_q_data;
> >  	struct vb2_buffer *s_vb, *d_vb;
> >  	struct v4l2_buffer *s_buf, *d_buf;
> >  	unsigned long flags;
> > @@ -1332,7 +1370,7 @@ static irqreturn_t vpe_irq(int irq_vpe, void
> *data)
> >  	d_buf->sequence = ctx->sequence;
> >
> >  	d_q_data = &ctx->q_data[Q_DATA_DST];
> > -	if (d_q_data->flags & Q_DATA_INTERLACED) {
> > +	if (d_q_data->flags & Q_IS_INTERLACED) {
> >  		d_buf->field = ctx->field;
> >  		if (ctx->field == V4L2_FIELD_BOTTOM) {
> >  			ctx->sequence++;
> > @@ -1346,12 +1384,28 @@ static irqreturn_t vpe_irq(int irq_vpe, void
> *data)
> >  		ctx->sequence++;
> >  	}
> >
> > -	if (ctx->deinterlacing)
> > -		s_vb = ctx->src_vbs[2];
> > +	s_q_data = &ctx->q_data[Q_DATA_SRC];
> > +
> > +	if (ctx->deinterlacing) {
> > +		/* Allow source buffer to be dequeued only if it won't be
> used
> > +		 * in the next iteration. All vbs are initialized to first
> > +		 * buffer and we are shifting buffers every iteration, for
> the
> > +		 * first two iterations, no buffer will be dequeued.
> > +		 * This ensures that driver will keep (n-2)th (n-1)th and
> (n)th
> > +		 * field when deinterlacing is enabled */
> > +		if (ctx->src_vbs[2] != ctx->src_vbs[1])
> > +			s_vb = ctx->src_vbs[2];
> > +		else
> > +			s_vb = NULL;
> > +	}
> >
> >  	spin_lock_irqsave(&dev->lock, flags);
> > -	v4l2_m2m_buf_done(s_vb, VB2_BUF_STATE_DONE);
> > +
> > +	if (s_vb)
> > +		v4l2_m2m_buf_done(s_vb, VB2_BUF_STATE_DONE);
> > +
> >  	v4l2_m2m_buf_done(d_vb, VB2_BUF_STATE_DONE);
> > +
> >  	spin_unlock_irqrestore(&dev->lock, flags);
> >
> >  	if (ctx->deinterlacing) {
> > @@ -1467,7 +1521,7 @@ static int __vpe_try_fmt(struct vpe_ctx *ctx,
> struct v4l2_format *f,
> >  	struct v4l2_pix_format_mplane *pix = &f->fmt.pix_mp;
> >  	struct v4l2_plane_pix_format *plane_fmt;
> >  	unsigned int w_align;
> > -	int i, depth, depth_bytes;
> > +	int i, depth, depth_bytes, height;
> >
> >  	if (!fmt || !(fmt->types & type)) {
> >  		vpe_err(ctx->dev, "Fourcc format (0x%08x) invalid.\n", @@ -
> 1475,7
> > +1529,8 @@ static int __vpe_try_fmt(struct vpe_ctx *ctx, struct
> v4l2_format *f,
> >  		return -EINVAL;
> >  	}
> >
> > -	if (pix->field != V4L2_FIELD_NONE && pix->field !=
> V4L2_FIELD_ALTERNATE)
> > +	if (pix->field != V4L2_FIELD_NONE && pix->field !=
> V4L2_FIELD_ALTERNATE
> > +			&& pix->field != V4L2_FIELD_SEQ_TB)
> >  		pix->field = V4L2_FIELD_NONE;
> >
> >  	depth = fmt->vpdma_fmt[VPE_LUMA]->depth; @@ -1509,6 +1564,14 @@
> > static int __vpe_try_fmt(struct vpe_ctx *ctx, struct v4l2_format *f,
> >  	pix->num_planes = fmt->coplanar ? 2 : 1;
> >  	pix->pixelformat = fmt->fourcc;
> >
> > +	/* for the actual image parameters, we need to consider the field
> height
> > +	 * of the image for SEQ_TB buffers.
> > +	 */
> > +	if (pix->field == V4L2_FIELD_SEQ_TB)
> > +		height = pix->height / 2;
> > +	else
> > +		height = pix->height;
> > +
> >  	if (!pix->colorspace) {
> >  		if (fmt->fourcc == V4L2_PIX_FMT_RGB24 ||
> >  				fmt->fourcc == V4L2_PIX_FMT_BGR24 || @@ -1516,7
> +1579,7 @@ static
> > int __vpe_try_fmt(struct vpe_ctx *ctx, struct v4l2_format *f,
> >  				fmt->fourcc == V4L2_PIX_FMT_BGR32) {
> >  			pix->colorspace = V4L2_COLORSPACE_SRGB;
> >  		} else {
> > -			if (pix->height > 1280)	/* HD */
> > +			if (height > 1280)	/* HD */
> >  				pix->colorspace = V4L2_COLORSPACE_REC709;
> >  			else			/* SD */
> >  				pix->colorspace = V4L2_COLORSPACE_SMPTE170M; @@
> -1593,9 +1656,15
> > @@ static int __vpe_s_fmt(struct vpe_ctx *ctx, struct v4l2_format *f)
> >  	q_data->c_rect.height	= q_data->height;
> >
> >  	if (q_data->field == V4L2_FIELD_ALTERNATE)
> > -		q_data->flags |= Q_DATA_INTERLACED;
> > +		q_data->flags |= Q_DATA_INTERLACED_ALTERNATE;
> > +	else if (q_data->field == V4L2_FIELD_SEQ_TB)
> > +		q_data->flags |= Q_DATA_INTERLACED_SEQ_TB;
> >  	else
> > -		q_data->flags &= ~Q_DATA_INTERLACED;
> > +		q_data->flags &= ~Q_IS_INTERLACED;
> > +
> > +	/* the crop height is halved for the case of SEQ_TB buffers */
> > +	if (q_data->flags & Q_DATA_INTERLACED_SEQ_TB)
> > +		q_data->c_rect.height /= 2;
> >
> >  	vpe_dbg(ctx->dev, "Setting format for type %d, wxh: %dx%d, fmt:
> %d bpl_y %d",
> >  		f->type, q_data->width, q_data->height, q_data->fmt-
> >fourcc, @@
> > -1631,6 +1700,7 @@ static int vpe_s_fmt(struct file *file, void
> *priv,
> > struct v4l2_format *f)  static int __vpe_try_selection(struct vpe_ctx
> > *ctx, struct v4l2_selection *s)  {
> >  	struct vpe_q_data *q_data;
> > +	int height;
> >
> >  	if ((s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
> >  	    (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT)) @@ -1665,13 +1735,22
> @@
> > static int __vpe_try_selection(struct vpe_ctx *ctx, struct
> v4l2_selection *s)
> >  		return -EINVAL;
> >  	}
> >
> > +	/*
> > +	* For SEQ_TB buffers, crop height should be less than the height
> of
> > +	* the field height, not the buffer height
> > +	*/
> > +	if (q_data->flags & Q_DATA_INTERLACED_SEQ_TB)
> > +		height = q_data->height / 2;
> > +	else
> > +		height = q_data->height;
> > +
> >  	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);
> > +		&s->r.height, MIN_H, 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) @@ -1919,11 +1998,12
> @@
> > static int vpe_buf_prepare(struct vb2_buffer *vb)
> >  	num_planes = q_data->fmt->coplanar ? 2 : 1;
> >
> >  	if (vb->vb2_queue->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
> > -		if (!(q_data->flags & Q_DATA_INTERLACED)) {
> > +		if (!(q_data->flags & Q_IS_INTERLACED)) {
> >  			vb->v4l2_buf.field = V4L2_FIELD_NONE;
> >  		} else {
> >  			if (vb->v4l2_buf.field != V4L2_FIELD_TOP &&
> > -					vb->v4l2_buf.field != V4L2_FIELD_BOTTOM)
> > +			vb->v4l2_buf.field != V4L2_FIELD_BOTTOM &&
> > +			vb->v4l2_buf.field != V4L2_FIELD_SEQ_TB)
> >  				return -EINVAL;
> >  		}
> >  	}
> >