On Tue, Jan 20, 2015 at 11:46:57AM +0200, Ville Syrjälä wrote: > On Tue, Jan 20, 2015 at 11:33:22AM +0200, Ander Conselvan de Oliveira wrote: > > On 01/20/2015 11:22 AM, Daniel Vetter wrote: > > > On Mon, Jan 19, 2015 at 03:51:47PM +0200, Ander Conselvan de Oliveira wrote: > > >> @@ -13346,6 +13364,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) > > >> > > >> crtc->base.enabled = crtc->active; > > >> crtc->primary_enabled = primary_get_hw_state(crtc); > > >> + primary_update_size(crtc); > > > > > > I think Ville raised a really good point about the fragility of this and > > > restoring plane state correctly. I think conceptually it makes more sense > > > to restore the primary plane state together with the fb in the loop at the > > > end of intel_modeset_init. Would that still work, or is that too late for > > > when we change pipe state when sanititizing crtcs? > > > > That should work. Actually, Chris sent a patch that did that some time > > ago, and Ville commented that "[he was] thinking that for now these > > would be better placed in intel_modeset_readout_hw_state() where we read > > out the primary plane enabled state as well". [1] > > > > So just to make it completely clear, does the problem of restoring plane > > state correctly supersedes Ville's previous comment? > > Well, I still think intel_modeset_readout_hw_state() is the right place > for this. We would just need to keep the user state and current state > clearly separated (intel_modeset_readout_hw_state() should not touch the > user state). But if that's too hard to do without massive changes, maybe > we can put it somewhere else in the meantime. Well most of the plane state readout for the initial config (which this seems to be a problem with) isn't in that function but in modeset_init. And we currently have no means to check the requested vs the actual state for the plane configuration. Thinking about that we probably don't need that really: The reason for the modeset config checks is to catch bugs with requested vs. actual state, since we can't test anything really automatically. But with plane config changes we can do full functional tests using display CRCs. So from that pov I don't see a big reason to add all that complexity. Long term we should try to consolidate the plane state readout a bit (outside of readout_hw_state perhaps), but for now just adding the code in the modeset_init loop seems best to me. >> > [1] > > http://permalink.gmane.org/gmane.comp.freedesktop.xorg.drivers.intel/46466 In short I disagree with your comment referenced above. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html