RE: [PATCH v3 4/4] media: ti-vpe: Add support for SEQ_TB buffers
-Original Message-
From: Hans Verkuil [mailto:hverk...@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...@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 */
-#defineQ_DATA_FRAME_1D (1 0)
-#defineQ_DATA_MODE_TILED (1 1)
-#defineQ_DATA_INTERLACED (1 2)
+#defineQ_DATA_FRAME_1D (1 0)
+#defineQ_DATA_MODE_TILED (1 1)
+#defineQ_DATA_INTERLACED_ALTERNATE (1 2)
+#defineQ_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
RE: [PATCH v3 1/4] media: ti-vpe: Use data offset for getting dma_addr for a plane
-Original Message-
From: Hans Verkuil [mailto:hverk...@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...@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
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@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
-Original Message-
From: Hans Verkuil [mailto:hverk...@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:hverk...@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...@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 majord...@vger.kernel.org More majordomo
info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v2 1/4] media: ti-vpe: Use data offset for getting dma_addr for a plane
-Original Message-
From: Hans Verkuil [mailto:hverk...@xs4all.nl]
Sent: Sunday, November 23, 2014 6:23 PM
To: Devshatwar, Nikhil; linux-media@vger.kernel.org
Subject: Re: [PATCH v2 1/4] media: ti-vpe: Use data offset for getting
dma_addr for a plane
On 11/14/2014 12:20 PM, 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...@ti.com
---
drivers/media/platform/ti-vpe/vpe.c | 12 ++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/media/platform/ti-vpe/vpe.c
b/drivers/media/platform/ti-vpe/vpe.c
index 0ae19ee..e59eb81 100644
--- a/drivers/media/platform/ti-vpe/vpe.c
+++ b/drivers/media/platform/ti-vpe/vpe.c
@@ -495,6 +495,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.
*/
@@ -1002,7 +1010,7 @@ static void add_out_dtd(struct vpe_ctx *ctx,
int port)
int plane = fmt-coplanar ? p_data-vb_part : 0;
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 output buffer(%d) dma_addr
failed\n, @@ -1042,7
+1050,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,
I suspect this does not do what you want. data_offset is set by the
application for an output stream (i.e. from memory to the hardware) and
it is set by the driver for a capture stream (i.e. from hardware to
memory). It looks like you expect that it is set by the application for
capture streams as well, but that's not true.
Yes, I agree
I should use the new function (with data_offset) only in the add_in_dtd
add_in_dtd is called for the OUTPUT streams only
(And some internal buffers)
Regards,
Hans
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [[PATCH]] vb2: verify data_offset only if nonzero bytesused
On Monday 23 June 2014 01:25 PM, Hans Verkuil wrote:
On 06/22/2014 12:47 PM, Nikhil Devshatwar wrote:
verify_planes would fail if the user space fills up the data_offset field
and bytesused is left as zero. Correct this.
Checking for data_offset bytesused is not correct as it might fail some of
the valid use cases. e.g. when working with SEQ_TB buffers, for bottom field,
data_offset can be high but it can have less bytesused.
The real check should be to verify that all the bytesused after data_offset
fit withing the length of the plane.
Signed-off-by: Nikhil Devshatwar nikhil...@ti.com
---
drivers/media/v4l2-core/videobuf2-core.c |9 +++--
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/media/v4l2-core/videobuf2-core.c
b/drivers/media/v4l2-core/videobuf2-core.c
index 7c4489c..9a0ccb6 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -587,12 +587,9 @@ static int __verify_length(struct vb2_buffer *vb, const
struct v4l2_buffer *b)
? b-m.planes[plane].length
: vb-v4l2_planes[plane].length;
- if (b-m.planes[plane].bytesused length)
- return -EINVAL;
-
- if (b-m.planes[plane].data_offset 0
- b-m.planes[plane].data_offset =
- b-m.planes[plane].bytesused)
+ if (b-m.planes[plane].bytesused 0
+ b-m.planes[plane].data_offset +
+ b-m.planes[plane].bytesused length)
Nacked-by: Hans Verkuil hans.verk...@cisco.com
bytesused *includes* data_offset. So the effective payload is
'bytesused - data_offset' starting at offset 'data_offset' from the
start of the buffer.
Ohh! I misinterpreted bytesused field
I Will correct the condition
So your new condition is wrong.
Regards,
Hans
return -EINVAL;
}
} else {
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
