Re: [PATCH 2/2] mailbox: mtk-cmdq: Move pm_runimte_get and put to mbox_chan_ops API

From: AngeloGioacchino Del Regno
Date: Mon Jun 24 2024 - 07:29:43 EST


Il 20/06/24 16:39, Jassi Brar ha scritto:
On Thu, Jun 20, 2024 at 1:33 AM Jason-JH Lin (林睿祥)
<Jason-JH.Lin@xxxxxxxxxxxx> wrote:

On Wed, 2024-06-19 at 10:38 -0500, Jassi Brar wrote:

External email : Please do not click links or open attachments until
you have verified the sender or the content.
On Wed, Jun 19, 2024 at 3:18 AM AngeloGioacchino Del Regno
<angelogioacchino.delregno@xxxxxxxxxxxxx> wrote:
Il 18/06/24 17:59, Jassi Brar ha scritto:
.....

For example, when static content is displayed on screen, the CMDQ
mailbox never
gets shut down, but no communication happens for a relatively long
time; the
overhead of actually shutting down the mailbox and setting it back
up would be
increasing latency in an unacceptable manner.

Hmm... in your driver, startup() is _empty_ and shutdown() is
all
under a spin-lock with irqs disabled, so that too shouldn't be
expensive. Right?
Then what causes unacceptable latencies?


I found that the BUG report only occurred when opening the camera APP.
Maybe something in imgsys_cmdq_sendtask() is too expensive or somewhere
else in imgsys driver.

If you move anything from submit() into startup(), which is once per
lifetime of a channel, it will only make imgsys_cmdq_sendtask()
cheaper.
Btw, the imgsys code is not public, I don't know how it looks.


I'm wondering why this BUG report is not occurred in display use case
which is more frequent than imgsys use case.
Does this mean sleep() is not always called in pm_runtime_get_sync()
and if we can guarantee this sleep() won't be called, then using
pm_runtime_get_sync() in atomic context is OK?

Instead of hacking around, maybe try using startup() and shutdown()
which is for such uses? Maybe request/release channel as part of RPM
in your client driver if you are worried about the noise?


This is why I opted for autosuspend - it's only bringing down
certain clocks for
the CMDQ HW, adding up a bit of power saving to the mix which, for
some use cases
such as mobile devices with relatively small batteries, is
definitely important.

I'll also briefly (and only briefly) mention that 120Hz displays
are already a
common thing and in this case the gap between TX and ACK is ~8.33ms
instead, let
alone that displays with a framerate of more than 120Hz also do
exist even though
they're less common.

I don't know how even busier channels help your point.

All of the above describes a few of the reasons why autosuspend is
a good choice
here, instead of a shutdown->startup flow.
And again - I can place some bets that PM would also be applicable
to SoCs from
other vendors as well, with most probably different benefits (but
still with some
power related benefits!) compared to MediaTek.

I agree with Angelo's point!

Ok, but you or Angelo still don't explain "unacceptable latencies"
when your startup() and shutdown() are empty. You just want api
changed, which we can but at least do you part and tell me where the
bottleneck (unexpected latency) comes from.


"I want numbers" is a sensible request, honestly I would do the same so I totally
understand that.

Jason, can you please perform latency measurements on 60Hz and *especially* ISP/cam
cases while "continuously" calling startup() and shutdown() for every power saving
operation?


Thanks,
Angelo