[PATCH] drm/atomic-helper: Add option to update planes only on active crtc

[Daniel Vetter]

On Mon, Jul 27, 2015 at 02:44:31PM +0200, Thierry Reding wrote:
> On Wed, Jul 22, 2015 at 06:02:27PM +0200, Daniel Vetter wrote:
> [...]
> > diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> > index c6276aebfec3..783edc242648 100644
> > --- a/drivers/gpu/drm/tegra/drm.c
> > +++ b/drivers/gpu/drm/tegra/drm.c
> > @@ -56,7 +56,7 @@ static void tegra_atomic_complete(struct tegra_drm *tegra,
> >  	 */
> >  
> >  	drm_atomic_helper_commit_modeset_disables(drm, state);
> > -	drm_atomic_helper_commit_planes(drm, state);
> > +	drm_atomic_helper_commit_planes(drm, state, false);
> >  	drm_atomic_helper_commit_modeset_enables(drm, state);
> >  
> >  	drm_atomic_helper_wait_for_vblanks(drm, state);
> 
> I tried to give this a go with my ongoing DPMS work and I can't make
> this work. I think we'll need something more involved to fix this
> properly with runtime PM enabled drivers.
> 
> The reason why this isn't working on Tegra is because with the rework
> done for DPMS, the CRTC will be put into reset in ->disable() and taken
> out of reset in ->enable() (eventually I suspect that it'd also be
> powergated in ->disable() and unpowergated in ->enable(), but that
> should have no further side-effects than the reset).
> 
> There are two cases where this leads to problems, though as far as I can
> tell they are the same problem: with legacy fbdev enabled, the initial
> modeset works fine (though that's only because the mode is actually set
> twice due to the monitor pulsing HPD). If I then run modetest with the
> same mode set as fbdev, I also see planes updated properly. Now after I
> exit modetest it will do a modeset (it's what modetest does, no matter
> if the mode it did set matches fbdev, not sure if that's really desired)
> and that ends up with no primary plane being set.
> 
> The second case where I've seen this happen is with legacy fbdev
> disabled. In that case, running modetest won't ever display the correct
> plane. I'm somewhat surprised that it even works, given that the CRTC to
> which the plane's registers belong is in reset and has clocks disabled
> at the time of the ->atomic_update() call. I suspect this could be
> related to the fact that we're accessing only plane registers, and they
> go through some sort of muxing machinery that may not underly the same
> clocking and reset restrictions as the other registers. Anyway, the
> result is that all of the changes done in ->atomic_update() will be lost
> when the CRTC is enabled, and therefore the only case where the
> drm_atomic_helper_commit_planes() call works is when the CRTC stays on
> (essentially what used to be ->mode_set_base()).
> 
> Moreover, I'd say with active_only set to true, the core shouldn't even
> call ->atomic_update() for planes associated with the CRTC because at
> this point it's still off. drm_atomic_helper_commit_modeset_enables() is
> what will turn the CRTC on.
> 
> So all in all, I think what we need is actually five steps:
> 
> 	1) disable all planes that will no longer be used in the new
> 	   state
> 	2) disable all CRTCs that will no longer be used in the new
> 	   state
> 	3) update all planes that have changed from old to new state
> 	4) enable CRTCs that were not used in the old state but are
> 	   enabled in the new state
> 	5) enable all planes that were not used in the old state but
> 	   are enabled in the new state
> 
> 1) and 5) I think could be helpers that are called from drivers directly
> in their ->enable() functions.
> 
> The downside of the above is that there are a bunch of cases where we'd
> need to be very careful to preserve atomicity across updates. For planes
> that are enabled on a CRTC that remains active between two states, they
> would need to be updated in the same ->atomic_begin()/->atomic_flush()
> sequence as planes that move or change framebuffer, otherwise the frames
> won't be perfect.
> 
> Similarly I think we'd want a way to allow drivers to atomically enable
> updates. That is, enabling the output and setting the planes on them
> should be an atomic operation. Currently depending on how fast the mode
> is being set you could have a short period where the mode has been
> applied, but plane updates aren't visible yet. That would essentially
> mean that 1) & 2) as well as 4) & 5) in the above would need to be
> collapsed into single steps, at the end of which the CRTC hardware needs
> to commit all changes at once. I suspect that for most (all?) cases this
> may not even matter, though. If the CRTC is set up to scan out black if
> no planes are active, then this should not be noticable.

On i915 we do a slightly different sequence:
1) kill all the planes for crtcs which will be shut down in step 2). Those
are all the ones for which needs_modeset is true. A helper to do this
could be implemented using the plane_funcs->atomic_disable hook. To make
this nicely atomic we should also wrap them with ->atomic_begin and
->atomic_flush.
2) shut down crtcs/encoders using drm_atomic_helper_commit_modeset_disables
3) enable crtcs/encoders with new config using drm_atomic_helper_commit_modeset_enables
4) do atomic plane updates on _all_ active crtc. This is important since
if you combine a mode change (which forces the crtc to be disabled in 2
and enabled again in 3) you want to first disable all the planes in 1 and
then enable them with the new config in 4. Also if you do a dpms off->on
change your hw likely has lost plane state too, so that needs to be
restored.

One consequence is that step 3 only enables the pipe itself with no planes
displaying at all, even if there are planes enabled on a given crtc.
That's only done in step 4.

Another consequence is that if you want to switch planes between CRTCs and
there's no modeset involved then that would need to be done in step 4. But
since CRTCs usually aren't gen-locked you can't do that atomically (in
general at least). I'd just forbid CRTC switching for planes when the
original CRTC is still active completely, probably not a corner-case we'll
ever care about. I tried to implement that in the revised patch version
but I think I failed.

In that sequence there's no step 3 from your sequence, that's folded in
with the atomic updates in step 4. That means if you do a flip-only on one
crtc and a full modeset on another the flip will be unecessarily stalled,
but that's what you get for piling things up ;-)

Note that my patch only tries to implement step 4) in the above sequence,
step 1 is still tbd. And as Maarten pointed out I failed at implementing
step 4 correctly for all corner cases.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch