On 11/24/16 13:10, Tomi Valkeinen wrote: > 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. > Yes, I know that. In the atomic driver the setup is always done in the drm_mode_config_funcs atomic_commit. The tilcdc commit takes does runtime_get() there, before the setup starts, and does a runtime_put() after drm_atomic_helper_commit_modeset_enables() has run. The drm_atomic_helper_commit_modeset_enables() has turned the display on, if necessary, an that will keep the HW powered until the display is turned off by another commit. Now, I am not sure if drm atomic core assumes optimizes the mode setting in commit, if there previously has been >>>> 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. > I don't undestand the same mechanism could not be used for runtime resume. Why should it matter if we configure the previous drm atomic state after system suspend or simple LCDC power down suspend? There may be some need for differentiation with more complex display hardware, but I see no such need for tilcdc. > Tomi >
Attachment:
signature.asc
Description: OpenPGP digital signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
