Re: [PATCH 3/3] mailbox: mediatek: Remove busylist
Hi CK,
On Fri, 2019-02-15 at 00:01 +0800, CK Hu wrote:
> Hi, Dennis:
>
> On Tue, 2019-02-12 at 10:18 +0800, Dennis-YC Hsieh wrote:
> > Hi CK,
> >
> > On Tue, 2019-01-29 at 17:20 +0800, Houlong Wei (魏厚龙) wrote:
> > > -Original Message-
> > > From: CK Hu [mailto:ck...@mediatek.com]
> > > Sent: Wednesday, January 16, 2019 1:05 PM
> > > To: Jassi Brar ; Matthias Brugger
> > > ; Houlong Wei (魏厚龙)
> > > Cc: linux-kernel@vger.kernel.org; linux-arm-ker...@lists.infradead.org;
> > > linux-media...@lists.infradead.org; srv_heupstream
> > > ; CK Hu (胡俊光)
> > > Subject: [PATCH 3/3] mailbox: mediatek: Remove busylist
> > >
> > > After implement abort_data, controller need not to implement its own
> > > queue. Remove busylist because it's useless.
> >
> > Remove busy list in controller makes client driver have no way to queue
> > pkt in gce hardware thread, which may hurt display and multimedia
> > performance since each pkt waits IRQ delay before previous pkt. Suggest
> > keep busy list design.
>
> If some client driver need gce to cascade pkt to prevent irq delay, I
> should keep busylist. For drm driver, I still want to apply the first
> two patches of this series and remove atomic feature because drm could
> keep just one pkt and reuse it to prevent frequently allocate/free pkt
> and the total commands executed in vblank period would be
> well-controlled. Do you have any concern about removing atomic feature?
>
> Regards,
> CK
>
I have no concern on remove atomic feature.
Regards,
Dennis
> >
> >
> > Regards,
> > Dennis
> >
> > >
> > > Signed-off-by: CK Hu
> > > ---
> > > drivers/mailbox/mtk-cmdq-mailbox.c | 255 -
> > > 1 file changed, 29 insertions(+), 226 deletions(-)
> > >
> > > diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c
> > > b/drivers/mailbox/mtk-cmdq-mailbox.c
> > > index f2219f263ef6..45c59f677ecb 100644
> > > --- a/drivers/mailbox/mtk-cmdq-mailbox.c
> > > +++ b/drivers/mailbox/mtk-cmdq-mailbox.c
> > > @@ -16,9 +16,7 @@
> > > #include
> > > #include
> > >
> > > -#define CMDQ_OP_CODE_MASK(0xff << CMDQ_OP_CODE_SHIFT)
> > > #define CMDQ_IRQ_MASK0x
> > > -#define CMDQ_NUM_CMD(t) (t->cmd_buf_size /
> > > CMDQ_INST_SIZE)
> > >
> > > #define CMDQ_CURR_IRQ_STATUS 0x10
> > > #define CMDQ_THR_SLOT_CYCLES 0x30
> > > @@ -47,22 +45,10 @@
> > > #define CMDQ_THR_IRQ_EN (CMDQ_THR_IRQ_ERROR |
> > > CMDQ_THR_IRQ_DONE)
> > > #define CMDQ_THR_IS_WAITING BIT(31)
> > >
> > > -#define CMDQ_JUMP_BY_OFFSET 0x1000
> > > -#define CMDQ_JUMP_BY_PA 0x1001
> > > -
> > > struct cmdq_thread {
> > > struct mbox_chan*chan;
> > > void __iomem*base;
> > > - struct list_headtask_busy_list;
> > > u32 priority;
> > > - boolatomic_exec;
> > > -};
> > > -
> > > -struct cmdq_task {
> > > - struct cmdq *cmdq;
> > > - struct list_headlist_entry;
> > > - dma_addr_t pa_base;
> > > - struct cmdq_thread *thread;
> > > struct cmdq_pkt *pkt; /* the packet sent from mailbox client */
> > > };
> > >
> > > @@ -130,171 +116,47 @@ static void cmdq_thread_disable(struct cmdq *cmdq,
> > > struct cmdq_thread *thread)
> > > writel(CMDQ_THR_DISABLED, thread->base + CMDQ_THR_ENABLE_TASK); }
> > >
> > > -/* notify GCE to re-fetch commands by setting GCE thread PC */ -static
> > > void cmdq_thread_invalidate_fetched_data(struct cmdq_thread *thread) -{
> > > - writel(readl(thread->base + CMDQ_THR_CURR_ADDR),
> > > -thread->base + CMDQ_THR_CURR_ADDR);
> > > -}
> > > -
> > > -static void cmdq_task_insert_into_thread(struct cmdq_task *task) -{
> > > - struct device *dev = task->cmdq->mbox.dev;
> > > - struct cmdq_thread *thread = task->thread;
> > > - struct cmdq_task *prev_task = list_last_entry(
> > > - &thread->task_busy_list, typeof(*task), list_entry);
> > > - u64 *prev_task_base = prev_task->pkt->va_base;
> > > -
> > > - /* let previous task jump to th
Re: [PATCH 3/3] mailbox: mediatek: Remove busylist
Hi, Dennis:
On Tue, 2019-02-12 at 10:18 +0800, Dennis-YC Hsieh wrote:
> Hi CK,
>
> On Tue, 2019-01-29 at 17:20 +0800, Houlong Wei (魏厚龙) wrote:
> > -Original Message-
> > From: CK Hu [mailto:ck...@mediatek.com]
> > Sent: Wednesday, January 16, 2019 1:05 PM
> > To: Jassi Brar ; Matthias Brugger
> > ; Houlong Wei (魏厚龙)
> > Cc: linux-kernel@vger.kernel.org; linux-arm-ker...@lists.infradead.org;
> > linux-media...@lists.infradead.org; srv_heupstream
> > ; CK Hu (胡俊光)
> > Subject: [PATCH 3/3] mailbox: mediatek: Remove busylist
> >
> > After implement abort_data, controller need not to implement its own queue.
> > Remove busylist because it's useless.
>
> Remove busy list in controller makes client driver have no way to queue
> pkt in gce hardware thread, which may hurt display and multimedia
> performance since each pkt waits IRQ delay before previous pkt. Suggest
> keep busy list design.
If some client driver need gce to cascade pkt to prevent irq delay, I
should keep busylist. For drm driver, I still want to apply the first
two patches of this series and remove atomic feature because drm could
keep just one pkt and reuse it to prevent frequently allocate/free pkt
and the total commands executed in vblank period would be
well-controlled. Do you have any concern about removing atomic feature?
Regards,
CK
>
>
> Regards,
> Dennis
>
> >
> > Signed-off-by: CK Hu
> > ---
> > drivers/mailbox/mtk-cmdq-mailbox.c | 255 -
> > 1 file changed, 29 insertions(+), 226 deletions(-)
> >
> > diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c
> > b/drivers/mailbox/mtk-cmdq-mailbox.c
> > index f2219f263ef6..45c59f677ecb 100644
> > --- a/drivers/mailbox/mtk-cmdq-mailbox.c
> > +++ b/drivers/mailbox/mtk-cmdq-mailbox.c
> > @@ -16,9 +16,7 @@
> > #include
> > #include
> >
> > -#define CMDQ_OP_CODE_MASK (0xff << CMDQ_OP_CODE_SHIFT)
> > #define CMDQ_IRQ_MASK 0x
> > -#define CMDQ_NUM_CMD(t)(t->cmd_buf_size /
> > CMDQ_INST_SIZE)
> >
> > #define CMDQ_CURR_IRQ_STATUS 0x10
> > #define CMDQ_THR_SLOT_CYCLES 0x30
> > @@ -47,22 +45,10 @@
> > #define CMDQ_THR_IRQ_EN(CMDQ_THR_IRQ_ERROR |
> > CMDQ_THR_IRQ_DONE)
> > #define CMDQ_THR_IS_WAITINGBIT(31)
> >
> > -#define CMDQ_JUMP_BY_OFFSET0x1000
> > -#define CMDQ_JUMP_BY_PA0x1001
> > -
> > struct cmdq_thread {
> > struct mbox_chan*chan;
> > void __iomem*base;
> > - struct list_headtask_busy_list;
> > u32 priority;
> > - boolatomic_exec;
> > -};
> > -
> > -struct cmdq_task {
> > - struct cmdq *cmdq;
> > - struct list_headlist_entry;
> > - dma_addr_t pa_base;
> > - struct cmdq_thread *thread;
> > struct cmdq_pkt *pkt; /* the packet sent from mailbox client */
> > };
> >
> > @@ -130,171 +116,47 @@ static void cmdq_thread_disable(struct cmdq *cmdq,
> > struct cmdq_thread *thread)
> > writel(CMDQ_THR_DISABLED, thread->base + CMDQ_THR_ENABLE_TASK); }
> >
> > -/* notify GCE to re-fetch commands by setting GCE thread PC */ -static
> > void cmdq_thread_invalidate_fetched_data(struct cmdq_thread *thread) -{
> > - writel(readl(thread->base + CMDQ_THR_CURR_ADDR),
> > - thread->base + CMDQ_THR_CURR_ADDR);
> > -}
> > -
> > -static void cmdq_task_insert_into_thread(struct cmdq_task *task) -{
> > - struct device *dev = task->cmdq->mbox.dev;
> > - struct cmdq_thread *thread = task->thread;
> > - struct cmdq_task *prev_task = list_last_entry(
> > - &thread->task_busy_list, typeof(*task), list_entry);
> > - u64 *prev_task_base = prev_task->pkt->va_base;
> > -
> > - /* let previous task jump to this task */
> > - dma_sync_single_for_cpu(dev, prev_task->pa_base,
> > - prev_task->pkt->cmd_buf_size, DMA_TO_DEVICE);
> > - prev_task_base[CMDQ_NUM_CMD(prev_task->pkt) - 1] =
> > - (u64)CMDQ_JUMP_BY_PA << 32 | task->pa_base;
> > - dma_sync_single_for_device(dev, prev_task->pa_base,
> > - prev_task->pkt->cmd_buf_size, DMA_TO_DEVICE);
> > -
> > - cmdq_thread_invalidate_fetched_data(thread);
> > -}
> > -
> > -static bo
Re: [PATCH 3/3] mailbox: mediatek: Remove busylist
Hi CK,
On Tue, 2019-01-29 at 17:20 +0800, Houlong Wei (魏厚龙) wrote:
> -Original Message-
> From: CK Hu [mailto:ck...@mediatek.com]
> Sent: Wednesday, January 16, 2019 1:05 PM
> To: Jassi Brar ; Matthias Brugger
> ; Houlong Wei (魏厚龙)
> Cc: linux-kernel@vger.kernel.org; linux-arm-ker...@lists.infradead.org;
> linux-media...@lists.infradead.org; srv_heupstream
> ; CK Hu (胡俊光)
> Subject: [PATCH 3/3] mailbox: mediatek: Remove busylist
>
> After implement abort_data, controller need not to implement its own queue.
> Remove busylist because it's useless.
Remove busy list in controller makes client driver have no way to queue
pkt in gce hardware thread, which may hurt display and multimedia
performance since each pkt waits IRQ delay before previous pkt. Suggest
keep busy list design.
Regards,
Dennis
>
> Signed-off-by: CK Hu
> ---
> drivers/mailbox/mtk-cmdq-mailbox.c | 255 -
> 1 file changed, 29 insertions(+), 226 deletions(-)
>
> diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c
> b/drivers/mailbox/mtk-cmdq-mailbox.c
> index f2219f263ef6..45c59f677ecb 100644
> --- a/drivers/mailbox/mtk-cmdq-mailbox.c
> +++ b/drivers/mailbox/mtk-cmdq-mailbox.c
> @@ -16,9 +16,7 @@
> #include
> #include
>
> -#define CMDQ_OP_CODE_MASK(0xff << CMDQ_OP_CODE_SHIFT)
> #define CMDQ_IRQ_MASK0x
> -#define CMDQ_NUM_CMD(t) (t->cmd_buf_size /
> CMDQ_INST_SIZE)
>
> #define CMDQ_CURR_IRQ_STATUS 0x10
> #define CMDQ_THR_SLOT_CYCLES 0x30
> @@ -47,22 +45,10 @@
> #define CMDQ_THR_IRQ_EN (CMDQ_THR_IRQ_ERROR |
> CMDQ_THR_IRQ_DONE)
> #define CMDQ_THR_IS_WAITING BIT(31)
>
> -#define CMDQ_JUMP_BY_OFFSET 0x1000
> -#define CMDQ_JUMP_BY_PA 0x1001
> -
> struct cmdq_thread {
> struct mbox_chan*chan;
> void __iomem*base;
> - struct list_headtask_busy_list;
> u32 priority;
> - boolatomic_exec;
> -};
> -
> -struct cmdq_task {
> - struct cmdq *cmdq;
> - struct list_headlist_entry;
> - dma_addr_t pa_base;
> - struct cmdq_thread *thread;
> struct cmdq_pkt *pkt; /* the packet sent from mailbox client */
> };
>
> @@ -130,171 +116,47 @@ static void cmdq_thread_disable(struct cmdq *cmdq,
> struct cmdq_thread *thread)
> writel(CMDQ_THR_DISABLED, thread->base + CMDQ_THR_ENABLE_TASK); }
>
> -/* notify GCE to re-fetch commands by setting GCE thread PC */ -static void
> cmdq_thread_invalidate_fetched_data(struct cmdq_thread *thread) -{
> - writel(readl(thread->base + CMDQ_THR_CURR_ADDR),
> -thread->base + CMDQ_THR_CURR_ADDR);
> -}
> -
> -static void cmdq_task_insert_into_thread(struct cmdq_task *task) -{
> - struct device *dev = task->cmdq->mbox.dev;
> - struct cmdq_thread *thread = task->thread;
> - struct cmdq_task *prev_task = list_last_entry(
> - &thread->task_busy_list, typeof(*task), list_entry);
> - u64 *prev_task_base = prev_task->pkt->va_base;
> -
> - /* let previous task jump to this task */
> - dma_sync_single_for_cpu(dev, prev_task->pa_base,
> - prev_task->pkt->cmd_buf_size, DMA_TO_DEVICE);
> - prev_task_base[CMDQ_NUM_CMD(prev_task->pkt) - 1] =
> - (u64)CMDQ_JUMP_BY_PA << 32 | task->pa_base;
> - dma_sync_single_for_device(dev, prev_task->pa_base,
> -prev_task->pkt->cmd_buf_size, DMA_TO_DEVICE);
> -
> - cmdq_thread_invalidate_fetched_data(thread);
> -}
> -
> -static bool cmdq_command_is_wfe(u64 cmd) -{
> - u64 wfe_option = CMDQ_WFE_UPDATE | CMDQ_WFE_WAIT | CMDQ_WFE_WAIT_VALUE;
> - u64 wfe_op = (u64)(CMDQ_CODE_WFE << CMDQ_OP_CODE_SHIFT) << 32;
> - u64 wfe_mask = (u64)CMDQ_OP_CODE_MASK << 32 | 0x;
> -
> - return ((cmd & wfe_mask) == (wfe_op | wfe_option));
> -}
> -
> -/* we assume tasks in the same display GCE thread are waiting the same
> event. */ -static void cmdq_task_remove_wfe(struct cmdq_task *task) -{
> - struct device *dev = task->cmdq->mbox.dev;
> - u64 *base = task->pkt->va_base;
> - int i;
> -
> - dma_sync_single_for_cpu(dev, task->pa_base, task->pkt->cmd_buf_size,
> - DMA_TO_DEVICE);
> - for (i = 0; i < CMDQ_NUM_CMD(task->pkt); i++)
> - if (cmdq_command_is_wfe(base[i]))
> - base[i] = (u64
[PATCH 3/3] mailbox: mediatek: Remove busylist
After implement abort_data, controller need not to implement its own
queue. Remove busylist because it's useless.
Signed-off-by: CK Hu
---
drivers/mailbox/mtk-cmdq-mailbox.c | 255 -
1 file changed, 29 insertions(+), 226 deletions(-)
diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c
b/drivers/mailbox/mtk-cmdq-mailbox.c
index f2219f263ef6..45c59f677ecb 100644
--- a/drivers/mailbox/mtk-cmdq-mailbox.c
+++ b/drivers/mailbox/mtk-cmdq-mailbox.c
@@ -16,9 +16,7 @@
#include
#include
-#define CMDQ_OP_CODE_MASK (0xff << CMDQ_OP_CODE_SHIFT)
#define CMDQ_IRQ_MASK 0x
-#define CMDQ_NUM_CMD(t)(t->cmd_buf_size /
CMDQ_INST_SIZE)
#define CMDQ_CURR_IRQ_STATUS 0x10
#define CMDQ_THR_SLOT_CYCLES 0x30
@@ -47,22 +45,10 @@
#define CMDQ_THR_IRQ_EN(CMDQ_THR_IRQ_ERROR |
CMDQ_THR_IRQ_DONE)
#define CMDQ_THR_IS_WAITINGBIT(31)
-#define CMDQ_JUMP_BY_OFFSET0x1000
-#define CMDQ_JUMP_BY_PA0x1001
-
struct cmdq_thread {
struct mbox_chan*chan;
void __iomem*base;
- struct list_headtask_busy_list;
u32 priority;
- boolatomic_exec;
-};
-
-struct cmdq_task {
- struct cmdq *cmdq;
- struct list_headlist_entry;
- dma_addr_t pa_base;
- struct cmdq_thread *thread;
struct cmdq_pkt *pkt; /* the packet sent from mailbox client */
};
@@ -130,171 +116,47 @@ static void cmdq_thread_disable(struct cmdq *cmdq,
struct cmdq_thread *thread)
writel(CMDQ_THR_DISABLED, thread->base + CMDQ_THR_ENABLE_TASK);
}
-/* notify GCE to re-fetch commands by setting GCE thread PC */
-static void cmdq_thread_invalidate_fetched_data(struct cmdq_thread *thread)
-{
- writel(readl(thread->base + CMDQ_THR_CURR_ADDR),
- thread->base + CMDQ_THR_CURR_ADDR);
-}
-
-static void cmdq_task_insert_into_thread(struct cmdq_task *task)
-{
- struct device *dev = task->cmdq->mbox.dev;
- struct cmdq_thread *thread = task->thread;
- struct cmdq_task *prev_task = list_last_entry(
- &thread->task_busy_list, typeof(*task), list_entry);
- u64 *prev_task_base = prev_task->pkt->va_base;
-
- /* let previous task jump to this task */
- dma_sync_single_for_cpu(dev, prev_task->pa_base,
- prev_task->pkt->cmd_buf_size, DMA_TO_DEVICE);
- prev_task_base[CMDQ_NUM_CMD(prev_task->pkt) - 1] =
- (u64)CMDQ_JUMP_BY_PA << 32 | task->pa_base;
- dma_sync_single_for_device(dev, prev_task->pa_base,
- prev_task->pkt->cmd_buf_size, DMA_TO_DEVICE);
-
- cmdq_thread_invalidate_fetched_data(thread);
-}
-
-static bool cmdq_command_is_wfe(u64 cmd)
-{
- u64 wfe_option = CMDQ_WFE_UPDATE | CMDQ_WFE_WAIT | CMDQ_WFE_WAIT_VALUE;
- u64 wfe_op = (u64)(CMDQ_CODE_WFE << CMDQ_OP_CODE_SHIFT) << 32;
- u64 wfe_mask = (u64)CMDQ_OP_CODE_MASK << 32 | 0x;
-
- return ((cmd & wfe_mask) == (wfe_op | wfe_option));
-}
-
-/* we assume tasks in the same display GCE thread are waiting the same event.
*/
-static void cmdq_task_remove_wfe(struct cmdq_task *task)
-{
- struct device *dev = task->cmdq->mbox.dev;
- u64 *base = task->pkt->va_base;
- int i;
-
- dma_sync_single_for_cpu(dev, task->pa_base, task->pkt->cmd_buf_size,
- DMA_TO_DEVICE);
- for (i = 0; i < CMDQ_NUM_CMD(task->pkt); i++)
- if (cmdq_command_is_wfe(base[i]))
- base[i] = (u64)CMDQ_JUMP_BY_OFFSET << 32 |
- CMDQ_JUMP_PASS;
- dma_sync_single_for_device(dev, task->pa_base, task->pkt->cmd_buf_size,
- DMA_TO_DEVICE);
-}
-
static bool cmdq_thread_is_in_wfe(struct cmdq_thread *thread)
{
return readl(thread->base + CMDQ_THR_WAIT_TOKEN) & CMDQ_THR_IS_WAITING;
}
-static void cmdq_thread_wait_end(struct cmdq_thread *thread,
-unsigned long end_pa)
-{
- struct device *dev = thread->chan->mbox->dev;
- unsigned long curr_pa;
-
- if (readl_poll_timeout_atomic(thread->base + CMDQ_THR_CURR_ADDR,
- curr_pa, curr_pa == end_pa, 1, 20))
- dev_err(dev, "GCE thread cannot run to end.\n");
-}
-
-static void cmdq_task_exec_done(struct cmdq_task *task, enum cmdq_cb_status
sta)
-{
- struct cmdq_task_cb *cb = &task->pkt->async_cb;
- struct cmdq_cb_data data;
-
- WARN_ON(cb->cb == (cmdq_async_flush_cb)NULL);
- data.sta = sta;
- data.data = cb->data;
- cb->cb(data);
-
- list_del(&task->list_entry);
-}
-
-static void cmdq_task_handle_error(struct cmdq_task *task)
-{
- struct cmdq_thread *thread = task->thread;
- struct
