From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jungo Lin Subject: Re: [RFC,v3 8/9] media: platform: Add Mediatek ISP P1 SCP communication Date: Fri, 26 Jul 2019 16:07:17 +0800 Message-ID: <1564128437.1212.615.camel@mtksdccf07> References: <20190611035344.29814-1-jungo.lin@mediatek.com> <20190611035344.29814-9-jungo.lin@mediatek.com> <20190710095827.GC181405@chromium.org> <1563675513.1212.444.camel@mtksdccf07> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: Tomasz Figa Cc: devicetree@vger.kernel.org, Sean Cheng =?UTF-8?Q?=28=E9=84=AD=E6=98=87=E5=BC=98=29?= , Frederic Chen =?UTF-8?Q?=28=E9=99=B3=E4=BF=8A=E5=85=83=29?= , Rynn Wu =?UTF-8?Q?=28=E5=90=B3=E8=82=B2=E6=81=A9=29?= , srv_heupstream , Rob Herring , Ryan Yu =?UTF-8?Q?=28=E4=BD=99=E5=AD=9F=E4=BF=AE=29?= , Frankie Chiu =?UTF-8?Q?=28=E9=82=B1=E6=96=87=E5=87=B1=29?= , Hans Verkuil , ddavenport@chromium.org, Sj Huang , "moderated list:ARM/Mediatek SoC support" , Laurent Pinchart , Matthias Brugger , Mauro Carvalho Chehab , list@263.net:IOMMU DRIVERS Hi Jungo, > > On Sun, Jul 21, 2019 at 11:18 AM Jungo Lin wrote: > [snip] > > > > + wake_up_interruptible(&isp_ctx->composer_tx_thread.wq); > > > > + isp_ctx->composer_tx_thread.thread = NULL; > > > > + } > > > > + > > > > + if (isp_ctx->composer_deinit_thread.thread) { > > > > + wake_up(&isp_ctx->composer_deinit_thread.wq); > > > > + isp_ctx->composer_deinit_thread.thread = NULL; > > > > + } > > > > + mutex_unlock(&isp_ctx->lock); > > > > + > > > > + pm_runtime_put_sync(&p1_dev->pdev->dev); > > > > > > No need to use the sync variant. > > > > > > > We don't get this point. If we will call pm_runtime_get_sync in > > mtk_isp_hw_init function, will we need to call > > pm_runtime_put_sync_autosuspend in mtk_isp_hw_release in next patch? > > As we know, we should call runtime pm functions in pair. > > > > My point is that pm_runtime_put_sync() is only needed if one wants the > runtime count to be decremented after the function returns. Normally > there is no need to do so and one would call pm_runtime_put(), or if > autosuspend is used, pm_runtime_put_autosuspend() (note there is no > "sync" in the name). > > [snip] Ok, got your point. We will change to use pm_runtime_put_autosuspend() which has ASYNC flag. > > > +static void isp_composer_handler(void *data, unsigned int len, void *priv) > > > > +{ > > > > + struct mtk_isp_p1_ctx *isp_ctx = (struct mtk_isp_p1_ctx *)priv; > > > > + struct isp_p1_device *p1_dev = p1_ctx_to_dev(isp_ctx); > > > > + struct device *dev = &p1_dev->pdev->dev; > > > > + struct mtk_isp_scp_p1_cmd *ipi_msg; > > > > + > > > > + ipi_msg = (struct mtk_isp_scp_p1_cmd *)data; > > > > > > Should we check that len == sizeof(*ipi_msg)? (Or at least >=, if data could > > > contain some extra bytes at the end.) > > > > > > > The len parameter is the actual sending bytes from SCP to kernel. > > In the runtime, it is only 6 bytes for isp_ack_info command > > However, sizeof(*ipi_msg) is large due to struct mtk_isp_scp_p1_cmd is > > union structure. > > > > That said we still should check if len is enough to cover the data > we're accessing below. > Ok, we will add the len checking before accessing the data. > > > > + > > > > + if (ipi_msg->cmd_id != ISP_CMD_ACK) > > > > + return; > > > > + > > > > + if (ipi_msg->ack_info.cmd_id == ISP_CMD_FRAME_ACK) { > > > > + dev_dbg(dev, "ack frame_num:%d", > > > > + ipi_msg->ack_info.frame_seq_no); > > > > + atomic_set(&isp_ctx->composed_frame_id, > > > > + ipi_msg->ack_info.frame_seq_no); > > > > > > I suppose we are expecting here that ipi_msg->ack_info.frame_seq_no would be > > > just isp_ctx->composed_frame_id + 1, right? If not, we probably dropped some > > > frames and we should handle that somehow. > > > > > > > No, we use isp_ctx->composed_frame_id to save which frame sequence > > number are composed done in SCP. In new design, we will move this from > > isp_ctx to p1_dev. > > But we compose the frames in order, don't we? Wouldn't every composed > frame would be just previous frame ID + 1? > > [snip] Yes, we compose the frames in order. At the same time, we already increased "frame ID + 1" in mtk_isp_req_enqueue() for each new request before sending to SCP for composing. After receiving the ACK from SCP, we think the frame ID is composed done and save by isp_ctx->composed_frame_id(v3). [RFC v3] void mtk_isp_req_enqueue(struct device *dev, struct media_request *req) { ... frameparams.frame_seq_no = isp_ctx->frame_seq_no++; [RFC v4] void mtk_isp_req_enqueue(struct mtk_cam_dev *cam, struct mtk_cam_dev_request *req) { struct mtk_isp_p1_device *p1_dev = dev_get_drvdata(cam->dev); /* Accumulated frame sequence number */ req->frame_params.frame_seq_no = ++p1_dev->enqueue_frame_seq_no; > > > > +void isp_composer_hw_init(struct device *dev) > > > > +{ > > > > + struct mtk_isp_scp_p1_cmd composer_tx_cmd; > > > > + struct isp_p1_device *p1_dev = get_p1_device(dev); > > > > + struct mtk_isp_p1_ctx *isp_ctx = &p1_dev->isp_ctx; > > > > + > > > > + memset(&composer_tx_cmd, 0, sizeof(composer_tx_cmd)); > > > > + composer_tx_cmd.cmd_id = ISP_CMD_INIT; > > > > + composer_tx_cmd.frameparam.hw_module = isp_ctx->isp_hw_module; > > > > + composer_tx_cmd.frameparam.cq_addr.iova = isp_ctx->scp_mem_iova; > > > > + composer_tx_cmd.frameparam.cq_addr.scp_addr = isp_ctx->scp_mem_pa; > > > > > > Should we also specify the size of the buffer? Otherwise we could end up > > > with some undetectable overruns. > > > > > > > The size of SCP composer's memory is fixed to 0x200000. > > Is it necessary to specify the size of this buffer? > > > > #define MTK_ISP_COMPOSER_MEM_SIZE 0x200000 > > > > ptr = dma_alloc_coherent(p1_dev->cam_dev.smem_dev, > > MTK_ISP_COMPOSER_MEM_SIZE, &addr, GFP_KERNEL); > > > > Okay, but please add a comment saying that this is an implicit > requirement of the firmware. > > Best regards, > Tomasz Ok, we will add comments. Best regards, Jungo 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 X-Spam-Level: X-Spam-Status: No, score=-2.2 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,UNPARSEABLE_RELAY,URIBL_BLOCKED, USER_AGENT_SANE_2 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 42B61C7618B for ; Fri, 26 Jul 2019 08:07:25 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 174912173E for ; Fri, 26 Jul 2019 08:07:25 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1725945AbfGZIHY (ORCPT ); Fri, 26 Jul 2019 04:07:24 -0400 Received: from mailgw02.mediatek.com ([210.61.82.184]:52894 "EHLO mailgw02.mediatek.com" rhost-flags-OK-FAIL-OK-FAIL) by vger.kernel.org with ESMTP id S1725815AbfGZIHX (ORCPT ); Fri, 26 Jul 2019 04:07:23 -0400 X-UUID: cbf2136ac42e4a648bc565fa019146ae-20190726 X-UUID: cbf2136ac42e4a648bc565fa019146ae-20190726 Received: from mtkexhb02.mediatek.inc [(172.21.101.103)] by mailgw02.mediatek.com (envelope-from ) (Cellopoint E-mail Firewall v4.1.10 Build 0707 with TLS) with ESMTP id 1274394466; Fri, 26 Jul 2019 16:07:17 +0800 Received: from mtkcas07.mediatek.inc (172.21.101.84) by mtkmbs01n1.mediatek.inc (172.21.101.68) with Microsoft SMTP Server (TLS) id 15.0.1395.4; Fri, 26 Jul 2019 16:07:17 +0800 Received: from [172.21.84.99] (172.21.84.99) by mtkcas07.mediatek.inc (172.21.101.73) with Microsoft SMTP Server id 15.0.1395.4 via Frontend Transport; Fri, 26 Jul 2019 16:07:17 +0800 Message-ID: <1564128437.1212.615.camel@mtksdccf07> Subject: Re: [RFC,v3 8/9] media: platform: Add Mediatek ISP P1 SCP communication From: Jungo Lin To: Tomasz Figa CC: Hans Verkuil , Laurent Pinchart , Matthias Brugger , Mauro Carvalho Chehab , Linux Media Mailing List , "moderated list:ARM/Mediatek SoC support" , "list@263.net:IOMMU DRIVERS , Joerg Roedel ," , , srv_heupstream , , Rob Herring , Sean Cheng =?UTF-8?Q?=28=E9=84=AD=E6=98=87=E5=BC=98=29?= , "Sj Huang" , Frederic Chen =?UTF-8?Q?=28=E9=99=B3=E4=BF=8A=E5=85=83=29?= , Ryan Yu =?UTF-8?Q?=28=E4=BD=99=E5=AD=9F=E4=BF=AE=29?= , Rynn Wu =?UTF-8?Q?=28=E5=90=B3=E8=82=B2=E6=81=A9=29?= , Frankie Chiu =?UTF-8?Q?=28=E9=82=B1=E6=96=87=E5=87=B1=29?= Date: Fri, 26 Jul 2019 16:07:17 +0800 In-Reply-To: References: <20190611035344.29814-1-jungo.lin@mediatek.com> <20190611035344.29814-9-jungo.lin@mediatek.com> <20190710095827.GC181405@chromium.org> <1563675513.1212.444.camel@mtksdccf07> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.2.3-0ubuntu6 Content-Transfer-Encoding: 7bit MIME-Version: 1.0 X-MTK: N Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org Hi, Tomasz: On Thu, 2019-07-25 at 19:56 +0900, Tomasz Figa wrote: > Hi Jungo, > > On Sun, Jul 21, 2019 at 11:18 AM Jungo Lin wrote: > [snip] > > > > + wake_up_interruptible(&isp_ctx->composer_tx_thread.wq); > > > > + isp_ctx->composer_tx_thread.thread = NULL; > > > > + } > > > > + > > > > + if (isp_ctx->composer_deinit_thread.thread) { > > > > + wake_up(&isp_ctx->composer_deinit_thread.wq); > > > > + isp_ctx->composer_deinit_thread.thread = NULL; > > > > + } > > > > + mutex_unlock(&isp_ctx->lock); > > > > + > > > > + pm_runtime_put_sync(&p1_dev->pdev->dev); > > > > > > No need to use the sync variant. > > > > > > > We don't get this point. If we will call pm_runtime_get_sync in > > mtk_isp_hw_init function, will we need to call > > pm_runtime_put_sync_autosuspend in mtk_isp_hw_release in next patch? > > As we know, we should call runtime pm functions in pair. > > > > My point is that pm_runtime_put_sync() is only needed if one wants the > runtime count to be decremented after the function returns. Normally > there is no need to do so and one would call pm_runtime_put(), or if > autosuspend is used, pm_runtime_put_autosuspend() (note there is no > "sync" in the name). > > [snip] Ok, got your point. We will change to use pm_runtime_put_autosuspend() which has ASYNC flag. > > > +static void isp_composer_handler(void *data, unsigned int len, void *priv) > > > > +{ > > > > + struct mtk_isp_p1_ctx *isp_ctx = (struct mtk_isp_p1_ctx *)priv; > > > > + struct isp_p1_device *p1_dev = p1_ctx_to_dev(isp_ctx); > > > > + struct device *dev = &p1_dev->pdev->dev; > > > > + struct mtk_isp_scp_p1_cmd *ipi_msg; > > > > + > > > > + ipi_msg = (struct mtk_isp_scp_p1_cmd *)data; > > > > > > Should we check that len == sizeof(*ipi_msg)? (Or at least >=, if data could > > > contain some extra bytes at the end.) > > > > > > > The len parameter is the actual sending bytes from SCP to kernel. > > In the runtime, it is only 6 bytes for isp_ack_info command > > However, sizeof(*ipi_msg) is large due to struct mtk_isp_scp_p1_cmd is > > union structure. > > > > That said we still should check if len is enough to cover the data > we're accessing below. > Ok, we will add the len checking before accessing the data. > > > > + > > > > + if (ipi_msg->cmd_id != ISP_CMD_ACK) > > > > + return; > > > > + > > > > + if (ipi_msg->ack_info.cmd_id == ISP_CMD_FRAME_ACK) { > > > > + dev_dbg(dev, "ack frame_num:%d", > > > > + ipi_msg->ack_info.frame_seq_no); > > > > + atomic_set(&isp_ctx->composed_frame_id, > > > > + ipi_msg->ack_info.frame_seq_no); > > > > > > I suppose we are expecting here that ipi_msg->ack_info.frame_seq_no would be > > > just isp_ctx->composed_frame_id + 1, right? If not, we probably dropped some > > > frames and we should handle that somehow. > > > > > > > No, we use isp_ctx->composed_frame_id to save which frame sequence > > number are composed done in SCP. In new design, we will move this from > > isp_ctx to p1_dev. > > But we compose the frames in order, don't we? Wouldn't every composed > frame would be just previous frame ID + 1? > > [snip] Yes, we compose the frames in order. At the same time, we already increased "frame ID + 1" in mtk_isp_req_enqueue() for each new request before sending to SCP for composing. After receiving the ACK from SCP, we think the frame ID is composed done and save by isp_ctx->composed_frame_id(v3). [RFC v3] void mtk_isp_req_enqueue(struct device *dev, struct media_request *req) { ... frameparams.frame_seq_no = isp_ctx->frame_seq_no++; [RFC v4] void mtk_isp_req_enqueue(struct mtk_cam_dev *cam, struct mtk_cam_dev_request *req) { struct mtk_isp_p1_device *p1_dev = dev_get_drvdata(cam->dev); /* Accumulated frame sequence number */ req->frame_params.frame_seq_no = ++p1_dev->enqueue_frame_seq_no; > > > > +void isp_composer_hw_init(struct device *dev) > > > > +{ > > > > + struct mtk_isp_scp_p1_cmd composer_tx_cmd; > > > > + struct isp_p1_device *p1_dev = get_p1_device(dev); > > > > + struct mtk_isp_p1_ctx *isp_ctx = &p1_dev->isp_ctx; > > > > + > > > > + memset(&composer_tx_cmd, 0, sizeof(composer_tx_cmd)); > > > > + composer_tx_cmd.cmd_id = ISP_CMD_INIT; > > > > + composer_tx_cmd.frameparam.hw_module = isp_ctx->isp_hw_module; > > > > + composer_tx_cmd.frameparam.cq_addr.iova = isp_ctx->scp_mem_iova; > > > > + composer_tx_cmd.frameparam.cq_addr.scp_addr = isp_ctx->scp_mem_pa; > > > > > > Should we also specify the size of the buffer? Otherwise we could end up > > > with some undetectable overruns. > > > > > > > The size of SCP composer's memory is fixed to 0x200000. > > Is it necessary to specify the size of this buffer? > > > > #define MTK_ISP_COMPOSER_MEM_SIZE 0x200000 > > > > ptr = dma_alloc_coherent(p1_dev->cam_dev.smem_dev, > > MTK_ISP_COMPOSER_MEM_SIZE, &addr, GFP_KERNEL); > > > > Okay, but please add a comment saying that this is an implicit > requirement of the firmware. > > Best regards, > Tomasz Ok, we will add comments. Best regards, Jungo 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 X-Spam-Level: X-Spam-Status: No, score=-2.2 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,UNPARSEABLE_RELAY,URIBL_BLOCKED,USER_AGENT_SANE_2 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 54EB9C7618B for ; Fri, 26 Jul 2019 08:07:37 +0000 (UTC) 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 mail.kernel.org (Postfix) with ESMTPS id 28D342173E for ; Fri, 26 Jul 2019 08:07:37 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="PAU47R50" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 28D342173E Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=mediatek.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Date:To:From:Subject:Message-ID:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=RA29ErD3PwM622T0gL9dn01wwA5CU0iq4fCeOuc1t4M=; b=PAU47R50w8lqqg lQggAIuPBvYrpPDmWZ1UiV97zCVl4ASnizsbIv6W2fEhFhMlE2jAJr/61HuY8eOeyuxeuYoF53uYx vT/byPxVuXImndpFBlkgrgWKRGY+AJDEHE+puOXI/iqN4vPQEDX1o6E5B/ZcRKRMltIG+9vzt5a5L KU++oT8neHWEXaEHUhoxR3g2Iv+nRCVeDOkI7MTylewTA9+1myzLQKrGCKcsL5Pk56LaJ19iZKsWw zlxzM3q2NguA+EPavwbQc4AbkpcnVTq544NOKtDVyjr4Dys1NNt0g3T1dumovBs4UM5X9oatTbfdi Jnrwv0nZMMmbFM0LkVJw==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92 #3 (Red Hat Linux)) id 1hqvGK-0007Ta-N3; Fri, 26 Jul 2019 08:07:36 +0000 Received: from mailgw01.mediatek.com ([216.200.240.184]) by bombadil.infradead.org with esmtps (Exim 4.92 #3 (Red Hat Linux)) id 1hqvGG-0007Sw-VJ; Fri, 26 Jul 2019 08:07:34 +0000 X-UUID: 686622a3cb524163b26db958b4c8a2eb-20190726 X-UUID: 686622a3cb524163b26db958b4c8a2eb-20190726 Received: from mtkcas66.mediatek.inc [(172.29.193.44)] by mailgw01.mediatek.com (envelope-from ) (musrelay.mediatek.com ESMTP with TLS) with ESMTP id 164958814; Fri, 26 Jul 2019 00:07:27 -0800 Received: from MTKMBS01N1.mediatek.inc (172.21.101.68) by MTKMBS62N1.mediatek.inc (172.29.193.41) with Microsoft SMTP Server (TLS) id 15.0.1395.4; Fri, 26 Jul 2019 01:07:25 -0700 Received: from mtkcas07.mediatek.inc (172.21.101.84) by mtkmbs01n1.mediatek.inc (172.21.101.68) with Microsoft SMTP Server (TLS) id 15.0.1395.4; Fri, 26 Jul 2019 16:07:17 +0800 Received: from [172.21.84.99] (172.21.84.99) by mtkcas07.mediatek.inc (172.21.101.73) with Microsoft SMTP Server id 15.0.1395.4 via Frontend Transport; Fri, 26 Jul 2019 16:07:17 +0800 Message-ID: <1564128437.1212.615.camel@mtksdccf07> Subject: Re: [RFC,v3 8/9] media: platform: Add Mediatek ISP P1 SCP communication From: Jungo Lin To: Tomasz Figa Date: Fri, 26 Jul 2019 16:07:17 +0800 In-Reply-To: References: <20190611035344.29814-1-jungo.lin@mediatek.com> <20190611035344.29814-9-jungo.lin@mediatek.com> <20190710095827.GC181405@chromium.org> <1563675513.1212.444.camel@mtksdccf07> X-Mailer: Evolution 3.2.3-0ubuntu6 MIME-Version: 1.0 X-MTK: N X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20190726_010733_024326_42436D2B X-CRM114-Status: GOOD ( 23.99 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: devicetree@vger.kernel.org, Sean Cheng =?UTF-8?Q?=28=E9=84=AD=E6=98=87=E5=BC=98=29?= , Frederic Chen =?UTF-8?Q?=28=E9=99=B3=E4=BF=8A=E5=85=83=29?= , Rynn Wu =?UTF-8?Q?=28=E5=90=B3=E8=82=B2=E6=81=A9=29?= , srv_heupstream , Rob Herring , Ryan Yu =?UTF-8?Q?=28=E4=BD=99=E5=AD=9F=E4=BF=AE=29?= , Frankie Chiu =?UTF-8?Q?=28=E9=82=B1=E6=96=87=E5=87=B1=29?= , Hans Verkuil , ddavenport@chromium.org, Sj Huang , "moderated list:ARM/Mediatek SoC support" , Laurent Pinchart , Matthias Brugger , Mauro Carvalho Chehab , "list@263.net:IOMMU DRIVERS , Joerg Roedel ," , Linux Media Mailing List Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi, Tomasz: On Thu, 2019-07-25 at 19:56 +0900, Tomasz Figa wrote: > Hi Jungo, > > On Sun, Jul 21, 2019 at 11:18 AM Jungo Lin wrote: > [snip] > > > > + wake_up_interruptible(&isp_ctx->composer_tx_thread.wq); > > > > + isp_ctx->composer_tx_thread.thread = NULL; > > > > + } > > > > + > > > > + if (isp_ctx->composer_deinit_thread.thread) { > > > > + wake_up(&isp_ctx->composer_deinit_thread.wq); > > > > + isp_ctx->composer_deinit_thread.thread = NULL; > > > > + } > > > > + mutex_unlock(&isp_ctx->lock); > > > > + > > > > + pm_runtime_put_sync(&p1_dev->pdev->dev); > > > > > > No need to use the sync variant. > > > > > > > We don't get this point. If we will call pm_runtime_get_sync in > > mtk_isp_hw_init function, will we need to call > > pm_runtime_put_sync_autosuspend in mtk_isp_hw_release in next patch? > > As we know, we should call runtime pm functions in pair. > > > > My point is that pm_runtime_put_sync() is only needed if one wants the > runtime count to be decremented after the function returns. Normally > there is no need to do so and one would call pm_runtime_put(), or if > autosuspend is used, pm_runtime_put_autosuspend() (note there is no > "sync" in the name). > > [snip] Ok, got your point. We will change to use pm_runtime_put_autosuspend() which has ASYNC flag. > > > +static void isp_composer_handler(void *data, unsigned int len, void *priv) > > > > +{ > > > > + struct mtk_isp_p1_ctx *isp_ctx = (struct mtk_isp_p1_ctx *)priv; > > > > + struct isp_p1_device *p1_dev = p1_ctx_to_dev(isp_ctx); > > > > + struct device *dev = &p1_dev->pdev->dev; > > > > + struct mtk_isp_scp_p1_cmd *ipi_msg; > > > > + > > > > + ipi_msg = (struct mtk_isp_scp_p1_cmd *)data; > > > > > > Should we check that len == sizeof(*ipi_msg)? (Or at least >=, if data could > > > contain some extra bytes at the end.) > > > > > > > The len parameter is the actual sending bytes from SCP to kernel. > > In the runtime, it is only 6 bytes for isp_ack_info command > > However, sizeof(*ipi_msg) is large due to struct mtk_isp_scp_p1_cmd is > > union structure. > > > > That said we still should check if len is enough to cover the data > we're accessing below. > Ok, we will add the len checking before accessing the data. > > > > + > > > > + if (ipi_msg->cmd_id != ISP_CMD_ACK) > > > > + return; > > > > + > > > > + if (ipi_msg->ack_info.cmd_id == ISP_CMD_FRAME_ACK) { > > > > + dev_dbg(dev, "ack frame_num:%d", > > > > + ipi_msg->ack_info.frame_seq_no); > > > > + atomic_set(&isp_ctx->composed_frame_id, > > > > + ipi_msg->ack_info.frame_seq_no); > > > > > > I suppose we are expecting here that ipi_msg->ack_info.frame_seq_no would be > > > just isp_ctx->composed_frame_id + 1, right? If not, we probably dropped some > > > frames and we should handle that somehow. > > > > > > > No, we use isp_ctx->composed_frame_id to save which frame sequence > > number are composed done in SCP. In new design, we will move this from > > isp_ctx to p1_dev. > > But we compose the frames in order, don't we? Wouldn't every composed > frame would be just previous frame ID + 1? > > [snip] Yes, we compose the frames in order. At the same time, we already increased "frame ID + 1" in mtk_isp_req_enqueue() for each new request before sending to SCP for composing. After receiving the ACK from SCP, we think the frame ID is composed done and save by isp_ctx->composed_frame_id(v3). [RFC v3] void mtk_isp_req_enqueue(struct device *dev, struct media_request *req) { ... frameparams.frame_seq_no = isp_ctx->frame_seq_no++; [RFC v4] void mtk_isp_req_enqueue(struct mtk_cam_dev *cam, struct mtk_cam_dev_request *req) { struct mtk_isp_p1_device *p1_dev = dev_get_drvdata(cam->dev); /* Accumulated frame sequence number */ req->frame_params.frame_seq_no = ++p1_dev->enqueue_frame_seq_no; > > > > +void isp_composer_hw_init(struct device *dev) > > > > +{ > > > > + struct mtk_isp_scp_p1_cmd composer_tx_cmd; > > > > + struct isp_p1_device *p1_dev = get_p1_device(dev); > > > > + struct mtk_isp_p1_ctx *isp_ctx = &p1_dev->isp_ctx; > > > > + > > > > + memset(&composer_tx_cmd, 0, sizeof(composer_tx_cmd)); > > > > + composer_tx_cmd.cmd_id = ISP_CMD_INIT; > > > > + composer_tx_cmd.frameparam.hw_module = isp_ctx->isp_hw_module; > > > > + composer_tx_cmd.frameparam.cq_addr.iova = isp_ctx->scp_mem_iova; > > > > + composer_tx_cmd.frameparam.cq_addr.scp_addr = isp_ctx->scp_mem_pa; > > > > > > Should we also specify the size of the buffer? Otherwise we could end up > > > with some undetectable overruns. > > > > > > > The size of SCP composer's memory is fixed to 0x200000. > > Is it necessary to specify the size of this buffer? > > > > #define MTK_ISP_COMPOSER_MEM_SIZE 0x200000 > > > > ptr = dma_alloc_coherent(p1_dev->cam_dev.smem_dev, > > MTK_ISP_COMPOSER_MEM_SIZE, &addr, GFP_KERNEL); > > > > Okay, but please add a comment saying that this is an implicit > requirement of the firmware. > > Best regards, > Tomasz Ok, we will add comments. Best regards, Jungo _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel