Re: [PATCH] mailbox: mtk-cmdq-mailbox: Implement Runtime PM with autosuspend

[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@xxxxxxxxxxxx>
> > Signed-off-by: AngeloGioacchino Del Regno <
> > angelogioacchino.delregno@xxxxxxxxxxxxx>
> > ---
> >
> > 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@xxxxxxxxxxxx>

Thank you!
Angelo