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

[Jyri Sarha]

On 11/24/16 12:25, Tomi Valkeinen wrote:
> On 24/11/16 12:03, Jyri Sarha wrote:
>> On 11/24/16 11:43, Tomi Valkeinen wrote:
>>> What is the difference? If mode changes, you need to disable and enable
>>> the crtc, right? What other cases there are to enable the display? After
>>> blank? Then the display has been off, and I presume palette has to be
>>> loaded.
>>
>> At the moment the palette or register values do not appear to vanish
>> ever. But that is probably due to PM not doing much to optimize the LCDC
>> power consumption.
> 
> If runtime PM for the device goes to suspend, you have to presume the IP
> has lost all context. That may not always happen, but that's what the
> driver has to presume, unless there's a way for the driver to verify
> whether the context has been lost or not.
> 

There is couple of registers I can verify that from (reset value !=
value always used by the driver).

>> Anyway, if simple enable is enough to turn on the display - all video
>> timings, frame buffer dma addresses etc. are already in the registers -
>> then I think it is safe to assume that the palette is still in there too.
> 
> 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.

> 
>> 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.

I will implement that as soon as I get this current mess with LCDC rev 1
and bridge support sorted out.

> So, I haven't looked at the tilcdc code in detail in this regard, but my
> guess is that runtime PM resume could be used to set hardcoded things to
> registers, stuff that you always know how they are set. Everything else
> can be just programmed at enable.
> 
> Looking at the registers to find out if they're intact is fine, but it's
> just an optimization.
> 

Yes, of course.

BR,
Jyri

-------------- 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/c71c2559/attachment-0001.sig>