| Message ID | 20220711211141.349902-1-ezequiel@vanguardiasur.com.ar |
|---|---|
| Headers | show |
| Series | videobuf2: Replace vb2_find_timestamp() with vb2_find_buffer() | expand |
On Mo, 2022-07-11 at 18:11 -0300, Ezequiel Garcia wrote: > From: Ezequiel Garcia <ezequiel@collabora.com> > > All users of vb2_find_timestamp() combine it with vb2_get_buffer() > to retrieve a videobuf2 buffer, given a u64 timestamp. > > Introduce an API for this use-case. Users will be converted to the new > API as follow-up commits. > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > Acked-by: Tomasz Figa <tfiga@chromium.org> Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de> regards Philipp
On Mo, 2022-07-11 at 18:11 -0300, Ezequiel Garcia wrote: > Use the newly introduced vb2_find_buffer API to get a vb2_buffer > given a buffer timestamp. > > Cc: Philipp Zabel <p.zabel@pengutronix.de> > Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> > Acked-by: Tomasz Figa <tfiga@chromium.org> Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de> regards Philipp
Hi Ezequiel, On Mon 11 Jul 22, 18:11, Ezequiel Garcia wrote: > Use the newly introduced vb2_find_buffer API to get a vb2_buffer > given a buffer timestamp. Looks good with the changes requested by Jernej! Reviewed-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> As a sidenote for later: maybe we could merge cedrus_dst_buf_addr and cedrus_buf_addr into one. Thanks, Paul > Cc: Maxime Ripard <mripard@kernel.org> > Cc: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > Cc: Jernej Skrabec <jernej.skrabec@gmail.com> > Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> > Acked-by: Tomasz Figa <tfiga@chromium.org> > --- > drivers/staging/media/sunxi/cedrus/cedrus.h | 24 ++++++----- > .../staging/media/sunxi/cedrus/cedrus_h264.c | 16 +++---- > .../staging/media/sunxi/cedrus/cedrus_h265.c | 16 +++---- > .../staging/media/sunxi/cedrus/cedrus_mpeg2.c | 28 ++++-------- > .../staging/media/sunxi/cedrus/cedrus_vp8.c | 43 ++++--------------- > 5 files changed, 46 insertions(+), 81 deletions(-) > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h b/drivers/staging/media/sunxi/cedrus/cedrus.h > index 3bc094eb497f..c054dbe3d3bc 100644 > --- a/drivers/staging/media/sunxi/cedrus/cedrus.h > +++ b/drivers/staging/media/sunxi/cedrus/cedrus.h > @@ -233,19 +233,23 @@ static inline dma_addr_t cedrus_buf_addr(struct vb2_buffer *buf, > } > > static inline dma_addr_t cedrus_dst_buf_addr(struct cedrus_ctx *ctx, > - int index, unsigned int plane) > + struct vb2_buffer *buf, > + unsigned int plane) > { > - struct vb2_buffer *buf = NULL; > - struct vb2_queue *vq; > - > - if (index < 0) > - return 0; > + return buf ? cedrus_buf_addr(buf, &ctx->dst_fmt, plane) : 0; > +} > > - vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE); > - if (vq) > - buf = vb2_get_buffer(vq, index); > +static inline void cedrus_write_ref_buf_addr(struct cedrus_ctx *ctx, > + struct vb2_queue *q, > + u64 timestamp, > + u32 luma_reg, > + u32 chroma_reg) > +{ > + struct cedrus_dev *dev = ctx->dev; > + struct vb2_buffer *buf = vb2_find_buffer(q, timestamp); > > - return buf ? cedrus_buf_addr(buf, &ctx->dst_fmt, plane) : 0; > + cedrus_write(dev, luma_reg, cedrus_dst_buf_addr(ctx, buf, 0)); > + cedrus_write(dev, chroma_reg, cedrus_dst_buf_addr(ctx, buf, 1)); > } > > static inline struct cedrus_buffer * > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > index d8fb93035470..0559efeac125 100644 > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > @@ -111,16 +111,16 @@ static void cedrus_write_frame_list(struct cedrus_ctx *ctx, > for (i = 0; i < ARRAY_SIZE(decode->dpb); i++) { > const struct v4l2_h264_dpb_entry *dpb = &decode->dpb[i]; > struct cedrus_buffer *cedrus_buf; > - int buf_idx; > + struct vb2_buffer *buf; > > if (!(dpb->flags & V4L2_H264_DPB_ENTRY_FLAG_VALID)) > continue; > > - buf_idx = vb2_find_timestamp(cap_q, dpb->reference_ts, 0); > - if (buf_idx < 0) > + buf = vb2_find_buffer(cap_q, dpb->reference_ts); > + if (!buf) > continue; > > - cedrus_buf = vb2_to_cedrus_buffer(cap_q->bufs[buf_idx]); > + cedrus_buf = vb2_to_cedrus_buffer(buf); > position = cedrus_buf->codec.h264.position; > used_dpbs |= BIT(position); > > @@ -186,7 +186,7 @@ static void _cedrus_write_ref_list(struct cedrus_ctx *ctx, > const struct v4l2_h264_dpb_entry *dpb; > const struct cedrus_buffer *cedrus_buf; > unsigned int position; > - int buf_idx; > + struct vb2_buffer *buf; > u8 dpb_idx; > > dpb_idx = ref_list[i].index; > @@ -195,11 +195,11 @@ static void _cedrus_write_ref_list(struct cedrus_ctx *ctx, > if (!(dpb->flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE)) > continue; > > - buf_idx = vb2_find_timestamp(cap_q, dpb->reference_ts, 0); > - if (buf_idx < 0) > + buf = vb2_find_buffer(cap_q, dpb->reference_ts); > + if (!buf) > continue; > > - cedrus_buf = vb2_to_cedrus_buffer(cap_q->bufs[buf_idx]); > + cedrus_buf = vb2_to_cedrus_buffer(buf); > position = cedrus_buf->codec.h264.position; > > sram_array[i] |= position << 1; > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c > index 44f385be9f6c..60cc13e4d0a9 100644 > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c > @@ -102,14 +102,14 @@ static void cedrus_h265_frame_info_write_single(struct cedrus_ctx *ctx, > unsigned int index, > bool field_pic, > u32 pic_order_cnt[], > - int buffer_index) > + struct vb2_buffer *buf) > { > struct cedrus_dev *dev = ctx->dev; > - dma_addr_t dst_luma_addr = cedrus_dst_buf_addr(ctx, buffer_index, 0); > - dma_addr_t dst_chroma_addr = cedrus_dst_buf_addr(ctx, buffer_index, 1); > + dma_addr_t dst_luma_addr = cedrus_dst_buf_addr(ctx, buf, 0); > + dma_addr_t dst_chroma_addr = cedrus_dst_buf_addr(ctx, buf, 1); > dma_addr_t mv_col_buf_addr[2] = { > - cedrus_h265_frame_info_mv_col_buf_addr(ctx, buffer_index, 0), > - cedrus_h265_frame_info_mv_col_buf_addr(ctx, buffer_index, > + cedrus_h265_frame_info_mv_col_buf_addr(ctx, buf->index, 0), > + cedrus_h265_frame_info_mv_col_buf_addr(ctx, buf->index, > field_pic ? 1 : 0) > }; > u32 offset = VE_DEC_H265_SRAM_OFFSET_FRAME_INFO + > @@ -141,7 +141,7 @@ static void cedrus_h265_frame_info_write_dpb(struct cedrus_ctx *ctx, > unsigned int i; > > for (i = 0; i < num_active_dpb_entries; i++) { > - int buffer_index = vb2_find_timestamp(vq, dpb[i].timestamp, 0); > + struct vb2_buffer *buf = vb2_find_buffer(vq, dpb[i].timestamp); > u32 pic_order_cnt[2] = { > dpb[i].pic_order_cnt[0], > dpb[i].pic_order_cnt[1] > @@ -149,7 +149,7 @@ static void cedrus_h265_frame_info_write_dpb(struct cedrus_ctx *ctx, > > cedrus_h265_frame_info_write_single(ctx, i, dpb[i].field_pic, > pic_order_cnt, > - buffer_index); > + buf); > } > } > > @@ -616,7 +616,7 @@ static void cedrus_h265_setup(struct cedrus_ctx *ctx, > cedrus_h265_frame_info_write_single(ctx, output_pic_list_index, > slice_params->pic_struct != 0, > pic_order_cnt, > - run->dst->vb2_buf.index); > + &run->dst->vb2_buf); > > cedrus_write(dev, VE_DEC_H265_OUTPUT_FRAME_IDX, output_pic_list_index); > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c > index 5dad2f296c6d..22d6cae9a710 100644 > --- a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c > @@ -54,13 +54,9 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run) > const struct v4l2_ctrl_mpeg2_picture *pic; > const struct v4l2_ctrl_mpeg2_quantisation *quantisation; > dma_addr_t src_buf_addr, dst_luma_addr, dst_chroma_addr; > - dma_addr_t fwd_luma_addr, fwd_chroma_addr; > - dma_addr_t bwd_luma_addr, bwd_chroma_addr; > struct cedrus_dev *dev = ctx->dev; > struct vb2_queue *vq; > const u8 *matrix; > - int forward_idx; > - int backward_idx; > unsigned int i; > u32 reg; > > @@ -123,27 +119,19 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run) > cedrus_write(dev, VE_DEC_MPEG_PICBOUNDSIZE, reg); > > /* Forward and backward prediction reference buffers. */ > - > vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE); > > - forward_idx = vb2_find_timestamp(vq, pic->forward_ref_ts, 0); > - fwd_luma_addr = cedrus_dst_buf_addr(ctx, forward_idx, 0); > - fwd_chroma_addr = cedrus_dst_buf_addr(ctx, forward_idx, 1); > - > - cedrus_write(dev, VE_DEC_MPEG_FWD_REF_LUMA_ADDR, fwd_luma_addr); > - cedrus_write(dev, VE_DEC_MPEG_FWD_REF_CHROMA_ADDR, fwd_chroma_addr); > - > - backward_idx = vb2_find_timestamp(vq, pic->backward_ref_ts, 0); > - bwd_luma_addr = cedrus_dst_buf_addr(ctx, backward_idx, 0); > - bwd_chroma_addr = cedrus_dst_buf_addr(ctx, backward_idx, 1); > - > - cedrus_write(dev, VE_DEC_MPEG_BWD_REF_LUMA_ADDR, bwd_luma_addr); > - cedrus_write(dev, VE_DEC_MPEG_BWD_REF_CHROMA_ADDR, bwd_chroma_addr); > + cedrus_write_ref_buf_addr(ctx, vq, pic->forward_ref_ts, > + VE_DEC_MPEG_FWD_REF_LUMA_ADDR, > + VE_DEC_MPEG_FWD_REF_CHROMA_ADDR); > + cedrus_write_ref_buf_addr(ctx, vq, pic->backward_ref_ts, > + VE_DEC_MPEG_BWD_REF_LUMA_ADDR, > + VE_DEC_MPEG_BWD_REF_CHROMA_ADDR); > > /* Destination luma and chroma buffers. */ > > - dst_luma_addr = cedrus_dst_buf_addr(ctx, run->dst->vb2_buf.index, 0); > - dst_chroma_addr = cedrus_dst_buf_addr(ctx, run->dst->vb2_buf.index, 1); > + dst_luma_addr = cedrus_dst_buf_addr(ctx, &run->dst->vb2_buf, 0); > + dst_chroma_addr = cedrus_dst_buf_addr(ctx, &run->dst->vb2_buf, 1); > > cedrus_write(dev, VE_DEC_MPEG_REC_LUMA, dst_luma_addr); > cedrus_write(dev, VE_DEC_MPEG_REC_CHROMA, dst_chroma_addr); > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_vp8.c b/drivers/staging/media/sunxi/cedrus/cedrus_vp8.c > index f4016684b32d..196cf692186d 100644 > --- a/drivers/staging/media/sunxi/cedrus/cedrus_vp8.c > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_vp8.c > @@ -661,7 +661,6 @@ static void cedrus_vp8_setup(struct cedrus_ctx *ctx, > dma_addr_t luma_addr, chroma_addr; > dma_addr_t src_buf_addr; > int header_size; > - int qindex; > u32 reg; > > cedrus_engine_enable(ctx, CEDRUS_CODEC_VP8); > @@ -805,43 +804,17 @@ static void cedrus_vp8_setup(struct cedrus_ctx *ctx, > reg |= VE_VP8_LF_DELTA0(slice->lf.mb_mode_delta[0]); > cedrus_write(dev, VE_VP8_MODE_LF_DELTA, reg); > > - luma_addr = cedrus_dst_buf_addr(ctx, run->dst->vb2_buf.index, 0); > - chroma_addr = cedrus_dst_buf_addr(ctx, run->dst->vb2_buf.index, 1); > + luma_addr = cedrus_dst_buf_addr(ctx, &run->dst->vb2_buf, 0); > + chroma_addr = cedrus_dst_buf_addr(ctx, &run->dst->vb2_buf, 1); > cedrus_write(dev, VE_VP8_REC_LUMA, luma_addr); > cedrus_write(dev, VE_VP8_REC_CHROMA, chroma_addr); > > - qindex = vb2_find_timestamp(cap_q, slice->last_frame_ts, 0); > - if (qindex >= 0) { > - luma_addr = cedrus_dst_buf_addr(ctx, qindex, 0); > - chroma_addr = cedrus_dst_buf_addr(ctx, qindex, 1); > - cedrus_write(dev, VE_VP8_FWD_LUMA, luma_addr); > - cedrus_write(dev, VE_VP8_FWD_CHROMA, chroma_addr); > - } else { > - cedrus_write(dev, VE_VP8_FWD_LUMA, 0); > - cedrus_write(dev, VE_VP8_FWD_CHROMA, 0); > - } > - > - qindex = vb2_find_timestamp(cap_q, slice->golden_frame_ts, 0); > - if (qindex >= 0) { > - luma_addr = cedrus_dst_buf_addr(ctx, qindex, 0); > - chroma_addr = cedrus_dst_buf_addr(ctx, qindex, 1); > - cedrus_write(dev, VE_VP8_BWD_LUMA, luma_addr); > - cedrus_write(dev, VE_VP8_BWD_CHROMA, chroma_addr); > - } else { > - cedrus_write(dev, VE_VP8_BWD_LUMA, 0); > - cedrus_write(dev, VE_VP8_BWD_CHROMA, 0); > - } > - > - qindex = vb2_find_timestamp(cap_q, slice->alt_frame_ts, 0); > - if (qindex >= 0) { > - luma_addr = cedrus_dst_buf_addr(ctx, qindex, 0); > - chroma_addr = cedrus_dst_buf_addr(ctx, qindex, 1); > - cedrus_write(dev, VE_VP8_ALT_LUMA, luma_addr); > - cedrus_write(dev, VE_VP8_ALT_CHROMA, chroma_addr); > - } else { > - cedrus_write(dev, VE_VP8_ALT_LUMA, 0); > - cedrus_write(dev, VE_VP8_ALT_CHROMA, 0); > - } > + cedrus_write_ref_buf_addr(ctx, cap_q, slice->last_frame_ts, > + VE_VP8_FWD_LUMA, VE_VP8_FWD_CHROMA); > + cedrus_write_ref_buf_addr(ctx, cap_q, slice->golden_frame_ts, > + VE_VP8_BWD_LUMA, VE_VP8_BWD_CHROMA); > + cedrus_write_ref_buf_addr(ctx, cap_q, slice->alt_frame_ts, > + VE_VP8_ALT_LUMA, VE_VP8_ALT_CHROMA); > > cedrus_write(dev, VE_H264_CTRL, VE_H264_CTRL_VP8 | > VE_H264_CTRL_DECODE_ERR_INT | > -- > 2.34.3 >
12.07.2022 00:11, Ezequiel Garcia пишет: > Use the newly introduced vb2_find_buffer API to get a vb2_buffer > given a buffer timestamp. > > Cc: Dmitry Osipenko <digetx@gmail.com> > Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> > Acked-by: Tomasz Figa <tfiga@chromium.org> > --- > drivers/media/platform/nvidia/tegra-vde/h264.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/drivers/media/platform/nvidia/tegra-vde/h264.c b/drivers/media/platform/nvidia/tegra-vde/h264.c > index 88f81a134ba0..204e474d57f7 100644 > --- a/drivers/media/platform/nvidia/tegra-vde/h264.c > +++ b/drivers/media/platform/nvidia/tegra-vde/h264.c > @@ -659,20 +659,19 @@ static struct vb2_buffer *get_ref_buf(struct tegra_ctx *ctx, > { > const struct v4l2_h264_dpb_entry *dpb = ctx->h264.decode_params->dpb; > struct vb2_queue *cap_q = &ctx->fh.m2m_ctx->cap_q_ctx.q; > - int buf_idx = -1; > + struct vb2_buffer *vb = NULL; > > if (dpb[dpb_idx].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE) > - buf_idx = vb2_find_timestamp(cap_q, > - dpb[dpb_idx].reference_ts, 0); > + vb = vb2_find_buffer(cap_q, dpb[dpb_idx].reference_ts); > > /* > * If a DPB entry is unused or invalid, address of current destination > * buffer is returned. > */ > - if (buf_idx < 0) > + if (!vb) > return &dst->vb2_buf; > > - return vb2_get_buffer(cap_q, buf_idx); > + return vb; > } > > static int tegra_vde_validate_vb_size(struct tegra_ctx *ctx, Acked-by: Dmitry Osipenko <digetx@gmail.com>
Dne petek, 15. julij 2022 ob 13:48:34 CEST je Ezequiel Garcia napisal(a): > Hi Nicolas, > > Thanks a lot for the test and the bug report. > > On Thu, Jul 14, 2022 at 4:26 PM Nicolas Dufresne <nicolas@ndufresne.ca> wrote: > > Hi Ezequiel, > > > > I started testing with these patches and found some NULL dreferences, see > > my comment inline... > > > > Le lundi 11 juillet 2022 à 18:11 -0300, Ezequiel Garcia a écrit : > > > Use the newly introduced vb2_find_buffer API to get a vb2_buffer > > > given a buffer timestamp. > > > > > > Cc: Maxime Ripard <mripard@kernel.org> > > > Cc: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > > > Cc: Jernej Skrabec <jernej.skrabec@gmail.com> > > > Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> > > > Acked-by: Tomasz Figa <tfiga@chromium.org> > > > --- > > > > > > drivers/staging/media/sunxi/cedrus/cedrus.h | 24 ++++++----- > > > .../staging/media/sunxi/cedrus/cedrus_h264.c | 16 +++---- > > > .../staging/media/sunxi/cedrus/cedrus_h265.c | 16 +++---- > > > .../staging/media/sunxi/cedrus/cedrus_mpeg2.c | 28 ++++-------- > > > .../staging/media/sunxi/cedrus/cedrus_vp8.c | 43 ++++--------------- > > > 5 files changed, 46 insertions(+), 81 deletions(-) > > > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h > > > b/drivers/staging/media/sunxi/cedrus/cedrus.h index > > > 3bc094eb497f..c054dbe3d3bc 100644 > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus.h > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus.h > > > @@ -233,19 +233,23 @@ static inline dma_addr_t cedrus_buf_addr(struct > > > vb2_buffer *buf,> > > > > } > > > > > > static inline dma_addr_t cedrus_dst_buf_addr(struct cedrus_ctx *ctx, > > > > > > - int index, unsigned int > > > plane) > > > + struct vb2_buffer *buf, > > > + unsigned int plane) > > > > > > { > > > > > > - struct vb2_buffer *buf = NULL; > > > - struct vb2_queue *vq; > > > - > > > - if (index < 0) > > > - return 0; > > > + return buf ? cedrus_buf_addr(buf, &ctx->dst_fmt, plane) : 0; > > > +} > > > > > > - vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, > > > V4L2_BUF_TYPE_VIDEO_CAPTURE); > > > - if (vq) > > > - buf = vb2_get_buffer(vq, index); > > > +static inline void cedrus_write_ref_buf_addr(struct cedrus_ctx *ctx, > > > + struct vb2_queue *q, > > > + u64 timestamp, > > > + u32 luma_reg, > > > + u32 chroma_reg) > > > +{ > > > + struct cedrus_dev *dev = ctx->dev; > > > + struct vb2_buffer *buf = vb2_find_buffer(q, timestamp); > > > > > > - return buf ? cedrus_buf_addr(buf, &ctx->dst_fmt, plane) : 0; > > > + cedrus_write(dev, luma_reg, cedrus_dst_buf_addr(ctx, buf, 0)); > > > + cedrus_write(dev, chroma_reg, cedrus_dst_buf_addr(ctx, buf, 1)); > > > > > > } > > > > > > static inline struct cedrus_buffer * > > > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > > > b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c index > > > d8fb93035470..0559efeac125 100644 > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > > > @@ -111,16 +111,16 @@ static void cedrus_write_frame_list(struct > > > cedrus_ctx *ctx,> > > > > for (i = 0; i < ARRAY_SIZE(decode->dpb); i++) { > > > > > > const struct v4l2_h264_dpb_entry *dpb = &decode->dpb[i]; > > > struct cedrus_buffer *cedrus_buf; > > > > > > - int buf_idx; > > > + struct vb2_buffer *buf; > > > > > > if (!(dpb->flags & V4L2_H264_DPB_ENTRY_FLAG_VALID)) > > > > > > continue; > > > > > > - buf_idx = vb2_find_timestamp(cap_q, dpb->reference_ts, 0); > > > - if (buf_idx < 0) > > > + buf = vb2_find_buffer(cap_q, dpb->reference_ts); > > > + if (!buf) > > > > > > continue; > > > > > > - cedrus_buf = vb2_to_cedrus_buffer(cap_q->bufs[buf_idx]); > > > + cedrus_buf = vb2_to_cedrus_buffer(buf); > > > > > > position = cedrus_buf->codec.h264.position; > > > used_dpbs |= BIT(position); > > > > > > @@ -186,7 +186,7 @@ static void _cedrus_write_ref_list(struct cedrus_ctx > > > *ctx,> > > > > const struct v4l2_h264_dpb_entry *dpb; > > > const struct cedrus_buffer *cedrus_buf; > > > unsigned int position; > > > > > > - int buf_idx; > > > + struct vb2_buffer *buf; > > > > > > u8 dpb_idx; > > > > > > dpb_idx = ref_list[i].index; > > > > > > @@ -195,11 +195,11 @@ static void _cedrus_write_ref_list(struct > > > cedrus_ctx *ctx,> > > > > if (!(dpb->flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE)) > > > > > > continue; > > > > > > - buf_idx = vb2_find_timestamp(cap_q, dpb->reference_ts, 0); > > > - if (buf_idx < 0) > > > + buf = vb2_find_buffer(cap_q, dpb->reference_ts); > > > + if (!buf) > > > > > > continue; > > > > > > - cedrus_buf = vb2_to_cedrus_buffer(cap_q->bufs[buf_idx]); > > > + cedrus_buf = vb2_to_cedrus_buffer(buf); > > > > > > position = cedrus_buf->codec.h264.position; > > > > > > sram_array[i] |= position << 1; > > > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c > > > b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c index > > > 44f385be9f6c..60cc13e4d0a9 100644 > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c > > > @@ -102,14 +102,14 @@ static void > > > cedrus_h265_frame_info_write_single(struct cedrus_ctx *ctx,> > > > > unsigned int index, > > > bool field_pic, > > > u32 pic_order_cnt[], > > > > > > - int buffer_index) > > > + struct vb2_buffer *buf) > > > > > > { > > > > > > struct cedrus_dev *dev = ctx->dev; > > > > > > - dma_addr_t dst_luma_addr = cedrus_dst_buf_addr(ctx, buffer_index, > > > 0); > > > - dma_addr_t dst_chroma_addr = cedrus_dst_buf_addr(ctx, > > > buffer_index, 1); + dma_addr_t dst_luma_addr = > > > cedrus_dst_buf_addr(ctx, buf, 0); + dma_addr_t dst_chroma_addr = > > > cedrus_dst_buf_addr(ctx, buf, 1);> > > > > dma_addr_t mv_col_buf_addr[2] = { > > > > > > - cedrus_h265_frame_info_mv_col_buf_addr(ctx, buffer_index, > > > 0), > > > - cedrus_h265_frame_info_mv_col_buf_addr(ctx, buffer_index, > > > + cedrus_h265_frame_info_mv_col_buf_addr(ctx, buf->index, > > > 0), > > > + cedrus_h265_frame_info_mv_col_buf_addr(ctx, buf->index, > > > > > > field_pic ? 1 : 0) > > > > Previously, -1 would be passed to > > cedrus_h265_frame_info_mv_col_buf_addr(), > > which would not find a buffer at that index, and would return 0. Now the > > code will crash with a NULL pointer deref. > > Is it really correct to pass -1 to cedrus_h265_frame_info_mv_col_buf_addr? > It seems it get casted into an unsigned type and then used to calculate > an address for DMA. I totally agree that this is a latent bug and it should be fixed. H264 checks for negative value, but not HEVC. Current code just makes out of bounds read, which is tolerated, but yours causes NULL pointer dereference, which is not. I suggest that following check is added in cedrus_h265_frame_info_write_dpb(): if (buffer_index < 0) continue; I can send it as a fix so it gets backported and you update this patch so above if is changed to NULL pointer check. Do you all agree? Nicolas, do you prefer to send such patch, since you're first who noticed something odd with the code? Best regards, Jernej > > static inline dma_addr_t > cedrus_h265_frame_info_mv_col_buf_addr(struct cedrus_ctx *ctx, > unsigned int index, unsigned int > field) { > return ctx->codec.h265.mv_col_buf_addr + index * > ctx->codec.h265.mv_col_buf_unit_size + > field * ctx->codec.h265.mv_col_buf_unit_size / 2; > } > > Fixing the driver to go back to the previous behavior is trivial, > but this looks odd. > > Jernej, Paul, any thoughts? > > Thanks, > Ezequiel > > > > }; > > > u32 offset = VE_DEC_H265_SRAM_OFFSET_FRAME_INFO + > > > > > > @@ -141,7 +141,7 @@ static void cedrus_h265_frame_info_write_dpb(struct > > > cedrus_ctx *ctx,> > > > > unsigned int i; > > > > > > for (i = 0; i < num_active_dpb_entries; i++) { > > > > > > - int buffer_index = vb2_find_timestamp(vq, > > > dpb[i].timestamp, 0); + struct vb2_buffer *buf = > > > vb2_find_buffer(vq, dpb[i].timestamp);> > > > > u32 pic_order_cnt[2] = { > > > > > > dpb[i].pic_order_cnt[0], > > > dpb[i].pic_order_cnt[1] > > > > > > @@ -149,7 +149,7 @@ static void cedrus_h265_frame_info_write_dpb(struct > > > cedrus_ctx *ctx,> > > > > cedrus_h265_frame_info_write_single(ctx, i, > > > dpb[i].field_pic, > > > > > > pic_order_cnt, > > > > > > - buffer_index); > > > + buf); > > > > > > } > > > > > > } > > > > > > @@ -616,7 +616,7 @@ static void cedrus_h265_setup(struct cedrus_ctx > > > *ctx, > > > > > > cedrus_h265_frame_info_write_single(ctx, output_pic_list_index, > > > > > > slice_params->pic_struct != 0, > > > pic_order_cnt, > > > > > > - run->dst->vb2_buf.index); > > > + &run->dst->vb2_buf); > > > > > > cedrus_write(dev, VE_DEC_H265_OUTPUT_FRAME_IDX, > > > output_pic_list_index); > > > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c > > > b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c index > > > 5dad2f296c6d..22d6cae9a710 100644 > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c > > > @@ -54,13 +54,9 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx > > > *ctx, struct cedrus_run *run)> > > > > const struct v4l2_ctrl_mpeg2_picture *pic; > > > const struct v4l2_ctrl_mpeg2_quantisation *quantisation; > > > dma_addr_t src_buf_addr, dst_luma_addr, dst_chroma_addr; > > > > > > - dma_addr_t fwd_luma_addr, fwd_chroma_addr; > > > - dma_addr_t bwd_luma_addr, bwd_chroma_addr; > > > > > > struct cedrus_dev *dev = ctx->dev; > > > struct vb2_queue *vq; > > > const u8 *matrix; > > > > > > - int forward_idx; > > > - int backward_idx; > > > > > > unsigned int i; > > > u32 reg; > > > > > > @@ -123,27 +119,19 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx > > > *ctx, struct cedrus_run *run)> > > > > cedrus_write(dev, VE_DEC_MPEG_PICBOUNDSIZE, reg); > > > > > > /* Forward and backward prediction reference buffers. */ > > > > > > - > > > > > > vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, > > > V4L2_BUF_TYPE_VIDEO_CAPTURE); > > > > > > - forward_idx = vb2_find_timestamp(vq, pic->forward_ref_ts, 0); > > > - fwd_luma_addr = cedrus_dst_buf_addr(ctx, forward_idx, 0); > > > - fwd_chroma_addr = cedrus_dst_buf_addr(ctx, forward_idx, 1); > > > - > > > - cedrus_write(dev, VE_DEC_MPEG_FWD_REF_LUMA_ADDR, fwd_luma_addr); > > > - cedrus_write(dev, VE_DEC_MPEG_FWD_REF_CHROMA_ADDR, > > > fwd_chroma_addr); > > > - > > > - backward_idx = vb2_find_timestamp(vq, pic->backward_ref_ts, 0); > > > - bwd_luma_addr = cedrus_dst_buf_addr(ctx, backward_idx, 0); > > > - bwd_chroma_addr = cedrus_dst_buf_addr(ctx, backward_idx, 1); > > > - > > > - cedrus_write(dev, VE_DEC_MPEG_BWD_REF_LUMA_ADDR, bwd_luma_addr); > > > - cedrus_write(dev, VE_DEC_MPEG_BWD_REF_CHROMA_ADDR, > > > bwd_chroma_addr); > > > + cedrus_write_ref_buf_addr(ctx, vq, pic->forward_ref_ts, > > > + VE_DEC_MPEG_FWD_REF_LUMA_ADDR, > > > + VE_DEC_MPEG_FWD_REF_CHROMA_ADDR); > > > + cedrus_write_ref_buf_addr(ctx, vq, pic->backward_ref_ts, > > > + VE_DEC_MPEG_BWD_REF_LUMA_ADDR, > > > + VE_DEC_MPEG_BWD_REF_CHROMA_ADDR); > > > > > > /* Destination luma and chroma buffers. */ > > > > > > - dst_luma_addr = cedrus_dst_buf_addr(ctx, run->dst->vb2_buf.index, > > > 0); > > > - dst_chroma_addr = cedrus_dst_buf_addr(ctx, > > > run->dst->vb2_buf.index, 1); + dst_luma_addr = > > > cedrus_dst_buf_addr(ctx, &run->dst->vb2_buf, 0); + dst_chroma_addr > > > = cedrus_dst_buf_addr(ctx, &run->dst->vb2_buf, 1);> > > > > cedrus_write(dev, VE_DEC_MPEG_REC_LUMA, dst_luma_addr); > > > cedrus_write(dev, VE_DEC_MPEG_REC_CHROMA, dst_chroma_addr); > > > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_vp8.c > > > b/drivers/staging/media/sunxi/cedrus/cedrus_vp8.c index > > > f4016684b32d..196cf692186d 100644 > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_vp8.c > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_vp8.c > > > @@ -661,7 +661,6 @@ static void cedrus_vp8_setup(struct cedrus_ctx *ctx, > > > > > > dma_addr_t luma_addr, chroma_addr; > > > dma_addr_t src_buf_addr; > > > int header_size; > > > > > > - int qindex; > > > > > > u32 reg; > > > > > > cedrus_engine_enable(ctx, CEDRUS_CODEC_VP8); > > > > > > @@ -805,43 +804,17 @@ static void cedrus_vp8_setup(struct cedrus_ctx > > > *ctx, > > > > > > reg |= VE_VP8_LF_DELTA0(slice->lf.mb_mode_delta[0]); > > > cedrus_write(dev, VE_VP8_MODE_LF_DELTA, reg); > > > > > > - luma_addr = cedrus_dst_buf_addr(ctx, run->dst->vb2_buf.index, 0); > > > - chroma_addr = cedrus_dst_buf_addr(ctx, run->dst->vb2_buf.index, > > > 1); > > > + luma_addr = cedrus_dst_buf_addr(ctx, &run->dst->vb2_buf, 0); > > > + chroma_addr = cedrus_dst_buf_addr(ctx, &run->dst->vb2_buf, 1); > > > > > > cedrus_write(dev, VE_VP8_REC_LUMA, luma_addr); > > > cedrus_write(dev, VE_VP8_REC_CHROMA, chroma_addr); > > > > > > - qindex = vb2_find_timestamp(cap_q, slice->last_frame_ts, 0); > > > - if (qindex >= 0) { > > > - luma_addr = cedrus_dst_buf_addr(ctx, qindex, 0); > > > - chroma_addr = cedrus_dst_buf_addr(ctx, qindex, 1); > > > - cedrus_write(dev, VE_VP8_FWD_LUMA, luma_addr); > > > - cedrus_write(dev, VE_VP8_FWD_CHROMA, chroma_addr); > > > - } else { > > > - cedrus_write(dev, VE_VP8_FWD_LUMA, 0); > > > - cedrus_write(dev, VE_VP8_FWD_CHROMA, 0); > > > - } > > > - > > > - qindex = vb2_find_timestamp(cap_q, slice->golden_frame_ts, 0); > > > - if (qindex >= 0) { > > > - luma_addr = cedrus_dst_buf_addr(ctx, qindex, 0); > > > - chroma_addr = cedrus_dst_buf_addr(ctx, qindex, 1); > > > - cedrus_write(dev, VE_VP8_BWD_LUMA, luma_addr); > > > - cedrus_write(dev, VE_VP8_BWD_CHROMA, chroma_addr); > > > - } else { > > > - cedrus_write(dev, VE_VP8_BWD_LUMA, 0); > > > - cedrus_write(dev, VE_VP8_BWD_CHROMA, 0); > > > - } > > > - > > > - qindex = vb2_find_timestamp(cap_q, slice->alt_frame_ts, 0); > > > - if (qindex >= 0) { > > > - luma_addr = cedrus_dst_buf_addr(ctx, qindex, 0); > > > - chroma_addr = cedrus_dst_buf_addr(ctx, qindex, 1); > > > - cedrus_write(dev, VE_VP8_ALT_LUMA, luma_addr); > > > - cedrus_write(dev, VE_VP8_ALT_CHROMA, chroma_addr); > > > - } else { > > > - cedrus_write(dev, VE_VP8_ALT_LUMA, 0); > > > - cedrus_write(dev, VE_VP8_ALT_CHROMA, 0); > > > - } > > > + cedrus_write_ref_buf_addr(ctx, cap_q, slice->last_frame_ts, > > > + VE_VP8_FWD_LUMA, VE_VP8_FWD_CHROMA); > > > + cedrus_write_ref_buf_addr(ctx, cap_q, slice->golden_frame_ts, > > > + VE_VP8_BWD_LUMA, VE_VP8_BWD_CHROMA); > > > + cedrus_write_ref_buf_addr(ctx, cap_q, slice->alt_frame_ts, > > > + VE_VP8_ALT_LUMA, VE_VP8_ALT_CHROMA); > > > > > > cedrus_write(dev, VE_H264_CTRL, VE_H264_CTRL_VP8 | > > > > > > VE_H264_CTRL_DECODE_ERR_INT |