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

[Philipp Zabel]

Hi Stefan,

On Wed, 2018-08-01 at 12:00 +0200, Stefan Agner wrote:
[...]
> > @@ -307,14 +335,15 @@ void mxsfb_plane_atomic_update(struct
> > mxsfb_drm_private *mxsfb,
> >  			drm_crtc_send_vblank_event(crtc, event);
> >  		}
> >  	}
> >  	spin_unlock_irq(&crtc->dev->event_lock);
> >  
> > -	if (!fb)
> > +	if (!mxsfb->enabled)
> >  		return;
> >  
> 
> I think this is the wrong thing to do.
> 
> The simple KMS helper callback ->update() is called by the
> ->atomic_update() callback of drm_plane_helper_funcs.
> 
> And the documentation of atomic_update() states:
> https://01.org/linuxgraphics/gfx-docs/drm/gpu/drm-kms-helpers.html#c.drm_plane_helper_funcs
> 
> "Note that the power state of the display pipe when this function is
> called depends upon the exact helpers and calling sequence the driver
> has picked. See drm_atomic_helper_commit_planes() for a discussion of
> the tradeoffs and variants of plane commit helpers."
> 
> The documentation of drm_atomic_helper_commit_planes() then has more
> information:
> https://01.org/linuxgraphics/gfx-docs/drm/gpu/drm-kms-helpers.html#c.drm_atomic_helper_commit_planes
> 
> Bottom line, we should be using drm_atomic_helper_commit_tail_rpm() for
> runtime pm...
> 
> So adding something like:
> 
> static const struct drm_mode_config_helper_funcs
> mxsfb_drm_mode_config_helpers = {
> 	.atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
> };
> 
> And add something like this in mxsfb_load:
> 
> 	drm->mode_config.funcs		= &mxsfb_mode_config_funcs;
> +	dev->mode_config.helper_private = &mxsfb_drm_mode_config_helpers;
> ...
> 
> Should make the stack not calling update while the pipe is disabled.
> 
> With that you do not have to keep state locally and can always apply the
> new state in ->enable().

Yes, thank you for the explanation. That is exactly what I would have
expected.

regards
Philipp