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
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel