[PATCH v3 02/11] drm/tilcdc: implement palette loading for rev1

[Jyri Sarha]

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
> 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20161124/fbea7e11/attachment.sig>