On Tue, May 12, 2015 at 02:06:39PM +0200, Maarten Lankhorst wrote: > Op 11-05-15 om 19:11 schreef Daniel Vetter: > > On Mon, May 11, 2015 at 04:24:40PM +0200, Maarten Lankhorst wrote: > >> @@ -6079,26 +6059,29 @@ void intel_crtc_control(struct drm_crtc *crtc, bool enable) > >> enum intel_display_power_domain domain; > >> unsigned long domains; > >> > >> + if (enable == intel_crtc->active) > >> + return; > >> + > >> + if (enable && !crtc->state->enable) > >> + return; > > I think we only need to check for !state->enable here. Changing dpms while > > the crtc is fully of is totally legit. And at least -modesetting loves to > > do just that iirc. > > > > I'll will be caught by state->active implying state->enable, but that's > > hard to read imo. > > -Daniel > As discussed on irc it's not. :-) Hm there's actually a bug in drm_atomic_helper_connector_dpms I think ... we seem to unconditionally update crtc_state->active. Oh I'm missing that ->enable == false also implies that no connectors are connected to the crtc, so we can't ever end up setting this to true. So indeed changing active while enable == false is impossible. With that is it really possible to see have that early return at all? Should we wrap it in a WARN_ON as impossible? -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
