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@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