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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id D42ADC4332F for ; Mon, 10 Oct 2022 06:14:28 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229854AbiJJGO1 (ORCPT ); Mon, 10 Oct 2022 02:14:27 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:39266 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231218AbiJJGOX (ORCPT ); Mon, 10 Oct 2022 02:14:23 -0400 Received: from mailgw01.mediatek.com (unknown [60.244.123.138]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id D917753019; Sun, 9 Oct 2022 23:14:15 -0700 (PDT) X-UUID: 173974fa39a744a5aebac5de95bd5301-20221010 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=mediatek.com; s=dk; h=Content-Transfer-Encoding:MIME-Version:Content-Type:References:In-Reply-To:Date:CC:To:From:Subject:Message-ID; bh=c+o2Ttaw46ADrlswqu08VV5Sy7+w8Nx1fLn1x4kEvnE=; b=FFZ2B5/JndCCr/8bCekK+CDlj0Wu0ZT9KJ2Nspgr3/mOqOFFUBRenS9RAGpxBfH5+lbyJxX5B3q2tMeymiiJkA9JctfU6g9qAuj2r3QrvEQfUA855e17Hxq6jBjTnnRxO/ctfbcfo+z6UKyyBaLEndmTCrYrYjQWYsjXtGpAfl8=; X-CID-P-RULE: Release_Ham X-CID-O-INFO: VERSION:1.1.11,REQID:d9133e50-263b-4867-9596-63cd523aa2d0,IP:0,U RL:0,TC:0,Content:-5,EDM:0,RT:0,SF:0,FILE:0,BULK:0,RULE:Release_Ham,ACTION :release,TS:-5 X-CID-META: VersionHash:39a5ff1,CLOUDID:cd7cbffe-ee8c-4ff7-afe9-644435e96625,B ulkID:nil,BulkQuantity:0,Recheck:0,SF:102,TC:nil,Content:0,EDM:-3,IP:nil,U RL:0,File:nil,Bulk:nil,QS:nil,BEC:nil,COL:0 X-UUID: 173974fa39a744a5aebac5de95bd5301-20221010 Received: from mtkmbs11n1.mediatek.inc [(172.21.101.185)] by mailgw01.mediatek.com (envelope-from ) (Generic MTA with TLSv1.2 ECDHE-RSA-AES256-GCM-SHA384 256/256) with ESMTP id 243472699; Mon, 10 Oct 2022 14:14:08 +0800 Received: from mtkmbs11n1.mediatek.inc (172.21.101.185) by mtkmbs10n1.mediatek.inc (172.21.101.34) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.792.15; Mon, 10 Oct 2022 14:14:07 +0800 Received: from mhfsdcap04 (10.17.3.154) by mtkmbs11n1.mediatek.inc (172.21.101.73) with Microsoft SMTP Server id 15.2.792.15 via Frontend Transport; Mon, 10 Oct 2022 14:14:05 +0800 Message-ID: <07b1b0e292eaa71201d5f7e169325638231a3159.camel@mediatek.com> Subject: Re: [PATCH, v2] media: mediatek: vcodec: Add to support VP9 inner racing mode From: "mingjia.zhang@mediatek.com" To: Hans Verkuil , Yunfei Dong , Alexandre Courbot , Nicolas Dufresne , AngeloGioacchino Del Regno , Benjamin Gaignard , Tiffany Lin , Andrew-CT Chen , Mauro Carvalho Chehab , Rob Herring , Matthias Brugger , Tomasz Figa CC: Irui Wang , George Sun , Steve Cho , , , , dri-devel , Xiaoyong Lu , , Hsin-Yi Wang , Fritz Koenig , , Date: Mon, 10 Oct 2022 14:14:05 +0800 In-Reply-To: <27263bbf-10d0-174d-38b4-5d6b0a6bc9bd@xs4all.nl> References: <20220727061310.2307-1-mingjia.zhang@mediatek.com> <27263bbf-10d0-174d-38b4-5d6b0a6bc9bd@xs4all.nl> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.28.5-0ubuntu0.18.04.2 MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-MTK: N Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Hans, Thanks for you reply and useful comments. After reviewing the error handling in vdec_vp9_slice_core_decode(), I found that the problem you pointed out does exist in error handle part. 'instance' may be used uninitialized in this function. I have fixed this build warning and reviewed other code, thanks. Thanks, mingjia On Wed, 2022-08-24 at 15:02 +0200, Hans Verkuil wrote: > Hi Mingjia, > > On 27/07/2022 08:13, Mingjia Zhang wrote: > > In order to reduce decoder latency, enable VP9 inner racing mode. > > Send lat trans buffer information to core when trigger lat to work, > > need not to wait until lat decode done. > > > > Signed-off-by: mingjia zhang > > I'm getting this compile warning: > > drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c: In > function 'vdec_vp9_slice_core_decode': > drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c:221 > 8:50: warning: 'instance' may be used uninitialized in this function > [-Wmaybe-uninitialized] > 2218 | if (IS_VDEC_INNER_RACING(instance->ctx->dev- > >dec_capability)) > | ^~ > > I think you need to take a close look at the error handling in > vdec_vp9_slice_core_decode(). > > After each error there is a 'goto err;' and that will run the new > code, and that doesn't > feel right. > > Regards, > > Hans > > > --- > > 1. CTS/GTS test pass > > 2. Fluster result: Ran 240/303 tests successfully > > --- > > .../vcodec/vdec/vdec_vp9_req_lat_if.c | 64 ++++++++++++--- > > ---- > > 1 file changed, 40 insertions(+), 24 deletions(-) > > > > 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 fb1c36a3592d..92b47f0fdf40 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 > > @@ -436,6 +436,7 @@ struct vdec_vp9_slice_ref { > > * @frame_ctx: 4 frame context according to VP9 Spec > > * @frame_ctx_helper: 4 frame context according to newest > > kernel spec > > * @dirty: state of each frame context > > + * @local_vsi: local instance vsi information > > * @init_vsi: vsi used for initialized VP9 instance > > * @vsi: vsi used for decoding/flush ... > > * @core_vsi: vsi used for Core stage > > @@ -482,6 +483,8 @@ struct vdec_vp9_slice_instance { > > struct v4l2_vp9_frame_context frame_ctx_helper; > > unsigned char dirty[4]; > > > > + struct vdec_vp9_slice_vsi local_vsi; > > + > > /* MicroP vsi */ > > union { > > struct vdec_vp9_slice_init_vsi *init_vsi; > > @@ -1616,16 +1619,10 @@ static int > > vdec_vp9_slice_update_single(struct vdec_vp9_slice_instance > > *instance > > } > > > > static int vdec_vp9_slice_update_lat(struct > > vdec_vp9_slice_instance *instance, > > - struct vdec_lat_buf *lat_buf, > > - struct vdec_vp9_slice_pfc *pfc) > > + struct vdec_vp9_slice_vsi *vsi) > > { > > - struct vdec_vp9_slice_vsi *vsi; > > - > > - vsi = &pfc->vsi; > > - memcpy(&pfc->state[0], &vsi->state, sizeof(vsi->state)); > > - > > mtk_vcodec_debug(instance, "Frame %u LAT CRC 0x%08x %lx %lx\n", > > - pfc->seq, vsi->state.crc[0], > > + (instance->seq - 1), vsi->state.crc[0], > > (unsigned long)vsi->trans.dma_addr, > > (unsigned long)vsi->trans.dma_addr_end); > > > > @@ -2090,6 +2087,13 @@ static int vdec_vp9_slice_lat_decode(void > > *h_vdec, struct mtk_vcodec_mem *bs, > > return ret; > > } > > > > + if (IS_VDEC_INNER_RACING(instance->ctx->dev->dec_capability)) { > > + vdec_vp9_slice_vsi_from_remote(vsi, instance->vsi, 0); > > + memcpy(&instance->local_vsi, vsi, sizeof(*vsi)); > > + vdec_msg_queue_qbuf(&ctx->dev->msg_queue_core_ctx, > > lat_buf); > > + vsi = &instance->local_vsi; > > + } > > + > > if (instance->irq) { > > ret = mtk_vcodec_wait_for_done_ctx(ctx, MTK_INST_IR > > Q_RECEIVED, > > WAIT_INTR_TIMEOUT_MS > > , MTK_VDEC_LAT0); > > @@ -2102,22 +2106,25 @@ static int vdec_vp9_slice_lat_decode(void > > *h_vdec, struct mtk_vcodec_mem *bs, > > } > > > > vdec_vp9_slice_vsi_from_remote(vsi, instance->vsi, 0); > > - ret = vdec_vp9_slice_update_lat(instance, lat_buf, pfc); > > + ret = vdec_vp9_slice_update_lat(instance, vsi); > > > > - /* LAT trans full, no more UBE or decode timeout */ > > - if (ret) { > > - mtk_vcodec_err(instance, "VP9 decode error: %d\n", > > ret); > > - return ret; > > - } > > + if (!IS_VDEC_INNER_RACING(instance->ctx->dev->dec_capability)) > > + /* LAT trans full, no more UBE or decode timeout */ > > + if (ret) { > > + mtk_vcodec_err(instance, "frame[%d] decode > > error: %d\n", > > + ret, (instance->seq - 1)); > > + return ret; > > + } > > > > - mtk_vcodec_debug(instance, "lat dma addr: 0x%lx 0x%lx\n", > > - (unsigned long)pfc->vsi.trans.dma_addr, > > - (unsigned long)pfc->vsi.trans.dma_addr_end); > > > > - vdec_msg_queue_update_ube_wptr(&ctx->msg_queue, > > - vsi->trans.dma_addr_end + > > - ctx- > > >msg_queue.wdma_addr.dma_addr); > > - vdec_msg_queue_qbuf(&ctx->dev->msg_queue_core_ctx, lat_buf); > > + vsi->trans.dma_addr_end += ctx->msg_queue.wdma_addr.dma_addr; > > + vdec_msg_queue_update_ube_wptr(&ctx->msg_queue, vsi- > > >trans.dma_addr_end); > > + if (!IS_VDEC_INNER_RACING(instance->ctx->dev->dec_capability)) > > + vdec_msg_queue_qbuf(&ctx->dev->msg_queue_core_ctx, > > lat_buf); > > + > > + mtk_vcodec_debug(instance, "lat trans end addr(0x%lx), ube > > start addr(0x%lx)\n", > > + (unsigned long)vsi->trans.dma_addr_end, > > + (unsigned long)ctx- > > >msg_queue.wdma_addr.dma_addr); > > > > return 0; > > } > > @@ -2193,10 +2200,14 @@ static int > > vdec_vp9_slice_core_decode(struct vdec_lat_buf *lat_buf) > > goto err; > > } > > > > - pfc->vsi.trans.dma_addr_end += ctx- > > >msg_queue.wdma_addr.dma_addr; > > mtk_vcodec_debug(instance, "core dma_addr_end 0x%lx\n", > > (unsigned long)pfc->vsi.trans.dma_addr_end); > > - vdec_msg_queue_update_ube_rptr(&ctx->msg_queue, pfc- > > >vsi.trans.dma_addr_end); > > + > > + if (IS_VDEC_INNER_RACING(instance->ctx->dev->dec_capability)) > > + vdec_msg_queue_update_ube_rptr(&ctx->msg_queue, pfc- > > >vsi.trans.dma_addr); > > + else > > + vdec_msg_queue_update_ube_rptr(&ctx->msg_queue, pfc- > > >vsi.trans.dma_addr_end); > > + > > ctx->dev->vdec_pdata->cap_to_disp(ctx, 0, lat_buf- > > >src_buf_req); > > > > return 0; > > @@ -2204,7 +2215,12 @@ static int vdec_vp9_slice_core_decode(struct > > vdec_lat_buf *lat_buf) > > err: > > if (ctx && pfc) { > > /* always update read pointer */ > > - vdec_msg_queue_update_ube_rptr(&ctx->msg_queue, pfc- > > >vsi.trans.dma_addr_end); > > + if (IS_VDEC_INNER_RACING(instance->ctx->dev- > > >dec_capability)) > > + vdec_msg_queue_update_ube_rptr(&ctx->msg_queue, > > + pfc- > > >vsi.trans.dma_addr); > > + else > > + vdec_msg_queue_update_ube_rptr(&ctx->msg_queue, > > + pfc- > > >vsi.trans.dma_addr_end); > > > > if (fb) > > ctx->dev->vdec_pdata->cap_to_disp(ctx, 1, > > lat_buf->src_buf_req);