Re: [PATCH v2] drm/mxsfb: Fix runtime PM for unpowering lcdif block

[Leonard Crestez]

On Thu, 2018-08-02 at 13:06 +0200, Stefan Agner wrote:
> On 02.08.2018 12:17, Philipp Zabel wrote:
> > On Tue, 2018-07-31 at 12:17 +0000, Leonard Crestez wrote:
> > > On Tue, 2018-07-31 at 13:54 +0200, Philipp Zabel wrote:
> > > > On Tue, 2018-07-17 at 13:48 +0300, Leonard Crestez wrote:
> > > > > Adding lcdif nodes to a power domain currently does work, it results in
> > > > > black/corrupted screens or hangs. While the driver does enable runtime
> > > > > pm it does not deal correctly with the block being unpowered.
> > > > > 
> > > > > Ensure power is on when required by adding pm_runtime_get/put_sync to
> > > > > mxsfb_pipe_enable/disable.
> > > > > 
> > > > > Since power is lost on suspend implement PM_SLEEP_OPS using
> > > > > drm_mode_config_helper_suspend/resume.
> > > > > 
> > > > > The mxsfb_plane_atomic_update function can get called before
> > > > > mxsfb_pipe_enable while the block is not yet powered. When this happens
> > > > > the write to LCDIF_NEXT_BUF is lost causing corrupt display on unblank
> > > > > until a refresh.
> > > > 
> > > > Why does this happen?
> > > 
> > > I'm not sure what you're asking but register writes to unpowered or
> > > unclocked blocks are not expected to "just work". Here the write is
> > > ignored/lost but I think on imx8 it can even cause a bus error.
> > > 
> > > The approach here is to only set the framebuffer address as part of
> > > activating the display.
> > 
> > I wonder why atomic update is called at all while the pipe is not
> > enabled.
> 
> It can be made to behave differently (see also my review).

Setting the framebuffer before enabling the crtc makes sense to me,
otherwise an initial corrupt frame would be impossible to avoid.

It's just that it slightly complicates PM for simple devices. Maybe
drm_simple_display_pipe could have prepare/unprepare callbacks for
PM+modeset separate from just crtc enable/disable?