On Thu, 2018-06-28 at 09:57 +0800, CK Hu wrote:
> 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....@mediatek.com wrote:
> > > > From: "hs.l...@mediatek.com" <hs.l...@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 <houlong....@mediatek.com>
> > > > Signed-off-by: HS Liao <hs.l...@mediatek.com>
> > > > Signed-off-by: CK Hu <ck...@mediatek.com>
> > > > ---
> > > > 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,
Hello Jassi, what's your opinion on 'abort' function ?
> 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
>
> > > > +}
> > > > +
>
> [...]
>
> >
>
>
