On Tue, Jun 16, 2015 at 11:27:55AM +0300, Ville Syrjälä wrote:
On Mon, Jun 15, 2015 at 03:53:43PM +0200, Daniel Vetter wrote:
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.
We could create some kind of fake fb without an actual gem object. The wm code just needs the metadata.
Yeah I considered that, but we have an awful lot of code that blindly upcasts to the underlying gem bo. Not sure whether that's a lot more solid than just having a NULL fb for code to trip over. But full-blown fb metadata with a NULL obj pointer is indeed an option to keep in mind. -Daniel