On 24/11/16 12:38, Jyri Sarha wrote:
As long as the driver makes sure the device doesn't go to suspend (by having called pm_runtime_get).
Runtime get has always been called when modeset_nofb() is called.
Yes, runtime get is needed to access the HW, but the question here was "has runtime_get been active ever since setting the registers previously".
If you do
runtime_get(); setup_things(); runtime_put();
runtime_get(); setup_more_things(); runtime_put();
the code is broken as the context is possibly lost between those two blocks. Unless runtime suspend/resume callbacks do a full context save and restore.
Then it is a different issue, that I should probably put the same functionality into PM runtime_suspend() and runtime_resume() callbacks, that is currently in suspend() and resume() callbacks, to be ready if PM ever does anything more for LCDC that it does today. I could of course add a test if the registers are still intact before doing a restore.
You can do things in resume callback, but I think quite often it's simplest to just do those things when enabling the display. The device never goes to suspend if the display is enabled. And if you disable the display, the device should go to suspend, so usually enable is called after the device has been in suspend.
Well, the two places are pretty much the same thing in tilcdc, because enable calls pm_runtime_get(). Also with atomic the suspend/resume implementation is really straight forward. Just get the current atomic state with drm_atomic_helper_suspend() and commit it back in with drm_atomic_helper_resume(), if needed. I don't think I should implement myself something that is so well provided by pm and drm infrastructure.
The suspend/resume you're talking there is the system suspend/resume. That's quite different than the runtime suspend/resume, and they should do very different things.
The current system suspend/resume in tilcdc looks fine.
Tomi