[Patch 00/16] media: vpe: maintenance

[Patch 00/16] media: vpe: maintenance

[Benoit Parrot]

This a collection of backlog patches I have been carrying for the VPE
driver.

It adds supports for SEQ_BT as well as NV21.
And fixes a number of issues both through v4l2-compliance and normal
usage.

======================================
v4l2-compliance SHA: 5b168dc8473911227890526bad26553d9e8ff81b, 32 bits

Compliance test for vpe device /dev/video0:

Driver Info:
	Driver name      : vpe
	Card type        : vpe
	Bus info         : platform:vpe
	Driver version   : 5.3.0
	Capabilities     : 0x84204000
		Video Memory-to-Memory Multiplanar
		Streaming
		Extended Pix Format
		Device Capabilities
	Device Caps      : 0x04204000
		Video Memory-to-Memory Multiplanar
		Streaming
		Extended Pix Format

Required ioctls:
	test VIDIOC_QUERYCAP: OK

Allow for multiple opens:
	test second /dev/video0 open: OK
	test VIDIOC_QUERYCAP: OK
	test VIDIOC_G/S_PRIORITY: OK
	test for unlimited opens: 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/ENUM_FREQ_BANDS: 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

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)
	test VIDIOC_G/S_EDID: OK (Not Supported)

Control ioctls:
	test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK
	test VIDIOC_QUERYCTRL: OK
	test VIDIOC_G/S_CTRL: OK
	test VIDIOC_G/S/TRY_EXT_CTRLS: OK
	test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK
	test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
	Standard Controls: 1 Private Controls: 1

Format ioctls:
	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
	test VIDIOC_TRY_FMT: OK
	test VIDIOC_S_FMT: OK
	test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
	test Cropping: OK
	test Composing: OK
	test Scaling: OK

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:
		warn: v4l2-test-buffers.cpp(683): VIDIOC_CREATE_BUFS not supported
		warn: v4l2-test-buffers.cpp(683): VIDIOC_CREATE_BUFS not supported
	test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
	test VIDIOC_EXPBUF: OK
	test Requests: OK (Not Supported)

Test input 0:

Streaming ioctls:
	test read/write: OK (Not Supported)
	test blocking wait: OK
	Video Capture Multiplanar: Captured 58 buffers    
	test MMAP (no poll): OK
	Video Capture Multiplanar: Captured 58 buffers    
	test MMAP (select): OK
	Video Capture Multiplanar: Captured 58 buffers    
	test MMAP (epoll): OK
	test USERPTR (no poll): OK (Not Supported)
	test USERPTR (select): OK (Not Supported)
	test DMABUF: Cannot test, specify --expbuf-device

Total for vpe device /dev/video0: 51, Succeeded: 51, Failed: 0, Warnings: 2
======================================

Benoit Parrot (13):
  media: ti-vpe: vpe: Fix Motion Vector vpdma stride
  media: ti-vpe: vpe: Add missing null pointer checks
  media: ti-vpe: vpe: Remove unnecessary use of container_of
  media: ti-vpe: vpe: fix a v4l2-compliance failure causing a kernel
    panic
  media: ti-vpe: vpe: fix a v4l2-compliance warning about invalid pixel
    format
  media: ti-vpe: vpe: Make sure YUYV is set as default format
  media: ti-vpe: vpe: fix a v4l2-compliance failure about invalid
    sizeimage
  media: ti-vpe: vpe: fix a v4l2-compliance failure about frame sequence
    number
  media: ti-vpe: vpe: ensure buffers are cleaned up properly in abort
    cases
  media: ti-vpe: vpdma: Use fixed type for address in descriptor
  media: ti-vpe: Set the DMA mask and coherent mask
  media: ti-vpe: vpe: fix v4l2_compliance issue related to xfer_func
  media: ti-vpe: vpe: don't rely on colorspace member for conversion

Nikhil Devshatwar (2):
  media: ti-vpe: Add support for SEQ_BT
  media: ti-vpe: Add support for NV21 format

Ram Prasad (1):
  media: ti-vpe: Set MAX height supported to 2048 pixels

 drivers/media/platform/ti-vpe/vpdma.c      |  11 +-
 drivers/media/platform/ti-vpe/vpdma.h      |   2 +
 drivers/media/platform/ti-vpe/vpdma_priv.h |   5 +-
 drivers/media/platform/ti-vpe/vpe.c        | 251 +++++++++++++++------
 4 files changed, 192 insertions(+), 77 deletions(-)

-- 
2.17.1

[Patch 01/16] media: ti-vpe: vpe: Fix Motion Vector vpdma stride

[Benoit Parrot]

commit 52831a418fa6 ("[media] media: ti-vpe: vpe: allow use of user
specified stride") and commit 8c1e4fa17e92 ("[media] media: ti-vpe: vpdma:
add support for user specified stride") resulted in the Motion Vector
stride to be the same as the image stride.

This caused memory corruption in the output image as mentionned in
commit 44f98adf71a8 ("[media] media: ti-vpe: vpe: Fix line stride
for output motion vector").

Fixes: 52831a418fa6 ("[media] media: ti-vpe: vpe: allow use of user specified stride")
Fixes: 8c1e4fa17e92 ("[media] media: ti-vpe: vpdma: add support for user specified stride")
Signed-off-by: Benoit Parrot <bparrot@ti.com>
Acked-by: Nikhil Devshatwar <nikhil.nd@ti.com>
---
 drivers/media/platform/ti-vpe/vpe.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
index 60b575bb44c4..5ba72445584d 100644
--- a/drivers/media/platform/ti-vpe/vpe.c
+++ b/drivers/media/platform/ti-vpe/vpe.c
@@ -1013,11 +1013,14 @@ static void add_out_dtd(struct vpe_ctx *ctx, int port)
 	dma_addr_t dma_addr;
 	u32 flags = 0;
 	u32 offset = 0;
+	u32 stride;
 
 	if (port == VPE_PORT_MV_OUT) {
 		vpdma_fmt = &vpdma_misc_fmts[VPDMA_DATA_FMT_MV];
 		dma_addr = ctx->mv_buf_dma[mv_buf_selector];
 		q_data = &ctx->q_data[Q_DATA_SRC];
+		stride = ALIGN((q_data->width * vpdma_fmt->depth) >> 3,
+			       VPDMA_STRIDE_ALIGN);
 	} else {
 		/* to incorporate interleaved formats */
 		int plane = fmt->coplanar ? p_data->vb_part : 0;
@@ -1044,6 +1047,7 @@ static void add_out_dtd(struct vpe_ctx *ctx, int port)
 		}
 		/* Apply the offset */
 		dma_addr += offset;
+		stride = q_data->bytesperline[VPE_LUMA];
 	}
 
 	if (q_data->flags & Q_DATA_FRAME_1D)
@@ -1055,7 +1059,7 @@ static void add_out_dtd(struct vpe_ctx *ctx, int port)
 			   MAX_W, MAX_H);
 
 	vpdma_add_out_dtd(&ctx->desc_list, q_data->width,
-			  q_data->bytesperline[VPE_LUMA], &q_data->c_rect,
+			  stride, &q_data->c_rect,
 			  vpdma_fmt, dma_addr, MAX_OUT_WIDTH_REG1,
 			  MAX_OUT_HEIGHT_REG1, p_data->channel, flags);
 }
@@ -1074,10 +1078,13 @@ static void add_in_dtd(struct vpe_ctx *ctx, int port)
 	dma_addr_t dma_addr;
 	u32 flags = 0;
 	u32 offset = 0;
+	u32 stride;
 
 	if (port == VPE_PORT_MV_IN) {
 		vpdma_fmt = &vpdma_misc_fmts[VPDMA_DATA_FMT_MV];
 		dma_addr = ctx->mv_buf_dma[mv_buf_selector];
+		stride = ALIGN((q_data->width * vpdma_fmt->depth) >> 3,
+			       VPDMA_STRIDE_ALIGN);
 	} else {
 		/* to incorporate interleaved formats */
 		int plane = fmt->coplanar ? p_data->vb_part : 0;
@@ -1104,6 +1111,7 @@ static void add_in_dtd(struct vpe_ctx *ctx, int port)
 		}
 		/* Apply the offset */
 		dma_addr += offset;
+		stride = q_data->bytesperline[VPE_LUMA];
 
 		if (q_data->flags & Q_DATA_INTERLACED_SEQ_TB) {
 			/*
@@ -1139,10 +1147,10 @@ static void add_in_dtd(struct vpe_ctx *ctx, int port)
 	if (p_data->vb_part && fmt->fourcc == V4L2_PIX_FMT_NV12)
 		frame_height /= 2;
 
-	vpdma_add_in_dtd(&ctx->desc_list, q_data->width,
-			 q_data->bytesperline[VPE_LUMA], &q_data->c_rect,
-		vpdma_fmt, dma_addr, p_data->channel, field, flags, frame_width,
-		frame_height, 0, 0);
+	vpdma_add_in_dtd(&ctx->desc_list, q_data->width, stride,
+			 &q_data->c_rect, vpdma_fmt, dma_addr,
+			 p_data->channel, field, flags, frame_width,
+			 frame_height, 0, 0);
 }
 
 /*
-- 
2.17.1

[Patch 02/16] media: ti-vpe: vpe: Add missing null pointer checks

[Benoit Parrot]

A few NULL pointer checks were missing.
Add check with appropriate return code.

Signed-off-by: Benoit Parrot <bparrot@ti.com>
---
 drivers/media/platform/ti-vpe/vpe.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
index 5ba72445584d..56f60dbea15c 100644
--- a/drivers/media/platform/ti-vpe/vpe.c
+++ b/drivers/media/platform/ti-vpe/vpe.c
@@ -1537,6 +1537,8 @@ static int vpe_g_fmt(struct file *file, void *priv, struct v4l2_format *f)
 		return -EINVAL;
 
 	q_data = get_q_data(ctx, f->type);
+	if (!q_data)
+		return -EINVAL;
 
 	pix->width = q_data->width;
 	pix->height = q_data->height;
@@ -2001,6 +2003,8 @@ static int vpe_queue_setup(struct vb2_queue *vq,
 	struct vpe_q_data *q_data;
 
 	q_data = get_q_data(ctx, vq->type);
+	if (!q_data)
+		return -EINVAL;
 
 	*nplanes = q_data->nplanes;
 
@@ -2025,6 +2029,8 @@ static int vpe_buf_prepare(struct vb2_buffer *vb)
 	vpe_dbg(ctx->dev, "type: %d\n", vb->vb2_queue->type);
 
 	q_data = get_q_data(ctx, vb->vb2_queue->type);
+	if (!q_data)
+		return -EINVAL;
 	num_planes = q_data->nplanes;
 
 	if (vb->vb2_queue->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
@@ -2481,7 +2487,12 @@ static int vpe_probe(struct platform_device *pdev)
 	mutex_init(&dev->dev_mutex);
 
 	dev->res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
-			"vpe_top");
+						"vpe_top");
+	if (!dev->res) {
+		dev_err(&pdev->dev, "missing 'vpe_top' resources data\n");
+		return -ENODEV;
+	}
+
 	/*
 	 * HACK: we get resource info from device tree in the form of a list of
 	 * VPE sub blocks, the driver currently uses only the base of vpe_top
-- 
2.17.1

Re: [Patch 02/16] media: ti-vpe: vpe: Add missing null pointer checks

[Austin Kim]

2019년 9월 28일 (토) 오전 3:37, Benoit Parrot <bparrot@ti.com>님이 작성:
>
> A few NULL pointer checks were missing.
> Add check with appropriate return code.
>
> Signed-off-by: Benoit Parrot <bparrot@ti.com>
> ---
>  drivers/media/platform/ti-vpe/vpe.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
> index 5ba72445584d..56f60dbea15c 100644
> --- a/drivers/media/platform/ti-vpe/vpe.c
> +++ b/drivers/media/platform/ti-vpe/vpe.c
> @@ -1537,6 +1537,8 @@ static int vpe_g_fmt(struct file *file, void *priv, struct v4l2_format *f)
>                 return -EINVAL;
>
>         q_data = get_q_data(ctx, f->type);
> +       if (!q_data)
> +               return -EINVAL;

With this commit, it seems that 'Null Pointer Dereference' could be
avoidable even though 'get_q_data(ctx, f->type);' returns NULL.

* Original Code:
        q_data = get_q_data(ctx, f->type);
        // q_data = NULL;

        pix->width = q_data->width;
        // pix->width =  (NULL)->width;
        // In this case, data abort would be raised.

>
>         pix->width = q_data->width;
>         pix->height = q_data->height;
> @@ -2001,6 +2003,8 @@ static int vpe_queue_setup(struct vb2_queue *vq,
>         struct vpe_q_data *q_data;
>
>         q_data = get_q_data(ctx, vq->type);
> +       if (!q_data)
> +               return -EINVAL;
>
>         *nplanes = q_data->nplanes;
>
> @@ -2025,6 +2029,8 @@ static int vpe_buf_prepare(struct vb2_buffer *vb)
>         vpe_dbg(ctx->dev, "type: %d\n", vb->vb2_queue->type);
>
>         q_data = get_q_data(ctx, vb->vb2_queue->type);
> +       if (!q_data)
> +               return -EINVAL;
>         num_planes = q_data->nplanes;
>
>         if (vb->vb2_queue->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
> @@ -2481,7 +2487,12 @@ static int vpe_probe(struct platform_device *pdev)
>         mutex_init(&dev->dev_mutex);
>
>         dev->res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> -                       "vpe_top");
> +                                               "vpe_top");
> +       if (!dev->res) {
> +               dev_err(&pdev->dev, "missing 'vpe_top' resources data\n");
> +               return -ENODEV;
> +       }
> +
>         /*
>          * HACK: we get resource info from device tree in the form of a list of
>          * VPE sub blocks, the driver currently uses only the base of vpe_top
> --
> 2.17.1
>

Re: [Patch 02/16] media: ti-vpe: vpe: Add missing null pointer checks

[Benoit Parrot]

Hi Austin,

Thanks for the review,

Austin Kim <austinkernel.kim@gmail.com> wrote on Sun [2019-Sep-29 09:08:37 +0900]:
> 2019년 9월 28일 (토) 오전 3:37, Benoit Parrot <bparrot@ti.com>님이 작성:
> >
> > A few NULL pointer checks were missing.
> > Add check with appropriate return code.
> >
> > Signed-off-by: Benoit Parrot <bparrot@ti.com>
> > ---
> >  drivers/media/platform/ti-vpe/vpe.c | 13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
> > index 5ba72445584d..56f60dbea15c 100644
> > --- a/drivers/media/platform/ti-vpe/vpe.c
> > +++ b/drivers/media/platform/ti-vpe/vpe.c
> > @@ -1537,6 +1537,8 @@ static int vpe_g_fmt(struct file *file, void *priv, struct v4l2_format *f)
> >                 return -EINVAL;
> >
> >         q_data = get_q_data(ctx, f->type);
> > +       if (!q_data)
> > +               return -EINVAL;
> 
> With this commit, it seems that 'Null Pointer Dereference' could be
> avoidable even though 'get_q_data(ctx, f->type);' returns NULL.
> 
> * Original Code:
>         q_data = get_q_data(ctx, f->type);
>         // q_data = NULL;
> 
>         pix->width = q_data->width;
>         // pix->width =  (NULL)->width;
>         // In this case, data abort would be raised.

Yes I know this that is why the NULL check were added.

You mentionned earlier that the NULL pointer dereference could be
avoidable, but based on your comment I fail to see what you mean.

Please also note that this patch was a result of static analysis software
(klocwork) warnings.

Benoit

> 
> >
> >         pix->width = q_data->width;
> >         pix->height = q_data->height;
> > @@ -2001,6 +2003,8 @@ static int vpe_queue_setup(struct vb2_queue *vq,
> >         struct vpe_q_data *q_data;
> >
> >         q_data = get_q_data(ctx, vq->type);
> > +       if (!q_data)
> > +               return -EINVAL;
> >
> >         *nplanes = q_data->nplanes;
> >
> > @@ -2025,6 +2029,8 @@ static int vpe_buf_prepare(struct vb2_buffer *vb)
> >         vpe_dbg(ctx->dev, "type: %d\n", vb->vb2_queue->type);
> >
> >         q_data = get_q_data(ctx, vb->vb2_queue->type);
> > +       if (!q_data)
> > +               return -EINVAL;
> >         num_planes = q_data->nplanes;
> >
> >         if (vb->vb2_queue->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
> > @@ -2481,7 +2487,12 @@ static int vpe_probe(struct platform_device *pdev)
> >         mutex_init(&dev->dev_mutex);
> >
> >         dev->res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> > -                       "vpe_top");
> > +                                               "vpe_top");
> > +       if (!dev->res) {
> > +               dev_err(&pdev->dev, "missing 'vpe_top' resources data\n");
> > +               return -ENODEV;
> > +       }
> > +
> >         /*
> >          * HACK: we get resource info from device tree in the form of a list of
> >          * VPE sub blocks, the driver currently uses only the base of vpe_top
> > --
> > 2.17.1
> >

Re: [Patch 02/16] media: ti-vpe: vpe: Add missing null pointer checks

[Austin Kim]

2019년 10월 1일 (화) 오전 12:56, Benoit Parrot <bparrot@ti.com>님이 작성:
>
> Hi Austin,
>
> Thanks for the review,

It's my pleasure.
Hope to see this patch will arrive the destination(linux-next) safely.  :)

[Patch 03/16] media: ti-vpe: vpe: Remove unnecessary use of container_of

[Benoit Parrot]

Instead of saving a pointer to the 'fh' member of struct vpe_ctx to
later have to use container_of to retrieve the actual pointer to the
context structure, which seems to confuse static code analysis tool
anyways, just save the pointer to the actual structure and then retrieve
it directly.

Signed-off-by: Benoit Parrot <bparrot@ti.com>
---
 drivers/media/platform/ti-vpe/vpe.c | 22 +++++++---------------
 1 file changed, 7 insertions(+), 15 deletions(-)

diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
index 56f60dbea15c..0e9cb0319a92 100644
--- a/drivers/media/platform/ti-vpe/vpe.c
+++ b/drivers/media/platform/ti-vpe/vpe.c
@@ -900,14 +900,6 @@ static int set_srcdst_params(struct vpe_ctx *ctx)
 	return 0;
 }
 
-/*
- * Return the vpe_ctx structure for a given struct file
- */
-static struct vpe_ctx *file2ctx(struct file *file)
-{
-	return container_of(file->private_data, struct vpe_ctx, fh);
-}
-
 /*
  * mem2mem callbacks
  */
@@ -1527,7 +1519,7 @@ static int vpe_enum_fmt(struct file *file, void *priv,
 static int vpe_g_fmt(struct file *file, void *priv, struct v4l2_format *f)
 {
 	struct v4l2_pix_format_mplane *pix = &f->fmt.pix_mp;
-	struct vpe_ctx *ctx = file2ctx(file);
+	struct vpe_ctx *ctx = file->private_data;
 	struct vb2_queue *vq;
 	struct vpe_q_data *q_data;
 	int i;
@@ -1689,7 +1681,7 @@ static int __vpe_try_fmt(struct vpe_ctx *ctx, struct v4l2_format *f,
 
 static int vpe_try_fmt(struct file *file, void *priv, struct v4l2_format *f)
 {
-	struct vpe_ctx *ctx = file2ctx(file);
+	struct vpe_ctx *ctx = file->private_data;
 	struct vpe_fmt *fmt = find_format(f);
 
 	if (V4L2_TYPE_IS_OUTPUT(f->type))
@@ -1762,7 +1754,7 @@ static int __vpe_s_fmt(struct vpe_ctx *ctx, struct v4l2_format *f)
 static int vpe_s_fmt(struct file *file, void *priv, struct v4l2_format *f)
 {
 	int ret;
-	struct vpe_ctx *ctx = file2ctx(file);
+	struct vpe_ctx *ctx = file->private_data;
 
 	ret = vpe_try_fmt(file, priv, f);
 	if (ret)
@@ -1847,7 +1839,7 @@ static int __vpe_try_selection(struct vpe_ctx *ctx, struct v4l2_selection *s)
 static int vpe_g_selection(struct file *file, void *fh,
 		struct v4l2_selection *s)
 {
-	struct vpe_ctx *ctx = file2ctx(file);
+	struct vpe_ctx *ctx = file->private_data;
 	struct vpe_q_data *q_data;
 	bool use_c_rect = false;
 
@@ -1908,7 +1900,7 @@ static int vpe_g_selection(struct file *file, void *fh,
 static int vpe_s_selection(struct file *file, void *fh,
 		struct v4l2_selection *s)
 {
-	struct vpe_ctx *ctx = file2ctx(file);
+	struct vpe_ctx *ctx = file->private_data;
 	struct vpe_q_data *q_data;
 	struct v4l2_selection sel = *s;
 	int ret;
@@ -2275,7 +2267,7 @@ static int vpe_open(struct file *file)
 	init_adb_hdrs(ctx);
 
 	v4l2_fh_init(&ctx->fh, video_devdata(file));
-	file->private_data = &ctx->fh;
+	file->private_data = ctx;
 
 	hdl = &ctx->hdl;
 	v4l2_ctrl_handler_init(hdl, 1);
@@ -2360,7 +2352,7 @@ static int vpe_open(struct file *file)
 static int vpe_release(struct file *file)
 {
 	struct vpe_dev *dev = video_drvdata(file);
-	struct vpe_ctx *ctx = file2ctx(file);
+	struct vpe_ctx *ctx = file->private_data;
 
 	vpe_dbg(dev, "releasing instance %p\n", ctx);
 
-- 
2.17.1

[Patch 04/16] media: ti-vpe: Add support for SEQ_BT

[Benoit Parrot]

From: Nikhil Devshatwar <nikhil.nd@ti.com>

SEQ_BT indicates the buffer for bottom field needs to be processed
before the top field.

Simplify the field selection logic to support SEQ_BT as well.

Modify the interlace flags to include any of alternate, SEQ_TB, SEQ_BT.
Update other format error checking to consider SEQ_BT.
Replace SEQ_TB with SEQ_XX wherever applicable.

Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
Signed-off-by: Benoit Parrot <bparrot@ti.com>
---
 drivers/media/platform/ti-vpe/vpe.c | 73 ++++++++++++++++++-----------
 1 file changed, 46 insertions(+), 27 deletions(-)

diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
index 0e9cb0319a92..5d0ec5f7ca25 100644
--- a/drivers/media/platform/ti-vpe/vpe.c
+++ b/drivers/media/platform/ti-vpe/vpe.c
@@ -328,9 +328,14 @@ struct vpe_q_data {
 #define	Q_DATA_MODE_TILED		BIT(1)
 #define	Q_DATA_INTERLACED_ALTERNATE	BIT(2)
 #define	Q_DATA_INTERLACED_SEQ_TB	BIT(3)
+#define	Q_DATA_INTERLACED_SEQ_BT	BIT(4)
+
+#define Q_IS_SEQ_XX		(Q_DATA_INTERLACED_SEQ_TB | \
+				Q_DATA_INTERLACED_SEQ_BT)
 
 #define Q_IS_INTERLACED		(Q_DATA_INTERLACED_ALTERNATE | \
-				Q_DATA_INTERLACED_SEQ_TB)
+				Q_DATA_INTERLACED_SEQ_TB | \
+				Q_DATA_INTERLACED_SEQ_BT)
 
 enum {
 	Q_DATA_SRC = 0,
@@ -1105,24 +1110,31 @@ static void add_in_dtd(struct vpe_ctx *ctx, int port)
 		dma_addr += offset;
 		stride = q_data->bytesperline[VPE_LUMA];
 
-		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;
+		/*
+		 * field used in VPDMA desc  = 0 (top) / 1 (bottom)
+		 * Use top or bottom field from same vb alternately
+		 * For each de-interlacing operation, f,f-1,f-2 should be one
+		 * of TBT or BTB
+		 */
+		if (q_data->flags & Q_DATA_INTERLACED_SEQ_TB ||
+		    q_data->flags & Q_DATA_INTERLACED_SEQ_BT) {
+			/* Select initial value based on format */
+			if (q_data->flags & Q_DATA_INTERLACED_SEQ_BT)
+				field = 1;
+			else
+				field = 0;
+
+			/* Toggle for each vb_index and each operation */
+			field = (field + p_data->vb_index + ctx->sequence) % 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;
 			}
 		}
@@ -1177,12 +1189,14 @@ static void device_run(void *priv)
 	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
+	if (ctx->deinterlacing && s_q_data->flags & Q_IS_SEQ_XX &&
+	    ctx->sequence % 2 == 0) {
+		/* When using SEQ_XX type buffers, each buffer has two fields
+		 * each buffer has two fields (top & bottom)
+		 * Removing one buffer is actually getting two fields
+		 * Alternate between two operations:-
+		 * Even : consume one field but DO NOT REMOVE from queue
+		 * Odd : consume other field and REMOVE from queue
 		 */
 		ctx->src_vbs[0] = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
 		WARN_ON(ctx->src_vbs[0] == NULL);
@@ -1573,8 +1587,10 @@ 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
-			&& pix->field != V4L2_FIELD_SEQ_TB)
+	if (pix->field != V4L2_FIELD_NONE &&
+	    pix->field != V4L2_FIELD_ALTERNATE &&
+	    pix->field != V4L2_FIELD_SEQ_TB &&
+	    pix->field != V4L2_FIELD_SEQ_BT)
 		pix->field = V4L2_FIELD_NONE;
 
 	depth = fmt->vpdma_fmt[VPE_LUMA]->depth;
@@ -1626,9 +1642,9 @@ static int __vpe_try_fmt(struct vpe_ctx *ctx, struct v4l2_format *f,
 
 	/*
 	 * For the actual image parameters, we need to consider the field
-	 * height of the image for SEQ_TB buffers.
+	 * height of the image for SEQ_XX buffers.
 	 */
-	if (pix->field == V4L2_FIELD_SEQ_TB)
+	if (pix->field == V4L2_FIELD_SEQ_TB || pix->field == V4L2_FIELD_SEQ_BT)
 		height = pix->height / 2;
 	else
 		height = pix->height;
@@ -1734,11 +1750,13 @@ static int __vpe_s_fmt(struct vpe_ctx *ctx, struct v4l2_format *f)
 		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 if (q_data->field == V4L2_FIELD_SEQ_BT)
+		q_data->flags |= Q_DATA_INTERLACED_SEQ_BT;
 	else
 		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)
+	/* the crop height is halved for the case of SEQ_XX buffers */
+	if (q_data->flags & Q_IS_SEQ_XX)
 		q_data->c_rect.height /= 2;
 
 	vpe_dbg(ctx->dev, "Setting format for type %d, wxh: %dx%d, fmt: %d bpl_y %d",
@@ -1811,10 +1829,10 @@ static int __vpe_try_selection(struct vpe_ctx *ctx, struct v4l2_selection *s)
 	}
 
 	/*
-	 * For SEQ_TB buffers, crop height should be less than the height of
+	 * For SEQ_XX 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)
+	if (q_data->flags & Q_IS_SEQ_XX)
 		height = q_data->height / 2;
 	else
 		height = q_data->height;
@@ -2031,7 +2049,8 @@ static int vpe_buf_prepare(struct vb2_buffer *vb)
 		} else {
 			if (vbuf->field != V4L2_FIELD_TOP &&
 			    vbuf->field != V4L2_FIELD_BOTTOM &&
-			    vbuf->field != V4L2_FIELD_SEQ_TB)
+			    vbuf->field != V4L2_FIELD_SEQ_TB &&
+			    vbuf->field != V4L2_FIELD_SEQ_BT)
 				return -EINVAL;
 		}
 	}
-- 
2.17.1

[Patch 05/16] media: ti-vpe: Add support for NV21 format

[Benoit Parrot]

From: Nikhil Devshatwar <nikhil.nd@ti.com>

In NV21 format, the chroma plane is written to memory such that the U
and V components are swapped for NV12.

Create a new entry in the VPDMA formats to describe the correct data
types used in the data descriptors.

Update all checks for NV12 and add NV21 there as well.

Add support for V4L2_PIX_FMT_NV21 format for both capture and output
streams.

Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
Signed-off-by: Benoit Parrot <bparrot@ti.com>
Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/media/platform/ti-vpe/vpdma.c      | 11 ++++++--
 drivers/media/platform/ti-vpe/vpdma.h      |  1 +
 drivers/media/platform/ti-vpe/vpdma_priv.h |  1 +
 drivers/media/platform/ti-vpe/vpe.c        | 29 +++++++++++++++++-----
 4 files changed, 34 insertions(+), 8 deletions(-)

diff --git a/drivers/media/platform/ti-vpe/vpdma.c b/drivers/media/platform/ti-vpe/vpdma.c
index 53d27cd6e10a..817d287c8138 100644
--- a/drivers/media/platform/ti-vpe/vpdma.c
+++ b/drivers/media/platform/ti-vpe/vpdma.c
@@ -56,6 +56,11 @@ const struct vpdma_data_format vpdma_yuv_fmts[] = {
 		.data_type	= DATA_TYPE_C420,
 		.depth		= 4,
 	},
+	[VPDMA_DATA_FMT_CB420] = {
+		.type		= VPDMA_DATA_FMT_TYPE_YUV,
+		.data_type	= DATA_TYPE_CB420,
+		.depth		= 4,
+	},
 	[VPDMA_DATA_FMT_YCR422] = {
 		.type		= VPDMA_DATA_FMT_TYPE_YUV,
 		.data_type	= DATA_TYPE_YCR422,
@@ -825,7 +830,8 @@ void vpdma_rawchan_add_out_dtd(struct vpdma_desc_list *list, int width,
 	channel = next_chan = raw_vpdma_chan;
 
 	if (fmt->type == VPDMA_DATA_FMT_TYPE_YUV &&
-			fmt->data_type == DATA_TYPE_C420) {
+	    (fmt->data_type == DATA_TYPE_C420 ||
+	     fmt->data_type == DATA_TYPE_CB420)) {
 		rect.height >>= 1;
 		rect.top >>= 1;
 		depth = 8;
@@ -893,7 +899,8 @@ void vpdma_add_in_dtd(struct vpdma_desc_list *list, int width,
 	channel = next_chan = chan_info[chan].num;
 
 	if (fmt->type == VPDMA_DATA_FMT_TYPE_YUV &&
-			fmt->data_type == DATA_TYPE_C420) {
+	    (fmt->data_type == DATA_TYPE_C420 ||
+	     fmt->data_type == DATA_TYPE_CB420)) {
 		rect.height >>= 1;
 		rect.top >>= 1;
 		depth = 8;
diff --git a/drivers/media/platform/ti-vpe/vpdma.h b/drivers/media/platform/ti-vpe/vpdma.h
index 28bc94129348..bce17329c4c9 100644
--- a/drivers/media/platform/ti-vpe/vpdma.h
+++ b/drivers/media/platform/ti-vpe/vpdma.h
@@ -71,6 +71,7 @@ enum vpdma_yuv_formats {
 	VPDMA_DATA_FMT_C444,
 	VPDMA_DATA_FMT_C422,
 	VPDMA_DATA_FMT_C420,
+	VPDMA_DATA_FMT_CB420,
 	VPDMA_DATA_FMT_YCR422,
 	VPDMA_DATA_FMT_YC444,
 	VPDMA_DATA_FMT_CRY422,
diff --git a/drivers/media/platform/ti-vpe/vpdma_priv.h b/drivers/media/platform/ti-vpe/vpdma_priv.h
index c488609bc162..d8ae3e1cd54d 100644
--- a/drivers/media/platform/ti-vpe/vpdma_priv.h
+++ b/drivers/media/platform/ti-vpe/vpdma_priv.h
@@ -92,6 +92,7 @@
 #define DATA_TYPE_C444				0x4
 #define DATA_TYPE_C422				0x5
 #define DATA_TYPE_C420				0x6
+#define DATA_TYPE_CB420				0x16
 #define DATA_TYPE_YC444				0x8
 #define DATA_TYPE_YCB422			0x7
 #define DATA_TYPE_YCR422			0x17
diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
index 5d0ec5f7ca25..f3ee9ff87927 100644
--- a/drivers/media/platform/ti-vpe/vpe.c
+++ b/drivers/media/platform/ti-vpe/vpe.c
@@ -248,6 +248,14 @@ static struct vpe_fmt vpe_formats[] = {
 				    &vpdma_yuv_fmts[VPDMA_DATA_FMT_C420],
 				  },
 	},
+	{
+		.fourcc		= V4L2_PIX_FMT_NV21,
+		.types		= VPE_FMT_TYPE_CAPTURE | VPE_FMT_TYPE_OUTPUT,
+		.coplanar	= 1,
+		.vpdma_fmt	= { &vpdma_yuv_fmts[VPDMA_DATA_FMT_Y420],
+				    &vpdma_yuv_fmts[VPDMA_DATA_FMT_CB420],
+				  },
+	},
 	{
 		.fourcc		= V4L2_PIX_FMT_YUYV,
 		.types		= VPE_FMT_TYPE_CAPTURE | VPE_FMT_TYPE_OUTPUT,
@@ -686,7 +694,8 @@ static void set_cfg_modes(struct vpe_ctx *ctx)
 	 * Cfg Mode 1: YUV422 source, disable upsampler, DEI is de-interlacing.
 	 */
 
-	if (fmt->fourcc == V4L2_PIX_FMT_NV12)
+	if (fmt->fourcc == V4L2_PIX_FMT_NV12 ||
+	    fmt->fourcc == V4L2_PIX_FMT_NV21)
 		cfg_mode = 0;
 
 	write_field(us1_reg0, cfg_mode, VPE_US_MODE_MASK, VPE_US_MODE_SHIFT);
@@ -701,7 +710,8 @@ static void set_line_modes(struct vpe_ctx *ctx)
 	struct vpe_fmt *fmt = ctx->q_data[Q_DATA_SRC].fmt;
 	int line_mode = 1;
 
-	if (fmt->fourcc == V4L2_PIX_FMT_NV12)
+	if (fmt->fourcc == V4L2_PIX_FMT_NV12 ||
+	    fmt->fourcc == V4L2_PIX_FMT_NV21)
 		line_mode = 0;		/* double lines to line buffer */
 
 	/* regs for now */
@@ -763,7 +773,8 @@ static void set_dst_registers(struct vpe_ctx *ctx)
 	 */
 	val |= VPE_DS_SRC_DEI_SCALER | VPE_CSC_SRC_DEI_SCALER;
 
-	if (fmt->fourcc != V4L2_PIX_FMT_NV12)
+	if (fmt->fourcc != V4L2_PIX_FMT_NV12 &&
+	    fmt->fourcc != V4L2_PIX_FMT_NV21)
 		val |= VPE_DS_BYPASS;
 
 	mmr_adb->out_fmt_reg[0] = val;
@@ -1129,8 +1140,13 @@ static void add_in_dtd(struct vpe_ctx *ctx, int port)
 
 			if (field) {
 				int height = q_data->height / 2;
-				int bpp = fmt->fourcc == V4L2_PIX_FMT_NV12 ?
-						1 : (vpdma_fmt->depth >> 3);
+				int bpp;
+
+				if (fmt->fourcc == V4L2_PIX_FMT_NV12 ||
+				    fmt->fourcc == V4L2_PIX_FMT_NV21)
+					bpp = 1;
+				else
+					bpp = vpdma_fmt->depth >> 3;
 
 				if (plane)
 					height /= 2;
@@ -1148,7 +1164,8 @@ static void add_in_dtd(struct vpe_ctx *ctx, int port)
 	frame_width = q_data->c_rect.width;
 	frame_height = q_data->c_rect.height;
 
-	if (p_data->vb_part && fmt->fourcc == V4L2_PIX_FMT_NV12)
+	if (p_data->vb_part && (fmt->fourcc == V4L2_PIX_FMT_NV12 ||
+				fmt->fourcc == V4L2_PIX_FMT_NV21))
 		frame_height /= 2;
 
 	vpdma_add_in_dtd(&ctx->desc_list, q_data->width, stride,
-- 
2.17.1

[Patch 06/16] media: ti-vpe: Set MAX height supported to 2048 pixels

[Benoit Parrot]

From: Ram Prasad <x0038811@ti.com>

VPE's max height supported MAX_H is set to 1184 which is the
padded height from VC1 decoder output.

In case of 90, 270 degree rotated video processing, input to
VPE will be 1080x1920, 720x1280 etc and MAX_H needs to be set
correct value. Setting MAX_H to 2048 as worst case height.

Signed-off-by: Ram Prasad <x0038811@ti.com>
Signed-off-by: Benoit Parrot <bparrot@ti.com>
---
 drivers/media/platform/ti-vpe/vpe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
index f3ee9ff87927..bbbf11174e16 100644
--- a/drivers/media/platform/ti-vpe/vpe.c
+++ b/drivers/media/platform/ti-vpe/vpe.c
@@ -52,7 +52,7 @@
 #define MIN_W		32
 #define MIN_H		32
 #define MAX_W		2048
-#define MAX_H		1184
+#define MAX_H		2048
 
 /* required alignments */
 #define S_ALIGN		0	/* multiple of 1 */
-- 
2.17.1

[Patch 07/16] media: ti-vpe: vpe: fix a v4l2-compliance failure causing a kernel panic

[Benoit Parrot]

v4l2-compliance fails with this message:

   warn: v4l2-test-formats.cpp(717): \
   	TRY_FMT cannot handle an invalid pixelformat.
   test VIDIOC_TRY_FMT: FAIL

This causes the following kernel panic:

Unable to handle kernel paging request at virtual address 56595561
pgd = ecd80e00
*pgd=00000000
Internal error: Oops: 205 [#1] PREEMPT SMP ARM
...
CPU: 0 PID: 930 Comm: v4l2-compliance Not tainted \
	4.14.62-01715-gc8cd67f49a19 #1
Hardware name: Generic DRA72X (Flattened Device Tree)
task: ece44d80 task.stack: ecc6e000
PC is at __vpe_try_fmt+0x18c/0x2a8 [ti_vpe]
LR is at 0x8

Because the driver fails to properly check the 'num_planes' values for
proper ranges it ends up accessing out of bound data causing the kernel
panic.

Since this driver only handle single or dual plane pixel format, make
sure the provided value does not exceed 2 planes.

Signed-off-by: Benoit Parrot <bparrot@ti.com>
Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/media/platform/ti-vpe/vpe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
index bbbf11174e16..1278d457f753 100644
--- a/drivers/media/platform/ti-vpe/vpe.c
+++ b/drivers/media/platform/ti-vpe/vpe.c
@@ -1650,7 +1650,7 @@ static int __vpe_try_fmt(struct vpe_ctx *ctx, struct v4l2_format *f,
 			      &pix->height, MIN_H, MAX_H, H_ALIGN,
 			      S_ALIGN);
 
-	if (!pix->num_planes)
+	if (!pix->num_planes || pix->num_planes > 2)
 		pix->num_planes = fmt->coplanar ? 2 : 1;
 	else if (pix->num_planes > 1 && !fmt->coplanar)
 		pix->num_planes = 1;
-- 
2.17.1

Re: [Patch 07/16] media: ti-vpe: vpe: fix a v4l2-compliance failure causing a kernel panic

[Hans Verkuil]

On 9/27/19 8:36 PM, Benoit Parrot wrote:
> v4l2-compliance fails with this message:
> 
>    warn: v4l2-test-formats.cpp(717): \
>    	TRY_FMT cannot handle an invalid pixelformat.
>    test VIDIOC_TRY_FMT: FAIL
> 
> This causes the following kernel panic:
> 
> Unable to handle kernel paging request at virtual address 56595561
> pgd = ecd80e00
> *pgd=00000000
> Internal error: Oops: 205 [#1] PREEMPT SMP ARM
> ...
> CPU: 0 PID: 930 Comm: v4l2-compliance Not tainted \
> 	4.14.62-01715-gc8cd67f49a19 #1
> Hardware name: Generic DRA72X (Flattened Device Tree)
> task: ece44d80 task.stack: ecc6e000
> PC is at __vpe_try_fmt+0x18c/0x2a8 [ti_vpe]
> LR is at 0x8
> 
> Because the driver fails to properly check the 'num_planes' values for
> proper ranges it ends up accessing out of bound data causing the kernel
> panic.
> 
> Since this driver only handle single or dual plane pixel format, make
> sure the provided value does not exceed 2 planes.
> 
> Signed-off-by: Benoit Parrot <bparrot@ti.com>
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/media/platform/ti-vpe/vpe.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
> index bbbf11174e16..1278d457f753 100644
> --- a/drivers/media/platform/ti-vpe/vpe.c
> +++ b/drivers/media/platform/ti-vpe/vpe.c
> @@ -1650,7 +1650,7 @@ static int __vpe_try_fmt(struct vpe_ctx *ctx, struct v4l2_format *f,
>  			      &pix->height, MIN_H, MAX_H, H_ALIGN,
>  			      S_ALIGN);
>  
> -	if (!pix->num_planes)
> +	if (!pix->num_planes || pix->num_planes > 2)
>  		pix->num_planes = fmt->coplanar ? 2 : 1;
>  	else if (pix->num_planes > 1 && !fmt->coplanar)
>  		pix->num_planes = 1;
> 

This looks weird.

Why not just unconditionally do:

	pix->num_planes = fmt->coplanar ? 2 : 1;

Regards,

	Hans

Re: [Patch 07/16] media: ti-vpe: vpe: fix a v4l2-compliance failure causing a kernel panic

[Benoit Parrot]

Hans Verkuil <hverkuil@xs4all.nl> wrote on Mon [2019-Sep-30 10:35:05 +0200]:
> On 9/27/19 8:36 PM, Benoit Parrot wrote:
> > v4l2-compliance fails with this message:
> > 
> >    warn: v4l2-test-formats.cpp(717): \
> >    	TRY_FMT cannot handle an invalid pixelformat.
> >    test VIDIOC_TRY_FMT: FAIL
> > 
> > This causes the following kernel panic:
> > 
> > Unable to handle kernel paging request at virtual address 56595561
> > pgd = ecd80e00
> > *pgd=00000000
> > Internal error: Oops: 205 [#1] PREEMPT SMP ARM
> > ...
> > CPU: 0 PID: 930 Comm: v4l2-compliance Not tainted \
> > 	4.14.62-01715-gc8cd67f49a19 #1
> > Hardware name: Generic DRA72X (Flattened Device Tree)
> > task: ece44d80 task.stack: ecc6e000
> > PC is at __vpe_try_fmt+0x18c/0x2a8 [ti_vpe]
> > LR is at 0x8
> > 
> > Because the driver fails to properly check the 'num_planes' values for
> > proper ranges it ends up accessing out of bound data causing the kernel
> > panic.
> > 
> > Since this driver only handle single or dual plane pixel format, make
> > sure the provided value does not exceed 2 planes.
> > 
> > Signed-off-by: Benoit Parrot <bparrot@ti.com>
> > Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> > ---
> >  drivers/media/platform/ti-vpe/vpe.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
> > index bbbf11174e16..1278d457f753 100644
> > --- a/drivers/media/platform/ti-vpe/vpe.c
> > +++ b/drivers/media/platform/ti-vpe/vpe.c
> > @@ -1650,7 +1650,7 @@ static int __vpe_try_fmt(struct vpe_ctx *ctx, struct v4l2_format *f,
> >  			      &pix->height, MIN_H, MAX_H, H_ALIGN,
> >  			      S_ALIGN);
> >  
> > -	if (!pix->num_planes)
> > +	if (!pix->num_planes || pix->num_planes > 2)
> >  		pix->num_planes = fmt->coplanar ? 2 : 1;
> >  	else if (pix->num_planes > 1 && !fmt->coplanar)
> >  		pix->num_planes = 1;
> > 
> 
> This looks weird.
> 
> Why not just unconditionally do:
> 
> 	pix->num_planes = fmt->coplanar ? 2 : 1;

In order to not change the behavior, VPE would assume that NV12 format for
instance were always sent as 2 separate planes buffers. So for backward
compatibility this is order to handle both cases where NV12 could be
handled as both a single plane and a dual plane buffers based on what the
user space application intent on passing in/out.

Benoit

> 
> Regards,
> 
> 	Hans

[Patch 08/16] media: ti-vpe: vpe: fix a v4l2-compliance warning about invalid pixel format

[Benoit Parrot]

v4l2-compliance warns with this message:

   warn: v4l2-test-formats.cpp(717): \
 	TRY_FMT cannot handle an invalid pixelformat.
   warn: v4l2-test-formats.cpp(718): \
 	This may or may not be a problem. For more information see:
   warn: v4l2-test-formats.cpp(719): \
 	http://www.mail-archive.com/linux-media@vger.kernel.org/msg56550.html
	...
   test VIDIOC_TRY_FMT: FAIL

We need to make sure that the returns a valid pixel format in all
instance. Based on the v4l2 framework convention drivers must return a
valid pixel format when the requested pixel format is either invalid or
not supported.

Signed-off-by: Benoit Parrot <bparrot@ti.com>
Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/media/platform/ti-vpe/vpe.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
index 1278d457f753..d3f1ae8b72fa 100644
--- a/drivers/media/platform/ti-vpe/vpe.c
+++ b/drivers/media/platform/ti-vpe/vpe.c
@@ -351,20 +351,25 @@ enum {
 };
 
 /* find our format description corresponding to the passed v4l2_format */
-static struct vpe_fmt *find_format(struct v4l2_format *f)
+static struct vpe_fmt *__find_format(u32 fourcc)
 {
 	struct vpe_fmt *fmt;
 	unsigned int k;
 
 	for (k = 0; k < ARRAY_SIZE(vpe_formats); k++) {
 		fmt = &vpe_formats[k];
-		if (fmt->fourcc == f->fmt.pix.pixelformat)
+		if (fmt->fourcc == fourcc)
 			return fmt;
 	}
 
 	return NULL;
 }
 
+static struct vpe_fmt *find_format(struct v4l2_format *f)
+{
+	return __find_format(f->fmt.pix.pixelformat);
+}
+
 /*
  * there is one vpe_dev structure in the driver, it is shared by
  * all instances.
@@ -1599,9 +1604,9 @@ static int __vpe_try_fmt(struct vpe_ctx *ctx, struct v4l2_format *f,
 	unsigned int stride = 0;
 
 	if (!fmt || !(fmt->types & type)) {
-		vpe_err(ctx->dev, "Fourcc format (0x%08x) invalid.\n",
+		vpe_dbg(ctx->dev, "Fourcc format (0x%08x) invalid.\n",
 			pix->pixelformat);
-		return -EINVAL;
+		fmt = __find_format(V4L2_PIX_FMT_YUYV);
 	}
 
 	if (pix->field != V4L2_FIELD_NONE &&
-- 
2.17.1

[Patch 09/16] media: ti-vpe: vpe: Make sure YUYV is set as default format

[Benoit Parrot]

v4l2-compliance fails with this message:

   fail: v4l2-test-formats.cpp(672): \
	Video Capture Multiplanar: TRY_FMT(G_FMT) != G_FMT
   fail: v4l2-test-formats.cpp(672): \
	Video Output Multiplanar: TRY_FMT(G_FMT) != G_FMT
	...
   test VIDIOC_TRY_FMT: FAIL

Fixes: 94ed726e8e01 ("media: ti-vpe: Add support for NV21 format")

The default pixel format was setup as pointing to a specific offset in
the vpe_formats table assuming it was pointing to the V4L2_PIX_FMT_YUYV
entry. This became false after the addition on the NV21 format (see
above commid-id)

So instead of hard-coding an offset which might change over time we need
to use a lookup helper instead so we know the default will always be what
we intended.

Signed-off-by: Benoit Parrot <bparrot@ti.com>
Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/media/platform/ti-vpe/vpe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
index d3f1ae8b72fa..7aa83026fb6c 100644
--- a/drivers/media/platform/ti-vpe/vpe.c
+++ b/drivers/media/platform/ti-vpe/vpe.c
@@ -2321,7 +2321,7 @@ static int vpe_open(struct file *file)
 	v4l2_ctrl_handler_setup(hdl);
 
 	s_q_data = &ctx->q_data[Q_DATA_SRC];
-	s_q_data->fmt = &vpe_formats[2];
+	s_q_data->fmt = __find_format(V4L2_PIX_FMT_YUYV);
 	s_q_data->width = 1920;
 	s_q_data->height = 1080;
 	s_q_data->nplanes = 1;
-- 
2.17.1

[Patch 10/16] media: ti-vpe: vpe: fix a v4l2-compliance failure about invalid sizeimage

[Benoit Parrot]

v4l2-compliance fails with this message:

   fail: v4l2-test-formats.cpp(463): !pfmt.sizeimage
   fail: v4l2-test-formats.cpp(736): \
	Video Capture Multiplanar is valid, \
	but TRY_FMT failed to return a format
   test VIDIOC_TRY_FMT: FAIL

This failure is causd by the driver failing to handle out range
'bytesperline' values from user space applications.

VPDMA hardware is limited to 64k line stride (16 bytes aligned, so 65520
bytes). So make sure the provided or calculated 'bytesperline' is
smaller than the maximum value.

Signed-off-by: Benoit Parrot <bparrot@ti.com>
Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/media/platform/ti-vpe/vpdma.h | 1 +
 drivers/media/platform/ti-vpe/vpe.c   | 4 ++++
 2 files changed, 5 insertions(+)

diff --git a/drivers/media/platform/ti-vpe/vpdma.h b/drivers/media/platform/ti-vpe/vpdma.h
index bce17329c4c9..393fcbb3cb40 100644
--- a/drivers/media/platform/ti-vpe/vpdma.h
+++ b/drivers/media/platform/ti-vpe/vpdma.h
@@ -57,6 +57,7 @@ struct vpdma_data_format {
 						 * line stride of source and dest
 						 * buffers should be 16 byte aligned
 						 */
+#define VPDMA_MAX_STRIDE		65520	/* Max line stride 16 byte aligned */
 #define VPDMA_DTD_DESC_SIZE		32	/* 8 words */
 #define VPDMA_CFD_CTD_DESC_SIZE		16	/* 4 words */
 
diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
index 7aa83026fb6c..0a7cf9c820c6 100644
--- a/drivers/media/platform/ti-vpe/vpe.c
+++ b/drivers/media/platform/ti-vpe/vpe.c
@@ -1694,6 +1694,10 @@ static int __vpe_try_fmt(struct vpe_ctx *ctx, struct v4l2_format *f,
 		if (stride > plane_fmt->bytesperline)
 			plane_fmt->bytesperline = stride;
 
+		plane_fmt->bytesperline = clamp_t(u32, plane_fmt->bytesperline,
+						  stride,
+						  VPDMA_MAX_STRIDE);
+
 		plane_fmt->bytesperline = ALIGN(plane_fmt->bytesperline,
 						VPDMA_STRIDE_ALIGN);
 
-- 
2.17.1

[Patch 11/16] media: ti-vpe: vpe: fix a v4l2-compliance failure about frame sequence number

[Benoit Parrot]

v4l2-compliance fails with this message:

   fail: v4l2-test-buffers.cpp(294): \
	(int)g_sequence() < seq.last_seq + 1
   fail: v4l2-test-buffers.cpp(740): \
	buf.check(m2m_q, last_m2m_seq)
   fail: v4l2-test-buffers.cpp(974): \
	captureBufs(node, q, m2m_q, frame_count, true)
   test MMAP: FAIL

The driver is failing to update the source frame sequence number in the
vb2 buffer object. Only the destination frame sequence was being
updated.

This is only a reporting issue if the user space app actually cares
about the frame sequence number. But it is fixed nonetheless.

Signed-off-by: Benoit Parrot <bparrot@ti.com>
Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/media/platform/ti-vpe/vpe.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
index 0a7cf9c820c6..8ab1c3241b74 100644
--- a/drivers/media/platform/ti-vpe/vpe.c
+++ b/drivers/media/platform/ti-vpe/vpe.c
@@ -1440,6 +1440,7 @@ static irqreturn_t vpe_irq(int irq_vpe, void *data)
 		d_vb->timecode = s_vb->timecode;
 
 	d_vb->sequence = ctx->sequence;
+	s_vb->sequence = ctx->sequence;
 
 	d_q_data = &ctx->q_data[Q_DATA_DST];
 	if (d_q_data->flags & Q_IS_INTERLACED) {
-- 
2.17.1

[Patch 12/16] media: ti-vpe: vpe: ensure buffers are cleaned up properly in abort cases

[Benoit Parrot]

v4l2-compliance fails with this message:

   fail: v4l2-test-buffers.cpp(691): ret == 0
   fail: v4l2-test-buffers.cpp(974): captureBufs(node, q, m2m_q,
frame_count, true)
   test MMAP: FAIL

This caused the following Kernel Warning:

WARNING: CPU: 0 PID: 961 at
drivers/media/v4l2-core/videobuf2-core.c:1658
__vb2_queue_cancel+0x174/0x1d8
...
CPU: 0 PID: 961 Comm: v4l2-compliance Not tainted
4.14.62-01720-g20ecd717e87a #6
Hardware name: Generic DRA72X (Flattened Device Tree)
Backtrace:
[<c020b5bc>] (dump_backtrace) from [<c020b8a0>] (show_stack+0x18/0x1c)
 r7:00000009 r6:60070013 r5:00000000 r4:c1053824
[<c020b888>] (show_stack) from [<c09232e8>] (dump_stack+0x90/0xa4)
[<c0923258>] (dump_stack) from [<c022b740>] (__warn+0xec/0x104)
  r7:00000009 r6:c0c0ad50 r5:00000000 r4:00000000
[<c022b654>] (__warn) from [<c022b810>] (warn_slowpath_null+0x28/0x30)
  r9:00000008 r8:00000000 r7:eced4808 r6:edbc9bac r5:eced4844
r4:eced4808
[<c022b7e8>] (warn_slowpath_null) from [<c0726f48>]
(__vb2_queue_cancel+0x174/0x1d8)
[<c0726dd4>] (__vb2_queue_cancel) from [<c0727648>]
(vb2_core_queue_release+0x20/0x40)
  r10:ecc7bd70 r9:00000008 r8:00000000 r7:edb73010 r6:edbc9bac
r5:eced4844
  r4:eced4808 r3:00000004
[<c0727628>] (vb2_core_queue_release) from [<c0729528>]
(vb2_queue_release+0x10/0x14)
  r5:edbc9810 r4:eced4800
[<c0729518>] (vb2_queue_release) from [<c0724d08>]
(v4l2_m2m_ctx_release+0x1c/0x30)
[<c0724cec>] (v4l2_m2m_ctx_release) from [<bf0e8f28>]
(vpe_release+0x74/0xb0 [ti_vpe])
  r5:edbc9810 r4:ed67a400
[<bf0e8eb4>] (vpe_release [ti_vpe]) from [<c070fccc>]
(v4l2_release+0x3c/0x80)
  r7:edb73010 r6:ed176aa0 r5:edbc9868 r4:ed5119c0
[<c070fc90>] (v4l2_release) from [<c033cf1c>] (__fput+0x8c/0x1dc)
  r5:ecc7bd70 r4:ed5119c0
[<c033ce90>] (__fput) from [<c033d0cc>] (____fput+0x10/0x14)
  r10:00000000 r9:ed5119c0 r8:ece392d0 r7:c1059544 r6:ece38d80
r5:ece392b4
  r4:00000000
[<c033d0bc>] (____fput) from [<c0246e00>] (task_work_run+0x98/0xb8)
[<c0246d68>] (task_work_run) from [<c022f1d8>] (do_exit+0x170/0xa80)
  r9:ece351fc r8:00000000 r7:ecde3f58 r6:ffffe000 r5:ece351c0
r4:ece38d80
[<c022f068>] (do_exit) from [<c022fb6c>] (do_group_exit+0x48/0xc4)
  r7:000000f8
[<c022fb24>] (do_group_exit) from [<c022fc00>]
(__wake_up_parent+0x0/0x28)
  r7:000000f8 r6:b6c6a798 r5:00000001 r4:00000001
[<c022fbe8>] (SyS_exit_group) from [<c0207c80>]
(ret_fast_syscall+0x0/0x4c)

These warnings are caused by buffers which not properly cleaned
up/release during an abort use case.

In the abort cases the VPDMA desc buffers would still be mapped and the
in-flight VB2 buffers would not be released properly causing a kernel
warning from being generated by the videobuf2-core level.

Signed-off-by: Benoit Parrot <bparrot@ti.com>
Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/media/platform/ti-vpe/vpe.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
index 8ab1c3241b74..ad9d8b559cad 100644
--- a/drivers/media/platform/ti-vpe/vpe.c
+++ b/drivers/media/platform/ti-vpe/vpe.c
@@ -1427,9 +1427,6 @@ static irqreturn_t vpe_irq(int irq_vpe, void *data)
 	 /* the previous dst mv buffer becomes the next src mv buffer */
 	ctx->src_mv_buf_selector = !ctx->src_mv_buf_selector;
 
-	if (ctx->aborting)
-		goto finished;
-
 	s_vb = ctx->src_vbs[0];
 	d_vb = ctx->dst_vb;
 
@@ -1494,6 +1491,9 @@ static irqreturn_t vpe_irq(int irq_vpe, void *data)
 	ctx->src_vbs[0] = NULL;
 	ctx->dst_vb = NULL;
 
+	if (ctx->aborting)
+		goto finished;
+
 	ctx->bufs_completed++;
 	if (ctx->bufs_completed < ctx->bufs_per_job && job_ready(ctx)) {
 		device_run(ctx);
@@ -2404,6 +2404,12 @@ static int vpe_release(struct file *file)
 
 	mutex_lock(&dev->dev_mutex);
 	free_mv_buffers(ctx);
+
+	vpdma_unmap_desc_buf(dev->vpdma, &ctx->desc_list.buf);
+	vpdma_unmap_desc_buf(dev->vpdma, &ctx->mmr_adb);
+	vpdma_unmap_desc_buf(dev->vpdma, &ctx->sc_coeff_h);
+	vpdma_unmap_desc_buf(dev->vpdma, &ctx->sc_coeff_v);
+
 	vpdma_free_desc_list(&ctx->desc_list);
 	vpdma_free_desc_buf(&ctx->mmr_adb);
 
-- 
2.17.1

[Patch 13/16] media: ti-vpe: vpdma: Use fixed type for address in descriptor

[Benoit Parrot]

Using dma_addr_t as the type to hold address inside of a fix sized
descriptor used by the vpdma firmware is prone to fail when the expected
width is 32 bits and suddenly when CONFIG_LPAE is enabled the data size
is now 64 bits shifted the remaining members of the descriptor in memory
which confuses the firmware.

Signed-off-by: Benoit Parrot <bparrot@ti.com>
---
 drivers/media/platform/ti-vpe/vpdma_priv.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/ti-vpe/vpdma_priv.h b/drivers/media/platform/ti-vpe/vpdma_priv.h
index d8ae3e1cd54d..0bbee45338bd 100644
--- a/drivers/media/platform/ti-vpe/vpdma_priv.h
+++ b/drivers/media/platform/ti-vpe/vpdma_priv.h
@@ -166,11 +166,11 @@ struct vpdma_dtd {
 		u32		xfer_length_height;
 		u32		w1;
 	};
-	dma_addr_t		start_addr;
+	u32			start_addr;
 	u32			pkt_ctl;
 	union {
 		u32		frame_width_height;	/* inbound */
-		dma_addr_t	desc_write_addr;	/* outbound */
+		u32		desc_write_addr;	/* outbound */
 	};
 	union {
 		u32		start_h_v;		/* inbound */
-- 
2.17.1

[Patch 14/16] media: ti-vpe: Set the DMA mask and coherent mask

[Benoit Parrot]

VPE uses VPDMA (built-in dma engine) to transfer data to and from the IP
and memory. VPDMA expect 32 bits addresses. To make sure that is always
the case set the DMA mask and coherent mask for the device.

Signed-off-by: Benoit Parrot <bparrot@ti.com>
---
 drivers/media/platform/ti-vpe/vpe.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
index ad9d8b559cad..d7f8eb901475 100644
--- a/drivers/media/platform/ti-vpe/vpe.c
+++ b/drivers/media/platform/ti-vpe/vpe.c
@@ -2517,6 +2517,13 @@ static int vpe_probe(struct platform_device *pdev)
 	struct vpe_dev *dev;
 	int ret, irq, func;
 
+	ret = dma_coerce_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
+	if (ret) {
+		dev_err(&pdev->dev,
+			"32-bit consistent DMA enable failed\n");
+		return ret;
+	}
+
 	dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
 	if (!dev)
 		return -ENOMEM;
-- 
2.17.1

[Patch 15/16] media: ti-vpe: vpe: fix v4l2_compliance issue related to xfer_func

[Benoit Parrot]

All 4 of the "colorspace" components were not originally handled.
Causing issue related to xfer_func not being initialized properly.

This was found with v4l2-compliance test.

Signed-off-by: Benoit Parrot <bparrot@ti.com>
---
 drivers/media/platform/ti-vpe/vpe.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
index d7f8eb901475..e30981cd3e8f 100644
--- a/drivers/media/platform/ti-vpe/vpe.c
+++ b/drivers/media/platform/ti-vpe/vpe.c
@@ -324,6 +324,9 @@ struct vpe_q_data {
 	unsigned int		nplanes;			/* Current number of planes */
 	unsigned int		bytesperline[VPE_MAX_PLANES];	/* bytes per line in memory */
 	enum v4l2_colorspace	colorspace;
+	enum v4l2_ycbcr_encoding ycbcr_enc;
+	enum v4l2_xfer_func	xfer_func;
+	enum v4l2_quantization	quant;
 	enum v4l2_field		field;				/* supported field value */
 	unsigned int		flags;
 	unsigned int		sizeimage[VPE_MAX_PLANES];	/* image size in memory */
@@ -1576,6 +1579,9 @@ static int vpe_g_fmt(struct file *file, void *priv, struct v4l2_format *f)
 
 	if (V4L2_TYPE_IS_OUTPUT(f->type)) {
 		pix->colorspace = q_data->colorspace;
+		pix->xfer_func = q_data->xfer_func;
+		pix->ycbcr_enc = q_data->ycbcr_enc;
+		pix->quantization = q_data->quant;
 	} else {
 		struct vpe_q_data *s_q_data;
 
@@ -1583,6 +1589,9 @@ static int vpe_g_fmt(struct file *file, void *priv, struct v4l2_format *f)
 		s_q_data = get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE);
 
 		pix->colorspace = s_q_data->colorspace;
+		pix->xfer_func = s_q_data->xfer_func;
+		pix->ycbcr_enc = s_q_data->ycbcr_enc;
+		pix->quantization = s_q_data->quant;
 	}
 
 	pix->num_planes = q_data->nplanes;
@@ -1758,6 +1767,9 @@ static int __vpe_s_fmt(struct vpe_ctx *ctx, struct v4l2_format *f)
 	q_data->width		= pix->width;
 	q_data->height		= pix->height;
 	q_data->colorspace	= pix->colorspace;
+	q_data->xfer_func	= pix->xfer_func;
+	q_data->ycbcr_enc	= pix->ycbcr_enc;
+	q_data->quant		= pix->quantization;
 	q_data->field		= pix->field;
 	q_data->nplanes		= pix->num_planes;
 
-- 
2.17.1

[Patch 16/16] media: ti-vpe: vpe: don't rely on colorspace member for conversion

[Benoit Parrot]

Up to now VPE was relying on the colorspace value of struct v4l2_format
as an indication to perform color space conversion from YUV to RGB or
not.

Instead we should used the source/destination fourcc codes as a more
reliable indication to perform color space conversion or not.

Signed-off-by: Benoit Parrot <bparrot@ti.com>
---
 drivers/media/platform/ti-vpe/vpe.c | 41 ++++++++++++++++++++++-------
 1 file changed, 32 insertions(+), 9 deletions(-)

diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
index e30981cd3e8f..d002adc6263f 100644
--- a/drivers/media/platform/ti-vpe/vpe.c
+++ b/drivers/media/platform/ti-vpe/vpe.c
@@ -353,6 +353,32 @@ enum {
 	Q_DATA_DST = 1,
 };
 
+static bool is_fmt_rgb(u32 fourcc)
+{
+	if (fourcc == V4L2_PIX_FMT_RGB24 ||
+	    fourcc == V4L2_PIX_FMT_BGR24 ||
+	    fourcc == V4L2_PIX_FMT_RGB32 ||
+	    fourcc == V4L2_PIX_FMT_BGR32 ||
+	    fourcc == V4L2_PIX_FMT_RGB565 ||
+	    fourcc == V4L2_PIX_FMT_RGB555)
+		return true;
+
+	return false;
+}
+
+/*
+ * This helper is only used to setup the color space converter
+ * the actual value returned is only to broadly differentiate between
+ * RGB and YUV
+ */
+static enum  v4l2_colorspace fourcc_to_colorspace(u32 fourcc)
+{
+	if (is_fmt_rgb(fourcc))
+		return V4L2_COLORSPACE_SRGB;
+
+	return V4L2_COLORSPACE_SMPTE170M;
+}
+
 /* find our format description corresponding to the passed v4l2_format */
 static struct vpe_fmt *__find_format(u32 fourcc)
 {
@@ -764,11 +790,10 @@ static void set_src_registers(struct vpe_ctx *ctx)
 static void set_dst_registers(struct vpe_ctx *ctx)
 {
 	struct vpe_mmr_adb *mmr_adb = ctx->mmr_adb.addr;
-	enum v4l2_colorspace clrspc = ctx->q_data[Q_DATA_DST].colorspace;
 	struct vpe_fmt *fmt = ctx->q_data[Q_DATA_DST].fmt;
 	u32 val = 0;
 
-	if (clrspc == V4L2_COLORSPACE_SRGB) {
+	if (is_fmt_rgb(fmt->fourcc)) {
 		val |= VPE_RGB_OUT_SELECT;
 		vpdma_set_bg_color(ctx->dev->vpdma,
 			(struct vpdma_data_format *)fmt->vpdma_fmt[0], 0xff);
@@ -912,7 +937,8 @@ static int set_srcdst_params(struct vpe_ctx *ctx)
 	set_dei_regs(ctx);
 
 	csc_set_coeff(ctx->dev->csc, &mmr_adb->csc_regs[0],
-		s_q_data->colorspace, d_q_data->colorspace);
+		      fourcc_to_colorspace(s_q_data->fmt->fourcc),
+		      fourcc_to_colorspace(d_q_data->fmt->fourcc));
 
 	sc_set_hs_coeffs(ctx->dev->sc, ctx->sc_coeff_h.addr, src_w, dst_w);
 	sc_set_vs_coeffs(ctx->dev->sc, ctx->sc_coeff_v.addr, src_h, dst_h);
@@ -1285,7 +1311,7 @@ static void device_run(void *priv)
 	if (ctx->deinterlacing)
 		add_out_dtd(ctx, VPE_PORT_MV_OUT);
 
-	if (d_q_data->colorspace == V4L2_COLORSPACE_SRGB) {
+	if (is_fmt_rgb(d_q_data->fmt->fourcc)) {
 		add_out_dtd(ctx, VPE_PORT_RGB_OUT);
 	} else {
 		add_out_dtd(ctx, VPE_PORT_LUMA_OUT);
@@ -1327,7 +1353,7 @@ static void device_run(void *priv)
 	}
 
 	/* sync on channel control descriptors for output ports */
-	if (d_q_data->colorspace == V4L2_COLORSPACE_SRGB) {
+	if (is_fmt_rgb(d_q_data->fmt->fourcc)) {
 		vpdma_add_sync_on_channel_ctd(&ctx->desc_list,
 			VPE_CHAN_RGB_OUT);
 	} else {
@@ -1682,10 +1708,7 @@ static int __vpe_try_fmt(struct vpe_ctx *ctx, struct v4l2_format *f,
 		height = pix->height;
 
 	if (!pix->colorspace) {
-		if (fmt->fourcc == V4L2_PIX_FMT_RGB24 ||
-				fmt->fourcc == V4L2_PIX_FMT_BGR24 ||
-				fmt->fourcc == V4L2_PIX_FMT_RGB32 ||
-				fmt->fourcc == V4L2_PIX_FMT_BGR32) {
+		if (is_fmt_rgb(fmt->fourcc)) {
 			pix->colorspace = V4L2_COLORSPACE_SRGB;
 		} else {
 			if (height > 1280)	/* HD */
-- 
2.17.1

Re: [Patch 16/16] media: ti-vpe: vpe: don't rely on colorspace member for conversion

[Hans Verkuil]

On 9/27/19 8:36 PM, Benoit Parrot wrote:
> Up to now VPE was relying on the colorspace value of struct v4l2_format
> as an indication to perform color space conversion from YUV to RGB or
> not.
> 
> Instead we should used the source/destination fourcc codes as a more
> reliable indication to perform color space conversion or not.
> 
> Signed-off-by: Benoit Parrot <bparrot@ti.com>
> ---
>  drivers/media/platform/ti-vpe/vpe.c | 41 ++++++++++++++++++++++-------
>  1 file changed, 32 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
> index e30981cd3e8f..d002adc6263f 100644
> --- a/drivers/media/platform/ti-vpe/vpe.c
> +++ b/drivers/media/platform/ti-vpe/vpe.c
> @@ -353,6 +353,32 @@ enum {
>  	Q_DATA_DST = 1,
>  };
>  
> +static bool is_fmt_rgb(u32 fourcc)
> +{
> +	if (fourcc == V4L2_PIX_FMT_RGB24 ||
> +	    fourcc == V4L2_PIX_FMT_BGR24 ||
> +	    fourcc == V4L2_PIX_FMT_RGB32 ||
> +	    fourcc == V4L2_PIX_FMT_BGR32 ||
> +	    fourcc == V4L2_PIX_FMT_RGB565 ||
> +	    fourcc == V4L2_PIX_FMT_RGB555)
> +		return true;
> +
> +	return false;
> +}

Why not add a 'bool is_rgb' to struct vpe_fmt?

It is all too easy to forget to update this function if a new RGB format is
added to the vpe_formats array in the future.

> +
> +/*
> + * This helper is only used to setup the color space converter
> + * the actual value returned is only to broadly differentiate between
> + * RGB and YUV
> + */
> +static enum  v4l2_colorspace fourcc_to_colorspace(u32 fourcc)

double space after enum.

> +{
> +	if (is_fmt_rgb(fourcc))
> +		return V4L2_COLORSPACE_SRGB;
> +
> +	return V4L2_COLORSPACE_SMPTE170M;
> +}

This is highly confusing. RGB or YUV conversion does not change the colorspace
at all.

There are four colorimetry related fields: colorspace, xfer_func, ycbcr_enc and
quantization. Most hardware (including this one AFAICT) have a 3x3 matrix + a
vector to do the conversion. This only allows you to convert between different
ycbcr encodings and quantization ranges. The colorspace and xfer_func parameters
stay unchanged (you need gamma tables and two other 3x3 matrices for that).

So if you want to set up the 3x3 matrix + vector correctly, then you need to
provide the ycbcr_enc value + quantization value of the source to your function.
ycbcr_enc is only valid of YUV pixelformats, of course, and you can use
V4L2_MAP_COLORSPACE_DEFAULT, V4L2_MAP_YCBCR_ENC_DEFAULT and V4L2_MAP_QUANTIZATION_DEFAULT
if one or more of these values as set by userspace are 0.

Since the V4L2 API does not (yet) support setting ycbcr_enc and quantization for the
capture queue you have to provide that information yourself:

For RGB ycbcr_enc is of course ignored, and quantization should be full range.
For YUV it depends: the quantization is always limited range, but for the ycbcr_enc
you have a choice: if the source was YUV, then you can decide to just copy the
ycbcr_enc value. Alternatively, you can convert to 601 for SDTV and 709 for anything
else. The latter is also the recommended choice if the source was RGB.

In any case, please do not use enum v4l2_colorspace in the csc_set_coeff function:
it's misleading.

https://hverkuil.home.xs4all.nl/spec/uapi/v4l/colorspaces.html and the following two
sections contain a lot of information about how colorspaces work.

Regards,

	Hans

> +
>  /* find our format description corresponding to the passed v4l2_format */
>  static struct vpe_fmt *__find_format(u32 fourcc)
>  {
> @@ -764,11 +790,10 @@ static void set_src_registers(struct vpe_ctx *ctx)
>  static void set_dst_registers(struct vpe_ctx *ctx)
>  {
>  	struct vpe_mmr_adb *mmr_adb = ctx->mmr_adb.addr;
> -	enum v4l2_colorspace clrspc = ctx->q_data[Q_DATA_DST].colorspace;
>  	struct vpe_fmt *fmt = ctx->q_data[Q_DATA_DST].fmt;
>  	u32 val = 0;
>  
> -	if (clrspc == V4L2_COLORSPACE_SRGB) {
> +	if (is_fmt_rgb(fmt->fourcc)) {
>  		val |= VPE_RGB_OUT_SELECT;
>  		vpdma_set_bg_color(ctx->dev->vpdma,
>  			(struct vpdma_data_format *)fmt->vpdma_fmt[0], 0xff);
> @@ -912,7 +937,8 @@ static int set_srcdst_params(struct vpe_ctx *ctx)
>  	set_dei_regs(ctx);
>  
>  	csc_set_coeff(ctx->dev->csc, &mmr_adb->csc_regs[0],
> -		s_q_data->colorspace, d_q_data->colorspace);
> +		      fourcc_to_colorspace(s_q_data->fmt->fourcc),
> +		      fourcc_to_colorspace(d_q_data->fmt->fourcc));
>  
>  	sc_set_hs_coeffs(ctx->dev->sc, ctx->sc_coeff_h.addr, src_w, dst_w);
>  	sc_set_vs_coeffs(ctx->dev->sc, ctx->sc_coeff_v.addr, src_h, dst_h);
> @@ -1285,7 +1311,7 @@ static void device_run(void *priv)
>  	if (ctx->deinterlacing)
>  		add_out_dtd(ctx, VPE_PORT_MV_OUT);
>  
> -	if (d_q_data->colorspace == V4L2_COLORSPACE_SRGB) {
> +	if (is_fmt_rgb(d_q_data->fmt->fourcc)) {
>  		add_out_dtd(ctx, VPE_PORT_RGB_OUT);
>  	} else {
>  		add_out_dtd(ctx, VPE_PORT_LUMA_OUT);
> @@ -1327,7 +1353,7 @@ static void device_run(void *priv)
>  	}
>  
>  	/* sync on channel control descriptors for output ports */
> -	if (d_q_data->colorspace == V4L2_COLORSPACE_SRGB) {
> +	if (is_fmt_rgb(d_q_data->fmt->fourcc)) {
>  		vpdma_add_sync_on_channel_ctd(&ctx->desc_list,
>  			VPE_CHAN_RGB_OUT);
>  	} else {
> @@ -1682,10 +1708,7 @@ static int __vpe_try_fmt(struct vpe_ctx *ctx, struct v4l2_format *f,
>  		height = pix->height;
>  
>  	if (!pix->colorspace) {
> -		if (fmt->fourcc == V4L2_PIX_FMT_RGB24 ||
> -				fmt->fourcc == V4L2_PIX_FMT_BGR24 ||
> -				fmt->fourcc == V4L2_PIX_FMT_RGB32 ||
> -				fmt->fourcc == V4L2_PIX_FMT_BGR32) {
> +		if (is_fmt_rgb(fmt->fourcc)) {
>  			pix->colorspace = V4L2_COLORSPACE_SRGB;
>  		} else {
>  			if (height > 1280)	/* HD */
> 

Re: [Patch 16/16] media: ti-vpe: vpe: don't rely on colorspace member for conversion

[Benoit Parrot]

Hans Verkuil <hverkuil@xs4all.nl> wrote on Mon [2019-Sep-30 11:05:13 +0200]:
> On 9/27/19 8:36 PM, Benoit Parrot wrote:
> > Up to now VPE was relying on the colorspace value of struct v4l2_format
> > as an indication to perform color space conversion from YUV to RGB or
> > not.
> > 
> > Instead we should used the source/destination fourcc codes as a more
> > reliable indication to perform color space conversion or not.
> > 
> > Signed-off-by: Benoit Parrot <bparrot@ti.com>
> > ---
> >  drivers/media/platform/ti-vpe/vpe.c | 41 ++++++++++++++++++++++-------
> >  1 file changed, 32 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
> > index e30981cd3e8f..d002adc6263f 100644
> > --- a/drivers/media/platform/ti-vpe/vpe.c
> > +++ b/drivers/media/platform/ti-vpe/vpe.c
> > @@ -353,6 +353,32 @@ enum {
> >  	Q_DATA_DST = 1,
> >  };
> >  
> > +static bool is_fmt_rgb(u32 fourcc)
> > +{
> > +	if (fourcc == V4L2_PIX_FMT_RGB24 ||
> > +	    fourcc == V4L2_PIX_FMT_BGR24 ||
> > +	    fourcc == V4L2_PIX_FMT_RGB32 ||
> > +	    fourcc == V4L2_PIX_FMT_BGR32 ||
> > +	    fourcc == V4L2_PIX_FMT_RGB565 ||
> > +	    fourcc == V4L2_PIX_FMT_RGB555)
> > +		return true;
> > +
> > +	return false;
> > +}
> 
> Why not add a 'bool is_rgb' to struct vpe_fmt?
> 
> It is all too easy to forget to update this function if a new RGB format is
> added to the vpe_formats array in the future.

Yeah I debate going that route also.
I can change it easily enough.

Although, depending on the further changes required below this may be moot.
We'll see.

> 
> > +
> > +/*
> > + * This helper is only used to setup the color space converter
> > + * the actual value returned is only to broadly differentiate between
> > + * RGB and YUV
> > + */
> > +static enum  v4l2_colorspace fourcc_to_colorspace(u32 fourcc)
> 
> double space after enum.

Arrg, I'll fix that.

> 
> > +{
> > +	if (is_fmt_rgb(fourcc))
> > +		return V4L2_COLORSPACE_SRGB;
> > +
> > +	return V4L2_COLORSPACE_SMPTE170M;
> > +}
> 
> This is highly confusing. RGB or YUV conversion does not change the colorspace
> at all.
> 

Well I see here that I am missing some understanding.
But as you saw all I am trying to get to is this: do we need to setup YUV
to RGB conversion or not.

Regarding csc_set_coeff() I was not trying to re-write it but use the
pixel_formats as hint. I understand that this might not be as flexible as
it needs to be (meaning handling colorspace, xfer_func, ycbr_encoding or
quantization in a sane way) but to at least get the correct direction of the
conversion. At the moment VPE driver only handles YUV to RGB conversion.


> There are four colorimetry related fields: colorspace, xfer_func, ycbcr_enc and
> quantization. Most hardware (including this one AFAICT) have a 3x3 matrix + a
> vector to do the conversion. This only allows you to convert between different
> ycbcr encodings and quantization ranges. The colorspace and xfer_func parameters
> stay unchanged (you need gamma tables and two other 3x3 matrices for that).
> 
> So if you want to set up the 3x3 matrix + vector correctly, then you need to
> provide the ycbcr_enc value + quantization value of the source to your function.
> ycbcr_enc is only valid of YUV pixelformats, of course, and you can use
> V4L2_MAP_COLORSPACE_DEFAULT, V4L2_MAP_YCBCR_ENC_DEFAULT and V4L2_MAP_QUANTIZATION_DEFAULT
> if one or more of these values as set by userspace are 0.
> 
> Since the V4L2 API does not (yet) support setting ycbcr_enc and quantization for the
> capture queue you have to provide that information yourself:
> 
> For RGB ycbcr_enc is of course ignored, and quantization should be full range.
> For YUV it depends: the quantization is always limited range, but for the ycbcr_enc
> you have a choice: if the source was YUV, then you can decide to just copy the
> ycbcr_enc value. Alternatively, you can convert to 601 for SDTV and 709 for anything
> else. The latter is also the recommended choice if the source was RGB.
> 
> In any case, please do not use enum v4l2_colorspace in the csc_set_coeff function:
> it's misleading.

But I guess I can rewrite the csc_set_coeff() module API to use at least
pixel_format instead of enum_colorspace at least as a first step.

Full color space parameters support (meaning all 4 of them) would probably
be better done as its own future patch series.

Benoit

> 
> https://hverkuil.home.xs4all.nl/spec/uapi/v4l/colorspaces.html and the following two
> sections contain a lot of information about how colorspaces work.
> 
> Regards,
> 
> 	Hans
> 
> > +
> >  /* find our format description corresponding to the passed v4l2_format */
> >  static struct vpe_fmt *__find_format(u32 fourcc)
> >  {
> > @@ -764,11 +790,10 @@ static void set_src_registers(struct vpe_ctx *ctx)
> >  static void set_dst_registers(struct vpe_ctx *ctx)
> >  {
> >  	struct vpe_mmr_adb *mmr_adb = ctx->mmr_adb.addr;
> > -	enum v4l2_colorspace clrspc = ctx->q_data[Q_DATA_DST].colorspace;
> >  	struct vpe_fmt *fmt = ctx->q_data[Q_DATA_DST].fmt;
> >  	u32 val = 0;
> >  
> > -	if (clrspc == V4L2_COLORSPACE_SRGB) {
> > +	if (is_fmt_rgb(fmt->fourcc)) {
> >  		val |= VPE_RGB_OUT_SELECT;
> >  		vpdma_set_bg_color(ctx->dev->vpdma,
> >  			(struct vpdma_data_format *)fmt->vpdma_fmt[0], 0xff);
> > @@ -912,7 +937,8 @@ static int set_srcdst_params(struct vpe_ctx *ctx)
> >  	set_dei_regs(ctx);
> >  
> >  	csc_set_coeff(ctx->dev->csc, &mmr_adb->csc_regs[0],
> > -		s_q_data->colorspace, d_q_data->colorspace);
> > +		      fourcc_to_colorspace(s_q_data->fmt->fourcc),
> > +		      fourcc_to_colorspace(d_q_data->fmt->fourcc));
> >  
> >  	sc_set_hs_coeffs(ctx->dev->sc, ctx->sc_coeff_h.addr, src_w, dst_w);
> >  	sc_set_vs_coeffs(ctx->dev->sc, ctx->sc_coeff_v.addr, src_h, dst_h);
> > @@ -1285,7 +1311,7 @@ static void device_run(void *priv)
> >  	if (ctx->deinterlacing)
> >  		add_out_dtd(ctx, VPE_PORT_MV_OUT);
> >  
> > -	if (d_q_data->colorspace == V4L2_COLORSPACE_SRGB) {
> > +	if (is_fmt_rgb(d_q_data->fmt->fourcc)) {
> >  		add_out_dtd(ctx, VPE_PORT_RGB_OUT);
> >  	} else {
> >  		add_out_dtd(ctx, VPE_PORT_LUMA_OUT);
> > @@ -1327,7 +1353,7 @@ static void device_run(void *priv)
> >  	}
> >  
> >  	/* sync on channel control descriptors for output ports */
> > -	if (d_q_data->colorspace == V4L2_COLORSPACE_SRGB) {
> > +	if (is_fmt_rgb(d_q_data->fmt->fourcc)) {
> >  		vpdma_add_sync_on_channel_ctd(&ctx->desc_list,
> >  			VPE_CHAN_RGB_OUT);
> >  	} else {
> > @@ -1682,10 +1708,7 @@ static int __vpe_try_fmt(struct vpe_ctx *ctx, struct v4l2_format *f,
> >  		height = pix->height;
> >  
> >  	if (!pix->colorspace) {
> > -		if (fmt->fourcc == V4L2_PIX_FMT_RGB24 ||
> > -				fmt->fourcc == V4L2_PIX_FMT_BGR24 ||
> > -				fmt->fourcc == V4L2_PIX_FMT_RGB32 ||
> > -				fmt->fourcc == V4L2_PIX_FMT_BGR32) {
> > +		if (is_fmt_rgb(fmt->fourcc)) {
> >  			pix->colorspace = V4L2_COLORSPACE_SRGB;
> >  		} else {
> >  			if (height > 1280)	/* HD */
> > 
>