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

[Thierry Reding]

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.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20150727/1cafa0aa/attachment.sig>