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

[Jason-JH Lin (林睿祥)]

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@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?

> +
>  	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@xxxxxxxxxxxx>

Regards,
Jason-JH.Lin