Re: [PATCH v11 4/4] media: add Rockchip VPU JPEG encoder driver

[Ezequiel Garcia <ezequiel@xxxxxxxxxxxxx>]

Hi Hans,

On Wed, 2018-12-05 at 16:01 +0100, Hans Verkuil wrote:
> On 11/30/18 18:34, Ezequiel Garcia wrote:
> > Add a mem2mem driver for the VPU available on Rockchip SoCs.
> > Currently only JPEG encoding is supported, for RK3399 and RK3288
> > platforms.
> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel@xxxxxxxxxxxxx>
> > ---
> 
> <snip>
> 
[..]
> 
> 
> Unless something unexpected happens, then v12 should be the final
> version and I'll make a pull request for it. Note that it will
> probably won't make 4.20, unless you manage to do it within the next
> hour :-)
> 

Thanks for the review. Here are the changes that will be on v12.

Besides your feedback, I found a missing parenthesis issue,
which seems to have sneaked into v11! Apparently, v11 had
last minute changes and I failed to run v4l2-compliance.  

diff --git a/drivers/staging/media/rockchip/vpu/rockchip_vpu_drv.c b/drivers/staging/media/rockchip/vpu/rockchip_vpu_drv.c
index f2752a0c71c0..962412c79b91 100644
--- a/drivers/staging/media/rockchip/vpu/rockchip_vpu_drv.c
+++ b/drivers/staging/media/rockchip/vpu/rockchip_vpu_drv.c
@@ -60,11 +60,13 @@ static void rockchip_vpu_job_finish(struct rockchip_vpu_dev *vpu,
 	dst->sequence = ctx->sequence_cap++;
 
 	dst->field = src->field;
-	if (dst->flags & V4L2_BUF_FLAG_TIMECODE)
+	if (src->flags & V4L2_BUF_FLAG_TIMECODE)
 		dst->timecode = src->timecode;
 	dst->vb2_buf.timestamp = src->vb2_buf.timestamp;
-	dst->flags &= ~V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
-	dst->flags |= src->flags & V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
+	dst->flags &= ~(V4L2_BUF_FLAG_TSTAMP_SRC_MASK |
+			V4L2_BUF_FLAG_TIMECODE);
+	dst->flags |= src->flags & (V4L2_BUF_FLAG_TSTAMP_SRC_MASK |
+				    V4L2_BUF_FLAG_TIMECODE);
 
 	avail_size = vb2_plane_size(&dst->vb2_buf, 0) -
 		     ctx->vpu_dst_fmt->header_size;
@@ -151,6 +153,12 @@ enc_queue_init(void *priv, struct vb2_queue *src_vq, struct vb2_queue *dst_vq)
 	src_vq->drv_priv = ctx;
 	src_vq->ops = &rockchip_vpu_enc_queue_ops;
 	src_vq->mem_ops = &vb2_dma_contig_memops;
+
+	/*
+	 * Driver does mostly sequential access, so sacrifice TLB efficiency
+	 * for faster allocation. Also, no CPU access on the source queue,
+	 * so no kernel mapping needed.
+	 */
 	src_vq->dma_attrs = DMA_ATTR_ALLOC_SINGLE_PAGES |
 			    DMA_ATTR_NO_KERNEL_MAPPING;
 	src_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer);
@@ -197,8 +205,6 @@ static int rockchip_vpu_s_ctrl(struct v4l2_ctrl *ctrl)
 		ctx->jpeg_quality = ctrl->val;
 		break;
 	default:
-		vpu_err("Invalid control id = %d, val = %d\n",
-			ctrl->id, ctrl->val);
 		return -EINVAL;
 	}
 
diff --git a/drivers/staging/media/rockchip/vpu/rockchip_vpu_enc.c b/drivers/staging/media/rockchip/vpu/rockchip_vpu_enc.c
index 6aadd194e999..3dbd15d5fabe 100644
--- a/drivers/staging/media/rockchip/vpu/rockchip_vpu_enc.c
+++ b/drivers/staging/media/rockchip/vpu/rockchip_vpu_enc.c
@@ -142,7 +142,7 @@ rockchip_vpu_get_default_fmt(struct rockchip_vpu_ctx *ctx, bool bitstream)
 	formats = dev->variant->enc_fmts;
 	num_fmts = dev->variant->num_enc_fmts;
 	for (i = 0; i < num_fmts; i++) {
-		if (bitstream == formats[i].codec_mode != RK_VPU_MODE_NONE)
+		if (bitstream == (formats[i].codec_mode != RK_VPU_MODE_NONE))
 			return &formats[i];
 	}
 	return NULL;