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: >>> 3) 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
