Message ID | 20220328195936.82552-22-nicolas.dufresne@collabora.com (mailing list archive) |
---|---|
State | Superseded |
Headers |
Received: from vger.kernel.org ([23.128.96.18]) by www.linuxtv.org with esmtp (Exim 4.92) (envelope-from <linux-media-owner@vger.kernel.org>) id 1nYvXv-009st7-Kv; Mon, 28 Mar 2022 20:00:59 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1344324AbiC1UCi (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Mon, 28 Mar 2022 16:02:38 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:57646 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1344319AbiC1UCV (ORCPT <rfc822;linux-media@vger.kernel.org>); Mon, 28 Mar 2022 16:02:21 -0400 Received: from bhuna.collabora.co.uk (bhuna.collabora.co.uk [IPv6:2a00:1098:0:82:1000:25:2eeb:e3e3]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id DC9F53B3C2; Mon, 28 Mar 2022 13:00:22 -0700 (PDT) Received: from [127.0.0.1] (localhost [127.0.0.1]) (Authenticated sender: nicolas) with ESMTPSA id 6107D1F438B3 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com; s=mail; t=1648497621; bh=qASp9j35h86HZHqACbW4hS8pRQcR68hjlS1LdUain6U=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=bSYBFsly9GrPfSwBI9lPzlksp9cEBsdRIyvQw2RwExw4t9cHPSfmpMTol+4t9Z2RU j3QxeXh0CDyalwcffVXx4lzqWFskW6LvhdiGU6/f/U6aoMhbw10J6KNiJlBMIt34Vf 6TdZjf7ouduq5K7Pt9Ycmpw/MAjAuQFQtlsOFIzIXiTU1unRooRT5vrlpMau3Wb9Q8 7gHQ9mYwOpa140DlGQVjpz00gfy8tTVhUXBHeGu0DK3mqncCtAmwzApZdFTOqn4dYC pfSbmR0rwINKx3pzQUCTmyLuYdhp2fXJrNk7QfryzTqKFzBp2uidnWwtBJTj42CSjg UM9DR4nZgn8GA== From: Nicolas Dufresne <nicolas.dufresne@collabora.com> To: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>, Philipp Zabel <p.zabel@pengutronix.de>, Mauro Carvalho Chehab <mchehab@kernel.org>, Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: kernel@collabora.com, linux-media@vger.kernel.org, linux-rockchip@lists.infradead.org, linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org Subject: [PATCH v1 21/24] media: hantro: Stop using H.264 parameter pic_num Date: Mon, 28 Mar 2022 15:59:33 -0400 Message-Id: <20220328195936.82552-22-nicolas.dufresne@collabora.com> X-Mailer: git-send-email 2.34.1 In-Reply-To: <20220328195936.82552-1-nicolas.dufresne@collabora.com> References: <20220328195936.82552-1-nicolas.dufresne@collabora.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,SPF_HELO_PASS,SPF_PASS, T_SCC_BODY_TEXT_LINE,UNPARSEABLE_RELAY autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org X-LSpam-Score: -2.5 (--) X-LSpam-Report: No, score=-2.5 required=5.0 tests=BAYES_00=-1.9,DKIM_SIGNED=0.1,DKIM_VALID=-0.1,DKIM_VALID_AU=-0.1,HEADER_FROM_DIFFERENT_DOMAINS=0.5,MAILING_LIST_MULTI=-1,UNPARSEABLE_RELAY=0.001 autolearn=ham autolearn_force=no |
Series |
[v1,01/24] media: h264: Increase reference lists size to 32
|
|
Commit Message
Nicolas Dufresne
March 28, 2022, 7:59 p.m. UTC
The hardware expects FrameNumWrap or long_term_frame_idx. Picture
numbers are per field, and are mostly used during the memory
management process, which is done in userland. This fixes two
ITU conformance tests:
- MR6_BT_B
- MR8_BT_B
Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
drivers/staging/media/hantro/hantro_h264.c | 2 --
1 file changed, 2 deletions(-)
Comments
Hey Nicolas, The term pic_num is now only present in the following files: ``` ❯ rg 'pic_num' staging/media/rkvdec/rkvdec-h264.c 766: * Assign an invalid pic_num if DPB entry at that position is inactive. 768: * reference picture with pic_num 0, triggering output picture media/platform/amphion/vpu_windsor.c 485: u32 pic_num; media/platform/mediatek/vcodec/vdec/vdec_h264_req_if.c 97: unsigned short pic_num; 346: dst_entry->pic_num = src_entry->pic_num; media/v4l2-core/v4l2-h264.c 143: * but with frame_num (wrapped). As for frame the pic_num and frame_num 306: /* this is pic_num for frame and frame_num (wrapped) for field, 307: * but for frame pic_num is equal to frame_num (wrapped). ``` In v4l2-h264 and rkvdec-h264 it is only present as comment and the term is not part of the specification. In vpu_windsor it is actually never used. And for the mediatek driver the same might apply. It might be worth it to get rid of that term all together while you are at it. On 28.03.2022 15:59, Nicolas Dufresne wrote: >The hardware expects FrameNumWrap or long_term_frame_idx. Picture >numbers are per field, and are mostly used during the memory >management process, which is done in userland. This fixes two >ITU conformance tests: > > - MR6_BT_B > - MR8_BT_B > >Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> Reviewed-by: Sebastian Fricke <sebastian.fricke@collabora.com> Greetings, Sebastian >--- > drivers/staging/media/hantro/hantro_h264.c | 2 -- > 1 file changed, 2 deletions(-) > >diff --git a/drivers/staging/media/hantro/hantro_h264.c b/drivers/staging/media/hantro/hantro_h264.c >index 0b4d2491be3b..228629fb3cdf 100644 >--- a/drivers/staging/media/hantro/hantro_h264.c >+++ b/drivers/staging/media/hantro/hantro_h264.c >@@ -354,8 +354,6 @@ u16 hantro_h264_get_ref_nbr(struct hantro_ctx *ctx, unsigned int dpb_idx) > > if (!(dpb->flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE)) > return 0; >- if (dpb->flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM) >- return dpb->pic_num; > return dpb->frame_num; > } > >-- >2.34.1 >
Le mercredi 30 mars 2022 à 09:42 +0200, Sebastian Fricke a écrit : > Hey Nicolas, > > The term pic_num is now only present in the following files: > ``` > ❯ rg 'pic_num' > staging/media/rkvdec/rkvdec-h264.c > 766: * Assign an invalid pic_num if DPB entry at that position is inactive. > 768: * reference picture with pic_num 0, triggering output picture I should probably translate this one, since the HW uses frame_num, not pic_num. > > media/platform/amphion/vpu_windsor.c > 485: u32 pic_num; Amphion Windsor is a stateful driver, I cannot comment on the user of pic_num for that type of driver. > > media/platform/mediatek/vcodec/vdec/vdec_h264_req_if.c > 97: unsigned short pic_num; > 346: dst_entry->pic_num = src_entry->pic_num; This is being sent to the firmware, so its a difficult change to make without testing it first. I do have HW to test this, but would prefer doing so in a seperate patchset. Note that MTK does not support field decoding, so pic_num == frame_num. So whatever it does here is likely correct. > > media/v4l2-core/v4l2-h264.c > 143: * but with frame_num (wrapped). As for frame the pic_num and frame_num > 306: /* this is pic_num for frame and frame_num (wrapped) for field, > 307: * but for frame pic_num is equal to frame_num (wrapped). > ``` > > In v4l2-h264 and rkvdec-h264 it is only present as comment and the term > is not part of the specification. > In vpu_windsor it is actually never used. > And for the mediatek driver the same might apply. > It might be worth it to get rid of that term all together while you are > at it. Amphion Windsor is a stateful driver, I'd leave it to the maintainer to cleanup unused variables if there is. In general the term is not invalid, its just that the value can be trivially deduced from frame_num and the value depends on the current picture parity, which makes it an unstable identifier. > > On 28.03.2022 15:59, Nicolas Dufresne wrote: > > The hardware expects FrameNumWrap or long_term_frame_idx. Picture > > numbers are per field, and are mostly used during the memory > > management process, which is done in userland. This fixes two > > ITU conformance tests: > > > > - MR6_BT_B > > - MR8_BT_B > > > > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > Reviewed-by: Sebastian Fricke <sebastian.fricke@collabora.com> > > Greetings, > Sebastian > > --- > > drivers/staging/media/hantro/hantro_h264.c | 2 -- > > 1 file changed, 2 deletions(-) > > > > diff --git a/drivers/staging/media/hantro/hantro_h264.c b/drivers/staging/media/hantro/hantro_h264.c > > index 0b4d2491be3b..228629fb3cdf 100644 > > --- a/drivers/staging/media/hantro/hantro_h264.c > > +++ b/drivers/staging/media/hantro/hantro_h264.c > > @@ -354,8 +354,6 @@ u16 hantro_h264_get_ref_nbr(struct hantro_ctx *ctx, unsigned int dpb_idx) > > > > if (!(dpb->flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE)) > > return 0; > > - if (dpb->flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM) > > - return dpb->pic_num; > > return dpb->frame_num; > > } > > > > -- > > 2.34.1 > >
diff --git a/drivers/staging/media/hantro/hantro_h264.c b/drivers/staging/media/hantro/hantro_h264.c index 0b4d2491be3b..228629fb3cdf 100644 --- a/drivers/staging/media/hantro/hantro_h264.c +++ b/drivers/staging/media/hantro/hantro_h264.c @@ -354,8 +354,6 @@ u16 hantro_h264_get_ref_nbr(struct hantro_ctx *ctx, unsigned int dpb_idx) if (!(dpb->flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE)) return 0; - if (dpb->flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM) - return dpb->pic_num; return dpb->frame_num; }