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

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

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