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

[AngeloGioacchino Del Regno]

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