Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Alexandre Courbot <acourbot@chromium.org>
To:     Ezequiel Garcia <ezequiel@collabora.com>,
        Boris Brezillon <boris.brezillon@collabora.com>
Cc:     linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
        Alexandre Courbot <acourbot@chromium.org>
Subject: [PATCH] media: hantro: make update_dpb() not leave holes in DPB
Date:   Fri, 15 Nov 2019 12:50:13 +0900
Message-Id: <20191115035013.145152-1-acourbot@chromium.org>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

update_dpb() reorders the DPB entries such as the same frame in two
consecutive decoding requests always ends up in the same DPB slot.

It first clears all the active flags in the DPB, and then checks whether
the active flag is not set to decide whether an entry is a candidate for
matching or not.

However, this means that unused DPB entries are also candidates for
matching, and an unused entry will match if we are testing it against a
frame which (TopFieldOrderCount, BottomFieldOrderCount) is (0, 0).

As it turns out, this happens for the very first frame of a stream, but
it is not a problem as it would be copied to the first entry anyway.
More concerning is that after an IDR frame the Top/BottomFieldOrderCount
can be reset to 0, and this time update_dpb() will match the IDR frame
to the first unused entry of the DPB (and not the entry at index 0 as
would be expected) because the first slots will have different values.

The Hantro driver is ok with this, but when trying to use the same
function for another driver (MT8183) I noticed decoding artefacts caused
by this hole in the DPB.

Fix this by maintaining a list of DPB slots that are actually in use,
instead of relying on the absence of the active flag to do so. This also
allows us to optimize matching a bit.

Signed-off-by: Alexandre Courbot <acourbot@chromium.org>
---
 drivers/staging/media/hantro/hantro_h264.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/media/hantro/hantro_h264.c b/drivers/staging/media/hantro/hantro_h264.c
index 568640eab3a6..2357068b0f82 100644
--- a/drivers/staging/media/hantro/hantro_h264.c
+++ b/drivers/staging/media/hantro/hantro_h264.c
@@ -474,14 +474,19 @@ static void update_dpb(struct hantro_ctx *ctx)
 {
 	const struct v4l2_ctrl_h264_decode_params *dec_param;
 	DECLARE_BITMAP(new, ARRAY_SIZE(dec_param->dpb)) = { 0, };
+	DECLARE_BITMAP(in_use, ARRAY_SIZE(dec_param->dpb)) = { 0, };
 	DECLARE_BITMAP(used, ARRAY_SIZE(dec_param->dpb)) = { 0, };
 	unsigned int i, j;
 
 	dec_param = ctx->h264_dec.ctrls.decode;
 
-	/* Disable all entries by default. */
-	for (i = 0; i < ARRAY_SIZE(ctx->h264_dec.dpb); i++)
+	/* Mark entries in use before disabling them. */
+	for (i = 0; i < ARRAY_SIZE(ctx->h264_dec.dpb); i++) {
+		if (ctx->h264_dec.dpb[i].flags &
+		    V4L2_H264_DPB_ENTRY_FLAG_ACTIVE)
+			set_bit(i, in_use);
 		ctx->h264_dec.dpb[i].flags &= ~V4L2_H264_DPB_ENTRY_FLAG_ACTIVE;
+	}
 
 	/* Try to match new DPB entries with existing ones by their POCs. */
 	for (i = 0; i < ARRAY_SIZE(dec_param->dpb); i++) {
@@ -492,18 +497,19 @@ static void update_dpb(struct hantro_ctx *ctx)
 
 		/*
 		 * To cut off some comparisons, iterate only on target DPB
-		 * entries which are not used yet.
+		 * entries which are already used.
 		 */
-		for_each_clear_bit(j, used, ARRAY_SIZE(ctx->h264_dec.dpb)) {
+		for_each_set_bit(j, in_use, ARRAY_SIZE(ctx->h264_dec.dpb)) {
 			struct v4l2_h264_dpb_entry *cdpb;
 
 			cdpb = &ctx->h264_dec.dpb[j];
-			if (cdpb->flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE ||
-			    !dpb_entry_match(cdpb, ndpb))
+			if (!dpb_entry_match(cdpb, ndpb))
 				continue;
 
 			*cdpb = *ndpb;
 			set_bit(j, used);
+			/* Don't reiterate on this one. */
+			clear_bit(j, in_use);
 			break;
 		}
 
-- 
2.24.0.432.g9d3f5f5b63-goog