[Intel-gfx] [PATCH] drm/atomic: pass old crtc state to atomic_begin/flush.

[Daniel Vetter]

On Mon, Jun 15, 2015 at 11:18:46AM +0200, Maarten Lankhorst wrote:
> Op 15-06-15 om 11:13 schreef Daniel Vetter:
> > On Mon, Jun 15, 2015 at 09:30:19AM +0200, Maarten Lankhorst wrote:
> >> Op 15-06-15 om 09:10 schreef Daniel Vetter:
> >>> On Fri, Jun 12, 2015 at 11:18:22AM +0200, Maarten Lankhorst wrote:
> >>>> In intel it's useful to keep track of some state changes with old
> >>>> crtc state vs new state, for example to disable initial planes or
> >>>> when a modeset's prevented during fastboot.
> >>>>
> >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> >>> Hm, thus far the approach has been that the various ->check callbacks diff
> >>> the state and set appropriate stuff like needs_modeset or planes_changed.
> >>> And with intel_crtc->atomic we've kinda started to build up similar
> >>> things for i915. What do you plan to use this for?
> >>> -Daniel
> >> On a modeset I want to disable all old planes by calling plane->disable_plane, which is old_crtc_state->plane_mask.
> >> This is for initial hw readout, where a plane might be active without a fb set. I want to run it during vblank evasion if possible, which
> >> means in atomic_begin or flush.
> >>
> >> commit_plane is not called if the old and new state both have a NULL fb, so the initial plane would stay active in this case.
> > Hm, so this is for the i915 state readout code. Imo we shouldn't ever leak
> > this out of the state readout code but instead sanitize the plane state to
> > make sense. Roughly this would be:
> > - read out crtc state
> > - try to reconstruct initial fb for primary plane, if this succeeds then
> >   fully link up the plane with the crtc in the plane_state.
> Agreed. Right now get_initial_plane_config takes an initial_plane_state, could we make this atomic too?

The initial fb takeover code is a bit tricky since we need to temporarily
store a few things while not everything is set up yet fully. We could try
to move that information into the plane state, but it would duplicate
existing information stored in state->fb->i915_gem_object. Not sure
whether it's worth it to have something fully atomic for plane state
readout.

The other option would be to allow enabled planes without a full-blown fb
object, but experience says this leads to piles of drama in the watermark
code.
> 
> > - then walk all planes for the crtc, and if any plane is enabled in the hw
> >   state but doesn't have fb/crtc set in the plane_state force-disable it.
> Can we disable those planes without penalty? Some of them call watermark update, this is a bug but still..

What kind of penalty do you think of? Performance doesn't matter since if
we get a bad state and need to frob planes around we'll likely also need a
full modeset anyway.

Wrt watermark updates that needs to be avoided ofc, so similar to the
"force disable all planes in crtc_disable" problem. We might be able to
reuse a bit of code for that.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch