Messages in this thread | | | From | Jason-JH Lin (林睿祥) <> | Subject | Re: [PATCH v2] mailbox: remove runtime GCE clk control | Date | Wed, 11 Oct 2023 03:03:06 +0000 |
| |
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 >
| |