Re: [PATCH v2] mailbox: remove runtime GCE clk control

["Jason-JH Lin (林睿祥)" <Jason-JH.Lin@mediatek.com>]

On Tue, 2023-10-10 at 13:52 +0200, AngeloGioacchino Del Regno wrote:
> Il 06/10/23 11:15, Jason-JH Lin (林睿祥) ha scritto:
> > Hi Angelo,
> > 
> > Thanks for the reviews.
> > 
> > On Wed, 2023-10-04 at 11:20 +0200, AngeloGioacchino Del Regno
> > wrote:
> > > Il 04/10/23 10:54, Jason-JH.Lin ha scritto:
> > > > 1. GCE is a frequently used module, so runtime controlling
> > > > GCE clock won't save too much power and its original design
> > > > doesn't expect it to be enabled and disabled too frequently.
> > > > 
> > > > 2. Runtime controlling GCE clock will cause display HW register
> > > > configured in worng stream done event issue below:
> > > >     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
> > > > 
> > > > So just remove the runtime GCE clock contorl.
> > > > 
> > > > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > > 
> > > Instead of entirely removing the logic that controls the clocks
> > > and
> > > always
> > > refuse to save power, what about using autosuspend?
> > > 
> > > If the two cases that you're describing are happening always in a
> > > range of
> > > time, we could *yes* remove the "manual" bulk disable/enable
> > > calls,
> > > but then
> > > we could use runtime_suspend/runtime_resume callbacks for that.
> > > 
> > > Hint: pm_runtime_set_autosuspend_delay(dev, 1000);
> > > 
> > > Regards,
> > > Angelo
> > > 
> > 
> > These 2 issues are caused by GCE bulk_clk enable/disable too
> > frequently.
> > 
> > As I now, pm_runtime_set_autosuspend_delay() is for controlling the
> > power domain. The power domain of GCE is infrasys which can only be
> > enabled/disabled by spm during the whole system resume/suspend.
> > So I'm not sure about how can pm_runtime_set_autosuspend_delay()
> > save
> > power for GCE bulk_clk in this case.
> > 
> > Could you give more hint for me please?
> > 
> 
> I think it's faster if I send my own version of that, I'm testing
> that right now
> on multiple Chromebooks to check if this solves the issue that you're
> describing,
> which I believe it to be the "apparent random lockups" or display
> stuttering when
> in a high load situation.

Thanks for your help, I can reproduce it with your version.

> 
> I don't seem to get any more stuttering nor apparent random lockups
> on a MT8195
> Chromebook, and that's with my pm_runtime autosuspend solution, with
> a runtime
> suspend delay of 100ms, which I'm trying to decrease as much as
> possible in order
> to keep saving as much power as possible.
> 
> For this, if you could better describe how to reliably reproduce the
> issue that
> you have described, it would help me a bit in making this as good as
> possible.

After reverting this modification, you'll see 2 issues on MT8188 and
MT8195 probability:
1. A flicker shift may occur randomly when moving the cursor on the
edge of external DP monitor rapidly.
2. A premature wake caused by CMDQ during the Suspend/Resume test
randomly.

I'll send you the detail reproduce steps privately.

Regards,
Jason-JH.Lin

> 
> Thanks,
> Angelo
>