From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id CB7C3EB64D9 for ; Wed, 12 Jul 2023 10:48:35 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=eEJdrf6q7TBNWtE8ezpGw1SReF+lQCzLtAaCZ/cAtBQ=; b=H75ojX99vtHKhp RGqTwoSrc12raEF7P0LU645NuSa/WkfboXKSLBlEk1pkiu/f6MpxgQjZbxOa+DPLVcIzPYP6IGnxB 71SyKHeU/mKyvyhlq7wCuw4rDUExOTubOgNOEkv61XHHhFZITgNqrAiNDwA58o1lXikEvXUpdhbae 3G9SePIWGhe+g9rjQrFQSgQRVOsL3a6ADZX15J7ynXpawRv7wGGPPvCYR7ci8GNkP2UsnUVG47f2L WHIKdvZjnS0uUSNQx66Ov28bBD1z2JPf7q9/eVYp2GU0BTs1KRhZ11ffW1cId1qT2l8U6tpe79R+f 3pks7fYtFJkb5tukQCHw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qJXON-00HK9V-03; Wed, 12 Jul 2023 10:48:19 +0000 Received: from mail-pl1-x62d.google.com ([2607:f8b0:4864:20::62d]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1qJXOH-00HK6u-2W for linux-rockchip@lists.infradead.org; Wed, 12 Jul 2023 10:48:17 +0000 Received: by mail-pl1-x62d.google.com with SMTP id d9443c01a7336-1b852785a65so4240955ad.0 for ; Wed, 12 Jul 2023 03:48:07 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; t=1689158887; x=1691750887; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:from:to :cc:subject:date:message-id:reply-to; bh=zIVoN8xJJ1apuh5u3RRjUDBxovBp1snddoUz1sWtVUc=; b=BMvGqPrzLoLp9tbS8Yi6csf86apIKKPA5HWYXK9DYb8XNFkNsvZ0WDcihp4d25hQ11 Kp4M7rgCghETeCFFVfSft7bWcb5rqBRLKR8vDgX2JBSsSWdZ+NwoRbLyRT0cAW48o5tx k8VWkqL4n8vyampu/AnPvTgSXMGnRjNu6zCuA= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1689158887; x=1691750887; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=zIVoN8xJJ1apuh5u3RRjUDBxovBp1snddoUz1sWtVUc=; b=eDRAgD5j3pSkoIEYOuFHzHEFSvpr0fY7VY/ksl6CyRbAgcJLd+YIDtzPhlSugYlN9G wwnLo4IdIb7iJ/+VmXKSLAP5rxf+UUuvaz3bEGQyn6kzwCvRqjl9QF+R0BBE6VCilKpa LIP88AwemHCifGLhXRlD4/KaZ+dZgcfhTSU3VBAh01PAEuz1tknqYyTYTJty1V0c5YrZ Gn+wYNCWoIvBlkC0IEBzfOH+Q6vjQ1QaMAdJrxfWBKG7xskiplgxIZFFtvc6OBqu1SIs n9+fFwzThD3PNpCCG0MGEkHEj/celLXXcKou6mnIwkH8omxHfBIRcXpRV1f7cOzh0rq/ OZCA== X-Gm-Message-State: ABy/qLadcWv0PGWbaT4xyGl9PcGC8tkN0MRSiQX/twjyhFvAhmWSft+f /lcPx1bIgIAV7uNdlzAAcPkg9A== X-Google-Smtp-Source: APBJJlEAi57Syr+e1VCEgUMbZ1p6tYBNaJWe5nyxeKIdojZAG0t41NYxzQsL8d3LNtrPXXR9NgKs8g== X-Received: by 2002:a17:902:eccf:b0:1b8:5a42:5145 with SMTP id a15-20020a170902eccf00b001b85a425145mr1488788plh.31.1689158887019; Wed, 12 Jul 2023 03:48:07 -0700 (PDT) Received: from chromium.org (0.223.81.34.bc.googleusercontent.com. [34.81.223.0]) by smtp.gmail.com with ESMTPSA id io12-20020a17090312cc00b001b9dfa24523sm3622055plb.213.2023.07.12.03.48.03 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 12 Jul 2023 03:48:06 -0700 (PDT) Date: Wed, 12 Jul 2023 10:48:01 +0000 From: Tomasz Figa To: Hsia-Jun Li Cc: Benjamin Gaignard , linux-media@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, m.szyprowski@samsung.com, linux-mediatek@lists.infradead.org, linux-arm-msm@vger.kernel.org, hverkuil-cisco@xs4all.nl, ezequiel@vanguardiasur.com.ar, p.zabel@pengutronix.de, linux-rockchip@lists.infradead.org, mchehab@kernel.org, linux-staging@lists.linux.dev, ming.qian@nxp.com, kernel@collabora.com, gregkh@linuxfoundation.org, nicolas.dufresne@collabora.com Subject: Re: [PATCH v3 04/11] media: videobuf2: Stop define VB2_MAX_FRAME as global Message-ID: <20230712104801.tgawhexpm53ocgd6@chromium.org> References: <20230622131349.144160-1-benjamin.gaignard@collabora.com> <20230622131349.144160-5-benjamin.gaignard@collabora.com> <5cb3f216-5041-a155-5d2c-059dc1f15024@collabora.com> <25b21252-0d3a-3e50-0012-57055f386fee@synaptics.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <25b21252-0d3a-3e50-0012-57055f386fee@synaptics.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230712_034813_834537_77F18350 X-CRM114-Status: GOOD ( 38.60 ) X-BeenThere: linux-rockchip@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Upstream kernel work for Rockchip platforms List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Sender: "Linux-rockchip" Errors-To: linux-rockchip-bounces+linux-rockchip=archiver.kernel.org@lists.infradead.org On Mon, Jul 03, 2023 at 04:35:30PM +0800, Hsia-Jun Li wrote: > = > On 7/3/23 16:09, Benjamin Gaignard wrote: > > CAUTION: Email originated externally, do not click links or open > > attachments unless you recognize the sender and know the content is > > safe. > > = > > = > > Le 30/06/2023 =E0 11:51, Hsia-Jun Li a =E9crit : > > > = > > > On 6/22/23 21:13, Benjamin Gaignard wrote: > > > > CAUTION: Email originated externally, do not click links or open > > > > attachments unless you recognize the sender and know the content is > > > > safe. > > > > = > > > > = > > > > After changing bufs arrays to a dynamic allocated array > > > > VB2_MAX_FRAME doesn't mean anything for videobuf2 core. > > > = > > > I think make it 64 which is the VB2_MAX_FRAME in Android GKI kernel is > > > more reasonable. > > > = > > > It would be hard to iterate the whole array, it would go worse with a > > > filter. Such iterate may need to go twice because you mix > > > post-processing buffer and decoding buffer(with MV) in the same array. > > = > > Here I don't want to change drivers behavior so I keep the same value. > > If it happens that they need more buffers, like for dynamic resolution > > change > > feature for Verisilicon VP9 decoder, case by case patches will be neede= d. > > = > I just don't like the idea that using a variant length array here. > = "I don't like" is not an argument. We had a number of arguments for using a generic helper (originally idr, but later decided to go with XArray, because the former is now deprecated) that we pointed out in our review comments for previous revisions. It wasn't really about the size being variable, but rather avoiding open coding things in vb2 and duplicating what's already implemented in generic code. > And I could explain why you won't need so many buffers for the performance > of decoding. > = > VP9 could support 10 reference frames in dpb. > = > Even for those frequent resolution changing test set, it would only happen > to two resolutions, > = > 32 would be enough for 20 buffers of two resolution plus golden frames. It > also leaves enough slots for re-order latency. > = > If your case had more two resolutions, likes low->medium->high. > = > I would suggest just skip the medium resolutions, just allocate the lower > one first for fast playback then the highest for all the possible > = > medium cases. Reallocation happens frequently would only cause memory > fragment, nothing benefits your performance. > = We have mechanisms in the kernel to deal with memory fragmentation (migration/compaction) and it would still only matters for the pathologic cases of hardware that require physically contiguous memory. Modern hardware with proper DMA capabilities can either scatter-gather or are equipped with an IOMMU, so the allocation always happens in page granularity and fragmentation is avoided. Best regards, Tomasz > > = > > > = > > > > Remove it from the core definitions but keep it for drivers internal > > > > needs. > > > > = > > > > Signed-off-by: Benjamin Gaignard > > > > --- > > > > =A0 drivers/media/common/videobuf2/videobuf2-core.c | 2 ++ > > > > =A0 drivers/media/platform/amphion/vdec.c | 1 + > > > > .../media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c | 2 ++ > > > > =A0 drivers/media/platform/qcom/venus/hfi.h | 2 ++ > > > > =A0 drivers/media/platform/verisilicon/hantro_hw.h | 2 ++ > > > > =A0 drivers/staging/media/ipu3/ipu3-v4l2.c | 2 ++ > > > > =A0 include/media/videobuf2-core.h | 1 - > > > > =A0 include/media/videobuf2-v4l2.h | 4 ---- > > > > =A0 8 files changed, 11 insertions(+), 5 deletions(-) > > > > = > > > > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c > > > > b/drivers/media/common/videobuf2/videobuf2-core.c > > > > index 86e1e926fa45..899783f67580 100644 > > > > --- a/drivers/media/common/videobuf2/videobuf2-core.c > > > > +++ b/drivers/media/common/videobuf2/videobuf2-core.c > > > > @@ -31,6 +31,8 @@ > > > > = > > > > =A0 #include > > > > = > > > > +#define VB2_MAX_FRAME=A0 32 > > > > + > > > > =A0 static int debug; > > > > =A0 module_param(debug, int, 0644); > > > > = > > > > diff --git a/drivers/media/platform/amphion/vdec.c > > > > b/drivers/media/platform/amphion/vdec.c > > > > index 3fa1a74a2e20..b3219f6d17fa 100644 > > > > --- a/drivers/media/platform/amphion/vdec.c > > > > +++ b/drivers/media/platform/amphion/vdec.c > > > > @@ -28,6 +28,7 @@ > > > > = > > > > =A0 #define VDEC_MIN_BUFFER_CAP=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 8 > > > > =A0 #define VDEC_MIN_BUFFER_OUT=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 8 > > > > +#define VB2_MAX_FRAME=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0 32 > > > > = > > > > =A0 struct vdec_fs_info { > > > > =A0=A0=A0=A0=A0=A0=A0=A0 char name[8]; > > > > diff --git > > > > a/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c > > > > b/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c > > > > index 6532a69f1fa8..a1e0f24bb91c 100644 > > > > --- a/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_= if.c > > > > +++ b/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_= if.c > > > > @@ -16,6 +16,8 @@ > > > > =A0 #include "../vdec_drv_if.h" > > > > =A0 #include "../vdec_vpu_if.h" > > > > = > > > > +#define VB2_MAX_FRAME=A0 32 > > > > + > > > > =A0 /* reset_frame_context defined in VP9 spec */ > > > > =A0 #define VP9_RESET_FRAME_CONTEXT_NONE0 0 > > > > =A0 #define VP9_RESET_FRAME_CONTEXT_NONE1 1 > > > > diff --git a/drivers/media/platform/qcom/venus/hfi.h > > > > b/drivers/media/platform/qcom/venus/hfi.h > > > > index f25d412d6553..bd5ca5a8b945 100644 > > > > --- a/drivers/media/platform/qcom/venus/hfi.h > > > > +++ b/drivers/media/platform/qcom/venus/hfi.h > > > > @@ -10,6 +10,8 @@ > > > > = > > > > =A0 #include "hfi_helper.h" > > > > = > > > > +#define VB2_MAX_FRAME=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 32 > > > > + > > > > =A0 #define VIDC_SESSION_TYPE_VPE=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0 0 > > > > =A0 #define VIDC_SESSION_TYPE_ENC=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0 1 > > > > =A0 #define VIDC_SESSION_TYPE_DEC=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0 2 > > > > diff --git a/drivers/media/platform/verisilicon/hantro_hw.h > > > > b/drivers/media/platform/verisilicon/hantro_hw.h > > > > index e83f0c523a30..9e8faf7ba6fb 100644 > > > > --- a/drivers/media/platform/verisilicon/hantro_hw.h > > > > +++ b/drivers/media/platform/verisilicon/hantro_hw.h > > > > @@ -15,6 +15,8 @@ > > > > =A0 #include > > > > =A0 #include > > > > = > > > > +#define VB2_MAX_FRAME=A0 32 > > > > + > > > > =A0 #define DEC_8190_ALIGN_MASK=A0=A0=A0 0x07U > > > > = > > > > =A0 #define MB_DIM=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 = 16 > > > > diff --git a/drivers/staging/media/ipu3/ipu3-v4l2.c > > > > b/drivers/staging/media/ipu3/ipu3-v4l2.c > > > > index e530767e80a5..6627b5c2d4d6 100644 > > > > --- a/drivers/staging/media/ipu3/ipu3-v4l2.c > > > > +++ b/drivers/staging/media/ipu3/ipu3-v4l2.c > > > > @@ -10,6 +10,8 @@ > > > > =A0 #include "ipu3.h" > > > > =A0 #include "ipu3-dmamap.h" > > > > = > > > > +#define VB2_MAX_FRAME=A0 32 > > > > + > > > > =A0 /******************** v4l2_subdev_ops ********************/ > > > > = > > > > =A0 #define IPU3_RUNNING_MODE_VIDEO=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0 0 > > > > diff --git a/include/media/videobuf2-core.h > > > > b/include/media/videobuf2-core.h > > > > index 77921cf894ef..080b783d608d 100644 > > > > --- a/include/media/videobuf2-core.h > > > > +++ b/include/media/videobuf2-core.h > > > > @@ -20,7 +20,6 @@ > > > > =A0 #include > > > > =A0 #include > > > > = > > > > -#define VB2_MAX_FRAME=A0 (32) > > > > =A0 #define VB2_MAX_PLANES (8) > > > > = > > > > =A0 /** > > > > diff --git a/include/media/videobuf2-v4l2.h > > > > b/include/media/videobuf2-v4l2.h > > > > index 5a845887850b..88a7a565170e 100644 > > > > --- a/include/media/videobuf2-v4l2.h > > > > +++ b/include/media/videobuf2-v4l2.h > > > > @@ -15,10 +15,6 @@ > > > > =A0 #include > > > > =A0 #include > > > > = > > > > -#if VB2_MAX_FRAME !=3D VIDEO_MAX_FRAME > > > > -#error VB2_MAX_FRAME !=3D VIDEO_MAX_FRAME > > > > -#endif > > > > - > > > > =A0 #if VB2_MAX_PLANES !=3D VIDEO_MAX_PLANES > > > > =A0 #error VB2_MAX_PLANES !=3D VIDEO_MAX_PLANES > > > > =A0 #endif > > > > -- = > > > > 2.39.2 > > > > = > -- = > Hsia-Jun(Randy) Li > = _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip