2022-07-11 21:15:21

by Ezequiel Garcia

[permalink] [raw]

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

---

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

Cc: Maxime Ripard <[email protected]>
Cc: Paul Kocialkowski <[email protected]>
Cc: Jernej Skrabec <[email protected]>
Signed-off-by: Ezequiel Garcia <[email protected]>
Acked-by: Tomasz Figa <[email protected]>
---
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

2022-07-12 07:33:08

by Paul Kocialkowski

[permalink] [raw]

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

---

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 <[email protected]>

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

Thanks,

Paul

> Cc: Maxime Ripard <[email protected]>
> Cc: Paul Kocialkowski <[email protected]>
> Cc: Jernej Skrabec <[email protected]>
> Signed-off-by: Ezequiel Garcia <[email protected]>
> Acked-by: Tomasz Figa <[email protected]>
> ---
> 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

---

Attachments:

(No filename) (11.96 kB)
signature.asc (499.00 B)
Download all attachments

2022-07-14 19:38:02

by Nicolas Dufresne

[permalink] [raw]

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

---

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 <[email protected]>
> Cc: Paul Kocialkowski <[email protected]>
> Cc: Jernej Skrabec <[email protected]>
> Signed-off-by: Ezequiel Garcia <[email protected]>
> Acked-by: Tomasz Figa <[email protected]>
> ---
> 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 |

2022-07-15 12:12:29

by Ezequiel Garcia

[permalink] [raw]

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

---

Hi Nicolas,

Thanks a lot for the test and the bug report.

On Thu, Jul 14, 2022 at 4:26 PM Nicolas Dufresne <[email protected]> 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 <[email protected]>
> > Cc: Paul Kocialkowski <[email protected]>
> > Cc: Jernej Skrabec <[email protected]>
> > Signed-off-by: Ezequiel Garcia <[email protected]>
> > Acked-by: Tomasz Figa <[email protected]>
> > ---
> > 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 |
>

2022-07-16 13:06:42

by Jernej Škrabec

[permalink] [raw]

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

---

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 <[email protected]>
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 <[email protected]>
> > > Cc: Paul Kocialkowski <[email protected]>
> > > Cc: Jernej Skrabec <[email protected]>
> > > Signed-off-by: Ezequiel Garcia <[email protected]>
> > > Acked-by: Tomasz Figa <[email protected]>
> > > ---
> > >
> > > 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 |

2022-07-16 15:35:18

by Ezequiel Garcia

[permalink] [raw]

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

---

Hi Jernej,

On Sat, Jul 16, 2022 at 9:36 AM Jernej Škrabec <[email protected]> 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 <[email protected]>
> 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 <[email protected]>
> > > > Cc: Paul Kocialkowski <[email protected]>
> > > > Cc: Jernej Skrabec <[email protected]>
> > > > Signed-off-by: Ezequiel Garcia <[email protected]>
> > > > Acked-by: Tomasz Figa <[email protected]>
> > > > ---
> > > >
> > > > 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 |
>
>
>
>

2022-07-18 16:23:19

by Nicolas Dufresne

[permalink] [raw]

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

---

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 <[email protected]> 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 <[email protected]>
> > 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 <[email protected]>
> > > > > Cc: Paul Kocialkowski <[email protected]>
> > > > > Cc: Jernej Skrabec <[email protected]>
> > > > > Signed-off-by: Ezequiel Garcia <[email protected]>
> > > > > Acked-by: Tomasz Figa <[email protected]>
> > > > > ---
> > > > >
> > > > > 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 <[email protected]>
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 <[email protected]>
---
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 |
> >
> >
> >
> >