lkml.org 
[lkml]   [2023]   [Oct]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH] mailbox: mtk-cmdq-mailbox: Implement Runtime PM with autosuspend
From
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

\
 
 \ /
  Last update: 2023-10-12 10:34    [W:0.053 / U:0.044 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site