Messages in this thread | | | From | Jason-JH Lin (林睿祥) <> | Subject | Re: [PATCH] mailbox: mtk-cmdq-mailbox: Implement Runtime PM with autosuspend | Date | Thu, 12 Oct 2023 08:00:39 +0000 |
| |
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.
> > 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?
> + > 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().
Thanks for your improvement patch again!
and
Tested-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
Regards, Jason-JH.Lin
| |