Re: [PATCH] drm/msm/adreno: adreno_gpu: Use suspend() instead of idle() on load error

[Johan Hovold <johan@kernel.org>]

On Wed, Mar 29, 2023 at 10:45:52PM +0300, Dmitry Baryshkov wrote:
> On Wed, 29 Mar 2023 at 18:48, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
> > On 29.03.2023 16:37, Johan Hovold wrote:
> > > On Wed, Mar 29, 2023 at 04:04:44PM +0200, Konrad Dybcio wrote:
> > >> If we fail to initialize the GPU for whatever reason (say we don't
> > >> embed the GPU firmware files in the initrd), the error path involves
> > >> pm_runtime_put_sync() which then calls idle() instead of suspend().
> > >>
> > >> This is suboptimal, as it means that we're not going through the
> > >> clean shutdown sequence. With at least A619_holi, this makes the GPU
> > >> not wake up until it goes through at least one more start-fail-stop
> > >> cycle. Fix that by using pm_runtime_put_sync_suspend to force a clean
> > >> shutdown.
> > >
> > > This does not sound right. If pm_runtime_put_sync() fails to suspend the
> > > device when the usage count drops to zero, then you have a bug somewhere
> > > else.
> > I was surprised to see that it was not called as well, but I wasn't able
> > to track it down before..
> 
> Could you please check that it's autosuspend who kicks in? In other
> words, if we disable autosuspend, the pm_runtime_put_sync is enough()?

Yes, that's it. The runtime PM implementation changed at one point and
since you need to disable autosuspend first to actually get synchronous
behaviour. My bad.

> That would probably mean that we lack some kind of reset in the hw_init path.
> 
> On the other hand, I do not know how the device will react to the
> error-in-the-middle state. Modems for example, can enter the state
> where you can not properly turn it off once it starts the boot
> process.
> 
> And if we remember the efforts that Akhil has put into making sure
> that the GPU is properly reset in case of an _error_, it might be
> nearly impossible to shut it down in a proper way.
> 
> Thus said, I think that unless there is an obvious way to restart the
> init process, Korad's pm_runtime_put_sync_suspend() looks like a
> correct fix to me.

I'd prefer to fix this by disabling autosuspend, but as that would
involve also moving the call to enable autosuspend to this function (and
add the missing disable on unbind), Konrad's patch using
pm_runtime_put_sync_suspend() is probably the best option for now. I can
send a patch to move the autosuspend handling on top.

Perhaps you can just amend the commit message to clarify that not all fw
is apparently preloaded and also mention that you need to use
pm_runtime_put_sync_suspend() due to autosuspend being enabled.

Johan