Received: by 2002:ac0:a581:0:0:0:0:0 with SMTP id m1-v6csp7046485imm; Wed, 27 Jun 2018 19:00:40 -0700 (PDT) X-Google-Smtp-Source: ADUXVKI3iaGM+KxiYSIPMxxfTTGYvPKFql8gR/RDse1hNm9J93NJ7fHkqne0b1L9QZZ7R9xnCaXh X-Received: by 2002:a17:902:a505:: with SMTP id s5-v6mr8623121plq.66.1530151240422; Wed, 27 Jun 2018 19:00:40 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1530151240; cv=none; d=google.com; s=arc-20160816; b=eT0D1anWMcpX2J6A7VR9Fl+Ex3L9VI5YBImr34tVorq+JchBHubGp6bTs47jM8X1xz aSGt9lDaF44zSmPkfWWOlMK0b6dU21nnxCBDuDtByQRIP9ZdFvBBRujU3m+vkQz6fzJN qzq6n8eKMQG3OOqD9f+TD7C6VWbAnDsBxuucpOKs213aBht/x4jxcgqCr2hnbn59o37c nSl79hDNpsK+btG4OIKokMzLAID7FgyG0gObJ13hWa1FLMf/i2u3eRo/ye8R5Rt3K/2e 8Uu4t6hZXdKOCOHnlANa6ZP6Eu5qVy8t/neag6H8EU4fz8n1MIMx7PwTI8Q3B+O5hH5S azog== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding:mime-version :references:in-reply-to:date:cc:to:from:subject:message-id :arc-authentication-results; bh=n1jzUAO6oLrcOZMXORWFwwc5+SJ1UvQpXeRBr0RtYBU=; b=x7VHJeEJZ2mPOUyCsG0+ymeUz8/ob1Xz3IZLQysvT7y45WfmMzGegGiMZx2C3L7wZ8 gL+wUizslCHWSzR0eewiWYUfLWbTSDMW0WMWOlMTqTYGl3iFugobjilL3OE4WHTWdr+P vRWvAnjH/a5UIafAz+bEzXrdYOFaD3o7nilIFbObWwM00RVfE7C5NFs04667jpxJVb2c lPg9U+brBFfX1trRLMJj4QsXIZUBqP/ckECGLzlGjyHg0uhehly5v4AgSHt/LjKIpHAQ cgqihaD/lIMFjp4yEEdjTpzOWQw4S/GegvT566K56ZNmN58eC3o7LPCewJ/YZ3viXpz/ 1PyQ== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id l12-v6si5280229plc.215.2018.06.27.19.00.25; Wed, 27 Jun 2018 19:00:40 -0700 (PDT) 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; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932205AbeF1B5s (ORCPT + 99 others); Wed, 27 Jun 2018 21:57:48 -0400 Received: from mailgw01.mediatek.com ([210.61.82.183]:8134 "EHLO mailgw01.mediatek.com" rhost-flags-OK-FAIL-OK-FAIL) by vger.kernel.org with ESMTP id S932146AbeF1B5q (ORCPT ); Wed, 27 Jun 2018 21:57:46 -0400 X-UUID: 3d9ee94859aa4a4490d4ee0957ae18db-20180628 Received: from mtkcas08.mediatek.inc [(172.21.101.126)] by mailgw01.mediatek.com (envelope-from ) (mhqrelay.mediatek.com ESMTP with TLS) with ESMTP id 662498536; Thu, 28 Jun 2018 09:57:41 +0800 Received: from mtkcas07.mediatek.inc (172.21.101.84) by mtkmbs08n1.mediatek.inc (172.21.101.55) with Microsoft SMTP Server (TLS) id 15.0.1210.3; Thu, 28 Jun 2018 09:57:39 +0800 Received: from [172.21.77.4] (172.21.77.4) by mtkcas07.mediatek.inc (172.21.101.73) with Microsoft SMTP Server id 15.0.1210.3 via Frontend Transport; Thu, 28 Jun 2018 09:57:39 +0800 Message-ID: <1530151059.6193.27.camel@mtksdaap41> Subject: Re: [PATCH v21 2/4] mailbox: mediatek: Add Mediatek CMDQ driver From: CK Hu To: houlong wei CC: Jassi Brar , Matthias Brugger , Rob Herring , Daniel Kurtz , Sascha Hauer , "devicetree@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "linux-mediatek@lists.infradead.org" , srv_heupstream , Sascha Hauer , Philipp Zabel , "Nicolas Boichat" , Cawa Cheng =?UTF-8?Q?=28=E9=84=AD=E6=9B=84=E7=A6=A7=29?= , Bibby Hsieh =?UTF-8?Q?=28=E8=AC=9D=E6=BF=9F=E9=81=A0=29?= , YT Shen =?UTF-8?Q?=28=E6=B2=88=E5=B2=B3=E9=9C=86=29?= , Daoyuan Huang =?UTF-8?Q?=28=E9=BB=83=E9=81=93=E5=8E=9F=29?= , Damon Chu =?UTF-8?Q?=28=E6=9C=B1=E5=B3=BB=E8=B3=A2=29?= , Josh-YC Liu =?UTF-8?Q?=28=E5=8A=89=E8=82=B2=E8=AA=A0=29?= , Glory Hung =?UTF-8?Q?=28=E6=B4=AA=E6=99=BA=E7=91=8B=29?= , Jiaguang Zhang =?UTF-8?Q?=28=E5=BC=A0=E5=8A=A0=E5=B9=BF=29?= , Dennis-YC Hsieh =?UTF-8?Q?=28=E8=AC=9D=E5=AE=87=E5=93=B2=29?= , Monica Wang =?UTF-8?Q?=28=E7=8E=8B=E5=AD=9F=E5=A9=B7=29?= , Hs Liao =?UTF-8?Q?=28=E5=BB=96=E5=AE=8F=E7=A5=A5=29?= Date: Thu, 28 Jun 2018 09:57:39 +0800 In-Reply-To: <1530100416.31725.15.camel@mhfsdcap03> References: <1517383693-25591-1-git-send-email-houlong.wei@mediatek.com> <1517383693-25591-3-git-send-email-houlong.wei@mediatek.com> <1519185212.8716.32.camel@mtksdaap41> <1530100416.31725.15.camel@mhfsdcap03> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.10.4-0ubuntu2 MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-MTK: N Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, Houlong: On Wed, 2018-06-27 at 19:53 +0800, houlong wei wrote: > On Wed, 2018-02-21 at 11:53 +0800, CK Hu wrote: > > Hi, Houlong: > > > > I've one inline comment. > > > > On Wed, 2018-01-31 at 15:28 +0800, houlong.wei@mediatek.com wrote: > > > From: "hs.liao@mediatek.com" > > > > > > This patch is first version of Mediatek Command Queue(CMDQ) driver. The > > > CMDQ is used to help write registers with critical time limitation, > > > such as updating display configuration during the vblank. It controls > > > Global Command Engine (GCE) hardware to achieve this requirement. > > > Currently, CMDQ only supports display related hardwares, but we expect > > > it can be extended to other hardwares for future requirements. > > > > > > Signed-off-by: Houlong Wei > > > Signed-off-by: HS Liao > > > Signed-off-by: CK Hu > > > --- > > > drivers/mailbox/Kconfig | 10 + > > > drivers/mailbox/Makefile | 2 + > > > drivers/mailbox/mtk-cmdq-mailbox.c | 594 ++++++++++++++++++++++++++++++ > > > include/linux/mailbox/mtk-cmdq-mailbox.h | 77 ++++ > > > 4 files changed, 683 insertions(+) > > > create mode 100644 drivers/mailbox/mtk-cmdq-mailbox.c > > > create mode 100644 include/linux/mailbox/mtk-cmdq-mailbox.h > > > > > > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig > > > index ba2f152..43bb26f 100644 > > > --- a/drivers/mailbox/Kconfig > > > +++ b/drivers/mailbox/Kconfig > > > @@ -171,4 +171,14 @@ config BCM_FLEXRM_MBOX > > > Mailbox implementation of the Broadcom FlexRM ring manager, > > > which provides access to various offload engines on Broadcom > > > SoCs. Say Y here if you want to use the Broadcom FlexRM. > > > + [...] > > > + > > > +static void cmdq_task_exec(struct cmdq_pkt *pkt, struct cmdq_thread *thread) > > > +{ > > > + struct cmdq *cmdq; > > > + struct cmdq_task *task; > > > + unsigned long curr_pa, end_pa; > > > + > > > + cmdq = dev_get_drvdata(thread->chan->mbox->dev); > > > + > > > + /* Client should not flush new tasks if suspended. */ > > > + WARN_ON(cmdq->suspended); > > > + > > > + task = kzalloc(sizeof(*task), GFP_ATOMIC); > > > + task->cmdq = cmdq; > > > + INIT_LIST_HEAD(&task->list_entry); > > > + task->pa_base = pkt->pa_base; > > > + task->thread = thread; > > > + task->pkt = pkt; > > > + > > > + if (list_empty(&thread->task_busy_list)) { > > > + WARN_ON(clk_enable(cmdq->clock) < 0); > > > + WARN_ON(cmdq_thread_reset(cmdq, thread) < 0); > > > + > > > + writel(task->pa_base, thread->base + CMDQ_THR_CURR_ADDR); > > > + writel(task->pa_base + pkt->cmd_buf_size, > > > + thread->base + CMDQ_THR_END_ADDR); > > > + writel(CMDQ_THR_IRQ_EN, thread->base + CMDQ_THR_IRQ_ENABLE); > > > + writel(CMDQ_THR_ENABLED, thread->base + CMDQ_THR_ENABLE_TASK); > > > + > > > + mod_timer(&thread->timeout, > > > + jiffies + msecs_to_jiffies(CMDQ_TIMEOUT_MS)); > > > + } else { > > > + WARN_ON(cmdq_thread_suspend(cmdq, thread) < 0); > > > + curr_pa = readl(thread->base + CMDQ_THR_CURR_ADDR); > > > + end_pa = readl(thread->base + CMDQ_THR_END_ADDR); > > > + > > > + /* > > > + * Atomic execution should remove the following wfe, i.e. only > > > + * wait event at first task, and prevent to pause when running. > > > + */ > > > + if (thread->atomic_exec) { > > > + /* GCE is executing if command is not WFE */ > > > + if (!cmdq_thread_is_in_wfe(thread)) { > > > + cmdq_thread_resume(thread); > > > + cmdq_thread_wait_end(thread, end_pa); > > > + WARN_ON(cmdq_thread_suspend(cmdq, thread) < 0); > > > + /* set to this task directly */ > > > + writel(task->pa_base, > > > + thread->base + CMDQ_THR_CURR_ADDR); > > > + } else { > > > + cmdq_task_insert_into_thread(task); > > > + cmdq_task_remove_wfe(task); > > > + smp_mb(); /* modify jump before enable thread */ > > > + } > > > + } else { > > > + /* check boundary */ > > > + if (curr_pa == end_pa - CMDQ_INST_SIZE || > > > + curr_pa == end_pa) { > > > + /* set to this task directly */ > > > + writel(task->pa_base, > > > + thread->base + CMDQ_THR_CURR_ADDR); > > > + } else { > > > + cmdq_task_insert_into_thread(task); > > > + smp_mb(); /* modify jump before enable thread */ > > > + } > > > + } > > > + writel(task->pa_base + pkt->cmd_buf_size, > > > + thread->base + CMDQ_THR_END_ADDR); > > > + cmdq_thread_resume(thread); > > > + } > > > + list_move_tail(&task->list_entry, &thread->task_busy_list); > > > > You implement a list to queue command because you need to execute > > multiple packet in the same vblank period. I've a suggestion that you > > need not to implement a list. Once cmdq driver receive two packet as > > below: > > > > Packet 1: > > (1) clear vblank event > > (2) wait vblank event > > (3) write register setting 1 > > (4) no operation > > > > Packet 2: > > (1) clear vblank event > > (2) wait vblank event > > (3) write register setting 2 > > (4) no operation > > > > In your current design, you modify these two packet as: > > > > Packet 1: > > (1) clear vblank event > > (2) wait vblank event > > (3) write register setting 1 > > (4) Jump to packet 2 (modified) > > > > Packet 2: > > (1) no operation (modified) > > (2) no operation (modified) > > (3) write register setting 2 > > (4) no operation > > > > So the register setting 1 and register setting 2 could be executed in > > the same vblank period. > > > > My suggestion is: when the client want to send packet 2, it 'abort' > > packet 1 at first. The 'abort' means remove it from channel. In current > > mailbox interface, mbox_free_channel() is most like abort function, but > > my abort would keep the channel. So maybe you need to implement a new > > mailbox interface which could remove packet in the channel. So the step > > would be: > > > > (1) Client generate packet 1 which include write register setting 1 > > (2) Client send packet 1 to channel A > > > > When client want to send register setting 2, > > > > (3) Client abort channel A > > (4) Client generate packet 2 which include write register setting 1 & 2 > > (5) Client send packet 2 to channel A > > > > Once you have the abort function, you could use the queue mechanism in > > mailbox core instead of implementing your own. > > > > For the client which have the atomic requirement, it also need not to > > implement a list to keep what command have not executed. So the abort > > interface would make client and controller much simpler. > > > > Regards, > > CK > > > > Hi CK, thanks for you suggestion. Since current mailbox framework has > no 'abort' function and need add new interface. It may be complicated > to do this. Could we keep current solution? > I imagine that 'abort' is a simple function. Is my imagination incorrect? So you would like choose implementing a complicated queue mechanism in mtk_cmdq driver rather than implementing abort function. Maybe both are complicated. I think the more important thing is that do you and maintainer agree to implement abort function which could reduce mtk-self-queue in mtk_cmdq driver. If the answer is yes, there could be two patch sequence A and B, the patch sequence A is A.1 mtk_cmdq driver with mtk-self-queue. A.2 mtk display driver use mtk_cmdq without abort function. A.3 add mailbox abort function A.4 mtk_cmdq driver support abort function and remove mtk-self-queue. A.5 mtk display driver use mtk_cmdq with abort function And the patch sequence B is B.1 add mailbox abort function B.2 mtk_cmdq driver with abort function and no mtk-self-queue B.3 mtk display driver use mtk_cmdq with abort function Which one do you think is complicated? So let's back to the more important thing: 'do you agree to implement abort function which could reduce mtk-self-queue in mtk_cmdq driver?' Regards, CK > > > +} > > > + [...] >