| Message ID | 20231004085430.19538-1-jason-jh.lin@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] mailbox: remove runtime GCE clk control | expand |
Il 04/10/23 10:54, Jason-JH.Lin ha scritto: > 1. GCE is a frequently used module, so runtime controlling > GCE clock won't save too much power and its original design > doesn't expect it to be enabled and disabled too frequently. > > 2. Runtime controlling GCE clock will cause display HW register > configured in worng stream done event issue below: > GCE should config HW in every vblanking duration. > The stream done event is the start signal of vblanking. > > If stream done event is sent between GCE clk_disable > and clk_enable. After GCE clk_enable the stream done event > may not appear immediately and have about 3us delay. > > Normal case: > clk_disable -> get EventA -> clk_enable -> clear EventA > -> wait EventB -> get EventB -> config HW > > Abnormal case: > clk_disable -> get EventA -> clk_enable -> EventA delay appear > -> clear EventA fail -> wait EventB but get EventA -> config HW > > So just remove the runtime GCE clock contorl. > > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com> Instead of entirely removing the logic that controls the clocks and always refuse to save power, what about using autosuspend? If the two cases that you're describing are happening always in a range of time, we could *yes* remove the "manual" bulk disable/enable calls, but then we could use runtime_suspend/runtime_resume callbacks for that. Hint: pm_runtime_set_autosuspend_delay(dev, 1000); Regards, Angelo > --- > v1 -> v2: > 1. Rebase on Linux 6.6-rc4 > 2. Adjust commit message. > --- > drivers/mailbox/mtk-cmdq-mailbox.c | 18 +++++------------- > 1 file changed, 5 insertions(+), 13 deletions(-) > > diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c b/drivers/mailbox/mtk-cmdq-mailbox.c > index 4d62b07c1411..a3c2d318beb7 100644 > --- a/drivers/mailbox/mtk-cmdq-mailbox.c > +++ b/drivers/mailbox/mtk-cmdq-mailbox.c > @@ -140,7 +140,6 @@ static void cmdq_init(struct cmdq *cmdq) > int i; > u32 gctl_regval = 0; > > - WARN_ON(clk_bulk_enable(cmdq->pdata->gce_num, cmdq->clocks)); > if (cmdq->pdata->control_by_sw) > gctl_regval = GCE_CTRL_BY_SW; > if (cmdq->pdata->sw_ddr_en) > @@ -152,7 +151,6 @@ static void cmdq_init(struct cmdq *cmdq) > writel(CMDQ_THR_ACTIVE_SLOT_CYCLES, cmdq->base + CMDQ_THR_SLOT_CYCLES); > for (i = 0; i <= CMDQ_MAX_EVENT; i++) > writel(i, cmdq->base + CMDQ_SYNC_TOKEN_UPDATE); > - clk_bulk_disable(cmdq->pdata->gce_num, cmdq->clocks); > } > > static int cmdq_thread_reset(struct cmdq *cmdq, struct cmdq_thread *thread) > @@ -283,10 +281,8 @@ static void cmdq_thread_irq_handler(struct cmdq *cmdq, > break; > } > > - if (list_empty(&thread->task_busy_list)) { > + if (list_empty(&thread->task_busy_list)) > cmdq_thread_disable(cmdq, thread); > - clk_bulk_disable(cmdq->pdata->gce_num, cmdq->clocks); > - } > } > > static irqreturn_t cmdq_irq_handler(int irq, void *dev) > @@ -333,7 +329,7 @@ static int cmdq_suspend(struct device *dev) > if (cmdq->pdata->sw_ddr_en) > cmdq_sw_ddr_enable(cmdq, false); > > - clk_bulk_unprepare(cmdq->pdata->gce_num, cmdq->clocks); > + clk_bulk_disable_unprepare(cmdq->pdata->gce_num, cmdq->clocks); > > return 0; > } > @@ -342,7 +338,7 @@ static int cmdq_resume(struct device *dev) > { > struct cmdq *cmdq = dev_get_drvdata(dev); > > - WARN_ON(clk_bulk_prepare(cmdq->pdata->gce_num, cmdq->clocks)); > + WARN_ON(clk_bulk_prepare_enable(cmdq->pdata->gce_num, cmdq->clocks)); > cmdq->suspended = false; > > if (cmdq->pdata->sw_ddr_en) > @@ -358,7 +354,7 @@ static int cmdq_remove(struct platform_device *pdev) > if (cmdq->pdata->sw_ddr_en) > cmdq_sw_ddr_enable(cmdq, false); > > - clk_bulk_unprepare(cmdq->pdata->gce_num, cmdq->clocks); > + clk_bulk_disable_unprepare(cmdq->pdata->gce_num, cmdq->clocks); > return 0; > } > > @@ -384,8 +380,6 @@ static int cmdq_mbox_send_data(struct mbox_chan *chan, void *data) > task->pkt = pkt; > > if (list_empty(&thread->task_busy_list)) { > - WARN_ON(clk_bulk_enable(cmdq->pdata->gce_num, cmdq->clocks)); > - > /* > * The thread reset will clear thread related register to 0, > * including pc, end, priority, irq, suspend and enable. Thus > @@ -457,7 +451,6 @@ static void cmdq_mbox_shutdown(struct mbox_chan *chan) > } > > cmdq_thread_disable(cmdq, thread); > - clk_bulk_disable(cmdq->pdata->gce_num, cmdq->clocks); > > done: > /* > @@ -497,7 +490,6 @@ static int cmdq_mbox_flush(struct mbox_chan *chan, unsigned long timeout) > > cmdq_thread_resume(thread); > cmdq_thread_disable(cmdq, thread); > - clk_bulk_disable(cmdq->pdata->gce_num, cmdq->clocks); > > out: > spin_unlock_irqrestore(&thread->chan->lock, flags); > @@ -631,7 +623,7 @@ static int cmdq_probe(struct platform_device *pdev) > > platform_set_drvdata(pdev, cmdq); > > - WARN_ON(clk_bulk_prepare(cmdq->pdata->gce_num, cmdq->clocks)); > + WARN_ON(clk_bulk_prepare_enable(cmdq->pdata->gce_num, cmdq->clocks)); > > cmdq_init(cmdq); >
Hi Angelo, Thanks for the reviews. On Wed, 2023-10-04 at 11:20 +0200, AngeloGioacchino Del Regno wrote: > Il 04/10/23 10:54, Jason-JH.Lin ha scritto: > > 1. GCE is a frequently used module, so runtime controlling > > GCE clock won't save too much power and its original design > > doesn't expect it to be enabled and disabled too frequently. > > > > 2. Runtime controlling GCE clock will cause display HW register > > configured in worng stream done event issue below: > > GCE should config HW in every vblanking duration. > > The stream done event is the start signal of vblanking. > > > > If stream done event is sent between GCE clk_disable > > and clk_enable. After GCE clk_enable the stream done event > > may not appear immediately and have about 3us delay. > > > > Normal case: > > clk_disable -> get EventA -> clk_enable -> clear EventA > > -> wait EventB -> get EventB -> config HW > > > > Abnormal case: > > clk_disable -> get EventA -> clk_enable -> EventA delay appear > > -> clear EventA fail -> wait EventB but get EventA -> config HW > > > > So just remove the runtime GCE clock contorl. > > > > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com> > > Instead of entirely removing the logic that controls the clocks and > always > refuse to save power, what about using autosuspend? > > If the two cases that you're describing are happening always in a > range of > time, we could *yes* remove the "manual" bulk disable/enable calls, > but then > we could use runtime_suspend/runtime_resume callbacks for that. > > Hint: pm_runtime_set_autosuspend_delay(dev, 1000); > > Regards, > Angelo > These 2 issues are caused by GCE bulk_clk enable/disable too frequently. As I now, pm_runtime_set_autosuspend_delay() is for controlling the power domain. The power domain of GCE is infrasys which can only be enabled/disabled by spm during the whole system resume/suspend. So I'm not sure about how can pm_runtime_set_autosuspend_delay() save power for GCE bulk_clk in this case. Could you give more hint for me please? Regards, Jason-JH.Lin
Il 06/10/23 11:15, Jason-JH Lin (林睿祥) ha scritto: > Hi Angelo, > > Thanks for the reviews. > > On Wed, 2023-10-04 at 11:20 +0200, AngeloGioacchino Del Regno wrote: >> Il 04/10/23 10:54, Jason-JH.Lin ha scritto: >>> 1. GCE is a frequently used module, so runtime controlling >>> GCE clock won't save too much power and its original design >>> doesn't expect it to be enabled and disabled too frequently. >>> >>> 2. Runtime controlling GCE clock will cause display HW register >>> configured in worng stream done event issue below: >>> GCE should config HW in every vblanking duration. >>> The stream done event is the start signal of vblanking. >>> >>> If stream done event is sent between GCE clk_disable >>> and clk_enable. After GCE clk_enable the stream done event >>> may not appear immediately and have about 3us delay. >>> >>> Normal case: >>> clk_disable -> get EventA -> clk_enable -> clear EventA >>> -> wait EventB -> get EventB -> config HW >>> >>> Abnormal case: >>> clk_disable -> get EventA -> clk_enable -> EventA delay appear >>> -> clear EventA fail -> wait EventB but get EventA -> config HW >>> >>> So just remove the runtime GCE clock contorl. >>> >>> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com> >> >> Instead of entirely removing the logic that controls the clocks and >> always >> refuse to save power, what about using autosuspend? >> >> If the two cases that you're describing are happening always in a >> range of >> time, we could *yes* remove the "manual" bulk disable/enable calls, >> but then >> we could use runtime_suspend/runtime_resume callbacks for that. >> >> Hint: pm_runtime_set_autosuspend_delay(dev, 1000); >> >> Regards, >> Angelo >> > > These 2 issues are caused by GCE bulk_clk enable/disable too > frequently. > > As I now, pm_runtime_set_autosuspend_delay() is for controlling the > power domain. The power domain of GCE is infrasys which can only be > enabled/disabled by spm during the whole system resume/suspend. > So I'm not sure about how can pm_runtime_set_autosuspend_delay() save > power for GCE bulk_clk in this case. > > Could you give more hint for me please? > I think it's faster if I send my own version of that, I'm testing that right now on multiple Chromebooks to check if this solves the issue that you're describing, which I believe it to be the "apparent random lockups" or display stuttering when in a high load situation. I don't seem to get any more stuttering nor apparent random lockups on a MT8195 Chromebook, and that's with my pm_runtime autosuspend solution, with a runtime suspend delay of 100ms, which I'm trying to decrease as much as possible in order to keep saving as much power as possible. For this, if you could better describe how to reliably reproduce the issue that you have described, it would help me a bit in making this as good as possible. Thanks, Angelo
On Tue, 2023-10-10 at 13:52 +0200, AngeloGioacchino Del Regno wrote: > Il 06/10/23 11:15, Jason-JH Lin (林睿祥) ha scritto: > > Hi Angelo, > > > > Thanks for the reviews. > > > > On Wed, 2023-10-04 at 11:20 +0200, AngeloGioacchino Del Regno > > wrote: > > > Il 04/10/23 10:54, Jason-JH.Lin ha scritto: > > > > 1. GCE is a frequently used module, so runtime controlling > > > > GCE clock won't save too much power and its original design > > > > doesn't expect it to be enabled and disabled too frequently. > > > > > > > > 2. Runtime controlling GCE clock will cause display HW register > > > > configured in worng stream done event issue below: > > > > GCE should config HW in every vblanking duration. > > > > The stream done event is the start signal of vblanking. > > > > > > > > If stream done event is sent between GCE clk_disable > > > > and clk_enable. After GCE clk_enable the stream done event > > > > may not appear immediately and have about 3us delay. > > > > > > > > Normal case: > > > > clk_disable -> get EventA -> clk_enable -> clear EventA > > > > -> wait EventB -> get EventB -> config HW > > > > > > > > Abnormal case: > > > > clk_disable -> get EventA -> clk_enable -> EventA delay > > > > appear > > > > -> clear EventA fail -> wait EventB but get EventA -> > > > > config HW > > > > > > > > So just remove the runtime GCE clock contorl. > > > > > > > > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com> > > > > > > Instead of entirely removing the logic that controls the clocks > > > and > > > always > > > refuse to save power, what about using autosuspend? > > > > > > If the two cases that you're describing are happening always in a > > > range of > > > time, we could *yes* remove the "manual" bulk disable/enable > > > calls, > > > but then > > > we could use runtime_suspend/runtime_resume callbacks for that. > > > > > > Hint: pm_runtime_set_autosuspend_delay(dev, 1000); > > > > > > Regards, > > > Angelo > > > > > > > These 2 issues are caused by GCE bulk_clk enable/disable too > > frequently. > > > > As I now, pm_runtime_set_autosuspend_delay() is for controlling the > > power domain. The power domain of GCE is infrasys which can only be > > enabled/disabled by spm during the whole system resume/suspend. > > So I'm not sure about how can pm_runtime_set_autosuspend_delay() > > save > > power for GCE bulk_clk in this case. > > > > Could you give more hint for me please? > > > > I think it's faster if I send my own version of that, I'm testing > that right now > on multiple Chromebooks to check if this solves the issue that you're > describing, > which I believe it to be the "apparent random lockups" or display > stuttering when > in a high load situation. Thanks for your help, I can reproduce it with your version. > > I don't seem to get any more stuttering nor apparent random lockups > on a MT8195 > Chromebook, and that's with my pm_runtime autosuspend solution, with > a runtime > suspend delay of 100ms, which I'm trying to decrease as much as > possible in order > to keep saving as much power as possible. > > For this, if you could better describe how to reliably reproduce the > issue that > you have described, it would help me a bit in making this as good as > possible. After reverting this modification, you'll see 2 issues on MT8188 and MT8195 probability: 1. A flicker shift may occur randomly when moving the cursor on the edge of external DP monitor rapidly. 2. A premature wake caused by CMDQ during the Suspend/Resume test randomly. I'll send you the detail reproduce steps privately. Regards, Jason-JH.Lin > > Thanks, > Angelo >
diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c b/drivers/mailbox/mtk-cmdq-mailbox.c index 4d62b07c1411..a3c2d318beb7 100644 --- a/drivers/mailbox/mtk-cmdq-mailbox.c +++ b/drivers/mailbox/mtk-cmdq-mailbox.c @@ -140,7 +140,6 @@ static void cmdq_init(struct cmdq *cmdq) int i; u32 gctl_regval = 0; - WARN_ON(clk_bulk_enable(cmdq->pdata->gce_num, cmdq->clocks)); if (cmdq->pdata->control_by_sw) gctl_regval = GCE_CTRL_BY_SW; if (cmdq->pdata->sw_ddr_en) @@ -152,7 +151,6 @@ static void cmdq_init(struct cmdq *cmdq) writel(CMDQ_THR_ACTIVE_SLOT_CYCLES, cmdq->base + CMDQ_THR_SLOT_CYCLES); for (i = 0; i <= CMDQ_MAX_EVENT; i++) writel(i, cmdq->base + CMDQ_SYNC_TOKEN_UPDATE); - clk_bulk_disable(cmdq->pdata->gce_num, cmdq->clocks); } static int cmdq_thread_reset(struct cmdq *cmdq, struct cmdq_thread *thread) @@ -283,10 +281,8 @@ static void cmdq_thread_irq_handler(struct cmdq *cmdq, break; } - if (list_empty(&thread->task_busy_list)) { + if (list_empty(&thread->task_busy_list)) cmdq_thread_disable(cmdq, thread); - clk_bulk_disable(cmdq->pdata->gce_num, cmdq->clocks); - } } static irqreturn_t cmdq_irq_handler(int irq, void *dev) @@ -333,7 +329,7 @@ static int cmdq_suspend(struct device *dev) if (cmdq->pdata->sw_ddr_en) cmdq_sw_ddr_enable(cmdq, false); - clk_bulk_unprepare(cmdq->pdata->gce_num, cmdq->clocks); + clk_bulk_disable_unprepare(cmdq->pdata->gce_num, cmdq->clocks); return 0; } @@ -342,7 +338,7 @@ static int cmdq_resume(struct device *dev) { struct cmdq *cmdq = dev_get_drvdata(dev); - WARN_ON(clk_bulk_prepare(cmdq->pdata->gce_num, cmdq->clocks)); + WARN_ON(clk_bulk_prepare_enable(cmdq->pdata->gce_num, cmdq->clocks)); cmdq->suspended = false; if (cmdq->pdata->sw_ddr_en) @@ -358,7 +354,7 @@ static int cmdq_remove(struct platform_device *pdev) if (cmdq->pdata->sw_ddr_en) cmdq_sw_ddr_enable(cmdq, false); - clk_bulk_unprepare(cmdq->pdata->gce_num, cmdq->clocks); + clk_bulk_disable_unprepare(cmdq->pdata->gce_num, cmdq->clocks); return 0; } @@ -384,8 +380,6 @@ static int cmdq_mbox_send_data(struct mbox_chan *chan, void *data) task->pkt = pkt; if (list_empty(&thread->task_busy_list)) { - WARN_ON(clk_bulk_enable(cmdq->pdata->gce_num, cmdq->clocks)); - /* * The thread reset will clear thread related register to 0, * including pc, end, priority, irq, suspend and enable. Thus @@ -457,7 +451,6 @@ static void cmdq_mbox_shutdown(struct mbox_chan *chan) } cmdq_thread_disable(cmdq, thread); - clk_bulk_disable(cmdq->pdata->gce_num, cmdq->clocks); done: /* @@ -497,7 +490,6 @@ static int cmdq_mbox_flush(struct mbox_chan *chan, unsigned long timeout) cmdq_thread_resume(thread); cmdq_thread_disable(cmdq, thread); - clk_bulk_disable(cmdq->pdata->gce_num, cmdq->clocks); out: spin_unlock_irqrestore(&thread->chan->lock, flags); @@ -631,7 +623,7 @@ static int cmdq_probe(struct platform_device *pdev) platform_set_drvdata(pdev, cmdq); - WARN_ON(clk_bulk_prepare(cmdq->pdata->gce_num, cmdq->clocks)); + WARN_ON(clk_bulk_prepare_enable(cmdq->pdata->gce_num, cmdq->clocks)); cmdq_init(cmdq);
1. GCE is a frequently used module, so runtime controlling GCE clock won't save too much power and its original design doesn't expect it to be enabled and disabled too frequently. 2. Runtime controlling GCE clock will cause display HW register configured in worng stream done event issue below: GCE should config HW in every vblanking duration. The stream done event is the start signal of vblanking. If stream done event is sent between GCE clk_disable and clk_enable. After GCE clk_enable the stream done event may not appear immediately and have about 3us delay. Normal case: clk_disable -> get EventA -> clk_enable -> clear EventA -> wait EventB -> get EventB -> config HW Abnormal case: clk_disable -> get EventA -> clk_enable -> EventA delay appear -> clear EventA fail -> wait EventB but get EventA -> config HW So just remove the runtime GCE clock contorl. Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com> --- v1 -> v2: 1. Rebase on Linux 6.6-rc4 2. Adjust commit message. --- drivers/mailbox/mtk-cmdq-mailbox.c | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-)