[PATCH v2 0/8] videobuf2: Replace vb2_find_timestamp() with vb2_find_buffer()

[PATCH v2 0/8] videobuf2: Replace vb2_find_timestamp() with vb2_find_buffer()

[Ezequiel Garcia]

All users of vb2_find_timestamp() combine it with vb2_get_buffer()
to retrieve a videobuf2 buffer, given a u64 timestamp.

Therefore, this series removes vb2_find_timestamp() and instead
introduces a vb2_find_buffer, which is more suitable, making
videobuf2 API slightly cleaner.

Changes from v1:

* Introduce API in its final shape, to make review easier.
* Prefix cedrus_write_ref_buf_addr and move to common cedrus.c

Ezequiel Garcia (8):
  videobuf2: Introduce vb2_find_buffer()
  mediatek: vcodec: Use vb2_find_buffer
  tegra-vde: Use vb2_find_buffer
  vicodec: Use vb2_find_buffer
  hantro: Use vb2_find_buffer
  rkvdec: Use vb2_find_buffer
  cedrus: Use vb2_find_buffer
  videobuf2: Remove vb2_find_timestamp()

 .../media/common/videobuf2/videobuf2-v4l2.c   | 11 +++--
 .../vcodec/vdec/vdec_h264_req_common.c        |  7 ++-
 .../mediatek/vcodec/vdec/vdec_vp8_req_if.c    |  7 ++-
 .../vcodec/vdec/vdec_vp9_req_lat_if.c         |  8 ++--
 .../media/platform/nvidia/tegra-vde/h264.c    |  9 ++--
 .../media/test-drivers/vicodec/vicodec-core.c |  8 +---
 drivers/staging/media/hantro/hantro_drv.c     |  6 +--
 .../staging/media/hantro/hantro_g2_vp9_dec.c  | 10 ++---
 drivers/staging/media/rkvdec/rkvdec-h264.c    | 41 ++++++------------
 drivers/staging/media/rkvdec/rkvdec-vp9.c     | 10 ++---
 drivers/staging/media/sunxi/cedrus/cedrus.h   | 24 ++++++-----
 .../staging/media/sunxi/cedrus/cedrus_h264.c  | 16 +++----
 .../staging/media/sunxi/cedrus/cedrus_h265.c  | 16 +++----
 .../staging/media/sunxi/cedrus/cedrus_mpeg2.c | 28 ++++--------
 .../staging/media/sunxi/cedrus/cedrus_vp8.c   | 43 ++++---------------
 include/media/videobuf2-v4l2.h                | 12 ++----
 16 files changed, 96 insertions(+), 160 deletions(-)

-- 
2.34.3

[PATCH v2 1/8] videobuf2: Introduce vb2_find_buffer()

[Ezequiel Garcia]

From: Ezequiel Garcia <ezequiel@collabora.com>

All users of vb2_find_timestamp() combine it with vb2_get_buffer()
to retrieve a videobuf2 buffer, given a u64 timestamp.

Introduce an API for this use-case. Users will be converted to the new
API as follow-up commits.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
Acked-by: Tomasz Figa <tfiga@chromium.org>
---
 drivers/media/common/videobuf2/videobuf2-v4l2.c | 12 ++++++++++++
 include/media/videobuf2-v4l2.h                  | 10 ++++++++++
 2 files changed, 22 insertions(+)

diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
index 075d24ebf44c..f26cb8586bd4 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -638,6 +638,18 @@ int vb2_find_timestamp(const struct vb2_queue *q, u64 timestamp,
 }
 EXPORT_SYMBOL_GPL(vb2_find_timestamp);
 
+struct vb2_buffer *vb2_find_buffer(struct vb2_queue *q, u64 timestamp)
+{
+	unsigned int i;
+
+	for (i = 0; i < q->num_buffers; i++)
+		if (q->bufs[i]->copied_timestamp &&
+		    q->bufs[i]->timestamp == timestamp)
+			return vb2_get_buffer(q, i);
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(vb2_find_buffer);
+
 /*
  * vb2_querybuf() - query video buffer information
  * @q:		videobuf queue
diff --git a/include/media/videobuf2-v4l2.h b/include/media/videobuf2-v4l2.h
index d818d9707695..76e405c0b003 100644
--- a/include/media/videobuf2-v4l2.h
+++ b/include/media/videobuf2-v4l2.h
@@ -78,6 +78,16 @@ struct vb2_v4l2_buffer {
 int vb2_find_timestamp(const struct vb2_queue *q, u64 timestamp,
 		       unsigned int start_idx);
 
+/**
+ * vb2_find_buffer() - Find a buffer with given timestamp
+ *
+ * @q:		pointer to &struct vb2_queue with videobuf2 queue.
+ * @timestamp:	the timestamp to find.
+ *
+ * Returns the buffer with the given @timestamp, or NULL if not found.
+ */
+struct vb2_buffer *vb2_find_buffer(struct vb2_queue *q, u64 timestamp);
+
 int vb2_querybuf(struct vb2_queue *q, struct v4l2_buffer *b);
 
 /**
-- 
2.34.3

[PATCH v2 2/8] mediatek: vcodec: Use vb2_find_buffer

[Ezequiel Garcia]

Use the newly introduced vb2_find_buffer API to get a vb2_buffer
given a buffer timestamp.

Cc: Tiffany Lin <tiffany.lin@mediatek.com>
Cc: Andrew-CT Chen <andrew-ct.chen@mediatek.com>
Cc: Yunfei Dong <yunfei.dong@mediatek.com>
Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
Acked-by: Tomasz Figa <tfiga@chromium.org>
---
 .../platform/mediatek/vcodec/vdec/vdec_h264_req_common.c  | 7 +++----
 .../media/platform/mediatek/vcodec/vdec/vdec_vp8_req_if.c | 7 +++----
 .../platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c   | 8 ++++----
 3 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/media/platform/mediatek/vcodec/vdec/vdec_h264_req_common.c b/drivers/media/platform/mediatek/vcodec/vdec/vdec_h264_req_common.c
index ca628321d272..580ce979e2a3 100644
--- a/drivers/media/platform/mediatek/vcodec/vdec/vdec_h264_req_common.c
+++ b/drivers/media/platform/mediatek/vcodec/vdec/vdec_h264_req_common.c
@@ -51,7 +51,7 @@ void mtk_vdec_h264_fill_dpb_info(struct mtk_vcodec_ctx *ctx,
 	struct vb2_queue *vq;
 	struct vb2_buffer *vb;
 	struct vb2_v4l2_buffer *vb2_v4l2;
-	int index, vb2_index;
+	int index;
 
 	vq = v4l2_m2m_get_vq(ctx->m2m_ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
 
@@ -62,8 +62,8 @@ void mtk_vdec_h264_fill_dpb_info(struct mtk_vcodec_ctx *ctx,
 			continue;
 		}
 
-		vb2_index = vb2_find_timestamp(vq, dpb->reference_ts, 0);
-		if (vb2_index < 0) {
+		vb = vb2_find_buffer(vq, dpb->reference_ts);
+		if (!vb) {
 			dev_err(&ctx->dev->plat_dev->dev,
 				"Reference invalid: dpb_index(%d) reference_ts(%lld)",
 				index, dpb->reference_ts);
@@ -76,7 +76,6 @@ void mtk_vdec_h264_fill_dpb_info(struct mtk_vcodec_ctx *ctx,
 		else
 			h264_dpb_info[index].reference_flag = 2;
 
-		vb = vq->bufs[vb2_index];
 		vb2_v4l2 = container_of(vb, struct vb2_v4l2_buffer, vb2_buf);
 		h264_dpb_info[index].field = vb2_v4l2->field;
 
diff --git a/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp8_req_if.c b/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp8_req_if.c
index eef102f3f4f3..e1fe2603e92e 100644
--- a/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp8_req_if.c
+++ b/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp8_req_if.c
@@ -237,7 +237,7 @@ static int vdec_vp8_slice_get_decode_parameters(struct vdec_vp8_slice_inst *inst
 	struct vb2_queue *vq;
 	struct vb2_buffer *vb;
 	u64 referenct_ts;
-	int index, vb2_index;
+	int index;
 
 	frame_header = vdec_vp8_slice_get_ctrl_ptr(inst->ctx, V4L2_CID_STATELESS_VP8_FRAME);
 	if (IS_ERR(frame_header))
@@ -246,8 +246,8 @@ static int vdec_vp8_slice_get_decode_parameters(struct vdec_vp8_slice_inst *inst
 	vq = v4l2_m2m_get_vq(ctx->m2m_ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
 	for (index = 0; index < 3; index++) {
 		referenct_ts = vdec_vp8_slice_get_ref_by_ts(frame_header, index);
-		vb2_index = vb2_find_timestamp(vq, referenct_ts, 0);
-		if (vb2_index < 0) {
+		vb = vb2_find_buffer(vq, referenct_ts);
+		if (!vb) {
 			if (!V4L2_VP8_FRAME_IS_KEY_FRAME(frame_header))
 				mtk_vcodec_err(inst, "reference invalid: index(%d) ts(%lld)",
 					       index, referenct_ts);
@@ -256,7 +256,6 @@ static int vdec_vp8_slice_get_decode_parameters(struct vdec_vp8_slice_inst *inst
 		}
 		inst->vsi->vp8_dpb_info[index].reference_flag = 1;
 
-		vb = vq->bufs[vb2_index];
 		inst->vsi->vp8_dpb_info[index].y_dma_addr =
 			vb2_dma_contig_plane_dma_addr(vb, 0);
 		if (ctx->q_data[MTK_Q_DATA_DST].fmt->num_planes == 2)
diff --git a/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c b/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c
index 81de876d5126..fb1c36a3592d 100644
--- a/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c
+++ b/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c
@@ -1672,7 +1672,6 @@ static int vdec_vp9_slice_setup_core_buffer(struct vdec_vp9_slice_instance *inst
 	struct vdec_vp9_slice_reference *ref;
 	int plane;
 	int size;
-	int idx;
 	int w;
 	int h;
 	int i;
@@ -1715,15 +1714,16 @@ static int vdec_vp9_slice_setup_core_buffer(struct vdec_vp9_slice_instance *inst
 	 */
 	for (i = 0; i < 3; i++) {
 		ref = &vsi->frame.ref[i];
-		idx = vb2_find_timestamp(vq, pfc->ref_idx[i], 0);
-		if (idx < 0) {
+		vb = vb2_find_buffer(vq, pfc->ref_idx[i]);
+		if (!vb) {
 			ref->frame_width = w;
 			ref->frame_height = h;
 			memset(&vsi->ref[i], 0, sizeof(vsi->ref[i]));
 		} else {
+			int idx = vb->index;
+
 			ref->frame_width = instance->dpb[idx].width;
 			ref->frame_height = instance->dpb[idx].height;
-			vb = vq->bufs[idx];
 			vsi->ref[i].y.dma_addr =
 				vb2_dma_contig_plane_dma_addr(vb, 0);
 			if (plane == 1)
-- 
2.34.3

[PATCH v2 3/8] tegra-vde: Use vb2_find_buffer

[Ezequiel Garcia]

Use the newly introduced vb2_find_buffer API to get a vb2_buffer
given a buffer timestamp.

Cc: Dmitry Osipenko <digetx@gmail.com>
Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
Acked-by: Tomasz Figa <tfiga@chromium.org>
---
 drivers/media/platform/nvidia/tegra-vde/h264.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/nvidia/tegra-vde/h264.c b/drivers/media/platform/nvidia/tegra-vde/h264.c
index 88f81a134ba0..204e474d57f7 100644
--- a/drivers/media/platform/nvidia/tegra-vde/h264.c
+++ b/drivers/media/platform/nvidia/tegra-vde/h264.c
@@ -659,20 +659,19 @@ static struct vb2_buffer *get_ref_buf(struct tegra_ctx *ctx,
 {
 	const struct v4l2_h264_dpb_entry *dpb = ctx->h264.decode_params->dpb;
 	struct vb2_queue *cap_q = &ctx->fh.m2m_ctx->cap_q_ctx.q;
-	int buf_idx = -1;
+	struct vb2_buffer *vb = NULL;
 
 	if (dpb[dpb_idx].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE)
-		buf_idx = vb2_find_timestamp(cap_q,
-					     dpb[dpb_idx].reference_ts, 0);
+		vb = vb2_find_buffer(cap_q, dpb[dpb_idx].reference_ts);
 
 	/*
 	 * If a DPB entry is unused or invalid, address of current destination
 	 * buffer is returned.
 	 */
-	if (buf_idx < 0)
+	if (!vb)
 		return &dst->vb2_buf;
 
-	return vb2_get_buffer(cap_q, buf_idx);
+	return vb;
 }
 
 static int tegra_vde_validate_vb_size(struct tegra_ctx *ctx,
-- 
2.34.3

[PATCH v2 4/8] vicodec: Use vb2_find_buffer

[Ezequiel Garcia]

Use the newly introduced vb2_find_buffer API to get a vb2_buffer
given a buffer timestamp.

Cc: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
Acked-by: Tomasz Figa <tfiga@chromium.org>
---
 drivers/media/test-drivers/vicodec/vicodec-core.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/media/test-drivers/vicodec/vicodec-core.c b/drivers/media/test-drivers/vicodec/vicodec-core.c
index be43f7d32df9..1d1bee111732 100644
--- a/drivers/media/test-drivers/vicodec/vicodec-core.c
+++ b/drivers/media/test-drivers/vicodec/vicodec-core.c
@@ -280,17 +280,13 @@ static int device_process(struct vicodec_ctx *ctx,
 		 */
 		if (!(ntohl(ctx->state.header.flags) & V4L2_FWHT_FL_I_FRAME)) {
 			struct vb2_buffer *ref_vb2_buf;
-			int ref_buf_idx;
 			struct vb2_queue *vq_cap =
 				v4l2_m2m_get_vq(ctx->fh.m2m_ctx,
 						V4L2_BUF_TYPE_VIDEO_CAPTURE);
 
-			ref_buf_idx = vb2_find_timestamp(vq_cap,
-							 ctx->state.ref_frame_ts, 0);
-			if (ref_buf_idx < 0)
+			ref_vb2_buf = vb2_find_buffer(vq_cap, ctx->state.ref_frame_ts);
+			if (!ref_vb2_buf)
 				return -EINVAL;
-
-			ref_vb2_buf = vq_cap->bufs[ref_buf_idx];
 			if (ref_vb2_buf->state == VB2_BUF_STATE_ERROR)
 				ret = -EINVAL;
 			ctx->state.ref_frame.buf =
-- 
2.34.3

[PATCH v2 5/8] hantro: Use vb2_find_buffer

[Ezequiel Garcia]

Use the newly introduced vb2_find_buffer API to get a vb2_buffer
given a buffer timestamp.

Cc: Philipp Zabel <p.zabel@pengutronix.de>
Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
Acked-by: Tomasz Figa <tfiga@chromium.org>
---
 drivers/staging/media/hantro/hantro_drv.c        |  6 ++----
 drivers/staging/media/hantro/hantro_g2_vp9_dec.c | 10 +++++-----
 2 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
index 01d33dcb0467..8cb5cf53e5e7 100644
--- a/drivers/staging/media/hantro/hantro_drv.c
+++ b/drivers/staging/media/hantro/hantro_drv.c
@@ -47,12 +47,10 @@ dma_addr_t hantro_get_ref(struct hantro_ctx *ctx, u64 ts)
 {
 	struct vb2_queue *q = v4l2_m2m_get_dst_vq(ctx->fh.m2m_ctx);
 	struct vb2_buffer *buf;
-	int index;
 
-	index = vb2_find_timestamp(q, ts, 0);
-	if (index < 0)
+	buf = vb2_find_buffer(q, ts);
+	if (!buf)
 		return 0;
-	buf = vb2_get_buffer(q, index);
 	return hantro_get_dec_buf_addr(ctx, buf);
 }
 
diff --git a/drivers/staging/media/hantro/hantro_g2_vp9_dec.c b/drivers/staging/media/hantro/hantro_g2_vp9_dec.c
index 91c21b634fab..6d452c779633 100644
--- a/drivers/staging/media/hantro/hantro_g2_vp9_dec.c
+++ b/drivers/staging/media/hantro/hantro_g2_vp9_dec.c
@@ -111,17 +111,17 @@ get_ref_buf(struct hantro_ctx *ctx, struct vb2_v4l2_buffer *dst, u64 timestamp)
 {
 	struct v4l2_m2m_ctx *m2m_ctx = ctx->fh.m2m_ctx;
 	struct vb2_queue *cap_q = &m2m_ctx->cap_q_ctx.q;
-	int buf_idx;
+	struct vb2_buffer *buf;
 
 	/*
 	 * If a ref is unused or invalid, address of current destination
 	 * buffer is returned.
 	 */
-	buf_idx = vb2_find_timestamp(cap_q, timestamp, 0);
-	if (buf_idx < 0)
-		return vb2_to_hantro_decoded_buf(&dst->vb2_buf);
+	buf = vb2_find_buffer(cap_q, timestamp);
+	if (!buf)
+		buf = &dst->vb2_buf;
 
-	return vb2_to_hantro_decoded_buf(vb2_get_buffer(cap_q, buf_idx));
+	return vb2_to_hantro_decoded_buf(buf);
 }
 
 static void update_dec_buf_info(struct hantro_decoded_buffer *buf,
-- 
2.34.3

[PATCH v2 6/8] rkvdec: Use vb2_find_buffer

[Ezequiel Garcia]

Use the newly introduced vb2_find_buffer API to get a vb2_buffer
given a buffer timestamp.

Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
Acked-by: Tomasz Figa <tfiga@chromium.org>
---
 drivers/staging/media/rkvdec/rkvdec-h264.c | 41 ++++++++--------------
 drivers/staging/media/rkvdec/rkvdec-vp9.c  | 10 +++---
 2 files changed, 19 insertions(+), 32 deletions(-)

diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
index 2992fb87cf72..4af5a831bde0 100644
--- a/drivers/staging/media/rkvdec/rkvdec-h264.c
+++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
@@ -109,7 +109,7 @@ struct rkvdec_h264_run {
 	const struct v4l2_ctrl_h264_sps *sps;
 	const struct v4l2_ctrl_h264_pps *pps;
 	const struct v4l2_ctrl_h264_scaling_matrix *scaling_matrix;
-	int ref_buf_idx[V4L2_H264_NUM_DPB_ENTRIES];
+	struct vb2_buffer *ref_buf[V4L2_H264_NUM_DPB_ENTRIES];
 };
 
 struct rkvdec_h264_ctx {
@@ -742,17 +742,16 @@ static void lookup_ref_buf_idx(struct rkvdec_ctx *ctx,
 		struct v4l2_m2m_ctx *m2m_ctx = ctx->fh.m2m_ctx;
 		const struct v4l2_h264_dpb_entry *dpb = run->decode_params->dpb;
 		struct vb2_queue *cap_q = &m2m_ctx->cap_q_ctx.q;
-		int buf_idx = -1;
+		struct vb2_buffer *buf = NULL;
 
 		if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE) {
-			buf_idx = vb2_find_timestamp(cap_q,
-						     dpb[i].reference_ts, 0);
-			if (buf_idx < 0)
+			buf = vb2_find_buffer(cap_q, dpb[i].reference_ts);
+			if (!buf)
 				pr_debug("No buffer for reference_ts %llu",
 					 dpb[i].reference_ts);
 		}
 
-		run->ref_buf_idx[i] = buf_idx;
+		run->ref_buf[i] = buf;
 	}
 }
 
@@ -805,7 +804,7 @@ static void assemble_hw_rps(struct rkvdec_ctx *ctx,
 			if (WARN_ON(ref->index >= ARRAY_SIZE(dec_params->dpb)))
 				continue;
 
-			dpb_valid = run->ref_buf_idx[ref->index] >= 0;
+			dpb_valid = run->ref_buf[ref->index] != NULL;
 			bottom = ref->fields == V4L2_H264_BOTTOM_FIELD_REF;
 
 			set_ps_field(hw_rps, DPB_INFO(i, j),
@@ -881,24 +880,6 @@ static const u32 poc_reg_tbl_bottom_field[16] = {
 	RKVDEC_REG_H264_POC_REFER2(1)
 };
 
-static struct vb2_buffer *
-get_ref_buf(struct rkvdec_ctx *ctx, struct rkvdec_h264_run *run,
-	    unsigned int dpb_idx)
-{
-	struct v4l2_m2m_ctx *m2m_ctx = ctx->fh.m2m_ctx;
-	struct vb2_queue *cap_q = &m2m_ctx->cap_q_ctx.q;
-	int buf_idx = run->ref_buf_idx[dpb_idx];
-
-	/*
-	 * If a DPB entry is unused or invalid, address of current destination
-	 * buffer is returned.
-	 */
-	if (buf_idx < 0)
-		return &run->base.bufs.dst->vb2_buf;
-
-	return vb2_get_buffer(cap_q, buf_idx);
-}
-
 static void config_registers(struct rkvdec_ctx *ctx,
 			     struct rkvdec_h264_run *run)
 {
@@ -971,8 +952,14 @@ static void config_registers(struct rkvdec_ctx *ctx,
 
 	/* config ref pic address & poc */
 	for (i = 0; i < ARRAY_SIZE(dec_params->dpb); i++) {
-		struct vb2_buffer *vb_buf = get_ref_buf(ctx, run, i);
-
+		struct vb2_buffer *vb_buf = run->ref_buf[i];
+
+		/*
+		 * If a DPB entry is unused or invalid, address of current destination
+		 * buffer is returned.
+		 */
+		if (!vb_buf)
+			vb_buf = &dst_buf->vb2_buf;
 		refer_addr = vb2_dma_contig_plane_dma_addr(vb_buf, 0);
 
 		if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE)
diff --git a/drivers/staging/media/rkvdec/rkvdec-vp9.c b/drivers/staging/media/rkvdec/rkvdec-vp9.c
index c2f42e76be10..d8c1c0db15c7 100644
--- a/drivers/staging/media/rkvdec/rkvdec-vp9.c
+++ b/drivers/staging/media/rkvdec/rkvdec-vp9.c
@@ -383,17 +383,17 @@ get_ref_buf(struct rkvdec_ctx *ctx, struct vb2_v4l2_buffer *dst, u64 timestamp)
 {
 	struct v4l2_m2m_ctx *m2m_ctx = ctx->fh.m2m_ctx;
 	struct vb2_queue *cap_q = &m2m_ctx->cap_q_ctx.q;
-	int buf_idx;
+	struct vb2_buffer *buf;
 
 	/*
 	 * If a ref is unused or invalid, address of current destination
 	 * buffer is returned.
 	 */
-	buf_idx = vb2_find_timestamp(cap_q, timestamp, 0);
-	if (buf_idx < 0)
-		return vb2_to_rkvdec_decoded_buf(&dst->vb2_buf);
+	buf = vb2_find_buffer(cap_q, timestamp);
+	if (!buf)
+		buf = &dst->vb2_buf;
 
-	return vb2_to_rkvdec_decoded_buf(vb2_get_buffer(cap_q, buf_idx));
+	return vb2_to_rkvdec_decoded_buf(buf);
 }
 
 static dma_addr_t get_mv_base_addr(struct rkvdec_decoded_buffer *buf)
-- 
2.34.3

[PATCH v2 7/8] cedrus: Use vb2_find_buffer

[Ezequiel Garcia]

Use the newly introduced vb2_find_buffer API to get a vb2_buffer
given a buffer timestamp.

Cc: Maxime Ripard <mripard@kernel.org>
Cc: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
Acked-by: Tomasz Figa <tfiga@chromium.org>
---
 drivers/staging/media/sunxi/cedrus/cedrus.h   | 24 ++++++-----
 .../staging/media/sunxi/cedrus/cedrus_h264.c  | 16 +++----
 .../staging/media/sunxi/cedrus/cedrus_h265.c  | 16 +++----
 .../staging/media/sunxi/cedrus/cedrus_mpeg2.c | 28 ++++--------
 .../staging/media/sunxi/cedrus/cedrus_vp8.c   | 43 ++++---------------
 5 files changed, 46 insertions(+), 81 deletions(-)

diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h b/drivers/staging/media/sunxi/cedrus/cedrus.h
index 3bc094eb497f..c054dbe3d3bc 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus.h
+++ b/drivers/staging/media/sunxi/cedrus/cedrus.h
@@ -233,19 +233,23 @@ static inline dma_addr_t cedrus_buf_addr(struct vb2_buffer *buf,
 }
 
 static inline dma_addr_t cedrus_dst_buf_addr(struct cedrus_ctx *ctx,
-					     int index, unsigned int plane)
+					     struct vb2_buffer *buf,
+					     unsigned int plane)
 {
-	struct vb2_buffer *buf = NULL;
-	struct vb2_queue *vq;
-
-	if (index < 0)
-		return 0;
+	return buf ? cedrus_buf_addr(buf, &ctx->dst_fmt, plane) : 0;
+}
 
-	vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE);
-	if (vq)
-		buf = vb2_get_buffer(vq, index);
+static inline void cedrus_write_ref_buf_addr(struct cedrus_ctx *ctx,
+					     struct vb2_queue *q,
+					     u64 timestamp,
+					     u32 luma_reg,
+					     u32 chroma_reg)
+{
+       struct cedrus_dev *dev = ctx->dev;
+       struct vb2_buffer *buf = vb2_find_buffer(q, timestamp);
 
-	return buf ? cedrus_buf_addr(buf, &ctx->dst_fmt, plane) : 0;
+       cedrus_write(dev, luma_reg, cedrus_dst_buf_addr(ctx, buf, 0));
+       cedrus_write(dev, chroma_reg, cedrus_dst_buf_addr(ctx, buf, 1));
 }
 
 static inline struct cedrus_buffer *
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
index d8fb93035470..0559efeac125 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
@@ -111,16 +111,16 @@ static void cedrus_write_frame_list(struct cedrus_ctx *ctx,
 	for (i = 0; i < ARRAY_SIZE(decode->dpb); i++) {
 		const struct v4l2_h264_dpb_entry *dpb = &decode->dpb[i];
 		struct cedrus_buffer *cedrus_buf;
-		int buf_idx;
+		struct vb2_buffer *buf;
 
 		if (!(dpb->flags & V4L2_H264_DPB_ENTRY_FLAG_VALID))
 			continue;
 
-		buf_idx = vb2_find_timestamp(cap_q, dpb->reference_ts, 0);
-		if (buf_idx < 0)
+		buf = vb2_find_buffer(cap_q, dpb->reference_ts);
+		if (!buf)
 			continue;
 
-		cedrus_buf = vb2_to_cedrus_buffer(cap_q->bufs[buf_idx]);
+		cedrus_buf = vb2_to_cedrus_buffer(buf);
 		position = cedrus_buf->codec.h264.position;
 		used_dpbs |= BIT(position);
 
@@ -186,7 +186,7 @@ static void _cedrus_write_ref_list(struct cedrus_ctx *ctx,
 		const struct v4l2_h264_dpb_entry *dpb;
 		const struct cedrus_buffer *cedrus_buf;
 		unsigned int position;
-		int buf_idx;
+		struct vb2_buffer *buf;
 		u8 dpb_idx;
 
 		dpb_idx = ref_list[i].index;
@@ -195,11 +195,11 @@ static void _cedrus_write_ref_list(struct cedrus_ctx *ctx,
 		if (!(dpb->flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE))
 			continue;
 
-		buf_idx = vb2_find_timestamp(cap_q, dpb->reference_ts, 0);
-		if (buf_idx < 0)
+		buf = vb2_find_buffer(cap_q, dpb->reference_ts);
+		if (!buf)
 			continue;
 
-		cedrus_buf = vb2_to_cedrus_buffer(cap_q->bufs[buf_idx]);
+		cedrus_buf = vb2_to_cedrus_buffer(buf);
 		position = cedrus_buf->codec.h264.position;
 
 		sram_array[i] |= position << 1;
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
index 44f385be9f6c..60cc13e4d0a9 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
@@ -102,14 +102,14 @@ static void cedrus_h265_frame_info_write_single(struct cedrus_ctx *ctx,
 						unsigned int index,
 						bool field_pic,
 						u32 pic_order_cnt[],
-						int buffer_index)
+						struct vb2_buffer *buf)
 {
 	struct cedrus_dev *dev = ctx->dev;
-	dma_addr_t dst_luma_addr = cedrus_dst_buf_addr(ctx, buffer_index, 0);
-	dma_addr_t dst_chroma_addr = cedrus_dst_buf_addr(ctx, buffer_index, 1);
+	dma_addr_t dst_luma_addr = cedrus_dst_buf_addr(ctx, buf, 0);
+	dma_addr_t dst_chroma_addr = cedrus_dst_buf_addr(ctx, buf, 1);
 	dma_addr_t mv_col_buf_addr[2] = {
-		cedrus_h265_frame_info_mv_col_buf_addr(ctx, buffer_index, 0),
-		cedrus_h265_frame_info_mv_col_buf_addr(ctx, buffer_index,
+		cedrus_h265_frame_info_mv_col_buf_addr(ctx, buf->index, 0),
+		cedrus_h265_frame_info_mv_col_buf_addr(ctx, buf->index,
 						       field_pic ? 1 : 0)
 	};
 	u32 offset = VE_DEC_H265_SRAM_OFFSET_FRAME_INFO +
@@ -141,7 +141,7 @@ static void cedrus_h265_frame_info_write_dpb(struct cedrus_ctx *ctx,
 	unsigned int i;
 
 	for (i = 0; i < num_active_dpb_entries; i++) {
-		int buffer_index = vb2_find_timestamp(vq, dpb[i].timestamp, 0);
+		struct vb2_buffer *buf = vb2_find_buffer(vq, dpb[i].timestamp);
 		u32 pic_order_cnt[2] = {
 			dpb[i].pic_order_cnt[0],
 			dpb[i].pic_order_cnt[1]
@@ -149,7 +149,7 @@ static void cedrus_h265_frame_info_write_dpb(struct cedrus_ctx *ctx,
 
 		cedrus_h265_frame_info_write_single(ctx, i, dpb[i].field_pic,
 						    pic_order_cnt,
-						    buffer_index);
+						    buf);
 	}
 }
 
@@ -616,7 +616,7 @@ static void cedrus_h265_setup(struct cedrus_ctx *ctx,
 	cedrus_h265_frame_info_write_single(ctx, output_pic_list_index,
 					    slice_params->pic_struct != 0,
 					    pic_order_cnt,
-					    run->dst->vb2_buf.index);
+					    &run->dst->vb2_buf);
 
 	cedrus_write(dev, VE_DEC_H265_OUTPUT_FRAME_IDX, output_pic_list_index);
 
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
index 5dad2f296c6d..22d6cae9a710 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
@@ -54,13 +54,9 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run)
 	const struct v4l2_ctrl_mpeg2_picture *pic;
 	const struct v4l2_ctrl_mpeg2_quantisation *quantisation;
 	dma_addr_t src_buf_addr, dst_luma_addr, dst_chroma_addr;
-	dma_addr_t fwd_luma_addr, fwd_chroma_addr;
-	dma_addr_t bwd_luma_addr, bwd_chroma_addr;
 	struct cedrus_dev *dev = ctx->dev;
 	struct vb2_queue *vq;
 	const u8 *matrix;
-	int forward_idx;
-	int backward_idx;
 	unsigned int i;
 	u32 reg;
 
@@ -123,27 +119,19 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run)
 	cedrus_write(dev, VE_DEC_MPEG_PICBOUNDSIZE, reg);
 
 	/* Forward and backward prediction reference buffers. */
-
 	vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE);
 
-	forward_idx = vb2_find_timestamp(vq, pic->forward_ref_ts, 0);
-	fwd_luma_addr = cedrus_dst_buf_addr(ctx, forward_idx, 0);
-	fwd_chroma_addr = cedrus_dst_buf_addr(ctx, forward_idx, 1);
-
-	cedrus_write(dev, VE_DEC_MPEG_FWD_REF_LUMA_ADDR, fwd_luma_addr);
-	cedrus_write(dev, VE_DEC_MPEG_FWD_REF_CHROMA_ADDR, fwd_chroma_addr);
-
-	backward_idx = vb2_find_timestamp(vq, pic->backward_ref_ts, 0);
-	bwd_luma_addr = cedrus_dst_buf_addr(ctx, backward_idx, 0);
-	bwd_chroma_addr = cedrus_dst_buf_addr(ctx, backward_idx, 1);
-
-	cedrus_write(dev, VE_DEC_MPEG_BWD_REF_LUMA_ADDR, bwd_luma_addr);
-	cedrus_write(dev, VE_DEC_MPEG_BWD_REF_CHROMA_ADDR, bwd_chroma_addr);
+	cedrus_write_ref_buf_addr(ctx, vq, pic->forward_ref_ts,
+				  VE_DEC_MPEG_FWD_REF_LUMA_ADDR,
+				  VE_DEC_MPEG_FWD_REF_CHROMA_ADDR);
+	cedrus_write_ref_buf_addr(ctx, vq, pic->backward_ref_ts,
+				  VE_DEC_MPEG_BWD_REF_LUMA_ADDR,
+				  VE_DEC_MPEG_BWD_REF_CHROMA_ADDR);
 
 	/* Destination luma and chroma buffers. */
 
-	dst_luma_addr = cedrus_dst_buf_addr(ctx, run->dst->vb2_buf.index, 0);
-	dst_chroma_addr = cedrus_dst_buf_addr(ctx, run->dst->vb2_buf.index, 1);
+	dst_luma_addr = cedrus_dst_buf_addr(ctx, &run->dst->vb2_buf, 0);
+	dst_chroma_addr = cedrus_dst_buf_addr(ctx, &run->dst->vb2_buf, 1);
 
 	cedrus_write(dev, VE_DEC_MPEG_REC_LUMA, dst_luma_addr);
 	cedrus_write(dev, VE_DEC_MPEG_REC_CHROMA, dst_chroma_addr);
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_vp8.c b/drivers/staging/media/sunxi/cedrus/cedrus_vp8.c
index f4016684b32d..196cf692186d 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_vp8.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_vp8.c
@@ -661,7 +661,6 @@ static void cedrus_vp8_setup(struct cedrus_ctx *ctx,
 	dma_addr_t luma_addr, chroma_addr;
 	dma_addr_t src_buf_addr;
 	int header_size;
-	int qindex;
 	u32 reg;
 
 	cedrus_engine_enable(ctx, CEDRUS_CODEC_VP8);
@@ -805,43 +804,17 @@ static void cedrus_vp8_setup(struct cedrus_ctx *ctx,
 	reg |= VE_VP8_LF_DELTA0(slice->lf.mb_mode_delta[0]);
 	cedrus_write(dev, VE_VP8_MODE_LF_DELTA, reg);
 
-	luma_addr = cedrus_dst_buf_addr(ctx, run->dst->vb2_buf.index, 0);
-	chroma_addr = cedrus_dst_buf_addr(ctx, run->dst->vb2_buf.index, 1);
+	luma_addr = cedrus_dst_buf_addr(ctx, &run->dst->vb2_buf, 0);
+	chroma_addr = cedrus_dst_buf_addr(ctx, &run->dst->vb2_buf, 1);
 	cedrus_write(dev, VE_VP8_REC_LUMA, luma_addr);
 	cedrus_write(dev, VE_VP8_REC_CHROMA, chroma_addr);
 
-	qindex = vb2_find_timestamp(cap_q, slice->last_frame_ts, 0);
-	if (qindex >= 0) {
-		luma_addr = cedrus_dst_buf_addr(ctx, qindex, 0);
-		chroma_addr = cedrus_dst_buf_addr(ctx, qindex, 1);
-		cedrus_write(dev, VE_VP8_FWD_LUMA, luma_addr);
-		cedrus_write(dev, VE_VP8_FWD_CHROMA, chroma_addr);
-	} else {
-		cedrus_write(dev, VE_VP8_FWD_LUMA, 0);
-		cedrus_write(dev, VE_VP8_FWD_CHROMA, 0);
-	}
-
-	qindex = vb2_find_timestamp(cap_q, slice->golden_frame_ts, 0);
-	if (qindex >= 0) {
-		luma_addr = cedrus_dst_buf_addr(ctx, qindex, 0);
-		chroma_addr = cedrus_dst_buf_addr(ctx, qindex, 1);
-		cedrus_write(dev, VE_VP8_BWD_LUMA, luma_addr);
-		cedrus_write(dev, VE_VP8_BWD_CHROMA, chroma_addr);
-	} else {
-		cedrus_write(dev, VE_VP8_BWD_LUMA, 0);
-		cedrus_write(dev, VE_VP8_BWD_CHROMA, 0);
-	}
-
-	qindex = vb2_find_timestamp(cap_q, slice->alt_frame_ts, 0);
-	if (qindex >= 0) {
-		luma_addr = cedrus_dst_buf_addr(ctx, qindex, 0);
-		chroma_addr = cedrus_dst_buf_addr(ctx, qindex, 1);
-		cedrus_write(dev, VE_VP8_ALT_LUMA, luma_addr);
-		cedrus_write(dev, VE_VP8_ALT_CHROMA, chroma_addr);
-	} else {
-		cedrus_write(dev, VE_VP8_ALT_LUMA, 0);
-		cedrus_write(dev, VE_VP8_ALT_CHROMA, 0);
-	}
+	cedrus_write_ref_buf_addr(ctx, cap_q, slice->last_frame_ts,
+				  VE_VP8_FWD_LUMA, VE_VP8_FWD_CHROMA);
+	cedrus_write_ref_buf_addr(ctx, cap_q, slice->golden_frame_ts,
+				  VE_VP8_BWD_LUMA, VE_VP8_BWD_CHROMA);
+	cedrus_write_ref_buf_addr(ctx, cap_q, slice->alt_frame_ts,
+				  VE_VP8_ALT_LUMA, VE_VP8_ALT_CHROMA);
 
 	cedrus_write(dev, VE_H264_CTRL, VE_H264_CTRL_VP8 |
 		     VE_H264_CTRL_DECODE_ERR_INT |
-- 
2.34.3

[PATCH v2 8/8] videobuf2: Remove vb2_find_timestamp()

[Ezequiel Garcia]

Now that we've transitioned all users to vb2_find_buffer API,
remove the unused vb2_find_timestamp().

Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
Acked-by: Tomasz Figa <tfiga@chromium.org>
---
 drivers/media/common/videobuf2/videobuf2-v4l2.c | 13 -------------
 include/media/videobuf2-v4l2.h                  | 16 ----------------
 2 files changed, 29 deletions(-)

diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
index f26cb8586bd4..4e84a0e1aca2 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -625,19 +625,6 @@ static const struct vb2_buf_ops v4l2_buf_ops = {
 	.copy_timestamp		= __copy_timestamp,
 };
 
-int vb2_find_timestamp(const struct vb2_queue *q, u64 timestamp,
-		       unsigned int start_idx)
-{
-	unsigned int i;
-
-	for (i = start_idx; i < q->num_buffers; i++)
-		if (q->bufs[i]->copied_timestamp &&
-		    q->bufs[i]->timestamp == timestamp)
-			return i;
-	return -1;
-}
-EXPORT_SYMBOL_GPL(vb2_find_timestamp);
-
 struct vb2_buffer *vb2_find_buffer(struct vb2_queue *q, u64 timestamp)
 {
 	unsigned int i;
diff --git a/include/media/videobuf2-v4l2.h b/include/media/videobuf2-v4l2.h
index 76e405c0b003..5a845887850b 100644
--- a/include/media/videobuf2-v4l2.h
+++ b/include/media/videobuf2-v4l2.h
@@ -62,22 +62,6 @@ struct vb2_v4l2_buffer {
 #define to_vb2_v4l2_buffer(vb) \
 	container_of(vb, struct vb2_v4l2_buffer, vb2_buf)
 
-/**
- * vb2_find_timestamp() - Find buffer with given timestamp in the queue
- *
- * @q:		pointer to &struct vb2_queue with videobuf2 queue.
- * @timestamp:	the timestamp to find.
- * @start_idx:	the start index (usually 0) in the buffer array to start
- *		searching from. Note that there may be multiple buffers
- *		with the same timestamp value, so you can restart the search
- *		by setting @start_idx to the previously found index + 1.
- *
- * Returns the buffer index of the buffer with the given @timestamp, or
- * -1 if no buffer with @timestamp was found.
- */
-int vb2_find_timestamp(const struct vb2_queue *q, u64 timestamp,
-		       unsigned int start_idx);
-
 /**
  * vb2_find_buffer() - Find a buffer with given timestamp
  *
-- 
2.34.3

Re: [PATCH v2 1/8] videobuf2: Introduce vb2_find_buffer()

[Philipp Zabel]

On Mo, 2022-07-11 at 18:11 -0300, Ezequiel Garcia wrote:
> From: Ezequiel Garcia <ezequiel@collabora.com>
> 
> All users of vb2_find_timestamp() combine it with vb2_get_buffer()
> to retrieve a videobuf2 buffer, given a u64 timestamp.
> 
> Introduce an API for this use-case. Users will be converted to the new
> API as follow-up commits.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> Acked-by: Tomasz Figa <tfiga@chromium.org>

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp

Re: [PATCH v2 5/8] hantro: Use vb2_find_buffer

[Philipp Zabel]

On Mo, 2022-07-11 at 18:11 -0300, Ezequiel Garcia wrote:
> Use the newly introduced vb2_find_buffer API to get a vb2_buffer
> given a buffer timestamp.
> 
> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> Acked-by: Tomasz Figa <tfiga@chromium.org>

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp

Re: [PATCH v2 7/8] cedrus: Use vb2_find_buffer

[Paul Kocialkowski]

[-- Attachment #1: Type: text/plain, Size: 12242 bytes --]

Hi Ezequiel,

On Mon 11 Jul 22, 18:11, Ezequiel Garcia wrote:
> Use the newly introduced vb2_find_buffer API to get a vb2_buffer
> given a buffer timestamp.

Looks good with the changes requested by Jernej!

Reviewed-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>

As a sidenote for later: maybe we could merge cedrus_dst_buf_addr and
cedrus_buf_addr into one.

Thanks,

Paul

> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> Acked-by: Tomasz Figa <tfiga@chromium.org>
> ---
>  drivers/staging/media/sunxi/cedrus/cedrus.h   | 24 ++++++-----
>  .../staging/media/sunxi/cedrus/cedrus_h264.c  | 16 +++----
>  .../staging/media/sunxi/cedrus/cedrus_h265.c  | 16 +++----
>  .../staging/media/sunxi/cedrus/cedrus_mpeg2.c | 28 ++++--------
>  .../staging/media/sunxi/cedrus/cedrus_vp8.c   | 43 ++++---------------
>  5 files changed, 46 insertions(+), 81 deletions(-)
> 
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h b/drivers/staging/media/sunxi/cedrus/cedrus.h
> index 3bc094eb497f..c054dbe3d3bc 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus.h
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus.h
> @@ -233,19 +233,23 @@ static inline dma_addr_t cedrus_buf_addr(struct vb2_buffer *buf,
>  }
>  
>  static inline dma_addr_t cedrus_dst_buf_addr(struct cedrus_ctx *ctx,
> -					     int index, unsigned int plane)
> +					     struct vb2_buffer *buf,
> +					     unsigned int plane)
>  {
> -	struct vb2_buffer *buf = NULL;
> -	struct vb2_queue *vq;
> -
> -	if (index < 0)
> -		return 0;
> +	return buf ? cedrus_buf_addr(buf, &ctx->dst_fmt, plane) : 0;
> +}
>  
> -	vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE);
> -	if (vq)
> -		buf = vb2_get_buffer(vq, index);
> +static inline void cedrus_write_ref_buf_addr(struct cedrus_ctx *ctx,
> +					     struct vb2_queue *q,
> +					     u64 timestamp,
> +					     u32 luma_reg,
> +					     u32 chroma_reg)
> +{
> +       struct cedrus_dev *dev = ctx->dev;
> +       struct vb2_buffer *buf = vb2_find_buffer(q, timestamp);
>  
> -	return buf ? cedrus_buf_addr(buf, &ctx->dst_fmt, plane) : 0;
> +       cedrus_write(dev, luma_reg, cedrus_dst_buf_addr(ctx, buf, 0));
> +       cedrus_write(dev, chroma_reg, cedrus_dst_buf_addr(ctx, buf, 1));
>  }
>  
>  static inline struct cedrus_buffer *
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> index d8fb93035470..0559efeac125 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> @@ -111,16 +111,16 @@ static void cedrus_write_frame_list(struct cedrus_ctx *ctx,
>  	for (i = 0; i < ARRAY_SIZE(decode->dpb); i++) {
>  		const struct v4l2_h264_dpb_entry *dpb = &decode->dpb[i];
>  		struct cedrus_buffer *cedrus_buf;
> -		int buf_idx;
> +		struct vb2_buffer *buf;
>  
>  		if (!(dpb->flags & V4L2_H264_DPB_ENTRY_FLAG_VALID))
>  			continue;
>  
> -		buf_idx = vb2_find_timestamp(cap_q, dpb->reference_ts, 0);
> -		if (buf_idx < 0)
> +		buf = vb2_find_buffer(cap_q, dpb->reference_ts);
> +		if (!buf)
>  			continue;
>  
> -		cedrus_buf = vb2_to_cedrus_buffer(cap_q->bufs[buf_idx]);
> +		cedrus_buf = vb2_to_cedrus_buffer(buf);
>  		position = cedrus_buf->codec.h264.position;
>  		used_dpbs |= BIT(position);
>  
> @@ -186,7 +186,7 @@ static void _cedrus_write_ref_list(struct cedrus_ctx *ctx,
>  		const struct v4l2_h264_dpb_entry *dpb;
>  		const struct cedrus_buffer *cedrus_buf;
>  		unsigned int position;
> -		int buf_idx;
> +		struct vb2_buffer *buf;
>  		u8 dpb_idx;
>  
>  		dpb_idx = ref_list[i].index;
> @@ -195,11 +195,11 @@ static void _cedrus_write_ref_list(struct cedrus_ctx *ctx,
>  		if (!(dpb->flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE))
>  			continue;
>  
> -		buf_idx = vb2_find_timestamp(cap_q, dpb->reference_ts, 0);
> -		if (buf_idx < 0)
> +		buf = vb2_find_buffer(cap_q, dpb->reference_ts);
> +		if (!buf)
>  			continue;
>  
> -		cedrus_buf = vb2_to_cedrus_buffer(cap_q->bufs[buf_idx]);
> +		cedrus_buf = vb2_to_cedrus_buffer(buf);
>  		position = cedrus_buf->codec.h264.position;
>  
>  		sram_array[i] |= position << 1;
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> index 44f385be9f6c..60cc13e4d0a9 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> @@ -102,14 +102,14 @@ static void cedrus_h265_frame_info_write_single(struct cedrus_ctx *ctx,
>  						unsigned int index,
>  						bool field_pic,
>  						u32 pic_order_cnt[],
> -						int buffer_index)
> +						struct vb2_buffer *buf)
>  {
>  	struct cedrus_dev *dev = ctx->dev;
> -	dma_addr_t dst_luma_addr = cedrus_dst_buf_addr(ctx, buffer_index, 0);
> -	dma_addr_t dst_chroma_addr = cedrus_dst_buf_addr(ctx, buffer_index, 1);
> +	dma_addr_t dst_luma_addr = cedrus_dst_buf_addr(ctx, buf, 0);
> +	dma_addr_t dst_chroma_addr = cedrus_dst_buf_addr(ctx, buf, 1);
>  	dma_addr_t mv_col_buf_addr[2] = {
> -		cedrus_h265_frame_info_mv_col_buf_addr(ctx, buffer_index, 0),
> -		cedrus_h265_frame_info_mv_col_buf_addr(ctx, buffer_index,
> +		cedrus_h265_frame_info_mv_col_buf_addr(ctx, buf->index, 0),
> +		cedrus_h265_frame_info_mv_col_buf_addr(ctx, buf->index,
>  						       field_pic ? 1 : 0)
>  	};
>  	u32 offset = VE_DEC_H265_SRAM_OFFSET_FRAME_INFO +
> @@ -141,7 +141,7 @@ static void cedrus_h265_frame_info_write_dpb(struct cedrus_ctx *ctx,
>  	unsigned int i;
>  
>  	for (i = 0; i < num_active_dpb_entries; i++) {
> -		int buffer_index = vb2_find_timestamp(vq, dpb[i].timestamp, 0);
> +		struct vb2_buffer *buf = vb2_find_buffer(vq, dpb[i].timestamp);
>  		u32 pic_order_cnt[2] = {
>  			dpb[i].pic_order_cnt[0],
>  			dpb[i].pic_order_cnt[1]
> @@ -149,7 +149,7 @@ static void cedrus_h265_frame_info_write_dpb(struct cedrus_ctx *ctx,
>  
>  		cedrus_h265_frame_info_write_single(ctx, i, dpb[i].field_pic,
>  						    pic_order_cnt,
> -						    buffer_index);
> +						    buf);
>  	}
>  }
>  
> @@ -616,7 +616,7 @@ static void cedrus_h265_setup(struct cedrus_ctx *ctx,
>  	cedrus_h265_frame_info_write_single(ctx, output_pic_list_index,
>  					    slice_params->pic_struct != 0,
>  					    pic_order_cnt,
> -					    run->dst->vb2_buf.index);
> +					    &run->dst->vb2_buf);
>  
>  	cedrus_write(dev, VE_DEC_H265_OUTPUT_FRAME_IDX, output_pic_list_index);
>  
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> index 5dad2f296c6d..22d6cae9a710 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> @@ -54,13 +54,9 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run)
>  	const struct v4l2_ctrl_mpeg2_picture *pic;
>  	const struct v4l2_ctrl_mpeg2_quantisation *quantisation;
>  	dma_addr_t src_buf_addr, dst_luma_addr, dst_chroma_addr;
> -	dma_addr_t fwd_luma_addr, fwd_chroma_addr;
> -	dma_addr_t bwd_luma_addr, bwd_chroma_addr;
>  	struct cedrus_dev *dev = ctx->dev;
>  	struct vb2_queue *vq;
>  	const u8 *matrix;
> -	int forward_idx;
> -	int backward_idx;
>  	unsigned int i;
>  	u32 reg;
>  
> @@ -123,27 +119,19 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run)
>  	cedrus_write(dev, VE_DEC_MPEG_PICBOUNDSIZE, reg);
>  
>  	/* Forward and backward prediction reference buffers. */
> -
>  	vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE);
>  
> -	forward_idx = vb2_find_timestamp(vq, pic->forward_ref_ts, 0);
> -	fwd_luma_addr = cedrus_dst_buf_addr(ctx, forward_idx, 0);
> -	fwd_chroma_addr = cedrus_dst_buf_addr(ctx, forward_idx, 1);
> -
> -	cedrus_write(dev, VE_DEC_MPEG_FWD_REF_LUMA_ADDR, fwd_luma_addr);
> -	cedrus_write(dev, VE_DEC_MPEG_FWD_REF_CHROMA_ADDR, fwd_chroma_addr);
> -
> -	backward_idx = vb2_find_timestamp(vq, pic->backward_ref_ts, 0);
> -	bwd_luma_addr = cedrus_dst_buf_addr(ctx, backward_idx, 0);
> -	bwd_chroma_addr = cedrus_dst_buf_addr(ctx, backward_idx, 1);
> -
> -	cedrus_write(dev, VE_DEC_MPEG_BWD_REF_LUMA_ADDR, bwd_luma_addr);
> -	cedrus_write(dev, VE_DEC_MPEG_BWD_REF_CHROMA_ADDR, bwd_chroma_addr);
> +	cedrus_write_ref_buf_addr(ctx, vq, pic->forward_ref_ts,
> +				  VE_DEC_MPEG_FWD_REF_LUMA_ADDR,
> +				  VE_DEC_MPEG_FWD_REF_CHROMA_ADDR);
> +	cedrus_write_ref_buf_addr(ctx, vq, pic->backward_ref_ts,
> +				  VE_DEC_MPEG_BWD_REF_LUMA_ADDR,
> +				  VE_DEC_MPEG_BWD_REF_CHROMA_ADDR);
>  
>  	/* Destination luma and chroma buffers. */
>  
> -	dst_luma_addr = cedrus_dst_buf_addr(ctx, run->dst->vb2_buf.index, 0);
> -	dst_chroma_addr = cedrus_dst_buf_addr(ctx, run->dst->vb2_buf.index, 1);
> +	dst_luma_addr = cedrus_dst_buf_addr(ctx, &run->dst->vb2_buf, 0);
> +	dst_chroma_addr = cedrus_dst_buf_addr(ctx, &run->dst->vb2_buf, 1);
>  
>  	cedrus_write(dev, VE_DEC_MPEG_REC_LUMA, dst_luma_addr);
>  	cedrus_write(dev, VE_DEC_MPEG_REC_CHROMA, dst_chroma_addr);
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_vp8.c b/drivers/staging/media/sunxi/cedrus/cedrus_vp8.c
> index f4016684b32d..196cf692186d 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_vp8.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_vp8.c
> @@ -661,7 +661,6 @@ static void cedrus_vp8_setup(struct cedrus_ctx *ctx,
>  	dma_addr_t luma_addr, chroma_addr;
>  	dma_addr_t src_buf_addr;
>  	int header_size;
> -	int qindex;
>  	u32 reg;
>  
>  	cedrus_engine_enable(ctx, CEDRUS_CODEC_VP8);
> @@ -805,43 +804,17 @@ static void cedrus_vp8_setup(struct cedrus_ctx *ctx,
>  	reg |= VE_VP8_LF_DELTA0(slice->lf.mb_mode_delta[0]);
>  	cedrus_write(dev, VE_VP8_MODE_LF_DELTA, reg);
>  
> -	luma_addr = cedrus_dst_buf_addr(ctx, run->dst->vb2_buf.index, 0);
> -	chroma_addr = cedrus_dst_buf_addr(ctx, run->dst->vb2_buf.index, 1);
> +	luma_addr = cedrus_dst_buf_addr(ctx, &run->dst->vb2_buf, 0);
> +	chroma_addr = cedrus_dst_buf_addr(ctx, &run->dst->vb2_buf, 1);
>  	cedrus_write(dev, VE_VP8_REC_LUMA, luma_addr);
>  	cedrus_write(dev, VE_VP8_REC_CHROMA, chroma_addr);
>  
> -	qindex = vb2_find_timestamp(cap_q, slice->last_frame_ts, 0);
> -	if (qindex >= 0) {
> -		luma_addr = cedrus_dst_buf_addr(ctx, qindex, 0);
> -		chroma_addr = cedrus_dst_buf_addr(ctx, qindex, 1);
> -		cedrus_write(dev, VE_VP8_FWD_LUMA, luma_addr);
> -		cedrus_write(dev, VE_VP8_FWD_CHROMA, chroma_addr);
> -	} else {
> -		cedrus_write(dev, VE_VP8_FWD_LUMA, 0);
> -		cedrus_write(dev, VE_VP8_FWD_CHROMA, 0);
> -	}
> -
> -	qindex = vb2_find_timestamp(cap_q, slice->golden_frame_ts, 0);
> -	if (qindex >= 0) {
> -		luma_addr = cedrus_dst_buf_addr(ctx, qindex, 0);
> -		chroma_addr = cedrus_dst_buf_addr(ctx, qindex, 1);
> -		cedrus_write(dev, VE_VP8_BWD_LUMA, luma_addr);
> -		cedrus_write(dev, VE_VP8_BWD_CHROMA, chroma_addr);
> -	} else {
> -		cedrus_write(dev, VE_VP8_BWD_LUMA, 0);
> -		cedrus_write(dev, VE_VP8_BWD_CHROMA, 0);
> -	}
> -
> -	qindex = vb2_find_timestamp(cap_q, slice->alt_frame_ts, 0);
> -	if (qindex >= 0) {
> -		luma_addr = cedrus_dst_buf_addr(ctx, qindex, 0);
> -		chroma_addr = cedrus_dst_buf_addr(ctx, qindex, 1);
> -		cedrus_write(dev, VE_VP8_ALT_LUMA, luma_addr);
> -		cedrus_write(dev, VE_VP8_ALT_CHROMA, chroma_addr);
> -	} else {
> -		cedrus_write(dev, VE_VP8_ALT_LUMA, 0);
> -		cedrus_write(dev, VE_VP8_ALT_CHROMA, 0);
> -	}
> +	cedrus_write_ref_buf_addr(ctx, cap_q, slice->last_frame_ts,
> +				  VE_VP8_FWD_LUMA, VE_VP8_FWD_CHROMA);
> +	cedrus_write_ref_buf_addr(ctx, cap_q, slice->golden_frame_ts,
> +				  VE_VP8_BWD_LUMA, VE_VP8_BWD_CHROMA);
> +	cedrus_write_ref_buf_addr(ctx, cap_q, slice->alt_frame_ts,
> +				  VE_VP8_ALT_LUMA, VE_VP8_ALT_CHROMA);
>  
>  	cedrus_write(dev, VE_H264_CTRL, VE_H264_CTRL_VP8 |
>  		     VE_H264_CTRL_DECODE_ERR_INT |
> -- 
> 2.34.3
> 

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 3/8] tegra-vde: Use vb2_find_buffer

[Dmitry Osipenko]

12.07.2022 00:11, Ezequiel Garcia пишет:
> Use the newly introduced vb2_find_buffer API to get a vb2_buffer
> given a buffer timestamp.
> 
> Cc: Dmitry Osipenko <digetx@gmail.com>
> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> Acked-by: Tomasz Figa <tfiga@chromium.org>
> ---
>  drivers/media/platform/nvidia/tegra-vde/h264.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/platform/nvidia/tegra-vde/h264.c b/drivers/media/platform/nvidia/tegra-vde/h264.c
> index 88f81a134ba0..204e474d57f7 100644
> --- a/drivers/media/platform/nvidia/tegra-vde/h264.c
> +++ b/drivers/media/platform/nvidia/tegra-vde/h264.c
> @@ -659,20 +659,19 @@ static struct vb2_buffer *get_ref_buf(struct tegra_ctx *ctx,
>  {
>  	const struct v4l2_h264_dpb_entry *dpb = ctx->h264.decode_params->dpb;
>  	struct vb2_queue *cap_q = &ctx->fh.m2m_ctx->cap_q_ctx.q;
> -	int buf_idx = -1;
> +	struct vb2_buffer *vb = NULL;
>  
>  	if (dpb[dpb_idx].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE)
> -		buf_idx = vb2_find_timestamp(cap_q,
> -					     dpb[dpb_idx].reference_ts, 0);
> +		vb = vb2_find_buffer(cap_q, dpb[dpb_idx].reference_ts);
>  
>  	/*
>  	 * If a DPB entry is unused or invalid, address of current destination
>  	 * buffer is returned.
>  	 */
> -	if (buf_idx < 0)
> +	if (!vb)
>  		return &dst->vb2_buf;
>  
> -	return vb2_get_buffer(cap_q, buf_idx);
> +	return vb;
>  }
>  
>  static int tegra_vde_validate_vb_size(struct tegra_ctx *ctx,

Acked-by: Dmitry Osipenko <digetx@gmail.com>

Re: [PATCH v2 7/8] cedrus: Use vb2_find_buffer

[Nicolas Dufresne]

Hi Ezequiel,

I started testing with these patches and found some NULL dreferences, see my
comment inline...

Le lundi 11 juillet 2022 à 18:11 -0300, Ezequiel Garcia a écrit :
> Use the newly introduced vb2_find_buffer API to get a vb2_buffer
> given a buffer timestamp.
> 
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> Acked-by: Tomasz Figa <tfiga@chromium.org>
> ---
>  drivers/staging/media/sunxi/cedrus/cedrus.h   | 24 ++++++-----
>  .../staging/media/sunxi/cedrus/cedrus_h264.c  | 16 +++----
>  .../staging/media/sunxi/cedrus/cedrus_h265.c  | 16 +++----
>  .../staging/media/sunxi/cedrus/cedrus_mpeg2.c | 28 ++++--------
>  .../staging/media/sunxi/cedrus/cedrus_vp8.c   | 43 ++++---------------
>  5 files changed, 46 insertions(+), 81 deletions(-)
> 
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h b/drivers/staging/media/sunxi/cedrus/cedrus.h
> index 3bc094eb497f..c054dbe3d3bc 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus.h
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus.h
> @@ -233,19 +233,23 @@ static inline dma_addr_t cedrus_buf_addr(struct vb2_buffer *buf,
>  }
>  
>  static inline dma_addr_t cedrus_dst_buf_addr(struct cedrus_ctx *ctx,
> -					     int index, unsigned int plane)
> +					     struct vb2_buffer *buf,
> +					     unsigned int plane)
>  {
> -	struct vb2_buffer *buf = NULL;
> -	struct vb2_queue *vq;
> -
> -	if (index < 0)
> -		return 0;
> +	return buf ? cedrus_buf_addr(buf, &ctx->dst_fmt, plane) : 0;
> +}
>  
> -	vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE);
> -	if (vq)
> -		buf = vb2_get_buffer(vq, index);
> +static inline void cedrus_write_ref_buf_addr(struct cedrus_ctx *ctx,
> +					     struct vb2_queue *q,
> +					     u64 timestamp,
> +					     u32 luma_reg,
> +					     u32 chroma_reg)
> +{
> +       struct cedrus_dev *dev = ctx->dev;
> +       struct vb2_buffer *buf = vb2_find_buffer(q, timestamp);
>  
> -	return buf ? cedrus_buf_addr(buf, &ctx->dst_fmt, plane) : 0;
> +       cedrus_write(dev, luma_reg, cedrus_dst_buf_addr(ctx, buf, 0));
> +       cedrus_write(dev, chroma_reg, cedrus_dst_buf_addr(ctx, buf, 1));
>  }
>  
>  static inline struct cedrus_buffer *
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> index d8fb93035470..0559efeac125 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> @@ -111,16 +111,16 @@ static void cedrus_write_frame_list(struct cedrus_ctx *ctx,
>  	for (i = 0; i < ARRAY_SIZE(decode->dpb); i++) {
>  		const struct v4l2_h264_dpb_entry *dpb = &decode->dpb[i];
>  		struct cedrus_buffer *cedrus_buf;
> -		int buf_idx;
> +		struct vb2_buffer *buf;
>  
>  		if (!(dpb->flags & V4L2_H264_DPB_ENTRY_FLAG_VALID))
>  			continue;
>  
> -		buf_idx = vb2_find_timestamp(cap_q, dpb->reference_ts, 0);
> -		if (buf_idx < 0)
> +		buf = vb2_find_buffer(cap_q, dpb->reference_ts);
> +		if (!buf)
>  			continue;
>  
> -		cedrus_buf = vb2_to_cedrus_buffer(cap_q->bufs[buf_idx]);
> +		cedrus_buf = vb2_to_cedrus_buffer(buf);
>  		position = cedrus_buf->codec.h264.position;
>  		used_dpbs |= BIT(position);
>  
> @@ -186,7 +186,7 @@ static void _cedrus_write_ref_list(struct cedrus_ctx *ctx,
>  		const struct v4l2_h264_dpb_entry *dpb;
>  		const struct cedrus_buffer *cedrus_buf;
>  		unsigned int position;
> -		int buf_idx;
> +		struct vb2_buffer *buf;
>  		u8 dpb_idx;
>  
>  		dpb_idx = ref_list[i].index;
> @@ -195,11 +195,11 @@ static void _cedrus_write_ref_list(struct cedrus_ctx *ctx,
>  		if (!(dpb->flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE))
>  			continue;
>  
> -		buf_idx = vb2_find_timestamp(cap_q, dpb->reference_ts, 0);
> -		if (buf_idx < 0)
> +		buf = vb2_find_buffer(cap_q, dpb->reference_ts);
> +		if (!buf)
>  			continue;
>  
> -		cedrus_buf = vb2_to_cedrus_buffer(cap_q->bufs[buf_idx]);
> +		cedrus_buf = vb2_to_cedrus_buffer(buf);
>  		position = cedrus_buf->codec.h264.position;
>  
>  		sram_array[i] |= position << 1;
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> index 44f385be9f6c..60cc13e4d0a9 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> @@ -102,14 +102,14 @@ static void cedrus_h265_frame_info_write_single(struct cedrus_ctx *ctx,
>  						unsigned int index,
>  						bool field_pic,
>  						u32 pic_order_cnt[],
> -						int buffer_index)
> +						struct vb2_buffer *buf)
>  {
>  	struct cedrus_dev *dev = ctx->dev;
> -	dma_addr_t dst_luma_addr = cedrus_dst_buf_addr(ctx, buffer_index, 0);
> -	dma_addr_t dst_chroma_addr = cedrus_dst_buf_addr(ctx, buffer_index, 1);
> +	dma_addr_t dst_luma_addr = cedrus_dst_buf_addr(ctx, buf, 0);
> +	dma_addr_t dst_chroma_addr = cedrus_dst_buf_addr(ctx, buf, 1);
>  	dma_addr_t mv_col_buf_addr[2] = {
> -		cedrus_h265_frame_info_mv_col_buf_addr(ctx, buffer_index, 0),
> -		cedrus_h265_frame_info_mv_col_buf_addr(ctx, buffer_index,
> +		cedrus_h265_frame_info_mv_col_buf_addr(ctx, buf->index, 0),
> +		cedrus_h265_frame_info_mv_col_buf_addr(ctx, buf->index,
>  						       field_pic ? 1 : 0)

Previously, -1 would be passed to cedrus_h265_frame_info_mv_col_buf_addr(),
which would not find a buffer at that index, and would return 0. Now the code
will crash with a NULL pointer deref.

>  	};
>  	u32 offset = VE_DEC_H265_SRAM_OFFSET_FRAME_INFO +
> @@ -141,7 +141,7 @@ static void cedrus_h265_frame_info_write_dpb(struct cedrus_ctx *ctx,
>  	unsigned int i;
>  
>  	for (i = 0; i < num_active_dpb_entries; i++) {
> -		int buffer_index = vb2_find_timestamp(vq, dpb[i].timestamp, 0);
> +		struct vb2_buffer *buf = vb2_find_buffer(vq, dpb[i].timestamp);
>  		u32 pic_order_cnt[2] = {
>  			dpb[i].pic_order_cnt[0],
>  			dpb[i].pic_order_cnt[1]
> @@ -149,7 +149,7 @@ static void cedrus_h265_frame_info_write_dpb(struct cedrus_ctx *ctx,
>  
>  		cedrus_h265_frame_info_write_single(ctx, i, dpb[i].field_pic,
>  						    pic_order_cnt,
> -						    buffer_index);
> +						    buf);
>  	}
>  }
>  
> @@ -616,7 +616,7 @@ static void cedrus_h265_setup(struct cedrus_ctx *ctx,
>  	cedrus_h265_frame_info_write_single(ctx, output_pic_list_index,
>  					    slice_params->pic_struct != 0,
>  					    pic_order_cnt,
> -					    run->dst->vb2_buf.index);
> +					    &run->dst->vb2_buf);
>  
>  	cedrus_write(dev, VE_DEC_H265_OUTPUT_FRAME_IDX, output_pic_list_index);
>  
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> index 5dad2f296c6d..22d6cae9a710 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> @@ -54,13 +54,9 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run)
>  	const struct v4l2_ctrl_mpeg2_picture *pic;
>  	const struct v4l2_ctrl_mpeg2_quantisation *quantisation;
>  	dma_addr_t src_buf_addr, dst_luma_addr, dst_chroma_addr;
> -	dma_addr_t fwd_luma_addr, fwd_chroma_addr;
> -	dma_addr_t bwd_luma_addr, bwd_chroma_addr;
>  	struct cedrus_dev *dev = ctx->dev;
>  	struct vb2_queue *vq;
>  	const u8 *matrix;
> -	int forward_idx;
> -	int backward_idx;
>  	unsigned int i;
>  	u32 reg;
>  
> @@ -123,27 +119,19 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run)
>  	cedrus_write(dev, VE_DEC_MPEG_PICBOUNDSIZE, reg);
>  
>  	/* Forward and backward prediction reference buffers. */
> -
>  	vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE);
>  
> -	forward_idx = vb2_find_timestamp(vq, pic->forward_ref_ts, 0);
> -	fwd_luma_addr = cedrus_dst_buf_addr(ctx, forward_idx, 0);
> -	fwd_chroma_addr = cedrus_dst_buf_addr(ctx, forward_idx, 1);
> -
> -	cedrus_write(dev, VE_DEC_MPEG_FWD_REF_LUMA_ADDR, fwd_luma_addr);
> -	cedrus_write(dev, VE_DEC_MPEG_FWD_REF_CHROMA_ADDR, fwd_chroma_addr);
> -
> -	backward_idx = vb2_find_timestamp(vq, pic->backward_ref_ts, 0);
> -	bwd_luma_addr = cedrus_dst_buf_addr(ctx, backward_idx, 0);
> -	bwd_chroma_addr = cedrus_dst_buf_addr(ctx, backward_idx, 1);
> -
> -	cedrus_write(dev, VE_DEC_MPEG_BWD_REF_LUMA_ADDR, bwd_luma_addr);
> -	cedrus_write(dev, VE_DEC_MPEG_BWD_REF_CHROMA_ADDR, bwd_chroma_addr);
> +	cedrus_write_ref_buf_addr(ctx, vq, pic->forward_ref_ts,
> +				  VE_DEC_MPEG_FWD_REF_LUMA_ADDR,
> +				  VE_DEC_MPEG_FWD_REF_CHROMA_ADDR);
> +	cedrus_write_ref_buf_addr(ctx, vq, pic->backward_ref_ts,
> +				  VE_DEC_MPEG_BWD_REF_LUMA_ADDR,
> +				  VE_DEC_MPEG_BWD_REF_CHROMA_ADDR);
>  
>  	/* Destination luma and chroma buffers. */
>  
> -	dst_luma_addr = cedrus_dst_buf_addr(ctx, run->dst->vb2_buf.index, 0);
> -	dst_chroma_addr = cedrus_dst_buf_addr(ctx, run->dst->vb2_buf.index, 1);
> +	dst_luma_addr = cedrus_dst_buf_addr(ctx, &run->dst->vb2_buf, 0);
> +	dst_chroma_addr = cedrus_dst_buf_addr(ctx, &run->dst->vb2_buf, 1);
>  
>  	cedrus_write(dev, VE_DEC_MPEG_REC_LUMA, dst_luma_addr);
>  	cedrus_write(dev, VE_DEC_MPEG_REC_CHROMA, dst_chroma_addr);
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_vp8.c b/drivers/staging/media/sunxi/cedrus/cedrus_vp8.c
> index f4016684b32d..196cf692186d 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_vp8.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_vp8.c
> @@ -661,7 +661,6 @@ static void cedrus_vp8_setup(struct cedrus_ctx *ctx,
>  	dma_addr_t luma_addr, chroma_addr;
>  	dma_addr_t src_buf_addr;
>  	int header_size;
> -	int qindex;
>  	u32 reg;
>  
>  	cedrus_engine_enable(ctx, CEDRUS_CODEC_VP8);
> @@ -805,43 +804,17 @@ static void cedrus_vp8_setup(struct cedrus_ctx *ctx,
>  	reg |= VE_VP8_LF_DELTA0(slice->lf.mb_mode_delta[0]);
>  	cedrus_write(dev, VE_VP8_MODE_LF_DELTA, reg);
>  
> -	luma_addr = cedrus_dst_buf_addr(ctx, run->dst->vb2_buf.index, 0);
> -	chroma_addr = cedrus_dst_buf_addr(ctx, run->dst->vb2_buf.index, 1);
> +	luma_addr = cedrus_dst_buf_addr(ctx, &run->dst->vb2_buf, 0);
> +	chroma_addr = cedrus_dst_buf_addr(ctx, &run->dst->vb2_buf, 1);
>  	cedrus_write(dev, VE_VP8_REC_LUMA, luma_addr);
>  	cedrus_write(dev, VE_VP8_REC_CHROMA, chroma_addr);
>  
> -	qindex = vb2_find_timestamp(cap_q, slice->last_frame_ts, 0);
> -	if (qindex >= 0) {
> -		luma_addr = cedrus_dst_buf_addr(ctx, qindex, 0);
> -		chroma_addr = cedrus_dst_buf_addr(ctx, qindex, 1);
> -		cedrus_write(dev, VE_VP8_FWD_LUMA, luma_addr);
> -		cedrus_write(dev, VE_VP8_FWD_CHROMA, chroma_addr);
> -	} else {
> -		cedrus_write(dev, VE_VP8_FWD_LUMA, 0);
> -		cedrus_write(dev, VE_VP8_FWD_CHROMA, 0);
> -	}
> -
> -	qindex = vb2_find_timestamp(cap_q, slice->golden_frame_ts, 0);
> -	if (qindex >= 0) {
> -		luma_addr = cedrus_dst_buf_addr(ctx, qindex, 0);
> -		chroma_addr = cedrus_dst_buf_addr(ctx, qindex, 1);
> -		cedrus_write(dev, VE_VP8_BWD_LUMA, luma_addr);
> -		cedrus_write(dev, VE_VP8_BWD_CHROMA, chroma_addr);
> -	} else {
> -		cedrus_write(dev, VE_VP8_BWD_LUMA, 0);
> -		cedrus_write(dev, VE_VP8_BWD_CHROMA, 0);
> -	}
> -
> -	qindex = vb2_find_timestamp(cap_q, slice->alt_frame_ts, 0);
> -	if (qindex >= 0) {
> -		luma_addr = cedrus_dst_buf_addr(ctx, qindex, 0);
> -		chroma_addr = cedrus_dst_buf_addr(ctx, qindex, 1);
> -		cedrus_write(dev, VE_VP8_ALT_LUMA, luma_addr);
> -		cedrus_write(dev, VE_VP8_ALT_CHROMA, chroma_addr);
> -	} else {
> -		cedrus_write(dev, VE_VP8_ALT_LUMA, 0);
> -		cedrus_write(dev, VE_VP8_ALT_CHROMA, 0);
> -	}
> +	cedrus_write_ref_buf_addr(ctx, cap_q, slice->last_frame_ts,
> +				  VE_VP8_FWD_LUMA, VE_VP8_FWD_CHROMA);
> +	cedrus_write_ref_buf_addr(ctx, cap_q, slice->golden_frame_ts,
> +				  VE_VP8_BWD_LUMA, VE_VP8_BWD_CHROMA);
> +	cedrus_write_ref_buf_addr(ctx, cap_q, slice->alt_frame_ts,
> +				  VE_VP8_ALT_LUMA, VE_VP8_ALT_CHROMA);
>  
>  	cedrus_write(dev, VE_H264_CTRL, VE_H264_CTRL_VP8 |
>  		     VE_H264_CTRL_DECODE_ERR_INT |

Re: [PATCH v2 7/8] cedrus: Use vb2_find_buffer

[Ezequiel Garcia]

Hi Nicolas,

Thanks a lot for the test and the bug report.

On Thu, Jul 14, 2022 at 4:26 PM Nicolas Dufresne <nicolas@ndufresne.ca> wrote:
>
> Hi Ezequiel,
>
> I started testing with these patches and found some NULL dreferences, see my
> comment inline...
>
> Le lundi 11 juillet 2022 à 18:11 -0300, Ezequiel Garcia a écrit :
> > Use the newly introduced vb2_find_buffer API to get a vb2_buffer
> > given a buffer timestamp.
> >
> > Cc: Maxime Ripard <mripard@kernel.org>
> > Cc: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
> > Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> > Acked-by: Tomasz Figa <tfiga@chromium.org>
> > ---
> >  drivers/staging/media/sunxi/cedrus/cedrus.h   | 24 ++++++-----
> >  .../staging/media/sunxi/cedrus/cedrus_h264.c  | 16 +++----
> >  .../staging/media/sunxi/cedrus/cedrus_h265.c  | 16 +++----
> >  .../staging/media/sunxi/cedrus/cedrus_mpeg2.c | 28 ++++--------
> >  .../staging/media/sunxi/cedrus/cedrus_vp8.c   | 43 ++++---------------
> >  5 files changed, 46 insertions(+), 81 deletions(-)
> >
> > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h b/drivers/staging/media/sunxi/cedrus/cedrus.h
> > index 3bc094eb497f..c054dbe3d3bc 100644
> > --- a/drivers/staging/media/sunxi/cedrus/cedrus.h
> > +++ b/drivers/staging/media/sunxi/cedrus/cedrus.h
> > @@ -233,19 +233,23 @@ static inline dma_addr_t cedrus_buf_addr(struct vb2_buffer *buf,
> >  }
> >
> >  static inline dma_addr_t cedrus_dst_buf_addr(struct cedrus_ctx *ctx,
> > -                                          int index, unsigned int plane)
> > +                                          struct vb2_buffer *buf,
> > +                                          unsigned int plane)
> >  {
> > -     struct vb2_buffer *buf = NULL;
> > -     struct vb2_queue *vq;
> > -
> > -     if (index < 0)
> > -             return 0;
> > +     return buf ? cedrus_buf_addr(buf, &ctx->dst_fmt, plane) : 0;
> > +}
> >
> > -     vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE);
> > -     if (vq)
> > -             buf = vb2_get_buffer(vq, index);
> > +static inline void cedrus_write_ref_buf_addr(struct cedrus_ctx *ctx,
> > +                                          struct vb2_queue *q,
> > +                                          u64 timestamp,
> > +                                          u32 luma_reg,
> > +                                          u32 chroma_reg)
> > +{
> > +       struct cedrus_dev *dev = ctx->dev;
> > +       struct vb2_buffer *buf = vb2_find_buffer(q, timestamp);
> >
> > -     return buf ? cedrus_buf_addr(buf, &ctx->dst_fmt, plane) : 0;
> > +       cedrus_write(dev, luma_reg, cedrus_dst_buf_addr(ctx, buf, 0));
> > +       cedrus_write(dev, chroma_reg, cedrus_dst_buf_addr(ctx, buf, 1));
> >  }
> >
> >  static inline struct cedrus_buffer *
> > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> > index d8fb93035470..0559efeac125 100644
> > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> > @@ -111,16 +111,16 @@ static void cedrus_write_frame_list(struct cedrus_ctx *ctx,
> >       for (i = 0; i < ARRAY_SIZE(decode->dpb); i++) {
> >               const struct v4l2_h264_dpb_entry *dpb = &decode->dpb[i];
> >               struct cedrus_buffer *cedrus_buf;
> > -             int buf_idx;
> > +             struct vb2_buffer *buf;
> >
> >               if (!(dpb->flags & V4L2_H264_DPB_ENTRY_FLAG_VALID))
> >                       continue;
> >
> > -             buf_idx = vb2_find_timestamp(cap_q, dpb->reference_ts, 0);
> > -             if (buf_idx < 0)
> > +             buf = vb2_find_buffer(cap_q, dpb->reference_ts);
> > +             if (!buf)
> >                       continue;
> >
> > -             cedrus_buf = vb2_to_cedrus_buffer(cap_q->bufs[buf_idx]);
> > +             cedrus_buf = vb2_to_cedrus_buffer(buf);
> >               position = cedrus_buf->codec.h264.position;
> >               used_dpbs |= BIT(position);
> >
> > @@ -186,7 +186,7 @@ static void _cedrus_write_ref_list(struct cedrus_ctx *ctx,
> >               const struct v4l2_h264_dpb_entry *dpb;
> >               const struct cedrus_buffer *cedrus_buf;
> >               unsigned int position;
> > -             int buf_idx;
> > +             struct vb2_buffer *buf;
> >               u8 dpb_idx;
> >
> >               dpb_idx = ref_list[i].index;
> > @@ -195,11 +195,11 @@ static void _cedrus_write_ref_list(struct cedrus_ctx *ctx,
> >               if (!(dpb->flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE))
> >                       continue;
> >
> > -             buf_idx = vb2_find_timestamp(cap_q, dpb->reference_ts, 0);
> > -             if (buf_idx < 0)
> > +             buf = vb2_find_buffer(cap_q, dpb->reference_ts);
> > +             if (!buf)
> >                       continue;
> >
> > -             cedrus_buf = vb2_to_cedrus_buffer(cap_q->bufs[buf_idx]);
> > +             cedrus_buf = vb2_to_cedrus_buffer(buf);
> >               position = cedrus_buf->codec.h264.position;
> >
> >               sram_array[i] |= position << 1;
> > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> > index 44f385be9f6c..60cc13e4d0a9 100644
> > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> > @@ -102,14 +102,14 @@ static void cedrus_h265_frame_info_write_single(struct cedrus_ctx *ctx,
> >                                               unsigned int index,
> >                                               bool field_pic,
> >                                               u32 pic_order_cnt[],
> > -                                             int buffer_index)
> > +                                             struct vb2_buffer *buf)
> >  {
> >       struct cedrus_dev *dev = ctx->dev;
> > -     dma_addr_t dst_luma_addr = cedrus_dst_buf_addr(ctx, buffer_index, 0);
> > -     dma_addr_t dst_chroma_addr = cedrus_dst_buf_addr(ctx, buffer_index, 1);
> > +     dma_addr_t dst_luma_addr = cedrus_dst_buf_addr(ctx, buf, 0);
> > +     dma_addr_t dst_chroma_addr = cedrus_dst_buf_addr(ctx, buf, 1);
> >       dma_addr_t mv_col_buf_addr[2] = {
> > -             cedrus_h265_frame_info_mv_col_buf_addr(ctx, buffer_index, 0),
> > -             cedrus_h265_frame_info_mv_col_buf_addr(ctx, buffer_index,
> > +             cedrus_h265_frame_info_mv_col_buf_addr(ctx, buf->index, 0),
> > +             cedrus_h265_frame_info_mv_col_buf_addr(ctx, buf->index,
> >                                                      field_pic ? 1 : 0)
>
> Previously, -1 would be passed to cedrus_h265_frame_info_mv_col_buf_addr(),
> which would not find a buffer at that index, and would return 0. Now the code
> will crash with a NULL pointer deref.
>

Is it really correct to pass -1 to cedrus_h265_frame_info_mv_col_buf_addr?
It seems it get casted into an unsigned type and then used to calculate
an address for DMA.

static inline dma_addr_t
cedrus_h265_frame_info_mv_col_buf_addr(struct cedrus_ctx *ctx,
                                       unsigned int index, unsigned int field)
{
        return ctx->codec.h265.mv_col_buf_addr + index *
               ctx->codec.h265.mv_col_buf_unit_size +
               field * ctx->codec.h265.mv_col_buf_unit_size / 2;
}

Fixing the driver to go back to the previous behavior is trivial,
but this looks odd.

Jernej, Paul, any thoughts?

Thanks,
Ezequiel

> >       };
> >       u32 offset = VE_DEC_H265_SRAM_OFFSET_FRAME_INFO +
> > @@ -141,7 +141,7 @@ static void cedrus_h265_frame_info_write_dpb(struct cedrus_ctx *ctx,
> >       unsigned int i;
> >
> >       for (i = 0; i < num_active_dpb_entries; i++) {
> > -             int buffer_index = vb2_find_timestamp(vq, dpb[i].timestamp, 0);
> > +             struct vb2_buffer *buf = vb2_find_buffer(vq, dpb[i].timestamp);
> >               u32 pic_order_cnt[2] = {
> >                       dpb[i].pic_order_cnt[0],
> >                       dpb[i].pic_order_cnt[1]
> > @@ -149,7 +149,7 @@ static void cedrus_h265_frame_info_write_dpb(struct cedrus_ctx *ctx,
> >
> >               cedrus_h265_frame_info_write_single(ctx, i, dpb[i].field_pic,
> >                                                   pic_order_cnt,
> > -                                                 buffer_index);
> > +                                                 buf);
> >       }
> >  }
> >
> > @@ -616,7 +616,7 @@ static void cedrus_h265_setup(struct cedrus_ctx *ctx,
> >       cedrus_h265_frame_info_write_single(ctx, output_pic_list_index,
> >                                           slice_params->pic_struct != 0,
> >                                           pic_order_cnt,
> > -                                         run->dst->vb2_buf.index);
> > +                                         &run->dst->vb2_buf);
> >
> >       cedrus_write(dev, VE_DEC_H265_OUTPUT_FRAME_IDX, output_pic_list_index);
> >
> > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> > index 5dad2f296c6d..22d6cae9a710 100644
> > --- a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> > @@ -54,13 +54,9 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run)
> >       const struct v4l2_ctrl_mpeg2_picture *pic;
> >       const struct v4l2_ctrl_mpeg2_quantisation *quantisation;
> >       dma_addr_t src_buf_addr, dst_luma_addr, dst_chroma_addr;
> > -     dma_addr_t fwd_luma_addr, fwd_chroma_addr;
> > -     dma_addr_t bwd_luma_addr, bwd_chroma_addr;
> >       struct cedrus_dev *dev = ctx->dev;
> >       struct vb2_queue *vq;
> >       const u8 *matrix;
> > -     int forward_idx;
> > -     int backward_idx;
> >       unsigned int i;
> >       u32 reg;
> >
> > @@ -123,27 +119,19 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run)
> >       cedrus_write(dev, VE_DEC_MPEG_PICBOUNDSIZE, reg);
> >
> >       /* Forward and backward prediction reference buffers. */
> > -
> >       vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE);
> >
> > -     forward_idx = vb2_find_timestamp(vq, pic->forward_ref_ts, 0);
> > -     fwd_luma_addr = cedrus_dst_buf_addr(ctx, forward_idx, 0);
> > -     fwd_chroma_addr = cedrus_dst_buf_addr(ctx, forward_idx, 1);
> > -
> > -     cedrus_write(dev, VE_DEC_MPEG_FWD_REF_LUMA_ADDR, fwd_luma_addr);
> > -     cedrus_write(dev, VE_DEC_MPEG_FWD_REF_CHROMA_ADDR, fwd_chroma_addr);
> > -
> > -     backward_idx = vb2_find_timestamp(vq, pic->backward_ref_ts, 0);
> > -     bwd_luma_addr = cedrus_dst_buf_addr(ctx, backward_idx, 0);
> > -     bwd_chroma_addr = cedrus_dst_buf_addr(ctx, backward_idx, 1);
> > -
> > -     cedrus_write(dev, VE_DEC_MPEG_BWD_REF_LUMA_ADDR, bwd_luma_addr);
> > -     cedrus_write(dev, VE_DEC_MPEG_BWD_REF_CHROMA_ADDR, bwd_chroma_addr);
> > +     cedrus_write_ref_buf_addr(ctx, vq, pic->forward_ref_ts,
> > +                               VE_DEC_MPEG_FWD_REF_LUMA_ADDR,
> > +                               VE_DEC_MPEG_FWD_REF_CHROMA_ADDR);
> > +     cedrus_write_ref_buf_addr(ctx, vq, pic->backward_ref_ts,
> > +                               VE_DEC_MPEG_BWD_REF_LUMA_ADDR,
> > +                               VE_DEC_MPEG_BWD_REF_CHROMA_ADDR);
> >
> >       /* Destination luma and chroma buffers. */
> >
> > -     dst_luma_addr = cedrus_dst_buf_addr(ctx, run->dst->vb2_buf.index, 0);
> > -     dst_chroma_addr = cedrus_dst_buf_addr(ctx, run->dst->vb2_buf.index, 1);
> > +     dst_luma_addr = cedrus_dst_buf_addr(ctx, &run->dst->vb2_buf, 0);
> > +     dst_chroma_addr = cedrus_dst_buf_addr(ctx, &run->dst->vb2_buf, 1);
> >
> >       cedrus_write(dev, VE_DEC_MPEG_REC_LUMA, dst_luma_addr);
> >       cedrus_write(dev, VE_DEC_MPEG_REC_CHROMA, dst_chroma_addr);
> > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_vp8.c b/drivers/staging/media/sunxi/cedrus/cedrus_vp8.c
> > index f4016684b32d..196cf692186d 100644
> > --- a/drivers/staging/media/sunxi/cedrus/cedrus_vp8.c
> > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_vp8.c
> > @@ -661,7 +661,6 @@ static void cedrus_vp8_setup(struct cedrus_ctx *ctx,
> >       dma_addr_t luma_addr, chroma_addr;
> >       dma_addr_t src_buf_addr;
> >       int header_size;
> > -     int qindex;
> >       u32 reg;
> >
> >       cedrus_engine_enable(ctx, CEDRUS_CODEC_VP8);
> > @@ -805,43 +804,17 @@ static void cedrus_vp8_setup(struct cedrus_ctx *ctx,
> >       reg |= VE_VP8_LF_DELTA0(slice->lf.mb_mode_delta[0]);
> >       cedrus_write(dev, VE_VP8_MODE_LF_DELTA, reg);
> >
> > -     luma_addr = cedrus_dst_buf_addr(ctx, run->dst->vb2_buf.index, 0);
> > -     chroma_addr = cedrus_dst_buf_addr(ctx, run->dst->vb2_buf.index, 1);
> > +     luma_addr = cedrus_dst_buf_addr(ctx, &run->dst->vb2_buf, 0);
> > +     chroma_addr = cedrus_dst_buf_addr(ctx, &run->dst->vb2_buf, 1);
> >       cedrus_write(dev, VE_VP8_REC_LUMA, luma_addr);
> >       cedrus_write(dev, VE_VP8_REC_CHROMA, chroma_addr);
> >
> > -     qindex = vb2_find_timestamp(cap_q, slice->last_frame_ts, 0);
> > -     if (qindex >= 0) {
> > -             luma_addr = cedrus_dst_buf_addr(ctx, qindex, 0);
> > -             chroma_addr = cedrus_dst_buf_addr(ctx, qindex, 1);
> > -             cedrus_write(dev, VE_VP8_FWD_LUMA, luma_addr);
> > -             cedrus_write(dev, VE_VP8_FWD_CHROMA, chroma_addr);
> > -     } else {
> > -             cedrus_write(dev, VE_VP8_FWD_LUMA, 0);
> > -             cedrus_write(dev, VE_VP8_FWD_CHROMA, 0);
> > -     }
> > -
> > -     qindex = vb2_find_timestamp(cap_q, slice->golden_frame_ts, 0);
> > -     if (qindex >= 0) {
> > -             luma_addr = cedrus_dst_buf_addr(ctx, qindex, 0);
> > -             chroma_addr = cedrus_dst_buf_addr(ctx, qindex, 1);
> > -             cedrus_write(dev, VE_VP8_BWD_LUMA, luma_addr);
> > -             cedrus_write(dev, VE_VP8_BWD_CHROMA, chroma_addr);
> > -     } else {
> > -             cedrus_write(dev, VE_VP8_BWD_LUMA, 0);
> > -             cedrus_write(dev, VE_VP8_BWD_CHROMA, 0);
> > -     }
> > -
> > -     qindex = vb2_find_timestamp(cap_q, slice->alt_frame_ts, 0);
> > -     if (qindex >= 0) {
> > -             luma_addr = cedrus_dst_buf_addr(ctx, qindex, 0);
> > -             chroma_addr = cedrus_dst_buf_addr(ctx, qindex, 1);
> > -             cedrus_write(dev, VE_VP8_ALT_LUMA, luma_addr);
> > -             cedrus_write(dev, VE_VP8_ALT_CHROMA, chroma_addr);
> > -     } else {
> > -             cedrus_write(dev, VE_VP8_ALT_LUMA, 0);
> > -             cedrus_write(dev, VE_VP8_ALT_CHROMA, 0);
> > -     }
> > +     cedrus_write_ref_buf_addr(ctx, cap_q, slice->last_frame_ts,
> > +                               VE_VP8_FWD_LUMA, VE_VP8_FWD_CHROMA);
> > +     cedrus_write_ref_buf_addr(ctx, cap_q, slice->golden_frame_ts,
> > +                               VE_VP8_BWD_LUMA, VE_VP8_BWD_CHROMA);
> > +     cedrus_write_ref_buf_addr(ctx, cap_q, slice->alt_frame_ts,
> > +                               VE_VP8_ALT_LUMA, VE_VP8_ALT_CHROMA);
> >
> >       cedrus_write(dev, VE_H264_CTRL, VE_H264_CTRL_VP8 |
> >                    VE_H264_CTRL_DECODE_ERR_INT |
>

Re: [PATCH v2 7/8] cedrus: Use vb2_find_buffer

[Jernej Škrabec]

Dne petek, 15. julij 2022 ob 13:48:34 CEST je Ezequiel Garcia napisal(a):
> Hi Nicolas,
> 
> Thanks a lot for the test and the bug report.
> 
> On Thu, Jul 14, 2022 at 4:26 PM Nicolas Dufresne <nicolas@ndufresne.ca> 
wrote:
> > Hi Ezequiel,
> > 
> > I started testing with these patches and found some NULL dreferences, see
> > my comment inline...
> > 
> > Le lundi 11 juillet 2022 à 18:11 -0300, Ezequiel Garcia a écrit :
> > > Use the newly introduced vb2_find_buffer API to get a vb2_buffer
> > > given a buffer timestamp.
> > > 
> > > Cc: Maxime Ripard <mripard@kernel.org>
> > > Cc: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
> > > Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> > > Acked-by: Tomasz Figa <tfiga@chromium.org>
> > > ---
> > > 
> > >  drivers/staging/media/sunxi/cedrus/cedrus.h   | 24 ++++++-----
> > >  .../staging/media/sunxi/cedrus/cedrus_h264.c  | 16 +++----
> > >  .../staging/media/sunxi/cedrus/cedrus_h265.c  | 16 +++----
> > >  .../staging/media/sunxi/cedrus/cedrus_mpeg2.c | 28 ++++--------
> > >  .../staging/media/sunxi/cedrus/cedrus_vp8.c   | 43 ++++---------------
> > >  5 files changed, 46 insertions(+), 81 deletions(-)
> > > 
> > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h
> > > b/drivers/staging/media/sunxi/cedrus/cedrus.h index
> > > 3bc094eb497f..c054dbe3d3bc 100644
> > > --- a/drivers/staging/media/sunxi/cedrus/cedrus.h
> > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus.h
> > > @@ -233,19 +233,23 @@ static inline dma_addr_t cedrus_buf_addr(struct
> > > vb2_buffer *buf,> > 
> > >  }
> > >  
> > >  static inline dma_addr_t cedrus_dst_buf_addr(struct cedrus_ctx *ctx,
> > > 
> > > -                                          int index, unsigned int
> > > plane)
> > > +                                          struct vb2_buffer *buf,
> > > +                                          unsigned int plane)
> > > 
> > >  {
> > > 
> > > -     struct vb2_buffer *buf = NULL;
> > > -     struct vb2_queue *vq;
> > > -
> > > -     if (index < 0)
> > > -             return 0;
> > > +     return buf ? cedrus_buf_addr(buf, &ctx->dst_fmt, plane) : 0;
> > > +}
> > > 
> > > -     vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx,
> > > V4L2_BUF_TYPE_VIDEO_CAPTURE);
> > > -     if (vq)
> > > -             buf = vb2_get_buffer(vq, index);
> > > +static inline void cedrus_write_ref_buf_addr(struct cedrus_ctx *ctx,
> > > +                                          struct vb2_queue *q,
> > > +                                          u64 timestamp,
> > > +                                          u32 luma_reg,
> > > +                                          u32 chroma_reg)
> > > +{
> > > +       struct cedrus_dev *dev = ctx->dev;
> > > +       struct vb2_buffer *buf = vb2_find_buffer(q, timestamp);
> > > 
> > > -     return buf ? cedrus_buf_addr(buf, &ctx->dst_fmt, plane) : 0;
> > > +       cedrus_write(dev, luma_reg, cedrus_dst_buf_addr(ctx, buf, 0));
> > > +       cedrus_write(dev, chroma_reg, cedrus_dst_buf_addr(ctx, buf, 1));
> > > 
> > >  }
> > >  
> > >  static inline struct cedrus_buffer *
> > > 
> > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> > > b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c index
> > > d8fb93035470..0559efeac125 100644
> > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> > > @@ -111,16 +111,16 @@ static void cedrus_write_frame_list(struct
> > > cedrus_ctx *ctx,> > 
> > >       for (i = 0; i < ARRAY_SIZE(decode->dpb); i++) {
> > >       
> > >               const struct v4l2_h264_dpb_entry *dpb = &decode->dpb[i];
> > >               struct cedrus_buffer *cedrus_buf;
> > > 
> > > -             int buf_idx;
> > > +             struct vb2_buffer *buf;
> > > 
> > >               if (!(dpb->flags & V4L2_H264_DPB_ENTRY_FLAG_VALID))
> > >               
> > >                       continue;
> > > 
> > > -             buf_idx = vb2_find_timestamp(cap_q, dpb->reference_ts, 0);
> > > -             if (buf_idx < 0)
> > > +             buf = vb2_find_buffer(cap_q, dpb->reference_ts);
> > > +             if (!buf)
> > > 
> > >                       continue;
> > > 
> > > -             cedrus_buf = vb2_to_cedrus_buffer(cap_q->bufs[buf_idx]);
> > > +             cedrus_buf = vb2_to_cedrus_buffer(buf);
> > > 
> > >               position = cedrus_buf->codec.h264.position;
> > >               used_dpbs |= BIT(position);
> > > 
> > > @@ -186,7 +186,7 @@ static void _cedrus_write_ref_list(struct cedrus_ctx
> > > *ctx,> > 
> > >               const struct v4l2_h264_dpb_entry *dpb;
> > >               const struct cedrus_buffer *cedrus_buf;
> > >               unsigned int position;
> > > 
> > > -             int buf_idx;
> > > +             struct vb2_buffer *buf;
> > > 
> > >               u8 dpb_idx;
> > >               
> > >               dpb_idx = ref_list[i].index;
> > > 
> > > @@ -195,11 +195,11 @@ static void _cedrus_write_ref_list(struct
> > > cedrus_ctx *ctx,> > 
> > >               if (!(dpb->flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE))
> > >               
> > >                       continue;
> > > 
> > > -             buf_idx = vb2_find_timestamp(cap_q, dpb->reference_ts, 0);
> > > -             if (buf_idx < 0)
> > > +             buf = vb2_find_buffer(cap_q, dpb->reference_ts);
> > > +             if (!buf)
> > > 
> > >                       continue;
> > > 
> > > -             cedrus_buf = vb2_to_cedrus_buffer(cap_q->bufs[buf_idx]);
> > > +             cedrus_buf = vb2_to_cedrus_buffer(buf);
> > > 
> > >               position = cedrus_buf->codec.h264.position;
> > >               
> > >               sram_array[i] |= position << 1;
> > > 
> > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> > > b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c index
> > > 44f385be9f6c..60cc13e4d0a9 100644
> > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> > > @@ -102,14 +102,14 @@ static void
> > > cedrus_h265_frame_info_write_single(struct cedrus_ctx *ctx,> > 
> > >                                               unsigned int index,
> > >                                               bool field_pic,
> > >                                               u32 pic_order_cnt[],
> > > 
> > > -                                             int buffer_index)
> > > +                                             struct vb2_buffer *buf)
> > > 
> > >  {
> > >  
> > >       struct cedrus_dev *dev = ctx->dev;
> > > 
> > > -     dma_addr_t dst_luma_addr = cedrus_dst_buf_addr(ctx, buffer_index,
> > > 0);
> > > -     dma_addr_t dst_chroma_addr = cedrus_dst_buf_addr(ctx,
> > > buffer_index, 1); +     dma_addr_t dst_luma_addr =
> > > cedrus_dst_buf_addr(ctx, buf, 0); +     dma_addr_t dst_chroma_addr =
> > > cedrus_dst_buf_addr(ctx, buf, 1);> > 
> > >       dma_addr_t mv_col_buf_addr[2] = {
> > > 
> > > -             cedrus_h265_frame_info_mv_col_buf_addr(ctx, buffer_index,
> > > 0),
> > > -             cedrus_h265_frame_info_mv_col_buf_addr(ctx, buffer_index,
> > > +             cedrus_h265_frame_info_mv_col_buf_addr(ctx, buf->index,
> > > 0),
> > > +             cedrus_h265_frame_info_mv_col_buf_addr(ctx, buf->index,
> > > 
> > >                                                      field_pic ? 1 : 0)
> > 
> > Previously, -1 would be passed to
> > cedrus_h265_frame_info_mv_col_buf_addr(),
> > which would not find a buffer at that index, and would return 0. Now the
> > code will crash with a NULL pointer deref.
> 
> Is it really correct to pass -1 to cedrus_h265_frame_info_mv_col_buf_addr?
> It seems it get casted into an unsigned type and then used to calculate
> an address for DMA.

I totally agree that this is a latent bug and it should be fixed. H264 checks 
for negative value, but not HEVC. Current code just makes out of bounds read, 
which is tolerated, but yours causes NULL pointer dereference, which is not.

I suggest that following check is added in cedrus_h265_frame_info_write_dpb():

if (buffer_index < 0)
 continue;

I can send it as a fix so it gets backported and you update this patch so above 
if is changed to NULL pointer check.

Do you all agree? Nicolas, do you prefer to send such patch, since you're first 
who noticed something odd with the code?

Best regards,
Jernej

> 
> static inline dma_addr_t
> cedrus_h265_frame_info_mv_col_buf_addr(struct cedrus_ctx *ctx,
>                                        unsigned int index, unsigned int
> field) {
>         return ctx->codec.h265.mv_col_buf_addr + index *
>                ctx->codec.h265.mv_col_buf_unit_size +
>                field * ctx->codec.h265.mv_col_buf_unit_size / 2;
> }
> 
> Fixing the driver to go back to the previous behavior is trivial,
> but this looks odd.
> 
> Jernej, Paul, any thoughts?
> 
> Thanks,
> Ezequiel
> 
> > >       };
> > >       u32 offset = VE_DEC_H265_SRAM_OFFSET_FRAME_INFO +
> > > 
> > > @@ -141,7 +141,7 @@ static void cedrus_h265_frame_info_write_dpb(struct
> > > cedrus_ctx *ctx,> > 
> > >       unsigned int i;
> > >       
> > >       for (i = 0; i < num_active_dpb_entries; i++) {
> > > 
> > > -             int buffer_index = vb2_find_timestamp(vq,
> > > dpb[i].timestamp, 0); +             struct vb2_buffer *buf =
> > > vb2_find_buffer(vq, dpb[i].timestamp);> > 
> > >               u32 pic_order_cnt[2] = {
> > >               
> > >                       dpb[i].pic_order_cnt[0],
> > >                       dpb[i].pic_order_cnt[1]
> > > 
> > > @@ -149,7 +149,7 @@ static void cedrus_h265_frame_info_write_dpb(struct
> > > cedrus_ctx *ctx,> > 
> > >               cedrus_h265_frame_info_write_single(ctx, i,
> > >               dpb[i].field_pic,
> > >               
> > >                                                   pic_order_cnt,
> > > 
> > > -                                                 buffer_index);
> > > +                                                 buf);
> > > 
> > >       }
> > >  
> > >  }
> > > 
> > > @@ -616,7 +616,7 @@ static void cedrus_h265_setup(struct cedrus_ctx
> > > *ctx,
> > > 
> > >       cedrus_h265_frame_info_write_single(ctx, output_pic_list_index,
> > >       
> > >                                           slice_params->pic_struct != 0,
> > >                                           pic_order_cnt,
> > > 
> > > -                                         run->dst->vb2_buf.index);
> > > +                                         &run->dst->vb2_buf);
> > > 
> > >       cedrus_write(dev, VE_DEC_H265_OUTPUT_FRAME_IDX,
> > >       output_pic_list_index);
> > > 
> > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> > > b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c index
> > > 5dad2f296c6d..22d6cae9a710 100644
> > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> > > @@ -54,13 +54,9 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx
> > > *ctx, struct cedrus_run *run)> > 
> > >       const struct v4l2_ctrl_mpeg2_picture *pic;
> > >       const struct v4l2_ctrl_mpeg2_quantisation *quantisation;
> > >       dma_addr_t src_buf_addr, dst_luma_addr, dst_chroma_addr;
> > > 
> > > -     dma_addr_t fwd_luma_addr, fwd_chroma_addr;
> > > -     dma_addr_t bwd_luma_addr, bwd_chroma_addr;
> > > 
> > >       struct cedrus_dev *dev = ctx->dev;
> > >       struct vb2_queue *vq;
> > >       const u8 *matrix;
> > > 
> > > -     int forward_idx;
> > > -     int backward_idx;
> > > 
> > >       unsigned int i;
> > >       u32 reg;
> > > 
> > > @@ -123,27 +119,19 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx
> > > *ctx, struct cedrus_run *run)> > 
> > >       cedrus_write(dev, VE_DEC_MPEG_PICBOUNDSIZE, reg);
> > >       
> > >       /* Forward and backward prediction reference buffers. */
> > > 
> > > -
> > > 
> > >       vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx,
> > >       V4L2_BUF_TYPE_VIDEO_CAPTURE);
> > > 
> > > -     forward_idx = vb2_find_timestamp(vq, pic->forward_ref_ts, 0);
> > > -     fwd_luma_addr = cedrus_dst_buf_addr(ctx, forward_idx, 0);
> > > -     fwd_chroma_addr = cedrus_dst_buf_addr(ctx, forward_idx, 1);
> > > -
> > > -     cedrus_write(dev, VE_DEC_MPEG_FWD_REF_LUMA_ADDR, fwd_luma_addr);
> > > -     cedrus_write(dev, VE_DEC_MPEG_FWD_REF_CHROMA_ADDR,
> > > fwd_chroma_addr);
> > > -
> > > -     backward_idx = vb2_find_timestamp(vq, pic->backward_ref_ts, 0);
> > > -     bwd_luma_addr = cedrus_dst_buf_addr(ctx, backward_idx, 0);
> > > -     bwd_chroma_addr = cedrus_dst_buf_addr(ctx, backward_idx, 1);
> > > -
> > > -     cedrus_write(dev, VE_DEC_MPEG_BWD_REF_LUMA_ADDR, bwd_luma_addr);
> > > -     cedrus_write(dev, VE_DEC_MPEG_BWD_REF_CHROMA_ADDR,
> > > bwd_chroma_addr);
> > > +     cedrus_write_ref_buf_addr(ctx, vq, pic->forward_ref_ts,
> > > +                               VE_DEC_MPEG_FWD_REF_LUMA_ADDR,
> > > +                               VE_DEC_MPEG_FWD_REF_CHROMA_ADDR);
> > > +     cedrus_write_ref_buf_addr(ctx, vq, pic->backward_ref_ts,
> > > +                               VE_DEC_MPEG_BWD_REF_LUMA_ADDR,
> > > +                               VE_DEC_MPEG_BWD_REF_CHROMA_ADDR);
> > > 
> > >       /* Destination luma and chroma buffers. */
> > > 
> > > -     dst_luma_addr = cedrus_dst_buf_addr(ctx, run->dst->vb2_buf.index,
> > > 0);
> > > -     dst_chroma_addr = cedrus_dst_buf_addr(ctx,
> > > run->dst->vb2_buf.index, 1); +     dst_luma_addr =
> > > cedrus_dst_buf_addr(ctx, &run->dst->vb2_buf, 0); +     dst_chroma_addr
> > > = cedrus_dst_buf_addr(ctx, &run->dst->vb2_buf, 1);> > 
> > >       cedrus_write(dev, VE_DEC_MPEG_REC_LUMA, dst_luma_addr);
> > >       cedrus_write(dev, VE_DEC_MPEG_REC_CHROMA, dst_chroma_addr);
> > > 
> > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_vp8.c
> > > b/drivers/staging/media/sunxi/cedrus/cedrus_vp8.c index
> > > f4016684b32d..196cf692186d 100644
> > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_vp8.c
> > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_vp8.c
> > > @@ -661,7 +661,6 @@ static void cedrus_vp8_setup(struct cedrus_ctx *ctx,
> > > 
> > >       dma_addr_t luma_addr, chroma_addr;
> > >       dma_addr_t src_buf_addr;
> > >       int header_size;
> > > 
> > > -     int qindex;
> > > 
> > >       u32 reg;
> > >       
> > >       cedrus_engine_enable(ctx, CEDRUS_CODEC_VP8);
> > > 
> > > @@ -805,43 +804,17 @@ static void cedrus_vp8_setup(struct cedrus_ctx
> > > *ctx,
> > > 
> > >       reg |= VE_VP8_LF_DELTA0(slice->lf.mb_mode_delta[0]);
> > >       cedrus_write(dev, VE_VP8_MODE_LF_DELTA, reg);
> > > 
> > > -     luma_addr = cedrus_dst_buf_addr(ctx, run->dst->vb2_buf.index, 0);
> > > -     chroma_addr = cedrus_dst_buf_addr(ctx, run->dst->vb2_buf.index,
> > > 1);
> > > +     luma_addr = cedrus_dst_buf_addr(ctx, &run->dst->vb2_buf, 0);
> > > +     chroma_addr = cedrus_dst_buf_addr(ctx, &run->dst->vb2_buf, 1);
> > > 
> > >       cedrus_write(dev, VE_VP8_REC_LUMA, luma_addr);
> > >       cedrus_write(dev, VE_VP8_REC_CHROMA, chroma_addr);
> > > 
> > > -     qindex = vb2_find_timestamp(cap_q, slice->last_frame_ts, 0);
> > > -     if (qindex >= 0) {
> > > -             luma_addr = cedrus_dst_buf_addr(ctx, qindex, 0);
> > > -             chroma_addr = cedrus_dst_buf_addr(ctx, qindex, 1);
> > > -             cedrus_write(dev, VE_VP8_FWD_LUMA, luma_addr);
> > > -             cedrus_write(dev, VE_VP8_FWD_CHROMA, chroma_addr);
> > > -     } else {
> > > -             cedrus_write(dev, VE_VP8_FWD_LUMA, 0);
> > > -             cedrus_write(dev, VE_VP8_FWD_CHROMA, 0);
> > > -     }
> > > -
> > > -     qindex = vb2_find_timestamp(cap_q, slice->golden_frame_ts, 0);
> > > -     if (qindex >= 0) {
> > > -             luma_addr = cedrus_dst_buf_addr(ctx, qindex, 0);
> > > -             chroma_addr = cedrus_dst_buf_addr(ctx, qindex, 1);
> > > -             cedrus_write(dev, VE_VP8_BWD_LUMA, luma_addr);
> > > -             cedrus_write(dev, VE_VP8_BWD_CHROMA, chroma_addr);
> > > -     } else {
> > > -             cedrus_write(dev, VE_VP8_BWD_LUMA, 0);
> > > -             cedrus_write(dev, VE_VP8_BWD_CHROMA, 0);
> > > -     }
> > > -
> > > -     qindex = vb2_find_timestamp(cap_q, slice->alt_frame_ts, 0);
> > > -     if (qindex >= 0) {
> > > -             luma_addr = cedrus_dst_buf_addr(ctx, qindex, 0);
> > > -             chroma_addr = cedrus_dst_buf_addr(ctx, qindex, 1);
> > > -             cedrus_write(dev, VE_VP8_ALT_LUMA, luma_addr);
> > > -             cedrus_write(dev, VE_VP8_ALT_CHROMA, chroma_addr);
> > > -     } else {
> > > -             cedrus_write(dev, VE_VP8_ALT_LUMA, 0);
> > > -             cedrus_write(dev, VE_VP8_ALT_CHROMA, 0);
> > > -     }
> > > +     cedrus_write_ref_buf_addr(ctx, cap_q, slice->last_frame_ts,
> > > +                               VE_VP8_FWD_LUMA, VE_VP8_FWD_CHROMA);
> > > +     cedrus_write_ref_buf_addr(ctx, cap_q, slice->golden_frame_ts,
> > > +                               VE_VP8_BWD_LUMA, VE_VP8_BWD_CHROMA);
> > > +     cedrus_write_ref_buf_addr(ctx, cap_q, slice->alt_frame_ts,
> > > +                               VE_VP8_ALT_LUMA, VE_VP8_ALT_CHROMA);
> > > 
> > >       cedrus_write(dev, VE_H264_CTRL, VE_H264_CTRL_VP8 |
> > >       
> > >                    VE_H264_CTRL_DECODE_ERR_INT |

Re: [PATCH v2 7/8] cedrus: Use vb2_find_buffer

[Ezequiel Garcia]

Hi Jernej,

On Sat, Jul 16, 2022 at 9:36 AM Jernej Škrabec <jernej.skrabec@gmail.com> wrote:
>
> Dne petek, 15. julij 2022 ob 13:48:34 CEST je Ezequiel Garcia napisal(a):
> > Hi Nicolas,
> >
> > Thanks a lot for the test and the bug report.
> >
> > On Thu, Jul 14, 2022 at 4:26 PM Nicolas Dufresne <nicolas@ndufresne.ca>
> wrote:
> > > Hi Ezequiel,
> > >
> > > I started testing with these patches and found some NULL dreferences, see
> > > my comment inline...
> > >
> > > Le lundi 11 juillet 2022 à 18:11 -0300, Ezequiel Garcia a écrit :
> > > > Use the newly introduced vb2_find_buffer API to get a vb2_buffer
> > > > given a buffer timestamp.
> > > >
> > > > Cc: Maxime Ripard <mripard@kernel.org>
> > > > Cc: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > > Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
> > > > Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> > > > Acked-by: Tomasz Figa <tfiga@chromium.org>
> > > > ---
> > > >
> > > >  drivers/staging/media/sunxi/cedrus/cedrus.h   | 24 ++++++-----
> > > >  .../staging/media/sunxi/cedrus/cedrus_h264.c  | 16 +++----
> > > >  .../staging/media/sunxi/cedrus/cedrus_h265.c  | 16 +++----
> > > >  .../staging/media/sunxi/cedrus/cedrus_mpeg2.c | 28 ++++--------
> > > >  .../staging/media/sunxi/cedrus/cedrus_vp8.c   | 43 ++++---------------
> > > >  5 files changed, 46 insertions(+), 81 deletions(-)
> > > >
> > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h
> > > > b/drivers/staging/media/sunxi/cedrus/cedrus.h index
> > > > 3bc094eb497f..c054dbe3d3bc 100644
> > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus.h
> > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus.h
> > > > @@ -233,19 +233,23 @@ static inline dma_addr_t cedrus_buf_addr(struct
> > > > vb2_buffer *buf,> >
> > > >  }
> > > >
> > > >  static inline dma_addr_t cedrus_dst_buf_addr(struct cedrus_ctx *ctx,
> > > >
> > > > -                                          int index, unsigned int
> > > > plane)
> > > > +                                          struct vb2_buffer *buf,
> > > > +                                          unsigned int plane)
> > > >
> > > >  {
> > > >
> > > > -     struct vb2_buffer *buf = NULL;
> > > > -     struct vb2_queue *vq;
> > > > -
> > > > -     if (index < 0)
> > > > -             return 0;
> > > > +     return buf ? cedrus_buf_addr(buf, &ctx->dst_fmt, plane) : 0;
> > > > +}
> > > >
> > > > -     vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx,
> > > > V4L2_BUF_TYPE_VIDEO_CAPTURE);
> > > > -     if (vq)
> > > > -             buf = vb2_get_buffer(vq, index);
> > > > +static inline void cedrus_write_ref_buf_addr(struct cedrus_ctx *ctx,
> > > > +                                          struct vb2_queue *q,
> > > > +                                          u64 timestamp,
> > > > +                                          u32 luma_reg,
> > > > +                                          u32 chroma_reg)
> > > > +{
> > > > +       struct cedrus_dev *dev = ctx->dev;
> > > > +       struct vb2_buffer *buf = vb2_find_buffer(q, timestamp);
> > > >
> > > > -     return buf ? cedrus_buf_addr(buf, &ctx->dst_fmt, plane) : 0;
> > > > +       cedrus_write(dev, luma_reg, cedrus_dst_buf_addr(ctx, buf, 0));
> > > > +       cedrus_write(dev, chroma_reg, cedrus_dst_buf_addr(ctx, buf, 1));
> > > >
> > > >  }
> > > >
> > > >  static inline struct cedrus_buffer *
> > > >
> > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> > > > b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c index
> > > > d8fb93035470..0559efeac125 100644
> > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> > > > @@ -111,16 +111,16 @@ static void cedrus_write_frame_list(struct
> > > > cedrus_ctx *ctx,> >
> > > >       for (i = 0; i < ARRAY_SIZE(decode->dpb); i++) {
> > > >
> > > >               const struct v4l2_h264_dpb_entry *dpb = &decode->dpb[i];
> > > >               struct cedrus_buffer *cedrus_buf;
> > > >
> > > > -             int buf_idx;
> > > > +             struct vb2_buffer *buf;
> > > >
> > > >               if (!(dpb->flags & V4L2_H264_DPB_ENTRY_FLAG_VALID))
> > > >
> > > >                       continue;
> > > >
> > > > -             buf_idx = vb2_find_timestamp(cap_q, dpb->reference_ts, 0);
> > > > -             if (buf_idx < 0)
> > > > +             buf = vb2_find_buffer(cap_q, dpb->reference_ts);
> > > > +             if (!buf)
> > > >
> > > >                       continue;
> > > >
> > > > -             cedrus_buf = vb2_to_cedrus_buffer(cap_q->bufs[buf_idx]);
> > > > +             cedrus_buf = vb2_to_cedrus_buffer(buf);
> > > >
> > > >               position = cedrus_buf->codec.h264.position;
> > > >               used_dpbs |= BIT(position);
> > > >
> > > > @@ -186,7 +186,7 @@ static void _cedrus_write_ref_list(struct cedrus_ctx
> > > > *ctx,> >
> > > >               const struct v4l2_h264_dpb_entry *dpb;
> > > >               const struct cedrus_buffer *cedrus_buf;
> > > >               unsigned int position;
> > > >
> > > > -             int buf_idx;
> > > > +             struct vb2_buffer *buf;
> > > >
> > > >               u8 dpb_idx;
> > > >
> > > >               dpb_idx = ref_list[i].index;
> > > >
> > > > @@ -195,11 +195,11 @@ static void _cedrus_write_ref_list(struct
> > > > cedrus_ctx *ctx,> >
> > > >               if (!(dpb->flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE))
> > > >
> > > >                       continue;
> > > >
> > > > -             buf_idx = vb2_find_timestamp(cap_q, dpb->reference_ts, 0);
> > > > -             if (buf_idx < 0)
> > > > +             buf = vb2_find_buffer(cap_q, dpb->reference_ts);
> > > > +             if (!buf)
> > > >
> > > >                       continue;
> > > >
> > > > -             cedrus_buf = vb2_to_cedrus_buffer(cap_q->bufs[buf_idx]);
> > > > +             cedrus_buf = vb2_to_cedrus_buffer(buf);
> > > >
> > > >               position = cedrus_buf->codec.h264.position;
> > > >
> > > >               sram_array[i] |= position << 1;
> > > >
> > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> > > > b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c index
> > > > 44f385be9f6c..60cc13e4d0a9 100644
> > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> > > > @@ -102,14 +102,14 @@ static void
> > > > cedrus_h265_frame_info_write_single(struct cedrus_ctx *ctx,> >
> > > >                                               unsigned int index,
> > > >                                               bool field_pic,
> > > >                                               u32 pic_order_cnt[],
> > > >
> > > > -                                             int buffer_index)
> > > > +                                             struct vb2_buffer *buf)
> > > >
> > > >  {
> > > >
> > > >       struct cedrus_dev *dev = ctx->dev;
> > > >
> > > > -     dma_addr_t dst_luma_addr = cedrus_dst_buf_addr(ctx, buffer_index,
> > > > 0);
> > > > -     dma_addr_t dst_chroma_addr = cedrus_dst_buf_addr(ctx,
> > > > buffer_index, 1); +     dma_addr_t dst_luma_addr =
> > > > cedrus_dst_buf_addr(ctx, buf, 0); +     dma_addr_t dst_chroma_addr =
> > > > cedrus_dst_buf_addr(ctx, buf, 1);> >
> > > >       dma_addr_t mv_col_buf_addr[2] = {
> > > >
> > > > -             cedrus_h265_frame_info_mv_col_buf_addr(ctx, buffer_index,
> > > > 0),
> > > > -             cedrus_h265_frame_info_mv_col_buf_addr(ctx, buffer_index,
> > > > +             cedrus_h265_frame_info_mv_col_buf_addr(ctx, buf->index,
> > > > 0),
> > > > +             cedrus_h265_frame_info_mv_col_buf_addr(ctx, buf->index,
> > > >
> > > >                                                      field_pic ? 1 : 0)
> > >
> > > Previously, -1 would be passed to
> > > cedrus_h265_frame_info_mv_col_buf_addr(),
> > > which would not find a buffer at that index, and would return 0. Now the
> > > code will crash with a NULL pointer deref.
> >
> > Is it really correct to pass -1 to cedrus_h265_frame_info_mv_col_buf_addr?
> > It seems it get casted into an unsigned type and then used to calculate
> > an address for DMA.
>
> I totally agree that this is a latent bug and it should be fixed. H264 checks
> for negative value, but not HEVC. Current code just makes out of bounds read,
> which is tolerated, but yours causes NULL pointer dereference, which is not.
>
> I suggest that following check is added in cedrus_h265_frame_info_write_dpb():
>
> if (buffer_index < 0)
>  continue;
>

Seems reasonable.

> I can send it as a fix so it gets backported and you update this patch so above
> if is changed to NULL pointer check.
>

Yes, I like this approach.

> Do you all agree? Nicolas, do you prefer to send such patch, since you're first
> who noticed something odd with the code?
>

Hans' last PR dropped the cedrus patch, and left vb2_find_timestamp
for the time being (for 5.20). I like the proposal of sending a fix that can
be backported. I can re-work the rest of this series on top.

Thanks,
Ezequiel

> Best regards,
> Jernej
>
> >
> > static inline dma_addr_t
> > cedrus_h265_frame_info_mv_col_buf_addr(struct cedrus_ctx *ctx,
> >                                        unsigned int index, unsigned int
> > field) {
> >         return ctx->codec.h265.mv_col_buf_addr + index *
> >                ctx->codec.h265.mv_col_buf_unit_size +
> >                field * ctx->codec.h265.mv_col_buf_unit_size / 2;
> > }
> >
> > Fixing the driver to go back to the previous behavior is trivial,
> > but this looks odd.
> >
> > Jernej, Paul, any thoughts?
> >
> > Thanks,
> > Ezequiel
> >
> > > >       };
> > > >       u32 offset = VE_DEC_H265_SRAM_OFFSET_FRAME_INFO +
> > > >
> > > > @@ -141,7 +141,7 @@ static void cedrus_h265_frame_info_write_dpb(struct
> > > > cedrus_ctx *ctx,> >
> > > >       unsigned int i;
> > > >
> > > >       for (i = 0; i < num_active_dpb_entries; i++) {
> > > >
> > > > -             int buffer_index = vb2_find_timestamp(vq,
> > > > dpb[i].timestamp, 0); +             struct vb2_buffer *buf =
> > > > vb2_find_buffer(vq, dpb[i].timestamp);> >
> > > >               u32 pic_order_cnt[2] = {
> > > >
> > > >                       dpb[i].pic_order_cnt[0],
> > > >                       dpb[i].pic_order_cnt[1]
> > > >
> > > > @@ -149,7 +149,7 @@ static void cedrus_h265_frame_info_write_dpb(struct
> > > > cedrus_ctx *ctx,> >
> > > >               cedrus_h265_frame_info_write_single(ctx, i,
> > > >               dpb[i].field_pic,
> > > >
> > > >                                                   pic_order_cnt,
> > > >
> > > > -                                                 buffer_index);
> > > > +                                                 buf);
> > > >
> > > >       }
> > > >
> > > >  }
> > > >
> > > > @@ -616,7 +616,7 @@ static void cedrus_h265_setup(struct cedrus_ctx
> > > > *ctx,
> > > >
> > > >       cedrus_h265_frame_info_write_single(ctx, output_pic_list_index,
> > > >
> > > >                                           slice_params->pic_struct != 0,
> > > >                                           pic_order_cnt,
> > > >
> > > > -                                         run->dst->vb2_buf.index);
> > > > +                                         &run->dst->vb2_buf);
> > > >
> > > >       cedrus_write(dev, VE_DEC_H265_OUTPUT_FRAME_IDX,
> > > >       output_pic_list_index);
> > > >
> > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> > > > b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c index
> > > > 5dad2f296c6d..22d6cae9a710 100644
> > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> > > > @@ -54,13 +54,9 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx
> > > > *ctx, struct cedrus_run *run)> >
> > > >       const struct v4l2_ctrl_mpeg2_picture *pic;
> > > >       const struct v4l2_ctrl_mpeg2_quantisation *quantisation;
> > > >       dma_addr_t src_buf_addr, dst_luma_addr, dst_chroma_addr;
> > > >
> > > > -     dma_addr_t fwd_luma_addr, fwd_chroma_addr;
> > > > -     dma_addr_t bwd_luma_addr, bwd_chroma_addr;
> > > >
> > > >       struct cedrus_dev *dev = ctx->dev;
> > > >       struct vb2_queue *vq;
> > > >       const u8 *matrix;
> > > >
> > > > -     int forward_idx;
> > > > -     int backward_idx;
> > > >
> > > >       unsigned int i;
> > > >       u32 reg;
> > > >
> > > > @@ -123,27 +119,19 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx
> > > > *ctx, struct cedrus_run *run)> >
> > > >       cedrus_write(dev, VE_DEC_MPEG_PICBOUNDSIZE, reg);
> > > >
> > > >       /* Forward and backward prediction reference buffers. */
> > > >
> > > > -
> > > >
> > > >       vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx,
> > > >       V4L2_BUF_TYPE_VIDEO_CAPTURE);
> > > >
> > > > -     forward_idx = vb2_find_timestamp(vq, pic->forward_ref_ts, 0);
> > > > -     fwd_luma_addr = cedrus_dst_buf_addr(ctx, forward_idx, 0);
> > > > -     fwd_chroma_addr = cedrus_dst_buf_addr(ctx, forward_idx, 1);
> > > > -
> > > > -     cedrus_write(dev, VE_DEC_MPEG_FWD_REF_LUMA_ADDR, fwd_luma_addr);
> > > > -     cedrus_write(dev, VE_DEC_MPEG_FWD_REF_CHROMA_ADDR,
> > > > fwd_chroma_addr);
> > > > -
> > > > -     backward_idx = vb2_find_timestamp(vq, pic->backward_ref_ts, 0);
> > > > -     bwd_luma_addr = cedrus_dst_buf_addr(ctx, backward_idx, 0);
> > > > -     bwd_chroma_addr = cedrus_dst_buf_addr(ctx, backward_idx, 1);
> > > > -
> > > > -     cedrus_write(dev, VE_DEC_MPEG_BWD_REF_LUMA_ADDR, bwd_luma_addr);
> > > > -     cedrus_write(dev, VE_DEC_MPEG_BWD_REF_CHROMA_ADDR,
> > > > bwd_chroma_addr);
> > > > +     cedrus_write_ref_buf_addr(ctx, vq, pic->forward_ref_ts,
> > > > +                               VE_DEC_MPEG_FWD_REF_LUMA_ADDR,
> > > > +                               VE_DEC_MPEG_FWD_REF_CHROMA_ADDR);
> > > > +     cedrus_write_ref_buf_addr(ctx, vq, pic->backward_ref_ts,
> > > > +                               VE_DEC_MPEG_BWD_REF_LUMA_ADDR,
> > > > +                               VE_DEC_MPEG_BWD_REF_CHROMA_ADDR);
> > > >
> > > >       /* Destination luma and chroma buffers. */
> > > >
> > > > -     dst_luma_addr = cedrus_dst_buf_addr(ctx, run->dst->vb2_buf.index,
> > > > 0);
> > > > -     dst_chroma_addr = cedrus_dst_buf_addr(ctx,
> > > > run->dst->vb2_buf.index, 1); +     dst_luma_addr =
> > > > cedrus_dst_buf_addr(ctx, &run->dst->vb2_buf, 0); +     dst_chroma_addr
> > > > = cedrus_dst_buf_addr(ctx, &run->dst->vb2_buf, 1);> >
> > > >       cedrus_write(dev, VE_DEC_MPEG_REC_LUMA, dst_luma_addr);
> > > >       cedrus_write(dev, VE_DEC_MPEG_REC_CHROMA, dst_chroma_addr);
> > > >
> > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_vp8.c
> > > > b/drivers/staging/media/sunxi/cedrus/cedrus_vp8.c index
> > > > f4016684b32d..196cf692186d 100644
> > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_vp8.c
> > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_vp8.c
> > > > @@ -661,7 +661,6 @@ static void cedrus_vp8_setup(struct cedrus_ctx *ctx,
> > > >
> > > >       dma_addr_t luma_addr, chroma_addr;
> > > >       dma_addr_t src_buf_addr;
> > > >       int header_size;
> > > >
> > > > -     int qindex;
> > > >
> > > >       u32 reg;
> > > >
> > > >       cedrus_engine_enable(ctx, CEDRUS_CODEC_VP8);
> > > >
> > > > @@ -805,43 +804,17 @@ static void cedrus_vp8_setup(struct cedrus_ctx
> > > > *ctx,
> > > >
> > > >       reg |= VE_VP8_LF_DELTA0(slice->lf.mb_mode_delta[0]);
> > > >       cedrus_write(dev, VE_VP8_MODE_LF_DELTA, reg);
> > > >
> > > > -     luma_addr = cedrus_dst_buf_addr(ctx, run->dst->vb2_buf.index, 0);
> > > > -     chroma_addr = cedrus_dst_buf_addr(ctx, run->dst->vb2_buf.index,
> > > > 1);
> > > > +     luma_addr = cedrus_dst_buf_addr(ctx, &run->dst->vb2_buf, 0);
> > > > +     chroma_addr = cedrus_dst_buf_addr(ctx, &run->dst->vb2_buf, 1);
> > > >
> > > >       cedrus_write(dev, VE_VP8_REC_LUMA, luma_addr);
> > > >       cedrus_write(dev, VE_VP8_REC_CHROMA, chroma_addr);
> > > >
> > > > -     qindex = vb2_find_timestamp(cap_q, slice->last_frame_ts, 0);
> > > > -     if (qindex >= 0) {
> > > > -             luma_addr = cedrus_dst_buf_addr(ctx, qindex, 0);
> > > > -             chroma_addr = cedrus_dst_buf_addr(ctx, qindex, 1);
> > > > -             cedrus_write(dev, VE_VP8_FWD_LUMA, luma_addr);
> > > > -             cedrus_write(dev, VE_VP8_FWD_CHROMA, chroma_addr);
> > > > -     } else {
> > > > -             cedrus_write(dev, VE_VP8_FWD_LUMA, 0);
> > > > -             cedrus_write(dev, VE_VP8_FWD_CHROMA, 0);
> > > > -     }
> > > > -
> > > > -     qindex = vb2_find_timestamp(cap_q, slice->golden_frame_ts, 0);
> > > > -     if (qindex >= 0) {
> > > > -             luma_addr = cedrus_dst_buf_addr(ctx, qindex, 0);
> > > > -             chroma_addr = cedrus_dst_buf_addr(ctx, qindex, 1);
> > > > -             cedrus_write(dev, VE_VP8_BWD_LUMA, luma_addr);
> > > > -             cedrus_write(dev, VE_VP8_BWD_CHROMA, chroma_addr);
> > > > -     } else {
> > > > -             cedrus_write(dev, VE_VP8_BWD_LUMA, 0);
> > > > -             cedrus_write(dev, VE_VP8_BWD_CHROMA, 0);
> > > > -     }
> > > > -
> > > > -     qindex = vb2_find_timestamp(cap_q, slice->alt_frame_ts, 0);
> > > > -     if (qindex >= 0) {
> > > > -             luma_addr = cedrus_dst_buf_addr(ctx, qindex, 0);
> > > > -             chroma_addr = cedrus_dst_buf_addr(ctx, qindex, 1);
> > > > -             cedrus_write(dev, VE_VP8_ALT_LUMA, luma_addr);
> > > > -             cedrus_write(dev, VE_VP8_ALT_CHROMA, chroma_addr);
> > > > -     } else {
> > > > -             cedrus_write(dev, VE_VP8_ALT_LUMA, 0);
> > > > -             cedrus_write(dev, VE_VP8_ALT_CHROMA, 0);
> > > > -     }
> > > > +     cedrus_write_ref_buf_addr(ctx, cap_q, slice->last_frame_ts,
> > > > +                               VE_VP8_FWD_LUMA, VE_VP8_FWD_CHROMA);
> > > > +     cedrus_write_ref_buf_addr(ctx, cap_q, slice->golden_frame_ts,
> > > > +                               VE_VP8_BWD_LUMA, VE_VP8_BWD_CHROMA);
> > > > +     cedrus_write_ref_buf_addr(ctx, cap_q, slice->alt_frame_ts,
> > > > +                               VE_VP8_ALT_LUMA, VE_VP8_ALT_CHROMA);
> > > >
> > > >       cedrus_write(dev, VE_H264_CTRL, VE_H264_CTRL_VP8 |
> > > >
> > > >                    VE_H264_CTRL_DECODE_ERR_INT |
>
>
>
>

Re: [PATCH v2 7/8] cedrus: Use vb2_find_buffer

[Nicolas Dufresne]

Le samedi 16 juillet 2022 à 11:55 -0300, Ezequiel Garcia a écrit :
> Hi Jernej,
> 
> On Sat, Jul 16, 2022 at 9:36 AM Jernej Škrabec <jernej.skrabec@gmail.com> wrote:
> > 
> > Dne petek, 15. julij 2022 ob 13:48:34 CEST je Ezequiel Garcia napisal(a):
> > > Hi Nicolas,
> > > 
> > > Thanks a lot for the test and the bug report.
> > > 
> > > On Thu, Jul 14, 2022 at 4:26 PM Nicolas Dufresne <nicolas@ndufresne.ca>
> > wrote:
> > > > Hi Ezequiel,
> > > > 
> > > > I started testing with these patches and found some NULL dreferences, see
> > > > my comment inline...
> > > > 
> > > > Le lundi 11 juillet 2022 à 18:11 -0300, Ezequiel Garcia a écrit :
> > > > > Use the newly introduced vb2_find_buffer API to get a vb2_buffer
> > > > > given a buffer timestamp.
> > > > > 
> > > > > Cc: Maxime Ripard <mripard@kernel.org>
> > > > > Cc: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > > > Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
> > > > > Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> > > > > Acked-by: Tomasz Figa <tfiga@chromium.org>
> > > > > ---
> > > > > 
> > > > >  drivers/staging/media/sunxi/cedrus/cedrus.h   | 24 ++++++-----
> > > > >  .../staging/media/sunxi/cedrus/cedrus_h264.c  | 16 +++----
> > > > >  .../staging/media/sunxi/cedrus/cedrus_h265.c  | 16 +++----
> > > > >  .../staging/media/sunxi/cedrus/cedrus_mpeg2.c | 28 ++++--------
> > > > >  .../staging/media/sunxi/cedrus/cedrus_vp8.c   | 43 ++++---------------
> > > > >  5 files changed, 46 insertions(+), 81 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h
> > > > > b/drivers/staging/media/sunxi/cedrus/cedrus.h index
> > > > > 3bc094eb497f..c054dbe3d3bc 100644
> > > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus.h
> > > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus.h
> > > > > @@ -233,19 +233,23 @@ static inline dma_addr_t cedrus_buf_addr(struct
> > > > > vb2_buffer *buf,> >
> > > > >  }
> > > > > 
> > > > >  static inline dma_addr_t cedrus_dst_buf_addr(struct cedrus_ctx *ctx,
> > > > > 
> > > > > -                                          int index, unsigned int
> > > > > plane)
> > > > > +                                          struct vb2_buffer *buf,
> > > > > +                                          unsigned int plane)
> > > > > 
> > > > >  {
> > > > > 
> > > > > -     struct vb2_buffer *buf = NULL;
> > > > > -     struct vb2_queue *vq;
> > > > > -
> > > > > -     if (index < 0)
> > > > > -             return 0;
> > > > > +     return buf ? cedrus_buf_addr(buf, &ctx->dst_fmt, plane) : 0;
> > > > > +}
> > > > > 
> > > > > -     vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx,
> > > > > V4L2_BUF_TYPE_VIDEO_CAPTURE);
> > > > > -     if (vq)
> > > > > -             buf = vb2_get_buffer(vq, index);
> > > > > +static inline void cedrus_write_ref_buf_addr(struct cedrus_ctx *ctx,
> > > > > +                                          struct vb2_queue *q,
> > > > > +                                          u64 timestamp,
> > > > > +                                          u32 luma_reg,
> > > > > +                                          u32 chroma_reg)
> > > > > +{
> > > > > +       struct cedrus_dev *dev = ctx->dev;
> > > > > +       struct vb2_buffer *buf = vb2_find_buffer(q, timestamp);
> > > > > 
> > > > > -     return buf ? cedrus_buf_addr(buf, &ctx->dst_fmt, plane) : 0;
> > > > > +       cedrus_write(dev, luma_reg, cedrus_dst_buf_addr(ctx, buf, 0));
> > > > > +       cedrus_write(dev, chroma_reg, cedrus_dst_buf_addr(ctx, buf, 1));
> > > > > 
> > > > >  }
> > > > > 
> > > > >  static inline struct cedrus_buffer *
> > > > > 
> > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> > > > > b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c index
> > > > > d8fb93035470..0559efeac125 100644
> > > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> > > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> > > > > @@ -111,16 +111,16 @@ static void cedrus_write_frame_list(struct
> > > > > cedrus_ctx *ctx,> >
> > > > >       for (i = 0; i < ARRAY_SIZE(decode->dpb); i++) {
> > > > > 
> > > > >               const struct v4l2_h264_dpb_entry *dpb = &decode->dpb[i];
> > > > >               struct cedrus_buffer *cedrus_buf;
> > > > > 
> > > > > -             int buf_idx;
> > > > > +             struct vb2_buffer *buf;
> > > > > 
> > > > >               if (!(dpb->flags & V4L2_H264_DPB_ENTRY_FLAG_VALID))
> > > > > 
> > > > >                       continue;
> > > > > 
> > > > > -             buf_idx = vb2_find_timestamp(cap_q, dpb->reference_ts, 0);
> > > > > -             if (buf_idx < 0)
> > > > > +             buf = vb2_find_buffer(cap_q, dpb->reference_ts);
> > > > > +             if (!buf)
> > > > > 
> > > > >                       continue;
> > > > > 
> > > > > -             cedrus_buf = vb2_to_cedrus_buffer(cap_q->bufs[buf_idx]);
> > > > > +             cedrus_buf = vb2_to_cedrus_buffer(buf);
> > > > > 
> > > > >               position = cedrus_buf->codec.h264.position;
> > > > >               used_dpbs |= BIT(position);
> > > > > 
> > > > > @@ -186,7 +186,7 @@ static void _cedrus_write_ref_list(struct cedrus_ctx
> > > > > *ctx,> >
> > > > >               const struct v4l2_h264_dpb_entry *dpb;
> > > > >               const struct cedrus_buffer *cedrus_buf;
> > > > >               unsigned int position;
> > > > > 
> > > > > -             int buf_idx;
> > > > > +             struct vb2_buffer *buf;
> > > > > 
> > > > >               u8 dpb_idx;
> > > > > 
> > > > >               dpb_idx = ref_list[i].index;
> > > > > 
> > > > > @@ -195,11 +195,11 @@ static void _cedrus_write_ref_list(struct
> > > > > cedrus_ctx *ctx,> >
> > > > >               if (!(dpb->flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE))
> > > > > 
> > > > >                       continue;
> > > > > 
> > > > > -             buf_idx = vb2_find_timestamp(cap_q, dpb->reference_ts, 0);
> > > > > -             if (buf_idx < 0)
> > > > > +             buf = vb2_find_buffer(cap_q, dpb->reference_ts);
> > > > > +             if (!buf)
> > > > > 
> > > > >                       continue;
> > > > > 
> > > > > -             cedrus_buf = vb2_to_cedrus_buffer(cap_q->bufs[buf_idx]);
> > > > > +             cedrus_buf = vb2_to_cedrus_buffer(buf);
> > > > > 
> > > > >               position = cedrus_buf->codec.h264.position;
> > > > > 
> > > > >               sram_array[i] |= position << 1;
> > > > > 
> > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> > > > > b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c index
> > > > > 44f385be9f6c..60cc13e4d0a9 100644
> > > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> > > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> > > > > @@ -102,14 +102,14 @@ static void
> > > > > cedrus_h265_frame_info_write_single(struct cedrus_ctx *ctx,> >
> > > > >                                               unsigned int index,
> > > > >                                               bool field_pic,
> > > > >                                               u32 pic_order_cnt[],
> > > > > 
> > > > > -                                             int buffer_index)
> > > > > +                                             struct vb2_buffer *buf)
> > > > > 
> > > > >  {
> > > > > 
> > > > >       struct cedrus_dev *dev = ctx->dev;
> > > > > 
> > > > > -     dma_addr_t dst_luma_addr = cedrus_dst_buf_addr(ctx, buffer_index,
> > > > > 0);
> > > > > -     dma_addr_t dst_chroma_addr = cedrus_dst_buf_addr(ctx,
> > > > > buffer_index, 1); +     dma_addr_t dst_luma_addr =
> > > > > cedrus_dst_buf_addr(ctx, buf, 0); +     dma_addr_t dst_chroma_addr =
> > > > > cedrus_dst_buf_addr(ctx, buf, 1);> >
> > > > >       dma_addr_t mv_col_buf_addr[2] = {
> > > > > 
> > > > > -             cedrus_h265_frame_info_mv_col_buf_addr(ctx, buffer_index,
> > > > > 0),
> > > > > -             cedrus_h265_frame_info_mv_col_buf_addr(ctx, buffer_index,
> > > > > +             cedrus_h265_frame_info_mv_col_buf_addr(ctx, buf->index,
> > > > > 0),
> > > > > +             cedrus_h265_frame_info_mv_col_buf_addr(ctx, buf->index,
> > > > > 
> > > > >                                                      field_pic ? 1 : 0)
> > > > 
> > > > Previously, -1 would be passed to
> > > > cedrus_h265_frame_info_mv_col_buf_addr(),
> > > > which would not find a buffer at that index, and would return 0. Now the
> > > > code will crash with a NULL pointer deref.
> > > 
> > > Is it really correct to pass -1 to cedrus_h265_frame_info_mv_col_buf_addr?
> > > It seems it get casted into an unsigned type and then used to calculate
> > > an address for DMA.
> > 
> > I totally agree that this is a latent bug and it should be fixed. H264 checks
> > for negative value, but not HEVC. Current code just makes out of bounds read,
> > which is tolerated, but yours causes NULL pointer dereference, which is not.
> > 
> > I suggest that following check is added in cedrus_h265_frame_info_write_dpb():
> > 
> > if (buffer_index < 0)
> >  continue;
> > 
> 
> Seems reasonable.
> 
> > I can send it as a fix so it gets backported and you update this patch so above
> > if is changed to NULL pointer check.
> > 
> 
> Yes, I like this approach.
> 
> > Do you all agree? Nicolas, do you prefer to send such patch, since you're first
> > who noticed something odd with the code?
> > 
> 
> Hans' last PR dropped the cedrus patch, and left vb2_find_timestamp
> for the time being (for 5.20). I like the proposal of sending a fix that can
> be backported. I can re-work the rest of this series on top.

Oops, I was to send a fix for that on top of the orignal, feel free to squash
that:

https://gitlab.collabora.com/nicolas/linux/-/commit/1093321ddb8d3fb06833f471a60d07ce94e78d88

From 1093321ddb8d3fb06833f471a60d07ce94e78d88 Mon Sep 17 00:00:00 2001
From: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Date: Fri, 15 Jul 2022 15:28:44 -0400
Subject: [PATCH] media: cedrus: Fix NULL buf dereference

This is a regression introduced after porting to vb2_find_buffer.
The function returnis NULL when the buffer isn't found. The HEVC
decoder would call a helper to get the motion vector buffer address
by passing the index. This is fixed by passing the buffer
pointer instead of the index.

Fixes: 7f3614514ab0 ("cedrus: Use vb2_find_buffer")
Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 drivers/staging/media/sunxi/cedrus/cedrus_h265.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
index 28d90fec9aea..d359726b4966 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
@@ -91,16 +91,13 @@ static void cedrus_h265_sram_write_data(struct cedrus_dev
*dev, void *data,
 
 static inline dma_addr_t
 cedrus_h265_frame_info_mv_col_buf_addr(struct cedrus_ctx *ctx,
-				       unsigned int index,
+				       struct vb2_buffer *buf,
 				       const struct v4l2_ctrl_hevc_sps *sps)
 {
 	struct cedrus_buffer *cedrus_buf = NULL;
-	struct vb2_buffer *buf = NULL;
 	struct vb2_queue *vq;
 
 	vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE);
-	if (vq)
-		buf = vb2_get_buffer(vq, index);
 
 	if (buf)
 		cedrus_buf = vb2_to_cedrus_buffer(buf);
@@ -148,8 +145,8 @@ static void cedrus_h265_frame_info_write_single(struct
cedrus_ctx *ctx,
 	dma_addr_t dst_luma_addr = cedrus_dst_buf_addr(ctx, buf, 0);
 	dma_addr_t dst_chroma_addr = cedrus_dst_buf_addr(ctx, buf, 1);
 	dma_addr_t mv_col_buf_addr[2] = {
-		cedrus_h265_frame_info_mv_col_buf_addr(ctx, buf->index, sps),
-		cedrus_h265_frame_info_mv_col_buf_addr(ctx, buf->index, sps)
+		cedrus_h265_frame_info_mv_col_buf_addr(ctx, buf, sps),
+		cedrus_h265_frame_info_mv_col_buf_addr(ctx, buf, sps)
 	};
 	u32 offset = VE_DEC_H265_SRAM_OFFSET_FRAME_INFO +
 		     VE_DEC_H265_SRAM_OFFSET_FRAME_INFO_UNIT * index;
-- 
GitLab

> 
> Thanks,
> Ezequiel
> 
> > Best regards,
> > Jernej
> > 
> > > 
> > > static inline dma_addr_t
> > > cedrus_h265_frame_info_mv_col_buf_addr(struct cedrus_ctx *ctx,
> > >                                        unsigned int index, unsigned int
> > > field) {
> > >         return ctx->codec.h265.mv_col_buf_addr + index *
> > >                ctx->codec.h265.mv_col_buf_unit_size +
> > >                field * ctx->codec.h265.mv_col_buf_unit_size / 2;
> > > }
> > > 
> > > Fixing the driver to go back to the previous behavior is trivial,
> > > but this looks odd.
> > > 
> > > Jernej, Paul, any thoughts?
> > > 
> > > Thanks,
> > > Ezequiel
> > > 
> > > > >       };
> > > > >       u32 offset = VE_DEC_H265_SRAM_OFFSET_FRAME_INFO +
> > > > > 
> > > > > @@ -141,7 +141,7 @@ static void cedrus_h265_frame_info_write_dpb(struct
> > > > > cedrus_ctx *ctx,> >
> > > > >       unsigned int i;
> > > > > 
> > > > >       for (i = 0; i < num_active_dpb_entries; i++) {
> > > > > 
> > > > > -             int buffer_index = vb2_find_timestamp(vq,
> > > > > dpb[i].timestamp, 0); +             struct vb2_buffer *buf =
> > > > > vb2_find_buffer(vq, dpb[i].timestamp);> >
> > > > >               u32 pic_order_cnt[2] = {
> > > > > 
> > > > >                       dpb[i].pic_order_cnt[0],
> > > > >                       dpb[i].pic_order_cnt[1]
> > > > > 
> > > > > @@ -149,7 +149,7 @@ static void cedrus_h265_frame_info_write_dpb(struct
> > > > > cedrus_ctx *ctx,> >
> > > > >               cedrus_h265_frame_info_write_single(ctx, i,
> > > > >               dpb[i].field_pic,
> > > > > 
> > > > >                                                   pic_order_cnt,
> > > > > 
> > > > > -                                                 buffer_index);
> > > > > +                                                 buf);
> > > > > 
> > > > >       }
> > > > > 
> > > > >  }
> > > > > 
> > > > > @@ -616,7 +616,7 @@ static void cedrus_h265_setup(struct cedrus_ctx
> > > > > *ctx,
> > > > > 
> > > > >       cedrus_h265_frame_info_write_single(ctx, output_pic_list_index,
> > > > > 
> > > > >                                           slice_params->pic_struct != 0,
> > > > >                                           pic_order_cnt,
> > > > > 
> > > > > -                                         run->dst->vb2_buf.index);
> > > > > +                                         &run->dst->vb2_buf);
> > > > > 
> > > > >       cedrus_write(dev, VE_DEC_H265_OUTPUT_FRAME_IDX,
> > > > >       output_pic_list_index);
> > > > > 
> > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> > > > > b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c index
> > > > > 5dad2f296c6d..22d6cae9a710 100644
> > > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> > > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> > > > > @@ -54,13 +54,9 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx
> > > > > *ctx, struct cedrus_run *run)> >
> > > > >       const struct v4l2_ctrl_mpeg2_picture *pic;
> > > > >       const struct v4l2_ctrl_mpeg2_quantisation *quantisation;
> > > > >       dma_addr_t src_buf_addr, dst_luma_addr, dst_chroma_addr;
> > > > > 
> > > > > -     dma_addr_t fwd_luma_addr, fwd_chroma_addr;
> > > > > -     dma_addr_t bwd_luma_addr, bwd_chroma_addr;
> > > > > 
> > > > >       struct cedrus_dev *dev = ctx->dev;
> > > > >       struct vb2_queue *vq;
> > > > >       const u8 *matrix;
> > > > > 
> > > > > -     int forward_idx;
> > > > > -     int backward_idx;
> > > > > 
> > > > >       unsigned int i;
> > > > >       u32 reg;
> > > > > 
> > > > > @@ -123,27 +119,19 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx
> > > > > *ctx, struct cedrus_run *run)> >
> > > > >       cedrus_write(dev, VE_DEC_MPEG_PICBOUNDSIZE, reg);
> > > > > 
> > > > >       /* Forward and backward prediction reference buffers. */
> > > > > 
> > > > > -
> > > > > 
> > > > >       vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx,
> > > > >       V4L2_BUF_TYPE_VIDEO_CAPTURE);
> > > > > 
> > > > > -     forward_idx = vb2_find_timestamp(vq, pic->forward_ref_ts, 0);
> > > > > -     fwd_luma_addr = cedrus_dst_buf_addr(ctx, forward_idx, 0);
> > > > > -     fwd_chroma_addr = cedrus_dst_buf_addr(ctx, forward_idx, 1);
> > > > > -
> > > > > -     cedrus_write(dev, VE_DEC_MPEG_FWD_REF_LUMA_ADDR, fwd_luma_addr);
> > > > > -     cedrus_write(dev, VE_DEC_MPEG_FWD_REF_CHROMA_ADDR,
> > > > > fwd_chroma_addr);
> > > > > -
> > > > > -     backward_idx = vb2_find_timestamp(vq, pic->backward_ref_ts, 0);
> > > > > -     bwd_luma_addr = cedrus_dst_buf_addr(ctx, backward_idx, 0);
> > > > > -     bwd_chroma_addr = cedrus_dst_buf_addr(ctx, backward_idx, 1);
> > > > > -
> > > > > -     cedrus_write(dev, VE_DEC_MPEG_BWD_REF_LUMA_ADDR, bwd_luma_addr);
> > > > > -     cedrus_write(dev, VE_DEC_MPEG_BWD_REF_CHROMA_ADDR,
> > > > > bwd_chroma_addr);
> > > > > +     cedrus_write_ref_buf_addr(ctx, vq, pic->forward_ref_ts,
> > > > > +                               VE_DEC_MPEG_FWD_REF_LUMA_ADDR,
> > > > > +                               VE_DEC_MPEG_FWD_REF_CHROMA_ADDR);
> > > > > +     cedrus_write_ref_buf_addr(ctx, vq, pic->backward_ref_ts,
> > > > > +                               VE_DEC_MPEG_BWD_REF_LUMA_ADDR,
> > > > > +                               VE_DEC_MPEG_BWD_REF_CHROMA_ADDR);
> > > > > 
> > > > >       /* Destination luma and chroma buffers. */
> > > > > 
> > > > > -     dst_luma_addr = cedrus_dst_buf_addr(ctx, run->dst->vb2_buf.index,
> > > > > 0);
> > > > > -     dst_chroma_addr = cedrus_dst_buf_addr(ctx,
> > > > > run->dst->vb2_buf.index, 1); +     dst_luma_addr =
> > > > > cedrus_dst_buf_addr(ctx, &run->dst->vb2_buf, 0); +     dst_chroma_addr
> > > > > = cedrus_dst_buf_addr(ctx, &run->dst->vb2_buf, 1);> >
> > > > >       cedrus_write(dev, VE_DEC_MPEG_REC_LUMA, dst_luma_addr);
> > > > >       cedrus_write(dev, VE_DEC_MPEG_REC_CHROMA, dst_chroma_addr);
> > > > > 
> > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_vp8.c
> > > > > b/drivers/staging/media/sunxi/cedrus/cedrus_vp8.c index
> > > > > f4016684b32d..196cf692186d 100644
> > > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_vp8.c
> > > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_vp8.c
> > > > > @@ -661,7 +661,6 @@ static void cedrus_vp8_setup(struct cedrus_ctx *ctx,
> > > > > 
> > > > >       dma_addr_t luma_addr, chroma_addr;
> > > > >       dma_addr_t src_buf_addr;
> > > > >       int header_size;
> > > > > 
> > > > > -     int qindex;
> > > > > 
> > > > >       u32 reg;
> > > > > 
> > > > >       cedrus_engine_enable(ctx, CEDRUS_CODEC_VP8);
> > > > > 
> > > > > @@ -805,43 +804,17 @@ static void cedrus_vp8_setup(struct cedrus_ctx
> > > > > *ctx,
> > > > > 
> > > > >       reg |= VE_VP8_LF_DELTA0(slice->lf.mb_mode_delta[0]);
> > > > >       cedrus_write(dev, VE_VP8_MODE_LF_DELTA, reg);
> > > > > 
> > > > > -     luma_addr = cedrus_dst_buf_addr(ctx, run->dst->vb2_buf.index, 0);
> > > > > -     chroma_addr = cedrus_dst_buf_addr(ctx, run->dst->vb2_buf.index,
> > > > > 1);
> > > > > +     luma_addr = cedrus_dst_buf_addr(ctx, &run->dst->vb2_buf, 0);
> > > > > +     chroma_addr = cedrus_dst_buf_addr(ctx, &run->dst->vb2_buf, 1);
> > > > > 
> > > > >       cedrus_write(dev, VE_VP8_REC_LUMA, luma_addr);
> > > > >       cedrus_write(dev, VE_VP8_REC_CHROMA, chroma_addr);
> > > > > 
> > > > > -     qindex = vb2_find_timestamp(cap_q, slice->last_frame_ts, 0);
> > > > > -     if (qindex >= 0) {
> > > > > -             luma_addr = cedrus_dst_buf_addr(ctx, qindex, 0);
> > > > > -             chroma_addr = cedrus_dst_buf_addr(ctx, qindex, 1);
> > > > > -             cedrus_write(dev, VE_VP8_FWD_LUMA, luma_addr);
> > > > > -             cedrus_write(dev, VE_VP8_FWD_CHROMA, chroma_addr);
> > > > > -     } else {
> > > > > -             cedrus_write(dev, VE_VP8_FWD_LUMA, 0);
> > > > > -             cedrus_write(dev, VE_VP8_FWD_CHROMA, 0);
> > > > > -     }
> > > > > -
> > > > > -     qindex = vb2_find_timestamp(cap_q, slice->golden_frame_ts, 0);
> > > > > -     if (qindex >= 0) {
> > > > > -             luma_addr = cedrus_dst_buf_addr(ctx, qindex, 0);
> > > > > -             chroma_addr = cedrus_dst_buf_addr(ctx, qindex, 1);
> > > > > -             cedrus_write(dev, VE_VP8_BWD_LUMA, luma_addr);
> > > > > -             cedrus_write(dev, VE_VP8_BWD_CHROMA, chroma_addr);
> > > > > -     } else {
> > > > > -             cedrus_write(dev, VE_VP8_BWD_LUMA, 0);
> > > > > -             cedrus_write(dev, VE_VP8_BWD_CHROMA, 0);
> > > > > -     }
> > > > > -
> > > > > -     qindex = vb2_find_timestamp(cap_q, slice->alt_frame_ts, 0);
> > > > > -     if (qindex >= 0) {
> > > > > -             luma_addr = cedrus_dst_buf_addr(ctx, qindex, 0);
> > > > > -             chroma_addr = cedrus_dst_buf_addr(ctx, qindex, 1);
> > > > > -             cedrus_write(dev, VE_VP8_ALT_LUMA, luma_addr);
> > > > > -             cedrus_write(dev, VE_VP8_ALT_CHROMA, chroma_addr);
> > > > > -     } else {
> > > > > -             cedrus_write(dev, VE_VP8_ALT_LUMA, 0);
> > > > > -             cedrus_write(dev, VE_VP8_ALT_CHROMA, 0);
> > > > > -     }
> > > > > +     cedrus_write_ref_buf_addr(ctx, cap_q, slice->last_frame_ts,
> > > > > +                               VE_VP8_FWD_LUMA, VE_VP8_FWD_CHROMA);
> > > > > +     cedrus_write_ref_buf_addr(ctx, cap_q, slice->golden_frame_ts,
> > > > > +                               VE_VP8_BWD_LUMA, VE_VP8_BWD_CHROMA);
> > > > > +     cedrus_write_ref_buf_addr(ctx, cap_q, slice->alt_frame_ts,
> > > > > +                               VE_VP8_ALT_LUMA, VE_VP8_ALT_CHROMA);
> > > > > 
> > > > >       cedrus_write(dev, VE_H264_CTRL, VE_H264_CTRL_VP8 |
> > > > > 
> > > > >                    VE_H264_CTRL_DECODE_ERR_INT |
> > 
> > 
> > 
> >