The ref builder only provided references that are marked as valid in the
dpb. Thus the current implementation of dpb_valid would always set the
flag to 1. This is not representing missing frames (this is called
'non-existing' pictures in the spec). In some context, these non-existing
pictures still need to occupy a slot in the reference list according to
the spec.
Signed-off-by: Nicolas Dufresne <[email protected]>
Reviewed-by: Sebastian Fricke <[email protected]>
---
drivers/staging/media/rkvdec/rkvdec-h264.c | 33 ++++++++++++++++------
1 file changed, 24 insertions(+), 9 deletions(-)
diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
index dff89732ddd0..bcde37d72244 100644
--- a/drivers/staging/media/rkvdec/rkvdec-h264.c
+++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
@@ -112,6 +112,7 @@ struct rkvdec_h264_run {
const struct v4l2_ctrl_h264_sps *sps;
const struct v4l2_ctrl_h264_pps *pps;
const struct v4l2_ctrl_h264_scaling_matrix *scaling_matrix;
+ int ref_buf_idx[V4L2_H264_NUM_DPB_ENTRIES];
};
struct rkvdec_h264_ctx {
@@ -725,6 +726,26 @@ static void assemble_hw_pps(struct rkvdec_ctx *ctx,
}
}
+static void lookup_ref_buf_idx(struct rkvdec_ctx *ctx,
+ struct rkvdec_h264_run *run)
+{
+ const struct v4l2_ctrl_h264_decode_params *dec_params = run->decode_params;
+ u32 i;
+
+ for (i = 0; i < ARRAY_SIZE(dec_params->dpb); i++) {
+ struct v4l2_m2m_ctx *m2m_ctx = ctx->fh.m2m_ctx;
+ const struct v4l2_h264_dpb_entry *dpb = run->decode_params->dpb;
+ struct vb2_queue *cap_q = &m2m_ctx->cap_q_ctx.q;
+ int buf_idx = -1;
+
+ if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE)
+ buf_idx = vb2_find_timestamp(cap_q,
+ dpb[i].reference_ts, 0);
+
+ run->ref_buf_idx[i] = buf_idx;
+ }
+}
+
static void assemble_hw_rps(struct rkvdec_ctx *ctx,
struct rkvdec_h264_run *run)
{
@@ -762,7 +783,7 @@ static void assemble_hw_rps(struct rkvdec_ctx *ctx,
for (j = 0; j < RKVDEC_NUM_REFLIST; j++) {
for (i = 0; i < h264_ctx->reflists.num_valid; i++) {
- u8 dpb_valid = 0;
+ bool dpb_valid = run->ref_buf_idx[i] >= 0;
u8 idx = 0;
switch (j) {
@@ -779,8 +800,6 @@ static void assemble_hw_rps(struct rkvdec_ctx *ctx,
if (idx >= ARRAY_SIZE(dec_params->dpb))
continue;
- dpb_valid = !!(dpb[idx].flags &
- V4L2_H264_DPB_ENTRY_FLAG_ACTIVE);
set_ps_field(hw_rps, DPB_INFO(i, j),
idx | dpb_valid << 4);
@@ -859,13 +878,8 @@ get_ref_buf(struct rkvdec_ctx *ctx, struct rkvdec_h264_run *run,
unsigned int dpb_idx)
{
struct v4l2_m2m_ctx *m2m_ctx = ctx->fh.m2m_ctx;
- const struct v4l2_h264_dpb_entry *dpb = run->decode_params->dpb;
struct vb2_queue *cap_q = &m2m_ctx->cap_q_ctx.q;
- int buf_idx = -1;
-
- if (dpb[dpb_idx].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE)
- buf_idx = vb2_find_timestamp(cap_q,
- dpb[dpb_idx].reference_ts, 0);
+ int buf_idx = run->ref_buf_idx[dpb_idx];
/*
* If a DPB entry is unused or invalid, address of current destination
@@ -1102,6 +1116,7 @@ static int rkvdec_h264_run(struct rkvdec_ctx *ctx)
assemble_hw_scaling_list(ctx, &run);
assemble_hw_pps(ctx, &run);
+ lookup_ref_buf_idx(ctx, &run);
assemble_hw_rps(ctx, &run);
config_registers(ctx, &run);
--
2.34.1
On Thu, Mar 31, 2022 at 03:37:15PM -0400, Nicolas Dufresne wrote:
> The ref builder only provided references that are marked as valid in the
> dpb. Thus the current implementation of dpb_valid would always set the
> flag to 1. This is not representing missing frames (this is called
> 'non-existing' pictures in the spec). In some context, these non-existing
> pictures still need to occupy a slot in the reference list according to
> the spec.
>
Good catch! OOC, did you find this because it was failing a test vector?
> Signed-off-by: Nicolas Dufresne <[email protected]>
> Reviewed-by: Sebastian Fricke <[email protected]>
Fixes: cd33c830448ba ("media: rkvdec: Add the rkvdec driver")
Reviewed-by: Ezequiel Garcia <[email protected]>
Thanks,
Ezequiel
> ---
> drivers/staging/media/rkvdec/rkvdec-h264.c | 33 ++++++++++++++++------
> 1 file changed, 24 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
> index dff89732ddd0..bcde37d72244 100644
> --- a/drivers/staging/media/rkvdec/rkvdec-h264.c
> +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
> @@ -112,6 +112,7 @@ struct rkvdec_h264_run {
> const struct v4l2_ctrl_h264_sps *sps;
> const struct v4l2_ctrl_h264_pps *pps;
> const struct v4l2_ctrl_h264_scaling_matrix *scaling_matrix;
> + int ref_buf_idx[V4L2_H264_NUM_DPB_ENTRIES];
> };
>
> struct rkvdec_h264_ctx {
> @@ -725,6 +726,26 @@ static void assemble_hw_pps(struct rkvdec_ctx *ctx,
> }
> }
>
> +static void lookup_ref_buf_idx(struct rkvdec_ctx *ctx,
> + struct rkvdec_h264_run *run)
> +{
> + const struct v4l2_ctrl_h264_decode_params *dec_params = run->decode_params;
> + u32 i;
> +
> + for (i = 0; i < ARRAY_SIZE(dec_params->dpb); i++) {
> + struct v4l2_m2m_ctx *m2m_ctx = ctx->fh.m2m_ctx;
> + const struct v4l2_h264_dpb_entry *dpb = run->decode_params->dpb;
> + struct vb2_queue *cap_q = &m2m_ctx->cap_q_ctx.q;
> + int buf_idx = -1;
> +
> + if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE)
> + buf_idx = vb2_find_timestamp(cap_q,
> + dpb[i].reference_ts, 0);
> +
> + run->ref_buf_idx[i] = buf_idx;
> + }
> +}
> +
> static void assemble_hw_rps(struct rkvdec_ctx *ctx,
> struct rkvdec_h264_run *run)
> {
> @@ -762,7 +783,7 @@ static void assemble_hw_rps(struct rkvdec_ctx *ctx,
>
> for (j = 0; j < RKVDEC_NUM_REFLIST; j++) {
> for (i = 0; i < h264_ctx->reflists.num_valid; i++) {
> - u8 dpb_valid = 0;
> + bool dpb_valid = run->ref_buf_idx[i] >= 0;
> u8 idx = 0;
>
> switch (j) {
> @@ -779,8 +800,6 @@ static void assemble_hw_rps(struct rkvdec_ctx *ctx,
>
> if (idx >= ARRAY_SIZE(dec_params->dpb))
> continue;
> - dpb_valid = !!(dpb[idx].flags &
> - V4L2_H264_DPB_ENTRY_FLAG_ACTIVE);
>
> set_ps_field(hw_rps, DPB_INFO(i, j),
> idx | dpb_valid << 4);
> @@ -859,13 +878,8 @@ get_ref_buf(struct rkvdec_ctx *ctx, struct rkvdec_h264_run *run,
> unsigned int dpb_idx)
> {
> struct v4l2_m2m_ctx *m2m_ctx = ctx->fh.m2m_ctx;
> - const struct v4l2_h264_dpb_entry *dpb = run->decode_params->dpb;
> struct vb2_queue *cap_q = &m2m_ctx->cap_q_ctx.q;
> - int buf_idx = -1;
> -
> - if (dpb[dpb_idx].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE)
> - buf_idx = vb2_find_timestamp(cap_q,
> - dpb[dpb_idx].reference_ts, 0);
> + int buf_idx = run->ref_buf_idx[dpb_idx];
>
> /*
> * If a DPB entry is unused or invalid, address of current destination
> @@ -1102,6 +1116,7 @@ static int rkvdec_h264_run(struct rkvdec_ctx *ctx)
>
> assemble_hw_scaling_list(ctx, &run);
> assemble_hw_pps(ctx, &run);
> + lookup_ref_buf_idx(ctx, &run);
> assemble_hw_rps(ctx, &run);
> config_registers(ctx, &run);
>
> --
> 2.34.1
>
Le samedi 02 avril 2022 à 08:16 -0300, Ezequiel Garcia a écrit :
> On Thu, Mar 31, 2022 at 03:37:15PM -0400, Nicolas Dufresne wrote:
> > The ref builder only provided references that are marked as valid in the
> > dpb. Thus the current implementation of dpb_valid would always set the
> > flag to 1. This is not representing missing frames (this is called
> > 'non-existing' pictures in the spec). In some context, these non-existing
> > pictures still need to occupy a slot in the reference list according to
> > the spec.
> >
>
> Good catch! OOC, did you find this because it was failing a test vector?
The effect is complex, so I could not correlate to specific tests. Also, what I
wanted to fix isn't covered by the ITU conformance, its mostly resiliance
requirement. But this should remove some of the IOMMU fault on broken streams
and make it less likely to use references that don't exists or aren't set what
we expect. After this change, the driver was getting more stable, and results
was also more reproducible (specially in parallel decode case, which I use to
speed up testing).
>
> > Signed-off-by: Nicolas Dufresne <[email protected]>
> > Reviewed-by: Sebastian Fricke <[email protected]>
>
> Fixes: cd33c830448ba ("media: rkvdec: Add the rkvdec driver")
> Reviewed-by: Ezequiel Garcia <[email protected]>
Thanks for the review.
>
> Thanks,
> Ezequiel
>
> > ---
> > drivers/staging/media/rkvdec/rkvdec-h264.c | 33 ++++++++++++++++------
> > 1 file changed, 24 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
> > index dff89732ddd0..bcde37d72244 100644
> > --- a/drivers/staging/media/rkvdec/rkvdec-h264.c
> > +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
> > @@ -112,6 +112,7 @@ struct rkvdec_h264_run {
> > const struct v4l2_ctrl_h264_sps *sps;
> > const struct v4l2_ctrl_h264_pps *pps;
> > const struct v4l2_ctrl_h264_scaling_matrix *scaling_matrix;
> > + int ref_buf_idx[V4L2_H264_NUM_DPB_ENTRIES];
> > };
> >
> > struct rkvdec_h264_ctx {
> > @@ -725,6 +726,26 @@ static void assemble_hw_pps(struct rkvdec_ctx *ctx,
> > }
> > }
> >
> > +static void lookup_ref_buf_idx(struct rkvdec_ctx *ctx,
> > + struct rkvdec_h264_run *run)
> > +{
> > + const struct v4l2_ctrl_h264_decode_params *dec_params = run->decode_params;
> > + u32 i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(dec_params->dpb); i++) {
> > + struct v4l2_m2m_ctx *m2m_ctx = ctx->fh.m2m_ctx;
> > + const struct v4l2_h264_dpb_entry *dpb = run->decode_params->dpb;
> > + struct vb2_queue *cap_q = &m2m_ctx->cap_q_ctx.q;
> > + int buf_idx = -1;
> > +
> > + if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE)
> > + buf_idx = vb2_find_timestamp(cap_q,
> > + dpb[i].reference_ts, 0);
> > +
> > + run->ref_buf_idx[i] = buf_idx;
> > + }
> > +}
> > +
> > static void assemble_hw_rps(struct rkvdec_ctx *ctx,
> > struct rkvdec_h264_run *run)
> > {
> > @@ -762,7 +783,7 @@ static void assemble_hw_rps(struct rkvdec_ctx *ctx,
> >
> > for (j = 0; j < RKVDEC_NUM_REFLIST; j++) {
> > for (i = 0; i < h264_ctx->reflists.num_valid; i++) {
> > - u8 dpb_valid = 0;
> > + bool dpb_valid = run->ref_buf_idx[i] >= 0;
> > u8 idx = 0;
> >
> > switch (j) {
> > @@ -779,8 +800,6 @@ static void assemble_hw_rps(struct rkvdec_ctx *ctx,
> >
> > if (idx >= ARRAY_SIZE(dec_params->dpb))
> > continue;
> > - dpb_valid = !!(dpb[idx].flags &
> > - V4L2_H264_DPB_ENTRY_FLAG_ACTIVE);
> >
> > set_ps_field(hw_rps, DPB_INFO(i, j),
> > idx | dpb_valid << 4);
> > @@ -859,13 +878,8 @@ get_ref_buf(struct rkvdec_ctx *ctx, struct rkvdec_h264_run *run,
> > unsigned int dpb_idx)
> > {
> > struct v4l2_m2m_ctx *m2m_ctx = ctx->fh.m2m_ctx;
> > - const struct v4l2_h264_dpb_entry *dpb = run->decode_params->dpb;
> > struct vb2_queue *cap_q = &m2m_ctx->cap_q_ctx.q;
> > - int buf_idx = -1;
> > -
> > - if (dpb[dpb_idx].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE)
> > - buf_idx = vb2_find_timestamp(cap_q,
> > - dpb[dpb_idx].reference_ts, 0);
> > + int buf_idx = run->ref_buf_idx[dpb_idx];
> >
> > /*
> > * If a DPB entry is unused or invalid, address of current destination
> > @@ -1102,6 +1116,7 @@ static int rkvdec_h264_run(struct rkvdec_ctx *ctx)
> >
> > assemble_hw_scaling_list(ctx, &run);
> > assemble_hw_pps(ctx, &run);
> > + lookup_ref_buf_idx(ctx, &run);
> > assemble_hw_rps(ctx, &run);
> > config_registers(ctx, &run);
> >
> > --
> > 2.34.1
> >
On Tue, Apr 5, 2022 at 12:11 PM Nicolas Dufresne
<[email protected]> wrote:
>
> Le samedi 02 avril 2022 à 08:16 -0300, Ezequiel Garcia a écrit :
> > On Thu, Mar 31, 2022 at 03:37:15PM -0400, Nicolas Dufresne wrote:
> > > The ref builder only provided references that are marked as valid in the
> > > dpb. Thus the current implementation of dpb_valid would always set the
> > > flag to 1. This is not representing missing frames (this is called
> > > 'non-existing' pictures in the spec). In some context, these non-existing
> > > pictures still need to occupy a slot in the reference list according to
> > > the spec.
> > >
> >
> > Good catch! OOC, did you find this because it was failing a test vector?
>
> The effect is complex, so I could not correlate to specific tests. Also, what I
> wanted to fix isn't covered by the ITU conformance, its mostly resiliance
> requirement. But this should remove some of the IOMMU fault on broken streams
> and make it less likely to use references that don't exists or aren't set what
> we expect. After this change, the driver was getting more stable, and results
> was also more reproducible (specially in parallel decode case, which I use to
> speed up testing).
>
Thanks for the details. This sounds like something that could
be added to the commit description itself.
> >
> > > Signed-off-by: Nicolas Dufresne <[email protected]>
> > > Reviewed-by: Sebastian Fricke <[email protected]>
> >
> > Fixes: cd33c830448ba ("media: rkvdec: Add the rkvdec driver")
> > Reviewed-by: Ezequiel Garcia <[email protected]>
>
> Thanks for the review.
>
No problem :)
> >
> > Thanks,
> > Ezequiel
> >
> > > ---
> > > drivers/staging/media/rkvdec/rkvdec-h264.c | 33 ++++++++++++++++------
> > > 1 file changed, 24 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
> > > index dff89732ddd0..bcde37d72244 100644
> > > --- a/drivers/staging/media/rkvdec/rkvdec-h264.c
> > > +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
> > > @@ -112,6 +112,7 @@ struct rkvdec_h264_run {
> > > const struct v4l2_ctrl_h264_sps *sps;
> > > const struct v4l2_ctrl_h264_pps *pps;
> > > const struct v4l2_ctrl_h264_scaling_matrix *scaling_matrix;
> > > + int ref_buf_idx[V4L2_H264_NUM_DPB_ENTRIES];
> > > };
> > >
> > > struct rkvdec_h264_ctx {
> > > @@ -725,6 +726,26 @@ static void assemble_hw_pps(struct rkvdec_ctx *ctx,
> > > }
> > > }
> > >
> > > +static void lookup_ref_buf_idx(struct rkvdec_ctx *ctx,
> > > + struct rkvdec_h264_run *run)
> > > +{
> > > + const struct v4l2_ctrl_h264_decode_params *dec_params = run->decode_params;
> > > + u32 i;
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(dec_params->dpb); i++) {
> > > + struct v4l2_m2m_ctx *m2m_ctx = ctx->fh.m2m_ctx;
> > > + const struct v4l2_h264_dpb_entry *dpb = run->decode_params->dpb;
> > > + struct vb2_queue *cap_q = &m2m_ctx->cap_q_ctx.q;
> > > + int buf_idx = -1;
> > > +
> > > + if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE)
> > > + buf_idx = vb2_find_timestamp(cap_q,
> > > + dpb[i].reference_ts, 0);
> > > +
> > > + run->ref_buf_idx[i] = buf_idx;
> > > + }
> > > +}
> > > +
> > > static void assemble_hw_rps(struct rkvdec_ctx *ctx,
> > > struct rkvdec_h264_run *run)
> > > {
> > > @@ -762,7 +783,7 @@ static void assemble_hw_rps(struct rkvdec_ctx *ctx,
> > >
> > > for (j = 0; j < RKVDEC_NUM_REFLIST; j++) {
> > > for (i = 0; i < h264_ctx->reflists.num_valid; i++) {
> > > - u8 dpb_valid = 0;
> > > + bool dpb_valid = run->ref_buf_idx[i] >= 0;
> > > u8 idx = 0;
> > >
> > > switch (j) {
> > > @@ -779,8 +800,6 @@ static void assemble_hw_rps(struct rkvdec_ctx *ctx,
> > >
> > > if (idx >= ARRAY_SIZE(dec_params->dpb))
> > > continue;
> > > - dpb_valid = !!(dpb[idx].flags &
> > > - V4L2_H264_DPB_ENTRY_FLAG_ACTIVE);
> > >
> > > set_ps_field(hw_rps, DPB_INFO(i, j),
> > > idx | dpb_valid << 4);
> > > @@ -859,13 +878,8 @@ get_ref_buf(struct rkvdec_ctx *ctx, struct rkvdec_h264_run *run,
> > > unsigned int dpb_idx)
> > > {
> > > struct v4l2_m2m_ctx *m2m_ctx = ctx->fh.m2m_ctx;
> > > - const struct v4l2_h264_dpb_entry *dpb = run->decode_params->dpb;
> > > struct vb2_queue *cap_q = &m2m_ctx->cap_q_ctx.q;
> > > - int buf_idx = -1;
> > > -
> > > - if (dpb[dpb_idx].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE)
> > > - buf_idx = vb2_find_timestamp(cap_q,
> > > - dpb[dpb_idx].reference_ts, 0);
> > > + int buf_idx = run->ref_buf_idx[dpb_idx];
> > >
> > > /*
> > > * If a DPB entry is unused or invalid, address of current destination
> > > @@ -1102,6 +1116,7 @@ static int rkvdec_h264_run(struct rkvdec_ctx *ctx)
> > >
> > > assemble_hw_scaling_list(ctx, &run);
> > > assemble_hw_pps(ctx, &run);
> > > + lookup_ref_buf_idx(ctx, &run);
> > > assemble_hw_rps(ctx, &run);
> > > config_registers(ctx, &run);
> > >
> > > --
> > > 2.34.1
> > >
>