Messages in this thread | | | Date | Thu, 12 Oct 2023 10:34:12 +0200 | Subject | Re: [PATCH] mailbox: mtk-cmdq-mailbox: Implement Runtime PM with autosuspend | From | AngeloGioacchino Del Regno <> |
| |
Il 12/10/23 10:00, Jason-JH Lin (林睿祥) ha scritto: > Hi Angelo, > > Thanks for your help! > > On Wed, 2023-10-11 at 13:03 +0200, AngeloGioacchino Del Regno wrote: >> MediaTek found an issue with display HW registers configuration, and >> located the reason in the CMDQ Mailbox driver; reporting the original >> comment with the analysis of this problem by Jason-JH Lin: >> >> 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 > > Please also help to add this comment here: > This abnormal case may configure display HW in the vactive or non-vblanking duration.
I can send a v2 to add the proposed line in the commit description...
...otherwise... Jassi, can you perhaps please add that while applying the commit, if there's no more feedback on this?
Asking for simplicity; I'm not against sending a v2, anyway.
> >> >> From his analysis we get that the GCE may finish its event processing >> after some amount of time (and not immediately after sending commands >> to it); since the GCE is used for more than just display, and it gets >> used frequently, solve this issue by implementing Runtime PM handlers >> with autosuspend: this allows us to overcome to the remote processor >> delay issues and reduce the clock enable()/disable() calls, while >> also >> still managing to save some power, which is something that we >> wouldn't >> be able to do if we just enable the GCE clocks at probe. >> >> Speaking of which: if Runtime PM is not available there will >> obviously >> be no way to get this power saving action so, in this case, the >> clocks >> will be enabled at probe() time, kept enabled for the entire driver's >> life and disabled at remove(). >> >> Reported-by: Jason-JH.Lin <jason-jh.lin@mediatek.com> >> Signed-off-by: AngeloGioacchino Del Regno < >> angelogioacchino.delregno@collabora.com> >> --- >> >> The 100ms autosuspend delay was estimated in a worst-case scenario >> and >> was tested on MT8192 and MT8195 Chromebooks, with internal display >> and >> external display running to maximize the number of CMDQ messages >> being >> sent through. >> That value can probably be decreased to half of what I've set, but on >> that I prefer being cautious and keep it at 100ms. >> >> P.S.: This also solves microstuttering issues that I've randomly seen >> on all MediaTek Chromebooks. >> >> drivers/mailbox/mtk-cmdq-mailbox.c | 80 +++++++++++++++++++++++++--- >> -- >> 1 file changed, 68 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c >> b/drivers/mailbox/mtk-cmdq-mailbox.c >> index 4d62b07c1411..de862e9137d5 100644 >> --- a/drivers/mailbox/mtk-cmdq-mailbox.c >> +++ b/drivers/mailbox/mtk-cmdq-mailbox.c >> @@ -13,10 +13,13 @@ >> #include <linux/kernel.h> >> #include <linux/module.h> >> #include <linux/platform_device.h> >> +#include <linux/pm_runtime.h> >> #include <linux/mailbox_controller.h> >> #include <linux/mailbox/mtk-cmdq-mailbox.h> >> #include <linux/of.h> >> >> +#define CMDQ_MBOX_AUTOSUSPEND_DELAY_MS 100 >> + >> #define CMDQ_OP_CODE_MASK (0xff << CMDQ_OP_CODE_SHIFT) >> #define CMDQ_NUM_CMD(t) (t->cmd_buf_size / >> CMDQ_INST_SIZE) >> #define CMDQ_GCE_NUM_MAX (2) >> @@ -283,10 +286,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) >> @@ -307,9 +308,26 @@ static irqreturn_t cmdq_irq_handler(int irq, >> void *dev) >> spin_unlock_irqrestore(&thread->chan->lock, flags); >> } >> >> + pm_runtime_mark_last_busy(cmdq->mbox.dev); > > Should we add this pm_runtime_put_autosuspend(cmdq->mbox.dev); here? >
The interrupt handler is guaranteed to execute (hence the interrupt is guaranteed to fire) before the suspend timer is estinguished: for this reason, it was not necessary to increase the runtime pm refcount in this function, hence it is not necessary to call pm_runtime_put_autosuspend() here.
>> + >> return IRQ_HANDLED; >> } >> >> +static int cmdq_runtime_resume(struct device *dev) >> +{ >> + struct cmdq *cmdq = dev_get_drvdata(dev); >> + >> + return clk_bulk_enable(cmdq->pdata->gce_num, cmdq->clocks); >> +} >> + >> +static int cmdq_runtime_suspend(struct device *dev) >> +{ >> + struct cmdq *cmdq = dev_get_drvdata(dev); >> + >> + clk_bulk_disable(cmdq->pdata->gce_num, cmdq->clocks); >> + return 0; >> +} >> + > > We can use them to replace the clk_bulk_enable() and clk_bulk_disable() > in cmdq_init(). >
No, because I expect those handlers to possibly grow in the future to handle more suspend/resume actions; besides, those are handlers, not generic helper functions, hence those shouldn't be called inside of other functions of this file, unless there's a good reason to do so (for example, letting this driver to still work without runtime PM support, which I'm doing in the probe() and remove() callbacks).
> Thanks for your improvement patch again! > > and > > Tested-by: Jason-JH.Lin <jason-jh.lin@mediatek.com> >
Thank you! Angelo
| |