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 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. > > 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 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
