Maxime,
First, sorry for the tone in my previous message. I wrote it too hastily.
On 2/11/20 2:26 AM, Maxime Ripard wrote:
On Tue, Feb 11, 2020 at 01:28:58AM -0600, Samuel Holland wrote:
The driver relies on being suspended when sun6i_dsi_encoder_enable() is called. The resume callback has a comment that says:
Some part of it can only be done once we get a number of lanes, see sun6i_dsi_inst_init
And then part of the resume callback only runs if dsi->device is not NULL (that is, if sun6i_dsi_attach() has been called). However, as the above call graph shows, the resume callback is guaranteed to be called before sun6i_dsi_attach(); it is called before child devices get their drivers attached.
Isn't it something that has been changed by your previous patch though?
No. Before the previous patch, sun6i_dsi_bind() requires sun6i_dsi_attach() to have been called first. So either the panel driver is not loaded, and issue #2 happens, or the panel driver is loaded, and you get the following modification to the above call graph:
mipi_dsi_host_register() ... __device_attach() pm_runtime_get_sync(dev->parent) -> Causes resume bus_for_each_drv() __device_attach_driver() [panel probe function] mipi_dsi_attach() sun6i_dsi_attach() pm_runtime_put(dev->parent) -> Async idle request component_add() ... sun6i_dsi_bind() ... sun6i_dsi_encoder_enable() pm_runtime_get_sync() -> Cancels idle request
And because `dev->power.runtime_status == RPM_ACTIVE` still, the callback is *not* run. Either way you have the same problem.
While the scenario I described is possible (since an unbounded amount of other work could be queued to pm_wq), I did more testing without these patches, and I could never trigger it. No matter what combination of module/built-in drivers I used, there was always enough time between mipi_dsi_host_register() and sun6i_dsi_encoder_enable() for the device to be suspended. So in practice sun6i_dsi_inst_init() was always called during boot.
Therefore, part of the controller initialization will only run if the device is suspended between the calls to mipi_dsi_host_register() and component_add() (which ends up calling sun6i_dsi_encoder_enable()). Again, as shown by the above call graph, this is not the case. It appears that the controller happens to work because it is still initialized by the bootloader.
We don't have any bootloader support for MIPI-DSI, so no, that's not it.
You are correct here. sun6i_dsi_inst_init() was indeed being called at boot. So my commit log is wrong.
Because the connector is hardcoded to always be connected, the device's runtime PM reference is not dropped until system suspend, when sun4i_drv_drm_sys_suspend() ends up calling sun6i_dsi_encoder_disable(). However, that is done as a system sleep PM hook, and at that point the system PM core has already taken another runtime PM reference, so sun6i_dsi_runtime_suspend() is not called. Likewise, by the time the PM core releases its reference, sun4i_drv_drm_sys_resume() has already re-enabled the encoder.
So after system suspend and resume, we have *still never called* sun6i_dsi_inst_init(), and now that the rest of the display pipeline has been reset, the DSI host is unable to communicate with the panel, causing VBLANK timeouts.
Either way, I guess just moving the pm_runtime_enable call to sun6i_dsi_attach will fix this, right? We don't really need to have the DSI controller powered up before that time anyway.
No. It would solve issue #2 (only if the previous patch is applied), but not issue #3.
Regardless of when runtime PM is enabled, sun6i_dsi_runtime_suspend() will not be called until the device's usage count drops to 0. And as long as a panel is bound, the controller's usage count will be >0, *even during system suspend* while the encoder is turned off.
Before the previous patch, the usage count would never drop to 0 under *any* circumstance.
FWIW, Samuel